Add path for retrieving the public key #5
Conversation
| }, | ||
|
|
||
| Callbacks: map[logical.Operation]framework.OperationFunc{ | ||
| logical.ReadOperation: withFieldValidator(b.pathPubkeyRead), |
There was a problem hiding this comment.
Should this be an Update operation instead since you're requiring key_version?
There was a problem hiding this comment.
Hmm I actually think it should be GET. It's possible to have params for get requests (they turn into URL params). I'm going to fix the curl command because vault write gcpkms/pubkey/my-key key_verison=1 doesn't feel right.
There was a problem hiding this comment.
I agree that read feels correct, though as is now the key_version would be a query parameter which is uncommon in Vault (AFAIK), and a little odd with it being required. Maybe it could be a path element, which is a pattern that would match: https://www.vaultproject.io/api/secret/transit/index.html#export-key
@briankassouf Might have some thoughts on the API conventions here.
There was a problem hiding this comment.
As in vault read gcpkms/pubkey/my-key/1? I'm not opposed to that, but can @briankassouf weight in quickly before I make that change?
There was a problem hiding this comment.
Using a GET parameter is fine, it's not a common pattern because it wasn't part of our framework implementation until recently. The kv version 2 API uses get parameters to retrieve a specific version of a secret. This aligns with that same usecase.
There was a problem hiding this comment.
Okay let's keep it as-is then 😄- thanks bk. @kalafut what other changes do we need to merge this
There was a problem hiding this comment.
Hey @sethvargo one last question. The KVV2 API defaults to the latest key version if version isn't provided. Does that same concept make sense here? I briefly skimmed the Google docs but it wasn't totally clear to me.
There was a problem hiding this comment.
I thought about it, but it would involve an extra API call (we'd have to list the key versions from the KMS api and grab the latest one). Two problems arise:
- What is "latest"? Is it the latest active key or the most recently created key?
- What if the user doesn't have permission to list all the keys.
That's why I opted to keep it required.
|
|
||
| | Method | Path | Produces | | ||
| | :------- | :--------------------------| :------------------------ | | ||
| | `GET` | `gcpkms/pubkey/:key` | `200 application/json` | |
There was a problem hiding this comment.
Change to POST if the code review comments re: Update are made. Your curl example is already correct though 🙂.
|
I can't merge, but 👍 |
|
Thanks! |
Adds a new path for retrieving the public key.
The docs changes will actually need to go over to the main Vault repo. I wish there was a way to pull both the plugin and its docs into Vault's build instead of having the docs live separately.
Once this is merged, I can open another PR to update the docs.