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

Add new STSProfileWithWebIdentityCredentialsProvider #4137

Merged

Conversation

Shelnutt2
Copy link
Member

@Shelnutt2 Shelnutt2 commented Jun 19, 2023

This new credential provider support chaining of assume from based on a profile configuration including support for web identity tokens as part of the chain. This is based on the upstream
STSProfileCredentialProvider. The upstream credential provider lacks support for the web identity tokens.

The main use case for this is inside EC2/ECS/EKS environments where a web identity token can be used as part of IRSA sequences.

This is configured for the new config_source via:
cfg["vfs.s3.config_source"] = "sts_profile_with_web_identity"


TYPE: FEATURE
DESC: Support new STSProfileWithWebIdentityCredentialsProvider S3 credential provider

@shortcut-integration
Copy link

This pull request has been linked to Shortcut Story #30586: Add Custom Credentials Provider for STS And Web Identity.

@Shelnutt2 Shelnutt2 force-pushed the sethshelnutt/sc-30586/add-custom-credentials-provider-for-sts branch 2 times, most recently from f92ba78 to 48e5fff Compare June 19, 2023 21:50
Copy link
Contributor

@robertbindar robertbindar left a comment

Choose a reason for hiding this comment

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

Looks good to me, please look if you can minimize leaking s3 specifics into filesystem cmake before merging.

tiledb/sm/filesystem/s3/CMakeLists.txt Outdated Show resolved Hide resolved
tiledb/sm/filesystem/CMakeLists.txt Outdated Show resolved Hide resolved
tiledb/sm/filesystem/s3/CMakeLists.txt Outdated Show resolved Hide resolved
@Shelnutt2 Shelnutt2 force-pushed the sethshelnutt/sc-30586/add-custom-credentials-provider-for-sts branch from 409f552 to b23ccef Compare June 20, 2023 12:14
Copy link
Member

@ihnorton ihnorton left a comment

Choose a reason for hiding this comment

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

Minor comments, but LGTM 👍

endif()
if(WIN32)
if(MSVC)
find_library(BCRYPT_LIBRARY bcrypt)
Copy link
Member

Choose a reason for hiding this comment

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

For future reference, newer AWS SDK versions have a separate list of non-core linkage which includes this, so we should be able to remove this in the future once we have a sufficiently high minimum version.

find_package(AWSSDK_EP REQUIRED COMPONENTS s3)
this_target_link_libraries(INTERFACE ${AWSSDK_LINK_LIBRARIES})
else()
find_package(AWSSDK_EP REQUIRED COMPONENTS s3)
Copy link
Member

Choose a reason for hiding this comment

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

*
* This file implements the S3 Credentials Provider to support STS and Web
* Identity.
*/
Copy link
Member

Choose a reason for hiding this comment

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

Suggest linking to upstream implementation so people can reference it if needed (ie if changes are made there, we may need to reflect them here): https://github.com/aws/aws-sdk-cpp/blob/main/src/aws-cpp-sdk-identity-management/source/auth/STSProfileCredentialsProvider.cpp

Copy link
Member

Choose a reason for hiding this comment

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

We might also want to make a feature request upstream so that this can go away eventually.

Shelnutt2 and others added 7 commits June 21, 2023 19:16
This new credential provider support chaining of assume from based on a
profile configuration including support for web identity tokens as part
of the chain. This is based on the upstream
`STSProfileCredentialProvider`. The upstream credential provider lacks
support for the web identity tokens.

The main use case for this is inside EC2/ECS/EKS environments where a
web identity token can be used as part of IRSA sequences.
This is a workaround until thr curl interfacr correctly handles system
libraries.
This is a temporary workaround. In a followup we will add this back and
correctly setup the filesystem object library. The filesytem object
library had a few issues with s3 that expanded beyond the scope of this
PR.
@Shelnutt2 Shelnutt2 force-pushed the sethshelnutt/sc-30586/add-custom-credentials-provider-for-sts branch from 310e1be to 7bfeed3 Compare June 21, 2023 23:38
@Shelnutt2
Copy link
Member Author

I've rebased, tested and confirmed now that #4131 was merged. This is working with our test case.

@Shelnutt2 Shelnutt2 merged commit daea2b2 into dev Jun 22, 2023
@Shelnutt2 Shelnutt2 deleted the sethshelnutt/sc-30586/add-custom-credentials-provider-for-sts branch June 22, 2023 01:31
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.

3 participants