Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update modsec crs config and template #2197

Merged
merged 2 commits into from
May 31, 2022

Conversation

henkworks
Copy link
Contributor

No description provided.

@henkworks henkworks requested a review from a team as a code owner September 30, 2021 14:30
@CLAassistant
Copy link

CLAassistant commented Sep 30, 2021

CLA assistant check
All committers have signed the CLA.

@puppet-community-rangefinder
Copy link

apache::mod::security is a class

that may have no external impact to Forge modules.

This module is declared in 173 of 578 indexed public Puppetfiles.


These results were generated with Rangefinder, a tool that helps predict the downstream impact of breaking changes to elements used in Puppet modules. You can run this on the command line to get a full report.

Exact matches are those that we can positively identify via namespace and the declaring modules' metadata. Non-namespaced items, such as Puppet 3.x functions, will always be reported as near matches only.

@github-actions
Copy link

This PR has been marked as stale because it has been open for a while and has had no recent activity. If this PR is still important to you please drop a comment below and we will add this to our backlog to complete. Otherwise, it will be closed in 7 days.

@github-actions github-actions bot added the stale label Apr 26, 2022
@henkworks
Copy link
Contributor Author

This PR has been marked as stale because it has been open for a while and has had no recent activity. If this PR is still important to you please drop a comment below and we will add this to our backlog to complete. Otherwise, it will be closed in 7 days.

What a stupid waste of people’s time.

@chelnak
Copy link
Contributor

chelnak commented Apr 26, 2022

Hey @henkworks, we appreciate your feedback and your contribution in this PR.

The stale-bot is one of the tools that helps us manage the hundreds of issues and pull requests that are spread across the large amounts of modules that we support.

It's simply there to help us understand if the issue or PR is still of value to the contributor and others that have joined the conversation.

All that we ask for is a simple acknowledgement to let us know that the issue or PR is still of value and we will make sure it stays in our to-do list.

With regards to this specific pull request, it would be fantastic if you could provide some context in the description so whoever picks it up can understand what you are trying to achieve.

We hope you enjoy the rest of your day!

@ekohl
Copy link
Collaborator

ekohl commented Apr 26, 2022

I agree with @henkworks that stale-bot is user/contributor hostile and have stated so before. The only reason this is stale is that there was no review. It's insulting to (potential) contributors. https://drewdevault.com/2021/10/26/stalebot.html describes it quite well.

@chelnak
Copy link
Contributor

chelnak commented Apr 26, 2022

Thanks for sharing your opinion @ekohl.

I've stated the reasons we are using the stale-bot in the reply above.. we are also a small team so utilities like stale-bot are useful. That said, things can be iterative and can change as we learn. Everything has to start somewhere.

Issues and pull requests are not being closed at the point of being marked. We are just asking for acknowledgement of the status of the issue.

Removing stale-bot from the conversation, this issue clearly has some value to the original contributor therefore we have removed the label and have asked for some more context that will help us with the review.

So let's continue to push this in a positive direction and see what we can do to help @henkworks. 👍

@henkworks
Copy link
Contributor Author

Hi @chelnak

The stale-bot is one of the tools that helps us manage the hundreds of issues and pull requests that are spread across the large amounts of modules that we support.

"helps us manage" seems like a euphemism to me. And in my experience the only "reason" (i.e. bad reason) to automatically close issues is that someone made the measure ("how many open bugs are there" or "the ratio of new to closed bugs" or anything similar) the target. See also Goodhart’s Law.

It's simply there to help us understand if the issue or PR is still of value to the contributor and others that have joined the conversation.
All that we ask for is a simple acknowledgement to let us know that the issue or PR is still of value and we will make sure it stays in our to-do list.

Meaning: unless I am willing to repeatedly (it is repeating, isn’t it? by default an issue on github monitored by the stale-bot becomes stale after 60 days AFAICT) comment for no other reason than to not have it closed by a stupid automatism, it can’t be that important to me.
Have you considered how badly that scales? Assuming I become a moderately active contributor and open an issue or PR every day for two months, I would have to comment on one of these everyday after these two months unless someone else causes activity. If I can sustain the "file one issue/PR per day" rhythm, just imagine what the workload just for keeping these open is after a year!
That is tiring. Personally, I’d probably stop filing issues with any project that has this mechanism enabled because it is a clear indicator that the owner does not care about the issues filed.

With regards to this specific pull request, it would be fantastic if you could provide some context in the description so whoever picks it up can understand what you are trying to achieve.

I think the commit messages are pretty clear:

  • I’m updating the config template
  • I’m removing an obsolete configuration parameter
    There is nothing in particular I’m trying to achieve. I just had to update this for my usecase because what the module is currently shipping is based on something that’s almost a decade old (CRS 2.2.9) and not even shipped with Debian oldoldstable anymore. I thought that in the spirit of "housekeeping" I’d contribute that back to upstream.

It’s IMHO the right thing to do when using FLOSS but experiences such as this one keep occuring and discourage me from doing so again in the future. I know others who feel the same about just closing inactive issues/PRs. Redhat/fedora does that too and I regularly talk with other IT professionals and amateurs who also think this is annoying and, in consequence, don’t bother reporting or contributing because they either have to waste their time in the future making sure their effort is not just thrown away automatically or see their effort being ignored and thrown away by a bot. Which makes me sad.
In that spirit: I won’t be contributing to anything puppet-related anymore until I can be sure this won’t happen to my efforts.

We hope you enjoy the rest of your day!

Thanks, you too.

@binford2k
Copy link
Contributor

In regards to the missing PR comment, the thing that you'll need to understand is that the modules team is expert at writing Puppet code, but not necessarily as much of an expert as you might be on all the technologies that our modules support. That means that understanding of why you're doing a thing is even more important than what you're doing.

I’m updating the config template

From what to what? Why's it being updated? Are you changing something because you like different defaults or is the previous version obsoleted in some way? How did you generate the new template? What defaults does it change? Etc.

I’m removing an obsolete configuration parameter

Is this deprecated or removed? What version did it become obsolete in? Does it change the supported software versions? If so, this change could imply a major or minor semver bump.

Admittedly, we should have provided that feedback when the PR was open. This housekeeping effort right now is helping us get caught up so we realistically can do this on a timely basis and keep up. But without this context, there's no way we can effectively review it.

@github-actions github-actions bot removed the stale label Apr 27, 2022
@henkworks
Copy link
Contributor Author

I’m updating the config template

From what to what? Why's it being updated? Are you changing something because you like different defaults or is the previous version obsoleted in some way? How did you generate the new template? What defaults does it change? Etc.

As I said but apparently did not format correctly so maybe it wasn’t clear which part of my change it related to:
I just had to update this for my usecase because what the module is currently shipping is based on something that’s almost a decade old (CRS 2.2.9) and not even shipped with Debian oldoldstable anymore.

I tried hard not to change any values, and AFAICT so did the CRS maintainers but since the differences between the old version of the config from 2.2.9 from 2012 and the current version 3.3.2 are enormous, some things may have slipped through.
The config for 3.3.2 contains some new features that had been added to the CRS at some points since 2.2.9. The config for 2.2.9 would probably at least partly still work but it doesn’t make use of some of the new features.
I generated the new template from the config file delivered with the CRS version 3.3.2 i.e. https://github.com/coreruleset/coreruleset/blob/v3.3/master/crs-setup.conf.example
I have no overview of what defaults were changed, cursory scanning suggests "none" but I can’t invest the time to check this in detail.

I’m removing an obsolete configuration parameter

Is this deprecated or removed? What version did it become obsolete in? Does it change the supported software versions? If so, this change could imply a major or minor semver bump.

It was removed because it became useless. I’m not quite sure on the distinction between "deprecated" and "removed" and "obsolete" etc.
See coreruleset/coreruleset@7934b7d for reference
It just doesn’t make sense for it to be there anymore since 2016 and CRS version 3 AFAICT because the value is not used anymore.

@ekohl
Copy link
Collaborator

ekohl commented Apr 27, 2022

A breaking change doesn't have to be a problem because I think the next release will already be a major release anyway.

It's testing CentOS-6 while 88d04e9 dropped support for it. Would you mind rebasing the PR?

@henkworks henkworks force-pushed the update_modsec_crs_setup_config branch from 8efef74 to 30d14ac Compare April 28, 2022 13:53
@henkworks
Copy link
Contributor Author

It's testing CentOS-6 while 88d04e9 dropped support for it. Would you mind rebasing the PR?

Done.
I first had some trouble with my setup and did The Wrong Thing, i.e. merged upstream into my branch. I hope I got it fixed correctly afterwards. Let me know if not.

@david22swan
Copy link
Member

@henkworks After a bit of investigation this looks good to me so gonna go ahead and merge.

@david22swan david22swan merged commit 943e55b into puppetlabs:main May 31, 2022
@henkworks henkworks deleted the update_modsec_crs_setup_config branch June 2, 2022 07:53
@ekohl ekohl linked an issue Dec 23, 2022 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

wrong modsec conf in templates/mod/security_crs.conf.erb
6 participants