diff --git a/docs/schemas/gcp/secretsproviderconfig.json b/docs/schemas/gcp/secretsproviderconfig.json index d6ef1b94..c484e2c2 100644 --- a/docs/schemas/gcp/secretsproviderconfig.json +++ b/docs/schemas/gcp/secretsproviderconfig.json @@ -31,6 +31,9 @@ ], "type": "object" }, + "allowShortKeyRotation": { + "type": "boolean" + }, "keyLocation": { "type": "string" }, @@ -48,7 +51,6 @@ "", "keyLocation", "keyName", - "keyRotationPeriod", "provision" ], "type": "object" diff --git a/pkg/clouds/gcloud/auth.go b/pkg/clouds/gcloud/auth.go index 161c87e1..e68e1ef0 100644 --- a/pkg/clouds/gcloud/auth.go +++ b/pkg/clouds/gcloud/auth.go @@ -6,6 +6,10 @@ package gcloud import ( "encoding/json" "fmt" + "strconv" + "strings" + + "github.com/pkg/errors" "github.com/simple-container-com/api/pkg/api" ) @@ -18,6 +22,35 @@ const ( SecretsProviderTypeGcpKms = "gcp-kms" ) +const ( + // DefaultKeyRotationPeriod is applied when keyRotationPeriod is unset. + // + // Cloud KMS bills every ACTIVE key version (ENABLED, DISABLED and + // DESTROY_SCHEDULED all count; only DESTROYED is free) and rotation never + // re-encrypts existing ciphertext, so every version a key mints stays + // load-bearing and billed for the lifetime of the key. That makes the + // rotation period a direct, compounding cost multiplier: one provisioned + // key per stack rotating daily adds a billed version per stack per day, + // forever. + DefaultKeyRotationPeriod = "7776000s" // 90 days + + // MinKeyRotationPeriodSeconds is the lower bound accepted for an explicit + // keyRotationPeriod. GCP's own floor is 86400s (1 day), which is far too + // low to be a sane default for a per-stack provisioned key: a period of a + // few hours or days passes GCP validation and silently accrues versions. + // Rejecting anything under 30 days turns that class of typo into a + // config-parse error instead of an unattributed bill months later. + MinKeyRotationPeriodSeconds = 2592000 // 30 days + + // GCP's own documented bounds for rotationPeriod: at least 24h, at most + // 876,000h. These are NOT waivable by AllowShortKeyRotation — a value + // outside them is rejected by the KMS API, and by then the KeyRing has + // already been created and can never be deleted. Failing here keeps that + // class of error away from any side effect. + GcpMinKeyRotationPeriodSeconds = 86400 // 24h + GcpMaxKeyRotationPeriodSeconds = 3153600000 // 876,000h +) + type ServiceAccountConfig struct { ProjectId string `json:"projectId" yaml:"projectId"` } @@ -58,7 +91,13 @@ type SecretsProviderConfig struct { // only applicable when provision=true KeyLocation string `json:"keyLocation" yaml:"keyLocation"` // only applicable when provision=true - KeyRotationPeriod string `json:"keyRotationPeriod" yaml:"keyRotationPeriod"` + KeyRotationPeriod string `json:"keyRotationPeriod,omitempty" yaml:"keyRotationPeriod,omitempty"` + // AllowShortKeyRotation opts out of the MinKeyRotationPeriodSeconds floor. + // Rotating faster than 30 days is a legitimate compliance choice; it is + // gated only because every rotation mints a permanently billed key version, + // so the common case of a mistyped period should fail loudly. Setting this + // makes the short period a deliberate, reviewable decision. + AllowShortKeyRotation bool `json:"allowShortKeyRotation,omitempty" yaml:"allowShortKeyRotation,omitempty"` // whether to provision key Provision bool `json:"provision" yaml:"provision"` @@ -80,6 +119,51 @@ func (r *SecretsProviderConfig) KeyUrl() string { return r.KeyName } +// EffectiveKeyRotationPeriod returns the configured rotation period, or +// DefaultKeyRotationPeriod when unset. +func (r *SecretsProviderConfig) EffectiveKeyRotationPeriod() string { + if r.KeyRotationPeriod == "" { + return DefaultKeyRotationPeriod + } + return r.KeyRotationPeriod +} + +// Validate checks the secrets-provider config. Follows the same shape as the +// other GCP configs in this package (PostgresGcpCloudsqlConfig.Validate, +// ExternalEgressIpConfig.Validate) so there is one convention to learn. +// +// keyRotationPeriod only applies when the key is provisioned here; a BYO key +// referenced by keyName ignores it, so a stale value must not block those +// consumers. An empty value is valid and means DefaultKeyRotationPeriod. +func (r *SecretsProviderConfig) Validate() error { + if !r.Provision || r.KeyRotationPeriod == "" { + return nil + } + raw := r.KeyRotationPeriod + if !strings.HasSuffix(raw, "s") { + return errors.Errorf("keyRotationPeriod %q must be a duration in seconds with an 's' suffix, e.g. %q", raw, DefaultKeyRotationPeriod) + } + secs, err := strconv.Atoi(strings.TrimSuffix(raw, "s")) + if err != nil { + return errors.Errorf("keyRotationPeriod %q must be a whole number of seconds with an 's' suffix, e.g. %q", raw, DefaultKeyRotationPeriod) + } + // GCP's own bounds first, and never waivable: outside them the KMS API + // rejects the key AFTER the KeyRing exists. + if secs < GcpMinKeyRotationPeriodSeconds { + return errors.Errorf("keyRotationPeriod %q is %d seconds; GCP requires at least %d (24h)", + raw, secs, GcpMinKeyRotationPeriodSeconds) + } + if secs > GcpMaxKeyRotationPeriodSeconds { + return errors.Errorf("keyRotationPeriod %q is %d seconds; GCP allows at most %d (876,000h)", + raw, secs, GcpMaxKeyRotationPeriodSeconds) + } + if secs < MinKeyRotationPeriodSeconds && !r.AllowShortKeyRotation { + return errors.Errorf("keyRotationPeriod %q (%ds) is below the %ds (30 day) minimum; set allowShortKeyRotation: true to override", + raw, secs, MinKeyRotationPeriodSeconds) + } + return nil +} + func (r *Credentials) ProviderType() string { return ProviderType } @@ -109,5 +193,18 @@ func ReadStateStorageConfig(config *api.Config) (api.Config, error) { } func ReadSecretsProviderConfig(config *api.Config) (api.Config, error) { - return api.ConvertConfig(config, &SecretsProviderConfig{}) + out, err := api.ConvertConfig(config, &SecretsProviderConfig{}) + if err != nil { + return out, err + } + // Validate here rather than only in the provisioner: the secrets-provider + // stack is Up'd only when its URL export is absent, so a provisioner-only + // check never runs for an already-provisioned stack and a bad value would + // sit unnoticed until a DR rebuild. + if sp, ok := out.Config.(*SecretsProviderConfig); ok { + if err := sp.Validate(); err != nil { + return out, err + } + } + return out, nil } diff --git a/pkg/clouds/gcloud/config_validation_test.go b/pkg/clouds/gcloud/config_validation_test.go index bdc7c303..6c91acfc 100644 --- a/pkg/clouds/gcloud/config_validation_test.go +++ b/pkg/clouds/gcloud/config_validation_test.go @@ -4,6 +4,8 @@ package gcloud import ( + "strconv" + "strings" "testing" . "github.com/onsi/gomega" @@ -88,3 +90,73 @@ func TestPostgresGcpCloudsqlConfig_ProxyAndNetworkHelpers(t *testing.T) { Expect((&PostgresGcpCloudsqlConfig{PrivateNetwork: lo.ToPtr("projects/p/global/networks/vpc")}).UsesPrivateIpProxy()).To(BeFalse()) Expect((&PostgresGcpCloudsqlConfig{PublicIpEnabled: lo.ToPtr(false)}).UsesPrivateIpProxy()).To(BeTrue()) } + +// Cloud KMS bills every ACTIVE key version (ENABLED, DISABLED and +// DESTROY_SCHEDULED; only DESTROYED is free) and rotation never re-encrypts +// existing ciphertext, so every version a key mints stays billed for the life +// of the key. With one provisioned key per stack, the rotation period is a +// compounding cost multiplier rather than a cosmetic setting. +func TestSecretsProviderConfig_EffectiveKeyRotationPeriod(t *testing.T) { + RegisterTestingT(t) + + Expect((&SecretsProviderConfig{}).EffectiveKeyRotationPeriod()).To(Equal(DefaultKeyRotationPeriod)) + Expect((&SecretsProviderConfig{KeyRotationPeriod: "31536000s"}).EffectiveKeyRotationPeriod()). + To(Equal("31536000s"), "an explicit value must win over the default") +} + +func TestDefaultKeyRotationPeriodIsSane(t *testing.T) { + RegisterTestingT(t) + + // Pin the value, not just its properties: asserting only "effective == + // Default" passes for any constant, including the 100000s (27.8h) typo + // this default replaced. + Expect(DefaultKeyRotationPeriod).To(Equal("7776000s"), "90 days") + + secs, err := strconv.Atoi(strings.TrimSuffix(DefaultKeyRotationPeriod, "s")) + Expect(err).To(BeNil()) + Expect(secs).To(BeNumerically(">=", MinKeyRotationPeriodSeconds)) + Expect(secs).To(BeNumerically(">", GcpMinKeyRotationPeriodSeconds), + "must be well clear of GCP's 24h minimum") + Expect(secs).To(BeNumerically("<=", GcpMaxKeyRotationPeriodSeconds)) +} + +func TestSecretsProviderConfig_Validate(t *testing.T) { + tests := []struct { + name string + cfg SecretsProviderConfig + errSubstr string + }{ + {name: "unset means default", cfg: SecretsProviderConfig{Provision: true}}, + {name: "90 days", cfg: SecretsProviderConfig{Provision: true, KeyRotationPeriod: "7776000s"}}, + {name: "exactly the 30-day floor", cfg: SecretsProviderConfig{Provision: true, KeyRotationPeriod: "2592000s"}}, + // keyRotationPeriod is meaningless for a BYO key referenced by keyName, + // so a stale value must not block those consumers. + {name: "provision disabled ignores the period", cfg: SecretsProviderConfig{Provision: false, KeyRotationPeriod: "100000s"}}, + {name: "below policy floor", cfg: SecretsProviderConfig{Provision: true, KeyRotationPeriod: "100000s"}, errSubstr: "30 day"}, + {name: "one second under the floor", cfg: SecretsProviderConfig{Provision: true, KeyRotationPeriod: "2591999s"}, errSubstr: "30 day"}, + {name: "policy floor waivable", cfg: SecretsProviderConfig{Provision: true, KeyRotationPeriod: "604800s", AllowShortKeyRotation: true}}, + // The opt-out waives the POLICY floor only. GCP's own bounds stay hard: + // breaching them fails at the KMS API after the KeyRing already exists, + // and a KeyRing can never be deleted. + {name: "gcp floor NOT waivable", cfg: SecretsProviderConfig{Provision: true, KeyRotationPeriod: "3600s", AllowShortKeyRotation: true}, errSubstr: "at least 86400"}, + {name: "zero NOT waivable", cfg: SecretsProviderConfig{Provision: true, KeyRotationPeriod: "0s", AllowShortKeyRotation: true}, errSubstr: "at least 86400"}, + {name: "negative NOT waivable", cfg: SecretsProviderConfig{Provision: true, KeyRotationPeriod: "-100s", AllowShortKeyRotation: true}, errSubstr: "at least 86400"}, + {name: "above gcp maximum", cfg: SecretsProviderConfig{Provision: true, KeyRotationPeriod: "3153600001s", AllowShortKeyRotation: true}, errSubstr: "at most"}, + {name: "missing seconds suffix", cfg: SecretsProviderConfig{Provision: true, KeyRotationPeriod: "7776000"}, errSubstr: "'s' suffix"}, + {name: "not a number but ends in s", cfg: SecretsProviderConfig{Provision: true, KeyRotationPeriod: "ninetydays"}, errSubstr: "whole number of seconds"}, + {name: "duration shorthand", cfg: SecretsProviderConfig{Provision: true, KeyRotationPeriod: "90d"}, errSubstr: "'s' suffix"}, + {name: "malformed still fails under the opt-out", cfg: SecretsProviderConfig{Provision: true, KeyRotationPeriod: "90d", AllowShortKeyRotation: true}, errSubstr: "'s' suffix"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + RegisterTestingT(t) + err := tt.cfg.Validate() + if tt.errSubstr == "" { + Expect(err).To(BeNil()) + return + } + Expect(err).NotTo(BeNil(), "expected %q to be rejected", tt.cfg.KeyRotationPeriod) + Expect(err.Error()).To(ContainSubstring(tt.errSubstr)) + }) + } +} diff --git a/pkg/clouds/pulumi/gcp/kms_key.go b/pkg/clouds/pulumi/gcp/kms_key.go index 1433a11d..1f057beb 100644 --- a/pkg/clouds/pulumi/gcp/kms_key.go +++ b/pkg/clouds/pulumi/gcp/kms_key.go @@ -12,7 +12,6 @@ import ( "google.golang.org/api/serviceusage/v1" "github.com/pkg/errors" - "github.com/samber/lo" "github.com/pulumi/pulumi-gcp/sdk/v8/go/gcp/kms" sdk "github.com/pulumi/pulumi/sdk/v3/go/pulumi" @@ -29,6 +28,14 @@ func KmsKeySecretsProvider(ctx *sdk.Context, stack api.Stack, input api.Resource return nil, errors.Errorf("failed to convert KmsKeyInput for %q", input.Descriptor.Type) } + // Validate before any side effect. Enabling service APIs mutates the + // project, and a KeyRing can never be deleted in GCP (destroying the Pulumi + // resource only drops it from state), so failing later would leave a + // permanent, un-recreatable-by-name KeyRing behind for a mere typo. + if err := kmsInput.Validate(); err != nil { + return nil, err + } + if err := enableServicesAPI(ctx.Context(), input.Descriptor.Config.Config, fmt.Sprintf("projects/%s/services/serviceusage.googleapis.com", kmsInput.ProjectId)); err != nil { _, _ = os.Stderr.WriteString(color.RedFmt("service usage API seems to be disabled on project %q, "+ @@ -54,7 +61,7 @@ func KmsKeySecretsProvider(ctx *sdk.Context, stack api.Stack, input api.Resource } // Create a new CryptoKey associated with the KeyRing. - rotationPeriod := lo.If(kmsInput.KeyRotationPeriod == "", "100000s").Else(kmsInput.KeyRotationPeriod) + rotationPeriod := kmsInput.EffectiveKeyRotationPeriod() key, err := kms.NewCryptoKey(ctx, input.ToResName(input.Descriptor.Name), &kms.CryptoKeyArgs{ Name: sdk.String(input.Descriptor.Name), diff --git a/pkg/clouds/pulumi/gcp/kms_key_test.go b/pkg/clouds/pulumi/gcp/kms_key_test.go new file mode 100644 index 00000000..c1c10863 --- /dev/null +++ b/pkg/clouds/pulumi/gcp/kms_key_test.go @@ -0,0 +1,181 @@ +// SPDX-License-Identifier: MIT +// Copyright (c) Simple Container + +package gcp + +import ( + "sync" + "testing" + + . "github.com/onsi/gomega" + + "github.com/pulumi/pulumi/sdk/v3/go/common/resource" + "github.com/pulumi/pulumi/sdk/v3/go/pulumi" + + gcpsdk "github.com/pulumi/pulumi-gcp/sdk/v8/go/gcp" + + "github.com/simple-container-com/api/pkg/api" + "github.com/simple-container-com/api/pkg/clouds/gcloud" + pApi "github.com/simple-container-com/api/pkg/clouds/pulumi/api" +) + +// createBasicProvisionParams leaves Provider nil, which panics inside +// kms.NewKeyRing. Build a real provider under the mocks instead. +func kmsProvisionParams(ctx *pulumi.Context) (pApi.ProvisionParams, error) { + params := createBasicProvisionParams() + prov, err := gcpsdk.NewProvider(ctx, "test-gcp", &gcpsdk.ProviderArgs{ + Project: pulumi.String("test-project"), + }) + if err != nil { + return params, err + } + params.Provider = prov + return params, nil +} + +// kmsMocks records what the provisioner actually asked Pulumi to create, so the +// tests can assert on the value that reaches the CryptoKey rather than on the +// helper that computes it. Without this, reverting the default in +// KmsKeySecretsProvider leaves every config-level test green. +type kmsMocks struct { + mu sync.Mutex + created []pulumi.MockResourceArgs +} + +func newKmsMocks() *kmsMocks { return &kmsMocks{} } + +func (m *kmsMocks) NewResource(args pulumi.MockResourceArgs) (string, resource.PropertyMap, error) { + m.mu.Lock() + defer m.mu.Unlock() + m.created = append(m.created, args) + return args.Name + "-id", args.Inputs, nil +} + +func (m *kmsMocks) Call(args pulumi.MockCallArgs) (resource.PropertyMap, error) { + return args.Args, nil +} + +func (m *kmsMocks) countOf(typeToken string) int { + m.mu.Lock() + defer m.mu.Unlock() + n := 0 + for _, a := range m.created { + if a.TypeToken == typeToken { + n++ + } + } + return n +} + +func (m *kmsMocks) rotationPeriod() string { + m.mu.Lock() + defer m.mu.Unlock() + for _, a := range m.created { + if a.TypeToken == "gcp:kms/cryptoKey:CryptoKey" { + if v, ok := a.Inputs["rotationPeriod"]; ok && v.IsString() { + return v.StringValue() + } + } + } + return "" +} + +func kmsResourceInput(cfg *gcloud.SecretsProviderConfig) api.ResourceInput { + return api.ResourceInput{ + Descriptor: &api.ResourceDescriptor{ + Type: gcloud.SecretsProviderTypeGcpKms, + Name: "test-stack-sc", + Config: api.Config{Config: cfg}, + }, + StackParams: &api.StackParams{Environment: "test"}, + } +} + +func baseKmsConfig() *gcloud.SecretsProviderConfig { + return &gcloud.SecretsProviderConfig{ + Provision: true, + KeyName: "test-key", + KeyLocation: "global", + Credentials: gcloud.Credentials{ + ServiceAccountConfig: gcloud.ServiceAccountConfig{ProjectId: "test-project"}, + }, + } +} + +// The default is the whole point of the change: an unset keyRotationPeriod must +// reach the CryptoKey as 90 days, not the previous 100000s (27.8h). +func TestKmsKeySecretsProvider_AppliesDefaultRotationPeriod(t *testing.T) { + RegisterTestingT(t) + + setGlobalServicesAPIClient(newMockServicesAPIClient()) + defer resetGlobalServicesAPIClient() + + mocks := newKmsMocks() + err := pulumi.RunErr(func(ctx *pulumi.Context) error { + params, err := kmsProvisionParams(ctx) + if err != nil { + return err + } + _, err = KmsKeySecretsProvider(ctx, api.Stack{}, kmsResourceInput(baseKmsConfig()), params) + return err + }, pulumi.WithMocks("project", "stack", mocks)) + + Expect(err).To(BeNil()) + Expect(mocks.rotationPeriod()).To(Equal(gcloud.DefaultKeyRotationPeriod), + "unset keyRotationPeriod must provision the 90-day default") + Expect(mocks.rotationPeriod()).NotTo(Equal("100000s"), + "the 27.8h default is what caused the key-version cost blowup") +} + +func TestKmsKeySecretsProvider_AppliesExplicitRotationPeriod(t *testing.T) { + RegisterTestingT(t) + + setGlobalServicesAPIClient(newMockServicesAPIClient()) + defer resetGlobalServicesAPIClient() + + cfg := baseKmsConfig() + cfg.KeyRotationPeriod = "31536000s" + + mocks := newKmsMocks() + err := pulumi.RunErr(func(ctx *pulumi.Context) error { + params, err := kmsProvisionParams(ctx) + if err != nil { + return err + } + _, err = KmsKeySecretsProvider(ctx, api.Stack{}, kmsResourceInput(cfg), params) + return err + }, pulumi.WithMocks("project", "stack", mocks)) + + Expect(err).To(BeNil()) + Expect(mocks.rotationPeriod()).To(Equal("31536000s")) +} + +// Ordering matters more than the message: a GCP KeyRing can never be deleted +// (destroying the Pulumi resource only drops it from state), so a rejected +// config must not leave one behind. Asserting zero created resources pins that, +// where asserting only the error would pass even if validation ran last. +func TestKmsKeySecretsProvider_RejectsBadPeriodBeforeCreatingAnything(t *testing.T) { + RegisterTestingT(t) + + setGlobalServicesAPIClient(newMockServicesAPIClient()) + defer resetGlobalServicesAPIClient() + + cfg := baseKmsConfig() + cfg.KeyRotationPeriod = "100000s" // the old default: below the 30-day floor + + mocks := newKmsMocks() + err := pulumi.RunErr(func(ctx *pulumi.Context) error { + params, err := kmsProvisionParams(ctx) + if err != nil { + return err + } + _, err = KmsKeySecretsProvider(ctx, api.Stack{}, kmsResourceInput(cfg), params) + return err + }, pulumi.WithMocks("project", "stack", mocks)) + + Expect(err).NotTo(BeNil()) + Expect(err.Error()).To(ContainSubstring("30 day")) + Expect(mocks.countOf("gcp:kms/keyRing:KeyRing")).To(Equal(0), + "a KeyRing can never be deleted; validation must run before it is created") + Expect(mocks.countOf("gcp:kms/cryptoKey:CryptoKey")).To(Equal(0)) +}