From 3e00562d922d623bd42375c9085498b76285fb7e Mon Sep 17 00:00:00 2001 From: Alex Wood Date: Thu, 29 Oct 2015 14:01:04 -0700 Subject: [PATCH 1/4] SigV4 as Default for All Amazon S3 Calls Remove hacks that supported 'S3 Signer' default for many common code paths. Now, SigV4 is always the default and special fallback logic is removed. You can still use the legacy signer, without special fallbacks, by specifying the signature version in your client: `client = Aws::S3::Client.new(signature_version: 's3')` --- aws-sdk-core/features/s3/step_definitions.rb | 2 + .../aws-sdk-core/plugins/s3_request_signer.rb | 128 +++--------------- .../aws/s3/client/region_detection_spec.rb | 70 +--------- aws-sdk-core/spec/aws/s3/client_spec.rb | 40 +----- 4 files changed, 27 insertions(+), 213 deletions(-) diff --git a/aws-sdk-core/features/s3/step_definitions.rb b/aws-sdk-core/features/s3/step_definitions.rb index b8a70841d2d..9ba753dda6c 100644 --- a/aws-sdk-core/features/s3/step_definitions.rb +++ b/aws-sdk-core/features/s3/step_definitions.rb @@ -14,7 +14,9 @@ break if objects.empty? @client.delete_objects(bucket: bucket, delete: { objects: objects }) end + puts "MADE IT TO DELETING BUCKET" @client.delete_bucket(bucket: bucket) + puts "DELETED BUCKET" end end diff --git a/aws-sdk-core/lib/aws-sdk-core/plugins/s3_request_signer.rb b/aws-sdk-core/lib/aws-sdk-core/plugins/s3_request_signer.rb index f34e555a39b..277c323ab57 100644 --- a/aws-sdk-core/lib/aws-sdk-core/plugins/s3_request_signer.rb +++ b/aws-sdk-core/lib/aws-sdk-core/plugins/s3_request_signer.rb @@ -6,28 +6,15 @@ module Plugins # @api private class S3RequestSigner < Seahorse::Client::Plugin - class SigningHandler < RequestSigner::Handler + option :signature_version, 'v4' - # List of regions that support older S3 signature versions. - # All new regions only support signature version 4. - V2_REGIONS = Set.new(%w( - us-east-1 - us-west-1 - us-west-2 - ap-northeast-1 - ap-southeast-1 - ap-southeast-2 - sa-east-1 - eu-west-1 - us-gov-west-1 - )) + class SigningHandler < RequestSigner::Handler def call(context) require_credentials(context) - version = signature_version(context) - case version - when /v4/ then apply_v4_signature(context) - when /s3/ then apply_v2_signature(context) + case context.config.signature_version + when 'v4' then apply_v4_signature(context) + when 's3' then apply_s3_legacy_signature(context) else raise "unsupported signature version #{version.inspect}" end @handler.call(context) @@ -42,59 +29,10 @@ def apply_v4_signature(context) ).sign(context.http_request) end - def apply_v2_signature(context) + def apply_s3_legacy_signature(context) Signers::S3.sign(context) end - def signature_version(context) - context[:cached_signature_version] || - context.config.signature_version || - version_by_region(context) - end - - def version_by_region(context) - if classic_endpoint?(context) - classic_sigv(context) - else - regional_sigv(context) - end - end - - def classic_endpoint?(context) - context.config.region == 'us-east-1' - end - - # When accessing the classic endpoint, s3.amazonaws.com, we don't know - # the region name. This makes creating a version 4 signature difficult. - # Choose v4 only if using KMS encryptions, which requires v4. - def classic_sigv(context) - if kms_encrypted?(context) - :v4 - else - :s3 - end - end - - def regional_sigv(context) - # Drop back to older S3 signature version when uploading objects for - # better performance. This optimization may be removed at some point - # in favor of always using signature version 4. - if V2_REGIONS.include?(context.config.region) - uploading_file?(context) && !kms_encrypted?(context) ? :s3 : :v4 - else - :v4 - end - end - - def kms_encrypted?(context) - context.params[:server_side_encryption] == 'aws:kms' - end - - def uploading_file?(context) - [:put_object, :upload_part].include?(context.operation_name) && - context.http_request.body.size > 0 - end - end # Abstract base class for the other two handlers @@ -143,8 +81,6 @@ def use_regional_endpoint_when_known(context, bucket) # request against the regional endpoint. class BucketSigningErrorHandler < Handler - SIGV4_MSG = /(Please use AWS4-HMAC-SHA256|AWS Signature Version 4)/ - def call(context) response = @handler.call(context) handle_region_errors(response) @@ -153,52 +89,34 @@ def call(context) private def handle_region_errors(response) - if sigv4_required_error?(response) - detect_region_and_retry(response) - elsif wrong_sigv4_region?(response) - extract_body_region_and_retry(response.context) + if wrong_sigv4_region?(response) + get_region_and_retry(response.context) else response end end - def sigv4_required_error?(resp) - resp.context.http_response.status_code == 400 && - resp.context.http_response.body_contents.match(SIGV4_MSG) && - resp.context.http_response.body.respond_to?(:truncate) - end - - def wrong_sigv4_region?(resp) - resp.context.http_response.status_code == 400 && - resp.context.http_response.body_contents.match(/.+?<\/Region>/) - end - - def extract_body_region_and_retry(context) + def get_region_and_retry(context) actual_region = region_from_body(context) - updgrade_to_v4(context, actual_region) + if actual_region.nil? || actual_region == "" + raise "Couldn't get region from body: #{context.body}" + end + update_bucket_cache(context, actual_region) log_warning(context, actual_region) + update_region_header(context, actual_region) @handler.call(context) end - def region_from_body(context) - context.http_response.body_contents.match(/(.+?)<\/Region>/)[1] + def update_bucket_cache(context, actual_region) + S3::BUCKET_REGIONS[context.params[:bucket]] = actual_region end - def detect_region_and_retry(resp) - context = resp.context - updgrade_to_v4(context, 'us-east-1') - resp = @handler.call(context) - if resp.successful? - resp - else - actual_region = region_from_location_header(context) - updgrade_to_v4(context, actual_region) - log_warning(context, actual_region) - @handler.call(context) - end + def wrong_sigv4_region?(resp) + resp.context.http_response.status_code == 400 && + resp.context.http_response.body_contents.match(/.+?<\/Region>/) end - def updgrade_to_v4(context, region) + def update_region_header(context, region) context.http_response.body.truncate(0) context.http_request.headers.delete('authorization') context.http_request.headers.delete('x-amz-security-token') @@ -207,13 +125,11 @@ def updgrade_to_v4(context, region) signer.sign(context.http_request) end - def region_from_location_header(context) - location = context.http_response.headers['location'] - location.match(/s3[.-](.+?)\.amazonaws\.com/)[1] + def region_from_body(context) + context.http_response.body_contents.match(/(.+?)<\/Region>/)[1] end def log_warning(context, actual_region) - S3::BUCKET_REGIONS[context.params[:bucket]] = actual_region msg = "S3 client configured for #{context.config.region.inspect} " + "but the bucket #{context.params[:bucket].inspect} is in " + "#{actual_region.inspect}; Please configure the proper region " + diff --git a/aws-sdk-core/spec/aws/s3/client/region_detection_spec.rb b/aws-sdk-core/spec/aws/s3/client/region_detection_spec.rb index e312e4963b6..b655bc51f97 100644 --- a/aws-sdk-core/spec/aws/s3/client/region_detection_spec.rb +++ b/aws-sdk-core/spec/aws/s3/client/region_detection_spec.rb @@ -17,72 +17,6 @@ module S3 S3::BUCKET_REGIONS.clear end - describe 'accessing eu-central-1 via classic endpoint and sigv2' do - - before(:each) do - - stub_request(:put, 'https://bucket.s3.amazonaws.com/key'). - to_return(status: [400, 'Bad Request'], body: "\nInvalidRequestThe authorization mechanism you have provided is not supported. Please use AWS4-HMAC-SHA256.164473A33C2BACDFI8hhmfGvN74WPyu6kduBoVhnHPgKATFF4nWlro8kRdpuFxXVgcRgLZ2oTxJFhZMgAn75GqyhUHY=") - - stub_request(:put, 'https://bucket.s3-external-1.amazonaws.com/key'). - to_return(status: [307, 'Temporary Redirect'], headers: { - 'Location' => 'https://bucket.s3.eu-central-1.amazonaws.com/key' - }) - - stub_request(:put, 'https://bucket.s3.eu-central-1.amazonaws.com/key'). - to_return(status: [200, 'Ok']) - - end - - it 'detects the sigv4 error, determines actual region and redirects' do - resp = client.put_object(bucket:'bucket', key:'key', body:'body') - host = resp.context.http_request.endpoint.host - expect(host).to eq('bucket.s3.eu-central-1.amazonaws.com') - end - - it 'uses cached regions to determine the actual region' do - client.put_object(bucket:'bucket', key:'key', body:'body') - client.put_object(bucket:'bucket', key:'key', body:'body') - client.put_object(bucket:'bucket', key:'key', body:'body') - end - - it 'sends a warning to stderr' do - expect($stderr).to receive(:write).with(<<-WARNING.strip + "\n") -S3 client configured for "us-east-1" but the bucket "bucket" is in "eu-central-1"; Please configure the proper region to avoid multiple unecessary redirects and signing attempts - WARNING - client.put_object(bucket:'bucket', key:'key', body:'body') - end - - it 'sends the warning to the logger if configured' do - logger = double('logger').as_null_object - expect(logger).to receive(:warn).with(<<-WARNING.strip + "\n") -S3 client configured for "us-east-1" but the bucket "bucket" is in "eu-central-1"; Please configure the proper region to avoid multiple unecessary redirects and signing attempts - WARNING - client = S3::Client.new(client_opts.merge(logger: logger)) - client.put_object(bucket:'bucket', key:'key', body:'body') - end - - it 'triggers a callback where you can listen for new bucket regions' do - yielded = nil - S3::BUCKET_REGIONS.bucket_added {|*args| yielded = args } - client.put_object(bucket:'bucket', key:'key', body:'body') - expect(yielded).to eq(['bucket', 'eu-central-1']) - end - - it 'triggers a callback where you can listen for new bucket regions' do - yielded = nil - S3::BUCKET_REGIONS.bucket_added {|*args| yielded = args } - client.put_object(bucket:'bucket', key:'key', body:'body') - expect(yielded).to eq(['bucket', 'eu-central-1']) - expect(S3::BUCKET_REGIONS.to_h).to eq('bucket' => 'eu-central-1') - - expect { - S3::BUCKET_REGIONS.bucket_added - }.to raise_error(ArgumentError, 'missing required block') - end - - end - describe 'accessing a bucket in eu-central-1 with wrong region' do before(:each) do @@ -102,7 +36,7 @@ module S3 end - describe 'accessing eu-central-1 bucket using classic endpoitn and wrong sigv4 region' do + describe 'accessing eu-central-1 bucket using classic endpoint and wrong sigv4 region' do before(:each) do @@ -116,7 +50,7 @@ module S3 it 'detects the moved permanently and redirects' do client = S3::Client.new(client_opts.merge( - region: 'us-west-2', signature_version:'v4') + region: 'us-west-2') ) resp = client.put_object(bucket:'bucket', key:'key', body:'body') host = resp.context.http_request.endpoint.host diff --git a/aws-sdk-core/spec/aws/s3/client_spec.rb b/aws-sdk-core/spec/aws/s3/client_spec.rb index 27bc7699261..781665d647c 100644 --- a/aws-sdk-core/spec/aws/s3/client_spec.rb +++ b/aws-sdk-core/spec/aws/s3/client_spec.rb @@ -102,37 +102,6 @@ module S3 "sa-east-1", "eu-west-1", "us-gov-west-1", - ].each do |region| - - it "defaults signature version 4 for #{region}" do - client = Client.new(stub_responses: true, region: region) - resp = client.head_object(bucket:'name', key:'key') - expect(resp.context.http_request.headers['authorization']).to match( - 'AWS4-HMAC-SHA256') - end - - it "falls back on classic S3 signing for #put_object in #{region}" do - client = Client.new(stub_responses: true, region: region) - resp = client.put_object(bucket:'name', key:'key', body:'data') - expect(resp.context.http_request.headers['authorization']).to match( - 'AWS akid:') - end - - it "forces v4 signing when aws:kms used for server side encryption" do - client = Client.new(stub_responses: true, region: region) - resp = client.put_object( - bucket: 'name', - key: 'key', - server_side_encryption: 'aws:kms', - body: 'data' - ) - expect(resp.context.http_request.headers['authorization']).to match( - 'AWS4-HMAC-SHA256') - end - - end - - [ "cn-north-1", "eu-central-1", "unknown-region", @@ -165,14 +134,7 @@ module S3 end end - it "defaults classic s3 signature us-east-1" do - client = Client.new(stub_responses: true, region: 'us-east-1') - resp = client.head_object(bucket:'name', key:'key') - expect(resp.context.http_request.headers['authorization']).to match( - 'AWS akid:') - end - - it "upgrades to signature version 4 when aws:kms used for sse" do + it "uses signature version 4 when aws:kms used for sse" do client = Client.new(stub_responses: true, region: 'us-east-1') resp = client.put_object( bucket: 'name', From 7b3e1c2e0e920de5ee01f05b7f7c25eb8de16769 Mon Sep 17 00:00:00 2001 From: Alex Wood Date: Thu, 12 Nov 2015 09:19:10 -0800 Subject: [PATCH 2/4] Remove Debug Statement from Cucumber Test No longer relevant, simply forgot to delete earlier. --- aws-sdk-core/features/s3/step_definitions.rb | 2 -- 1 file changed, 2 deletions(-) diff --git a/aws-sdk-core/features/s3/step_definitions.rb b/aws-sdk-core/features/s3/step_definitions.rb index 9ba753dda6c..b8a70841d2d 100644 --- a/aws-sdk-core/features/s3/step_definitions.rb +++ b/aws-sdk-core/features/s3/step_definitions.rb @@ -14,9 +14,7 @@ break if objects.empty? @client.delete_objects(bucket: bucket, delete: { objects: objects }) end - puts "MADE IT TO DELETING BUCKET" @client.delete_bucket(bucket: bucket) - puts "DELETED BUCKET" end end From c72f98180c1f9a7cd6076b4d6c2b0d3204b71576 Mon Sep 17 00:00:00 2001 From: Alex Wood Date: Thu, 12 Nov 2015 10:50:14 -0800 Subject: [PATCH 3/4] S3 Signer Error Message Improvement + CR Changes Changes some comments and stylistic points, as well as expanding the "incorrect signer" exception to list the two accepted signer types. --- .../aws-sdk-core/plugins/s3_request_signer.rb | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/aws-sdk-core/lib/aws-sdk-core/plugins/s3_request_signer.rb b/aws-sdk-core/lib/aws-sdk-core/plugins/s3_request_signer.rb index 277c323ab57..e9f1565e7a5 100644 --- a/aws-sdk-core/lib/aws-sdk-core/plugins/s3_request_signer.rb +++ b/aws-sdk-core/lib/aws-sdk-core/plugins/s3_request_signer.rb @@ -6,7 +6,7 @@ module Plugins # @api private class S3RequestSigner < Seahorse::Client::Plugin - option :signature_version, 'v4' + option(:signature_version, 'v4') class SigningHandler < RequestSigner::Handler @@ -15,7 +15,9 @@ def call(context) case context.config.signature_version when 'v4' then apply_v4_signature(context) when 's3' then apply_s3_legacy_signature(context) - else raise "unsupported signature version #{version.inspect}" + else + raise "unsupported signature version #{version.inspect}, valid"\ + " options: 'v4' (default), 's3'" end @handler.call(context) end @@ -75,11 +77,11 @@ def use_regional_endpoint_when_known(context, bucket) end - # This handler detects when a request fails because signature version 4 - # is required but not used. It follows up by making a request to - # determine the correct region, then finally a version 4 signed - # request against the regional endpoint. - class BucketSigningErrorHandler < Handler + # This handler detects when a request fails because of a mismatched bucket + # region. It follows up by making a request to determine the correct + # region, then finally a version 4 signed request against the correct + # regional endpoint. + class BucketRegionErrorHandler < Handler def call(context) response = @handler.call(context) @@ -150,7 +152,7 @@ def log_warning(context, actual_region) handler(SigningHandler, step: :sign) # AFTER signing - handle(BucketSigningErrorHandler, step: :sign, priority: 40) + handle(BucketRegionErrorHandler, step: :sign, priority: 40) end end From c10c1aaaa5b25dbf94d81ab57af116f596b5f501 Mon Sep 17 00:00:00 2001 From: Alex Wood Date: Thu, 12 Nov 2015 10:57:31 -0800 Subject: [PATCH 4/4] Change US Standard Endpoint for Aws::S3 Uses s3.amazonaws.com over s3-external-1.amazonaws.com for the us-east-1 region. --- aws-sdk-core/lib/aws-sdk-core/plugins/s3_request_signer.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/aws-sdk-core/lib/aws-sdk-core/plugins/s3_request_signer.rb b/aws-sdk-core/lib/aws-sdk-core/plugins/s3_request_signer.rb index e9f1565e7a5..f606e39d641 100644 --- a/aws-sdk-core/lib/aws-sdk-core/plugins/s3_request_signer.rb +++ b/aws-sdk-core/lib/aws-sdk-core/plugins/s3_request_signer.rb @@ -45,7 +45,7 @@ class Handler < Seahorse::Client::Handler def new_hostname(context, region) bucket = context.params[:bucket] if region == 'us-east-1' - "#{bucket}.s3-external-1.amazonaws.com" + "#{bucket}.s3.amazonaws.com" else bucket + '.' + URI.parse(EndpointProvider.resolve(region, 's3')).host end