aws/credential: Added credential_process provider #1874
Conversation
|
Thanks for creating this PR @micahhausler. We'll review this and get back to you with feedback. Initially my feedback is that the In addition, I think we'd want to remove the auto loading of configuration file from the
|
|
Would love to see this integrated. |
jasdel
added
the
response-requested
|
@micahhausler @jasdel Any updates on this feature? |
|
Would also like to see this implemented. We have a use case with aws-iam-authenticator with K8 and Okta. |
|
Hello @tahoward, thank you for posting your interesting on this feature. Out of curiosity what is the use case? I'm a little concerned for allowing reading of a file to fork and run a process. Sends a lot of red flags towards my way when I hear that. |
|
Hello @xibz we assume roles in AWS with SAML in order to access EKS clusters as we would like to keep access control bound to our domain. ATM we run an okta client CLI command and export It would also be great to see this implemented just for the sake of consistency when it comes to interpreting profile configurations in |
| @@ -9,3 +9,4 @@ awstesting/integration/smoke/_test/ | |||
| /vendor/pkg/ | |||
| /vendor/src/ | |||
| /private/model/cli/gen-api/gen-api | |||
| .*.swp | |||
There was a problem hiding this comment.
nit this file probably shouldn't change
| @@ -0,0 +1,228 @@ | |||
| package credentials | |||
There was a problem hiding this comment.
would you mind moving the process_provider.go and _test files to a processcreds package. Having the credential provider in their own package allow them to depend on the aws package if needed. the credentials package cannot use the aws package since aws imports `credentials.
| func (p *ProcessProvider) loadProfile(filename, profile string) (Value, error) { | ||
| config, err := ini.Load(filename) | ||
| if err != nil { | ||
| return Value{ProviderName: ProcessProviderName}, awserr.New("ProcessProviderLoad", "failed to load shared config file", err) |
There was a problem hiding this comment.
For these error messages could you please create consts to make comparing against them easier.
e.g:
const (
ErrCodeProcessProviderLoad = `ProcessProviderLoad`
)| return Value{ProviderName: ProcessProviderName}, err | ||
| } | ||
|
|
||
| creds, err := p.loadProfile(filename, p.profile()) |
There was a problem hiding this comment.
Loading the shared config file each time the credentials are retrieved have issues that I don't think is good for the SDK to perform. per #1993
| // file, and returns the credentials retrieved. An error will be returned if it | ||
| // fails to read from the file, or the data is invalid. | ||
| func (p *ProcessProvider) loadProfile(filename, profile string) (Value, error) { | ||
| config, err := ini.Load(filename) |
There was a problem hiding this comment.
I think we need to look at exporting aws/session/shard_config.go or at least put it under the SDK's root internal package so it can be reused by the process provider. There are modifications to the shared config logic that will be important for this utility.
There was a problem hiding this comment.
I think ideally the ProcessProvider doesn't need to read/load the shared config file at all. The loading of the contents of the shared config's credential_process value should be passed in via an input parameter. Similar to the stscreds's credential provider.
If the shared_config.go logic is exported it would make it easier for the ProcessProvider to have access to the credential_provider value.
| } | ||
|
|
||
| // Handle expiration | ||
| if len(resp.Expiration) > 0 { |
There was a problem hiding this comment.
Using a pointer for resp.Expiration would simplify receiving the time value. This is done for the SDK's endpoint credential provider
| // filename returns the filename to use to read AWS shared credentials. | ||
| // | ||
| // Will return an error if the user's home directory path cannot be found. | ||
| func (p *ProcessProvider) filename() (string, error) { |
There was a problem hiding this comment.
This information should come from the SDK's session similar to stscreds There are several locations, and various env var names that can drive the config file to load within the SDK. Having loading the shared config file utility do this work, will help reduce duplication.
| p.Profile = os.Getenv("AWS_PROFILE") | ||
| } | ||
| if p.Profile == "" { | ||
| p.Profile = os.Getenv("AWS_DEFAULT_PROFILE") |
There was a problem hiding this comment.
The SDK does not use the AWS_DEFAULT_PROFILE by default. this was a purely CLI environment variable that still exists for backwards compatibility. The SDK's session and env_config. go provides the logic to determine which environment variable to use.
| "path/filepath" | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/assert" |
There was a problem hiding this comment.
The SDK is looking to move away from testify and use pure go testing. It would be great to the pure go testing package for these tests.
| @@ -55,6 +58,9 @@ type sharedConfig struct { | |||
| AssumeRole assumeRoleConfig | |||
| AssumeRoleSource *sharedConfig | |||
|
|
|||
| // An external process to request credentials | |||
| CredentialProcess string | |||
There was a problem hiding this comment.
The sharedConfig utility is the utility that should be solely responsible for loading the shared config file.
|
@micahhausler Are you still working in this one? Would love to see this implemented! |
|
Thanks for taking the time to create this PR @micahhausler. Since we have two PRs for similar features. I think we should consolidate, and continue this discussion on #2217. |
diehlaws
removed
the
response-requested
Adds support for calling
credential_processlike the aws cli.I can add more to the docs once the design/implementation is cleared. Let me know if you want more tests on some of the failure cases.
Fixes #1834
ping @jasdel @kyleknap