Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

assume role chain parse tree #632

Merged
merged 10 commits into from
Aug 6, 2021
Merged

assume role chain parse tree #632

merged 10 commits into from
Aug 6, 2021

Conversation

rcoh
Copy link
Collaborator

@rcoh rcoh commented Aug 3, 2021

Description of changes:

This commit builds the parsing half of our AssumeRoleProvider implementation. Our implementation decouples
the (fairly complex) task of actually turning a profile file into a series of providers from the (fairly simple)
task if iterating through credential providers and executing them.

The descriptive test cases from the spec have been ported to JSON and some new ones have been added and they cover our implementation fairly exhaustively.

Future commits will implement a named-provider factory, wire up the STS client and actually turn this into a credentials provider.

  • Update CHANGELOG
  • fix dependency generation
  • figure out how we want these tests to be run in CI

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

rcoh added 2 commits August 3, 2021 17:52
This commit builds the parsing half of our AssumeRoleProvider implementation. Our implementation decouples
the (fairly complex) task of actually turning a profile file into a series of providers from the (fairly simple)
task if iterating through credential providers and executing them.

The descriptive test cases from the spec have been ported to JSON and some new ones have been added and they cover our implementation fairly exhaustively.

Future commits will implement a named-provider factory, wire up the STS client and actually turn this into a credentials provider.
This commit builds the parsing half of our AssumeRoleProvider implementation. Our implementation decouples
the (fairly complex) task of actually turning a profile file into a series of providers from the (fairly simple)
task if iterating through credential providers and executing them.

The descriptive test cases from the spec have been ported to JSON and some new ones have been added and they cover our implementation fairly exhaustively.

Future commits will implement a named-provider factory, wire up the STS client and actually turn this into a credentials provider.
@rcoh rcoh requested a review from jdisanti August 3, 2021 22:44
@@ -0,0 +1 @@
pub mod profile;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add the copyright header, and maybe a crate top-level doc comment.

Comment on lines +34 to +39
#[doc(hidden)]
pub fn load_profile() -> Result<(), Box<dyn Error>> {
// remove the non-usage warnings until we're actually using this module
let _ = repr::resolve_chain(&aws_types::profile::load(&Fs::real(), &Env::real())?)?;
Ok(())
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

because it's in a private module and nothing is using it yet, I need to actually use the function to remove a chain of non-usage warnings. temporary until the exec half is done

Comment on lines +231 to +237
if let (None, None, None) = (access_key, secret_key, session_token) {
return Err(ProfileFileError::MissingCredentialSource {
profile: profile.name().to_string(),
message: "expected `aws_access_key_id` and `aws_secret_access_key` to be defined"
.into(),
});
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This error message will be confusing if it's the session token that's missing. Although, from reading the test cases, I'm not sure if that case is possible?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the session token is optional so having that missing is never an error

Comment on lines +34 to +48
"docs": "ignore explicit credentials when source profile is specified",
"input": {
"profile": {
"A": {
"aws_access_key_id": "abc123",
"aws_secret_access_key": "def456",
"role_arn": "arn:aws:iam::123456789:role/RoleA",
"source_profile": "B"
},
"B": {
"aws_access_key_id": "ghi890",
"aws_secret_access_key": "jkl123"
}
},
"selected_profile": "A"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we log a warning when encountering this case? Or is there a valid use-case that takes advantage of this prioritization?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

that's a good call. In general I'm planning on setting up a nice logging message that clearly talks about the chain that will be used. It's probably worth logging whenever we make a priority choice as well

@rcoh rcoh enabled auto-merge (squash) August 6, 2021 03:13
@rcoh rcoh merged commit b2dff50 into main Aug 6, 2021
@rcoh rcoh deleted the auth-providers-repr branch August 6, 2021 03:26
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants