Skip to content

feat(auth): pin the WebAuthn credential algorithm set and drop RS256 (BACKLOG #1166) - #919

Open
wshallwshall wants to merge 2 commits into
mainfrom
claude/mgr-1166
Open

feat(auth): pin the WebAuthn credential algorithm set and drop RS256 (BACKLOG #1166)#919
wshallwshall wants to merge 2 commits into
mainfrom
claude/mgr-1166

Conversation

@wshallwshall

Copy link
Copy Markdown
Collaborator

One limb of BACKLOG #1166, the WebAuthn advertised-algorithm restriction. This does NOT close #1166 and does not flip its banner: the item's verdict moves by a scorecard re-score, which no Builder can perform, and several named surfaces stay open.

What was wrong

The relying party inherited py_webauthn's default algorithm set, which carries RS256 (-257). A COSE RSA identifier fixes the padding and the hash and leaves the modulus unbounded, so it cannot carry the 128-bit floor ASVS 11.2.3 asks for.

Re-measurement, by execution, before building

Interpreter named rather than assumed: C:\...\worktrees\manager-277cbb-b1166\.venv\Scripts\python.exe, CPython 3.14.6. Library versions introspected and compared to the lock in the same run, since the load-bearing question here is what the pinned library advertises:

package introspected requirements.lock
webauthn 3.0.0 3.0.0
cryptography 50.0.1 50.0.1
cbor2 6.1.2 6.1.2

All three agree, so the introspection is evidence about shipped behaviour. Neither bare python nor the primary checkout's venv was used.

The row is true and understates the defect on two axes. Both corrections are appended inside item #1166's own section.

  1. Advertising is not the control. verify_registration_response takes the same supported_pub_key_algs parameter and was also at the library default. An authenticator may ignore what is advertised; the verify call is the only place a credential is refused. A change restricting only the advertisement would have read like a fix and accepted exactly the same credentials, which is the shape this item exists to catch.
  2. The accepted set had no floor at all, not merely a 2048-bit one. Measured: an RS256 credential over a 2048-bit modulus registered and was ACCEPTED, and so did one over a 1024-bit modulus.

Three positive controls in the same run: ES256 accepted (the fixture builds valid credentials), a garbage response refused (the verifier is live), and PS256 (-37) refused (the library's algorithm screen was running and discriminating, so the RS256 acceptance was a real allow rather than an unreached branch).

What was built

One module constant, SUPPORTED_COSE_ALGS = (-8, -7) (EdDSA, ES256), fed to both ceremony halves through a single lazy resolver, so the offered set and the accepted set cannot drift. Plain ints at module scope so the module still imports without the optional [webauthn] extra.

EdDSA and ES256 carry no equivalent hole. The curve rides in the credential rather than in the identifier, but a credential whose curve is unknown or does not match its key cannot produce a verifiable assertion (measured: it registers, then every assertion against it is refused), so no sub-floor EC2 or OKP credential is ever usable. That asymmetry is why the floor is expressible here as a set of identifiers, and it is the same reasoning the 2026-08-22 signing-key limb recorded for EC.

Also: an ADR 0068 amendment recording the pin as a third policy pin beside attestation=NONE and user_verification=PREFERRED.

The cost, stated rather than hidden

An authenticator that offers only RS256 can no longer enrol a passkey. TPM-backed Windows Hello is the population that registers RSA credentials. This cost is reasoned, not measured -- no real authenticator was driven here, only the pinned library. Those operators keep TOTP, which ADR 0068's 2026-07-17 amendment already records as the alternative second factor, and the password leg is unchanged.

Deliberately not an operator setting: a knob re-admitting -257 would be exactly the operator-supplied weak configuration this requirement is failing on.

Proof

Three mutations, each red, each compiled before running so a syntax break could not masquerade as a behaviour failure, and each checked to FAIL rather than ERROR:

mutation result
advertise-side argument removed 2 failed, 6 passed
enforce-side argument removed 2 failed, 6 passed
-257 re-admitted to the pinned set 4 failed, 4 passed

Eight new rows in tests/test_webauthn_algs.py. One fires if a future webauthn release narrows its own default to match the pin, so the file cannot pass vacuously; another drives the library's own default path over the same RSA fixture, so a refusal row cannot pass on a broken fixture.

The instrument trap was checked and did not fire

This requirement's recorded absence pattern keys on a 3072-bit RSA modulus comparison. All 341 added lines were swept with it: zero matches, against a positive control in the same run where a planted comparison matched. The claim's own positive control still speaks in the corpus. No re-cut was needed and none was made.

A separate glyph sweep over the added lines found zero symbol-other characters (CLAUDE.md section 11), with a positive control.

Cross-item note, recorded rather than edited elsewhere

#1168 (ASVS 11.3.1) names this same site from the padding angle: "the shipped relying party advertises COSE -257 and every enrolled credential is verified with PKCS#1 v1.5 at each login". No credential can now be enrolled under -257, so that sentence is stale for new enrolments. Whoever works #1168 should re-measure rather than relay it. I did not edit #1168's section.

A measured residual, unfiled and named by subject

The COSE crv field is not validated at registration, so a credential whose curve is unknown or mismatched enrols and only fails at first assertion. On the mismatched-curve path the failure arrives as a raw ValueError from cryptography rather than a WebAuthnException, so verify_assertion's except WebAuthnException does not catch it and the rejection would not land on the audited invalid-input path ADR 0068 decision 1 requires. Self-harm only (the enrolling user is authenticated and breaks their own credential). This change neither causes nor fixes it. Recorded in the backlog note and the ADR amendment.

Checks run

All from this worktree's own venv.

  • ruff check . and ruff format --check . -- clean
  • mypy messagefoundry (strict) -- Success: no issues found in 268 source files
  • pytest tests/test_webauthn_algs.py tests/test_webauthn.py tests/test_webauthn_store.py tests/test_mfa.py -- 46 passed
  • pytest tests/test_crypto_inventory_doc.py tests/test_crypto_inventory_scanner.py tests/test_security_static.py tests/test_key_usage_scope_inventory.py tests/test_security_doc_drift.py -- 142 passed
  • pytest tests/test_api_auth.py tests/test_groups.py -- 83 passed
  • pytest tests/test_seam_discovery.py tests/test_session_rotation_primitive.py tests/test_new_dependency_check.py tests/test_worktree_venv_extras_parity.py tests/test_scan_tokens_source.py -- 331 passed
  • pytest tests/ -k asvs -- 564 passed, 12 skipped, 15350 deselected
  • Commit gates by hand and again via pre-commit: ledger gate, backlog parse, SPDX, control-char, forbidden-content, gitleaks, username-access-key, ruff, bandit -- all passed
  • scripts/security/crypto_inventory_check.py -- passed. messagefoundry/auth/webauthn.py was already inventoried for secrets + webauthn and gains no new crypto import; tests/ is outside WALK_ROOTS, so the new file's cryptography import registers nothing.

Checks NOT run, and the legs somebody must read after I exit

  • The full local suite. Not attempted: it does not finish reliably under fleet contention. The targeted sets above cover the change; -k asvs was run as a wider sweep.
  • ubuntu-latest. Everything above ran on Windows only. That platform split has already bitten this item once (a prior limb wrote a Windows OpenSSL cipher census into tests as though it were a fact about TLS and went red on ubuntu). Nothing here reads a platform cipher inventory, but read the leg.
  • pip-audit, cla, and the rolled-up CI gate -- never runnable locally.
  • The [webauthn] extra hazard, and it did NOT apply here. These tests importorskip("webauthn"), so without the extra a real downgrade would run completely green. The extra is installed in this worktree (3.0.0, matching the lock) and all eight rows executed rather than skipping. Confirm the CI leg that installs the extra actually ran them rather than skipping them.
  • I did not merge, enqueue, arm auto-merge, or apply any label.

…(BACKLOG #1166)

The relying party inherited py_webauthn's default algorithm set, which carries RS256
(-257). A COSE RSA identifier fixes the padding and the hash and leaves the MODULUS
unbounded, so it cannot carry the 128-bit floor ASVS 11.2.3 asks for.

Re-measured by execution before building, on the pinned webauthn==3.0.0 from this
worktree's own venv: an RS256 credential over a 2048-bit modulus registered and was
ACCEPTED, and so did one over a 1024-bit modulus. Positive controls in the same run:
ES256 accepted, garbage refused, PS256 refused (so the library's algorithm screen was
running and discriminating, and the RS256 acceptance was a real allow).

The item's row understates this twice, and the BACKLOG note corrects it:

  1. verify_registration_response takes the SAME supported_pub_key_algs parameter and
     was also at the library default. Advertising is a hint an authenticator may
     ignore; the verify call is the only refusal. Restricting the advertised set alone
     would have read like a fix while accepting exactly the same credentials.
  2. The accepted set had no floor at all, not merely a 2048-bit one.

Built: one module constant, SUPPORTED_COSE_ALGS = (-8, -7), fed to BOTH ceremony
halves through a single lazy resolver so the offer and the refusal cannot drift. EdDSA
and ES256 carry no equivalent hole: the curve rides in the credential rather than the
identifier, but a credential whose curve is unknown or mismatched cannot produce a
verifiable assertion (measured), so no sub-floor EC2 or OKP credential is ever usable.

Cost, stated rather than hidden: an authenticator offering only RS256 can no longer
enrol a passkey, and TPM-backed Windows Hello is that population. Those operators keep
TOTP, which ADR 0068's 2026-07-17 amendment already records as the alternative second
factor. Deliberately NOT an operator setting: a knob re-admitting -257 would be the
operator-supplied weak configuration this requirement is failing on.

Proof: three mutations, each red, each compiled first and checked to FAIL rather than
ERROR. Eight new rows, one of which fires if a future release narrows the library
default to match the pin, so the file cannot pass vacuously.

Instrument check: this requirement's recorded absence pattern keys on a 3072-bit RSA
comparison. All 341 added lines swept with it, zero matches, against a positive
control that matched a planted comparison. The guard still fires; no re-cut needed.

Does NOT close #1166. Its verdict moves by a scorecard re-score, which no Builder can
perform, and other named surfaces stay open.
@github-actions github-actions Bot added the ci-red A required check went red. Attribute it before retrying. label Sep 6, 2026
@github-actions github-actions Bot added the unread Green and mergeable, but not marked read. Read it, then add: reviewed label Sep 6, 2026
@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown

@wshallwshall

This pull request is green and nobody has marked it read.

Every check that has reported is passing and the reviewed label is absent, so
a reviewer has read this is the only thing between it and a merge. Nothing else reports that, which is
why this comment exists (BACKLOG #1413).

To clear it, IN THIS ORDER:

  1. If the branch is behind main, update it FIRST: gh pr update-branch --repo MEFORORG/MessageFoundry 919.
    That push fires synchronize, and synchronize REMOVES the reviewed label. Labelling
    before updating throws the label away and costs a round trip.
  2. Read the diff.
  3. gh pr edit 919 --repo MEFORORG/MessageFoundry --add-label reviewed

DO NOT TRIAGE THIS FROM mergeStateStatus. It returns one value with precedence, so BEHIND,
DIRTY and UNSTABLE each mask the missing check -- the requirement is invisible until you act on
something else. This check never reads that field; see scripts/ci/check_unread_prs.py.

A seat can find every pull request in this state in one call: gh pr list --label unread.
The label is withdrawn automatically once this pull request leaves the state.

The reviewed label is a PROCESS gate. It records that a step happened. It does not
establish that an independent party looked, and nothing here should be read as saying it does.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci-red A required check went red. Attribute it before retrying. unread Green and mergeable, but not marked read. Read it, then add: reviewed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant