-
Notifications
You must be signed in to change notification settings - Fork 255
Add HPKP support #132
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 HPKP support #132
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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-21) | ||
|
||
## Usage | ||
|
||
|
@@ -21,6 +22,7 @@ The following methods are going to be called, unless they are provided in a `ski | |
|
||
* `:set_csp_header` | ||
* `:set_hsts_header` | ||
* `:set_hpkp_header` | ||
* `:set_x_frame_options_header` | ||
* `:set_x_xss_protection_header` | ||
* `:set_x_content_type_options_header` | ||
|
@@ -51,6 +53,12 @@ This gem makes a few assumptions about how you will use some features. For exam | |
:img_src => "https:", | ||
:report_uri => '//example.com/uri-directive' | ||
} | ||
config.hpkp = { | ||
:max_age => 60.days.to_i, | ||
:include_subdomains => true, | ||
:report_uri => '//example.com/uri-directive', | ||
:pins => [{:sha256 => 'b5bb9d8014a0f9b1d61e21e796d78dccdf1352f23cd32812f4850b878ae4944c'}] | ||
} | ||
end | ||
|
||
# and then simply include this in application_controller.rb | ||
|
@@ -298,6 +306,24 @@ console.log("will raise an exception if not in script_hashes.yml!") | |
<% end %> | ||
``` | ||
|
||
### Public Key Pins | ||
|
||
``` | ||
config.hpkp = { | ||
max_age: 60.days.to_i, # max_age is a required parameter | ||
include_subdomains: true, # whether or not to apply pins to subdomains | ||
# Per the spec, SHA256 hashes are the only currently supported format. | ||
pins: [ | ||
{sha256: 'b5bb9d8014a0f9b1d61e21e796d78dccdf1352f23cd32812f4850b878ae4944c'}, | ||
{sha256: '73a2c64f9545172c1195efb6616ca5f7afd1df6f245407cafb90de3998a1c97f'} | ||
], | ||
enforce: true, # defaults to false (report-only mode) | ||
report_uri: '//example.com/uri-directive', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You may want to add a note and/or a warn that
So, if you have a pin failure you will not be able to send a report back to the hosting application via There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about something like
@ptoomey3 Do people typically supply a separate host? Do reports go cross-origin? I forget where CSP implementations ended up, but I've had to use a proxy host in the past 😭 to get around this |
||
app_name: 'example', | ||
tag_report_uri: true | ||
} | ||
``` | ||
|
||
### Using with Sinatra | ||
|
||
Here's an example using SecureHeaders for Sinatra applications: | ||
|
@@ -321,6 +347,7 @@ require 'secure_headers' | |
:img_src => "https: data:", | ||
:frame_src => "https: http:.twimg.com http://itunes.apple.com" | ||
} | ||
config.hpkp = false | ||
end | ||
|
||
class Donkey < Sinatra::Application | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,7 +6,7 @@ module Configuration | |
class << self | ||
attr_accessor :hsts, :x_frame_options, :x_content_type_options, | ||
:x_xss_protection, :csp, :x_download_options, :script_hashes, | ||
:x_permitted_cross_domain_policies | ||
:x_permitted_cross_domain_policies, :hpkp | ||
|
||
def configure &block | ||
instance_eval &block | ||
|
@@ -42,6 +42,7 @@ def ensure_security_headers options = {} | |
self.secure_headers_options = options | ||
before_filter :prep_script_hash | ||
before_filter :set_hsts_header | ||
before_filter :set_hpkp_header | ||
before_filter :set_x_frame_options_header | ||
before_filter :set_csp_header | ||
before_filter :set_x_xss_protection_header | ||
|
@@ -61,6 +62,7 @@ module InstanceMethods | |
def set_security_headers(options = self.class.secure_headers_options) | ||
set_csp_header(request, options[:csp]) | ||
set_hsts_header(options[:hsts]) | ||
set_hpkp_header(options[:hpkp]) | ||
set_x_frame_options_header(options[:x_frame_options]) | ||
set_x_xss_protection_header(options[:x_xss_protection]) | ||
set_x_content_type_options_header(options[:x_content_type_options]) | ||
|
@@ -136,6 +138,16 @@ def set_hsts_header(options=self.class.secure_headers_options[:hsts]) | |
set_a_header(:hsts, StrictTransportSecurity, options) | ||
end | ||
|
||
def set_hpkp_header(options=self.class.secure_headers_options[:hpkp]) | ||
return unless request.ssl? | ||
config = self.class.options_for :hpkp, options | ||
|
||
return if config == false || config.nil? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nice. can you add a test ensuring this header isn't set by default? |
||
|
||
hpkp_header = PublicKeyPins.new(config) | ||
set_header(hpkp_header) | ||
end | ||
|
||
def set_x_download_options_header(options=self.class.secure_headers_options[:x_download_options]) | ||
set_a_header(:x_download_options, XDownloadOptions, options) | ||
end | ||
|
@@ -168,6 +180,7 @@ def set_header(name_or_header, value=nil) | |
|
||
require "secure_headers/version" | ||
require "secure_headers/header" | ||
require "secure_headers/headers/public_key_pins" | ||
require "secure_headers/headers/content_security_policy" | ||
require "secure_headers/headers/x_frame_options" | ||
require "secure_headers/headers/strict_transport_security" | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,95 @@ | ||
module SecureHeaders | ||
class PublicKeyPinsBuildError < StandardError; end | ||
class PublicKeyPins < Header | ||
module Constants | ||
HPKP_HEADER_NAME = "Public-Key-Pins" | ||
ENV_KEY = 'secure_headers.public_key_pins' | ||
HASH_ALGORITHMS = [:sha256] | ||
DIRECTIVES = [:max_age] | ||
end | ||
class << self | ||
def symbol_to_hyphen_case sym | ||
sym.to_s.gsub('_', '-') | ||
end | ||
end | ||
include Constants | ||
|
||
def initialize(config=nil) | ||
@config = validate_config(config) | ||
|
||
@pins = @config.fetch(:pins, nil) | ||
@report_uri = @config.fetch(:report_uri, nil) | ||
@app_name = @config.fetch(:app_name, nil) | ||
@enforce = !!@config.fetch(:enforce, nil) | ||
@include_subdomains = !!@config.fetch(:include_subdomains, nil) | ||
@tag_report_uri = !!@config.fetch(:tag_report_uri, nil) | ||
end | ||
|
||
def name | ||
base = HPKP_HEADER_NAME | ||
if !@enforce | ||
base += "-Report-Only" | ||
end | ||
base | ||
end | ||
|
||
def value | ||
header_value = [ | ||
generic_directives, | ||
pin_directives, | ||
report_uri_directive, | ||
subdomain_directive | ||
].compact.join('; ').strip | ||
end | ||
|
||
def validate_config(config) | ||
raise PublicKeyPinsBuildError.new("config must be a hash.") unless config.is_a? Hash | ||
|
||
if !config[:max_age] | ||
raise PublicKeyPinsBuildError.new("max-age is a required directive.") | ||
elsif config[:max_age].to_s !~ /\A\d+\z/ | ||
raise PublicKeyPinsBuildError.new("max-age must be a number. | ||
#{config[:max_age]} was supplied.") | ||
elsif config[:pins] and config[:pins].length < 2 | ||
raise PublicKeyPinsBuildError.new("A minimum of 2 pins are required.") | ||
end | ||
|
||
config | ||
end | ||
|
||
def pin_directives | ||
return nil if @pins.nil? | ||
@pins.collect do |pin| | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As noted in http://tools.ietf.org/html/draft-ietf-websec-key-pinning-21#section-4.3 and http://tools.ietf.org/html/draft-ietf-websec-key-pinning-21#section-2.5:
So, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good catch ill add that check. |
||
pin.map do |token, hash| | ||
"pin-#{token}=\"#{hash}\"" if HASH_ALGORITHMS.include?(token) | ||
end | ||
end.join('; ') | ||
end | ||
|
||
def generic_directives | ||
DIRECTIVES.collect do |directive_name| | ||
build_directive(directive_name) if @config[directive_name] | ||
end.join('; ') | ||
end | ||
|
||
def build_directive(key) | ||
"#{self.class.symbol_to_hyphen_case(key)}=#{@config[key]}" | ||
end | ||
|
||
def report_uri_directive | ||
return nil if @report_uri.nil? | ||
|
||
if @tag_report_uri | ||
@report_uri = "#{@report_uri}?enforce=#{@enforce}" | ||
@report_uri += "&app_name=#{@app_name}" if @app_name | ||
end | ||
|
||
"report-uri=\"#{@report_uri}\"" | ||
end | ||
|
||
|
||
def subdomain_directive | ||
@include_subdomains ? 'includeSubDomains' : nil | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
require 'spec_helper' | ||
|
||
module SecureHeaders | ||
describe PublicKeyPins do | ||
specify{ expect(PublicKeyPins.new(max_age: 1234).name).to eq("Public-Key-Pins-Report-Only") } | ||
specify{ expect(PublicKeyPins.new(max_age: 1234, enforce: true).name).to eq("Public-Key-Pins") } | ||
|
||
specify { expect(PublicKeyPins.new({max_age: 1234}).value).to eq("max-age=1234")} | ||
specify { expect(PublicKeyPins.new(max_age: 1234).value).to eq("max-age=1234")} | ||
specify { | ||
config = {max_age: 1234, pins: [{sha256: 'base64encodedpin1'}, {sha256: 'base64encodedpin2'}]} | ||
header_value = "max-age=1234; pin-sha256=\"base64encodedpin1\"; pin-sha256=\"base64encodedpin2\"" | ||
expect(PublicKeyPins.new(config).value).to eq(header_value) | ||
} | ||
|
||
context "with an invalid configuration" do | ||
it "raises an exception when max-age is not provided" do | ||
expect { | ||
PublicKeyPins.new(foo: 'bar') | ||
}.to raise_error(PublicKeyPinsBuildError) | ||
end | ||
|
||
it "raises an exception with an invalid max-age" do | ||
expect { | ||
PublicKeyPins.new(max_age: 'abc123') | ||
}.to raise_error(PublicKeyPinsBuildError) | ||
end | ||
|
||
it 'raises an exception with less than 2 pins' do | ||
expect { | ||
config = {max_age: 1234, pins: [{sha256: 'base64encodedpin'}]} | ||
PublicKeyPins.new(config) | ||
}.to raise_error(PublicKeyPinsBuildError) | ||
end | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,7 +18,7 @@ class DummyClass | |
allow(subject).to receive(:request).and_return(request) | ||
end | ||
|
||
ALL_HEADERS = Hash[[:hsts, :csp, :x_frame_options, :x_content_type_options, :x_xss_protection, :x_permitted_cross_domain_policies].map{|header| [header, false]}] | ||
ALL_HEADERS = Hash[[:hpkp, :hsts, :csp, :x_frame_options, :x_content_type_options, :x_xss_protection, :x_permitted_cross_domain_policies].map{|header| [header, false]}] | ||
|
||
def stub_user_agent val | ||
allow(request).to receive_message_chain(:env, :[]).and_return(val) | ||
|
@@ -30,6 +30,7 @@ def options_for header | |
|
||
def reset_config | ||
::SecureHeaders::Configuration.configure do |config| | ||
config.hpkp = nil | ||
config.hsts = nil | ||
config.x_frame_options = nil | ||
config.x_content_type_options = nil | ||
|
@@ -42,6 +43,7 @@ def reset_config | |
|
||
def set_security_headers(subject) | ||
subject.set_csp_header | ||
subject.set_hpkp_header | ||
subject.set_hsts_header | ||
subject.set_x_frame_options_header | ||
subject.set_x_content_type_options_header | ||
|
@@ -71,6 +73,7 @@ def set_security_headers(subject) | |
subject.set_csp_header | ||
subject.set_x_frame_options_header | ||
subject.set_hsts_header | ||
subject.set_hpkp_header | ||
subject.set_x_xss_protection_header | ||
subject.set_x_content_type_options_header | ||
subject.set_x_download_options_header | ||
|
@@ -115,6 +118,17 @@ def set_security_headers(subject) | |
subject.set_hsts_header({:include_subdomains => true}) | ||
end | ||
|
||
it "does not set the HPKP header if disabled" do | ||
should_not_assign_header(HPKP_HEADER_NAME) | ||
subject.set_hpkp_header | ||
end | ||
|
||
it "does not set the HPKP header if request is over HTTP" do | ||
allow(subject).to receive_message_chain(:request, :ssl?).and_return(false) | ||
should_not_assign_header(HPKP_HEADER_NAME) | ||
subject.set_hpkp_header(max_age: 1234) | ||
end | ||
|
||
it "does not set the CSP header if disabled" do | ||
stub_user_agent(USER_AGENTS[:chrome]) | ||
should_not_assign_header(HEADER_NAME) | ||
|
@@ -136,6 +150,7 @@ def set_security_headers(subject) | |
it "does not set any headers when disabled" do | ||
::SecureHeaders::Configuration.configure do |config| | ||
config.hsts = false | ||
config.hpkp = false | ||
config.x_frame_options = false | ||
config.x_content_type_options = false | ||
config.x_xss_protection = false | ||
|
@@ -196,6 +211,38 @@ def set_security_headers(subject) | |
end | ||
end | ||
|
||
describe "#set_public_key_pins" do | ||
it "sets the Public-Key-Pins header" do | ||
should_assign_header(HPKP_HEADER_NAME + "-Report-Only", "max-age=1234") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is it valid to set this without at least one pin- entry? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes -- only """According to the processing rules of Section 2.1, the UA MUST ignore There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🆒 , thanks for the pointer. |
||
subject.set_hpkp_header(max_age: 1234) | ||
end | ||
|
||
it "allows you to enforce public key pinning" do | ||
should_assign_header(HPKP_HEADER_NAME, "max-age=1234") | ||
subject.set_hpkp_header(max_age: 1234, enforce: true) | ||
end | ||
|
||
it "allows you to specific a custom max-age value" do | ||
should_assign_header(HPKP_HEADER_NAME + "-Report-Only", 'max-age=1234') | ||
subject.set_hpkp_header(max_age: 1234) | ||
end | ||
|
||
it "allows you to specify includeSubdomains" do | ||
should_assign_header(HPKP_HEADER_NAME, "max-age=1234; includeSubDomains") | ||
subject.set_hpkp_header(max_age: 1234, include_subdomains: true, enforce: true) | ||
end | ||
|
||
it "allows you to specify a report-uri" do | ||
should_assign_header(HPKP_HEADER_NAME, "max-age=1234; report-uri=\"https://foobar.com\"") | ||
subject.set_hpkp_header(max_age: 1234, report_uri: "https://foobar.com", enforce: true) | ||
end | ||
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please use the pre-1.9 hash syntax => (sorry, still need to support 1.8.7) |
||
end | ||
end | ||
|
||
describe "#set_x_xss_protection" do | ||
it "sets the X-XSS-Protection header" do | ||
should_assign_header(X_XSS_PROTECTION_HEADER_NAME, SecureHeaders::XXssProtection::Constants::DEFAULT_VALUE) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This syntax feels a little awkward (could just be me). Given that it would be extremely rare for each pin to use their own hash type, it may make sense to just set it globally:
Or, just bake the hash type into the pin
I tend to prefer the first style, as there is no reason you can't/shouldn't use the same hash format for all pins.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SHA256 is the currently only supported hash algorithm but I wanted this to be flexible in the case it is extended.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I definitely think it should be flexible enough to support alternate hash algos. I just thought the array of hashes syntax was a bit awkward. I'll leave up to @oreoshake since it is purely stylistic preference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LOL
secure_headers
thrives on awkward abuse of theHash
class.But seriously, I like the "array of hashes" syntax more than either suggestion. I never said I was good at API design.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ha...fair enough 😄