VAULT-37632 allow restoring SSH CA from loaded snapshot (hashicorp#8581) (hashicorp#9034)

* allow restoring ssh config/ca

* add some unit tests

* address PR review

* imports and test upgrades

* linter complaints

* add PR comment and linter fixes

* address review

Co-authored-by: Bruno Oliveira de Souza <[email protected]>

Author: hc-github-team-secure-vault-core

builtin/logical/ssh/backend.go

@@ -56,6 +56,10 @@ func Backend(conf *logical.BackendConfig) (*backend, error) {
 				caPrivateKeyStoragePath,
 				keysStoragePrefix,
 			},
+
+			AllowSnapshotRead: []string{
+				"config/ca",
+			},
 		},
 
 		Paths: []*framework.Path{

builtin/logical/ssh/path_config_ca.go

@@ -107,6 +107,9 @@ func pathConfigCA(b *backend) *framework.Path {
 					OperationSuffix: "ca-configuration",
 				},
 			},
+			logical.RecoverOperation: &framework.PathOperation{
+				Callback: b.pathConfigCARecover,
+			},
 		},
 
 		HelpSynopsis: `Set the SSH private key used for signing certificates.`,
@@ -120,7 +123,9 @@ Read operations will return the public key, if already stored/generated.`,
 }
 
 func (b *backend) pathConfigCARead(ctx context.Context, req *logical.Request, data *framework.FieldData) (*logical.Response, error) {
-	publicKey, err := getCAPublicKey(ctx, req.Storage)
+	// prevent migration from deprecated paths on snapshot read as writes to a loaded snapshot storage are forbidden
+	allowMigration := !req.IsSnapshotReadOrList()
+	publicKey, err := getCAPublicKey(ctx, req.Storage, allowMigration)
 	if err != nil {
 		return nil, fmt.Errorf("failed to read CA public key: %w", err)
 	}
@@ -142,16 +147,22 @@ func (b *backend) pathConfigCADelete(ctx context.Context, req *logical.Request,
 	if err := req.Storage.Delete(ctx, caPrivateKeyStoragePath); err != nil {
 		return nil, err
 	}
+	if err := req.Storage.Delete(ctx, caPrivateKeyStoragePathDeprecated); err != nil {
+		return nil, err
+	}
 	if err := req.Storage.Delete(ctx, caPublicKeyStoragePath); err != nil {
 		return nil, err
 	}
+	if err := req.Storage.Delete(ctx, caPublicKeyStoragePathDeprecated); err != nil {
+		return nil, err
+	}
 	if err := req.Storage.Delete(ctx, caManagedKeyStoragePath); err != nil {
 		return nil, err
 	}
 	return nil, nil
 }
 
-func readStoredKey(ctx context.Context, storage logical.Storage, keyType string) (*keyStorageEntry, error) {
+func readStoredKeyEntry(ctx context.Context, storage logical.Storage, keyType string, allowMigration bool) (*logical.StorageEntry, error) {
 	var path, deprecatedPath string
 	switch keyType {
 	case caPrivateKey:
@@ -176,25 +187,39 @@ func readStoredKey(ctx context.Context, storage logical.Storage, keyType string)
 		if err != nil {
 			return nil, err
 		}
+
 		if entry != nil {
+			// modify entry variable, both for possible migration and also to comply with the expected JSON entry for the caller
 			entry, err = logical.StorageEntryJSON(path, keyStorageEntry{
 				Key: string(entry.Value),
 			})
 			if err != nil {
 				return nil, err
 			}
-			if err := storage.Put(ctx, entry); err != nil {
-				return nil, err
-			}
-			if err = storage.Delete(ctx, deprecatedPath); err != nil {
-				return nil, err
+			// migrations are disable on recover, as we can't write to the loaded snapshot storage
+			if allowMigration {
+				if err := storage.Put(ctx, entry); err != nil {
+					return nil, err
+				}
+				if err = storage.Delete(ctx, deprecatedPath); err != nil {
+					return nil, err
+				}
 			}
 		}
 	}
+	return entry, nil
+}
+
+// readStoredKey reads a key from storage, returning nil if not found.
+// ignore-nil-nil-function-check
+func readStoredKey(ctx context.Context, storage logical.Storage, keyType string, allowMigration bool) (*keyStorageEntry, error) {
+	entry, err := readStoredKeyEntry(ctx, storage, keyType, allowMigration)
+	if err != nil {
+		return nil, err
+	}
 	if entry == nil {
 		return nil, nil
 	}
-
 	var keyEntry keyStorageEntry
 	if err := entry.DecodeJSON(&keyEntry); err != nil {
 		return nil, err
@@ -450,10 +475,10 @@ func (b *backend) createManagedKey(ctx context.Context, s logical.Storage, manag
 	return nil
 }
 
-func getCAPublicKey(ctx context.Context, storage logical.Storage) (string, error) {
+func getCAPublicKey(ctx context.Context, storage logical.Storage, allowMigration bool) (string, error) {
 	var publicKey string
 
-	storedKeyEntry, err := readStoredKey(ctx, storage, caPublicKey)
+	storedKeyEntry, err := readStoredKey(ctx, storage, caPublicKey, allowMigration)
 	if err != nil {
 		return "", err
 	}
@@ -495,12 +520,13 @@ func readManagedKey(ctx context.Context, storage logical.Storage) (*managedKeySt
 }
 
 func caKeysConfigured(ctx context.Context, s logical.Storage) (bool, error) {
-	publicKeyEntry, err := readStoredKey(ctx, s, caPublicKey)
+	const allowMigration = false // no need to allow migration when just checking for existence, we can do that later
+	publicKeyEntry, err := readStoredKey(ctx, s, caPublicKey, allowMigration)
 	if err != nil {
 		return false, fmt.Errorf("failed to read CA public key: %w", err)
 	}
 
-	privateKeyEntry, err := readStoredKey(ctx, s, caPrivateKey)
+	privateKeyEntry, err := readStoredKey(ctx, s, caPrivateKey, allowMigration)
 	if err != nil {
 		return false, fmt.Errorf("failed to read CA private key: %w", err)
 	}
@@ -520,3 +546,64 @@ func caKeysConfigured(ctx context.Context, s logical.Storage) (bool, error) {
 
 	return false, nil
 }
+
+// pathConfigCARecover recovers the CA from the target snapshot back to the live storage.
+// ignore-nil-nil-function-check
+func (b *backend) pathConfigCARecover(ctx context.Context, req *logical.Request, data *framework.FieldData) (*logical.Response, error) {
+	// check live storage for existing keys. Disallow recovery if CA is already configured for consistency with create operation
+	found, err := caKeysConfigured(ctx, req.Storage)
+	if err != nil {
+		return nil, err
+	}
+	if found {
+		return logical.ErrorResponse("keys are already configured; delete them before recovering the CA"), nil
+	}
+
+	// fetch directly from the snapshot storage instead of following the usual restore procedure of getting the values
+	// from the req.Data, since those came from a previous CARead operation on the loaded snapshot, which only contains
+	// the public key.
+	snapshotStorage, err := logical.NewSnapshotStorageView(req)
+	if err != nil {
+		return nil, err
+	}
+	const allowMigration = false // prevent migration from deprecated paths as we can't allow writes on the snapshot storage
+	publicKeyEntry, err := readStoredKeyEntry(ctx, snapshotStorage, caPublicKey, allowMigration)
+	if err != nil {
+		return nil, fmt.Errorf("failed to read CA public key for restore: %w", err)
+	}
+	privateKeyEntry, err := readStoredKeyEntry(ctx, snapshotStorage, caPrivateKey, allowMigration)
+	if err != nil {
+		return nil, fmt.Errorf("failed to read CA private key for restore: %w", err)
+	}
+	managedKey, err := readManagedKey(ctx, snapshotStorage)
+	if err != nil {
+		return nil, fmt.Errorf("failed to read CA managed key for restore: %w", err)
+	}
+
+	if publicKeyEntry == nil && privateKeyEntry == nil && managedKey == nil {
+		return logical.ErrorResponse("no CA keys found in snapshot storage to restore"), nil
+	}
+
+	// it's possible that we've read the keys from a deprecated path in the snapshot, but it should be automatically
+	// upgraded to the new path anyway, so we don't care about restoring it back to the deprecated path
+	if publicKeyEntry != nil {
+		err = req.Storage.Put(ctx, publicKeyEntry)
+		if err != nil {
+			return nil, fmt.Errorf("failed to restore public key entry in storage: %w", err)
+		}
+	}
+	if privateKeyEntry != nil {
+		err = req.Storage.Put(ctx, privateKeyEntry)
+		if err != nil {
+			return nil, fmt.Errorf("failed to restore private key entry in storage: %w", err)
+		}
+	}
+	if managedKey != nil {
+		err = b.createManagedKey(ctx, req.Storage, managedKey.KeyName.String(), managedKey.KeyId.String())
+		if err != nil {
+			return nil, fmt.Errorf("failed to restore managed key entry in storage: %w", err)
+		}
+	}
+
+	return nil, nil
+}

builtin/logical/ssh/path_config_ca_test.go

@@ -10,7 +10,9 @@ import (
 	"strings"
 	"testing"
 
+	"github.com/hashicorp/vault/sdk/helper/testhelpers/snapshots"
 	"github.com/hashicorp/vault/sdk/logical"
+	"github.com/stretchr/testify/require"
 )
 
 func TestSSH_ConfigCAStorageUpgrade(t *testing.T) {
@@ -39,7 +41,7 @@ func TestSSH_ConfigCAStorageUpgrade(t *testing.T) {
 	}
 
 	// Reading it should return the key as well as upgrade the storage path
-	privateKeyEntry, err := readStoredKey(context.Background(), config.StorageView, caPrivateKey)
+	privateKeyEntry, err := readStoredKey(context.Background(), config.StorageView, caPrivateKey, true)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -73,7 +75,7 @@ func TestSSH_ConfigCAStorageUpgrade(t *testing.T) {
 	}
 
 	// Reading it should return the key as well as upgrade the storage path
-	publicKeyEntry, err := readStoredKey(context.Background(), config.StorageView, caPublicKey)
+	publicKeyEntry, err := readStoredKey(context.Background(), config.StorageView, caPublicKey, true)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -180,6 +182,28 @@ func TestSSH_ConfigCAUpdateDelete(t *testing.T) {
 	if err != nil || (resp != nil && resp.IsError()) {
 		t.Fatalf("bad: err: %v, resp:%v", err, resp)
 	}
+
+	// verify deletion of keys on deprecated path
+	err = config.StorageView.Put(context.Background(), &logical.StorageEntry{
+		Key:   caPublicKeyStoragePathDeprecated,
+		Value: []byte(testCAPublicKey),
+	})
+	require.NoError(t, err)
+	err = config.StorageView.Put(context.Background(), &logical.StorageEntry{
+		Key:   caPrivateKeyStoragePathDeprecated,
+		Value: []byte(testCAPrivateKey),
+	})
+	require.NoError(t, err)
+	caReq.Operation = logical.DeleteOperation
+	resp, err = b.HandleRequest(context.Background(), caReq)
+	if err != nil || (resp != nil && resp.IsError()) {
+		t.Fatalf("bad: err: %v, resp:%v", err, resp)
+	}
+	// ensure it was deleted
+	caReq.Operation = logical.ReadOperation
+	resp, err = b.HandleRequest(context.Background(), caReq)
+	require.NoError(t, err)
+	require.Error(t, resp.Error())
 }
 
 func createDeleteHelper(t *testing.T, b logical.Backend, config *logical.BackendConfig, index int, keyType string, keyBits int) {
@@ -340,7 +364,7 @@ func TestReadStoredKey(t *testing.T) {
 				t.Fatalf("error writing public key: %s", err)
 			}
 
-			publicKeyEntry, err := readStoredKey(context.Background(), storage, caPublicKey)
+			publicKeyEntry, err := readStoredKey(context.Background(), storage, caPublicKey, true)
 			if err != nil {
 				t.Fatalf("error reading public key: %s", err)
 			}
@@ -349,7 +373,7 @@ func TestReadStoredKey(t *testing.T) {
 				t.Fatalf("returned key does not match: expected %s, got %s", tt.publicKey, publicKeyEntry.Key)
 			}
 
-			privateKeyEntry, err := readStoredKey(context.Background(), storage, caPrivateKey)
+			privateKeyEntry, err := readStoredKey(context.Background(), storage, caPrivateKey, true)
 			if err != nil {
 				t.Fatalf("error reading private key: %s", err)
 			}
@@ -387,7 +411,7 @@ func TestGetCAPublicKey(t *testing.T) {
 				t.Fatalf("error writing key: %s", err)
 			}
 
-			key, err := getCAPublicKey(ctx, storage)
+			key, err := getCAPublicKey(ctx, storage, true)
 			if err != nil {
 				t.Fatalf("error retrieving public key: %s", err)
 			}
@@ -586,3 +610,106 @@ func readKey(ctx context.Context, s logical.Storage, path string) error {
 
 	return nil
 }
+
+// TestCARecover verifies secret recovery of the SSH CA
+func TestCARecover(t *testing.T) {
+	var err error
+	config := logical.TestBackendConfig()
+	config.StorageView = &logical.InmemStorage{}
+
+	b, err := Factory(context.Background(), config)
+	if err != nil {
+		t.Fatalf("Cannot create backend: %s", err)
+	}
+	tc := snapshots.NewSnapshotTestCase(t, b)
+
+	// generate CA keys on the snapshot storage
+	_, err = b.HandleRequest(context.Background(), &logical.Request{
+		Operation: logical.UpdateOperation,
+		Path:      "config/ca",
+		Storage:   tc.SnapshotStorage(),
+		Data: map[string]interface{}{
+			"public_key":  testCAPublicKey,
+			"private_key": testCAPrivateKey,
+		},
+	})
+	require.NoError(t, err)
+
+	// write different CA to the regular storage
+	_, err = b.HandleRequest(context.Background(), &logical.Request{
+		Operation: logical.UpdateOperation,
+		Path:      "config/ca",
+		Storage:   tc.RegularStorage(),
+		Data: map[string]interface{}{
+			"generate_signing_key": true,
+		},
+	})
+	require.NoError(t, err)
+	t.Run("read no side effects", func(t *testing.T) {
+		tc.RunRead(t, "config/ca")
+	})
+
+	t.Run("recover succeeds", func(t *testing.T) {
+		tc.DoRecover(t, "config/ca")
+		data, err := b.HandleRequest(context.Background(), &logical.Request{
+			Operation: logical.ReadOperation,
+			Path:      "config/ca",
+			Storage:   tc.SnapshotStorage(),
+			Data: map[string]interface{}{
+				"public_key": "should be the actual public key but the SSH CA recovery doesn't really care as it reads directly from snapshot storage",
+			},
+		})
+		require.NoError(t, err)
+		require.NotNil(t, data)
+		require.Equal(t, testCAPublicKey, data.Data["public_key"])
+	})
+}
+
+// TestCARecoverMigration is the same as TestCARecover, but recovering from a snapshot with the data in the deprecated
+// storage paths, which ensures that the migration logic is skipped during recovery.
+func TestCARecoverMigration(t *testing.T) {
+	var err error
+	config := logical.TestBackendConfig()
+	config.StorageView = &logical.InmemStorage{}
+
+	b, err := Factory(context.Background(), config)
+	if err != nil {
+		t.Fatalf("Cannot create backend: %s", err)
+	}
+	tc := snapshots.NewSnapshotTestCase(t, b)
+	err = tc.SnapshotStorage().Put(context.Background(), &logical.StorageEntry{
+		Key:   caPublicKeyStoragePathDeprecated,
+		Value: []byte(testCAPublicKey),
+	})
+	require.NoError(t, err)
+	err = tc.SnapshotStorage().Put(context.Background(), &logical.StorageEntry{
+		Key:   caPrivateKeyStoragePathDeprecated,
+		Value: []byte(testCAPrivateKey),
+	})
+	require.NoError(t, err)
+
+	require.NoError(t, err)
+	t.Run("read no side effects", func(t *testing.T) {
+		tc.RunRead(t, "config/ca")
+	})
+
+	t.Run("recover succeeds", func(t *testing.T) {
+		tc.DoRecover(t, "config/ca")
+		// ensure that even though the migration on read is disabled, by reading from the old path in the snapshot and
+		// creating a entry in the regular storage, that the entry ends up in the new path
+		entry, err := tc.RegularStorage().Get(context.Background(), caPublicKeyStoragePathDeprecated)
+		require.NoError(t, err)
+		require.Nil(t, entry)
+		data, err := b.HandleRequest(context.Background(), &logical.Request{
+			Operation: logical.ReadOperation,
+			Path:      "config/ca",
+			Storage:   tc.SnapshotStorage(),
+			Data: map[string]interface{}{
+				"public_key": "should be the actual public key but the SSH CA recovery doesn't really care as it reads directly from snapshot storage",
+			},
+		})
+		require.NoError(t, err)
+		require.NotNil(t, data)
+		require.Equal(t, testCAPublicKey, data.Data["public_key"])
+	})
+}

builtin/logical/ssh/path_fetch.go

@@ -29,7 +29,8 @@ func pathFetchPublicKey(b *backend) *framework.Path {
 }
 
 func (b *backend) pathFetchPublicKey(ctx context.Context, req *logical.Request, data *framework.FieldData) (*logical.Response, error) {
-	publicKey, err := getCAPublicKey(ctx, req.Storage)
+	const allowMigration = true // only paths that support snapshot reads are
+	publicKey, err := getCAPublicKey(ctx, req.Storage, allowMigration)
 	if err != nil {
 		return nil, err
 	}