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

waf integration fixes / improvements #3158

Merged
merged 9 commits into from
Feb 12, 2024

Conversation

electricjesus
Copy link
Member

@electricjesus electricjesus commented Feb 6, 2024

Description

This PR changes how coreruleset rules are delivered as part of the WAF integration

  • add unit testing for WAF render
  • remove embedded coreruleset. this greatly reduces the size of the config map rendered by operator.
  • leaving only one file available by default, tigera.conf
  • change dikastes command line flags to use coraza's direct file loading api
  • only reference coreruleset already installed in dikastes through a 'header' file (tigera.conf)
    • remove -waf-directive flags, swap with -waf-ruleset-file flag
    • remove -waf-ruleset-base-dir because DirFS didn't work as expected

For PR author

  • Tests for change.
  • If changing pkg/apis/, run make gen-files
  • If changing versions, run make gen-versions

For PR reviewers

A note for code reviewers - all pull requests must have the following:

  • Milestone set according to targeted release.
  • Appropriate labels:
    • kind/bug if this is a bugfix.
    • kind/enhancement if this is a a new feature.
    • enterprise if this PR applies to Calico Enterprise only.

Copy link
Member

@peterkellydev peterkellydev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM from WAF perspective and the changes we want to see with ruleset files.

@tmjd
Copy link
Member

tmjd commented Feb 12, 2024

Could you explain a bit further about this?

change dikastes command line flags to use coraza's direct file loading api

Primarily I'm wondering if this has impact on clusters that don't have internet access.

Another way to ask what I'm curious about is, how does dikastes fetch the ruleset data that is being removed in this PR?

Another thing I'm curious about, does this change what happens during upgrades? I think before this PR the ruleset gets installed/cached and then with future updates the existing ruleset would be used (I'm not sure about this behavior but it would match the behavior I usually expect with the operator). My thinking is that now if dikastes is fetching this data (or it is builtin) then I would think the data could change/update when upgrading.

@electricjesus
Copy link
Member Author

@tmjd sorry for the confusion, I typically call it API because it's an 'application programming interface' within the code.. not necessarily an API endpoint.

in this case I meant to say that it that just means that in dikastes we're just loading the file directly instead of using a Include <path-to-file> directive. e.g.:

from

-waf-directive "Include /etc/modsecurity-ruleset/tigera.conf"

to

-waf-ruleset-file /etc/modsecurity-ruleset/tigera.conf
  • tigera.conf is still a file mounted via config map, then used in that CLI flag
  • the rest of the core ruleset files are embedded within the dikastes binary, see here:
  • nothing is loaded via internet access

@tmjd
Copy link
Member

tmjd commented Feb 12, 2024

I discussed this a bit with @electricjesus and the intention is that when a user upgrades the ruleset will be updated along with the upgrade.

Copy link
Member

@tmjd tmjd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@electricjesus electricjesus merged commit cfbd5b9 into tigera:master Feb 12, 2024
5 checks passed
@electricjesus electricjesus deleted the fix-coraza-integration branch February 12, 2024 22:19
@electricjesus electricjesus mentioned this pull request Feb 21, 2024
5 tasks
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.

4 participants