fix(gcp): default KMS key rotation to 90 days and reject sub-30-day periods - #370
Open
Cre-eD wants to merge 2 commits into
Open
fix(gcp): default KMS key rotation to 90 days and reject sub-30-day periods#370Cre-eD wants to merge 2 commits into
Cre-eD wants to merge 2 commits into
Conversation
…eriods 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 <creeed22@gmail.com>
Cre-eD
requested review from
Laboratory,
smecsia and
universe-ops
as code owners
July 26, 2026 20:02
Semgrep Scan ResultsRepository:
Scanned at 2026-07-28 16:35 UTC |
📊 Statement coverageMeasured on the documented included set (see
Baseline: |
Security Scan ResultsRepository:
Scanned at 2026-07-28 16:36 UTC |
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 <creeed22@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
When
keyRotationPeriodis unset, the provisioned GCP secrets-provider key defaults to100000s— 27.8 hours. That reads like a typo for10000000s(~116 days), and because it sits just 16% above GCP's own86400sfloor it passes provider validation silently.It is not a cosmetic default:
The cost therefore compounds rather than plateauing. Observed on a real fleet: thousands of accrued key versions across a few dozen stacks, dominating that project's KMS spend and growing every month. Nothing surfaces it, because each individual version costs cents and the resource looks healthy.
Changes
100000s→7776000s(90 days).ValidateKeyRotationPeriod()rejects explicit periods under 30 days, plus malformed values (missingssuffix, non-integer, duration shorthand like90d). The next such typo fails at provisioning time instead of becoming an unattributed bill months later. GCP's own floor of 1 day is deliberately not the bound here — a few hours or days is valid to GCP and still wrong for a per-stack provisioned key.EffectiveKeyRotationPeriod()centralises the default so the provisioner and any future consumer cannot disagree about it.Compatibility
Existing keys are unaffected — rotation period is an in-place property, and changing the default does not alter already-provisioned keys or their existing versions. Operators who deliberately want faster rotation can still set
keyRotationPeriod, down to the 30-day floor.Tests
pkg/clouds/gcloud/kms_rotation_test.go: default fallback and explicit-value precedence; a guard that the default itself satisfies the floor and stays well clear of GCP's 1-day minimum (so100000scannot be reintroduced); and a table covering the floor boundary (2592000saccepted,2591999srejected),100000s,86400s, and each malformed form. Fullpkg/clouds/gcloudsuite passes with no regressions.