Fix re-migration of existing CA bundles (hashicorp#21316)

* Fix re-migration of existing version 1 storage bundles

Related: VAULT-17307

Signed-off-by: Alexander Scheel <[email protected]>

* Add test for v1->v2 migration post-issuer deletion

Signed-off-by: Alexander Scheel <[email protected]>

* Add changelog entry

Signed-off-by: Alexander Scheel <[email protected]>

* Add known-issue about PKI double migration

Signed-off-by: Alexander Scheel <[email protected]>

* Update website/content/partials/pki-double-migration-bug.mdx

Co-authored-by: Sarah Chavis <[email protected]>

* Update website/content/partials/pki-double-migration-bug.mdx

Co-authored-by: Sarah Chavis <[email protected]>

* Update website/content/partials/pki-double-migration-bug.mdx

* Additional clarity around known issue

Signed-off-by: Alexander Scheel <[email protected]>

---------

Signed-off-by: Alexander Scheel <[email protected]>
Co-authored-by: Sarah Chavis <[email protected]>

builtin/logical/pki/storage_migrations.go

@@ -88,23 +88,40 @@ func migrateStorage(ctx context.Context, b *backend, s logical.Storage) error {
 	var keyIdentifier keyID
 	sc := b.makeStorageContext(ctx, s)
 	if migrationInfo.legacyBundle != nil {
-		// Generate a unique name for the migrated items in case things were to be re-migrated again
-		// for some weird reason in the future...
-		migrationName := fmt.Sprintf("current-%d", time.Now().Unix())
-
-		b.Logger().Info("performing PKI migration to new keys/issuers layout")
-		anIssuer, aKey, err := sc.writeCaBundle(migrationInfo.legacyBundle, migrationName, migrationName)
-		if err != nil {
-			return err
+		// When the legacy bundle still exists, there's three scenarios we
+		// need to worry about:
+		//
+		// 1. When we have no migration log, we definitely want to migrate.
+		haveNoLog := migrationInfo.migrationLog == nil
+		// 2. When we have an (empty) log and the version is zero, we want to
+		//    migrate.
+		haveOldVersion := !haveNoLog && migrationInfo.migrationLog.MigrationVersion == 0
+		// 3. When we have a log and the version is at least 1 (where this
+		//    migration was introduced), we want to run the migration again
+		//    only if the legacy bundle hash has changed.
+		isCurrentOrBetterVersion := !haveNoLog && migrationInfo.migrationLog.MigrationVersion >= 1
+		haveChange := !haveNoLog && migrationInfo.migrationLog.Hash != migrationInfo.legacyBundleHash
+		haveVersionWithChange := isCurrentOrBetterVersion && haveChange
+
+		if haveNoLog || haveOldVersion || haveVersionWithChange {
+			// Generate a unique name for the migrated items in case things were to be re-migrated again
+			// for some weird reason in the future...
+			migrationName := fmt.Sprintf("current-%d", time.Now().Unix())
+
+			b.Logger().Info("performing PKI migration to new keys/issuers layout")
+			anIssuer, aKey, err := sc.writeCaBundle(migrationInfo.legacyBundle, migrationName, migrationName)
+			if err != nil {
+				return err
+			}
+			b.Logger().Info("Migration generated the following ids and set them as defaults",
+				"issuer id", anIssuer.ID, "key id", aKey.ID)
+			issuerIdentifier = anIssuer.ID
+			keyIdentifier = aKey.ID
+
+			// Since we do not have all the mount information available we must schedule
+			// the CRL to be rebuilt at a later time.
+			b.crlBuilder.requestRebuildIfActiveNode(b)
 		}
-		b.Logger().Info("Migration generated the following ids and set them as defaults",
-			"issuer id", anIssuer.ID, "key id", aKey.ID)
-		issuerIdentifier = anIssuer.ID
-		keyIdentifier = aKey.ID
-
-		// Since we do not have all the mount information available we must schedule
-		// the CRL to be rebuilt at a later time.
-		b.crlBuilder.requestRebuildIfActiveNode(b)
 	}
 
 	if migrationInfo.migrationLog != nil && migrationInfo.migrationLog.MigrationVersion == 1 {

builtin/logical/pki/storage_migrations_test.go

@@ -777,6 +777,98 @@ func TestBackupBundle(t *testing.T) {
 	require.NotEmpty(t, keyIds)
 }
 
+func TestDeletedIssuersPostMigration(t *testing.T) {
+	// We want to simulate the following scenario:
+	//
+	// 1.10.x: -> Create a CA.
+	// 1.11.0: -> Migrate to new issuer layout but version 1.
+	//         -> Delete existing issuers, create new ones.
+	// (now):  -> Migrate to version 2 layout, make sure we don't see
+	//            re-migration.
+
+	t.Parallel()
+	ctx := context.Background()
+	b, s := CreateBackendWithStorage(t)
+	sc := b.makeStorageContext(ctx, s)
+
+	// Reset the version the helper above set to 1.
+	b.pkiStorageVersion.Store(0)
+	require.True(t, b.useLegacyBundleCaStorage(), "pre migration we should have been told to use legacy storage.")
+
+	// Create a legacy CA bundle and write it out.
+	bundle := genCertBundle(t, b, s)
+	json, err := logical.StorageEntryJSON(legacyCertBundlePath, bundle)
+	require.NoError(t, err)
+	err = s.Put(ctx, json)
+	require.NoError(t, err)
+	legacyContents := requireFileExists(t, sc, legacyCertBundlePath, nil)
+
+	// Do a migration; this should provision an issuer and key.
+	initReq := &logical.InitializationRequest{Storage: s}
+	err = b.initialize(ctx, initReq)
+	require.NoError(t, err)
+	requireFileExists(t, sc, legacyCertBundlePath, legacyContents)
+	issuerIds, err := sc.listIssuers()
+	require.NoError(t, err)
+	require.NotEmpty(t, issuerIds)
+	keyIds, err := sc.listKeys()
+	require.NoError(t, err)
+	require.NotEmpty(t, keyIds)
+
+	// Hack: reset the version to 1, to simulate a pre-version-2 migration
+	// log.
+	info, err := getMigrationInfo(sc.Context, sc.Storage)
+	require.NoError(t, err, "failed to read migration info")
+	info.migrationLog.MigrationVersion = 1
+	err = setLegacyBundleMigrationLog(sc.Context, sc.Storage, info.migrationLog)
+	require.NoError(t, err, "failed to write migration info")
+
+	// Now delete all issuers and keys and create some new ones.
+	for _, issuerId := range issuerIds {
+		deleted, err := sc.deleteIssuer(issuerId)
+		require.True(t, deleted, "expected it to be deleted")
+		require.NoError(t, err, "error removing issuer")
+	}
+	for _, keyId := range keyIds {
+		deleted, err := sc.deleteKey(keyId)
+		require.True(t, deleted, "expected it to be deleted")
+		require.NoError(t, err, "error removing key")
+	}
+	emptyIssuers, err := sc.listIssuers()
+	require.NoError(t, err)
+	require.Empty(t, emptyIssuers)
+	emptyKeys, err := sc.listKeys()
+	require.NoError(t, err)
+	require.Empty(t, emptyKeys)
+
+	// Create a new issuer + key.
+	bundle = genCertBundle(t, b, s)
+	_, _, err = sc.writeCaBundle(bundle, "", "")
+	require.NoError(t, err)
+
+	// List which issuers + keys we currently have.
+	postDeletionIssuers, err := sc.listIssuers()
+	require.NoError(t, err)
+	require.NotEmpty(t, postDeletionIssuers)
+	postDeletionKeys, err := sc.listKeys()
+	require.NoError(t, err)
+	require.NotEmpty(t, postDeletionKeys)
+
+	// Now do another migration from 1->2. This should retain the newly
+	// created issuers+keys, but not revive any deleted ones.
+	err = b.initialize(ctx, initReq)
+	require.NoError(t, err)
+	requireFileExists(t, sc, legacyCertBundlePath, legacyContents)
+	postMigrationIssuers, err := sc.listIssuers()
+	require.NoError(t, err)
+	require.NotEmpty(t, postMigrationIssuers)
+	require.Equal(t, postMigrationIssuers, postDeletionIssuers, "regression failed: expected second migration from v1->v2 to not introduce new issuers")
+	postMigrationKeys, err := sc.listKeys()
+	require.NoError(t, err)
+	require.NotEmpty(t, postMigrationKeys)
+	require.Equal(t, postMigrationKeys, postDeletionKeys, "regression failed: expected second migration from v1->v2 to not introduce new keys")
+}
+
 // requireFailInMigration validate that we fail the operation with the appropriate error message to the end-user
 func requireFailInMigration(t *testing.T, b *backend, s logical.Storage, operation logical.Operation, path string) {
 	resp, err := b.HandleRequest(context.Background(), &logical.Request{

changelog/21316.txt

@@ -0,0 +1,3 @@
+```release-note:bug
+secrets/pki: Prevent deleted issuers from reappearing when migrating from a version 1 bundle to a version 2 bundle (versions including 1.13.0, 1.12.2, and 1.11.6); when managed keys were removed but referenced in the Vault 1.10 legacy CA bundle, this the error: `no managed key found with uuid`.
+```

website/content/docs/upgrading/upgrade-to-1.11.x.mdx

@@ -43,3 +43,5 @@ vault write auth/ldap/config max_page_size=-1
 #### Impacted Versions
 
 Affects Vault 1.11.10.
+
+@include 'pki-double-migration-bug.mdx'

website/content/docs/upgrading/upgrade-to-1.12.x.mdx

@@ -213,3 +213,5 @@ flag for [PKI roles](/vault/api-docs/secret/pki#createupdate-role).
 #### Impacted Versions
 
 Affects Vault 1.12.0+
+
+@include 'pki-double-migration-bug.mdx'

website/content/docs/upgrading/upgrade-to-1.13.x.mdx

@@ -159,3 +159,5 @@ Affects Vault 1.13.0+
 @include 'perf-standby-token-create-forwarding-failure.mdx'
 
 @include 'update-primary-known-issue.mdx'
+
+@include 'pki-double-migration-bug.mdx'

website/content/partials/pki-double-migration-bug.mdx

@@ -0,0 +1,30 @@
+### PKI storage migration revives deleted issuers
+
+Vault 1.11 introduced Storage v1, a new storage layout that supported
+multiple issuers within a single mount. Bug fixes in Vault 1.11.6, 1.12.2,
+and 1.13.0 corrected a write-ordering issue that lead to invalid CA chains.
+Specifically, incorrectly ordered writes could fail due to load, resulting
+in the mount being re-migrated next time it was loaded or silently
+truncating CA chains. This collection of bug fixes introduced Storage v2.
+
+#### Affected versions
+
+Vault may incorrectly re-migrated legacy issuers created before Vault 1.11 that
+were migrated to Storage v1 and deleted before upgrading to a Vault version with
+Storage v2.
+
+The migration fails when Vault finds managed keys associated with the legacy
+issuers that were removed from the managed key repository prior to the upgrade.
+
+The migration error appears in Vault logs as:
+
+> Error during migration of PKI mount:
+>   failed to lookup public key from managed key:
+>     no managed key found with uuid
+
+<Note>
+Issuers created in Vault 1.11+ and direct upgrades to a Storage v2 layout are
+not affected.
+</Note>
+
+The Storage v1 upgrade bug was fixed in Vault 1.14.1, 1.13.5, and 1.12.9.