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

Support AWS_CA_BUNDLE when talking to the S3 API #3841

Merged
merged 1 commit into from Feb 17, 2023

Conversation

kirat-singh
Copy link
Contributor

If you are running behind a MITM proxy, this allows for the AWS S3 driver to respect the AWS_CA_BUNDLE environment variable.

@milosgajdos
Copy link
Member

@kirat-singh can you please sign your commit as per the contributor guidelines

@milosgajdos
Copy link
Member

Also, please go fmt your code -- it does not seem to be.

@codecov-commenter
Copy link

codecov-commenter commented Feb 14, 2023

Codecov Report

Base: 56.56% // Head: 56.56% // No change to project coverage 👍

Coverage data is based on head (2bd346c) compared to base (cf87e8d).
Patch has no changes to coverable lines.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3841   +/-   ##
=======================================
  Coverage   56.56%   56.56%           
=======================================
  Files         105      105           
  Lines       10638    10638           
=======================================
  Hits         6017     6017           
  Misses       3948     3948           
  Partials      673      673           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@kirat-singh
Copy link
Contributor Author

apologies, go fmt run, and properly signed off now.

@milosgajdos
Copy link
Member

One more linting issue to fix

@kirat-singh
Copy link
Contributor Author

sorry, found the make lint target now. made sure it's clean.

Comment on lines 295 to 300
if present {
// unset AWS_CA_BUNDLE so AWS session won't raise LoadCustomCABundleError
os.Unsetenv("AWS_CA_BUNDLE")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Clearly the AWS SDK knows about the AWS_CA_BUNDLE environment variable. Why can't we defer to the AWS SDK to handle checking the environment and loading the CA certificate bundle itself?

Copy link
Member

Choose a reason for hiding this comment

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

I would also be in favour of less (or potentially no) code changes if the SDK can pick up the bundle automagically.

Previously we used a custom Transport in order to modify the user agent header.
This prevented the AWS SDK from being able to customize SSL and other client TLS
parameters since it could not understand the Transport type.

Instead we can simply use the SDK function MakeAddToUserAgentFreeFormHandler to
customize the UserAgent if necessary and leave all the TLS configuration to the
AWS SDK.

The only exception being SkipVerify which we have to handle, but we can set it
onto the standard http.Transport which does not interfere with the SDKs ability
to set other options.

Signed-off-by: Kirat Singh <kirat.singh@gmail.com>
@kirat-singh
Copy link
Contributor Author

@milosgajdos @corhere Would love to do that, but here's the rub.

The docker registry code creates it's own custom Transport: github.com/distribution/distribution/v3/registry/client/transport

I didn't write the original code, but I think it's because the AWS SDK doesn't support customization of InsecureSkipVerify or UserAgent on the http client.

So it's creating it's own transport, calling awsConfig.WithHttpClient(&http.Client{Transport: ))

And then it's passing the client off to the S3 session.

In the AWS SDK, the S3 session internally looks to see if AWS_CA_BUNDLE is set in the environment, if so, it will then poke around in the http client and transport, explicitly checking to see if it's subclassing the AWS SDK client.Transport type, and only if it is, will it apply AWS_CA_BUNDLE.

If it's not subclassing that type, and AWS_CA_BUNDLE is present, it blows up with a "unable to load custom CA bundle, HTTPClient's transport unsupported type"

So rather annoying, and it's why I chose to just grab the bits of code to load a CA bundle off AWS_CA_BUNDLE if set and set it onto the transport created in the docker registry.

@kirat-singh
Copy link
Contributor Author

by the way happy to add more comments into the code to explain the above if you think it would be helpful.

@corhere
Copy link
Collaborator

corhere commented Feb 16, 2023

The SDK docs say that the AWS_CA_BUNDLE magic will work as long as the custom HTTP client's transport is a stdlib http.Transport. Our custom transport type is only being used to modify the request User-Agent header with a hook. Guess what's built into the AWS SDK? They've even provided a factory function to construct a handler which mutates the request user-agent! There don't appear to be any major obstacles to enable automagical support for AWS_CA_BUNDLE by switching over to configuring the AWS session with a plain http.Transport. And doing so would also unlock support for AWS_SDK_GO_CLIENT_TLS_KEY/AWS_SDK_GO_CLIENT_TLS_CERT.

@kirat-singh
Copy link
Contributor Author

ooh nice, thanks for calling that out. I'll sort and update the PR.

@kirat-singh
Copy link
Contributor Author

@corhere Thanks for the suggestion. I've updated the PR. Nice and tiny now.

@milosgajdos milosgajdos merged commit e64b08a into distribution:main Feb 17, 2023
@kirat-singh
Copy link
Contributor Author

thank you!

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