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 support for STS-issued AWS Credentials #4603

Merged
merged 5 commits into from
Feb 9, 2024
Merged

Conversation

tariqajyusuf
Copy link
Contributor

Closes #4596

Description Of Changes

This change adds an option to pass session_token to the CLI, UI, or API when using the system discovery feature for AWS. Long-term credentials only require an access_key_id and secret_access_key. As it stands today, this means that long-term credentials have to be created to use any system discovery. By adding an option to use session_tokens, we now allow for temporary credentials that can reflect the principle of least access.

Code Changes

  • Update Python and React data models to include aws_session_token
  • Add --session_token as an optional argument to fides generate and fides scan
  • Add optional session token field to AWS discovery.

Steps to Confirm

After setting up the dev environment

  • Provision temporary credentials (e.g. aws sts assume-role ...)
  • Attempt to use just the access_key_id and secret_access_key, this should fail.
  • Now try to include the session_token argument, this should pass.

Pre-Merge Checklist

Copy link

vercel bot commented Feb 8, 2024

@tariqajyusuf is attempting to deploy a commit to the Ethyca Team on Vercel.

A member of the Team first needs to authorize it.

@adamsachs adamsachs self-requested a review February 8, 2024 19:15
Copy link
Contributor

@adamsachs adamsachs left a comment

Choose a reason for hiding this comment

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

thanks so much for this contribution, this change looks great @tariqajyusuf! appreciate all the diligence in getting this supported throughout the stack, from the CLI to the UI. i was able to test manually with some of our internal AWS accounts and things work great with temporary credentials, as well as with permanent credentials (i.e. no regressions).

i left one UI nitpick that you can feel free to ignore, i'll leave it up to your discretion! the other piece of this is that we'll want to get automated test coverage for this new functionality in place, but that's not really something you're able to do outside of our org (since the GH actions will require our GH secrets, etc.). so i went ahead and filed a follow up ticket for myself to add that test coverage in (#4607) - it should be a relatively straightforward update.

thanks again for being proactive and thoughtful in contributing this improvement to our codebase/product, we really appreciate it! 🙏

"--session_token",
type=str,
default="",
help="Connect to AWS using a temporary `Access Key`. _Requires options `--access_key_id`, `--secret_access_key`, `--session_token`, & `--region`._",
Copy link
Contributor

Choose a reason for hiding this comment

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

thank you for the diligent documentation here, this leads to a clear user experience!

@@ -330,6 +331,7 @@ def handle_aws_credentials_options(
aws_config = AWSConfig(
aws_access_key_id=access_key_id,
aws_secret_access_key=secret_access_key,
aws_session_token=session_token,
Copy link
Contributor

Choose a reason for hiding this comment

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

interesting, so this seems to still work even when passed through as None - seems like the boto3 client instantiation just ignores any kwargs with None values in its constructor, as that's what is going to happen downstream at https://github.com/ethyca/fides/blob/main/src/fides/connectors/aws.py#L23-L33

service_client = boto3.client(
        service,
        **config_dict,
    )

nothing to change here since it all works as we want and boto3 is pretty defensive, just thought it was noteworthy behavior!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I was watching for this to have some issues as I've experienced that with a few other AWS SDKs but it seems that it does a pretty good job at checking here.

@adamsachs adamsachs merged commit 6428833 into ethyca:main Feb 9, 2024
1 check failed
tariqajyusuf added a commit to tariqajyusuf/fidesdocs that referenced this pull request Feb 21, 2024
Include environment parameters for changes added in ethyca/fides#4603
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.

Add support for STS-issued AWS credentials
2 participants