Add new aws-iam authentication backend #1962
Conversation
The aws-iam authentication backend is able to validate arbitrary AWS IAM principals against AWS. It does this by receiving a signed AWS STS GetCallerIdentity request and forwarding that on to AWS. AWS will validate the request and return the principal, which allows Vault to then map the caller to a set of policies.
| $ vault write auth/aws-iam/role/dev-role bound_iam_principal=arn:aws:iam::123456789012:role/my_role policies=prod,dev max_ttl=500h | ||
| ``` | ||
|
|
||
| #### Configure a required X-Vault-Server Header (recommended) |
There was a problem hiding this comment.
I'd love some feedback. It would be great to require this header be set, but I'm not sure what the right default is. redirect_addr seems like a promising start, but I'm not sure if there's a good way to access that configuration variable from within an authentication backend. Any thoughts?
There was a problem hiding this comment.
I don't particularly want it to be required for all as it adds extra burden without benefit on dev setups or single-Vault shops. An administrator can easily make it required.
|
Also, this implements the scheme in #948 |
|
Yes, please. |
|
@joelthompson when did AWS implement GetCallerIdentity I remember talking about it in: #948. |
|
@fieldju -- not 100% sure. Support for it was committed in boto/botocore@1ddefda on April 5, so likely sometime around then. |
|
Yeah, im pretty stocked for this. Can anyone here speak to when 0.6.4 might get released? |
|
It's tentatively scheduled for some time in January. |
|
Why not merge now? |
|
Because we're currently at the end of the 0.6.3 timeframe. 0.6.4 comes after that. |
builtin/credential/aws-iam/cli.go
Outdated
| var err error | ||
|
|
||
| access_key, ok := m["aws_access_key_id"] | ||
| if ok { |
There was a problem hiding this comment.
Please use helper/awsutil to generate the credential chain as this guarantees similar behavior throughout Vault. When used within the Vault server, the http client should use cleanhttp as well.
There was a problem hiding this comment.
Done (on helper/awsutil)
| configEntry = &clientConfig{} | ||
| } | ||
|
|
||
| endpointStr := data.Get("endpoint").(string) |
There was a problem hiding this comment.
You should use GetOk to know whether or not blank values are purposeful or not, and then key what to do based on the OK value and the operation type. See the token store roles create/update function in vault/token_store.go.
There was a problem hiding this comment.
Good catch, thanks. Done.
|
|
||
| // In the future, might consider supporting GET | ||
| if method != "POST" { | ||
| return logical.ErrorResponse("Invalid method; currently only 'POST' is supported"), nil |
|
|
||
| // TODO: There are two potentially valid cases we're not yet supporting that would | ||
| // necessitate this check being changed. First, if we support GET requests. | ||
| // Second if we support presigned POST requests |
There was a problem hiding this comment.
Two stupid questions here. First, what are presigned POST requests in this context, since the client is submitting headers to Vault, and second, if those were to be supported, would it require breaking changes?
There was a problem hiding this comment.
Definitely not stupid questions, you're getting into some of the subtle inner workings of the AWS signature protocol!
A "presigned" request, in AWS parlance, is where the client embeds the signature information inside of the URI query string, rather than as separate HTTP headers. The idea is that this "presigned" request URI is handed out as a time-limited means of granting access to a particular resource (such as an object in an S3 bucket), which is a pretty similar concept to what we're doing here. Normally, this is done with GET requests, but there's no reason in principle that you couldn't do this with POST requests. In fact, in boto3 at least, if you call the generate_presigned_url method for GetCallerIdentity, it'll actually give you a presigned POST request because deep within the boto3 model, it thinks that GetCallerIdentity should always be a POST request. (See boto/boto3#756 for an example of this.)
And I'm pretty sure that supporting it wouldn't require breaking changes. Supporting GET would just involve removing that the non-empty body check and replacing it with making sure there's an action in the URI query string, and supporting presigned requests (whether GET or POST) would require teaching Vault to look in the query string to ensure that the X-Vault-Server (or whatever header name is decided upon) is included in the signed requests.
There was a problem hiding this comment.
OK. So, seems like this is good for now!
There was a problem hiding this comment.
Cool, though in thinking more, I think I should to update the docs to be explicit about this choice of not supporting presigned GET or POST requests (leaving this comment as a reminder to myself to do the docs update).
| if err != nil { | ||
| return logical.ErrorResponse("headers is invalid base64"), nil | ||
| } | ||
| var headers map[string]string |
There was a problem hiding this comment.
Is there ever a reason when a header being submitted might need to be duplicated? If so the client should send map[string][]string, IOW it should use http.Header.
There was a problem hiding this comment.
Need to? No. RFC 2616 is pretty adamant about that (yeah, I had to look that one up myself when working on this):
Multiple message-header fields with the same field-name MAY be present in a message if and only if the entire field-value for that header field is defined as a comma-separated list [i.e., #(values)]. It MUST be possible to combine the multiple header fields into one "field-name: field-value" pair, without changing the semantics of the message, by appending each subsequent field-value to the first, each separated by a comma.
There was a problem hiding this comment.
Yep -- I know about how you can do that, but there are definitely caveats. That's why I was asking above about what the Python client is doing there, if it is just replacing, or if it is squishing everything with commas.
In a general sense it feels to me like if we are sending headers over the API and want to be mildly standardized about it if we end up doing the same thing elsewhere in Vault (as any long-time user knows it's been a battle to get things more standardized) we should use the data structures that allow the user to more exactly specify what they should look like without making assumptions about whether they can be string-listed and so on.
But, feel free to convince me otherwise :-)
There was a problem hiding this comment.
The more I think about this, the more I think you're probably right. Vault is in go, the code should use go idioms as much as possible. Give me a few days to rewrite this using the go-standard format and also see what the python client looks like and I'll either have an updated PR or some actual code showing why this would be uglier :)
| request := buildHttpRequest(method, endpoint, parsedUrl, body, headers) | ||
| client := &http.Client{} | ||
| response, err := client.Do(request) | ||
| defer response.Body.Close() |
There was a problem hiding this comment.
You need to check that the response isn't nil before doing this defer.
builtin/credential/aws-iam/cli.go
Outdated
| svc := sts.New(stsSession) | ||
| stsRequest, _ := svc.GetCallerIdentityRequest(params) | ||
| if headerValue != "" { | ||
| stsRequest.HTTPRequest.Header.Add("X-Vault-Server", headerValue) |
There was a problem hiding this comment.
This should use the const value you declare later.
| }, | ||
| "policies": { | ||
| Type: framework.TypeString, | ||
| Default: "default", |
There was a problem hiding this comment.
Don't need "default" here as it gets added independently of the backend-defined policies.
There was a problem hiding this comment.
cool, thanks. Done.
| func (b *backend) pathRoleRead( | ||
| req *logical.Request, data *framework.FieldData) (*logical.Response, error) { | ||
|
|
||
| roleEntry, err := b.lockedAWSRole(req.Storage, strings.ToLower(data.Get("role").(string))) |
There was a problem hiding this comment.
Should check that role isn't empty.
There was a problem hiding this comment.
done (via nonLockedAWSRole)
| $ vault write auth/aws-iam/role/dev-role bound_iam_principal=arn:aws:iam::123456789012:role/my_role policies=prod,dev max_ttl=500h | ||
| ``` | ||
|
|
||
| #### Configure a required X-Vault-Server Header (recommended) |
There was a problem hiding this comment.
I don't particularly want it to be required for all as it adds extra burden without benefit on dev setups or single-Vault shops. An administrator can easily make it required.
builtin/credential/aws-iam/cli.go
Outdated
| var err error | ||
|
|
||
| access_key, ok := m["aws_access_key_id"] | ||
| if ok { |
There was a problem hiding this comment.
Done (on helper/awsutil)
builtin/credential/aws-iam/cli.go
Outdated
| svc := sts.New(stsSession) | ||
| stsRequest, _ := svc.GetCallerIdentityRequest(params) | ||
| if headerValue != "" { | ||
| stsRequest.HTTPRequest.Header.Add("X-Vault-Server", headerValue) |
builtin/credential/aws-iam/cli.go
Outdated
| stsRequest.HTTPRequest.Header.Add("X-Vault-Server", headerValue) | ||
| } | ||
| stsRequest.Sign() | ||
| headersJson, err := json.Marshal(transformHeaders(stsRequest.HTTPRequest.Header)) |
There was a problem hiding this comment.
I don't understand your question, sorry.
Are you asking why I'm JSON-encoding the request headers? It's because I need to encode the headers of what the STS request would be and send them to the aws-iam backend so that the backend can replay those headers (versus the headers on the request to the Vault server itself).
Are you asking why I'm calling the transformHeaders method? It's a bit arbitrary, but I found that Python and Go HTTP clients treat the headers a bit differently, in that Go maps a string to an array of strings, while Python maps a string to a single string. My original test client was Python, so I wrote the backend to support that, and then when I wrote the Go client, I discovered the discrepancy and felt that keeping the Python representation would lead to simpler code without a loss of generality, so I decided to keep it.
Are you asking something else I'm not understanding?
| configEntry = &clientConfig{} | ||
| } | ||
|
|
||
| endpointStr := data.Get("endpoint").(string) |
There was a problem hiding this comment.
Good catch, thanks. Done.
|
|
||
| // In the future, might consider supporting GET | ||
| if method != "POST" { | ||
| return logical.ErrorResponse("Invalid method; currently only 'POST' is supported"), nil |
|
|
||
| ## Comparison with AWS-EC2 Authentication Backend | ||
|
|
||
| The AWS-IAM and AWS-EC2 authentication backends serve similar purposes. Both |
There was a problem hiding this comment.
Sorry, I was honestly trying to make it as unbiased as possible. I guess I'm just not seeing the "significant drawbacks" that you're seeing. Can you provide some more details about what you think is missing so I can make this more fair and balanced?
|
|
||
| ## Authentication Workflow | ||
|
|
||
| AWS has introduced the [`sts:GetCallerIdentity`](http://docs.aws.amazon.com/STS/latest/APIReference/API_GetCallerIdentity.html) |
There was a problem hiding this comment.
I've reworded a little, let me know if this reads better.
|
|
||
| Each signed AWS request includes the current timestamp to mitigate the risk of | ||
| replay attacks. In addition, Vault allows you to require an additional header, | ||
| `X-Vault-Server`, to be present to mitigate against different types of replay |
There was a problem hiding this comment.
Fair enough. How about X-Vault-AWSIAM-Server-ID?
| $ vault write auth/aws-iam/role/dev-role bound_iam_principal=arn:aws:iam::123456789012:role/my_role policies=prod,dev max_ttl=500h | ||
| ``` | ||
|
|
||
| #### Configure a required X-Vault-Server Header (recommended) |
| return nil, fmt.Errorf("role entry not found") | ||
| } | ||
|
|
||
| if roleEntry.BoundIamPrincipal != canonicalArn { |
There was a problem hiding this comment.
That's a good question. The only thing Vault sees, by design, is the access key and the session token (if it's an STS token). There's no good way, in AWS, to determine which IAM principal an access key belongs to. The only real way of doing that is by calling the GetCallerIdentity method, which requires the secret key in order to sign the request, and by design, Vault never sees the secret key. Further, as @copumpkin states, the request itself will expire within 15 minutes, so it's not feasible to really store it.
It is possible, as @copumpkin states, to add a date constraint to an IAM role policy to cut off access for all session tokens that were issued before a certain date. However, as best as I can tell, GetCallerIdentity doesn't actually do any authorization checks, and so this trick won't work for denying future GetCallerIdentity requests.
If a Vault admin is concerned by this, then I think the right solution is some combination of low token TTL (so forcing a periodic renewal) and/or low max token TTL or just disabling renewal altogether (so forcing a revalidation that the client has valid IAM creds).
@jefferai -- in your most recent comment, you're quoting a section about GetSessionToken and not GetCallerIdentity. And, on your confusion, in my experience, I think that's inconsistent documentation standards. In this context, I think that the "account credentials" is credentials for the root user (which should almost never be used), IAM users are long-lived, static creds that we're desperately trying to avoid building a dependency on, and then you have short-lived creds that we like because they expire automatically. So, yes, the user could submit a new signed request with a renewal, but I think that breaks the paradigm of a "renewal" (which is just the client presenting the original token, without additional authenticating information, and asking for the lifetime to be extended, right?). I think that, if you want the user to have to present a new signed request, the easiest and best way to do that would be to just set a low TTL and max TTL (e.g., disallow renewals) and force the client to get a new authentication token.
This would probably be good to capture in the docs. @jefferai -- let me know if this makes sense to you and I'll add it.
Also improving the documentation
|
OK, I think I've addressed all the outstanding comments, please let me know if I've missed something, or if you've got more feedback! |
|
I can't speak to reviewing the code right now - most of it is over my head - but I will say that if (as seems to be the case) this provides a solution to the first secret problem in both Lambda and ECS, I can't overstate how important and useful this is. |
|
It depends if IAM roles is enough granularity. That is likely to depend on use case (or AWS architecture) and security policy since you can't map to particular instantiations. We're still in contact with the AWS team on adding more useful primitives so that tasks can be uniquely identified. |
|
Hello, I was just curious about the status of this PR, I noticed that you moved it from 0.6.5 -> 0.7.0. Cheers, |
|
@jefferai and I spoke offline about incorporating the matching of EC2 instances, so I've got most of that done, and will open a new PR when ready. |
|
Hi @joelthompson @jefferai - it looks like this might not make 0.7.0. Is that so? |
|
Correct, but hopefully it'll make it for 0.7.1. |
|
Also, everyone, sorry it took so long, but I've been travelling quite a bit recently and hadn't had time to implement the changes I needed to make until now. |
|
Closing in favor of #2441 |
The aws-iam authentication backend is able to validate arbitrary
AWS IAM principals against AWS. It does this by receiving a signed AWS
STS GetCallerIdentity request and forwarding that on to AWS. AWS will
validate the request and return the principal, which allows Vault to
then map the caller to a set of policies.
Many thanks to @copumpkin for his feedback in reviewing this thus far!