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 Config Parameter to Disable STS Credential Validation #50

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

ivancasco
Copy link

Issue #, if available: #36

Description of changes:
This PR introduces a new configuration parameter, validate_credentials, that allows users to disable the STS GetCallerIdentity check performed during the startup of the AWS Secrets Manager agent. By default, the credential validation is enabled to ensure credentials are valid during initialization and to ensure backwards compatibility, but this parameter provides flexibility for environments where the validation may not be required.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

…for AWS credentials STS validation on startup
@ivancasco ivancasco requested a review from a team as a code owner December 19, 2024 23:34
@simonmarty
Copy link
Contributor

Thanks for the contribution! I'm taking off work over the holidays so I'll probably get to reviewing this more closely in January.

@codecov-commenter
Copy link

codecov-commenter commented Dec 20, 2024

Codecov Report

Attention: Patch coverage is 46.15385% with 14 lines in your changes missing coverage. Please review.

Project coverage is 40.87%. Comparing base (a279f54) to head (7862361).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
aws_secretsmanager_agent/src/utils.rs 0.00% 11 Missing ⚠️
aws_secretsmanager_agent/src/config.rs 80.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #50      +/-   ##
==========================================
+ Coverage   40.79%   40.87%   +0.08%     
==========================================
  Files          15       15              
  Lines        8541     8560      +19     
==========================================
+ Hits         3484     3499      +15     
- Misses       5057     5061       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jirkafajfr
jirkafajfr previously approved these changes Dec 23, 2024
…ts to ensure proper configuration

refactor(tests): remove redundant test for validate_credentials as it is already covered in other tests
Err(e) if config.ignore_transient_errors() && is_transient_error(&e) => (),
Err(e) => Err(e)?,
};
// Only perform STS validation if enabled
Copy link
Contributor

Choose a reason for hiding this comment

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

You can remove that comment.

let sts_client = aws_sdk_sts::Client::from_conf(sts_builder.build());
match sts_client.get_caller_identity().send().await {
Ok(_) => (),
Err(e) if config.ignore_transient_errors() && is_transient_error(&e) => (),
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is a one-time startup check, let's not ignore transient errors.

sts_builder.set_region(Some(Region::new(region.clone())));
}

// Validate the region and credentials first
Copy link
Contributor

Choose a reason for hiding this comment

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

That comment can also be removed.

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.

4 participants