keyring: warn if removing a key that was used for encrypting variables #24766
Conversation
There was a problem hiding this comment.
Thanks for doing this @pkazmierczak!
I tested this locally and couldn't get a delete to work even when using the force flag. I believe we need to populate the RPC request force parameter from the HTTP request here in order for the HTTP->RPC translation to handle this new parameter.
It would also be good if we could address the following items:
- The CLI help output doesn't include the new flag option.
- It would be nice to have API and CLI tests to cover this addition as the RPC test did not catch the issue detailed above.
- Document the API and CLI changes.
.changelog/24766.txt
Outdated
| @@ -0,0 +1,3 @@ | |||
| ```release-note:improvement | |||
| keyring: Warn if deleting a key previously used to encrypt a variable | |||
There was a problem hiding this comment.
Is it worth expanding on this to note the variable should still exist? It immediately reads to me that the warning applies to any key used in the past.
| return wm, err | ||
| } | ||
|
|
||
| // KeyringDeleteOptions are parameters for the Delete API | ||
| type KeyringDeleteOptions struct { | ||
| KeyID string // UUID | ||
| Force bool |
There was a problem hiding this comment.
I know not many options have this, but could we add a comment to this explaining what the force boolean does? It means external tool developers have an easy time knowing and understanding.
nomad/keyring_endpoint.go
Outdated
| return err | ||
| } | ||
| if rootKeyInUse && !args.Force { | ||
| return fmt.Errorf("root key in use, cannot delete") |
There was a problem hiding this comment.
There is no formatting, so we could use the below (the suggestion won't compile as the file also needs the new import):
| return fmt.Errorf("root key in use, cannot delete") | |
| return errors.New("root key in use, cannot delete") |
There was a problem hiding this comment.
yeah force of habit I guess, but errors.New is a better choice here
|
@jrasell I addressed all your comments, thanks for the thorough review. |
This PR adds an additional check in the
Keyring.DeleteRPC to make sure we're not trying to delete a key that's been used to encrypt a variable. It also adds a-forceflag for the CLI/API to sidestep that check.Resolves #24591
Internal ref: https://hashicorp.atlassian.net/browse/NET-11829