Skip to content
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 4 commits into from
Aug 30, 2017

Conversation

joelthompson
Copy link
Contributor

This allows specifying a trailing wildcard in the bound_iam_principal_id
in a semantically similar way to AWS. If a user specifies a
bound_iam_principal_arn with a trailing wildcard, then Vault will NOT
resolve the unqiue ID (analogous to the way AWS handles wildcards) and
instead will just store the ARN.

At login time, if a wildcard is specified in the bound ARN, Vault will
first resolve the full ARN of the authenticating principal because the
path component of roles is not visible from the GetCallerIdentity
response. A cache has been included to speed up performance of these
lookups and should be append only. To prevent unbounded growth of the
cache, old entries are cleaned up if not used within a period of time.

Also, as the complexity of scenarios of when Vault might make AWS API
calls increases, including a recommended IAM policy to give the Vault
server that can be more simply referenced in the future.

Fixes #3179

This allows specifying a trailing wildcard in the bound_iam_principal_id
in a semantically similar way to AWS. If a user specifies a
bound_iam_principal_arn with a trailing wildcard, then Vault will NOT
resolve the unqiue ID (analogous to the way AWS handles wildcards) and
instead will just store the ARN.

At login time, if a wildcard is specified in the bound ARN, Vault will
first resolve the full ARN of the authenticating principal because the
path component of roles is not visible from the GetCallerIdentity
response. A cache has been included to speed up performance of these
lookups and should be append only. To prevent unbounded growth of the
cache, old entries are cleaned up if not used within a period of time.

Also, as the complexity of scenarios of when Vault might make AWS API
calls increases, including a recommended IAM policy to give the Vault
server that can be more simply referenced in the future.

Fixes hashicorp#3179
@@ -174,6 +181,9 @@ func (b *backend) periodicFunc(req *logical.Request) error {
b.tidyWhitelistIdentity(req.Storage, safety_buffer)
}

// get rid of old unique ID entries
b.cleanOldCachedUniqueIdMapping()
Copy link
Member

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.

Copy link
Member

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).

Copy link
Contributor Author

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

vishalnayak
vishalnayak previously approved these changes Aug 30, 2017
Copy link
Member

@vishalnayak vishalnayak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 👍 for Recommended IAM policy in the docs.

@@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/konw/know

@jefferai jefferai added this to the 0.8.2 milestone Aug 30, 2017
jefferai
jefferai previously approved these changes Aug 30, 2017
Copy link
Member

@jefferai jefferai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Will jump in and fix that comment real quick, then merge.

@jefferai jefferai dismissed stale reviews from vishalnayak and themself via 09972b1 August 30, 2017 21:51
@jefferai
Copy link
Member

Brilliant work as always, @joelthompson !

@jefferai jefferai merged commit c641938 into hashicorp:master Aug 30, 2017
chrishoffman pushed a commit that referenced this pull request Aug 31, 2017
* oss/master:
  Add 'discard' target to file audit backend (#3262)
  changelog++
  auth/aws: Allow wildcard in bound_iam_principal_id (#3213)
  changelog++
  Add option to set cluster TLS cipher suites. (#3228)
@joelthompson
Copy link
Contributor Author

Thanks @jefferai! And also for the silly typo fix :)

@joelthompson joelthompson deleted the aws_auth_wildcards branch August 31, 2017 02:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

auth/aws: support all principal types
3 participants