fix(gcp): default KMS key rotation to 90 days and reject sub-30-day periods

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"

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
 }

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))
+		})
+	}
+}

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),