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

Removed code for SecStatusEngine #3157

Closed
wants to merge 35 commits into from
Closed

Removed code for SecStatusEngine #3157

wants to merge 35 commits into from

Conversation

marcstern
Copy link
Contributor

@marcstern marcstern commented May 28, 2024

Removed code for SecStatusEngine.
Directives is still allowed but ignored.

@marcstern marcstern added the 2.x Related to ModSecurity version 2.x label May 28, 2024
@marcstern marcstern requested a review from airween May 28, 2024 15:59
@fzipi
Copy link
Contributor

fzipi commented May 28, 2024

If you remove SecRemoteRules, everything curl could be removed IMHO from modsec (but not mlogc), which eliminates a big dependency.

@fzipi
Copy link
Contributor

fzipi commented May 28, 2024

I think probably splitting this into smaller PRs would be what we want, right? One for SecStatusEngine. Another for SecRemoteRules*?

@airween
Copy link
Member

airween commented May 28, 2024

I agree with @fzipi: we should split this PR into more smaller, but I think it's more important that we have to announce that we will eliminate these functions in next(-next) release.

Perhaps in first step we should add a warning message if someone uses any of them, and (if the user checks the logs after startup) then it can be visible our aim.

Also we should make these eliminations in v3 too, in parallel with v2, I guess.

So I would close this PR without merging - what do you think guys?

@airween
Copy link
Member

airween commented May 28, 2024

Also we should check the CI logs - all builds were fail.

@marcstern
Copy link
Contributor Author

My bad:

  • SecStatusEngine is obsolete as there's no more server to receive this info.
  • SecRemoteRules isn't obsolete as someone may have created a server to deliver config files

So, we indeed must split the PR.
Unless nobody uses SecRemoteRules, we cannot remove it (do we need a poll?).
curl dependency must stay as long as SecRemoteRules is supported.

As SecStatusEngine is already broken, I think there's no problem to remove the code, even without announcing it (in advance), as it doesn't do anything already (except potentially introducing a delay).

@dune73
Copy link
Member

dune73 commented May 29, 2024

I'm not a fan of the remote rules and namely how it was being implemented, but commercial rule vendors do use this and I am sure there are people who host their own rules centrally and then load them on startup. We have to keep this around for the time being.

@marcstern marcstern changed the title Removed code for SecStatusEngine, SecRemoteRules, SecRemoteRulesFailAction Removed code for SecStatusEngine Jun 7, 2024
@marcstern
Copy link
Contributor Author

I re-introduced the code for SecRemoteRules & SecRemoteRulesFailAction

@fzipi
Copy link
Contributor

fzipi commented Jun 9, 2024

Looks like pipeline is failing...

@fzipi
Copy link
Contributor

fzipi commented Jun 12, 2024

Why are the ISSUE_TEMPLATES also being modified in this PR? Maybe move those to a new PR?

Copy link

sonarcloud bot commented Jun 12, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@marcstern marcstern closed this by deleting the head repository Jul 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.x Related to ModSecurity version 2.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants