From e600d436903dcc777dc3bd3fce450fbf049f0d70 Mon Sep 17 00:00:00 2001 From: Divya Pola <87338962+divyapola5@users.noreply.github.com> Date: Mon, 28 Aug 2023 11:58:16 -0500 Subject: [PATCH] Add safety logic for rejecting seal configuration changes (#22582) * Fix clone method and add new validation for same gen * Add safety logic for rejecting seal configuration changes * Remove ent build req for test file --- command/server.go | 39 +- command/server_sealgenerationinfo_test.go | 606 ++++++++++++++++++++++ internalshared/configutil/kms.go | 13 + vault/seal/seal.go | 69 +++ 4 files changed, 712 insertions(+), 15 deletions(-) create mode 100644 command/server_sealgenerationinfo_test.go diff --git a/command/server.go b/command/server.go index aed12170d6..e86f377c9b 100644 --- a/command/server.go +++ b/command/server.go @@ -2654,7 +2654,10 @@ func setSeal(c *ServerCommand, config *server.Config, infoKeys []string, info ma //////////////////////////////////////////////////////////////////////////////////////////////////////////////////// // Compute seal generation - sealGenerationInfo := c.computeSealGenerationInfo(existingSealGenerationInfo, allSealKmsConfigs) + sealGenerationInfo, err := c.computeSealGenerationInfo(existingSealGenerationInfo, allSealKmsConfigs) + if err != nil { + return nil, err + } //////////////////////////////////////////////////////////////////////////////////////////////////////////////////// // Create the Seals @@ -2680,7 +2683,7 @@ func setSeal(c *ServerCommand, config *server.Config, infoKeys []string, info ma return nil, errors.New("no enabled Seals in configuration") case containsShamir(enabledSealInfos) && containsShamir(disabledSealInfos): - return nil, errors.New("cannot migrate from one Shamir seal to another Shamir seal") + return nil, errors.New("shamir seals cannot be set disabled (they should simply not be set)") case len(enabledSealInfos) == 1 && containsShamir(enabledSealInfos): // The barrier seal is Shamir. If there are any disabled seals, then we put them all in the same @@ -2720,25 +2723,31 @@ func setSeal(c *ServerCommand, config *server.Config, infoKeys []string, info ma }, nil } -func (c *ServerCommand) computeSealGenerationInfo(existingSealGenInfo *vaultseal.SealGenerationInfo, sealConfigs []*configutil.KMS) *vaultseal.SealGenerationInfo { - if existingSealGenInfo == nil { - return &vaultseal.SealGenerationInfo{ - Generation: 1, - Seals: sealConfigs, - } - } - if cmp.Equal(existingSealGenInfo.Seals, sealConfigs) { - return existingSealGenInfo - } +func (c *ServerCommand) computeSealGenerationInfo(existingSealGenInfo *vaultseal.SealGenerationInfo, sealConfigs []*configutil.KMS) (*vaultseal.SealGenerationInfo, error) { + var generation uint64 + generation = 1 - generation := existingSealGenInfo.Generation + 1 + if existingSealGenInfo != nil { + if cmp.Equal(existingSealGenInfo.Seals, sealConfigs) { + return existingSealGenInfo, nil + } + generation = existingSealGenInfo.Generation + 1 + } c.logger.Info("incrementing seal config gen, new generation: ", "generation", generation) - // If the stored copy doesn't match the current configuration, we introduce a new generation which keeps track if a rewrap of all CSPs and seal wrapped values has completed (initially false). - return &vaultseal.SealGenerationInfo{ + // If the stored copy doesn't match the current configuration, we introduce a new generation + // which keeps track if a rewrap of all CSPs and seal wrapped values has completed (initially false). + newSealGenInfo := &vaultseal.SealGenerationInfo{ Generation: generation, Seals: sealConfigs, } + + err := newSealGenInfo.Validate(existingSealGenInfo) + if err != nil { + return nil, err + } + + return newSealGenInfo, nil } func initHaBackend(c *ServerCommand, config *server.Config, coreConfig *vault.CoreConfig, backend physical.Backend) (bool, error) { diff --git a/command/server_sealgenerationinfo_test.go b/command/server_sealgenerationinfo_test.go new file mode 100644 index 0000000000..15977d1c63 --- /dev/null +++ b/command/server_sealgenerationinfo_test.go @@ -0,0 +1,606 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: MPL-2.0 + +package command + +import ( + "os" + "testing" + + "github.com/hashicorp/vault/helper/testhelpers/corehelpers" + + "github.com/hashicorp/vault/vault/seal" + + "github.com/hashicorp/vault/internalshared/configutil" + "github.com/stretchr/testify/require" +) + +func init() { + if signed := os.Getenv("VAULT_LICENSE_CI"); signed != "" { + os.Setenv(EnvVaultLicense, signed) + } +} + +func TestMultiSealCases(t *testing.T) { + cases := []struct { + name string + existingSealGenInfo *seal.SealGenerationInfo + allSealKmsConfigs []*configutil.KMS + expectedSealGenInfo *seal.SealGenerationInfo + isErrorExpected bool + expectedErrorMsg string + }{ + // none_to_shamir + { + name: "none_to_shamir", + existingSealGenInfo: nil, + allSealKmsConfigs: []*configutil.KMS{ + { + Type: "shamir", + Name: "shamirSeal1", + Priority: 1, + }, + }, + expectedSealGenInfo: &seal.SealGenerationInfo{ + Generation: 1, + Seals: []*configutil.KMS{ + { + Type: "shamir", + Name: "shamirSeal1", + Priority: 1, + }, + }, + }, + }, + // none_to_auto + { + name: "none_to_auto", + existingSealGenInfo: nil, + allSealKmsConfigs: []*configutil.KMS{ + { + Type: "pkcs11", + Name: "autoSeal1", + Priority: 1, + }, + }, + expectedSealGenInfo: &seal.SealGenerationInfo{ + Generation: 1, + Seals: []*configutil.KMS{ + { + Type: "pkcs11", + Name: "autoSeal1", + Priority: 1, + }, + }, + }, + }, + // none_to_multi + { + name: "none_to_multi", + existingSealGenInfo: nil, + allSealKmsConfigs: []*configutil.KMS{ + { + Type: "pkcs11", + Name: "autoSeal1", + Priority: 1, + }, + { + Type: "pkcs11", + Name: "autoSeal2", + Priority: 2, + }, + }, + isErrorExpected: true, + expectedErrorMsg: "cannot add more than one seal", + }, + // shamir_to_auto + { + name: "shamir_to_auto", + existingSealGenInfo: &seal.SealGenerationInfo{ + Generation: 2, + Seals: []*configutil.KMS{ + { + Type: "shamir", + Name: "shamirSeal1", + Priority: 1, + }, + }, + }, + allSealKmsConfigs: []*configutil.KMS{ + { + Type: "pkcs11", + Name: "autoSeal1", + Priority: 1, + }, + }, + expectedSealGenInfo: &seal.SealGenerationInfo{ + Generation: 3, + Seals: []*configutil.KMS{ + { + Type: "pkcs11", + Name: "autoSeal1", + Priority: 1, + }, + }, + }, + }, + // shamir_to_multi + { + name: "shamir_to_multi", + existingSealGenInfo: &seal.SealGenerationInfo{ + Generation: 1, + Seals: []*configutil.KMS{ + { + Type: "shamir", + Name: "shamirSeal1", + Priority: 1, + }, + }, + }, + allSealKmsConfigs: []*configutil.KMS{ + { + Type: "pkcs11", + Name: "autoSeal1", + Priority: 2, + }, + { + Type: "pkcs11", + Name: "autoSeal2", + Priority: 3, + }, + }, + isErrorExpected: true, + expectedErrorMsg: "cannot add more than one seal", + }, + // auto_to_shamir_no_common_seal + { + name: "auto_to_shamir_no_common_seal", + existingSealGenInfo: &seal.SealGenerationInfo{ + Generation: 1, + Seals: []*configutil.KMS{ + { + Type: "pkcs11", + Name: "autoSeal1", + Priority: 1, + }, + }, + }, + allSealKmsConfigs: []*configutil.KMS{ + { + Type: "shamir", + Name: "shamirSeal1", + Priority: 1, + }, + }, + isErrorExpected: true, + expectedErrorMsg: "must have at least one seal in common with the old generation", + }, + // auto_to_shamir_with_common_seal + { + name: "auto_to_shamir_with_common_seal", + existingSealGenInfo: &seal.SealGenerationInfo{ + Generation: 1, + Seals: []*configutil.KMS{ + { + Type: "pkcs11", + Name: "autoSeal1", + Priority: 1, + }, + }, + }, + allSealKmsConfigs: []*configutil.KMS{ + { + Type: "shamir", + Name: "shamirSeal1", + Priority: 1, + }, + { + Type: "pkcs11", + Name: "autoSeal1", + Priority: 1, + Disabled: true, + }, + }, + expectedSealGenInfo: &seal.SealGenerationInfo{ + Generation: 2, + Seals: []*configutil.KMS{ + { + Type: "shamir", + Name: "shamirSeal1", + Priority: 1, + }, + { + Type: "pkcs11", + Name: "autoSeal1", + Priority: 1, + Disabled: true, + }, + }, + }, + }, + // auto_to_auto_no_common_seal + { + name: "auto_to_auto_no_common_seal", + existingSealGenInfo: &seal.SealGenerationInfo{ + Generation: 1, + Seals: []*configutil.KMS{ + { + Type: "pkcs11", + Name: "autoSeal1", + Priority: 1, + }, + }, + }, + allSealKmsConfigs: []*configutil.KMS{ + { + Type: "pkcs11", + Name: "autoSeal2", + Priority: 1, + }, + }, + isErrorExpected: true, + expectedErrorMsg: "must have at least one seal in common with the old generation", + }, + // auto_to_auto_with_common_seal + { + name: "auto_to_auto_with_common_seal", + existingSealGenInfo: &seal.SealGenerationInfo{ + Generation: 1, + Seals: []*configutil.KMS{ + { + Type: "pkcs11", + Name: "autoSeal1", + Priority: 1, + }, + }, + }, + allSealKmsConfigs: []*configutil.KMS{ + { + Type: "pkcs11", + Name: "autoSeal1", + Priority: 1, + Disabled: true, + }, + { + Type: "pkcs11", + Name: "autoSeal2", + Priority: 2, + }, + }, + expectedSealGenInfo: &seal.SealGenerationInfo{ + Generation: 2, + Seals: []*configutil.KMS{ + { + Type: "pkcs11", + Name: "autoSeal1", + Priority: 1, + Disabled: true, + }, + { + Type: "pkcs11", + Name: "autoSeal2", + Priority: 2, + }, + }, + }, + }, + // auto_to_multi_add_one + { + name: "auto_to_multi_add_one", + existingSealGenInfo: &seal.SealGenerationInfo{ + Generation: 1, + Seals: []*configutil.KMS{ + { + Type: "pkcs11", + Name: "autoSeal1", + Priority: 1, + }, + }, + }, + allSealKmsConfigs: []*configutil.KMS{ + { + Type: "pkcs11", + Name: "autoSeal1", + Priority: 1, + }, + { + Type: "pkcs11", + Name: "autoSeal2", + Priority: 2, + }, + }, + expectedSealGenInfo: &seal.SealGenerationInfo{ + Generation: 2, + Seals: []*configutil.KMS{ + { + Type: "pkcs11", + Name: "autoSeal1", + Priority: 1, + }, + { + Type: "pkcs11", + Name: "autoSeal2", + Priority: 2, + }, + }, + }, + }, + // auto_to_multi_add_two + { + name: "auto_to_multi_add_two", + existingSealGenInfo: &seal.SealGenerationInfo{ + Generation: 1, + Seals: []*configutil.KMS{ + { + Type: "pkcs11", + Name: "autoSeal1", + Priority: 1, + }, + }, + }, + allSealKmsConfigs: []*configutil.KMS{ + { + Type: "pkcs11", + Name: "autoSeal1", + Priority: 1, + }, + { + Type: "pkcs11", + Name: "autoSeal2", + Priority: 2, + }, + { + Type: "pkcs11", + Name: "autoSeal3", + Priority: 3, + }, + }, + isErrorExpected: true, + expectedErrorMsg: "cannot add more than one seal", + }, + // multi_to_auto_delete_one + { + name: "multi_to_auto_delete_one", + existingSealGenInfo: &seal.SealGenerationInfo{ + Generation: 1, + Seals: []*configutil.KMS{ + { + Type: "pkcs11", + Name: "autoSeal1", + Priority: 1, + }, + { + Type: "pkcs11", + Name: "autoSeal2", + Priority: 2, + }, + }, + }, + allSealKmsConfigs: []*configutil.KMS{ + { + Type: "pkcs11", + Name: "autoSeal1", + Priority: 1, + }, + }, + expectedSealGenInfo: &seal.SealGenerationInfo{ + Generation: 2, + Seals: []*configutil.KMS{ + { + Type: "pkcs11", + Name: "autoSeal1", + Priority: 1, + }, + }, + }, + }, + // multi_to_auto_delete_two + { + name: "multi_to_auto_delete_two", + existingSealGenInfo: &seal.SealGenerationInfo{ + Generation: 1, + Seals: []*configutil.KMS{ + { + Type: "pkcs11", + Name: "autoSeal1", + Priority: 1, + }, + { + Type: "pkcs11", + Name: "autoSeal2", + Priority: 2, + }, + { + Type: "pkcs11", + Name: "autoSeal3", + Priority: 3, + }, + }, + }, + allSealKmsConfigs: []*configutil.KMS{ + { + Type: "pkcs11", + Name: "autoSeal1", + Priority: 1, + }, + }, + isErrorExpected: true, + expectedErrorMsg: "cannot delete more than one seal", + }, + // disable_two_auto + { + name: "disable_two_auto", + existingSealGenInfo: &seal.SealGenerationInfo{ + Generation: 1, + Seals: []*configutil.KMS{ + { + Type: "pkcs11", + Name: "autoSeal1", + Priority: 1, + }, + { + Type: "pkcs11", + Name: "autoSeal2", + Priority: 2, + }, + { + Type: "pkcs11", + Name: "autoSeal3", + Priority: 3, + }, + }, + }, + allSealKmsConfigs: []*configutil.KMS{ + { + Type: "pkcs11", + Name: "autoSeal1", + Priority: 1, + }, + { + Type: "pkcs11", + Name: "autoSeal2", + Priority: 2, + Disabled: true, + }, + { + Type: "pkcs11", + Name: "autoSeal3", + Priority: 3, + Disabled: true, + }, + }, + expectedSealGenInfo: &seal.SealGenerationInfo{ + Generation: 2, + Seals: []*configutil.KMS{ + { + Type: "pkcs11", + Name: "autoSeal1", + Priority: 1, + }, + { + Type: "pkcs11", + Name: "autoSeal2", + Priority: 2, + Disabled: true, + }, + { + Type: "pkcs11", + Name: "autoSeal3", + Priority: 3, + Disabled: true, + }, + }, + }, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + cmd := &ServerCommand{} + cmd.logger = corehelpers.NewTestLogger(t) + sealGenInfo, err := cmd.computeSealGenerationInfo(tc.existingSealGenInfo, tc.allSealKmsConfigs) + switch { + case tc.isErrorExpected: + require.Error(t, err) + require.EqualError(t, err, tc.expectedErrorMsg) + require.Nil(t, sealGenInfo) + default: + require.NoError(t, err) + require.Equal(t, tc.expectedSealGenInfo, sealGenInfo) + } + }) + } + + cases2 := []struct { + name string + existingSealGenInfo *seal.SealGenerationInfo + newSealGenInfo *seal.SealGenerationInfo + isErrorExpected bool + expectedErrorMsg string + }{ + // same_generation_different_seals + { + name: "same_generation_different_seals", + existingSealGenInfo: &seal.SealGenerationInfo{ + Generation: 1, + Seals: []*configutil.KMS{ + { + Type: "pkcs11", + Name: "autoSeal1", + Priority: 1, + }, + { + Type: "pkcs11", + Name: "autoSeal2", + Priority: 2, + }, + }, + }, + newSealGenInfo: &seal.SealGenerationInfo{ + Generation: 1, + Seals: []*configutil.KMS{ + { + Type: "pkcs11", + Name: "autoSeal1", + Priority: 1, + }, + { + Type: "pkcs11", + Name: "autoSeal3", + Priority: 2, + }, + }, + }, + isErrorExpected: true, + expectedErrorMsg: "existing seal generation is the same, but the configured seals are different", + }, + + // same_generation_same_seals + { + name: "same_generation_same_seals", + existingSealGenInfo: &seal.SealGenerationInfo{ + Generation: 1, + Seals: []*configutil.KMS{ + { + Type: "pkcs11", + Name: "autoSeal1", + Priority: 1, + }, + { + Type: "pkcs11", + Name: "autoSeal2", + Priority: 2, + }, + }, + }, + newSealGenInfo: &seal.SealGenerationInfo{ + Generation: 1, + Seals: []*configutil.KMS{ + { + Type: "pkcs11", + Name: "autoSeal1", + Priority: 1, + }, + { + Type: "pkcs11", + Name: "autoSeal2", + Priority: 2, + }, + }, + }, + isErrorExpected: false, + }, + } + for _, tc := range cases2 { + t.Run(tc.name, func(t *testing.T) { + err := tc.newSealGenInfo.Validate(tc.existingSealGenInfo) + switch { + case tc.isErrorExpected: + require.Error(t, err) + require.EqualError(t, err, tc.expectedErrorMsg) + default: + require.NoError(t, err) + } + }) + } +} diff --git a/internalshared/configutil/kms.go b/internalshared/configutil/kms.go index 096c8b4ca9..cb2a79f2e0 100644 --- a/internalshared/configutil/kms.go +++ b/internalshared/configutil/kms.go @@ -437,3 +437,16 @@ func getEnvConfig(kms *KMS) map[string]string { return envValues } + +func (k *KMS) Clone() *KMS { + ret := &KMS{ + UnusedKeys: k.UnusedKeys, + Type: k.Type, + Purpose: k.Purpose, + Config: k.Config, + Name: k.Name, + Disabled: k.Disabled, + Priority: k.Priority, + } + return ret +} diff --git a/vault/seal/seal.go b/vault/seal/seal.go index e516307933..2b9773577d 100644 --- a/vault/seal/seal.go +++ b/vault/seal/seal.go @@ -13,6 +13,8 @@ import ( "sync/atomic" "time" + "github.com/google/go-cmp/cmp" + "github.com/hashicorp/go-hclog" "github.com/hashicorp/vault/internalshared/configutil" @@ -51,6 +53,73 @@ type SealGenerationInfo struct { rewrapped atomic.Bool } +// Validate is used to sanity check the seal generation info being created +func (sgi *SealGenerationInfo) Validate(existingSgi *SealGenerationInfo) error { + existingSealsLen := 0 + previousShamirConfigured := false + if existingSgi != nil { + 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") + } + return nil + } + + existingSealsLen = len(existingSgi.Seals) + for _, sealKmsConfig := range existingSgi.Seals { + if sealKmsConfig.Type == wrapping.WrapperTypeShamir.String() { + previousShamirConfigured = true + break + } + } + } + + numSealsToAdd := 0 + // With a previously configured shamir seal, we are either going from [shamir]->[auto] + // or [shamir]->[another shamir] (since we do not allow multiple shamir + // seals, and, mixed shamir and auto seals). Also, we do not allow shamir seals to + // be set disabled, so, the number of seals to add is always going to be the length + // of new seal configs. + if previousShamirConfigured { + numSealsToAdd = len(sgi.Seals) + } else { + numSealsToAdd = len(sgi.Seals) - existingSealsLen + } + + numSealsToDelete := existingSealsLen - len(sgi.Seals) + switch { + case numSealsToAdd > 1: + return errors.New("cannot add more than one seal") + + case numSealsToDelete > 1: + return errors.New("cannot delete more than one seal") + + 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 nil +} + +func haveCommonSeal(existingSealKmsConfigs, newSealKmsConfigs []*configutil.KMS) (result 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()) { + return true + } + } + } + return false +} + // SetRewrapped updates the SealGenerationInfo's rewrapped status to the provided value. func (sgi *SealGenerationInfo) SetRewrapped(value bool) { sgi.rewrapped.Store(value)