Stop using a certificate the signed-in account does not own (GRYT-905) - #385
Merged
Conversation
Signed in as test@sivert.io, the desktop client joined community.gryt.chat as
Sivert — the owner, who is root@sivert.io.
`getValidCertificate` checked a cached certificate two ways: not close to
expiry, and the JWK inside it still names the keypair on this device. It never
checked who the certificate was for. `answer-challenge.ts` then signs the
assertion with the `sub` it reads back out of that certificate:
const certificate = await getValidCertificate();
const sub = getCertificateSub() || "";
const assertion = await signAssertion(sub, host, nonce, { kind: "account" });
So the server was handed a real certificate, signed by the CA, for a key the
client genuinely holds, naming a person who was not at the keyboard. Every
check it can make passes. There is no server-side fix for this.
The two halves drift because they live apart and are cleared by different
code — the keypair in IndexedDB, the certificate in localStorage — and
`signOut` is the only path that takes both. A certificate outlives a session by
a wide margin, so anything that ends one without going through there leaves it
behind. The file already documented the other direction of the same drift; the
guard written for it only compared the key.
The rule moves to `certificate-verdict.ts`, which imports nothing, so it can be
run without Keycloak, IndexedDB, `fetch` or a keychain — needing all four is
how it went unchecked. Four answers rather than two, because the repairs
differ:
use ours, current, matches the key
stale ours and old — fetch, and leave it in place until the new one
lands, so a failed renewal offline does not also cost us the
`sub`
wrong-key ours but names a key we no longer hold — drop it
wrong-account somebody else's — drop it *and* the keypair under it
That last one takes the key because a certificate binds one `sub` to one key.
Minting a new one over the previous account's key would hand two accounts the
same key, and a server that pinned it would see one key arrive under a second
name. Dropping it is safe: an account key is random rather than derived, the CA
certifies a fresh one on the next sign-in, and DM keys come off the seed, which
this does not touch.
A null answer for the signed-in account is not a mismatch. Signed out, session
lapsed and Keycloak unreachable all answer null, and discarding somebody's
identity on the strength of a failed token refresh is the worse bug.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.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
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.
Signed in as
test@sivert.io, the desktop client joinedcommunity.gryt.chatas Sivert — the owner, who isroot@sivert.io. The member list was right. The identity was not.What was wrong
getValidCertificatechecked a cached certificate two ways — not close to expiry, and the JWK inside it still names the keypair on this device. It never checked who the certificate was for, even thoughparseJwtSubsits in the same file.answer-challenge.tsthen signs the assertion with thesubit reads back out of that certificate:So the server is handed a real certificate, signed by the CA, for a key the client genuinely holds, naming somebody who is not at the keyboard. Every check the server can make passes. There is no server-side fix for this one.
The two halves drift because they live apart and are cleared by different code — keypair in IndexedDB, certificate in localStorage — and
signOutis the only path that takes both. A certificate outlives a session by a wide margin. The file already documented the other direction of this same drift; the guard written for it only compared the key.The shape
The rule moves to
certificate-verdict.ts, which imports nothing. Needing Keycloak, IndexedDB,fetchand a keychain to reach this decision is how it went unchecked long enough to ship.Four answers rather than two, because the repairs differ:
usestalewrong-keywrong-accountWhat to look at
wrong-account. This is the part that changes behaviour rather than shape, and the part I am least sure about. The reasoning: a certificate binds onesubto one key, so minting a new one over the previous account's key hands two accounts the same key, and a server that pinned it sees one key arrive under a second name. I believe it is safe to drop — an account key iscrypto.subtle.generateKey, random rather than derived; the CA certifies a fresh one on the next sign-in; DM keys come off the seed, which this does not touch. If you would rather keep the key and only re-certify it, that is a one-line change and the check has a case for it.staledeliberately not clearing storage. That preserves today's behaviour: a renewal that fails offline should not also cost us thesubthatgetCertificateSubreads back. It does mean an expired certificate for the current account stays on disk, which it already did.Verification
yarn lint(which istsc -b+ eslint) passes.scripts/check-identity-certificate.mjsis new, wired intopackage.jsonand CI, and mutation-tested — five mutations, all caught:wrong-accountno longer clears the keypaircertificateVerdictNot verified end to end. Reproducing the original needs two real Keycloak accounts on one device, which I cannot drive. The diagnosis is read off the source and the reported symptom, and the fix is checked at the unit and source level.
🤖 Generated with Claude Code