-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
auth/aws: Allow wildcard in bound_iam_principal_id #3213
Merged
Merged
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -928,6 +928,12 @@ func (b *backend) pathLoginRenewIam( | |
} | ||
} | ||
|
||
// Note that the error messages below can leak a little bit of information about the role information | ||
// For example, if on renew, the client gets the "error parsing ARN..." error message, the client | ||
// will konw that it's a wildcard bind (but not the actual bind), even if the client can't actually | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. s/konw/know |
||
// read the role directly to know what the bind is. It's a relatively small amount of leakage, in | ||
// some fairly corner cases, and in the most likely error case (role has been changed to a new ARN), | ||
// the error message is identical. | ||
if roleEntry.BoundIamPrincipalARN != "" { | ||
// We might not get here if all bindings were on the inferred entity, which we've already validated | ||
// above | ||
|
@@ -936,10 +942,31 @@ func (b *backend) pathLoginRenewIam( | |
// Resolving unique IDs is enabled and the auth metadata contains the unique ID, so checking the | ||
// unique ID is authoritative at this stage | ||
if roleEntry.BoundIamPrincipalID != clientUserId { | ||
return nil, fmt.Errorf("role no longer bound to ID %q", clientUserId) | ||
return nil, fmt.Errorf("role no longer bound to ARN %q", canonicalArn) | ||
} | ||
} else if strings.HasSuffix(roleEntry.BoundIamPrincipalARN, "*") { | ||
fullArn := b.getCachedUserId(clientUserId) | ||
if fullArn == "" { | ||
entity, err := parseIamArn(canonicalArn) | ||
if err != nil { | ||
return nil, fmt.Errorf("error parsing ARN %q: %v", canonicalArn, err) | ||
} | ||
fullArn, err = b.fullArn(entity, req.Storage) | ||
if err != nil { | ||
return nil, fmt.Errorf("error looking up full ARN of entity %v: %v", entity, err) | ||
} | ||
if fullArn == "" { | ||
return nil, fmt.Errorf("got empty string back when looking up full ARN of entity %v", entity) | ||
} | ||
if clientUserId != "" { | ||
b.setCachedUserId(clientUserId, fullArn) | ||
} | ||
} | ||
if !strutil.GlobbedStringsMatch(roleEntry.BoundIamPrincipalARN, fullArn) { | ||
return nil, fmt.Errorf("role no longer bound to ARN %q", canonicalArn) | ||
} | ||
} else if roleEntry.BoundIamPrincipalARN != canonicalArn { | ||
return nil, fmt.Errorf("role no longer bound to arn %q", canonicalArn) | ||
return nil, fmt.Errorf("role no longer bound to ARN %q", canonicalArn) | ||
} | ||
} | ||
|
||
|
@@ -1123,7 +1150,7 @@ func (b *backend) pathLoginUpdateIam( | |
callerUniqueId := strings.Split(callerID.UserId, ":")[0] | ||
entity, err := parseIamArn(callerID.Arn) | ||
if err != nil { | ||
return logical.ErrorResponse(fmt.Sprintf("Error parsing arn: %v", err)), nil | ||
return logical.ErrorResponse(fmt.Sprintf("error parsing arn %q: %v", callerID.Arn, err)), nil | ||
} | ||
|
||
roleName := data.Get("role").(string) | ||
|
@@ -1152,8 +1179,27 @@ func (b *backend) pathLoginUpdateIam( | |
if callerUniqueId != roleEntry.BoundIamPrincipalID { | ||
return logical.ErrorResponse(fmt.Sprintf("expected IAM %s %s to resolve to unique AWS ID %q but got %q instead", entity.Type, entity.FriendlyName, roleEntry.BoundIamPrincipalID, callerUniqueId)), nil | ||
} | ||
} else if roleEntry.BoundIamPrincipalARN != "" && roleEntry.BoundIamPrincipalARN != entity.canonicalArn() { | ||
return logical.ErrorResponse(fmt.Sprintf("IAM Principal %q does not belong to the role %q", callerID.Arn, roleName)), nil | ||
} else if roleEntry.BoundIamPrincipalARN != "" { | ||
if strings.HasSuffix(roleEntry.BoundIamPrincipalARN, "*") { | ||
fullArn := b.getCachedUserId(callerUniqueId) | ||
if fullArn == "" { | ||
fullArn, err = b.fullArn(entity, req.Storage) | ||
if err != nil { | ||
return logical.ErrorResponse(fmt.Sprintf("error looking up full ARN of entity %v: %v", entity, err)), nil | ||
} | ||
if fullArn == "" { | ||
return logical.ErrorResponse(fmt.Sprintf("got empty string back when looking up full ARN of entity %v", entity)), nil | ||
} | ||
b.setCachedUserId(callerUniqueId, fullArn) | ||
} | ||
if !strutil.GlobbedStringsMatch(roleEntry.BoundIamPrincipalARN, fullArn) { | ||
// Note: Intentionally giving the exact same error message as a few lines below. Otherwise, we might leak information | ||
// about whether the bound IAM principal ARN is a wildcard or not, and what that wildcard is. | ||
return logical.ErrorResponse(fmt.Sprintf("IAM Principal %q does not belong to the role %q", callerID.Arn, roleName)), nil | ||
} | ||
} else if roleEntry.BoundIamPrincipalARN != entity.canonicalArn() { | ||
return logical.ErrorResponse(fmt.Sprintf("IAM Principal %q does not belong to the role %q", callerID.Arn, roleName)), nil | ||
} | ||
} | ||
|
||
policies := roleEntry.Policies | ||
|
@@ -1502,6 +1548,7 @@ type iamEntity struct { | |
SessionInfo string | ||
} | ||
|
||
// Returns a Vault-internal canonical ARN for referring to an IAM entity | ||
func (e *iamEntity) canonicalArn() string { | ||
entityType := e.Type | ||
// canonicalize "assumed-role" into "role" | ||
|
@@ -1516,6 +1563,46 @@ func (e *iamEntity) canonicalArn() string { | |
return fmt.Sprintf("arn:%s:iam::%s:%s/%s", e.Partition, e.AccountNumber, entityType, e.FriendlyName) | ||
} | ||
|
||
// This returns the "full" ARN of an iamEntity, how it would be referred to in AWS proper | ||
func (b *backend) fullArn(e *iamEntity, s logical.Storage) (string, error) { | ||
// Not assuming path is reliable for any entity types | ||
client, err := b.clientIAM(s, getAnyRegionForAwsPartition(e.Partition).ID(), e.AccountNumber) | ||
if err != nil { | ||
return "", fmt.Errorf("error creating IAM client: %v", err) | ||
} | ||
|
||
switch e.Type { | ||
case "user": | ||
input := iam.GetUserInput{ | ||
UserName: aws.String(e.FriendlyName), | ||
} | ||
resp, err := client.GetUser(&input) | ||
if err != nil { | ||
return "", fmt.Errorf("error fetching user %q: %v", e.FriendlyName, err) | ||
} | ||
if resp == nil { | ||
return "", fmt.Errorf("nil response from GetUser") | ||
} | ||
return *(resp.User.Arn), nil | ||
case "assumed-role": | ||
fallthrough | ||
case "role": | ||
input := iam.GetRoleInput{ | ||
RoleName: aws.String(e.FriendlyName), | ||
} | ||
resp, err := client.GetRole(&input) | ||
if err != nil { | ||
return "", fmt.Errorf("error fetching role %q: %v", e.FriendlyName, err) | ||
} | ||
if resp == nil { | ||
return "", fmt.Errorf("nil response form GetRole") | ||
} | ||
return *(resp.Role.Arn), nil | ||
default: | ||
return "", fmt.Errorf("unrecognized entity type: %s", e.Type) | ||
} | ||
} | ||
|
||
const iamServerIdHeader = "X-Vault-AWS-IAM-Server-ID" | ||
|
||
const pathLoginSyn = ` | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Do you see a reason to do it like this instead of using go-cache, since this is purely in-memory? I don't have a strong preference, go-cache will just do it internally and individually rather than on a schedule in a block.
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.
This would also get rid of the need to hold a mutex to do this operation and the need to store a last-accessed time. The downside, mainly, is that updating the expiration time still requires a replace operation (but the locking is handled internally).
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.
Because I wasn't aware go-cache existed :) I was mainly pattern matching off of the cached AWS clients. And hopefully patrickmn/go-cache#66 will get merged to make it easier to update the expiration time.
I'll work on moving to go-cache and push a new commit when ready.
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.
Done!