Prevent algorithm-confusion forgery with typed VerifyingKey

[zshenker]

Problem

Token::verify(&[u8]) selects the verification algorithm from the token's own alg header and applies it to the caller-supplied key bytes. Before this branch (HMAC-only, secret key) that was safe. With ES256/PS256 it becomes a full authentication bypass:

1. A server accepts ES256 tokens and calls token.verify(public_key_der) — the public key is, by definition, public.
2. An attacker crafts a token with arbitrary claims and sets the header alg = HmacSha256.
3. They compute HMAC-SHA256(key = public_key_der_bytes, mac0_input) and place it in the signature slot.
4. verify() reads alg = HmacSha256, dispatches to the HMAC branch, runs HMAC with the public-key bytes as the secret — it matches → forged token accepted.

This is the COSE analogue of the classic JWT RS256→HS256 alg-confusion / key-confusion attack. The alg field being in the authenticated protected header does not help, because the attacker is forging a fresh token, not tampering with an existing one, and the verification key is public.

Fix

Bind the algorithm to the key the relying party supplies, not to the token:

• New VerifyingKey enum (HmacSha256 / Es256 / Ps256) that carries its key material and its algorithm.
• New Token::verify_with_key(VerifyingKey) that rejects any token whose header alg does not match the key's algorithm before running any cryptography, then verifies.
• Token::verify(&[u8]) is #[deprecated] and now only accepts HMAC-SHA256, returning InvalidAlgorithm for ES256/PS256. This keeps every existing HMAC caller working (all of main is HMAC) while removing the vulnerable asymmetric path.

This mirrors how google/coset and post-CVE JWT libraries handle it: the application pins the algorithm; the token only gets to agree.

Compatibility

Source-compatible. verify(&[u8])'s signature is unchanged; existing HMAC usage compiles and behaves identically (just emits a deprecation note). The only behavior that changes is the not-yet-released asymmetric verify(public_key) path — which is exactly the vulnerable one.

Tests

• test_alg_confusion_hmac_over_public_key_is_rejected — reproduces the forgery and asserts it's rejected with InvalidAlgorithm.
• test_deprecated_verify_rejects_asymmetric / test_deprecated_verify_still_works_for_hmac.
• Existing ES256/PS256/HMAC tests migrated to verify_with_key; wrong-key tests extended to cover both the crypto-mismatch and algorithm-binding rejection paths.
• All 45 lib tests + 74 doctests pass; cargo clippy --all-targets clean; both asymmetric examples run and verify.

Docs, README, and all examples migrated to verify_with_key.

🤖 Generated with Claude Code

[jedisct1]

This is an automated review by Swival (https://swival.dev) using only open-source models.

I recommend merging this after a small formatting fix.

The security direction looks right to me. Token::verify_with_key now binds verification to the caller-provided VerifyingKey algorithm before running crypto in src/token.rs:189, which addresses the algorithm-confusion path described in the PR. Keeping the old verify(&[u8]) limited to HMAC at src/token.rs:161 also preserves the existing HMAC behavior while removing the unsafe asymmetric path. The regression test in src/tests.rs:1724 reproduces the public-key-as-HMAC-secret forgery and checks that it is rejected before verification, which is the right invariant to lock down.

I also like that the examples and docs were moved to verify_with_key, and that VerifyingKey is re-exported from src/lib.rs, so users can follow the safer API without reaching into internal modules.

The only thing I would fix before merging is formatting. cargo fmt --check currently fails in examples/asymmetric_signing.rs, examples/sample_es256_ps256_tokens.rs, and src/tests.rs around the newly added imports and the deprecated verify test call. That is not a design issue, but it does leave the branch inconsistent with normal Rust project hygiene and will create avoidable churn later.

I verified that cargo test passes on the latest local toolchain with 45 library tests and 74 doctests, and cargo clippy --all-targets -- -D warnings also passes. Once the formatting is fixed, I think this is a good change to merge into the ES256/PS256 branch.

[Copilot]

Pull request overview

This PR mitigates an algorithm-confusion forgery in Token::verify(&[u8]) by introducing a typed verification key that pins the verification algorithm to the relying party’s key, rather than trusting the token’s alg header.

Changes:

• Add VerifyingKey and a new Token::verify_with_key(VerifyingKey) API that rejects alg mismatches before performing cryptographic verification.
• Deprecate Token::verify(&[u8]) and restrict it to HMAC-SHA256 only (ES256/PS256 now return InvalidAlgorithm).
• Migrate tests, docs, README, and examples to use verify_with_key.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
src/header.rs	Introduces VerifyingKey enum and helpers to bind key material to an Algorithm.
src/token.rs	Adds verify_with_key, hardens deprecated verify to HMAC-only, and enforces algorithm/key binding.
src/tests.rs	Updates existing tests to use verify_with_key and adds regression coverage for alg-confusion scenarios.
src/lib.rs	Re-exports VerifyingKey and updates crate-level docs to use verify_with_key.
README.md	Updates usage docs and algorithm table to reflect typed verification keys and safer verification API.
examples/basic_usage.rs	Migrates verification calls to verify_with_key for HMAC usage.
examples/cat_validation.rs	Migrates verification calls to verify_with_key.
examples/cat_specific_claims.rs	Migrates verification calls to verify_with_key.
examples/asymmetric_signing.rs	Migrates asymmetric verification to verify_with_key.
examples/sample_es256_ps256_tokens.rs	Migrates asymmetric verification to verify_with_key.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[jedisct1]

Thanks for working through this. I reviewed the PR description, the current diff against the ES256/PS256 branch, the previous Swival comment, the Copilot review comments, and the affected verification paths in src/header.rs, src/token.rs, the examples, and the tests. I also ran rustup run stable cargo test, rustup run stable cargo fmt --check, rustup run stable cargo clippy --all-targets -- -D warnings, and both new asymmetric examples locally. This is an automated review by Swival (https://swival.dev) using only open-source models, intended to help both the author and the maintainers.

My recommendation is not to merge this quite yet, but I think the core security fix is the right shape and this should stay open.

The important implementation change looks correct to me. Token::verify_with_key in src/token.rs:191-225 checks the token's protected alg against the caller supplied VerifyingKey before running any cryptography, and the regression test in src/tests.rs:1727-1749 reproduces the public-key-as-HMAC-secret forgery and verifies that it is rejected with InvalidAlgorithm. Restricting the deprecated raw-byte verify path to HMAC in src/token.rs:163-173 also removes the unsafe asymmetric verification path from the new ES256/PS256 API surface.

The blocker I would fix before merge is in the examples. Both examples/asymmetric_signing.rs:91-95 and examples/sample_es256_ps256_tokens.rs:91-95 still include an Algorithm::HmacSha256 match arm that constructs VerifyingKey::HmacSha256(&public_key). Those functions are only called with ES256 and PS256 today, so this is not currently executed, but it documents exactly the unsafe key-confusion pattern the PR is meant to prevent: treating public key bytes as an HMAC secret. Since these are user-facing examples for the new safe API, I would make the HMAC arm explicitly unreachable or structure the example so only asymmetric algorithms can reach that code. That keeps the examples from teaching a dangerous fallback to readers who copy the pattern later.

I would also clean up the release and deprecation story before landing. src/token.rs:159-162 marks verify as deprecated since 0.3.0, while Cargo.toml:3 still declares 0.2.7. If this branch is intended to become a 0.3.0 breaking release, that should be made consistent with the versioning plan. If it is not, the since value should match the version that will actually ship. The PR description also calls the change source-compatible, but existing HMAC callers will now receive a deprecation warning, which can break downstream crates that build with #![deny(warnings)] or equivalent CI settings. That does not make the security fix wrong, but it does mean the compatibility impact should be described more carefully.

The earlier formatting concern appears to be resolved now. The test suite, formatting check, clippy, and the two asymmetric examples all pass locally for me on the latest stable toolchain. Once the example fallback and version/deprecation wording are tightened, I think this is a good security hardening change to merge into the ES256/PS256 work.

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.

[jedisct1]

Thanks for the update. I reviewed PR #6 against the current main branch after #5 landed, read the PR description, commit list, existing Swival review, Copilot comments, and review threads, and traced the affected paths in src/header.rs, src/token.rs, the examples, README, and the regression tests. I also validated the branch locally with rustup run stable cargo fmt -- --check, rustup run stable cargo test, rustup run stable cargo clippy --all-targets -- -D warnings, and the asymmetric_signing, sample_es256_ps256_tokens, and basic_usage examples. This is an automated review by Swival (https://swival.dev) using only open-source models, intended to help both the author and the maintainers.

My recommendation is not to merge this quite yet, but I would not close it. The core fix is valuable, and it is much better to finish this PR than to leave main with the documented asymmetric verify(public_key) pattern from #5 for longer than necessary.

The important part of the implementation looks right. Token::verify_with_key checks the protected-header algorithm against the caller supplied VerifyingKey before any cryptographic verification in src/token.rs:224-234, then dispatches to the matching primitive. The regression test in src/tests.rs:2269-2291 reproduces the HMAC-over-public-key forgery and confirms that an ES256 verifier rejects it with InvalidAlgorithm. The examples that previously had an HMAC fallback over public_key have also been fixed in examples/asymmetric_signing.rs:97-101 and examples/sample_es256_ps256_tokens.rs:97-101, and Cargo.toml:3 now matches the 0.3.0 deprecation version.

The first blocker I would fix is #[derive(Debug)] on VerifyingKey in src/header.rs:165. VerifyingKey::HmacSha256 carries the raw shared secret bytes, so a derived Debug implementation will print the secret if the value is ever logged, included in a panic, or captured by generic diagnostics. This is a security API, and the safe default should not expose key material. I agree with Copilot on this point. Please remove Debug, or provide a manual implementation that redacts the key bytes while still showing the variant name.

I would also mark VerifyingKey as #[non_exhaustive] before publishing it. Algorithm is already non-exhaustive, and its docs in src/header.rs:44-46 explicitly say future algorithm support should not break downstream matches. VerifyingKey is the public companion enum that mirrors those algorithms, so leaving it exhaustive creates the opposite compatibility story: the next supported algorithm will require adding a new VerifyingKey variant and will break downstream exhaustive matches. This is easiest to get right now, before the type is released.

One more safety point needs tightening in the docs and tests around the deprecated raw-byte API. The current text in src/token.rs:177-195 says verify now only accepts MAC tokens and returns InvalidAlgorithm for ES256 and PS256, which is true. But the actual alg-confusion exploit token declares HmacSha256, not ES256 or PS256. Because verify still routes Algorithm::HmacSha256 to verify_with_key(VerifyingKey::HmacSha256(key)) in src/token.rs:201-203, a caller who keeps doing verify(public_key_der) on attacker supplied tokens can still accept the forged HMAC token. The safety guarantee comes from migrating asymmetric verification to verify_with_key(VerifyingKey::Es256(...)) or VerifyingKey::Ps256(...), not from making the legacy raw-byte method intrinsically safe.

I do not think that means the design is wrong, since keeping HMAC compatibility is a reasonable tradeoff. I do think the deprecation docs, PR compatibility wording, and tests should avoid implying that verify itself can no longer be used in the confusion pattern. A small wording change plus a regression or comment that demonstrates the legacy method remains HMAC-only and must not be used with asymmetric public keys would make the boundary much clearer for future maintainers. If the intended guarantee is stronger than that, then the raw-byte API needs a harder break rather than just rejecting tokens whose own header says ES256 or PS256.

I do not consider Copilot's VerifyingKey<'_> lifetime comment a merge blocker. The branch passes clippy with warnings denied, and this repository does not enable elided_lifetimes_in_paths. Making the lifetime explicit in the public signature would be fine for readability, but it is not the issue I would hold this PR on.

Once the secret-leaking Debug implementation is fixed, the new public enum is made future-compatible, and the legacy verify safety wording is corrected, I think this should be mergeable and should land promptly against main.