From 0a8208cb9fe52ea2978612e68a8ba131d39b24d1 Mon Sep 17 00:00:00 2001 From: Dmitrii Creed Date: Sun, 26 Jul 2026 23:01:52 +0300 Subject: [PATCH 1/2] 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 e329e018d0d0792659c0b5e0aa31f9de0440b6de Mon Sep 17 00:00:00 2001 From: Dmitrii Creed Date: Tue, 28 Jul 2026 19:34:40 +0300 Subject: [PATCH 2/2] 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{