From 8db579b80162514344041ee0aacb17b99e0744c6 Mon Sep 17 00:00:00 2001 From: Piotr Kazmierczak <470696+pkazmierczak@users.noreply.github.com> Date: Tue, 31 Dec 2024 16:43:52 +0100 Subject: [PATCH 1/9] keyring: warn if removing a key that was used for encrypting variables --- api/keyring.go | 6 ++++-- command/operator_root_keyring_remove.go | 4 +++- nomad/keyring_endpoint.go | 9 +++++++++ nomad/structs/keyring.go | 1 + 4 files changed, 17 insertions(+), 3 deletions(-) diff --git a/api/keyring.go b/api/keyring.go index 69e0c9c1073..f55d3e45970 100644 --- a/api/keyring.go +++ b/api/keyring.go @@ -6,6 +6,7 @@ package api import ( "fmt" "net/url" + "strconv" ) // Keyring is used to access the Variables keyring. @@ -60,14 +61,15 @@ func (k *Keyring) List(q *QueryOptions) ([]*RootKeyMeta, *QueryMeta, error) { // Delete deletes a specific inactive key from the keyring func (k *Keyring) Delete(opts *KeyringDeleteOptions, w *WriteOptions) (*WriteMeta, error) { - wm, err := k.client.delete(fmt.Sprintf("/v1/operator/keyring/key/%v", - url.PathEscape(opts.KeyID)), nil, nil, w) + wm, err := k.client.delete(fmt.Sprintf("/v1/operator/keyring/key/%v?force=%v", + url.PathEscape(opts.KeyID), strconv.FormatBool(opts.Force)), nil, nil, w) return wm, err } // KeyringDeleteOptions are parameters for the Delete API type KeyringDeleteOptions struct { KeyID string // UUID + Force bool } // Rotate requests a key rotation diff --git a/command/operator_root_keyring_remove.go b/command/operator_root_keyring_remove.go index 942c351b0d7..f550d683b67 100644 --- a/command/operator_root_keyring_remove.go +++ b/command/operator_root_keyring_remove.go @@ -51,11 +51,12 @@ func (c *OperatorRootKeyringRemoveCommand) Name() string { } func (c *OperatorRootKeyringRemoveCommand) Run(args []string) int { - var verbose bool + var force, verbose bool flags := c.Meta.FlagSet("root keyring remove", FlagSetClient) flags.Usage = func() { c.Ui.Output(c.Help()) } flags.BoolVar(&verbose, "verbose", false, "") + flags.BoolVar(&force, "force", false, "Forces deletion of the root keyring even if it's in use.") if err := flags.Parse(args); err != nil { return 1 @@ -76,6 +77,7 @@ func (c *OperatorRootKeyringRemoveCommand) Run(args []string) int { } _, err = client.Keyring().Delete(&api.KeyringDeleteOptions{ KeyID: removeKey, + Force: force, }, nil) if err != nil { c.Ui.Error(fmt.Sprintf("error: %s", err)) diff --git a/nomad/keyring_endpoint.go b/nomad/keyring_endpoint.go index 692cb84ed37..1c022395388 100644 --- a/nomad/keyring_endpoint.go +++ b/nomad/keyring_endpoint.go @@ -348,6 +348,15 @@ func (k *Keyring) Delete(args *structs.KeyringDeleteRootKeyRequest, reply *struc return fmt.Errorf("active root key cannot be deleted - call rotate first") } + // make sure the key was used to encrypt an existing variable + rootKeyInUse, err := snap.IsRootKeyInUse(args.KeyID) + if err != nil { + return err + } + if rootKeyInUse && !args.Force { + return fmt.Errorf("root key in use, cannot delete") + } + _, index, err = k.srv.raftApply(structs.WrappedRootKeysDeleteRequestType, args) if err != nil { return err diff --git a/nomad/structs/keyring.go b/nomad/structs/keyring.go index a858a771ab5..5dc85a05a80 100644 --- a/nomad/structs/keyring.go +++ b/nomad/structs/keyring.go @@ -500,6 +500,7 @@ type KeyringUpdateRootKeyMetaResponse struct { type KeyringDeleteRootKeyRequest struct { KeyID string + Force bool WriteRequest } From aefca1b6ada3d16fad3a66037bcd7840260650fe Mon Sep 17 00:00:00 2001 From: Piotr Kazmierczak <470696+pkazmierczak@users.noreply.github.com> Date: Tue, 31 Dec 2024 17:21:42 +0100 Subject: [PATCH 2/9] test --- nomad/keyring_endpoint_test.go | 65 ++++++++++++++++++++-------------- 1 file changed, 38 insertions(+), 27 deletions(-) diff --git a/nomad/keyring_endpoint_test.go b/nomad/keyring_endpoint_test.go index 071d17686b0..5b28d65eec3 100644 --- a/nomad/keyring_endpoint_test.go +++ b/nomad/keyring_endpoint_test.go @@ -8,12 +8,11 @@ import ( "time" msgpackrpc "github.com/hashicorp/net-rpc-msgpackrpc/v2" - "github.com/shoenig/test/must" - "github.com/stretchr/testify/require" - "github.com/hashicorp/nomad/ci" + "github.com/hashicorp/nomad/nomad/mock" "github.com/hashicorp/nomad/nomad/structs" "github.com/hashicorp/nomad/testutil" + "github.com/shoenig/test/must" ) // TestKeyringEndpoint_CRUD exercises the basic keyring operations @@ -26,11 +25,11 @@ func TestKeyringEndpoint_CRUD(t *testing.T) { defer shutdown() testutil.WaitForKeyring(t, srv.RPC, "global") codec := rpcClient(t, srv) + state := srv.fsm.State() // Upsert a new key - key, err := structs.NewUnwrappedRootKey(structs.EncryptionAlgorithmAES256GCM) - require.NoError(t, err) + must.NoError(t, err) id := key.Meta.KeyID key = key.MakeActive() @@ -41,12 +40,19 @@ func TestKeyringEndpoint_CRUD(t *testing.T) { var updateResp structs.KeyringUpdateRootKeyResponse err = msgpackrpc.CallWithCodec(codec, "Keyring.Update", updateReq, &updateResp) - require.EqualError(t, err, structs.ErrPermissionDenied.Error()) + must.EqError(t, err, structs.ErrPermissionDenied.Error()) updateReq.AuthToken = rootToken.SecretID err = msgpackrpc.CallWithCodec(codec, "Keyring.Update", updateReq, &updateResp) - require.NoError(t, err) - require.NotEqual(t, uint64(0), updateResp.Index) + must.NoError(t, err) + must.NotEq(t, uint64(0), updateResp.Index) + + // Upsert a variable and encrypt it with that key (used for key deletion test + // below) + encryptedVar := mock.VariableEncrypted() + encryptedVar.KeyID = key.Meta.KeyID + varSetResp := state.VarSet(0, &structs.VarApplyStateRequest{Var: encryptedVar}) + must.NoError(t, varSetResp.Error) // Get doesn't need a token here because it uses mTLS role verification getReq := &structs.KeyringGetRootKeyRequest{ @@ -56,9 +62,9 @@ func TestKeyringEndpoint_CRUD(t *testing.T) { var getResp structs.KeyringGetRootKeyResponse err = msgpackrpc.CallWithCodec(codec, "Keyring.Get", getReq, &getResp) - require.NoError(t, err) - require.Equal(t, updateResp.Index, getResp.Index) - require.Equal(t, structs.EncryptionAlgorithmAES256GCM, getResp.Key.Meta.Algorithm) + must.NoError(t, err) + must.Eq(t, updateResp.Index, getResp.Index) + must.Eq(t, structs.EncryptionAlgorithmAES256GCM, getResp.Key.Meta.Algorithm) // Make a blocking query for List and wait for an Update. Note // that Get queries don't need ACL tokens in the test server @@ -82,13 +88,13 @@ func TestKeyringEndpoint_CRUD(t *testing.T) { updateReq.RootKey.Meta.CreateTime = time.Now().UTC().UnixNano() err = msgpackrpc.CallWithCodec(codec, "Keyring.Update", updateReq, &updateResp) - require.NoError(t, err) - require.NotEqual(t, uint64(0), updateResp.Index) + must.NoError(t, err) + must.NotEq(t, uint64(0), updateResp.Index) // wait for the blocking query to complete and check the response - require.NoError(t, <-errCh) - require.Equal(t, listResp.Index, updateResp.Index) - require.Len(t, listResp.Keys, 2) // bootstrap + new one + must.NoError(t, <-errCh) + must.Eq(t, listResp.Index, updateResp.Index) + must.SliceLen(t, 2, listResp.Keys) /// bootstrap + new one // Delete the key and verify that it's gone @@ -99,20 +105,25 @@ func TestKeyringEndpoint_CRUD(t *testing.T) { var delResp structs.KeyringDeleteRootKeyResponse err = msgpackrpc.CallWithCodec(codec, "Keyring.Delete", delReq, &delResp) - require.EqualError(t, err, structs.ErrPermissionDenied.Error()) + must.EqError(t, err, structs.ErrPermissionDenied.Error()) delReq.AuthToken = rootToken.SecretID err = msgpackrpc.CallWithCodec(codec, "Keyring.Delete", delReq, &delResp) - require.EqualError(t, err, "active root key cannot be deleted - call rotate first") + must.EqError(t, err, "active root key cannot be deleted - call rotate first") // set inactive updateReq.RootKey = updateReq.RootKey.MakeInactive() err = msgpackrpc.CallWithCodec(codec, "Keyring.Update", updateReq, &updateResp) - require.NoError(t, err) + must.NoError(t, err) err = msgpackrpc.CallWithCodec(codec, "Keyring.Delete", delReq, &delResp) - require.NoError(t, err) - require.Greater(t, delResp.Index, getResp.Index) + must.EqError(t, err, "root key in use, cannot delete") + + // delete with force + delReq.Force = true + err = msgpackrpc.CallWithCodec(codec, "Keyring.Delete", delReq, &delResp) + must.NoError(t, err) + must.Greater(t, getResp.Index, delResp.Index) listReq := &structs.KeyringListRootKeyMetaRequest{ QueryOptions: structs.QueryOptions{ @@ -121,9 +132,9 @@ func TestKeyringEndpoint_CRUD(t *testing.T) { }, } err = msgpackrpc.CallWithCodec(codec, "Keyring.List", listReq, &listResp) - require.NoError(t, err) - require.Greater(t, listResp.Index, getResp.Index) - require.Len(t, listResp.Keys, 1) // just the bootstrap key + must.NoError(t, err) + must.Greater(t, getResp.Index, listResp.Index) + must.SliceLen(t, 1, listResp.Keys) // just the bootstrap key } // TestKeyringEndpoint_validateUpdate exercises all the various @@ -140,7 +151,7 @@ func TestKeyringEndpoint_InvalidUpdates(t *testing.T) { // Setup an existing key key, err := structs.NewUnwrappedRootKey(structs.EncryptionAlgorithmAES256GCM) - require.NoError(t, err) + must.NoError(t, err) id := key.Meta.KeyID key = key.MakeActive() @@ -153,7 +164,7 @@ func TestKeyringEndpoint_InvalidUpdates(t *testing.T) { } var updateResp structs.KeyringUpdateRootKeyResponse err = msgpackrpc.CallWithCodec(codec, "Keyring.Update", updateReq, &updateResp) - require.NoError(t, err) + must.NoError(t, err) testCases := []struct { key *structs.UnwrappedRootKey @@ -211,7 +222,7 @@ func TestKeyringEndpoint_InvalidUpdates(t *testing.T) { } var updateResp structs.KeyringUpdateRootKeyResponse err := msgpackrpc.CallWithCodec(codec, "Keyring.Update", updateReq, &updateResp) - require.EqualError(t, err, tc.expectedErrMsg) + must.EqError(t, err, tc.expectedErrMsg) }) } From 57fd7b713857fd085b5e0dfc84632033864b9f55 Mon Sep 17 00:00:00 2001 From: Piotr Kazmierczak <470696+pkazmierczak@users.noreply.github.com> Date: Tue, 31 Dec 2024 17:22:40 +0100 Subject: [PATCH 3/9] cl --- .changelog/24766.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .changelog/24766.txt diff --git a/.changelog/24766.txt b/.changelog/24766.txt new file mode 100644 index 00000000000..a5693122e1e --- /dev/null +++ b/.changelog/24766.txt @@ -0,0 +1,3 @@ +```release-note:improvement +keyring: Warn if deleting a key previously used to encrypt a variable +``` From 8fc8e5e2e2568c9426f0098d6da5273884f5ecb6 Mon Sep 17 00:00:00 2001 From: Piotr Kazmierczak <470696+pkazmierczak@users.noreply.github.com> Date: Fri, 3 Jan 2025 14:39:11 +0100 Subject: [PATCH 4/9] pass the force param correctly to the RPC endpoint --- api/keyring_test.go | 4 ++-- command/agent/keyring_endpoint.go | 11 ++++++++--- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/api/keyring_test.go b/api/keyring_test.go index 9320aaf9d46..cabbf34823e 100644 --- a/api/keyring_test.go +++ b/api/keyring_test.go @@ -37,8 +37,8 @@ func TestKeyring_CRUD(t *testing.T) { assertQueryMeta(t, qm) must.Len(t, 2, keys) - // Delete the old key - wm, err = kr.Delete(&KeyringDeleteOptions{KeyID: oldKeyID}, nil) + // Delete the old key with force + wm, err = kr.Delete(&KeyringDeleteOptions{KeyID: oldKeyID, Force: true}, nil) must.NoError(t, err) assertWriteMeta(t, wm) diff --git a/command/agent/keyring_endpoint.go b/command/agent/keyring_endpoint.go index 870aae3f06c..657cb3c49d6 100644 --- a/command/agent/keyring_endpoint.go +++ b/command/agent/keyring_endpoint.go @@ -116,9 +116,14 @@ func (s *HTTPServer) KeyringRequest(resp http.ResponseWriter, req *http.Request) return s.keyringListRequest(resp, req) case strings.HasPrefix(path, "key"): keyID := strings.TrimPrefix(req.URL.Path, "/v1/operator/keyring/key/") + force := req.URL.Query()["force"][0] + forceBool, err := strconv.ParseBool(force) + if err != nil { + return nil, CodedError(422, "invalid force parameter") + } switch req.Method { case http.MethodDelete: - return s.keyringDeleteRequest(resp, req, keyID) + return s.keyringDeleteRequest(resp, req, keyID, forceBool) default: return nil, CodedError(405, ErrInvalidMethod) } @@ -185,9 +190,9 @@ func (s *HTTPServer) keyringRotateRequest(resp http.ResponseWriter, req *http.Re return out, nil } -func (s *HTTPServer) keyringDeleteRequest(resp http.ResponseWriter, req *http.Request, keyID string) (interface{}, error) { +func (s *HTTPServer) keyringDeleteRequest(resp http.ResponseWriter, req *http.Request, keyID string, force bool) (interface{}, error) { - args := structs.KeyringDeleteRootKeyRequest{KeyID: keyID} + args := structs.KeyringDeleteRootKeyRequest{KeyID: keyID, Force: force} s.parseWriteRequest(req, &args.WriteRequest) var out structs.KeyringDeleteRootKeyResponse From f16ff48d5baf1a0d40cd47b68a968b77e527bda1 Mon Sep 17 00:00:00 2001 From: Piotr Kazmierczak <470696+pkazmierczak@users.noreply.github.com> Date: Fri, 3 Jan 2025 14:39:18 +0100 Subject: [PATCH 5/9] James' suggestions --- .changelog/24766.txt | 2 +- api/keyring.go | 2 ++ nomad/keyring_endpoint.go | 3 ++- 3 files changed, 5 insertions(+), 2 deletions(-) diff --git a/.changelog/24766.txt b/.changelog/24766.txt index a5693122e1e..c173f439cac 100644 --- a/.changelog/24766.txt +++ b/.changelog/24766.txt @@ -1,3 +1,3 @@ ```release-note:improvement -keyring: Warn if deleting a key previously used to encrypt a variable +keyring: Warn if deleting a key previously used to encrypt an existing variable ``` diff --git a/api/keyring.go b/api/keyring.go index f55d3e45970..f528b326152 100644 --- a/api/keyring.go +++ b/api/keyring.go @@ -69,6 +69,8 @@ func (k *Keyring) Delete(opts *KeyringDeleteOptions, w *WriteOptions) (*WriteMet // KeyringDeleteOptions are parameters for the Delete API type KeyringDeleteOptions struct { KeyID string // UUID + // Force can be used to force deletion of a root keyring that was used to encrypt + // an existing variable or to sign a workload identity Force bool } diff --git a/nomad/keyring_endpoint.go b/nomad/keyring_endpoint.go index 1c022395388..6b65b898222 100644 --- a/nomad/keyring_endpoint.go +++ b/nomad/keyring_endpoint.go @@ -4,6 +4,7 @@ package nomad import ( + "errors" "fmt" "time" @@ -354,7 +355,7 @@ func (k *Keyring) Delete(args *structs.KeyringDeleteRootKeyRequest, reply *struc return err } if rootKeyInUse && !args.Force { - return fmt.Errorf("root key in use, cannot delete") + return errors.New("root key in use, cannot delete") } _, index, err = k.srv.raftApply(structs.WrappedRootKeysDeleteRequestType, args) From 3420f26ca0b5f5f2f30a1d341f48ff5c69effaf5 Mon Sep 17 00:00:00 2001 From: Piotr Kazmierczak <470696+pkazmierczak@users.noreply.github.com> Date: Fri, 3 Jan 2025 16:11:15 +0100 Subject: [PATCH 6/9] return error if trying to delete a non-existent key --- nomad/keyring_endpoint.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/nomad/keyring_endpoint.go b/nomad/keyring_endpoint.go index 6b65b898222..018f7f63816 100644 --- a/nomad/keyring_endpoint.go +++ b/nomad/keyring_endpoint.go @@ -345,6 +345,11 @@ func (k *Keyring) Delete(args *structs.KeyringDeleteRootKeyRequest, reply *struc if err != nil { return err } + + if rootKey == nil { + return errors.New("root key not found") + } + if rootKey != nil && rootKey.IsActive() { return fmt.Errorf("active root key cannot be deleted - call rotate first") } From 0b266c1eaba5bcf4c78fd0cea6f1ba0fd310f643 Mon Sep 17 00:00:00 2001 From: Piotr Kazmierczak <470696+pkazmierczak@users.noreply.github.com> Date: Fri, 3 Jan 2025 16:30:53 +0100 Subject: [PATCH 7/9] TestHTTP_Keyring_CRUD amended --- command/agent/keyring_endpoint.go | 10 ++++++++-- command/agent/keyring_endpoint_test.go | 14 ++++++++++++++ 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/command/agent/keyring_endpoint.go b/command/agent/keyring_endpoint.go index 657cb3c49d6..28170a1a153 100644 --- a/command/agent/keyring_endpoint.go +++ b/command/agent/keyring_endpoint.go @@ -116,8 +116,14 @@ func (s *HTTPServer) KeyringRequest(resp http.ResponseWriter, req *http.Request) return s.keyringListRequest(resp, req) case strings.HasPrefix(path, "key"): keyID := strings.TrimPrefix(req.URL.Path, "/v1/operator/keyring/key/") - force := req.URL.Query()["force"][0] - forceBool, err := strconv.ParseBool(force) + + var forceBool bool + var err error + forceQuery, ok := req.URL.Query()["force"] + if ok { + forceBool, err = strconv.ParseBool(forceQuery[0]) + } + if err != nil { return nil, CodedError(422, "invalid force parameter") } diff --git a/command/agent/keyring_endpoint_test.go b/command/agent/keyring_endpoint_test.go index 428da9d5078..2c99be9cf6e 100644 --- a/command/agent/keyring_endpoint_test.go +++ b/command/agent/keyring_endpoint_test.go @@ -13,6 +13,7 @@ import ( "time" "github.com/go-jose/go-jose/v3" + "github.com/hashicorp/nomad/nomad/mock" "github.com/shoenig/test/must" "github.com/hashicorp/nomad/ci" @@ -36,6 +37,13 @@ func TestHTTP_Keyring_CRUD(t *testing.T) { must.Len(t, 1, listResp) key0 := listResp[0].KeyID + // Create a variable to test force key deletion + state := s.server.State() + encryptedVar := mock.VariableEncrypted() + encryptedVar.KeyID = key0 + varSetResp := state.VarSet(0, &structs.VarApplyStateRequest{Var: encryptedVar}) + must.NoError(t, varSetResp.Error) + // Rotate req, err = http.NewRequest(http.MethodPut, "/v1/operator/keyring/rotate", nil) @@ -87,6 +95,12 @@ func TestHTTP_Keyring_CRUD(t *testing.T) { req, err = http.NewRequest(http.MethodDelete, "/v1/operator/keyring/key/"+key0, nil) must.NoError(t, err) obj, err = s.Server.KeyringRequest(respW, req) + must.Error(t, err) + must.EqError(t, err, "root key in use, cannot delete") + + req, err = http.NewRequest(http.MethodDelete, "/v1/operator/keyring/key/"+key0+"?force=true", nil) + must.NoError(t, err) + obj, err = s.Server.KeyringRequest(respW, req) must.NoError(t, err) req, err = http.NewRequest(http.MethodGet, "/v1/operator/keyring/keys", nil) From 19e7335181d6f346dd741318c2fcba2b7627e729 Mon Sep 17 00:00:00 2001 From: Piotr Kazmierczak <470696+pkazmierczak@users.noreply.github.com> Date: Fri, 3 Jan 2025 16:33:57 +0100 Subject: [PATCH 8/9] CLI help message adjusted --- command/operator_root_keyring_remove.go | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/command/operator_root_keyring_remove.go b/command/operator_root_keyring_remove.go index f550d683b67..8b29075403c 100644 --- a/command/operator_root_keyring_remove.go +++ b/command/operator_root_keyring_remove.go @@ -29,7 +29,14 @@ Usage: nomad operator root keyring remove [options] General Options: - ` + generalOptionsUsage(usageOptsDefault|usageOptsNoNamespace) + ` + generalOptionsUsage(usageOptsDefault|usageOptsNoNamespace) + ` + +Keyring Options: + + -force + Remove the key even if it was used to sign an existing variable + or workload identity. +` return strings.TrimSpace(helpText) } From 3f8db12edc8f12a906614cd8fe8215cdd8e69827 Mon Sep 17 00:00:00 2001 From: Piotr Kazmierczak <470696+pkazmierczak@users.noreply.github.com> Date: Fri, 3 Jan 2025 16:36:49 +0100 Subject: [PATCH 9/9] documentation --- command/operator_root_keyring_remove.go | 2 +- website/content/api-docs/operator/keyring.mdx | 5 +++++ .../content/docs/commands/operator/root/keyring-remove.mdx | 6 ++++++ 3 files changed, 12 insertions(+), 1 deletion(-) diff --git a/command/operator_root_keyring_remove.go b/command/operator_root_keyring_remove.go index 8b29075403c..b3afc92c127 100644 --- a/command/operator_root_keyring_remove.go +++ b/command/operator_root_keyring_remove.go @@ -31,7 +31,7 @@ General Options: ` + generalOptionsUsage(usageOptsDefault|usageOptsNoNamespace) + ` -Keyring Options: +Remove Options: -force Remove the key even if it was used to sign an existing variable diff --git a/website/content/api-docs/operator/keyring.mdx b/website/content/api-docs/operator/keyring.mdx index 7080d4828e4..feeb0b237af 100644 --- a/website/content/api-docs/operator/keyring.mdx +++ b/website/content/api-docs/operator/keyring.mdx @@ -233,6 +233,11 @@ The table below shows this endpoint's support for [blocking queries] and |------------------|--------------| | `NO` | `management` | +### Parameters + +- `force` `(bool: false)` - Remove the key even if it was used to sign an existing variable +or workload identity. + ### Sample Request ```shell-session diff --git a/website/content/docs/commands/operator/root/keyring-remove.mdx b/website/content/docs/commands/operator/root/keyring-remove.mdx index bcf701378e2..252e6d741a2 100644 --- a/website/content/docs/commands/operator/root/keyring-remove.mdx +++ b/website/content/docs/commands/operator/root/keyring-remove.mdx @@ -23,6 +23,12 @@ nomad operator root keyring remove [options] @include 'general_options.mdx' +## Remove Options + +- `-force`: Remove the key even if it was used to sign an existing variable +or workload identity. + + ## Examples ```shell-session