From bc62410d6ea037e46b9ef0a11b8aae76452276ab Mon Sep 17 00:00:00 2001 From: Florian Wininger Date: Tue, 19 Jul 2016 17:52:13 +0200 Subject: [PATCH 1/5] Disallow request in the future. --- lib/api_auth/base.rb | 7 ++++--- spec/railtie_spec.rb | 8 ++++++++ 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/lib/api_auth/base.rb b/lib/api_auth/base.rb index 71d6643a..ab99b2cf 100644 --- a/lib/api_auth/base.rb +++ b/lib/api_auth/base.rb @@ -41,7 +41,7 @@ def authentic?(request, secret_key, options = {}) false elsif !signatures_match?(headers, secret_key, options) false - elsif request_too_old?(headers) + elsif !check_request_date?(headers) false else true @@ -71,10 +71,11 @@ def generate_secret_key AUTH_HEADER_PATTERN = /APIAuth(?:-HMAC-(MD[245]|SHA(?:1|224|256|384|512)?))? ([^:]+):(.+)$/ - def request_too_old?(headers) + def check_request_date?(headers) # 900 seconds is 15 minutes - Time.httpdate(headers.timestamp).utc < (Time.now.utc - 900) + Time.httpdate(headers.timestamp).utc > (Time.now.utc - 900) && + Time.httpdate(headers.timestamp).utc < (Time.now.utc + 900) rescue ArgumentError true end diff --git a/spec/railtie_spec.rb b/spec/railtie_spec.rb index 9d5b7172..2feff180 100644 --- a/spec/railtie_spec.rb +++ b/spec/railtie_spec.rb @@ -77,6 +77,14 @@ def generated_response(request, action = :index) expect(response.code).to eq('401') end + it 'should forbid a request with properly signed headers but timestamp > 15 minutes' do + request = ActionController::TestRequest.new + request.env['DATE'] = 'Mon, 23 Jan 2100 03:29:56 GMT' + ApiAuth.sign!(request, '1044', API_KEY_STORE['1044']) + response = generated_response(request, :index) + expect(response.code).to eq('401') + end + it "should insert a DATE header in the request when one hasn't been specified" do request = ActionController::TestRequest.new ApiAuth.sign!(request, '1044', API_KEY_STORE['1044']) From 2a0aef129353fdb70ef57785bded50e582e19f89 Mon Sep 17 00:00:00 2001 From: Kevin Glowacz Date: Thu, 27 Oct 2016 11:26:44 -0500 Subject: [PATCH 2/5] Rename method slightly and add more context to test description --- lib/api_auth/base.rb | 4 ++-- spec/railtie_spec.rb | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/api_auth/base.rb b/lib/api_auth/base.rb index ab99b2cf..399ba073 100644 --- a/lib/api_auth/base.rb +++ b/lib/api_auth/base.rb @@ -41,7 +41,7 @@ def authentic?(request, secret_key, options = {}) false elsif !signatures_match?(headers, secret_key, options) false - elsif !check_request_date?(headers) + elsif !request_within_time_window?(headers) false else true @@ -71,7 +71,7 @@ def generate_secret_key AUTH_HEADER_PATTERN = /APIAuth(?:-HMAC-(MD[245]|SHA(?:1|224|256|384|512)?))? ([^:]+):(.+)$/ - def check_request_date?(headers) + def request_within_time_window?(headers) # 900 seconds is 15 minutes Time.httpdate(headers.timestamp).utc > (Time.now.utc - 900) && diff --git a/spec/railtie_spec.rb b/spec/railtie_spec.rb index 2feff180..80df3757 100644 --- a/spec/railtie_spec.rb +++ b/spec/railtie_spec.rb @@ -69,7 +69,7 @@ def generated_response(request, action = :index) expect(response.code).to eq('200') end - it 'should forbid a request with properly signed headers but timestamp > 15 minutes' do + it 'should forbid a request with properly signed headers but timestamp > 15 minutes ago' do request = ActionController::TestRequest.new request.env['DATE'] = 'Mon, 23 Jan 1984 03:29:56 GMT' ApiAuth.sign!(request, '1044', API_KEY_STORE['1044']) @@ -77,7 +77,7 @@ def generated_response(request, action = :index) expect(response.code).to eq('401') end - it 'should forbid a request with properly signed headers but timestamp > 15 minutes' do + it 'should forbid a request with properly signed headers but timestamp > 15 minutes in the future' do request = ActionController::TestRequest.new request.env['DATE'] = 'Mon, 23 Jan 2100 03:29:56 GMT' ApiAuth.sign!(request, '1044', API_KEY_STORE['1044']) From b9c714c218dea8e3af2331336764d1a9eacd536d Mon Sep 17 00:00:00 2001 From: Kevin Glowacz Date: Fri, 28 Oct 2016 15:05:05 -0500 Subject: [PATCH 3/5] Make authentic? specs fail for the right reason where previously the failures were all due to signature mismatch only --- spec/api_auth_spec.rb | 28 +++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/spec/api_auth_spec.rb b/spec/api_auth_spec.rb index 122801b4..ba165ab9 100644 --- a/spec/api_auth_spec.rb +++ b/spec/api_auth_spec.rb @@ -76,38 +76,40 @@ def hmac(secret_key, request, canonical_string = nil, digest = 'sha1') describe '.authentic?' do let(:request) do - new_request = Net::HTTP::Put.new('/resource.xml?foo=bar&bar=foo', - 'content-type' => 'text/plain', - 'content-md5' => '1B2M2Y8AsgTpgAmY7PhCfg==', - 'date' => Time.now.utc.httpdate - ) + Net::HTTP::Put.new('/resource.xml?foo=bar&bar=foo', + 'content-type' => 'text/plain', + 'content-md5' => '1B2M2Y8AsgTpgAmY7PhCfg==', + 'date' => Time.now.utc.httpdate + ) + end - signature = hmac('123', new_request) - new_request['Authorization'] = "APIAuth 1044:#{signature}" - new_request + let(:signed_request) do + signature = hmac('123', request) + request['Authorization'] = "APIAuth 1044:#{signature}" + request end it 'validates that the signature in the request header matches the way we sign it' do - expect(ApiAuth.authentic?(request, '123')).to eq true + expect(ApiAuth.authentic?(signed_request, '123')).to eq true end it 'fails to validate a non matching signature' do - expect(ApiAuth.authentic?(request, '456')).to eq false + expect(ApiAuth.authentic?(signed_request, '456')).to eq false end it 'fails to validate non matching md5' do request['content-md5'] = '12345' - expect(ApiAuth.authentic?(request, '123')).to eq false + expect(ApiAuth.authentic?(signed_request, '123')).to eq false end it 'fails to validate expired requests' do request['date'] = 16.minutes.ago.utc.httpdate - expect(ApiAuth.authentic?(request, '123')).to eq false + expect(ApiAuth.authentic?(signed_request, '123')).to eq false end it 'fails to validate if the date is invalid' do request['date'] = "٢٠١٤-٠٩-٠٨ ١٦:٣١:١٤ +٠٣٠٠" - expect(ApiAuth.authentic?(request, '123')).to eq false + expect(ApiAuth.authentic?(signed_request, '123')).to eq false end From a605b546072a71169deaffc1ee7f9483e2679282 Mon Sep 17 00:00:00 2001 From: Kevin Glowacz Date: Fri, 28 Oct 2016 15:07:57 -0500 Subject: [PATCH 4/5] Fix incorrect behaviour introduced with the request_too_old name change --- lib/api_auth/base.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/api_auth/base.rb b/lib/api_auth/base.rb index 399ba073..b618bb2c 100644 --- a/lib/api_auth/base.rb +++ b/lib/api_auth/base.rb @@ -77,7 +77,7 @@ def request_within_time_window?(headers) Time.httpdate(headers.timestamp).utc > (Time.now.utc - 900) && Time.httpdate(headers.timestamp).utc < (Time.now.utc + 900) rescue ArgumentError - true + false end def signatures_match?(headers, secret_key, options) From 3a8d792d949a95a17ae7fa71bb7b00eab7cea3dc Mon Sep 17 00:00:00 2001 From: Kevin Glowacz Date: Fri, 28 Oct 2016 15:08:36 -0500 Subject: [PATCH 5/5] Add new test for far future requests --- spec/api_auth_spec.rb | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/spec/api_auth_spec.rb b/spec/api_auth_spec.rb index ba165ab9..fe3ecc09 100644 --- a/spec/api_auth_spec.rb +++ b/spec/api_auth_spec.rb @@ -107,6 +107,11 @@ def hmac(secret_key, request, canonical_string = nil, digest = 'sha1') expect(ApiAuth.authentic?(signed_request, '123')).to eq false end + it 'fails to validate far future requests' do + request['date'] = 16.minutes.from_now.utc.httpdate + expect(ApiAuth.authentic?(signed_request, '123')).to eq false + end + it 'fails to validate if the date is invalid' do request['date'] = "٢٠١٤-٠٩-٠٨ ١٦:٣١:١٤ +٠٣٠٠" expect(ApiAuth.authentic?(signed_request, '123')).to eq false