-
Notifications
You must be signed in to change notification settings - Fork 137
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
Add IAM EC2 auth #161
Add IAM EC2 auth #161
Conversation
@evanphx What we're working on now is how this can work for both standard EC2 instances versus ECS tasks. ECS tasks use a different credentials API at:
|
60f70e2 breaks the ECS/EC2 auth into two separate public methods that share an interface for iam |
@maschwenk -- was thinking about this a bit. The Vault team understandably doesn't want to pull the entirety of the AWS SDK for Ruby into the Vault Ruby API client because a number of users probably won't even want it, and it could cause version conflicts with clients using a different version. And unfortunately, all the "magic" that AWS does on your behalf to figure out where to load credentials from isn't easily separable from the rest of the SDK. And the way that you're doing it won't allow clients to use explicit credentials, environment-variable-provided credentials, or AssumeRole-provided credentials (a workaround I suggest on the mailing list from time to time because I've been too lazy implement allowing binding multiple IAM principal ARNs to a single Vault role); doing any of those would require more code changes. So, why not just move that little bit of complexity to the client, removing from client code the complexity of figuring out whether they have to call |
Also, @maschwenk, for future reference:
Per the AWS documentation (see step 5), "The host header must be included as a signed header. If you include a date or x-amz-date header, you must also include that header in the list of signed headers." |
@joelthompson sounds good. The only remaining piece in that puzzle is the region. Whereas previously we had it coming from the AWS Metadata API, we'd now need it to be statically passed in to the method, is that ok? |
I'm... not sure. I think that, for now, the best way to handle this is actually to just have clients pass in the endpoint and then infer the region from the endpoint, with the endpoint defaulting to As to why I recommend this: First, sorry this is such a long explanation, but this is a pretty complex topic that's not well documented. STS is one of the weirdest AWS services -- it is "global" and has a single default global endpoint. But, unlike the other global services that I can think of off the top of my head (i.e., IAM and Route53), it has region-specific endpoints as well! AWS doesn't have great documentation about how to handle their home-spun authentication method in this case, so I did a bit of experimenting using boto3 (the python SDK, even though this is the Ruby SDK and Vault uses the golang SDK, and these are all corner cases that might not be well defined across SDKs) as it's the one I'm most familiar with. Also an important note: the STS and IAM services are "global" but only within a "partition" and there are currently three partitions -- AWS Standard, GovCloud, and AWS China. It's unfortunately a bit tricky for users to test things out in one of the other partitions. Vault has some awkward code to try to handle this. In this context, the region is needed for two reasons:
The second point is a bit more subtle. The actual endpoint that the Vault server uses is determined solely by the Vault server and not at all by the client, defaulting to So, if Vault admins do nothing, my proposed solution (defaulting to the endpoint of |
lib/vault/api/auth.rb
Outdated
vault_headers = { | ||
'User-Agent' => Vault::Client::USER_AGENT, | ||
'Content-Type' => 'application/x-www-form-urlencoded; charset=utf-8', | ||
'X-Vault-AWSIAM-Server-Id' => iam_auth_header_value |
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.
It's actually X-Vault-AWS-IAM-Server-ID (hyphen between AWS and IAM)
…ved, defaulting to https://sts.amazonaws.com (us-east-1)
@joelthompson Let me know if above commits follow your guidance above. Thanks so much for the thorough explanation. Maybe it makes sense to link to your PR/comment with regards to the STS endpoint. |
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.
Generally looking good to me :) The only thing I think really should be changed is the default server header value, and that should be a small change.
lib/vault/api/auth.rb
Outdated
require "aws-sigv4" | ||
require "base64" | ||
|
||
valid_sts_endpoint = %r{https:\/\/sts.?(.*).amazonaws.com}.match(sts_endpoint) |
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.
FWIW, the STS endpoint for STS in the China (Beijing) region (cn-north-1) is sts.cn-north-1.amazonaws.com.cn
. This will still work because your match doesn't have a $
at the end, but might be worth commenting so that future people don't add it and inadvertently break support for cn-north-1.
The endpoint for GovCloud is sts.us-gov-west-1.amazonaws.com
so it should work exactly as intended.
lib/vault/api/auth.rb
Outdated
@@ -13,6 +13,10 @@ def auth | |||
end | |||
|
|||
class Authenticate < Request | |||
|
|||
# canary header used for aws_ec2_iam | |||
IAM_SERVER_ID_HEADER = "canaryHeaderValue".freeze |
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.
I'm not a fan of this. Right now, it happens to work in Vault that if you haven't configured the server ID header value, it will accept any or no header value. This isn't great behavior, and might be something that I'd want to change in the future, so I'd rather not have the ruby client depend on it. Can you just make the default value nil
and then just add the header value into the vault_headers
if it's non-nil
.
lib/vault/api/auth.rb
Outdated
role: role, | ||
iam_http_request_method: request_method, | ||
iam_request_url: Base64.strict_encode64(sts_url), | ||
iam_request_headers: Base64.strict_encode64(vault_headers.merge(sig4_headers).to_json), |
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 is probably OK, but just want to call out that this format for encoding the headers is only compatible with Vault starting with v0.8.0.
expect(Secret).to receive(:decode).and_return secret | ||
expect(::Aws::Sigv4::Signer).to( | ||
receive(:new).with( | ||
service: 'sts', region: 'cn-north-1', credentials_provider: credentials_provider |
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.
I think this expectation could obviate the need for the comment in e598ffc
e1871e8
to
6217a9f
Compare
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.
Generally looks good to me from a Vault/AWS perspective! Thanks for doing this work! :)
I'm not too fluent in ruby, though, so I'm not the best person to comment on the details.
lib/vault/api/auth.rb
Outdated
|
||
# STS in the China (Beijing) region (cn-north-1) is sts.cn-north-1.amazonaws.com.cn | ||
# Take care changing below regex with that edge case in mind | ||
valid_sts_endpoint = %r{https:\/\/sts.?(.*).amazonaws.com}.match(sts_endpoint) |
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.
I think you need to escape the . to be correct: https:\/\/sts\.?(.*)\.amazonaws\.com
spec/integration/api/auth_spec.rb
Outdated
|
||
describe "#aws_iam" do | ||
before(:context) do | ||
vault_test_client.sys.enable_auth("aws", "aws", nil) |
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.
I think it would be good to also configure the header value here and then insert it, just to ensure it gets processed properly.
lib/vault/api/auth.rb
Outdated
# @param [CredentialProvider] credentials_provider | ||
# https://docs.aws.amazon.com/sdk-for-ruby/v3/api/Aws/CredentialProvider.html | ||
# @param [String] iam_auth_header_value optional | ||
# As of Jan 2018, Vault will accept ANY or NO header, but this is subject to change and should not be relied upon |
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.
Would be better to read: "Vault will accept ANY or NO header if none is configured by the Vault server admin"
@joelthompson Do you have anyone who would want to look at this in its updated state? |
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.
Hey @maschwenk -- one question and a couple cosmetic notes.
I do want to avoid any confusion, though. I'm not a HashiCorp employee, just a Vault community member and contributor who has done a fair amount of work on the AWS auth backend and who would like to see this merged because I think the community would benefit from your efforts on this PR. I jumped in hoping that doing so would make HashiCorp more willing to accept your PR but can't promise anything.
spec/integration/api/auth_spec.rb
Outdated
@@ -216,6 +216,7 @@ module Vault | |||
describe "#aws_iam" do | |||
before(:context) do | |||
vault_test_client.sys.enable_auth("aws", "aws", nil) | |||
vault_test_client.sys.put_auth_tune("aws", "iam_server_id_header_value" => "iam_header_canary") |
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.
I think the .sys.put_auth_tune
here isn't right. The configuration of the header value is done via the auth/aws/config/client
API endpoint, not via a mount-tune call.
lib/vault/api/auth.rb
Outdated
# Take care changing below regex with that edge case in mind | ||
# | ||
# @param [String] sts_endpoint | ||
# The raw pem contents to use for the login procedure. |
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 looks like something left over from copy/pasting?
lib/vault/api/auth.rb
Outdated
@@ -186,6 +186,60 @@ def aws_ec2(role, pkcs7, nonce = nil) | |||
return secret | |||
end | |||
|
|||
# Authenticate via IAM EC2 method by providing a AWS CredentialProvider (either ECS, AssumeRole, etc.) |
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.
It would be better to say something like "Authenticate via AWS IAM auth method" as this doesn't use the ec2 auth method at all.
@evanphx / @sethvargo - any updates on getting this merged in or what else needs to be done for it to be included? |
I'm no longer employed at HashiCorp, so I can't help you. Sorry |
Oops, thanks for responding anyway though! |
@flyinbutrs @sethvargo I need to respond to Joel's good feedback sometime this weekend. We've had to monkeypatch this in our own projects to get it working. After I make the changes hopefully a Vault employee can review it |
@flyinbutrs I've responded to Joel's feedback. I also need to do a little manual integration testing myself, in my current project I used the initial implementation I had here but have not swapped out for this final solution |
Just did some integration testing on our own projects and this works excellently (currently just monkeypatching it in). It'd be great to get some feedback from a current employee of Hashicorp /cc @evanphx |
@flyinbutrs Apologies, I'm hesitant to make any fixes until I get some indication that this might get merged.... |
No worries, understood. I'm using it off of your branch, and it's working great! Hopefully someone from hashicorp can take a look soon. @briancain? I see you active from Hashicorp on other Ruby repos... Any chance you can poke the right person for vault-ruby? Or maybe @jefferai since you seem to be a maintainer on vault itself? |
@evanphx I saw you merged a bunch of PR's on this repo this week. Any way you could cut a new release? I have a dependency on this in a gemspec so I don't have the option of pointing to master. Thanks! |
I melded the gist that @tristanmorgan posted and #144. I also used this python snippet on the AWS documentation.
This uses the lightweight
aws-sigv4
gem as suggested by @evanphx. The gist above had a method for achieving it completely without a gem but I noticed the gist had (what appeared to me to be) some errors:It wasn't signing all the headers
It wasn't signing the body of the request:
whereas the ruby AWS SDK does and so does the python snippet:
The basic outline for what I did is this:
@evanphx There are a couple open questions on our side that I think you'd be in a good position to answer:
What headers are necessary inIn our testing it worked without itvault_headers
? Do we need things likecontent-length
?TODO: