Allow rich_rules to be specified as a dictionary #40

Sxderp · 2020-03-30T12:48:04Z

When specifying rich rules as a dictionary ipsets and services can be
specified as lists. They will be expanded out by the jinja template into
individual rich rules for the parent zone.

PR progress checklist (to be filled in by reviewers)

Changes to documentation are appropriate (or tick if not required)
Changes to tests are appropriate (or tick if not required)
Reviews completed

What type of PR is this?

Primary type

[build] Changes related to the build system
[chore] Changes to the build process or auxiliary tools and libraries such as documentation generation
[ci] Changes to the continuous integration configuration
[feat] A new feature
[fix] A bug fix
[perf] A code change that improves performance
[refactor] A code change that neither fixes a bug nor adds a feature
[revert] A change used to revert a previous commit
[style] Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc.)

Secondary type

[docs] Documentation changes
[test] Adding missing or correcting existing tests

Does this PR introduce a `BREAKING CHANGE`?

No.

Related issues and/or pull requests

Similar to #39

Describe the changes you're proposing

No functionality is broken. If rich rules are specified as a list then all
the processing is ignore and the list is directly passed into the template,
as before.

This PR is similar to #39 . But it includes added functionality. The ability
for services and ipsets to be expanded, which allows for zone-like
configuration.

The goal of this PR was to allow extending rich rules between different
pillar files. Since lists are not merged (by default) between pillar files,
you are required to list all rich rules in a single file. By using a dictionary
we can properly extend the rules.

Use of the special keys "ipsets" and "services" was added for my
environment in particular, allowing for grouping of services and ipsets
similar to zones. I figured it might get enough traction to warrant
inclusion upstream.

Pillar / config required to test the proposed changes

firewalld:
  enabled: true
  ipset:
    manage: true
    pkg: ipset
  ipsets:
    fail2ban-ssh:
      short: fail2ban-ssh
      description: fail2ban-ssh
      type: 'hash:ip'
      entries:
        - 10.0.0.1
    other-ipset:
      short: other-ipset
      description: other-ipset
      type: 'hash:ip'
      entries:
        - 10.0.0.2
  zones:
    rich_public:
      short: rich_public
      description: "Example"
      rich_rules:
        ssh-rr:
          accept: true
          ipsets:
            - fail2ban-ssh
            - other-ipset
          services:
            - ssh
            - http

Debug log showing how the proposed changes work

Documentation checklist

Updated the README (e.g. Available states).
Updated pillar.example.

Testing checklist

Included in Kitchen (i.e. under state_top).
Covered by new/existing tests (e.g. InSpec, Serverspec, etc.).
Updated the relevant test pillar.

Additional context

Sxderp · 2020-03-30T12:58:03Z

Rebased and pushed. Added feat: to commit and chopped two characters off a comment for lint.

When specifying rich rules as a dictionary ipsets and services can be specified as lists. They will be expanded out by the jinja template into individual rich rules for the parent zone.

myii · 2020-03-30T18:49:47Z

@Sxderp Thanks for the contribution. To make this easier to review, I'd like to add some tests for this. To begin with, for the pillar/config you've shared, can you show how this would be rendered in the XML file? It would also be helpful if you give an example of the existing list method, showing the pillar and the rendered XML. We can then take those and add them to the test pillar, as well as use InSpec to confirm the rendered XML contains the changes we're expecting. Are you OK with doing this?

Sxderp · 2020-03-30T19:32:39Z

I'll provide the XML files. Give me a day or two.

myii · 2020-03-30T19:34:57Z

@Sxderp Thanks, please take your time. I don't need the entire files, just the specific sections related to rich_rules, showing what to expect if using a list versus using a dict.

Sxderp · 2020-03-31T13:35:08Z

With the following pillar (slightly expanded from above, to include the list) I get the below xmls:

firewalld:
  enabled: true
  ipset:
    manage: true
    pkg: ipset

  ipsets:
    fail2ban-ssh:
      short: fail2ban-ssh
      description: fail2ban-ssh
      type: 'hash:ip'
      entries:
        - 10.0.0.1
    other-ipset:
      short: other-ipset
      description: other-ipset
      type: 'hash:ip'
      entries:
        - 10.0.0.2

  zones:
    rich_public:
      short: rich_public
      description: "Example"
      rich_rules:
        ssh-rr:
          accept: true
          ipsets:
            - fail2ban-ssh
            - other-ipset
          services:
            - ssh
            - http

    rich_list_public:
      short: rich_list_public
      description: "Example"
      rich_rules:
        - accept: true
          ipset:
            name: fail2ban-ssh
          service: https
        - accept: true
          ipset:
            name: other-ipset
          service: https

rich_public.xml

<?xml version="1.0" encoding="utf-8"?>
<!--
  This file is managed/generated by salt.
  Do not edit this file manually, it will be overwritten!
  Modify the salt pillar for firewalld instead
-->
<zone>
  <short>rich_public</short>
  <description>Example</description>

  <rule>
    <source ipset="fail2ban-ssh" />
    <service name="ssh" />
    <accept/>    
  </rule>

  <rule>
    <source ipset="fail2ban-ssh" />
    <service name="http" />
    <accept/>    
  </rule>

  <rule>
    <source ipset="other-ipset" />
    <service name="ssh" />
    <accept/>    
  </rule>

  <rule>
    <source ipset="other-ipset" />
    <service name="http" />
    <accept/>    
  </rule>

</zone>

rich_list_public.xml

<?xml version="1.0" encoding="utf-8"?>
<!--
  This file is managed/generated by salt.
  Do not edit this file manually, it will be overwritten!
  Modify the salt pillar for firewalld instead
-->
<zone>
  <short>rich_list_public</short>
  <description>Example</description>

  <rule>
    <source ipset="fail2ban-ssh" />
    <service name="https" />
    <accept/>    
  </rule>

  <rule>
    <source ipset="other-ipset" />
    <service name="https" />
    <accept/>    
  </rule>

</zone>

myii · 2020-03-31T13:37:45Z

@Sxderp Great, can you also do the same for the original list method? That will make it much easier to write the tests.

Sxderp · 2020-03-31T13:46:28Z

Git makes that simple enough.

Seems my patch adds some spacing to the resulting zone file. If necessary I think that can be fixed.

firewalld:
  enabled: true
  ipset:
    manage: true
    pkg: ipset

  ipsets:
    fail2ban-ssh:
      short: fail2ban-ssh
      description: fail2ban-ssh
      type: 'hash:ip'
      entries:
        - 10.0.0.1
    other-ipset:
      short: other-ipset
      description: other-ipset
      type: 'hash:ip'
      entries:
        - 10.0.0.2

  zones:
    rich_list_public_old:
      short: rich_list_public_old
      description: "Example"
      rich_rules:
        - accept: true
          ipset:
            name: fail2ban-ssh
          service: https
        - accept: true
          ipset:
            name: other-ipset
          service: https

rich_list_public_old.xml

<?xml version="1.0" encoding="utf-8"?>
<!--
  This file is managed/generated by salt.
  Do not edit this file manually, it will be overwritten!
  Modify the salt pillar for firewalld instead
-->
<zone>
  <short>rich_list_public_old</short>
  <description>Example</description>
  <rule>
    <source ipset="fail2ban-ssh" />
    <service name="https" />
    <accept/>    
  </rule>
  <rule>
    <source ipset="other-ipset" />
    <service name="https" />
    <accept/>    
  </rule>
</zone>

myii · 2020-03-31T13:48:26Z

@Sxderp Thanks, I'll look into these soon to figure out some tests to add here.

myii · 2020-04-01T17:06:31Z

@Sxderp Got the tests written up and tested locally. Also made the whitespacing fix. Let me just run it in Travis now:

https://travis-ci.org/github/myii/firewalld-formula/builds/669795396

If that works out, I'll push it on top of your commit and we can discuss it further.

myii · 2020-04-01T17:19:53Z

@Sxderp So it worked, the arch-base-latest failure was because of some service tests that I also added. The main parts I'd like your feedback on:

myii@eeb79d3
- The two files have been effectively copied across verbatim, without the extra line breaks.
- We don't want false positives with existing zone files being "changed" due to these.
myii@865fb21
- I'd prefer the macro definition at the top of the file.
- There were two fixes for the extra line breaks covered at the top and bottom of this block.

myii · 2020-04-01T17:24:16Z

@Sxderp Actually, I've just gone ahead and added the two good commits here, so we can link to it in the review tools available in this PR itself. Please feel free to comment, if there's an issue, we can always remove/adjust the commits.

myii · 2020-04-01T17:40:10Z

@Sxderp OK, so the tests have helped uncover an issue with trailing whitespaces:

https://travis-ci.com/github/saltstack-formulas/firewalld-formula/jobs/311605982#L330-L341
- (The errors above that will be fixed with a small adjustment to that I'll make to .rubocop.yml).

So this has helped uncover that there are a number of lines with trailing whitespaces in the zone.xml template. That does need to be fixed in my opinion, so I'll adjust that as well and update the tests.

myii · 2020-04-01T17:50:41Z

OK, that all worked: https://travis-ci.org/github/myii/firewalld-formula/builds/669810643. Force pushing the changes back here.

* saltstack-formulas/firewalld-formula#40

* Checked using myii/ssf-formula#145

Sxderp · 2020-04-02T12:04:20Z

myii/firewalld-formula@eeb79d3
The two files have been effectively copied across verbatim, without the extra line breaks.
We don't want false positives with existing zone files being "changed" due to these.

From my limited knowledge of the testing framework looks good. But I'll defer to you.

myii/firewalld-formula@865fb21
I'd prefer the macro definition at the top of the file.
There were two fixes for the extra line breaks covered at the top and bottom of this block.

Sure. The only reason the macro was near the bottom was it took fewer changes for me to implement what I wanted. Moving to the top is fine.

OK, so the tests have helped uncover an issue with trailing whitespaces:

Interesting. I have my VIM highlight trailing spaces in bright red. Surprised I didn't see anything. Although, if they were there previously doesn't this cause the "False-Positive" issue; same as adding line breaks?

myii · 2020-04-02T12:16:34Z

Interesting. I have my VIM highlight trailing spaces in bright red. Surprised I didn't see anything.

I had the same issue with Vim! I was surprised so I went back to the current master branch but it was there alright. Perhaps Vim ignores trailing whitespace in XML files by default? It does get highlighted by vim-airline, to be fair.

Although, if they were there previously doesn't this cause the "False-Positive" issue; same as adding line breaks?

The difference here is that the trailing whitespace should never have been there in the first place (an error that should be fixed). If the original template had resulted in blank lines, then I would have wanted to maintain that instead with the tests, to avoid false-positives.

So are you OK with this being merged with the additions, @Sxderp?

Sxderp · 2020-04-02T12:18:53Z

So are you OK with this being merged with the additions

Yes. Good to go.

myii · 2020-04-02T12:20:20Z

Thanks for providing this new feature, @Sxderp -- merged.

saltstack-formulas-travis · 2020-04-02T12:28:37Z

🎉 This PR is included in version 0.10.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

* saltstack-formulas/firewalld-formula#40

Sxderp force-pushed the pr-add-dictionary-rich-rules2 branch from 2fb0b9a to cb75fc3 Compare March 30, 2020 12:57

feat: allow rich_rules to be specified as a dict

cd4cec0

When specifying rich rules as a dictionary ipsets and services can be specified as lists. They will be expanded out by the jinja template into individual rich rules for the parent zone.

Sxderp force-pushed the pr-add-dictionary-rich-rules2 branch from cb75fc3 to cd4cec0 Compare March 30, 2020 13:01

myii added a commit to myii/ssf-formula that referenced this pull request Apr 1, 2020

feat(firewalld): relax rubocop Metrics/BlockLength for new tests

060a87b

* saltstack-formulas/firewalld-formula#40

myii mentioned this pull request Apr 1, 2020

feat(firewalld): relax rubocop Metrics/BlockLength for new tests myii/ssf-formula#145

Merged

myii added 4 commits April 1, 2020 19:00

test(zones_spec): check content of rendered zone files

6ebfc6f

fix(zone.xml): adjust whitespacing to pass tests & macro at top of file

d8f0f47

style(zone.xml): remove all trailing whitespaces

204efe5

chore(rubocop): relax Metrics/BlockLength for new tests

c69fd6b

* Checked using myii/ssf-formula#145

myii force-pushed the pr-add-dictionary-rich-rules2 branch from d29b5b5 to c69fd6b Compare April 1, 2020 18:01

myii approved these changes Apr 2, 2020

View reviewed changes

myii merged commit 5c135df into saltstack-formulas:master Apr 2, 2020

saltstack-formulas-travis added the released label Apr 2, 2020

myii added a commit to myii/ssf-formula that referenced this pull request Apr 2, 2020

feat(firewalld): relax rubocop Metrics/BlockLength for new tests

326baf4

* saltstack-formulas/firewalld-formula#40

myii mentioned this pull request Apr 2, 2020

[BUG] Python3 incompatibilities saltstack-formulas/bind-formula#146

Open

Sxderp deleted the pr-add-dictionary-rich-rules2 branch April 9, 2020 15:12

Allow rich_rules to be specified as a dictionary #40

Allow rich_rules to be specified as a dictionary #40

Uh oh!

Conversation

Sxderp commented Mar 30, 2020 • edited by myii Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR progress checklist (to be filled in by reviewers)

What type of PR is this?

Primary type

Secondary type

Does this PR introduce a BREAKING CHANGE?

Related issues and/or pull requests

Describe the changes you're proposing

Pillar / config required to test the proposed changes

Debug log showing how the proposed changes work

Documentation checklist

Testing checklist

Additional context

Uh oh!

Sxderp commented Mar 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

myii commented Mar 30, 2020

Uh oh!

Sxderp commented Mar 30, 2020

Uh oh!

myii commented Mar 30, 2020

Uh oh!

Sxderp commented Mar 31, 2020

Uh oh!

myii commented Mar 31, 2020

Uh oh!

Sxderp commented Mar 31, 2020

Uh oh!

myii commented Mar 31, 2020

Uh oh!

myii commented Apr 1, 2020

Uh oh!

myii commented Apr 1, 2020

Uh oh!

myii commented Apr 1, 2020

Uh oh!

myii commented Apr 1, 2020

Uh oh!

myii commented Apr 1, 2020

Uh oh!

Sxderp commented Apr 2, 2020

Uh oh!

myii commented Apr 2, 2020

Uh oh!

Sxderp commented Apr 2, 2020

Uh oh!

myii commented Apr 2, 2020

Uh oh!

saltstack-formulas-travis commented Apr 2, 2020

Uh oh!

Uh oh!

Sxderp commented Mar 30, 2020 •

edited by myii

Loading

Does this PR introduce a `BREAKING CHANGE`?

Sxderp commented Mar 30, 2020 •

edited

Loading