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)