-
Notifications
You must be signed in to change notification settings - Fork 564
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
Implement assume-time policy limiting #1342
Conversation
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #1342 +/- ##
===========================================
- Coverage 42.19% 28.98% -13.21%
===========================================
Files 54 70 +16
Lines 6456 9663 +3207
===========================================
+ Hits 2724 2801 +77
- Misses 3283 6391 +3108
- Partials 449 471 +22
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@eldondevat The linter is failing - can you take a look? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine to me. I left a comment that needs clarification before merge.
if account.PolicyFile != "" { | ||
policy, err := os.ReadFile(account.PolicyFile) | ||
if err != nil { | ||
return nil, errors.Wrap(err, fmt.Sprintf("Failed to load supplimental policy file: %s", account.PolicyFile)) | ||
} | ||
params.Policy = aws.String(string(policy)) | ||
} | ||
|
||
if account.PolicyARNs != "" { | ||
var arns []*sts.PolicyDescriptorType | ||
for _, arn := range strings.Split(account.PolicyARNs, ",") { | ||
arns = append(arns, &sts.PolicyDescriptorType{Arn: aws.String(arn)}) | ||
} | ||
params.PolicyArns = arns |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens when the policies/policyARNs are removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The policyfile would be a local file. If the local file doesn't exist, I would expect that to generate the new wrapped error. If the PolicyFile doesn't represent a valid policy, or if there is an attempt to use the arn of a policy that doesn't exist, I would expect AssumeRole to return an error, so those would end up with a wrapped error and "Error retrieving STS credentials using SAML." There are a few things we could do, like validating the format of ARN's or the general shape of the json policy document ahead of time, but I wasn't sure how much you would want to add there. I don't think we can validate the existence of the actual policies pointed to by the policyARN's in a meaningful way, because we don't really have AWS creds yet. I am using this while scoping things like AWS' canned ReadOnly and SecurityAudit policies, and those haven't gone away yet.
I wasn't sure what you meant by "removed" precisely, but hopefully it was covered in there.
When an STS token is acquired, it's possible to use supplemental policies to limit the permissions of the token. One example use might be assuming an administrative role, but limiting the permissions to a read-only or security-review scope to use to provided credentials in an auditing tool. This PR implements assume-time limiting of permissions with additional managed policies or a local policy document.