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

Fix FIPS and global endpoint behavior for S3 ARNs #2370

Merged
merged 2 commits into from May 13, 2021

Conversation

vz10
Copy link
Contributor

@vz10 vz10 commented Apr 28, 2021

No description provided.

@codecov-commenter
Copy link

codecov-commenter commented Apr 28, 2021

Codecov Report

Merging #2370 (c8c0e69) into develop (e6b2581) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2370      +/-   ##
===========================================
+ Coverage    97.03%   97.04%   +0.01%     
===========================================
  Files           59       59              
  Lines        11115    11159      +44     
===========================================
+ Hits         10785    10829      +44     
  Misses         330      330              
Impacted Files Coverage Δ
botocore/client.py 98.68% <100.00%> (+0.03%) ⬆️
botocore/exceptions.py 99.50% <100.00%> (+<0.01%) ⬆️
botocore/utils.py 97.23% <100.00%> (-0.02%) ⬇️
botocore/signers.py 98.61% <0.00%> (+0.46%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e6b2581...c8c0e69. Read the comment docs.

Copy link
Member

@kyleknap kyleknap left a comment

Choose a reason for hiding this comment

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

Looks like a good start. Just had some questions and suggestions.

)

def _validate_global_regions(self, request):
if self._s3_config.get('use_arn_region'):
Copy link
Member

Choose a reason for hiding this comment

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

I'd assume we would want to use self._s3_config.get('use_arn_region', True) here since by default we use the arn region? Otherwise, commands like this no longer work as they used to resolve to the us-east-1 endpoint:

$ aws s3api get-object --bucket arn:aws:s3:us-east-1:123456789012:accesspoint:myendpoint --key foo --region s3-external-1 /tmp/foo

Unsupported configuration when using S3 access-points: Invalid configuration, client region is not a regional endpoint

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure. Looks like here we use opposite logic

if not self._s3_config.get('use_arn_region', False):

Copy link
Member

Choose a reason for hiding this comment

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

I double checked this, the default is True for the S3 client, but for the S3 control client it looks like it defaults to False. I think we need to stick with the default for the S3 client. Mainly if we made the default False here, we would introduce a regression where we would now fast fail if:

  1. The user has AWS_S3_US_EAST_1_REGIONAL_ENDPOINT=regional set (In v2, this environment variable no longer exists and essentially defaults to the regional value).
  2. The user does not set a region.
$ AWS_S3_US_EAST_1_REGIONAL_ENDPOINT=regional aws s3api get-object --bucket arn:aws:s3:us-east-1:123456789012:accesspoint:myendpoint --key foo  /tmp/foo
Unsupported configuration when using S3 access-points: Client is configured to use the global psuedo-region "aws-global". When providing access-point ARNs a regional endpoint must be specified.

I pushed up the update in the latest commit to fix this

botocore/utils.py Outdated Show resolved Hide resolved
botocore/utils.py Outdated Show resolved Hide resolved
botocore/utils.py Outdated Show resolved Hide resolved
botocore/utils.py Outdated Show resolved Hide resolved
botocore/utils.py Outdated Show resolved Hide resolved
@vz10 vz10 force-pushed the s3-arn-fips branch 2 times, most recently from 4b13643 to 1b6b95e Compare April 30, 2021 12:57
@joguSD joguSD removed their request for review May 13, 2021 00:01
@kyleknap
Copy link
Member

I pushed up a commit to address the feedback that I had for the previous commits. Everything else before that looked fine.

Specifically:
* Update S3 ARN validation error messages to be more explicit
* Add test to block new S3 FIPS pseudo-regions
* Block usage of unkonwn FIPS pseudo-regions
* Have use_arn_region checks use True as the default
* Add changelog entries
@kyleknap kyleknap merged commit 6451ae1 into boto:develop May 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants