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: support all principal types #3179

Closed
bmoylan opened this issue Aug 15, 2017 · 11 comments · Fixed by #3213
Closed

auth/aws: support all principal types #3179

bmoylan opened this issue Aug 15, 2017 · 11 comments · Fixed by #3213
Milestone

Comments

@bmoylan
Copy link

bmoylan commented Aug 15, 2017

This is partially bug-report and partially feature-request:

Bug: The aws credential backend currently panics when given an IAM principal ending in root (e.g. arn:aws:iam::111111111111:root), despite it being a valid IAM principal. It should return an error if this is unsupported.

FR: support this and all other principal types (currently missing root, federated-user, and group).

2017/08/13 18:08:59 http: panic serving 172.17.0.1:33966: runtime error: slice bounds out of range
goroutine 2535 [running]:
net/http.(*conn).serve.func1(0xc420d7c000)
	/goroot/src/net/http/server.go:1721 +0xd0
panic(0x17f5b00, 0x26ceb40)
	/goroot/src/runtime/panic.go:489 +0x2cf
github.com/hashicorp/vault/builtin/credential/aws.parseIamArn(0xc420644140, 0x1e, 0x0, 0xc420e1a938, 0x3b322f08)
	/gopath/src/github.com/hashicorp/vault/builtin/credential/aws/path_login.go:1291 +0x7f5
github.com/hashicorp/vault/builtin/credential/aws.(*backend).resolveArnToRealUniqueId(0xc42088e280, 0x2683140, 0xc420d99b30, 0xc420644140, 0x1e, 0x140, 0x1a5b5c0, 0x1, 0xc420ccf900)
	/gopath/src/github.com/hashicorp/vault/builtin/credential/aws/backend.go:194 +0x4d
github.com/hashicorp/vault/builtin/credential/aws.(*backend).(github.com/hashicorp/vault/builtin/credential/aws.resolveArnToRealUniqueId)-fm(0x2683140, 0xc420d99b30, 0xc420644140, 0x1e, 0xc4201408e0, 0x1, 0x0, 0x0)
	/gopath/src/github.com/hashicorp/vault/builtin/credential/aws/backend.go:79 +0x52
github.com/hashicorp/vault/builtin/credential/aws.(*backend).pathRoleCreateUpdate(0xc42088e280, 0xc420d800e0, 0xc420140650, 0x0, 0x0, 0x0)
	/gopath/src/github.com/hashicorp/vault/builtin/credential/aws/path_role.go:497 +0x52e8
github.com/hashicorp/vault/builtin/credential/aws.(*backend).(github.com/hashicorp/vault/builtin/credential/aws.pathRoleCreateUpdate)-fm(0xc420d800e0, 0xc420140650, 0x0, 0x6, 0xc420dea2f8)
	/gopath/src/github.com/hashicorp/vault/builtin/credential/aws/path_role.go:156 +0x3e
github.com/hashicorp/vault/logical/framework.(*Backend).HandleRequest(0xc420def520, 0xc420d800e0, 0xc420d800e0, 0xc4209ee098, 0x19)
	/gopath/src/github.com/hashicorp/vault/logical/framework/backend.go:221 +0x4c8
github.com/hashicorp/vault/vault.(*Router).routeCommon(0xc42044d9b0, 0xc420d800e0, 0x0, 0x0, 0x0, 0x0, 0x0)
	/gopath/src/github.com/hashicorp/vault/vault/router.go:326 +0x636
github.com/hashicorp/vault/vault.(*Router).Route(0xc42044d9b0, 0xc420d800e0, 0xc420d800e0, 0xc4203a0ba0, 0x0)
	/gopath/src/github.com/hashicorp/vault/vault/router.go:218 +0x3a
github.com/hashicorp/vault/vault.(*Core).handleRequest(0xc4201a0c00, 0xc420d800e0, 0x0, 0x0, 0x0, 0x0)
	/gopath/src/github.com/hashicorp/vault/vault/request_handling.go:188 +0xb10
github.com/hashicorp/vault/vault.(*Core).HandleRequest(0xc4201a0c00, 0xc420d800e0, 0x0, 0x0, 0x0)
	/gopath/src/github.com/hashicorp/vault/vault/request_handling.go:45 +0xc77
github.com/hashicorp/vault/http.request(0xc4201a0c00, 0x2681800, 0xc420d80000, 0xc4201c2100, 0xc420d800e0, 0x0, 0x0)
	/gopath/src/github.com/hashicorp/vault/http/handler.go:209 +0x3c
github.com/hashicorp/vault/http.handleLogical.func1(0x2681800, 0xc420d80000, 0xc4201c2100)
	/gopath/src/github.com/hashicorp/vault/http/logical.go:121 +0xfb
net/http.HandlerFunc.ServeHTTP(0xc4203dd820, 0x2681800, 0xc420d80000, 0xc4201c2100)
	/goroot/src/net/http/server.go:1942 +0x44
github.com/hashicorp/vault/http.handleRequestForwarding.func1(0x2681800, 0xc420d80000, 0xc4201c2100)
	/gopath/src/github.com/hashicorp/vault/http/handler.go:159 +0x1d6
net/http.HandlerFunc.ServeHTTP(0xc4203dd840, 0x2681800, 0xc420d80000, 0xc4201c2100)
	/goroot/src/net/http/server.go:1942 +0x44
net/http.(*ServeMux).ServeHTTP(0xc4201e1e00, 0x2681800, 0xc420d80000, 0xc4201c2100)
	/goroot/src/net/http/server.go:2238 +0x130
github.com/hashicorp/vault/http.wrapHelpHandler.func1(0x2681800, 0xc420d80000, 0xc4201c2100)
	/gopath/src/github.com/hashicorp/vault/http/help.go:22 +0x17f
net/http.HandlerFunc.ServeHTTP(0xc4203dd8e0, 0x2681800, 0xc420d80000, 0xc4201c2100)
	/goroot/src/net/http/server.go:1942 +0x44
github.com/hashicorp/vault/http.wrapGenericHandler.func1(0x2681800, 0xc420d80000, 0xc4201c2100)
	/gopath/src/github.com/hashicorp/vault/http/handler.go:86 +0xb1
net/http.HandlerFunc.ServeHTTP(0xc4203dd940, 0x2681800, 0xc420d80000, 0xc4201c2100)
	/goroot/src/net/http/server.go:1942 +0x44
net/http.serverHandler.ServeHTTP(0xc42056c840, 0x2681800, 0xc420d80000, 0xc4201c2100)
	/goroot/src/net/http/server.go:2568 +0x92
net/http.(*conn).serve(0xc420d7c000, 0x2682980, 0xc42039e480)
	/goroot/src/net/http/server.go:1825 +0x612
created by net/http.(*Server).Serve
	/goroot/src/net/http/server.go:2668 +0x2ce
@bmoylan bmoylan closed this as completed Aug 15, 2017
@bmoylan bmoylan reopened this Aug 16, 2017
@jefferai jefferai added this to the next-release milestone Aug 16, 2017
@jefferai
Copy link
Member

What version of Vault is this? I believe 0.8.0 doesn't accept these but also shouldn't panic.

@jefferai
Copy link
Member

@joelthompson I got the root part but I'm much less certain about proper handling of the rest -- if you are able to take a look at this it would be much appreciated!

@bmoylan
Copy link
Author

bmoylan commented Aug 16, 2017

I was testing on 0.7.3 but looked at 0.8.0 and master to make sure it wasn't already fixed (and the code looked unchanged).

Thanks for the quick fix on the panic! For adding the other principal types, they'll need to be added to this switch statement: https://github.com/hashicorp/vault/blob/v0.8.0/builtin/credential/aws/backend.go#L221

root is the one I'm most concerned with, as I'm trying to migrate to IAM auth from EC2 auth where I'm using bound_account_id. Alternatively, we could relax this check and figure out how to respect that option in IAM: https://github.com/hashicorp/vault/blob/v0.8.0/builtin/credential/aws/path_role.go#L565-L570

@joelthompson
Copy link
Contributor

@bmoylan -- can you elaborate a bit more on your specific use case? I'm a bit confused about what you're trying to achieve, and it might be because I've confused you with the documentation.

If you're inferring an EC2 instance type with the IAM auth, then you should be able to just do essentially a "list-and-shift" of your roles. Just specify auth_type=iam and inferred_entity_type=ec2_instance, leave the rest of the binds the same (assuming they're supported in the iam auth_type), and don't include a bound_iam_principal_arn. It should allow you to specify bound_account_id.

Alternatively, is your use case that you want to NOT infer the EC2 instance entity type but allow any IAM principal in a given account the ability to login to a Vault role? That's currently not possible (and won't be fixed by @jeffrai's change).

@jefferai -- sorry for taking a while to get back, things have been a bit hectic recently. I actually made a conscious decision to not support the root user because using it in this case is such a strong anti-pattern, and for many of the same reasons you encourage revocation of Vault root tokens immediately after they're used. I agree Vault shouldn't panic on this input, however, and I wasn't thinking of that type of input. In general, I'm in favor of giving uses more choices, but this is one of the few cases where I would think Vault probably should be opinionated absent a really good use case for why users should use it, I'm even sure this would solve @bmoylan's goal, and once this gets released, it's very difficult to stop supporting it. I'm also a little nervous about how this would flow through to the rest of the system (e.g., with both unique ID resolution on and off). The safest thing to do, for now, would be to remove the case "root": in parseIamArn line.

First, some background about the subtleties of AWS IAM entities.

It's important to understand the distinction between principal policies and resource policies. Principal policies are applied to a principal (e.g., an IAM user or a role) and specify which actions (e.g., s3:GetObject) on which resources (e.g., arn:aws:s3:::JoelsMagicBucket) that principal is permitted to take. Resource policies are applied to a resource (e.g., JoelsMagicBucket) and specify which principals (e.g., a particular IAM user or role) are permitted to take which actions (e.g., s3:GetObject) on the resource they're applied to.

Now, arn:aws:iam::123456789012:root can actually mean two very different things. It can mean the literal root user in an account. But it can also refer to every single IAM entity in that account, especially when used in a resource policy. As an illustration, let's say I have an S3 bucket, JoelsMagicBucket, in AWS account ID 987654321098. In the S3 bucket policy, if I give read access to arn:aws:iam::123456789012:root then that means that any IAM principal in account 123456789012 will be permitted to read from my bucket, provided that that IAM pricnipal has a principal policy attached to it allowing it also to read from JoelsMagicBucket. I suspect it was this second interpretation that @bmoylan had in mind in trying to specify the root user.

Groups are also a bit weird. Groups can only contain users, not roles. Users get all the policies that apply to them individually and that apply to all the groups they are a member of, which is what you'd expect. However, the frustrating thing is that you cannot specify an IAM group as the principal in a resource policy. That is, let's say I have a group, VaultContributors, that contains Joel and Jeff. You cannot put, in an S3 bucket policy, that VaultDevelopers is allowed read access. You would have to individually permissions both the Joel and Jeff.

Now, how this applies to Vault: I think the most natural interpretation for the bound IAM principal ARN is to consider it like you would a principal in a resource policy. You're applying a policy to a resource (i.e., a Vault role) specifying which IAM principals are allowed to access it. In that interpretation, specifying a bound_iam_principal_arn of arn:aws:iam::123456789012:root really means, "Authorize any IAM principal in account 123456789012, provided that that principal's policy also allows it" -- except that, in this case, there doesn't appear to be any authorization around sts:GetCallerIdentity so in this specific instance, you'd just ignore everything beginning with "provided that." That could be a valid use case, but it would require a little more work than what you had done, and it would also preclude using literal root credentials in the future (which I consider more of a feature than a bug).

As far as groups, if we're to follow the parallel to AWS resource policies, then we'd just simply disallow groups. If we wanted to support groups, it would basically require a new IAM permission to list group memberships on each login call, and basically follow similar semantics to LDAP (e.g., group memberships are only checked on initial login and not on subsequent token renews). I'd actually be open to doing so as it could solve one of the use cases that came up on the mailing list (and I'm a bit annoyed at myself for not thinking of supporting groups at that time).

Federated users is something I don't have a lot of experience with, and architecturally, it also gets weird. The federated-user identities, as best I can tell, really come from an OpenID Connect identity provider, and supporting them in Vault would essentially be (mis?)using AWS as sort of an OIDC-aware identity proxy between an external OIDC provider and Vault, and it'd be much better to just have a direct OIDC auth backend in Vault (e.g., #2525). Thoughts on this?

@joelthompson
Copy link
Contributor

I see you just announced the release while I was typing that up. I'll need to test later tonight what happens when you use actual root credentials to authenticate to Vault, my suspicion is that there will be some issues with it. And sorry the previous post was so long, but it's a really subtle topic!

@joelthompson
Copy link
Contributor

I just did a quick test using root creds (which I generated just for the purposes of the test and then deleted them immediately afterwards!), and found out that they work as expected with resolve_aws_unique_ids=false but ONLY with that -- the unique ID resolution can't handle the root user (since the root user doesn't really have a unique ID in that sense).

Fixing this -- either by actually supporting root user creds even when unique ID resolution is turned on, or by changing the meaning of binding to the root user (which at this point would be a breaking change -- will require a little bit of effort. I'm happy to contribute the code for it, @jefferai let me know which direction you think is right or if you want to chat a bit more once you've recovered from the 0.8.1 release :)

FWIW, I think I've made my opinion clear already on which option is better, but I'll defer to you, and I'll also add that I manually run the acceptance tests using both an IAM user and an EC2 instance in an instance profile before submitting PRs to the AWS auth backend. Since root users are unique enough in the AWS ecosystem, to properly support them, I'd need to also do the same for root users, which would involve generating and revoking root creds (I'm too paranoid to have root credentials laying around in any sort of long-term fashion).

@bmoylan
Copy link
Author

bmoylan commented Aug 17, 2017

Hey @joelthompson -- thanks for the long write up and investigation! The background is really helpful.

You're right that I'm hoping to create a role that authorizes any user/principal from a given account number without using EC2 inference. I have several clients (ec2 instance profiles, lambdas, devs on their laptops) from a particular account that i want to authorize. Totally in sync that root creds are bad and should never be used for day-to-day interactions. If we can't figure out how to support this via role declarations, my (lame) workaround would be to have a vault role that everything can STS into, but if that can be avoided it would be nice :)

@bmoylan
Copy link
Author

bmoylan commented Aug 17, 2017

I mentioned federated users for completeness because they're in the IAM docs, but I don't have experience with them either and don't really intend to use them.

@joelthompson
Copy link
Contributor

Thinking some more about this, and given that Vault has shipped with Jeff's changes, I don't think it would be wise at this point to change the meaning of the root principal. Making it mean all principals in an account would be somewhat intuitive to somebody who has deep experience with IAM resource policies, but it would be very unintuitive to somebody who doesn't, and might lead them to think they're restricting a role much more tightly than they actually are. Also, changing the semantics now would be changing a role from being the most tightly restricted to being the least restricted, which would be bad from the perspective of users' security. So, we won't solve your use case by changing the meaning of the root principal. Given that, I think the discussion of how to handle specifying root users is separate from this discussion, so I created a separate issue for it in #3198 and we can move this discussion there.

As for your issue, something I've been thinking about is allowing specifying wildcards in bound_iam_principal_arn, e.g., bound_iam_principal_arn=arn:aws:iam::123456789012:user/VaultUsers/* to match all users that have a path of /VaultUsers/. Or, for your specific use case, we could do bound_iam_principal_arn=arn:aws:iam::123456789012:* and we'd just match any IAM principal in that account. It's also fairly idiomatically AWS and isn't confusing.

Thoughts on this @bmoylan? Would this work for you?

@bmoylan
Copy link
Author

bmoylan commented Aug 17, 2017

Yep, a wildcard would absolutely fulfill my immediate needs.

@jefferai jefferai modified the milestones: next-release, 0.8.2 Aug 18, 2017
@joelthompson
Copy link
Contributor

Cool, I've started writing the code for wildcards, should hopefully have a PR ready in the next couple days :)

joelthompson added a commit to joelthompson/vault that referenced this issue Aug 19, 2017
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
@jefferai jefferai assigned jefferai and vishalnayak and unassigned jefferai Aug 29, 2017
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 a pull request may close this issue.

4 participants