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

Add support for Expect-CT HTTP header #322

Merged
merged 11 commits into from
Aug 25, 2017

Conversation

jacobbednarz
Copy link
Contributor

Expect-CT is a new security header that performs a request to a
certificate transparency log endpoint that will check whether the
presented certificate adhears to the UA's certificate transparency
policy.

Whilst building this out, I made the decision to use the hash
configuration syntax (similar to CSP) instead of the string based syntax
(like HSTS) to ensure we could validate and build various parts of the
value without needing to perform horrible regexes across it.

Fixes #321

@jacobbednarz
Copy link
Contributor Author

I've got this running in our CI suite at the moment and it's all 💚 here

Copy link
Contributor

@oreoshake oreoshake left a comment

Choose a reason for hiding this comment

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

Thanks @jacobbednarz! I've got a few nits (and I'm still digesting the spec). I think we should spell out the name of the spec even if the actual header value uses the abbreviated version.

@@ -51,6 +52,7 @@ def opt_out?
CSP = ContentSecurityPolicy

ALL_HEADER_CLASSES = [
ExpectCt,
Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer to spell out the full name of the header spec.

Copy link
Contributor Author

@jacobbednarz jacobbednarz Apr 3, 2017

Choose a reason for hiding this comment

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

Good call! Fixed via 8e528e2

@@ -116,7 +116,7 @@ def deep_copy_if_hash(value)

attr_writer :hsts, :x_frame_options, :x_content_type_options,
:x_xss_protection, :x_download_options, :x_permitted_cross_domain_policies,
:referrer_policy, :clear_site_data
:referrer_policy, :clear_site_data, :expect_ct
Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer to spell out the full name of the header spec.

class ExpectCt
HEADER_NAME = 'Expect-CT'.freeze
CONFIG_KEY = :expect_ct
INVALID_CONFIGURATION_ERROR = 'config must be a hash.'.freeze
Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer "

Copy link
Contributor Author

@jacobbednarz jacobbednarz Apr 3, 2017

Choose a reason for hiding this comment

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

Oh my! What caveman would ever commit files with mixed quotes?!?! 😛 Fixed in 8ef3f80

def enforced_directive
# Unfortunately `if @enforced` isn't enough here in case someone
# passes in a random string so let's be specific with it to prevent
# accidental enforcement.
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

end

def report_uri_directive
"report-uri=\"#{@report_uri}\"" if @report_uri
Copy link
Contributor

Choose a reason for hiding this comment

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

report-uri is quoted. Interesting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This did strike me as odd too but, you know, specs? ¯\_(ツ)_/¯

@oreoshake
Copy link
Contributor

whoops, can you add an assertion here that says this header is not applied by default.

@jacobbednarz
Copy link
Contributor Author

Of course! Added in c265600.

@jacobbednarz
Copy link
Contributor Author

We've been running this for over a week now and all is looking good on our front 👍

@jacobbednarz
Copy link
Contributor Author

This is pending release from one of the browser vendors to test the functionality confirms to the outlined spec. My comment above was merely, "this isn't blowing up our application with memory leaks or such".

@jacobbednarz
Copy link
Contributor Author

@jacobbednarz
Copy link
Contributor Author

jacobbednarz commented Aug 17, 2017 via email

@oreoshake oreoshake merged commit a1daa24 into github:master Aug 25, 2017
oreoshake pushed a commit that referenced this pull request Aug 25, 2017
* Update README with example usage for `Expect-CT`

* Add some tests for the expected API

* Add classes and loading to main entrypoints

* Add `ExpectCt` class

Adds the `ExpectCt` class and it's associated validation of the header
syntax

* Use full name of specification instead

* Use consistent double quotes

* Update README example to use correct naming for Expect-CT

* Set defaults for Expect-CT via helper

* Add missing comma in README example

* Add frozen string literal
@oreoshake
Copy link
Contributor

Released in 3.7.0

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.

None yet

2 participants