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

Added proper authentication for S3 client #16856

Merged
merged 13 commits into from Dec 10, 2020

Conversation

excitoon
Copy link
Contributor

@excitoon excitoon commented Nov 11, 2020

I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en

Changelog category (leave one):

  • Improvement

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

Added proper authentication using environment, ~/.aws and AssumeRole for S3 client.

Global configuration file setting s3.use_environment_credentials turns on attempt to retrieve credentials from environment, and it can be granularly overridden in endpoint/disk setting with same name: use_environment_credentials. If setting is on and credentials can not be retrieved from environment, credentials for given endpoint/disk are being used (possibly anonymous).

Environment credentials are retrieved by following procedure (default for AWS):

  1. environment variables AWS_ACCESS_KEY_ID, AWS_SECRET_ACCESS_KEY, AWS_SESSION_TOKEN;
  2. configuration file in $HOME/.aws;
  3. STS AssumeRole;
  4. ECS task role credentials specified in AWS_CONTAINER_CREDENTIALS_RELATIVE_URI or AWS_CONTAINER_CREDENTIALS_FULL_URI and AWS_ECS_CONTAINER_AUTHORIZATION_TOKEN;
  5. EC2 metadata credentials if AWS_EC2_METADATA_DISABLED is not set to true (in any case).

If credentials are retrieved but they are wrong, ClickHouse will not try another ones.

@robot-clickhouse robot-clickhouse added the pr-improvement Pull request with some product improvements label Nov 11, 2020
@excitoon
Copy link
Contributor Author

In AssumeRole mode S3 client tries to create HTTP client by his own means which leads to crash because in some code I expect HTTP client to be an instance of specific subclass:

2020.11.11 15:13:40.626835 [ 49 ] {} <Fatal> BaseDaemon: 6. /build/obj-x86_64-linux-gnu/../src/IO/S3/PocoHTTPClient.cpp:83: DB::S3::PocoHTTPClient::PocoHTTPClient(DB::S3::PocoHTTPClientConfiguration const&) @ 0x1aa01b29 in /usr/bin/clickhouse
2020.11.11 15:13:40.627556 [ 49 ] {} <Fatal> BaseDaemon: 7. /build/obj-x86_64-linux-gnu/../contrib/libcxx/include/memory:4094: DB::S3::PocoHTTPClientFactory::CreateHttpClient(Aws::Client::ClientConfiguration const&) const @ 0x1aa0d777 in /usr/bin/clickhouse
2020.11.11 15:13:40.629285 [ 49 ] {} <Fatal> BaseDaemon: 8. Aws::Http::CreateHttpClient(Aws::Client::ClientConfiguration const&) @ 0x1ec2917c in /usr/bin/clickhouse
2020.11.11 15:13:40.630962 [ 49 ] {} <Fatal> BaseDaemon: 9. Aws::Internal::AWSHttpResourceClient::AWSHttpResourceClient(Aws::Client::ClientConfiguration const&, char const*) @ 0x1ec3a953 in /usr/bin/clickhouse
2020.11.11 15:13:40.632738 [ 49 ] {} <Fatal> BaseDaemon: 10. Aws::Internal::AWSHttpResourceClient::AWSHttpResourceClient(char const*) @ 0x1ec3b219 in /usr/bin/clickhouse
2020.11.11 15:13:40.634412 [ 49 ] {} <Fatal> BaseDaemon: 11. Aws::Internal::EC2MetadataClient::EC2MetadataClient(char const*) @ 0x1ec3f75e in /usr/bin/clickhouse
2020.11.11 15:13:40.636064 [ 49 ] {} <Fatal> BaseDaemon: 12. Aws::Config::EC2InstanceProfileConfigLoader::EC2InstanceProfileConfigLoader(std::__1::shared_ptr<Aws::Internal::EC2MetadataClient> const&) @ 0x1ec0d09f in /usr/bin/clickhouse
2020.11.11 15:13:40.637760 [ 49 ] {} <Fatal> BaseDaemon: 13. std::__1::shared_ptr<Aws::Config::EC2InstanceProfileConfigLoader> std::__1::shared_ptr<Aws::Config::EC2InstanceProfileConfigLoader>::allocate_shared<std::__1::allocator<Aws::Config::EC2InstanceProfileConfigLoader> >(std::__1::allocator<Aws::Config::EC2InstanceProfileConfigLoader> const&) @ 0x1ebb327f in /usr/bin/clickhouse

@Akazz Akazz self-assigned this Dec 4, 2020
src/IO/S3Common.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@filimonov filimonov left a comment

Choose a reason for hiding this comment

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

Please add also some short docs in PR description - which auth methods we declare as supported, order how they are checked and link to aws official docs

src/IO/S3Common.cpp Outdated Show resolved Hide resolved
src/Storages/StorageS3.cpp Show resolved Hide resolved
Copy link
Contributor

@Akazz Akazz left a comment

Choose a reason for hiding this comment

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

These things may improve readability, so they really are optional and it is up to you

@@ -85,15 +89,107 @@ class AWSLogger final : public Aws::Utils::Logging::LogSystemInterface
std::unordered_map<String, Poco::Logger *> tag_loggers;
};

class S3CredentialsProviderChain : public Aws::Auth::AWSCredentialsProviderChain
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not simply use a builder function for Aws::Auth::AWSCredentialsProviderChain instead of this inheritance?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because it is protected.

: Aws::Client::AWSAuthV4Signer(
std::make_shared<Aws::Auth::SimpleAWSCredentialsProvider>(credentials),
std::make_shared<S3CredentialsProviderChain>(
Copy link
Contributor

Choose a reason for hiding this comment

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

This constructor would also become less coupled if it consumed Aws::Auth::AWSCredentialsProvider

@excitoon excitoon requested a review from Akazz December 7, 2020 23:28
Copy link
Contributor

@Akazz Akazz left a comment

Choose a reason for hiding this comment

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

Integration tests (thread) for 3dd9554 detects something unexpected with count of minio.list_objects(). I can't say for sure if its absolutely unrelated.

Apart from that above, LGTM.

@excitoon
Copy link
Contributor Author

Checking that in 30b0c38

@excitoon
Copy link
Contributor Author

excitoon commented Dec 10, 2020

Same test failed in #17934 -- it flaps

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-improvement Pull request with some product improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants