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

feat: implement IAM authentication with assumeRoleWithWebIdentityCredentialProvider #118

Merged
merged 1 commit into from
Jul 20, 2021

Conversation

samdyzon
Copy link

Hi all, I've put together a pull request for your consideration.

Description

Authentication with IAM roles in this plugin is limited to two methods: Hard-coding access id and secret, or EC2 metadata query (for instance roles). The change proposed here will allow the plugin to use a third methodology: assumeRoleWithWebIdentityCredentialProvider. The purpose of this change is to enable a Kubernetes pod-level access control to our selected S3 bucket without having to hard-code any credentials. This functionality is discussed in depth here: https://docs.aws.amazon.com/IAM/latest/UserGuide/id_roles_providers_create_oidc.html

Assuming the Pod ServiceAccount has been configured correctly, the Pod will have two environment variables injected: AWS_ROLE_ARN and AWS_WEB_IDENTITY_TOKEN_FILE. When this plugin creates the S3 client via the SDK, we can check if those values exist (after checking for hard-coded access tokens), and use those values to assume our role within the S3 Client.

I am not a PHP developer, so I am sure there is a more idiomatic way of implementing the required logic - but it is just a simple conditional to check the environment variables and create the S3 Client. Most of the code is straight from the developer documentation: https://docs.aws.amazon.com/sdk-for-php/v3/developer-guide/guide_credentials_provider.html#assume-role-with-web-identity-provider.

I have tested this functionality on my own AWS deployment and can confirm that it works, however I don't have any further kind of testing capability. I intend to deploy my fork of this plugin to production until this change has been merged (if it does, that is...). Feel free to let me know if there is anything I can do to help this get merged!

Cheers!

@andris-sevcenko
Copy link
Contributor

@samdyzon I shuffled the code around a little bit. Can you check if this works for you as intended?

https://github.com/craftcms/aws-s3/tree/pullrequests/samdyzon/master

@samdyzon
Copy link
Author

@andris-sevcenko Just tested and can confirm it still works as intended.

@andris-sevcenko andris-sevcenko merged commit 71e1b39 into craftcms:master Jul 20, 2021
@andris-sevcenko
Copy link
Contributor

Thanks for checking that for me. Just cut the 1.2.12 release with this addition!

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.

2 participants