Credentials Provider Initial Implementation & Design #179

rcoh · 2021-01-28T13:47:02Z

Issue #, if available: Fixes #149, Fixes #148
*Description of changes:*Initial Implementation of credential providers, design validated & used in #107

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

This also introduces aws/rust-runtime, runtime components specific to AWS.

aajtodd · 2021-01-28T14:29:04Z

aws/rust-runtime/auth/src/provider.rs

+
+/// Load Credentials from Environment Variables
+pub struct EnvironmentVariableCredentialsProvider {
+    env: Box<dyn Fn(&str) -> Result<String, VarError> + Send + Sync>


question

Why does the provider need to hold onto the environment?

in Rust, all the UTs run in a parallel so setting environment variables before tests doesn't work very well. This allows us to provide a HashMap of environment variables (or any function) for test purposes.

I'm imagining eventually we'll have protocol tests that specify the environment they're running in as well.

this might justify a separate module to handle env vars that could be appropriately mocked in unit tests

yeah I think the second time I do this I'll extract the module

aajtodd · 2021-01-28T14:29:58Z

aws/rust-runtime/auth/src/provider.rs

+
+const ENV_PROVIDER: &'static str = "EnvironmentVariable";
+
+impl ProvideCredentials for EnvironmentVariableCredentialsProvider {


question

What balance are you going to strike with CRT providers? Environment provider is obviously rather simple, some of the others are more involved (imds, x509, etc)

As soon as CRT bindings can get done, pretty much 100% CRT. But we're doing a private beta starting 2/15 and I just need something that mostly works that people can play with.

aajtodd · 2021-01-28T14:31:13Z

aws/rust-runtime/auth/src/lib.rs

+
+/// A Credentials Provider
+///
+/// This interface is intentionally NOT async. Credential providers should provide a separate


question

Can you elaborate on this? Why would you not want this to be async?

There's a few reasons:

predictability in the request pipeline. When you call call, I want to be as close as possible to that executing exactly one HTTP request. With Rusoto, there are a decent number of issues that are hard to track down where a credential provider hangs, then hangs all the requests. By pushing credential provider refreshes out of band, I'm hoping to side step that

Improved testability. If the credentials provider refresh is out of band, you can have full control over the HTTP stack its using, whether or not it refreshes, etc.

seems like there should be a larger discussion about this one. even if the creds fetch only has a file system dependency, it'd be reasonable to make it async.

Just to be clear, it's not that retrieving credentials is something sync necessarily—but the credentials provider that lives in the request path is sync (because the credentials have always been preloaded)

greenwoodcm · 2021-01-28T18:47:02Z

aws/rust-runtime/auth/src/lib.rs

+        let mut creds = f.debug_struct("Credentials");
+        creds.field("provider_name", &self.provider_name);
+        creds.finish()


you might consider the AKID in this debug string. AKIDs are generally considered non-sensitive without their corresponding secret keys. could be worth a double check on that though.

greenwoodcm · 2021-01-28T18:48:53Z

aws/rust-runtime/auth/src/lib.rs

+    /// If these credentials never expire, this value will be set to `None`
+    expires_after: Option<SystemTime>,
+
+    provider_name: &'static str,


is there a good reason for this to be a &'static str instead of just a String? is it performance? i wonder if provider name could be generated more dynamically in some use cases, for instance a role creds provider including the role name in provider_name. something to think about.

hmm, that's a good point. I was thinking this string was always going to be hard coded but I may be wrong. In any case, this field is private at the moment so it's easy to change to String at some point

greenwoodcm · 2021-01-28T18:58:12Z

aws/rust-runtime/auth/src/lib.rs

+
+const STATIC_CREDENTIALS: &'static str = "static";
+impl Credentials {
+    pub fn from_keys(


for clarity you might consider two separate constructors (not sure how much i like the names):

fn from_user_keys(access_key_id: String, secret_access_key: String); fn from_session_keys(access_key_id: String, secret_access_key: String, session_token: String);

not a bad idea. I'm not sure how often this constructor will actually get used in practice outside of examples (99.9% of people will use some form of environment provider)

greenwoodcm · 2021-01-28T18:59:25Z

aws/rust-runtime/auth/src/lib.rs

+
+/// A Credentials Provider
+///
+/// This interface is intentionally NOT async. Credential providers should provide a separate


seems like there should be a larger discussion about this one. even if the creds fetch only has a file system dependency, it'd be reasonable to make it async.

greenwoodcm · 2021-01-28T19:00:56Z

aws/rust-runtime/auth/src/lib.rs

+impl ProvideCredentials for Credentials {
+    fn credentials(&self) -> Result<Credentials, CredentialsError> {
+        Ok(self.clone())
+    }
+}


should this avoid the clone? perhaps by making ProvideCredentials return pointers to the strings?

I think the way you do this is by making Credentials borrow its fields. Not opposed to that, just seemed like a premature optimization at this point. For comparison, this is how it works in Rusoto right now (with clone)

another decent option could be having this function return &Credentials. I considered adding credentials_ref() for that exact reason but skipped it for now for the purpose of simplicity.

In general though, these credentials may outlive the provider (or at least the credentials within the provider)

greenwoodcm · 2021-01-28T19:02:29Z

aws/rust-runtime/auth/src/provider.rs

+
+/// Load Credentials from Environment Variables
+pub struct EnvironmentVariableCredentialsProvider {
+    env: Box<dyn Fn(&str) -> Result<String, VarError> + Send + Sync>


this might justify a separate module to handle env vars that could be appropriately mocked in unit tests

greenwoodcm · 2021-01-28T19:03:12Z

aws/rust-runtime/auth/src/provider.rs

+    std::env::var(key)
+}
+
+const ENV_PROVIDER: &'static str = "EnvironmentVariable";


np: might want to consolidate on a capitalization strategy with these provider strings

Credentials Provider Initial Implementation

5293f58

rcoh requested a review from a team January 28, 2021 13:47

aajtodd approved these changes Jan 28, 2021

View reviewed changes

rcoh merged commit 61dadae into main Jan 28, 2021

rcoh deleted the credential-providers branch January 28, 2021 15:39

greenwoodcm reviewed Jan 28, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Credentials Provider Initial Implementation & Design #179

Credentials Provider Initial Implementation & Design #179

rcoh commented Jan 28, 2021 •

edited

Loading

aajtodd Jan 28, 2021

rcoh Jan 28, 2021

greenwoodcm Jan 28, 2021

rcoh Jan 28, 2021

aajtodd Jan 28, 2021

rcoh Jan 28, 2021

aajtodd Jan 28, 2021

rcoh Jan 28, 2021

greenwoodcm Jan 28, 2021

rcoh Jan 28, 2021

greenwoodcm Jan 28, 2021

greenwoodcm Jan 28, 2021

rcoh Jan 28, 2021

greenwoodcm Jan 28, 2021

rcoh Jan 28, 2021

greenwoodcm Jan 28, 2021

greenwoodcm Jan 28, 2021

rcoh Jan 28, 2021

rcoh Jan 28, 2021

rcoh Jan 28, 2021

greenwoodcm Jan 28, 2021

greenwoodcm Jan 28, 2021


		const ENV_PROVIDER: &'static str = "EnvironmentVariable";

		impl ProvideCredentials for EnvironmentVariableCredentialsProvider {

Credentials Provider Initial Implementation & Design #179

Credentials Provider Initial Implementation & Design #179

Conversation

rcoh commented Jan 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rcoh commented Jan 28, 2021 •

edited

Loading