Enable to define if it is preferred to apply default windows rules or not

[snkutlu]

Set to true by default which will install the default firewall rules (as it is now)

[chef-supermarket]

Hi. I am an automated pull request bot named Curry. There are commits in this pull request whose authors are not yet authorized to contribute to Chef Software, Inc. projects or are using a non-GitHub verified email address. To become authorized to contribute, you will need to sign the Contributor License Agreement (CLA) as an individual or on behalf of your company. You can read more on Chef's blog.

Non-GitHub Verified Committers

There are 1 commit author(s) whose commits are authored by a non-GitHub verified email address. Chef will have to manually verify that they are authorized to contribute.

Please sign the CLA here.

[martinb3]

Hello! In addition to the CLA, I'd love to have a test to understand when this should return a failure (your shell_out! change). Thank you!

[snkutlu]

Hello @martinb3

• I should have a proper CLA already signed ..
• A couple of tests are in place for the functionality I added.

About the shell_out change I made, I encountered a chef-client failure when I run firewall cookbook first time on a windows server on which there were no firewall rules defined. delete_all_rules failed with a rc=1.

I can re-write that line with a control of whether any rules defined or not before trying to delete.. So we can get rid of rc=1 situation. Does this sound OK ?

-SNK

[martinb3]

Hi @snkutlu -- that would be great. Either way, I'd like to have a test to be sure we don't break the functionality further, since there's generally been less Windows expertise around this project (at least in my case).

As for the CLA, it must be signed using the same email address that you're using in Github. It looks like you are using an email address currently that even Github is not aware of (see how your username isn't a link at https://github.com/snkutlu/firewall/commit/bbecaa4b522b2556ddda73db0577f377c54effa9).

[snkutlu]

@martinb3 My e-mail address fixed, I think.. I added my company e-mail address as well to github. Can you please verify?

I will work on the change for the Windows part and create the PR asap.

[chef-supermarket]

Hi. Your friendly Curry bot here. Just letting you know that all commit authors have become authorized to contribute. I have added the "Signed CLA" label to this issue so it can easily be found in the future.

[martinb3]

Just to make this easier to read -- what do you think about making a separate recipe in the test fixture cookbook, just for Windows?

[martinb3]

Does a reset not add the default rules back? I'd love to avoid maintaining the Windows default rules in this cookbook.

[snkutlu]

@martinb3, for seperate recipe in test fixture cookbook, I will do this with a seperate test suite and seperate recipe solely for Windows part..

For the "reset" call, indeed it adds the default rules back and what I wanted to implement is to control that behavior under control. The property "default_rules" in firewall_rule provider will by default call this reset function and install default rules. But if one does not want to have these default rules then he/she can set this property to false so at the end of run, there will be only the rules which he / she wanted.

Idea is not to maintain the default windows rules but control whether they will be installed or not.

Hope it is clear now. If you think it will be better, I may add some mode comment in the code to make some explanation.

-SNK

[martinb3]

I think this is very close -- would you mind documenting the new parameter you created? https://github.com/chef-cookbooks/firewall#parameters-1

[martinb3]

Specifically, documenting that it is only for Windows (or renaming it windows_default_rules).

[snkutlu]

@martinb3 I have prepared two new branches for two new features on Windows Firewall and created a complete documentation update for all the changes in the last one but you are right, so I moved related documentation here..

[snkutlu]

go build

[martinb3]

Is this ready to go @snkutlu?

[snkutlu]

@martinb3 for me this feature is ready to go. If you are ok with this one, please merge.

As mentioned, I have two more features for Windows firewall but I did not want to make this PR more complicated :) After merge, I will create a new PR for two new features, or do you prefer seperate PRs?

[martinb3]

@snkutlu this needs to be rebased onto the latest master, and the commits need to be squashed. Would you mind doing that?

Also, do we really want this new parameter on every firewall_rule? It seems like it would be better on the firewall itself. It seems weird to have that defined on every single rule.

[snkutlu]

@martinb3 I will handle this in the weekend and update..

[snkutlu]

@martinb3 property moved to firewall provider (as you suggested which makes sense and logical, thx for pointing) commits squashed

All travis tests passed so ready to be merged, of course if it is OK for you as well.

[martinb3]

It seems like even Microsoft's Powershell resource for the firewall doesn't have this concept of reset -- you just specify exactly what you want: https://github.com/PowerShell/xNetworking/blob/dev/DSCResources/MSFT_xFirewall/MSFT_xFirewall.psm1

[martinb3]

After shopping around this change to various Windows admins and DevOps folks, the general response was that we shouldn't add this as a feature.

[snkutlu]

@martin3b in the link you provided for DSC, it defines functions to be used for firewall management, and in examples directory, there is one sample on how to use this functions to install default firewall rules

I did not see the relation with that feature and the link.

That feature I am trying to add makes an existing function usage in that recipe (reset!) based on a condition: Does the user want to have default rules or not?

As a system administrator, we do not / should not prefer default firewall rules applied to our systems. Security wise, block all then enable what you want is a good approach.

I am also fine if you want this "reset!" function to be completely removed from line 75. Then this will be a breaking change and will not be backward compatible.

I feel like, we are getting back and forth in the commits.With your comment on Feb 22, I thought we are OK with the commits and only a rebase and squash will be enough. But 1 week after that is done, I get comments on the commits again. Just my feelings.

By the way, not accepting this feature will not remove your concerns about: "I'm told this drops the network; this seems dangerous just to get some default rules."

[martinb3]

I apologize for confusing things -- when I reviewed this PR yesterday, I reviewed it as a whole. I withheld some comments earlier because it needed rebasing, and I didn't want to review a diff that wasn't actually against master; if I'd reviewed all of it then, I would have made suggestions that might not even have been relevant.

You appear to be introducing logic to reset servers that might have been tampered with, and I don't want to have to maintain that logic in this cookbook. This cookbook used to parse firewall rules, and it was very difficult to maintain.

I'll go through these commits and merge them into master, but I want to avoid the rule counting code, if possible.

[snkutlu]

Hi @martinb3

I was away from keyboard for a while due to some family health issues and I am back.

I will try to find a way to eliminate counting on rules.. And come with a solution for this.

[snkutlu]

Hi @martinb3 I want to close this PR and restructure and re-create a new one.

The functionality in the base code (before my PR) has some issues when adding / updating large number of rules: The server gets all its connections dropped which is not wanted, for sure.. I started to use this on quite number of windows servers and had this issue on them.

So I started to work on this behaviour and will come up with a new PR, hope very soon.

Thanks for all your support for this PR.