plugins/rest: Add support to get temp creds via AssumeRole #6634
Conversation
✅ Deploy Preview for openpolicyagent ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
plugins/rest/auth.go
Outdated
| cfgs := map[bool]int{} | ||
| cfgs[ap.AWSEnvironmentCredentials != nil]++ | ||
| cfgs[ap.AWSMetadataCredentials != nil]++ | ||
| cfgs[ap.AWSCustomIdentityCredentials != nil]++ | ||
| cfgs[ap.AWSWebIdentityCredentials != nil]++ | ||
| cfgs[ap.AWSProfileCredentials != nil]++ |
There was a problem hiding this comment.
💭 Nowadays, I'd go with
switch {
case ap.AWSEnvironmentCredentials != nil:
case ap.AWSMetadataCredentials != nil:
case ap.AWSCustomIdentityCredentials != nil:
case ap.AWSWebIdentityCredentials != nil:
case ap.AWSProfileCredentials != nil:
default:
return errors.New("a AWS credential service must be specified when S3 signing is enabled")
}but it's largely equivalent 😉
plugins/rest/aws.go
Outdated
| RoleArn string | ||
| AccessKey string | ||
| SecretKey string | ||
| SessionToken string |
There was a problem hiding this comment.
Are these attributes public because they're expected to be read/written somewhere (as e.g. json, not from env vars, as in populateFromEnv()), or could they just as well be private?
There was a problem hiding this comment.
They're currently expected to be written via env vars. I've tried to stay consistent with what we have done for the other credential providers.
There was a problem hiding this comment.
@ashutosh-narkar Will reading from custom AWS profile credentials file also be supported here?
plugins/rest/aws.go
Outdated
| if cs.SessionToken == "" { | ||
| // In case of missing SessionToken try to get SecurityToken | ||
| // AWS switched to use SessionToken, but SecurityToken was left for backward compatibility | ||
| cs.SessionToken = os.Getenv(securityTokenEnvVar) |
There was a problem hiding this comment.
AWS_SESSION_TOKEN/AWS_SECURITY_TOKEN is optional?
There was a problem hiding this comment.
I am looking to test this out as part of an integration test. I'll update when I have some verification.
There was a problem hiding this comment.
I did an integration test using this and did not have to provide AWS_SESSION_TOKEN . Also the end-to-end flow was successful.
| } | ||
|
|
||
| // short circuit if a reasonable amount of time until credential expiration remains | ||
| if time.Now().Add(time.Minute * 5).Before(cs.expiration) { |
There was a problem hiding this comment.
Would there be a scenario where a user might want this to be configurable? E.g. if the returned expiration is expected to be close to the 5 min time limit (or maybe these credentials are always expected to live long).
There was a problem hiding this comment.
We're doing the same for the Web Identity Credentials so I've used the same format. In the future we can surely look into making this configurable if required.
| stsRequestURL, _ := url.Parse(cs.stsPath()) | ||
|
|
||
| // construct an HTTP client with a reasonably short timeout | ||
| client := &http.Client{Timeout: time.Second * 10} |
There was a problem hiding this comment.
Same question as above, should this timeout perhaps be configurable?
| // verify existing credentials haven't changed | ||
| ts.accessKey = "OTHERKEY" | ||
| creds, _ = cs.credentials(context.Background()) | ||
| assertEq(creds.AccessKey, testAccessKey, t) |
There was a problem hiding this comment.
Consider adding another pair of assertions that we get the expected refresh/no refresh just after and before the hard-coded 5 minutes grace time.
There was a problem hiding this comment.
Good idea. Added a test case.
ashutosh-narkar
force-pushed
the
aws-assume-role
branch
3 times, most recently
from
d83f312 to
8fa8360
Compare
plugins/rest/aws.go
Outdated
| cs.AccessKey = os.Getenv(accessKeyEnvVar) | ||
| if cs.AccessKey == "" { | ||
| return errors.New("no " + accessKeyEnvVar + " set in environment") | ||
| } | ||
| cs.SecretKey = os.Getenv(secretKeyEnvVar) | ||
| if cs.SecretKey == "" { | ||
| return errors.New("no " + secretKeyEnvVar + " set in environment") | ||
| } |
There was a problem hiding this comment.
@ashutosh-narkar will OPA not be able to obtain these credentials from EC2 metadata service using aws.GetDefaultConfig() if they have not been provided in the environment?
There was a problem hiding this comment.
This will require OPA consumer to populate these credentials and keep them up to date.
There was a problem hiding this comment.
Added support for credentials fetching using Environment variables, Credentials file and EC2 metadata service.
plugins/rest/aws.go
Outdated
| cs.RoleArn = os.Getenv(awsRoleArnEnvVar) | ||
| if cs.RoleArn == "" { | ||
| return errors.New("no " + awsRoleArnEnvVar + " set in environment") | ||
| } |
There was a problem hiding this comment.
@ashutosh-narkar can this ARN be specified in OPA config yaml?
There was a problem hiding this comment.
Updated such that the ARN can be specified via config or environment variable.
ashutosh-narkar
force-pushed
the
aws-assume-role
branch
from
8fa8360 to
555a168
Compare
ashutosh-narkar
force-pushed
the
aws-assume-role
branch
from
3c1dfb8 to
1d07149
Compare
Adds support for signing AWS requests using temporary credentials obtained from AWS STS via AssumeRole operation. One use-case of this mechanism is for allowing existing IAM users to access AWS resources that they don't already have access to. It is also useful as a means to temporarily gain privileged access. Signed-off-by: Ashutosh Narkar <anarkar4387@gmail.com>
ashutosh-narkar
force-pushed
the
aws-assume-role
branch
from
1d07149 to
5c30a3a
Compare
Adds support for signing AWS requests using temporary credentials obtained from AWS STS via AssumeRole operation. One use-case of this mechanism is for allowing existing IAM users to access AWS resources that they don't already have access to. It is also useful as a means to temporarily gain privileged access.