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.

Sign up for GitHub

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

Jump to bottom

Add AWS STS authentication support #1884

Open
wants to merge 17 commits into
base: master
Choose a base branch
from
Open

Add AWS STS authentication support #1884

wants to merge 17 commits into from

Conversation

phoebusm
Copy link
Collaborator

@phoebusm phoebusm commented Oct 8, 2024

Copy of #1814 so @poodlewars can review

Reference Issues/PRs

#1883

What does this implement or fix?

Add support of AWS STS authentication

Any other comments?

STS is an AWS iam service which allows user account to gain access of resources by assuming role.
First, the SDK will send assume role request to iam. If the authentication is successful, iam will reply with a temporary access key. With that access key, SDK will now have access to the resources.
The above logic and the refresh of temporary access key are handled by the SDK.
P.S. the validity check of the temporary access key is made at the beginning of each IO only,

Maintainers of S3 C++ SDK has refused to align this authentication method with other SDKs (e.g. boto3), which create 2 problems:

  1. Role to be assumed is needed to specifed in the API but the role_arn, access key and id are needed to be specifed in the shared config file
  2. The method is not added to the default credential provider chain. Although the SDK allows us to supply a customized chain with the STS authentication method, it has led to two drawbacks, which force me to switch to a dedicated option to switch on STS authentication:
  • SDK will not auto refresh temporary credential. ArcticDB would be required to add a tedious key expiry detection and refresh logic
  • Extra maintainece is required as the chain will need stay align with the default one to align the user experience with previous user experience

Detailed setup of the STS method can be referred to the content of this PR.
The test added in this PR requires real S3. Temporary user, policy and role are created for the test in the pipeline.

The final caveat is seems the config file is loaded at the import of ArcticDB and not being refreshed at the entire lifecycle of the python interpreter. This has required a hack in the testing framework to always create the config file at the beginning of any tests. As it's real S3 test specific, it won't create any pain for day-to-day local development and general PR tests.

Checklist

Checklist for code changes...
  • Have you updated the relevant docstrings, documentation and copyright notice?
  • Is this contribution tested against all ArcticDB's features?
  • Do all exceptions introduced raise appropriate error messages?
  • Are API changes highlighted in the PR description?
  • Is the PR labelled as enhancement or bug so it appears in autogenerated release notes?

poodlewars
poodlewars requested changes Oct 8, 2024
View reviewed changes
cpp/arcticdb/storage/python_bindings.cpp Outdated
.def_property("ssl", &S3Override::ssl, &S3Override::set_ssl);
.def_property("ssl", &S3Override::ssl, &S3Override::set_ssl)
.def_property("aws_auth",
// pybind cannot smartly convert AWSAuthMethod defined in proto from C++ layer to Python layer
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove the comment

cpp/proto/arcticc/pb2/s3_storage.proto Outdated
@@ -27,4 +29,6 @@ message Config {
string ca_cert_dir = 15;
// Whether to leave the prefix unchanged, or to translate the periods in it.
bool use_raw_prefix = 16;
arcticc.pb2.storage_pb2.AWSAuthMethod aws_auth = 17; //default to be 0, DISABLED
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm very worried about adding anything to these protobufs after all the issues recently with the https flag.

There is actually no reason for this to go in the protobuf at all is there? We never intend to serialize it?

We will need extensive compatibility testing (similar to #1862 ) for this change either way.

cpp/arcticdb/storage/s3/s3_storage.cpp
@@ -86,13 +88,17 @@ S3Storage::S3Storage(const LibraryPath &library_path, OpenMode mode, const Confi
root_folder_(object_store_utils::get_root_folder(library_path)),
bucket_name_(conf.bucket_name()),
region_(conf.region()) {

Copy link
Collaborator

Choose a reason for hiding this comment

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

random whitespace

docs/mkdocs/docs/api/arctic_uri.md
@@ -22,11 +22,45 @@ Available options for S3:
| access | S3 access key |
| secret | S3 secret access key |
| path_prefix | Path within S3 bucket to use for data storage |
| aws_auth | If true, authentication to endpoint will be computed via AWS environment vars/config files. If no options are provided `aws_auth` will be assumed to be true. |
| aws_auth | AWS authentication method. If setting is `1` (or `true` for backward compatibility), authentication to endpoint will be computed via [AWS default credential provider chain](https://docs.aws.amazon.com/sdk-for-cpp/v1/developer-guide/credproviders.html). If setting is `2`, AWS Security Token Service (STS) will be the authentication method used. If no options are provided AWS authentication will be assumed to be not used. More info is provided below |
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's use human readable terms for this. default or true for the current behaviour, sts for this new behaviour?

docs/mkdocs/docs/api/arctic_uri.md
@@ -22,11 +22,45 @@ Available options for S3:
| access | S3 access key |
| secret | S3 secret access key |
| path_prefix | Path within S3 bucket to use for data storage |
| aws_auth | If true, authentication to endpoint will be computed via AWS environment vars/config files. If no options are provided `aws_auth` will be assumed to be true. |
| aws_auth | AWS authentication method. If setting is `1` (or `true` for backward compatibility), authentication to endpoint will be computed via [AWS default credential provider chain](https://docs.aws.amazon.com/sdk-for-cpp/v1/developer-guide/credproviders.html). If setting is `2`, AWS Security Token Service (STS) will be the authentication method used. If no options are provided AWS authentication will be assumed to be not used. More info is provided below |
Copy link
Collaborator

Choose a reason for hiding this comment

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

will be assumed to be not used replace with, will not be used and you should specify access and secret in the URI

python/arcticdb/storage_fixtures/s3.py
_PermissionCapableFactory: Type["MotoS3StorageFixtureFactory"] = None # To be set later

logging.getLogger("botocore").setLevel(logging.INFO)

S3_CONFIG_PATH = os.path.expanduser(os.path.join("~", ".aws", "config"))
S3_BACKUP_CONFIG_PATH = os.path.expanduser(os.path.join("~", ".aws", "bk_config"))
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's this for?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A temporary config file for sts credential and profile used in the test

python/arcticdb/storage_fixtures/s3.py


class Key:
def __init__(self, id: str, secret: str, user_name: str):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Make this kwargs only? Its invocations with three strings next to each other are hard to understand

python/arcticdb/storage_fixtures/s3.py
try:
iam_client.create_user(UserName=user_name)
out.sts_test_key = Key(None, None, user_name)
print("User created successfully.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

logging not print statements...

python/arcticdb/storage_fixtures/s3.py
RoleName=role_name,
PolicyArn=out.aws_policy_name
)
print("Policyattached to role successfully.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

typo Policyattached

python/arcticdb/storage_fixtures/s3.py
aws_access_key_id=factory.sts_test_key.id,
aws_secret_access_key=factory.sts_test_key.secret
)
for _ in range(20):
Copy link
Collaborator

Choose a reason for hiding this comment

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

20 retries seems excessive?

Copy link
Collaborator Author

@phoebusm phoebusm Oct 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Per my test, once it requires 13 seconds to get the resources available in S3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@poodlewars poodlewars poodlewars requested changes

@alexowens90 alexowens90 Awaiting requested review from alexowens90 alexowens90 is a code owner

@willdealtry willdealtry Awaiting requested review from willdealtry willdealtry is a code owner

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@phoebusm @poodlewars