From a7345f0621acbf141446eefeacde022cbf922dc9 Mon Sep 17 00:00:00 2001 From: Tim Gross Date: Thu, 7 Dec 2023 11:35:11 -0500 Subject: [PATCH] keyring: replicate RSA private key via `GetKey` RPC When we added a RSA key for signing Workload Identities, we added it to the keystore serialization but did not also add it to the `GetKey` RPC. This means that when a key is rotated, the RSA key will not come along. The Nomad leader signs all Workload Identities, but external consumers of WI (like Consul or Vault) will verify the WI against any of the servers. If the request to verify hits a follower, the follower will not have the RSA private key and cannot use the existing ed25519 key to verify WIs with the `RS256` algorithm. Add the RSA key material to the `GetKey` RPC. Also remove an extraneous write to disk that happens for each key each time we restart the Nomad server. Fixes: #19340 --- .changelog/19350.txt | 3 +++ nomad/encrypter.go | 10 ++++----- nomad/encrypter_test.go | 43 +++++++++++++++++++++++++++++++-------- nomad/keyring_endpoint.go | 7 ++++--- 4 files changed, 47 insertions(+), 16 deletions(-) create mode 100644 .changelog/19350.txt diff --git a/.changelog/19350.txt b/.changelog/19350.txt new file mode 100644 index 00000000000..34a53e82a81 --- /dev/null +++ b/.changelog/19350.txt @@ -0,0 +1,3 @@ +```release-note:bug +keyring: Fixed a bug where RSA keys were not replicated to followers +``` diff --git a/nomad/encrypter.go b/nomad/encrypter.go index 6d397cd3619..9264612f74d 100644 --- a/nomad/encrypter.go +++ b/nomad/encrypter.go @@ -116,7 +116,7 @@ func (e *Encrypter) loadKeystore() error { return fmt.Errorf("root key ID %s must match key file %s", key.Meta.KeyID, path) } - err = e.AddKey(key) + err = e.addCipher(key) if err != nil { return fmt.Errorf("could not add key file %s to keystore: %w", path, err) } @@ -338,15 +338,15 @@ func (e *Encrypter) addCipher(rootKey *structs.RootKey) error { } // GetKey retrieves the key material by ID from the keyring -func (e *Encrypter) GetKey(keyID string) ([]byte, error) { +func (e *Encrypter) GetKey(keyID string) ([]byte, []byte, error) { e.lock.RLock() defer e.lock.RUnlock() keyset, err := e.keysetByIDLocked(keyID) if err != nil { - return nil, err + return nil, nil, err } - return keyset.rootKey.Key, nil + return keyset.rootKey.Key, keyset.rootKey.RSAKey, nil } // activeKeySetLocked returns the keyset that belongs to the key marked as @@ -582,7 +582,7 @@ func (krr *KeyringReplicator) run(ctx context.Context) { } keyMeta := raw.(*structs.RootKeyMeta) - if key, err := krr.encrypter.GetKey(keyMeta.KeyID); err == nil && len(key) > 0 { + if key, _, err := krr.encrypter.GetKey(keyMeta.KeyID); err == nil && len(key) > 0 { // the key material is immutable so if we've already got it // we can move on to the next key continue diff --git a/nomad/encrypter_test.go b/nomad/encrypter_test.go index 1f18509bb08..3992f8bb681 100644 --- a/nomad/encrypter_test.go +++ b/nomad/encrypter_test.go @@ -16,6 +16,7 @@ import ( "github.com/go-jose/go-jose/v3/jwt" msgpackrpc "github.com/hashicorp/net-rpc-msgpackrpc" + "github.com/shoenig/test" "github.com/shoenig/test/must" "github.com/stretchr/testify/require" @@ -56,7 +57,7 @@ func TestEncrypter_LoadSave(t *testing.T) { tmpDir := t.TempDir() encrypter, err := NewEncrypter(srv, tmpDir) - require.NoError(t, err) + must.NoError(t, err) algos := []structs.EncryptionAlgorithm{ structs.EncryptionAlgorithmAES256GCM, @@ -65,13 +66,21 @@ func TestEncrypter_LoadSave(t *testing.T) { for _, algo := range algos { t.Run(string(algo), func(t *testing.T) { key, err := structs.NewRootKey(algo) - require.NoError(t, err) - require.NoError(t, encrypter.saveKeyToStore(key)) + must.Greater(t, 0, len(key.RSAKey)) + must.NoError(t, err) + must.NoError(t, encrypter.saveKeyToStore(key)) + // startup code path gotKey, err := encrypter.loadKeyFromStore( filepath.Join(tmpDir, key.Meta.KeyID+".nks.json")) - require.NoError(t, err) - require.NoError(t, encrypter.addCipher(gotKey)) + must.NoError(t, err) + must.NoError(t, encrypter.addCipher(gotKey)) + must.Greater(t, 0, len(gotKey.RSAKey)) + must.NoError(t, encrypter.saveKeyToStore(key)) + + active, err := encrypter.keysetByIDLocked(key.Meta.KeyID) + must.NoError(t, err) + must.Greater(t, 0, len(active.rootKey.RSAKey)) }) } } @@ -122,8 +131,17 @@ func TestEncrypter_Restore(t *testing.T) { var rotateResp structs.KeyringRotateRootKeyResponse for i := 0; i < 4; i++ { err := msgpackrpc.CallWithCodec(codec, "Keyring.Rotate", rotateReq, &rotateResp) - require.NoError(t, err) + must.NoError(t, err) + } + + // Ensure all rotated keys are correct + srv.encrypter.lock.Lock() + test.MapLen(t, 5, srv.encrypter.keyring) + for _, keyset := range srv.encrypter.keyring { + test.Len(t, 32, keyset.rootKey.Key) + test.Greater(t, 0, len(keyset.rootKey.RSAKey)) } + srv.encrypter.lock.Unlock() shutdown() @@ -146,6 +164,14 @@ func TestEncrypter_Restore(t *testing.T) { return len(listResp.Keys) == 5 // 4 new + the bootstrap key }, time.Second*5, time.Second, "expected keyring to be restored") + srv.encrypter.lock.Lock() + test.MapLen(t, 5, srv.encrypter.keyring) + for _, keyset := range srv.encrypter.keyring { + test.Len(t, 32, keyset.rootKey.Key) + test.Greater(t, 0, len(keyset.rootKey.RSAKey)) + } + srv.encrypter.lock.Unlock() + for _, keyMeta := range listResp.Keys { getReq := &structs.KeyringGetRootKeyRequest{ @@ -157,10 +183,11 @@ func TestEncrypter_Restore(t *testing.T) { } var getResp structs.KeyringGetRootKeyResponse err := msgpackrpc.CallWithCodec(codec, "Keyring.Get", getReq, &getResp) - require.NoError(t, err) + must.NoError(t, err) gotKey := getResp.Key - require.Len(t, gotKey.Key, 32) + must.Len(t, 32, gotKey.Key) + test.Greater(t, 0, len(gotKey.RSAKey)) } } diff --git a/nomad/keyring_endpoint.go b/nomad/keyring_endpoint.go index b3869799ec5..34aaeb1e2d4 100644 --- a/nomad/keyring_endpoint.go +++ b/nomad/keyring_endpoint.go @@ -268,13 +268,14 @@ func (k *Keyring) Get(args *structs.KeyringGetRootKeyRequest, reply *structs.Key } // retrieve the key material from the keyring - key, err := k.encrypter.GetKey(keyMeta.KeyID) + key, rsaKey, err := k.encrypter.GetKey(keyMeta.KeyID) if err != nil { return err } rootKey := &structs.RootKey{ - Meta: keyMeta, - Key: key, + Meta: keyMeta, + Key: key, + RSAKey: rsaKey, } reply.Key = rootKey