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.

Sign up for GitHub

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

Jump to bottom

Vault AWS EC2 auth #190

Merged
merged 4 commits into from
Aug 8, 2017
Merged

Vault AWS EC2 auth #190

merged 4 commits into from
Aug 8, 2017

Conversation

stuart-c
Copy link
Contributor

@stuart-c stuart-c commented Aug 4, 2017

This requires #177 to be merged first.

Add AWS ec2 auth support for Vault.

@stuart-c stuart-c changed the title Vault aws auth Vault AWS auth Aug 4, 2017
@hairyhenderson
Copy link
Owner

Awesome! This'll need to be rebased now that #177 is merged.

@stuart-c stuart-c force-pushed the vault-aws-auth branch 6 times, most recently from 1ce039e to fba6ee6 Compare August 4, 2017 14:20
@stuart-c stuart-c mentioned this pull request Aug 4, 2017
Closed
@stuart-c stuart-c changed the title Vault AWS auth Vault AWS EC2 auth Aug 4, 2017
hairyhenderson
hairyhenderson reviewed Aug 5, 2017
View reviewed changes
vault/auth.go Outdated
return ""
}

if skip := env.Getenv("VAULT_AUTH_AWS_EC2_SKIP", ""); skip != "" {
Copy link
Owner

Choose a reason for hiding this comment

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

I think if VAULT_AUTH_AWS_METHOD is required (i.e. must be set to ec2, and doesn't default), then this environment variable can be dropped.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking of moving it last in the order so both environment variables can be removed

Copy link
Owner

Choose a reason for hiding this comment

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

Isn't VAULT_AUTH_AWS_METHOD required to differentiate between ec2 and iam? Or do you mean you'd default to ec2?

Copy link
Owner

Choose a reason for hiding this comment

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

As far as moving it last in order, that's probably fine. There are cases where I use gomplate in CI environments and pass it a VAULT_TOKEN var so it can auth correctly. Those CI environments are generally in EC2, so it'd be good to not get tripped up there.

hairyhenderson
hairyhenderson reviewed Aug 5, 2017
View reviewed changes
vault/auth.go Outdated
logFatal("Invalid AWS_TIMEOUT value '%s' - must be an integer\n", timeout)
}

opts.Timeout = time.Duration(t) * time.Millisecond
Copy link
Owner

Choose a reason for hiding this comment

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

I just exposed a new MustAtoi in github.com/hairyhenderson/gomplate/typeconv, so you can do this instead:

opts.Timeout = time.Duration(typeconv.MustAtoi(os.Getenv("AWS_TIMEOUT"))) * time.Millisecond

@stuart-c stuart-c force-pushed the vault-aws-auth branch 5 times, most recently from e04d302 to 008b470 Compare August 6, 2017 07:54
@stuart-c stuart-c changed the title Vault AWS EC2 auth [WIP] Vault AWS EC2 auth Aug 6, 2017
@stuart-c
Copy link
Contributor Author

stuart-c commented Aug 6, 2017

I'm just trying to add some integration tests.

hairyhenderson
hairyhenderson reviewed Aug 6, 2017
View reviewed changes
vault/auth.go Outdated
opts := aws.ClientOptions{}

timeout := os.Getenv("AWS_TIMEOUT")
if timeout != "" {
Copy link
Owner

Choose a reason for hiding this comment

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

There's actually no need to have this check, because opts.Timeout's default is 0 anyways. There's no harm to simply setting it to 0 explicitly.

You could make things even simpler by setting Timeout while creating the struct (up on ln169):

opts := aws.ClientOptions{
  Timeout: time.Duration(typeconv.MustAtoi(os.Getenv("AWS_TIMEOUT"))) * time.Millisecond,
}

@stuart-c
Copy link
Contributor Author

stuart-c commented Aug 6, 2017

@hairyhenderson could you take a look at this now?

@stuart-c stuart-c changed the title [WIP] Vault AWS EC2 auth Vault AWS EC2 auth Aug 6, 2017
@hairyhenderson
Copy link
Owner

@stuart-c the integration test is failing:

not ok 33 Testing ec2 vault auth
# (in test file test/integration/datasources_vault.bats, line 134)
#   `vault auth-enable aws' failed with status 2
# Success! Data written to: secret/foo
# Error: Error making API request.
# 
# URL: POST http://127.0.0.1:8200/v1/sys/auth/aws
# Code: 400. Errors:
# 
# * unknown backend type: aws
# Success! Deleted 'secret/foo' if it existed.
# Disabled auth provider at path 'userpass' if it was enabled
# Disabled auth provider at path 'userpass2' if it was enabled
# Disabled auth provider at path 'approle' if it was enabled
# Disabled auth provider at path 'approle2' if it was enabled
# Disabled auth provider at path 'app-id' if it was enabled
# Disabled auth provider at path 'app-id2' if it was enabled
# Disabled auth provider at path 'aws' if it was enabled
# Policy 'writepol' deleted.
# Policy 'readpol' deleted.
# Successfully unmounted 'ssh' if it was mounted

hairyhenderson
hairyhenderson reviewed Aug 7, 2017
View reviewed changes
vault/auth.go Outdated
meta := aws.NewEc2Meta(opts)

if endpoint := env.Getenv("AWS_META_ENDPOINT"); endpoint != "" {
meta.Endpoint = endpoint
Copy link
Owner

Choose a reason for hiding this comment

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

I think this block belongs here instead: https://github.com/hairyhenderson/gomplate/blob/master/aws/ec2meta.go#L89

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean you'd prefer the AWS_META_ENDPOINT to be set in the ec2meta file?

Copy link
Owner

Choose a reason for hiding this comment

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

I'd prefer it to be read there - so that the metadata endpoint can be overridden in general, not just when using Vault EC2 auth

hairyhenderson
hairyhenderson reviewed Aug 7, 2017
View reviewed changes
vault/auth.go Outdated
meta.Endpoint = endpoint
}

vars["pkcs7"] = strings.Replace(strings.TrimSpace(meta.Dynamic("instance-identity/pkcs7")), "\n", "", -1)
Copy link
Owner

Choose a reason for hiding this comment

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

Why the extra strings.Replace? strings.TrimSpace will trim off \n characters... Or is this intended to remove all newlines in the middle of the string?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep all embedded newlines need to be removed.

@hairyhenderson
Copy link
Owner

the integration test is failing

Ah - it's because the version of Vault in hairyhenderson/gomplate-ci-build is too old - just pushed an update in hairyhenderson/dockerfiles@7af0f95

@hairyhenderson
Copy link
Owner

@stuart-c can you bump the Vault version in https://github.com/hairyhenderson/gomplate/blob/master/test/integration/Dockerfile#L3 so that make test-integration-docker succeeds?

@stuart-c
Copy link
Contributor Author

stuart-c commented Aug 7, 2017

That is now fixed, but the tests are hanging a bit further down. Not sure what's going on. Is it working for you locally?

@hairyhenderson
Copy link
Owner

Looks like you accidentally committed test/integration/aws and test/integration/meta - maybe should add those to a .gitignore

@hairyhenderson
Copy link
Owner

the tests are hanging a bit further down. Not sure what's going on

Maybe related to a CircleCI issue: https://status.circleci.com/incidents/1hf3n7yf8zrn?

@stuart-c
Copy link
Contributor Author

stuart-c commented Aug 7, 2017

Fixed the .gitignore

@stuart-c
Copy link
Contributor Author

stuart-c commented Aug 7, 2017

Looks to be a bats bug which I think I've now worked around...

So I think everything should now be good @hairyhenderson?

@hairyhenderson hairyhenderson merged commit d2cf55b into hairyhenderson:master Aug 8, 2017
@stuart-c stuart-c deleted the vault-aws-auth branch August 8, 2017 06:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hairyhenderson hairyhenderson hairyhenderson approved these changes

Assignees
No one assigned
Labels
api/minor enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@stuart-c @hairyhenderson