gocd / gocd-vault-secret-plugin

GoCD secrets plugin for HashiCorp's Vault
https://gocd.org
14 stars 7 forks source link

Implement dynamic path specification at plugin callsite #160

Closed 12345ieee closed 9 months ago

12345ieee commented 11 months ago

This PR implements a feature requested in several issues and that I need as well: the ability to specify a secret path at the plugin callsite instead of hardcoding every secret path in a new secretConfig.

It does so by introducing a new path:key syntax in secret calls, so that the the secret path is obtained by concatenating the root path with the callsite paths. Some examples from the PR README, assume the VaultPath of the vault secretConfig is secret/gocd, then:

The backwards incompatibilities are minimal, only keys of the form a:b get treated differently than in master.

This PR contains a set of mildly opinionated commits, the meat is in cbb1d0893561a25e64daf7b00c8fd9b49fb714fa , but I think each of them would be of help to the codebase. It is possible to pick and choose any of them to merge though.

I've read in https://github.com/gocd/gocd-vault-secret-plugin/pull/102#issuecomment-1170214877 that the plugin is currently unmaintained and that the maintainer suggested a private fork. I'd rather give back to upstream and not make a private fork, but if needed I can help with maintenance efforts.

The changes in this PR come with enough unit testing to ensure correctness, I've been using my plugin version in my cicd server for several days with no issue.

Thank you for the creation of this plugin, it was very helpful.

Closes: #65 #114 Partially addresses: #94 (cherry-picked to master independent of this PR)

12345ieee commented 9 months ago

Hello, is there anything I can do to help the review of this? Do you need the disparate changes split in multiple PRs?

chadlwilson commented 9 months ago

Hey there. Yeah, thanks again for this. You appear to have thought it through well, which is appreciated.

I'll cherry-pick a few of the commits to master to simplify and handle the lower risk stuff so we can focus this PR on the feature you want to add.

I don't actively use this plugin or Vault which is why this plugin has a bit more review friction (compared to others where I can easily sanity check and have necessary technical context), but if you have some tips for an easy way to play with a local Vault (e.g in a container) to sanity test this from an integration perspective, that might lower the barrier for me. I'm also open to adding external collaborators on the plugin, possibly moving it to gocd-contrib where it might better sit due to lack of core maintainer experience.

chadlwilson commented 9 months ago

I've reviewed and cherry-picked all but the "meat", and rebased your branch/PR to include only the feature change, which I'll try and get a chance to think through and sanity check :-)

12345ieee commented 9 months ago

Run docker run -p 8200:8200 hashicorp/vault.

It'll spit out a lot of stuff, then say: Root Token: hvs.<stuff> That's an omnipowerful token.

Then point a browser to http://localhost:8200/ui , log in with that token and add in all the test secrets you want from the webapp.

That vault content will be gone when you restart the container, so plan your tests well.

chadlwilson commented 9 months ago

Any reason to not just use / as the break and treat anything after the last / as the key?

Might reduce the need for having to think about escape syntax if : is allowed in path names? And i understand correctly, a bit more consistent with how you do things in the CLI with Vault?

12345ieee commented 9 months ago

The reason is twofold:

: was my choice for how separator-y felt, if it's good for unix PATH it's good for this as well

chadlwilson commented 9 months ago

OK, fair enough. Just getting a bit worried about all the syntax-within-syntax-within-syntax going on here and whether we'll back ourselves into a corner with more potential things to deal with.

But yeah, I guess the feature needs to clobber some syntax unless we make it more complicated by adding escaping logic; but probably not worth the effort.

12345ieee commented 9 months ago

I know it's not ideal, but introducing, say \: and \/ will back us into very bad corners.

If this causes any issues we can attack them as they come.

chadlwilson commented 9 months ago

See https://github.com/gocd/gocd-vault-secret-plugin/releases/tag/v1.3.0-282 - thanks!