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

bugfix: fix the vhost security configuration #2531

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

JGodin-C2C
Copy link

The modified variable was never passed to the template, resulting in a bad configuration of the vhost security if secrule were removed.

templates/vhost/_security.epp Outdated Show resolved Hide resolved
manifests/vhost.pp Show resolved Hide resolved
@JGodin-C2C
Copy link
Author

JGodin-C2C commented Feb 28, 2024

Can we launch the re-failed tests ? seems a problem on the CI side ?

The modified variable was never passed to the template, resulting in a
bad configuration of the vhost security if secrule were removed.

Signed-off-by: Julien Godin <[email protected]>
@JGodin-C2C JGodin-C2C requested a review from ekohl March 5, 2024 07:51
@stevegreengvl
Copy link

Seems to also cause a problem if the modsecure_disable_ids parameter is an Array, as the Hash Array built never gets passed to the "epp" processing

@JGodin-C2C
Copy link
Author

@stevegreengvl I dont get it ,
The _modsec_disable_ids seems to be passed to the Hash "security_params" and then injected into the epp.
Is there anything i missed ?

@stevegreengvl
Copy link

stevegreengvl commented Apr 15, 2024

@JGodin-C2C we have this in our PUppet code
apache::vhost { 'XXX':
...
modsec_disable_ids => [ array of IDs ],
...

Up to Version 11.1.0 of the Module all was fine in the generated vhost Config file, from Version 12.0 the Block below is missing from the generated file

<LocationMatch .*>
SecRuleRemoveById XXXXXX
SecRuleRemoveById YYYYYY

But I think this fix will resolve the issue

Copy link
Collaborator

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

I think this is correct, but it would be nice to have a test for this.

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