feat(user_key_pair): support Snowflake named key pairs, with rotation - #67
Merged
Conversation
Snowflake now supports named key pairs on a user, registered with ALTER USER ... ADD KEY PAIR. Unlike the legacy rsa_public_key and rsa_public_key_2 user properties, a user can hold up to 10 of them and each carries its own role restriction, expiration, disabled flag, and comment. Add a UserKeyPair resource (user_key_pairs: in YAML, or a key_pairs: list on a user) addressed as <name>?user=<username>, with: - create via ALTER USER ... ADD KEY PAIR, drop via REMOVE KEY PAIR - disabled/comment changes via MODIFY KEY PAIR SET/UNSET - fetch from SHOW USER KEY PAIRS, with rotated-out tombstones (<name>_ROTATED_<epoch_ms>) excluded from both fetch and the account-wide list sweep so snowcap never offers to remove a prior key inside its grace period Snowflake never echoes a key pair's public key back -- SHOW returns only a fingerprint -- so the spec carries a fingerprint computed from the configured key (base64 SHA-256 of the DER, the same value openssl produces) and drift is detected on that. A changed public_key therefore plans ALTER USER ... ROTATE KEY PAIR, which keeps the prior key valid for its grace period. Because that delta names a fingerprint rather than a key, update_resource now hands handlers that need it the full desired state, and lifecycle handlers may return several statements for one change (a rotation plus a property change has no single-statement form). role_restriction and days_to_expiry cannot be altered in Snowflake: the first fails the plan with a message explaining the remove-and-re-add path, the second is config-authoritative since Snowflake reports only an absolute expires_at. Key pair operations run as the role that manages the user (USERADMIN by default), which needs ownership of the user or MODIFY PROGRAMMATIC AUTHENTICATION METHODS on it; that privilege is now registered for grants.
Three fixes from review of the named key pair support: - Two users can each have a key pair with the same name, but _merge_pointers identified named resources by (type, name) alone and raised DuplicateResourceException for the second one. Resources now report the fqn params that are part of their identity through a new fqn_params property (empty for everything but key pairs), which _merge_pointers folds into the key. Unlike fqn, it is safe to read before vars are resolved. - SHOW USER KEY PAIRS reports the legacy rsa_public_key and rsa_public_key_2 properties under the reserved PUBLIC_KEY_1 and PUBLIC_KEY_2 names. A sync sweep picked those up as unmanaged key pairs, and building remote state for one raised out of the spec's reserved-name check, aborting the plan. Both fetch and the sweep now skip rows a config cannot declare. - `owner` is not fetchable, so remote state carries the default rather than what config declared, and owner_for_change reads the before-owner on an update. A key pair configured with a non-default owner was therefore created as that role but rotated, modified, and removed as USERADMIN. The execution strategy now reads the declared owner. A fourth finding -- a disabled key pair reading back as enabled once it expires -- does not hold: Snowflake reports DISABLED when a key pair is both disabled and past its expiration. Documented at the mapping and covered by a test.
Statements that followed a rename in the same change still addressed the key pair by its old name. A rename is a drop and create today (the name is part of the urn), so nothing reaches it, but the ordering bug was one refactor away from mattering. Also cover the two-step plan-file workflow: the rotation SQL reads the public key from the change's desired state, so a plan written to JSON and applied later has to carry it.
…y key rotation Rotation already happened on a public_key change, but with no control over what it leaves behind and two gaps around it. ALTER USER ... ROTATE KEY PAIR keeps the prior key valid for 24 hours by default so in-flight clients can transition. That is the wrong default when the rotation is the response to a leaked private key, so key pairs now take expire_rotated_key_pair_after_hours: 0 revokes the prior key immediately, a larger value widens the window. It describes the next rotation rather than the state of the key pair, so setting it plans nothing on its own, and it is never rendered into ADD KEY PAIR, which has no such option. A plan containing a rotation now says in as many words that the prior key survives, under what name, and for how long. Two fixes from the security review of the previous commits: - A rotated-out key was identified by `rotated_to` OR by a name shaped like <name>_ROTATED_<epoch_ms>. The name is chosen by whoever registers the key pair, so anyone who could add a key pair could name one to look like a tombstone and have it skipped by fetch, by drift detection, and by the sync sweep that removes what config does not declare. Only `rotated_to`, which Snowflake sets, identifies a rotated-out key now; the name shape is still refused in config so a config cannot claim a name out of the namespace Snowflake generates into. - An expiration added to (or dropped from) an existing key pair was silently ignored, because days_to_expiry is not fetchable. Snowflake reports whether a key pair expires even though it does not report the duration, so that much is now compared, and changing it fails the plan the way a changed role_restriction does. The duration of an existing expiration is still not comparable, which the docs say. Legacy rotation on rsa_public_key / rsa_public_key_2 was broken in a way named key pairs made visible: fetch_user never read rsa_public_key_2 back, so staging a second key re-applied it on every plan and the rotation never settled. It is read back now. Both legacy properties also accept a key with its PEM delimiters, which Snowflake's SQL rejects and DESC USER never returns -- they are normalized the way key pair material already was, via a shared snowcap.public_key module.
Three pieces of complexity that were not carrying their weight: - The rotation's grace-period argument was wrapped in a None check that IntProp.render already does. - update_user_key_pair had an UNSET branch nothing can reach: diff() drops None values before they become a delta, so no caller produces one, and the catch-all below already fails loudly if anything ever does. - The legacy public keys were normalized twice, in __post_init__ and again in to_dict. to_dict covers every read that matters, and it is the one that runs after vars resolve. Also dropped an empty-DER check that base64.b64decode can only reach for input the line above already refused.
Three integration seams outside the happy path, plus cleanups. `snowcap export --all` emitted user_key_pairs: blocks the loader then refused, because the exported dict carries fingerprint and has_expiration (derived, not constructor arguments) and cannot carry the public key Snowflake never returns. A sweep export is meant to produce config you can plan with, so key pairs are left out of it unless asked for by name; asked for by name, the block now contains only what the loader accepts, with public_key null for the operator to fill in. A live key pair named to look rotated-out passed the fetch filter -- by design, since trusting the generated name would hide it -- and was then refused by the spec, which is also how remote state is deserialized. In sync mode that aborted the whole plan over a key pair the config may not even manage. The name guardrails now live on the resource constructor, which is the config-facing path; the spec accepts every name Snowflake can report. The documented legacy rotation told the reader to finish by clearing rsa_public_key_2. An absent value means "not managed" everywhere in snowcap, so that step plans nothing and leaves the retired key live. The docs now say to run the UNSET out of band, and point at named key pairs, which retire the prior key on a timer instead. Also: dropped the RENAME branch (a rename changes the urn, so it is a drop and create, and the plan checker refuses renames outright), routed the account-wide sweep through the same SHOW helper a fetch uses so the two cannot drift apart and lose the cache hit, and imported normalize_fingerprint from the module that owns it. The rotation integration test now carries a role restriction and an expiry, so it verifies against a live account what the docs claim: that a rotation inherits both, and the plan right after is empty.
Review of PR #67No issues found. This PR adds a well-scoped
Nothing here rises to a confidence level worth blocking on. |
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
2 participants
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.
Snowflake added named key pairs for users: registered with
ALTER USER … ADD KEY PAIR, up to 10 per user, each with its own role restriction, expiration, disabled flag, and comment. Unlike the legacyrsa_public_key/rsa_public_key_2properties, they support rotation with a grace period.Adds a
UserKeyPairresource covering the whole surface, and fixes the legacy rotation path along the way.Usage
Standalone, or inline on the user that owns them:
What it does
ALTER USER u ADD KEY PAIR k PUBLIC_KEY = … [ROLE_RESTRICTION] [DAYS_TO_EXPIRY] [COMMENT]public_keyALTER USER u ROTATE KEY PAIR k PUBLIC_KEY = … [EXPIRE_ROTATED_KEY_PAIR_AFTER_HOURS]disabled/commentALTER USER u MODIFY KEY PAIR k SET …ALTER USER u REMOVE KEY PAIR kA key pair is addressed as
<name>?user=<username>, so two users can each have aMY_KEY.Drift detection without the key
Snowflake never echoes a key pair's public key back —
SHOW USER KEY PAIRSreturns only a SHA-256 fingerprint. So the spec carries a fingerprint computed from the configured key (base64 SHA-256 of the DER, the same valueopensslproduces), and drift is detected on that. A changedpublic_keytherefore plans a rotation rather than reading as "unchanged".Because that delta names a fingerprint rather than a key,
update_resourcenow hands handlers that need it the full desired state, and a lifecycle handler may return several statements for one change — a rotation plus a property change has no single-statement form in Snowflake.Rotation
Change
public_keyand apply. The prior key stays valid for a grace period (Snowflake's default is 24 hours) so clients can pick up the new key without downtime, and Snowflake keeps it under a generated<name>_ROTATED_<epoch_ms>name until it expires. Snowcap leaves those tombstones alone rather than removing them early, and the plan says explicitly that the prior key survives, under what name, and for how long.For a leaked private key,
expire_rotated_key_pair_after_hours: 0revokes the prior key immediately. The field describes the next rotation rather than the state of the key pair, so setting it plans nothing on its own and is never emitted intoADD KEY PAIR, which has no such option.What Snowflake won't let us change
role_restrictionand the presence of an expiration are fixed at registration, so both fail the plan with a message pointing at remove-and-re-add. The length of an existing expiration isn't comparable — Snowflake reports an absoluteexpires_at, not the duration — and the docs say so.Legacy key rotation, fixed
fetch_usernever readrsa_public_key_2back, so staging a second key re-applied it on every plan and the rotation never settled. Both legacy properties now round-trip, and both accept keys with their PEM delimiters (Snowflake's SQL rejects those, andDESC USERnever returns them). The one step Snowcap can't do — unsetting the retired key, since an absent value means "not managed" — is now called out in the docs instead of being implied to work.Notes for reviewers
MODIFY PROGRAMMATIC AUTHENTICATION METHODSon it. That privilege is now registered so it can be granted through Snowcap._merge_pointersidentified named resources by(type, name)alone, which made a second user'sMY_KEYlook like a duplicate. Resources now report identity-bearing fqn params through afqn_paramsproperty — empty for every existing resource, so their identity is unchanged.--resource user_key_pairstill exports them, as a block withpublic_keyleft to fill in.rotated_tocolumn, never by the generated name — the name is chosen by whoever registers the key, and trusting it would hide a live key from drift detection and from the sync sweep.Testing
2124 unit tests pass (59 new, covering SQL generation, the plan/diff pipeline, the plan-file round trip, and the fetch mapping); lint, format, typecheck and codespell are clean. Integration tests for create → rotate → disable → drop, the fetch round-trip, and the legacy two-key readback need a live account and have not been run here.
🤖 Generated with Claude Code
https://claude.ai/code/session_01WvLBPPa67ZahEFF6BZkXRS
Generated by Claude Code