You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
fix(server): make an API key's digest name exactly one namespace
POST /api/namespaces/api-key let the caller supply the key's plaintext. The stored primary key
was SHA256(plaintext), the uniqueness constraint was the composite (key_digest, namespace_id),
and authentication resolved the digest with an unbounded scope and took whichever row came back.
So the same digest could exist in two namespaces, and an administrator or owner of any namespace
could register a key whose plaintext matched a key in a namespace they had no relationship with
and then authenticate as that namespace's key, inheriting its role rather than their own.
The unbounded resolve carried its own justification -- "the digest is itself the capability
identifying the namespace" -- and that reasoning holds only if a digest can name one namespace.
It could not. Which of the two rows won was not deterministic: the query planned as a sequential
scan, so the answer followed physical order, which an attacker influences by choosing when to
create the colliding key and which a routine key rotation flips on its own. A 50% success rate
on an authentication attempt is not a mitigation; the defect is that the two rows are
indistinguishable to the authenticator at all.
Reproduced end to end against Postgres before the fix: an attacker-registered plaintext resolved
to the victim's owner key, at owner role, in a namespace the attacker was 403 for on every other
route.
Five changes, so that no single one of them is load-bearing:
CreateAPIKey.Key is gone. The plaintext is generated server-side and always was for every real
caller -- the console never sent the field, and nothing in this repository or in cloud/ did
either. A caller can no longer name a secret another namespace may already hold. The field's
stated purpose, idempotent provisioning, is the vulnerability restated.
Migration 023 puts a unique index on api_keys.key_digest, so the schema enforces what the
authenticator assumes and a colliding insert fails regardless of any application check. Rows in a
colliding group are deleted, all of them, none elected the survivor: authentication was already
picking between them arbitrarily, so no row can be trusted to belong to whoever holds the
plaintext, and one may have been planted precisely to collide. Deleting rather than 007's
non-destructive demotion is forced by the constraint being added -- two rows sharing a digest
cannot both survive an index that forbids it -- and expiring one in place would leave the choice
of which to bless, which is the decision the whole bug is that we cannot make. So it is loud
instead of silent: the migration RAISEs a warning naming the digest and both namespaces before
deleting, because a revoked credential the operator never hears about is an outage with no cause.
APIKeyResolve refuses a digest matching more than one row instead of choosing, and AuthAPIKey
logs it. A duplicate is a security event, not a lookup outcome. This guard is what covers the
window the index cannot: a deployment between upgrade and migration, where the colliding rows
still exist.
The cached authentication is keyed by a new prefix. AuthAPIKey consults the cache before the
store, so an entry written before the index existed -- one that may have resolved a colliding
digest into either namespace -- would authenticate for its remaining TTL without ever reaching
the ambiguity guard, including after the migration deleted both rows. Changing the key makes
those entries unreadable rather than trusted.
install_keys gets the same index and the same guard in InstallKeyResolve. installKeyTenant also
resolves a digest unbounded; install key plaintexts are server-generated so no collision is
reachable, but the invariant is identical and nothing in the schema was holding it. It gets no
DELETE: an install_keys row carries install_key_events by ON DELETE CASCADE and devices by ON
DELETE SET NULL, so deleting to satisfy a constraint that has never been violated would destroy
enrollment history to fix nothing. If a collision somehow exists the migration aborts on the
index and the operator is told.
The composite primary key (key_digest, namespace_id) stays. It is now implied by the new index,
but it is the target of the devices and install_key_events foreign keys, which reference the pair.
resolveUnique in pg/utils.go carries the Limit(2)-and-count for both resolvers rather than each
holding a copy, and migrationStatements replaces the third copy of read-file-split-on-bun:split.
TestScopeIsolationInstallKeyResolve asserted the old behaviour -- it planted one digest in two
namespaces and expected each bounded resolve to answer with its own. That premise is what this
commit forbids. It keeps both halves of its subject with a digest per namespace: each resolves
its own, and neither reaches the other's. The shared digest case is now
TestInstallKeyDigestIsGloballyUnique.
Reported-by: Edu0x01
Copy file name to clipboardExpand all lines: server/api/services/scope.go
+1-1Lines changed: 1 addition & 1 deletion
Original file line number
Diff line number
Diff line change
@@ -6,7 +6,7 @@ import (
6
6
7
7
constreasonInternalSessionMutation="internal SSH-driven session mutation: no namespace exists anywhere in the call chain yet; bounding it changes the SSH-facing contract (see #6749)"
8
8
9
-
constreasonInstallKeyTenant="enrolling with an install key alone, whose digest is itself the capability identifying the namespace"
9
+
constreasonInstallKeyTenant="enrolling with an install key alone, whose digest install_keys_key_digest_unique makes name exactly one namespace"
10
10
11
11
// BoundTo bounds an operation to the tenant a request carries. An absent tenant refuses the request
12
12
// rather than widening it to every namespace, matching the tenant-guard middleware's fail-closed
0 commit comments