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

Move header escaping responsibility from the end user to Puppet #1512

Merged
merged 2 commits into from
Aug 24, 2022

Conversation

smortex
Copy link
Member

@smortex smortex commented Aug 19, 2022

Pull Request (PR) description

More and more security headers are JSON formatted documents. With the hardcoded quotes, we have to think hard not to break the generated NGINX configuration.

Rely on Puppet programmatic string representation to avoid the end user to have to worry about escaping quotes.

This change is backward incompatible

If you manually escaped header contents before passing them to the module, you need to remove the extra escaping, e.g.:

diff --git a/site-modules/profile/functions/security_headers.pp b/site-modules/profile/functions/security_headers.pp
index c6087a2..ba6434d 100644
--- a/site-modules/profile/functions/security_headers.pp
+++ b/site-modules/profile/functions/security_headers.pp
@@ -27,9 +27,9 @@ function profile::security_headers(
     x_content_type_options    => 'nosniff',
     referrer_policy           => 'strict-origin-when-cross-origin',
     permissions_policy        => 'accelerometer=(), ambient-light-sensor=(), autoplay=(), battery=(), camera=(), cross-origin-isolated=(), display-capture=(), document-domain=(), encrypted-media=(), execution-while-not-rendered=(), execution-while-out-of-viewport=(), fullscreen=(), geolocation=(), gyroscope=(), keyboard-map=(), magnetometer=(), microphone=(), midi=(), navigation-override=(), payment=(), picture-in-picture=(), publickey-credentials-get=(), screen-wake-lock=(), sync-xhr=(), usb=(), web-share=(), xr-spatial-tracking=()',
-    nel                       => '{\"report_to\":\"default\",\"max_age\":31536000,\"include_subdomains\":true}',
-    report_to                 => '{\"group\":\"default\",\"max_age\":31536000,\"endpoints\":[{\"url\":\"https://example.com/\"}],\"include_subdomains\":true}',
-    expect_ct                 => 'report-uri=\"https://example.com/\", max-age=86400',
+    nel                       => '{"report_to":"default","max_age":31536000,"include_subdomains":true}',
+    report_to                 => '{"group":"default","max_age":31536000,"endpoints":[{"url":"https://example.com"}],"include_subdomains":true}',
+    expect_ct                 => 'report-uri="https://example.com/", max-age=86400',
     x_xss_protection          => '0',
   } + $options.lest || { {} }
 

This Pull Request (PR) fixes the following issues

n/a

More and more security headers are JSON formatted documents.  With the
hardcoded quotes, we have to think hard not to break the generated NGINX
configuration.

This commit add a unit test to demonstrate the issue.
@kenyon kenyon changed the title Move headers escaping responsability form the end user to Puppet Move header escaping responsibility from the end user to Puppet Aug 19, 2022
@smortex smortex marked this pull request as ready for review August 19, 2022 02:44
Copy link
Member

@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 asked this elsewhere too, but is the StringConverter a public API? Or can we convert the file to EPP and use more native methods?

No need for the end-user to escape quotes anymore!
@smortex
Copy link
Member Author

smortex commented Aug 23, 2022

I asked this elsewhere too, but is the StringConverter a public API? Or can we convert the file to EPP and use more native methods?

I added a similar fix to this PR: switching to EPP would be great but this is a massive change. So for now, just rely on Ruby's String#inspect.

@smortex smortex requested a review from ekohl August 23, 2022 00:13
Copy link
Member

@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.

Just switching this file feels like a small change, but I didn't look at the details.

@smortex
Copy link
Member Author

smortex commented Aug 24, 2022

Just switching this file feels like a small change, but I didn't look at the details.

The backwards-incompatible is probably over-zealous, but people like me who had to manually escape things will have a broken config after this change, so better safe than sorry 😄. I updated the PR description above to describe the needed changes. Will do the same for the other PR because it also has consequences for some use cases.

@smortex smortex merged commit 30b84c8 into master Aug 24, 2022
@smortex smortex deleted the escape-headers branch August 24, 2022 18:59
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.

2 participants