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

refactor:write oidc credentials to temp file #164

Merged
merged 1 commit into from
Sep 19, 2023

Conversation

brivu
Copy link
Contributor

@brivu brivu commented Sep 18, 2023

The current version of the orb exports the OIDC credentials to $BASH_ENV.

In cases where multiple profiles are created, some aws commands will use the credentials from $BASH_ENV by default, bypassing any profiles that's specified.

This PR writes the generated OIDC keys to a temporary file that's sourced to complete the configuration process.

@Ian-Rowland-i2
Copy link

Ian-Rowland-i2 commented Sep 19, 2023

The update to write the credentials to disk works fine for Linux based executors. Will it work for Windows executors too ('tmp' does not exist on Windows!)?

Shouldn't there be a test job to prove that this works on Windows?

@Ian-Rowland-i2
Copy link

Ian-Rowland-i2 commented Sep 19, 2023

Also if the configure_role_arn example code is to be believed, it is possible to call aws-cli/setup without a role_arn (supported by the setup documentation which stipulates that the role_arn parameter is not required.)

When called without a role_arn the call to assume_role_with_web_identity is skipped and the temp file is not created.

Then this code will then fail because the temp file is missing?

See #165 to see more details of how the code is currently (not) working with circleci/aws-cli@4.1.1

@brivu brivu force-pushed the refactor/write-keys-to-tmp-file branch from 97cdbc5 to 6edb69c Compare September 19, 2023 17:55
@brivu
Copy link
Contributor Author

brivu commented Sep 19, 2023

The update to write the credentials to disk works fine for Linux based executors. Will it work for Windows executors too ('tmp' does not exist on Windows!)?

Shouldn't there be a test job to prove that this works on Windows?

This is a really good point. Let me take a look at this.

EDIT: I just tested this with this windows executor and it worked. I think the expectation is that bash.exe is the shell that's used. Let me add that to the documentation.

windows:
  machine:
    image: windows-server-2019-vs2019:stable
  shell: bash.exe
  resource_class: windows.medium

@brivu brivu force-pushed the refactor/write-keys-to-tmp-file branch from 6c6d30f to 031664d Compare September 19, 2023 18:12
@brivu brivu force-pushed the refactor/write-keys-to-tmp-file branch from 031664d to f94033f Compare September 19, 2023 18:20
@brivu
Copy link
Contributor Author

brivu commented Sep 19, 2023

When called without a role_arn the call to assume_role_with_web_identity is skipped and the temp file is not created.

Then this code will then fail because the temp file is missing?

@Ian-Rowland-i2 - There are two ways to authenticate with using the aws-cli orb. You can use static "master" keys or you can use OIDC by providing a role-arn

If you choose to not provide a role-arn, you'll need to store the "master" keys as environment variables. Therefore, when the setup command runs and no role-arn is given, it'll jump straight to the configure step using the configure.sh script and use the stored environment variables to configure the keys to a profile.

The temp_file is only created in the assume_role_with_web_identity command and is then sourced in the config.sh. The validation below checks to see if there are already keys stored as environment variables. If there aren't, the temp file is sourced and the keys are pulled from there.

if [ -z "$AWS_CLI_STR_ACCESS_KEY_ID" ] && [ -z "${AWS_CLI_STR_SECRET_ACCESS_KEY}" ]; then 
    temp_file="/tmp/${AWS_CLI_STR_PROFILE_NAME}.keys"
    . "$temp_file"
fi

Thanks for taking a look. Let me know if you have other questions

@brivu brivu merged commit 3e1d0d0 into master Sep 19, 2023
2 checks passed
@brivu brivu deleted the refactor/write-keys-to-tmp-file branch September 19, 2023 20:38
Comment on lines +48 to +50
echo "export AWS_CLI_STR_ACCESS_KEY_ID=\"${AWS_ACCESS_KEY_ID}\""
echo "export AWS_CLI_STR_SECRET_ACCESS_KEY=\"${AWS_SECRET_ACCESS_KEY}\""
echo "export AWS_CLI_STR_SESSION_TOKEN=\"${AWS_SESSION_TOKEN}\""
Copy link

Choose a reason for hiding this comment

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

This is a breaking change for the orb.

Most customers likely relied on the existence of AWS_ACCESS_KEY_ID, AWS_SECRET_ACCESS_KEY, AWS_SESSION_TOKEN. They are standard env variables that AWS clients automatically detect. They are now gone.

Please roll back this PR on the 4.x branch.

Copy link

Choose a reason for hiding this comment

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

Agree this was unexpected and definitely a breaking change.

Copy link

Choose a reason for hiding this comment

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

Yup, this broke our CI build in a very confusing way.

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.

Provide a way for assume_role_with_web_identity to not set AWS_* environment variables in BASH_ENV
6 participants