From ff055c6425d58c46c999220849248e9c103074c6 Mon Sep 17 00:00:00 2001 From: Steve Clark Date: Mon, 22 Aug 2022 11:43:05 -0400 Subject: [PATCH 1/2] Migrate existing PKI mounts that only contains a key - We missed testing a use-case of the migration that someone has a PKI mount point that generated a CSR but never called set-signed back on that mount point so it only contains a key. --- builtin/logical/pki/storage.go | 6 ++ .../logical/pki/storage_migrations_test.go | 82 +++++++++++++++++++ 2 files changed, 88 insertions(+) diff --git a/builtin/logical/pki/storage.go b/builtin/logical/pki/storage.go index ad9ac81c05c1..be7b88e37e0e 100644 --- a/builtin/logical/pki/storage.go +++ b/builtin/logical/pki/storage.go @@ -951,6 +951,12 @@ func (sc *storageContext) writeCaBundle(caBundle *certutil.CertBundle, issuerNam return nil, nil, err } + // We may have existing mounts that only contained a key with no certificate yet as a signed CSR + // was never setup within the mount. + if caBundle.Certificate == "" { + return &issuerEntry{}, myKey, nil + } + myIssuer, _, err := sc.importIssuer(caBundle.Certificate, issuerName) if err != nil { return nil, nil, err diff --git a/builtin/logical/pki/storage_migrations_test.go b/builtin/logical/pki/storage_migrations_test.go index 78ac24fb4379..96da109bbf57 100644 --- a/builtin/logical/pki/storage_migrations_test.go +++ b/builtin/logical/pki/storage_migrations_test.go @@ -60,6 +60,88 @@ func Test_migrateStorageEmptyStorage(t *testing.T) { require.Equal(t, logEntry.Hash, logEntry2.Hash) } +func Test_migrateStorageOnlyKey(t *testing.T) { + t.Parallel() + startTime := time.Now() + 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.") + + bundle := genCertBundle(t, b, s) + // Clear everything except for the key + bundle.SerialNumber = "" + bundle.CAChain = []string{} + bundle.Certificate = "" + bundle.IssuingCA = "" + + json, err := logical.StorageEntryJSON(legacyCertBundlePath, bundle) + require.NoError(t, err) + err = s.Put(ctx, json) + require.NoError(t, err) + + request := &logical.InitializationRequest{Storage: s} + err = b.initialize(ctx, request) + require.NoError(t, err) + + issuerIds, err := sc.listIssuers() + require.NoError(t, err) + require.Equal(t, 0, len(issuerIds)) + + keyIds, err := sc.listKeys() + require.NoError(t, err) + require.Equal(t, 1, len(keyIds)) + + logEntry, err := getLegacyBundleMigrationLog(ctx, s) + require.NoError(t, err) + require.NotNil(t, logEntry) + require.Equal(t, latestMigrationVersion, logEntry.MigrationVersion) + require.True(t, len(strings.TrimSpace(logEntry.Hash)) > 0, + "Hash value (%s) should not have been empty", logEntry.Hash) + require.True(t, startTime.Before(logEntry.Created), + "created log entry time (%v) was before our start time(%v)?", logEntry.Created, startTime) + require.Equal(t, logEntry.CreatedIssuer, issuerID("")) + require.Equal(t, logEntry.CreatedKey, keyIds[0]) + + keyId := keyIds[0] + key, err := sc.fetchKeyById(keyId) + require.NoError(t, err) + require.True(t, strings.HasPrefix(key.Name, "current-"), + "expected key name to start with current- was %s", key.Name) + require.Equal(t, keyId, key.ID) + require.Equal(t, strings.TrimSpace(bundle.PrivateKey), strings.TrimSpace(key.PrivateKey)) + require.Equal(t, bundle.PrivateKeyType, key.PrivateKeyType) + + // Make sure we kept the old bundle + _, certBundle, err := getLegacyCertBundle(ctx, s) + require.NoError(t, err) + require.Equal(t, bundle, certBundle) + + // Make sure we setup the default values + keysConfig, err := sc.getKeysConfig() + require.NoError(t, err) + require.Equal(t, &keyConfigEntry{DefaultKeyId: keyId}, keysConfig) + + issuersConfig, err := sc.getIssuersConfig() + require.NoError(t, err) + require.Equal(t, &issuerConfigEntry{}, issuersConfig) + + // Make sure if we attempt to re-run the migration nothing happens... + err = migrateStorage(ctx, b, s) + require.NoError(t, err) + logEntry2, err := getLegacyBundleMigrationLog(ctx, s) + require.NoError(t, err) + require.NotNil(t, logEntry2) + + require.Equal(t, logEntry.Created, logEntry2.Created) + require.Equal(t, logEntry.Hash, logEntry2.Hash) + + require.False(t, b.useLegacyBundleCaStorage(), "post migration we are still told to use legacy storage") +} + func Test_migrateStorageSimpleBundle(t *testing.T) { t.Parallel() startTime := time.Now() From 95379429df2a1a94b3af01079f3d04317a25db68 Mon Sep 17 00:00:00 2001 From: Steve Clark Date: Mon, 22 Aug 2022 12:56:34 -0400 Subject: [PATCH 2/2] Add cl --- changelog/16813.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 changelog/16813.txt diff --git a/changelog/16813.txt b/changelog/16813.txt new file mode 100644 index 000000000000..352fa0916766 --- /dev/null +++ b/changelog/16813.txt @@ -0,0 +1,3 @@ +```release-note:bug +secrets/pki: Fix migration to properly handle mounts that contain only keys, no certificates +```