From 6df6001ca6067792b61ba56194885d03c7c3a58a Mon Sep 17 00:00:00 2001 From: Dmitrii Creed Date: Sun, 26 Jul 2026 23:01:52 +0300 Subject: [PATCH 1/3] fix(gcp): default KMS key rotation to 90 days and reject sub-30-day periods The provisioned secrets-provider key defaulted to a 100000s rotation period when keyRotationPeriod was unset. That is 27.8 hours, which reads like a typo for 10000000s (~116 days) and sits just 16% above GCP's own 86400s floor, so it passes provider validation silently. It is not a cosmetic default. 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. Since a key is provisioned per stack, a daily rotation adds a billed version per stack per day indefinitely, and the resulting cost compounds rather than plateauing. Observed in a real fleet: thousands of accrued versions across a few dozen stacks, dominating the project's KMS spend and growing every month. Changes: - default rotation period 100000s -> 7776000s (90 days) - ValidateKeyRotationPeriod rejects explicit periods under 30 days, plus malformed values (no 's' suffix, non-integer, duration shorthand), so the next such typo fails at provisioning time instead of becoming an unattributed bill months later - EffectiveKeyRotationPeriod centralises the default so the provisioner and any future consumer cannot disagree Existing keys are unaffected: rotation period is an in-place property and changing the default does not alter already-provisioned keys or their versions. Operators who deliberately want faster rotation can still set keyRotationPeriod explicitly, down to the 30-day floor. Signed-off-by: Dmitrii Creed --- pkg/clouds/gcloud/auth.go | 56 +++++++++++++++++++ pkg/clouds/gcloud/kms_rotation_test.go | 74 ++++++++++++++++++++++++++ pkg/clouds/pulumi/gcp/kms_key.go | 6 ++- 3 files changed, 134 insertions(+), 2 deletions(-) create mode 100644 pkg/clouds/gcloud/kms_rotation_test.go diff --git a/pkg/clouds/gcloud/auth.go b/pkg/clouds/gcloud/auth.go index 161c87e1..5cd249df 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,27 @@ 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 +) + type ServiceAccountConfig struct { ProjectId string `json:"projectId" yaml:"projectId"` } @@ -80,6 +105,37 @@ 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 +} + +// ValidateKeyRotationPeriod checks an explicitly configured rotation period. +// Only applicable when provision=true; an empty value is valid and means the +// default applies. +func (r *SecretsProviderConfig) ValidateKeyRotationPeriod() error { + if 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) + } + if secs < MinKeyRotationPeriodSeconds { + return errors.Errorf("keyRotationPeriod %q is %d seconds, below the minimum of %d (30 days): every rotation mints a key version that Cloud KMS bills for the lifetime of the key, so short periods accrue cost indefinitely", + raw, secs, MinKeyRotationPeriodSeconds) + } + return nil +} + func (r *Credentials) ProviderType() string { return ProviderType } diff --git a/pkg/clouds/gcloud/kms_rotation_test.go b/pkg/clouds/gcloud/kms_rotation_test.go new file mode 100644 index 00000000..d35f1ccb --- /dev/null +++ b/pkg/clouds/gcloud/kms_rotation_test.go @@ -0,0 +1,74 @@ +// SPDX-License-Identifier: MIT +// Copyright (c) Simple Container + +package gcloud + +import ( + "strconv" + "strings" + "testing" + + . "github.com/onsi/gomega" +) + +// The rotation period is a cost multiplier, not a cosmetic setting: Cloud KMS +// bills every active key version, rotation never re-encrypts existing +// ciphertext (so old versions stay load-bearing and billed), and provisioning +// creates one key per stack. A too-short period therefore accrues billed +// versions for the lifetime of the key. These tests pin the default and the +// floor so a regression shows up here rather than on an invoice. +func TestSecretsProviderConfig_EffectiveKeyRotationPeriod(t *testing.T) { + RegisterTestingT(t) + + Expect((&SecretsProviderConfig{}).EffectiveKeyRotationPeriod()).To(Equal(DefaultKeyRotationPeriod), + "unset rotation period must fall back to the default") + Expect((&SecretsProviderConfig{KeyRotationPeriod: "31536000s"}).EffectiveKeyRotationPeriod()). + To(Equal("31536000s"), "an explicit value must win over the default") +} + +func TestDefaultKeyRotationPeriodIsSane(t *testing.T) { + RegisterTestingT(t) + + secs, err := strconv.Atoi(strings.TrimSuffix(DefaultKeyRotationPeriod, "s")) + Expect(err).To(BeNil(), "default must be a parseable seconds value") + Expect(secs).To(BeNumerically(">=", MinKeyRotationPeriodSeconds), + "the default must itself satisfy the validation floor") + // Guards against reintroducing a value like 100000s (27.8 hours), which is + // above GCP's own 86400s floor and so passes provider validation while + // minting a billed key version roughly every day. + Expect(secs).To(BeNumerically(">", 86400), + "default must be well clear of GCP's 1-day minimum") +} + +func TestSecretsProviderConfig_ValidateKeyRotationPeriod(t *testing.T) { + tests := []struct { + name string + period string + errSubstr string + }{ + {name: "unset is valid and means default", period: ""}, + {name: "90 days", period: "7776000s"}, + {name: "exactly the 30-day floor", period: "2592000s"}, + {name: "one year", period: "31536000s"}, + {name: "below floor: 27.8 hours", period: "100000s", errSubstr: "below the minimum"}, + {name: "below floor: GCP minimum of one day", period: "86400s", errSubstr: "below the minimum"}, + {name: "below floor: one second under", period: "2591999s", errSubstr: "below the minimum"}, + {name: "missing seconds suffix", period: "7776000", errSubstr: "'s' suffix"}, + {name: "not a number", period: "ninetydays", errSubstr: "'s' suffix"}, + {name: "fractional seconds", period: "2592000.5s", errSubstr: "whole number of seconds"}, + {name: "duration shorthand is not accepted", period: "90d", errSubstr: "'s' suffix"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + RegisterTestingT(t) + err := (&SecretsProviderConfig{Provision: true, KeyRotationPeriod: tt.period}). + ValidateKeyRotationPeriod() + if tt.errSubstr == "" { + Expect(err).To(BeNil()) + return + } + Expect(err).NotTo(BeNil(), "expected %q to be rejected", tt.period) + 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..9627766e 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" @@ -54,7 +53,10 @@ 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) + if err := kmsInput.ValidateKeyRotationPeriod(); err != nil { + return nil, err + } + rotationPeriod := kmsInput.EffectiveKeyRotationPeriod() key, err := kms.NewCryptoKey(ctx, input.ToResName(input.Descriptor.Name), &kms.CryptoKeyArgs{ Name: sdk.String(input.Descriptor.Name), From a4e1b3a318ffe99d3f2bf76290fc9c4f363f7767 Mon Sep 17 00:00:00 2001 From: Dmitrii Creed Date: Tue, 28 Jul 2026 19:34:40 +0300 Subject: [PATCH 2/3] fix(gcp): validate rotation period before any side effect; add opt-out Review follow-ups on the KMS rotation default: - Validation ran AFTER enableServicesAPI (which mutates the project) and AFTER kms.NewKeyRing. A GCP KeyRing can never be deleted (destroying the Pulumi resource only drops it from state), so a mistyped rotation period left a permanent, un-recreatable-by-name KeyRing behind. Validation now runs immediately after the config type assertion, before any side effect. - A hard sub-30-day floor is a breaking change for consumers with a deliberate short rotation (7-day rotation is a common compliance setting), and cost is not a good reason to break someone's key-rotation policy. Added allowShortKeyRotation to keep the floor a typo-catcher rather than an imposed policy; the error message names the escape hatch, and the opt-out does not relax the malformed-value checks. - One test case could not fail for the reason its name implied: "ninetydays" ends in 's', so it took the numeric branch whose message also contains "'s' suffix". It now asserts the numeric message, keeping the two branches distinguishable, plus cases for a bare suffix and a negative value. Signed-off-by: Dmitrii Creed --- pkg/clouds/gcloud/auth.go | 10 +++++++-- pkg/clouds/gcloud/kms_rotation_test.go | 31 +++++++++++++++++++++++++- pkg/clouds/pulumi/gcp/kms_key.go | 11 ++++++--- 3 files changed, 46 insertions(+), 6 deletions(-) diff --git a/pkg/clouds/gcloud/auth.go b/pkg/clouds/gcloud/auth.go index 5cd249df..9a154f42 100644 --- a/pkg/clouds/gcloud/auth.go +++ b/pkg/clouds/gcloud/auth.go @@ -84,6 +84,12 @@ type SecretsProviderConfig struct { KeyLocation string `json:"keyLocation" yaml:"keyLocation"` // only applicable when provision=true KeyRotationPeriod string `json:"keyRotationPeriod" yaml:"keyRotationPeriod"` + // 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" yaml:"allowShortKeyRotation"` // whether to provision key Provision bool `json:"provision" yaml:"provision"` @@ -129,8 +135,8 @@ func (r *SecretsProviderConfig) ValidateKeyRotationPeriod() error { if err != nil { return errors.Errorf("keyRotationPeriod %q must be a whole number of seconds with an 's' suffix, e.g. %q", raw, DefaultKeyRotationPeriod) } - if secs < MinKeyRotationPeriodSeconds { - return errors.Errorf("keyRotationPeriod %q is %d seconds, below the minimum of %d (30 days): every rotation mints a key version that Cloud KMS bills for the lifetime of the key, so short periods accrue cost indefinitely", + if secs < MinKeyRotationPeriodSeconds && !r.AllowShortKeyRotation { + return errors.Errorf("keyRotationPeriod %q is %d seconds, below the minimum of %d (30 days): every rotation mints a key version that Cloud KMS bills for the lifetime of the key, so short periods accrue cost indefinitely. Set allowShortKeyRotation: true if the faster rotation is deliberate", raw, secs, MinKeyRotationPeriodSeconds) } return nil diff --git a/pkg/clouds/gcloud/kms_rotation_test.go b/pkg/clouds/gcloud/kms_rotation_test.go index d35f1ccb..76c40c58 100644 --- a/pkg/clouds/gcloud/kms_rotation_test.go +++ b/pkg/clouds/gcloud/kms_rotation_test.go @@ -54,7 +54,13 @@ func TestSecretsProviderConfig_ValidateKeyRotationPeriod(t *testing.T) { {name: "below floor: GCP minimum of one day", period: "86400s", errSubstr: "below the minimum"}, {name: "below floor: one second under", period: "2591999s", errSubstr: "below the minimum"}, {name: "missing seconds suffix", period: "7776000", errSubstr: "'s' suffix"}, - {name: "not a number", period: "ninetydays", errSubstr: "'s' suffix"}, + // "ninetydays" ends in 's', so it reaches the numeric branch, not the + // suffix branch. Asserting the numeric message keeps the two branches + // distinguishable — otherwise a regression that collapsed them would + // still pass. + {name: "not a number but ends in s", period: "ninetydays", errSubstr: "whole number of seconds"}, + {name: "suffix only", period: "s", errSubstr: "whole number of seconds"}, + {name: "negative", period: "-100s", errSubstr: "below the minimum"}, {name: "fractional seconds", period: "2592000.5s", errSubstr: "whole number of seconds"}, {name: "duration shorthand is not accepted", period: "90d", errSubstr: "'s' suffix"}, } @@ -72,3 +78,26 @@ func TestSecretsProviderConfig_ValidateKeyRotationPeriod(t *testing.T) { }) } } + +// A deliberate sub-30-day rotation must remain expressible: this is a shared +// library, and a compliance requirement for faster rotation is legitimate. The +// opt-out is what keeps the floor a typo-catcher rather than a policy imposed on +// every consumer. +func TestSecretsProviderConfig_AllowShortKeyRotationOptsOutOfTheFloor(t *testing.T) { + RegisterTestingT(t) + + short := &SecretsProviderConfig{Provision: true, KeyRotationPeriod: "604800s"} // 7 days + Expect(short.ValidateKeyRotationPeriod()).NotTo(BeNil(), + "a short period must fail by default so typos surface") + Expect(short.ValidateKeyRotationPeriod().Error()).To(ContainSubstring("allowShortKeyRotation"), + "the error must name the escape hatch") + + deliberate := &SecretsProviderConfig{Provision: true, KeyRotationPeriod: "604800s", AllowShortKeyRotation: true} + Expect(deliberate.ValidateKeyRotationPeriod()).To(BeNil(), + "an explicit opt-out must be honoured") + + // The opt-out must not disable the malformed-value checks: it is about the + // floor, not about accepting garbage. + malformed := &SecretsProviderConfig{Provision: true, KeyRotationPeriod: "90d", AllowShortKeyRotation: true} + Expect(malformed.ValidateKeyRotationPeriod()).NotTo(BeNil()) +} diff --git a/pkg/clouds/pulumi/gcp/kms_key.go b/pkg/clouds/pulumi/gcp/kms_key.go index 9627766e..c9bdf2a6 100644 --- a/pkg/clouds/pulumi/gcp/kms_key.go +++ b/pkg/clouds/pulumi/gcp/kms_key.go @@ -28,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.ValidateKeyRotationPeriod(); 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, "+ @@ -53,9 +61,6 @@ func KmsKeySecretsProvider(ctx *sdk.Context, stack api.Stack, input api.Resource } // Create a new CryptoKey associated with the KeyRing. - if err := kmsInput.ValidateKeyRotationPeriod(); err != nil { - return nil, err - } rotationPeriod := kmsInput.EffectiveKeyRotationPeriod() key, err := kms.NewCryptoKey(ctx, input.ToResName(input.Descriptor.Name), &kms.CryptoKeyArgs{ From 92cb3a4db332c9d6d6b0720c0848cc846b299ba3 Mon Sep 17 00:00:00 2001 From: Dmitrii Creed Date: Sun, 9 Aug 2026 11:23:27 +0400 Subject: [PATCH 3/3] fix(gcp): enforce GCP's own rotation bounds, gate on provision, cover the provisioner Review follow-ups. The opt-out was too broad. AllowShortKeyRotation short-circuited the whole lower-bound check, so 0s, -100s and 3600s all passed SC validation and were handed to kms.NewCryptoKey. GCP rejects anything under 24h, but only after enableServicesAPI and kms.NewKeyRing have run, which is exactly the late-failure this change exists to prevent (a KeyRing can never be deleted). GCP's documented bounds, 86400s and 3153600000s, are now enforced unconditionally; the opt-out waives only the 30-day policy band above them. keyRotationPeriod and allowShortKeyRotation lacked omitempty, so the generated JSON schema listed them under 'required'. Regenerating with cmd/schema-gen confirms the fix: allowShortKeyRotation is now an optional property and keyRotationPeriod has left 'required', which it should never have been given that unset is the documented happy path. Validation is now gated on provision: a BYO key referenced by keyName ignores keyRotationPeriod entirely, so a stale value must not block those consumers. It also runs from ReadSecretsProviderConfig, not only from 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 surface first during a DR rebuild. Renamed to Validate() to match PostgresGcpCloudsqlConfig and ExternalEgressIpConfig, with the table moved into config_validation_test.go where the sibling validators are tested. This is a public API surface, so the bespoke name would have been permanent. Added provisioner tests. The previous tests only exercised the config helpers: reverting the default in KmsKeySecretsProvider left all of them green, which is the one line the change exists for. The new tests drive the provisioner under pulumi mocks and assert the value that actually reaches the CryptoKey, plus zero created resources on a rejected config, which pins the ordering rather than just the error. Both mutations now fail. Signed-off-by: Dmitrii Creed --- docs/schemas/gcp/secretsproviderconfig.json | 4 +- pkg/clouds/gcloud/auth.go | 53 +++++- pkg/clouds/gcloud/config_validation_test.go | 72 ++++++++ pkg/clouds/gcloud/kms_rotation_test.go | 103 ----------- pkg/clouds/pulumi/gcp/kms_key.go | 2 +- pkg/clouds/pulumi/gcp/kms_key_test.go | 181 ++++++++++++++++++++ 6 files changed, 301 insertions(+), 114 deletions(-) delete mode 100644 pkg/clouds/gcloud/kms_rotation_test.go create mode 100644 pkg/clouds/pulumi/gcp/kms_key_test.go 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 9a154f42..e68e1ef0 100644 --- a/pkg/clouds/gcloud/auth.go +++ b/pkg/clouds/gcloud/auth.go @@ -41,6 +41,14 @@ const ( // 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 { @@ -83,13 +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" yaml:"allowShortKeyRotation"` + AllowShortKeyRotation bool `json:"allowShortKeyRotation,omitempty" yaml:"allowShortKeyRotation,omitempty"` // whether to provision key Provision bool `json:"provision" yaml:"provision"` @@ -120,11 +128,15 @@ func (r *SecretsProviderConfig) EffectiveKeyRotationPeriod() string { return r.KeyRotationPeriod } -// ValidateKeyRotationPeriod checks an explicitly configured rotation period. -// Only applicable when provision=true; an empty value is valid and means the -// default applies. -func (r *SecretsProviderConfig) ValidateKeyRotationPeriod() error { - if 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 @@ -135,8 +147,18 @@ func (r *SecretsProviderConfig) ValidateKeyRotationPeriod() error { 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 is %d seconds, below the minimum of %d (30 days): every rotation mints a key version that Cloud KMS bills for the lifetime of the key, so short periods accrue cost indefinitely. Set allowShortKeyRotation: true if the faster rotation is deliberate", + return errors.Errorf("keyRotationPeriod %q (%ds) is below the %ds (30 day) minimum; set allowShortKeyRotation: true to override", raw, secs, MinKeyRotationPeriodSeconds) } return nil @@ -171,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/gcloud/kms_rotation_test.go b/pkg/clouds/gcloud/kms_rotation_test.go deleted file mode 100644 index 76c40c58..00000000 --- a/pkg/clouds/gcloud/kms_rotation_test.go +++ /dev/null @@ -1,103 +0,0 @@ -// SPDX-License-Identifier: MIT -// Copyright (c) Simple Container - -package gcloud - -import ( - "strconv" - "strings" - "testing" - - . "github.com/onsi/gomega" -) - -// The rotation period is a cost multiplier, not a cosmetic setting: Cloud KMS -// bills every active key version, rotation never re-encrypts existing -// ciphertext (so old versions stay load-bearing and billed), and provisioning -// creates one key per stack. A too-short period therefore accrues billed -// versions for the lifetime of the key. These tests pin the default and the -// floor so a regression shows up here rather than on an invoice. -func TestSecretsProviderConfig_EffectiveKeyRotationPeriod(t *testing.T) { - RegisterTestingT(t) - - Expect((&SecretsProviderConfig{}).EffectiveKeyRotationPeriod()).To(Equal(DefaultKeyRotationPeriod), - "unset rotation period must fall back to the default") - Expect((&SecretsProviderConfig{KeyRotationPeriod: "31536000s"}).EffectiveKeyRotationPeriod()). - To(Equal("31536000s"), "an explicit value must win over the default") -} - -func TestDefaultKeyRotationPeriodIsSane(t *testing.T) { - RegisterTestingT(t) - - secs, err := strconv.Atoi(strings.TrimSuffix(DefaultKeyRotationPeriod, "s")) - Expect(err).To(BeNil(), "default must be a parseable seconds value") - Expect(secs).To(BeNumerically(">=", MinKeyRotationPeriodSeconds), - "the default must itself satisfy the validation floor") - // Guards against reintroducing a value like 100000s (27.8 hours), which is - // above GCP's own 86400s floor and so passes provider validation while - // minting a billed key version roughly every day. - Expect(secs).To(BeNumerically(">", 86400), - "default must be well clear of GCP's 1-day minimum") -} - -func TestSecretsProviderConfig_ValidateKeyRotationPeriod(t *testing.T) { - tests := []struct { - name string - period string - errSubstr string - }{ - {name: "unset is valid and means default", period: ""}, - {name: "90 days", period: "7776000s"}, - {name: "exactly the 30-day floor", period: "2592000s"}, - {name: "one year", period: "31536000s"}, - {name: "below floor: 27.8 hours", period: "100000s", errSubstr: "below the minimum"}, - {name: "below floor: GCP minimum of one day", period: "86400s", errSubstr: "below the minimum"}, - {name: "below floor: one second under", period: "2591999s", errSubstr: "below the minimum"}, - {name: "missing seconds suffix", period: "7776000", errSubstr: "'s' suffix"}, - // "ninetydays" ends in 's', so it reaches the numeric branch, not the - // suffix branch. Asserting the numeric message keeps the two branches - // distinguishable — otherwise a regression that collapsed them would - // still pass. - {name: "not a number but ends in s", period: "ninetydays", errSubstr: "whole number of seconds"}, - {name: "suffix only", period: "s", errSubstr: "whole number of seconds"}, - {name: "negative", period: "-100s", errSubstr: "below the minimum"}, - {name: "fractional seconds", period: "2592000.5s", errSubstr: "whole number of seconds"}, - {name: "duration shorthand is not accepted", period: "90d", errSubstr: "'s' suffix"}, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - RegisterTestingT(t) - err := (&SecretsProviderConfig{Provision: true, KeyRotationPeriod: tt.period}). - ValidateKeyRotationPeriod() - if tt.errSubstr == "" { - Expect(err).To(BeNil()) - return - } - Expect(err).NotTo(BeNil(), "expected %q to be rejected", tt.period) - Expect(err.Error()).To(ContainSubstring(tt.errSubstr)) - }) - } -} - -// A deliberate sub-30-day rotation must remain expressible: this is a shared -// library, and a compliance requirement for faster rotation is legitimate. The -// opt-out is what keeps the floor a typo-catcher rather than a policy imposed on -// every consumer. -func TestSecretsProviderConfig_AllowShortKeyRotationOptsOutOfTheFloor(t *testing.T) { - RegisterTestingT(t) - - short := &SecretsProviderConfig{Provision: true, KeyRotationPeriod: "604800s"} // 7 days - Expect(short.ValidateKeyRotationPeriod()).NotTo(BeNil(), - "a short period must fail by default so typos surface") - Expect(short.ValidateKeyRotationPeriod().Error()).To(ContainSubstring("allowShortKeyRotation"), - "the error must name the escape hatch") - - deliberate := &SecretsProviderConfig{Provision: true, KeyRotationPeriod: "604800s", AllowShortKeyRotation: true} - Expect(deliberate.ValidateKeyRotationPeriod()).To(BeNil(), - "an explicit opt-out must be honoured") - - // The opt-out must not disable the malformed-value checks: it is about the - // floor, not about accepting garbage. - malformed := &SecretsProviderConfig{Provision: true, KeyRotationPeriod: "90d", AllowShortKeyRotation: true} - Expect(malformed.ValidateKeyRotationPeriod()).NotTo(BeNil()) -} diff --git a/pkg/clouds/pulumi/gcp/kms_key.go b/pkg/clouds/pulumi/gcp/kms_key.go index c9bdf2a6..1f057beb 100644 --- a/pkg/clouds/pulumi/gcp/kms_key.go +++ b/pkg/clouds/pulumi/gcp/kms_key.go @@ -32,7 +32,7 @@ func KmsKeySecretsProvider(ctx *sdk.Context, stack api.Stack, input api.Resource // 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.ValidateKeyRotationPeriod(); err != nil { + if err := kmsInput.Validate(); err != nil { return nil, err } 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)) +}