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

Add Support for Static Account #107

Merged
merged 35 commits into from Jul 8, 2021

Conversation

lawliet89
Copy link
Contributor

@lawliet89 lawliet89 commented Feb 24, 2021

Overview

Fixes #60.

This is a cleanup of the original PR #67.

It adds support for Vault to use pre-created GCP Service Accounts and issue tokens/keys.

There are some scenarios where the environment is wary of allowing automated tools like Vault access to x.setIamPolicy permissions. This PR allows users to create these service accounts first, and then let Vault issue credentials.

Related Issues/Pull Requests

$ make test-acc
?       github.com/hashicorp/vault-plugin-secrets-gcp   [no test files]
ok      github.com/hashicorp/vault-plugin-secrets-gcp/plugin    399.185s
ok      github.com/hashicorp/vault-plugin-secrets-gcp/plugin/iamutil    3.171s
?       github.com/hashicorp/vault-plugin-secrets-gcp/plugin/iamutil/internal   [no test files]
ok      github.com/hashicorp/vault-plugin-secrets-gcp/plugin/util       0.005s
?       github.com/hashicorp/vault-plugin-secrets-gcp/scripts/gohelpers [no test files]

Docs will be added to upstream Vault once the code is reviewed and merged in.

Contributor Checklist

[ ] Add relevant docs to upstream Vault repository, or sufficient reasoning why docs won’t be added yet
[x] Add output for any tests not ran in CI to the PR description (eg, acceptance tests)
[x] Backwards compatible

@lawliet89 lawliet89 mentioned this pull request Feb 24, 2021
3 tasks
@lawliet89
Copy link
Contributor Author

Hey @calvn and @kalafut, this is the second part of the static account PR that @emilymye started working on a while ago.

@husunal
Copy link

husunal commented Feb 24, 2021

@lawliet89 thanks for working on this. This is one of the key features that we would love to see in Vault's GCP secret backend.

@lawliet89
Copy link
Contributor Author

lawliet89 commented Mar 8, 2021

@husunal It might be a while before this gets reviewed going by the usual timelines. In the meantime, you can actually compile this yourself and load it up into Vault as a plugin to use it. Process is a bit cumbersome though. See https://www.vaultproject.io/docs/internals/plugins

I have a build for Linux AMD64 here but for a security product like this, I think it's better off if you can compile and verify it yourself.

@lawliet89
Copy link
Contributor Author

Hey @calvn and @kalafut, could I get this looked at please?

@peter-fe
Copy link

@lawliet89, thx for pushing that forward. Lovely feature that would perfectly fit CI/CD needs, having a static SA in the project with all needed permissions and you could get a (short lived) token for that account via Vault in the build pipelines.
This decoupling would massively ease the current process of having those permissions reflected through temp. Vault created SAs.

@nrbunn
Copy link

nrbunn commented May 13, 2021

@lawliet89 100% agree with @peter-fe . Watching closely for this merge.

@kalafut
Copy link
Contributor

kalafut commented May 19, 2021

@lawliet89 Yes, I'd like to review this one soon. Thank you for your work on it!

Copy link
Member

@austingebauer austingebauer left a comment

Choose a reason for hiding this comment

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

Thanks carrying this forward, @lawliet89! Just started reviewing this and left some initial comments. I'll circle back for another review shortly.

Are you able to provide some Vault CLI usage examples of the feature? I was able to get most of the way there, but I had some trouble generating a service account key for a static account. Examples would be super helpful and valuable context for the PR.

plugin/path_role_set.go Outdated Show resolved Hide resolved
plugin/static_account.go Outdated Show resolved Hide resolved
plugin/path_static_account.go Outdated Show resolved Hide resolved
plugin/path_static_account.go Outdated Show resolved Hide resolved
plugin/path_static_account_secrets.go Outdated Show resolved Hide resolved
plugin/path_static_account_secrets.go Outdated Show resolved Hide resolved
plugin/path_static_account_secrets.go Outdated Show resolved Hide resolved
plugin/path_static_account_secrets.go Outdated Show resolved Hide resolved
plugin/path_static_account_secrets.go Outdated Show resolved Hide resolved
plugin/path_static_account_secrets.go Outdated Show resolved Hide resolved
@lawliet89
Copy link
Contributor Author

lawliet89 commented Jun 28, 2021

Hi @austingebauer thanks for the review.

I've cooked up a demo with Terraform on how you can configure a Vault server with a custom build of this plugin installed: https://github.com/lawliet89/vault-static-demo

$ vault read gcp-static-test/static/test/key
Key                 Value
---                 -----
lease_id            gcp-static-test/static/test/key/OvWX6161yc3k9QDWVOIRZuyu
lease_duration      1h
lease_renewable     true
key_algorithm       KEY_ALG_RSA_2048
key_type            TYPE_GOOGLE_CREDENTIALS_FILE
private_key_data xxxxxxx

I might need your help with some aspects of the PR as we go back and forth because:

  • I wasn't the original author and I modified it to resolve conflicts and fix crashes to make it work.
  • I am not very faimilar with Vault internals
  • It's been more than half a year since I worked on the PR

- Remove unnecessary check
- Fix description
- Fix some inconsistencies
@lawliet89
Copy link
Contributor Author

I've answered/addressed the review comments.

Also ran the integration tests and everything looks OK.

calvn
calvn previously approved these changes Jul 6, 2021
Copy link
Member

@calvn calvn left a comment

Choose a reason for hiding this comment

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

One more comment around the static account path, but otherwise looks good. Thanks a ton for picking this up!

austingebauer
austingebauer previously approved these changes Jul 6, 2021
Copy link
Member

@austingebauer austingebauer left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍 Thanks again for carrying this forward!

Did you happen to already prepare documentation for this feature? If not, I'd be happy to help with that contribution over in the Vault repo.

@lawliet89 lawliet89 dismissed stale reviews from austingebauer and calvn via 8fba665 July 7, 2021 06:18
@lawliet89
Copy link
Contributor Author

lawliet89 commented Jul 7, 2021

@austingebauer I've not prepared the documentation yet.

@calvn I've renamed the prefix to static-account. See my reply at #107 (comment)

Re-running the acceptance tests:

$ make test-acc
?       github.com/hashicorp/vault-plugin-secrets-gcp   [no test files]
ok      github.com/hashicorp/vault-plugin-secrets-gcp/plugin    363.110s
ok      github.com/hashicorp/vault-plugin-secrets-gcp/plugin/iamutil    5.718s
?       github.com/hashicorp/vault-plugin-secrets-gcp/plugin/iamutil/internal   [no test files]
ok      github.com/hashicorp/vault-plugin-secrets-gcp/plugin/util       (cached)
?       github.com/hashicorp/vault-plugin-secrets-gcp/scripts/gohelpers [no test files]

Also, due to timezone differences, please feel free to make minor changes to the PR to get it accepted. Don't have to wait for me to respond 12 hours later 😅

@sethvargo sethvargo mentioned this pull request Jul 7, 2021
Copy link
Member

@austingebauer austingebauer left a comment

Choose a reason for hiding this comment

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

Thanks, @lawliet89. LGTM. I'm going to merge this now.

I'm happy to write the documentation for the feature. Also open to collaborating on it 👍

@austingebauer austingebauer merged commit 40f93fe into hashicorp:master Jul 8, 2021
austingebauer added a commit that referenced this pull request Jul 8, 2021
Co-authored-by: Emily Ye <emilyye@google.com>
Co-authored-by: Calvin Leung Huang <1883212+calvn@users.noreply.github.com>
Co-authored-by: Austin Gebauer <34121980+austingebauer@users.noreply.github.com>
@lawliet89
Copy link
Contributor Author

@austingebauer I've opened a draft PR to work on the documentation: hashicorp/vault#12027

Please feel free to make any edits.

@austingebauer
Copy link
Member

austingebauer commented Jul 9, 2021

Thanks for doing that, @lawliet89! I'll work on the docs with you off of that branch.

@peter-fe
Copy link

peter-fe commented Jul 21, 2021

@calvn, @austingebauer can you give a rough estimate when this will be released?
Seems the last release was at start of the year.

@austingebauer
Copy link
Member

@peter-fe - This is targeted to be released in Vault 1.8 which should go out around the end of July.

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.

GCP secrets: please support issuing keys/tokens for static service accounts
8 participants