From bc42d56c7a4a41919d633d92299525bd9a635915 Mon Sep 17 00:00:00 2001 From: Steven Clark Date: Wed, 20 Sep 2023 16:51:52 -0400 Subject: [PATCH] Seal-HA: Match multiple seals using name/type only (#23203) * Match multiple seals using name/type only - This fix addresses an issue that changing any seal configuration in an existing seal stanza such as the Vault token would cause negate the seal matching. - If this was the only seal that was previously used or slight tweaks happened to all the seals Vault would fail to start with an error of "must have at least one seal in common with the old generation." - Also add a little more output to the validation error messages about the current seal and configured seal information to help in diagnosing errors in the future * Tweak formatting and text on method doc * Update comment around forcing a seal rewrap --- command/server.go | 4 + command/server_sealgenerationinfo_test.go | 37 +++++++- internalshared/configutil/kms.go | 4 +- vault/seal/seal.go | 104 +++++++++++++++++++--- 4 files changed, 132 insertions(+), 17 deletions(-) diff --git a/command/server.go b/command/server.go index 9f84830f83..e8cd04087e 100644 --- a/command/server.go +++ b/command/server.go @@ -2784,6 +2784,10 @@ func (c *ServerCommand) computeSealGenerationInfo(existingSealGenInfo *vaultseal generation := uint64(1) if existingSealGenInfo != nil { + // This forces a seal re-wrap on all seal related config changes, as we can't + // be sure what effect the config change might do. This is purposefully different + // from within the Validate call below that just matches on seal configs based + // on name/type. if cmp.Equal(existingSealGenInfo.Seals, sealConfigs) { return existingSealGenInfo, nil } diff --git a/command/server_sealgenerationinfo_test.go b/command/server_sealgenerationinfo_test.go index c7ce6bd73f..7e7b0995d1 100644 --- a/command/server_sealgenerationinfo_test.go +++ b/command/server_sealgenerationinfo_test.go @@ -599,7 +599,7 @@ func TestMultiSealCases(t *testing.T) { switch { case tc.isErrorExpected: require.Error(t, err) - require.EqualError(t, err, tc.expectedErrorMsg) + require.ErrorContains(t, err, tc.expectedErrorMsg) require.Nil(t, sealGenInfo) default: require.NoError(t, err) @@ -726,6 +726,39 @@ func TestMultiSealCases(t *testing.T) { isErrorExpected: true, expectedErrorMsg: "cannot make seal config changes while seal re-wrap is in progress, please revert any seal configuration changes", }, + // single seal migration use-case + { + name: "single_seal_migration", + existingSealGenInfo: &seal.SealGenerationInfo{ + Generation: 2, + Seals: []*configutil.KMS{ + { + Type: "transit", + Name: "transit", + Priority: 1, + }, + }, + }, + newSealGenInfo: &seal.SealGenerationInfo{ + Generation: 1, + Seals: []*configutil.KMS{ + { + Type: "transit", + Name: "transit-disabled", + Priority: 1, + Disabled: true, + }, + { + Type: "shamir", + Name: "shamir", + Priority: 1, + }, + }, + }, + isRewrapped: true, + hasPartiallyWrappedPaths: false, + isErrorExpected: false, + }, // have partially wrapped paths { name: "have_partially_wrapped_paths", @@ -801,7 +834,7 @@ func TestMultiSealCases(t *testing.T) { switch { case tc.isErrorExpected: require.Error(t, err) - require.EqualError(t, err, tc.expectedErrorMsg) + require.ErrorContains(t, err, tc.expectedErrorMsg) default: require.NoError(t, err) } diff --git a/internalshared/configutil/kms.go b/internalshared/configutil/kms.go index d68eaeb187..aa09d42cb6 100644 --- a/internalshared/configutil/kms.go +++ b/internalshared/configutil/kms.go @@ -44,6 +44,8 @@ type EntropyMode int const ( EntropyUnknown EntropyMode = iota EntropyAugmentation + + KmsRenameDisabledSuffix = "-disabled" ) type Entropy struct { @@ -133,7 +135,7 @@ func parseKMS(result *[]*KMS, list *ast.ObjectList, blockName string, maxKMS int name := strings.ToLower(key) // ensure that seals of the same type will have unique names for seal migration if disabled { - name += "-disabled" + name += KmsRenameDisabledSuffix } if v, ok := m["name"]; ok { name, ok = v.(string) diff --git a/vault/seal/seal.go b/vault/seal/seal.go index 26b2be56c6..32ab453c92 100644 --- a/vault/seal/seal.go +++ b/vault/seal/seal.go @@ -10,6 +10,7 @@ import ( "fmt" "reflect" "sort" + "strings" "sync/atomic" "time" @@ -62,10 +63,16 @@ type SealGenerationInfo struct { func (sgi *SealGenerationInfo) Validate(existingSgi *SealGenerationInfo, hasPartiallyWrappedPaths bool) error { existingSealsLen := 0 previousShamirConfigured := false + existingSealNameAndType := "[]" + configuredSealNameAndType := sealNameAndTypeAsStr(sgi.Seals) + if existingSgi != nil { + existingSealNameAndType = sealNameAndTypeAsStr(existingSgi.Seals) if sgi.Generation == existingSgi.Generation { - if !cmp.Equal(sgi.Seals, existingSgi.Seals) { - return errors.New("existing seal generation is the same, but the configured seals are different") + if !haveMatchingSeals(sgi.Seals, existingSgi.Seals) { + return fmt.Errorf("existing seal generation is the same, but the configured seals are different\n"+ + "Existing seals: %v\n"+ + "Configured seals: %v", existingSealNameAndType, configuredSealNameAndType) } return nil } @@ -98,37 +105,106 @@ func (sgi *SealGenerationInfo) Validate(existingSgi *SealGenerationInfo, hasPart numSealsToDelete := existingSealsLen - len(sgi.Seals) switch { case numSealsToAdd > 1: - return errors.New("cannot add more than one seal") + return fmt.Errorf("cannot add more than one seal\n"+ + "Existing seals: %v\n"+ + "Configured seals: %v", existingSealNameAndType, configuredSealNameAndType) case numSealsToDelete > 1: - return errors.New("cannot delete more than one seal") + return fmt.Errorf("cannot delete more than one seal\n"+ + "Existing seals: %v\n"+ + "Configured seals: %v", existingSealNameAndType, configuredSealNameAndType) case !previousShamirConfigured && existingSgi != nil && !haveCommonSeal(existingSgi.Seals, sgi.Seals): // With a previously configured shamir seal, we are either going from [shamir]->[auto] or [shamir]->[another shamir], // in which case we cannot have a common seal because shamir seals cannot be set to disabled, they can only be deleted. - return errors.New("must have at least one seal in common with the old generation") + return fmt.Errorf("must have at least one seal in common with the old generation\n"+ + "Existing seals: %v\n"+ + "Configured seals: %v", existingSealNameAndType, configuredSealNameAndType) } return nil } -func haveCommonSeal(existingSealKmsConfigs, newSealKmsConfigs []*configutil.KMS) (result bool) { +func sealNameAndTypeAsStr(seals []*configutil.KMS) string { + info := []string{} + for _, seal := range seals { + info = append(info, fmt.Sprintf("Name: %s Type: %s", seal.Name, seal.Type)) + } + return fmt.Sprintf("[%s]", strings.Join(info, ", ")) +} + +// haveMatchingSeals verifies that we have the corresponding matching seals by name and type, config and other +// properties are ignored in the comparison +func haveMatchingSeals(existingSealKmsConfigs, newSealKmsConfigs []*configutil.KMS) bool { + if len(existingSealKmsConfigs) != len(newSealKmsConfigs) { + return false + } + + for _, existingSealKmsConfig := range existingSealKmsConfigs { + found := false + for _, newSealKmsConfig := range newSealKmsConfigs { + if cmp.Equal(existingSealKmsConfig, newSealKmsConfig, compareKMSConfigByNameAndType()) { + found = true + break + } + } + + if !found { + return false + } + } + return true +} + +// haveCommonSeal verifies that we have at least one matching seal across +// the inputs by name and type, config and other properties are ignored in +// the comparison +func haveCommonSeal(existingSealKmsConfigs, newSealKmsConfigs []*configutil.KMS) bool { for _, existingSealKmsConfig := range existingSealKmsConfigs { for _, newSealKmsConfig := range newSealKmsConfigs { - // Clone the existing seal config and set 'Disabled' and 'Priority' fields same as the - // new seal config, because there might be a case where a seal might be disabled in - // current config, but might be stored as enabled previously, and this still needs to - // be considered as a common seal. - clonedSgi := existingSealKmsConfig.Clone() - clonedSgi.Disabled = newSealKmsConfig.Disabled - clonedSgi.Priority = newSealKmsConfig.Priority - if cmp.Equal(clonedSgi, newSealKmsConfig.Clone()) { + // Technically we might be matching the "wrong" seal if the old seal was renamed to + // "transit-disabled" and we have a new seal named transit. There isn't any way for + // us to properly distinguish between them + if cmp.Equal(existingSealKmsConfig, newSealKmsConfig, compareKMSConfigByNameAndType()) { return true } } } + + // We might have renamed a disabled seal that was previously used so attempt to match by + // removing the "-disabled" suffix + for _, seal := range findRenamedDisabledSeals(newSealKmsConfigs) { + clonedSeal := seal.Clone() + clonedSeal.Name = strings.TrimSuffix(clonedSeal.Name, configutil.KmsRenameDisabledSuffix) + + for _, existingSealKmsConfig := range existingSealKmsConfigs { + if cmp.Equal(existingSealKmsConfig, clonedSeal, compareKMSConfigByNameAndType()) { + return true + } + } + } + return false } +func findRenamedDisabledSeals(configs []*configutil.KMS) []*configutil.KMS { + diabledSeals := []*configutil.KMS{} + for _, seal := range configs { + if seal.Disabled && strings.HasSuffix(seal.Name, configutil.KmsRenameDisabledSuffix) { + diabledSeals = append(diabledSeals, seal) + } + } + return diabledSeals +} + +func compareKMSConfigByNameAndType() cmp.Option { + // We only match based on name and type to avoid configuration changes such + // as a Vault token change in the config map from eliminating the match and + // preventing startup on a matching seal. + return cmp.Comparer(func(a, b *configutil.KMS) bool { + return a.Name == b.Name && a.Type == b.Type + }) +} + // SetRewrapped updates the SealGenerationInfo's rewrapped status to the provided value. func (sgi *SealGenerationInfo) SetRewrapped(value bool) { sgi.rewrapped.Store(value)