Add HPKP support #132
Add HPKP support #132
Conversation
| pin_directives, | ||
| report_uri_directive, | ||
| subdomain_directive | ||
| ].join.strip |
There was a problem hiding this comment.
nit: can you join(';') instead of prefixing some with "; ..." ?
|
Damn you 1.8.7. I'm ready to drop support and the timing is right given the pending 2.0 release. |
| return unless request.ssl? | ||
| config = self.class.options_for :hpkp, options | ||
|
|
||
| return if config == false || config.nil? |
There was a problem hiding this comment.
nice. can you add a test ensuring this header isn't set by default?
|
I'd love an additional sanity check from someone with experience working with the header. @ptoomey3 ? Did the pin-* value need to be quoted or something? |
| return '' if @pins.nil? | ||
| @pins.collect do |pin| | ||
| pin.map do |token, hash| | ||
| "; pin-#{token}=\"#{hash}\"" if HASH_ALGORITHMS.include?(token) |
There was a problem hiding this comment.
please add a test for the pin values
There was a problem hiding this comment.
@oreoshake - This (\"#{hash}\") is the quoting that is necessary in Firefox 35 and will soon be mandatory in Chrome Canary. 👍
|
If you merge master, the build will pass on 1.8.7 |
| def report_uri_directive | ||
| return '' if @report_uri.nil? | ||
|
|
||
| if @report_uri.start_with?('//') |
There was a problem hiding this comment.
This block seems unneeded. Supporting a scheme relative URL in the config implies the resulting final URL will go over http or https based on some sort of context. But, it will always get set to go over https. I vote that report_uri is documented simply as :report_uri => "http[s]://example.com/uri-directive".
|
I just gave that a quick glance, but left some initial feedback. More generally, I would be hesitant to set this for Chrome, as the current stable implementation has a number of bugs: https://code.google.com/p/chromium/issues/detail?id=444511 The above bugs prevent |
| @@ -8,6 +8,7 @@ The gem will automatically apply several headers that are related to security. | |||
| - X-Content-Type-Options - [Prevent content type sniffing](http://msdn.microsoft.com/en-us/library/ie/gg622941\(v=vs.85\).aspx) | |||
| - X-Download-Options - [Prevent file downloads opening](http://msdn.microsoft.com/en-us/library/ie/jj542450(v=vs.85).aspx) | |||
| - X-Permitted-Cross-Domain-Policies - [Restrict Adobe Flash Player's access to data](https://www.adobe.com/devnet/adobe-media-server/articles/cross-domain-xml-for-streaming.html) | |||
| - Public Key Pinning - Pin certificate fingerprints in the browser to prevent man-in-the-middle attacks due to compromised Certificate Authorites. [Public Key Pinnning Specification](https://tools.ietf.org/html/draft-ietf-websec-key-pinning-20) | |||
There was a problem hiding this comment.
I think draft 21 is the latest.
|
|
||
| it "allows you to specify a report-uri with app_name" do | ||
| should_assign_header(HPKP_HEADER_NAME, "max-age=1234; report-uri=\"https://foobar.com?enforce=true&app_name=my_app\"") | ||
| subject.set_hpkp_header(max_age: 1234, report_uri: "https://foobar.com", app_name: "my_app", tag_report_uri: true, enforce: true) |
There was a problem hiding this comment.
please use the pre-1.9 hash syntax => (sorry, still need to support 1.8.7)
|
@ptoomey3 what say you, is hpkp ready for inclusion? Any browser quirks worth working around? |
|
I haven't actually tested it in Chrome V42 stable, but when I tested Chrome V42 beta the HPKP bug was fixed. So, in all likelihood, yeah, this should be stable enough in Chrome/Firefox to be useful. |
I don't believe so. |
I've added support for HPKP headers now that it is supported in both Chrome and Firefox.