refactor(webauthn): use RequestOrigin instead of RelyingPartyId in WebAuthnIDL (fix #185)#188
Conversation
AlfioEmanueleFresta
changed the title
msirringhaus
left a comment
msirringhaus
left a comment
There was a problem hiding this comment.
I could imagine http:// being used in some self-hosting env, but I'm ok leaving it out for now.
I'm in general feeling a bit uneasy about writing all the parsing ourselves, but if there is nothing suitable available (e.g. by Mozilla/servo), then so be it.
One inline comment needs clarification, otherwise LGTM.
Usage of |
Yeah, that's a good callout.
The RP should still be able to detect the mismatched origin, so phishing resistance would be preservezd. But I agree, this should be restricted to |
AlfioEmanueleFresta
force-pushed
the
origin-parsing
branch
from
81861b5 to
806107f
Compare
|
Thank you for your feedback!
|
… (#190) Closes #187. Stacked on #188. ## What Replaces the strict-equality check between the JSON request's `rp.id` and the origin's effective domain with the spec-correct "registrable domain suffix of or equal to" relation from HTML §6.5 (referenced by WebAuthn L3 §5.1.3 step 7 and §5.1.7 step 9). Net effect: `rp.id = "example.org"` now works against an origin of `https://login.example.org`, while `rp.id = "co.uk"` against `https://example.co.uk` is still correctly rejected because `co.uk` is a public suffix. ## PSL strategy Per the design discussion in #173 (libwebauthn should not bundle and manage its own PSL): - Adds a `PublicSuffixList` trait with sync `registrable_domain` / `public_suffix` methods. - Provides a `DatFilePublicSuffixList` impl that reads a Public Suffix List `.dat` file at construction time. `from_system_file()` reads the standard `/usr/share/publicsuffix/public_suffix_list.dat`, kept current by the system package manager. - No PSL data is bundled in the libwebauthn crate. - `publicsuffix = "1.5"`. Same crate as #173, just used offline. ## Plumbing into the parsing API `WebAuthnIDL::from_json` and `FromIdlModel::from_idl_model` now take an additional `psl: &dyn PublicSuffixList` parameter. This is the simplest shape we discussed - explicit, no hidden state on `RequestOrigin`. Yes, it's another breaking signature change on top of #188; the 0.4.0 release is already breaking, so we eat the cost once. Origin parsing (`Origin::from_str`, `RequestOrigin::try_from`) stays purely syntactic. PSL is only consulted at validation time. ## Commits 1. Add `PublicSuffixList` trait and `DatFilePublicSuffixList` impl. 2. Use `PublicSuffixList` for the registrable-suffix check in `from_idl_model`. ## Tests - Unit tests on the suffix-check helper using a small `MockPublicSuffixList` (recognises `com`, `co.uk`, `org`, `net`). - Unit tests on the mock itself. - 2 new positive integration tests on `from_json` for both `MakeCredentialRequest` and `GetAssertionRequest`: rp.id-as-parent-registrable-suffix accepted, rp.id-as-eTLD rejected. ## Test plan - [x] `cargo build --workspace --all-targets --all-features` - [x] `cargo fmt --all -- --check` - [x] `cargo clippy --workspace --all-targets --all-features -- -D warnings` - [x] `cargo test --workspace` (151 tests) - [x] `cargo publish --dry-run -p libwebauthn`
4 participants
Motivation
Closes #185.
WebAuthnIDL::from_jsoncurrently takes a&RelyingPartyId, which forces callers (e.g. credentialsd) to overriderequest.originandrequest.cross_originafter parsing because the bare host is not a valid origin string and we have no place to record the top-level origin. This replaces that parameter with aRequestOriginthat carries the actual origin context, so the parsed request comes out correct without a post-parse fixup.What changes
OriginandRequestOrigintypes inlibwebauthn::ops::webauthn.Originis a struct withhost: OriginHostandport: Option<u16>;RequestOriginwraps it with an optionaltop_origin. Convenience constructors:RequestOrigin::new(origin),RequestOrigin::new_cross_origin(origin, top_origin),RequestOrigin::try_from(&str | String)for one-shot parsing of"https://host[:port]".url::Hostso we follow the WHATWG URL Standard host parser (domain / IPv4 / bracketed IPv6). Errors are wrapped into a localHostParseError/OriginParseErrorso theurlcrate's error type does not leak into the public API.WebAuthnIDL::from_jsonandFromIdlModel::from_idl_modelnow take&RequestOrigin. The parsedrequest.originis the full URL string ("https://example.org", no longer the bare host), andrequest.top_origin: Option<String>replaces the oldcross_origin: Option<bool>.ClientDatadropscross_origin: Option<bool>and derivescrossOriginin the JSON fromtop_origin.is_some().topOriginis now emitted when present.0.4.0since the publicfrom_jsonsignature and the request struct fields are breaking changes.Intentional non-changes
Originis a plain struct, not an enum. We only supporthttps. If we need to support a second scheme later it can become an enum without breaking call-site field access.rp.idvalidation against the origin is still strict equality. The spec actually wants a registrable-suffix check (WebAuthn L3 §5.1.3 / §5.1.7); that is tracked separately in Validate rp.id as a registrable suffix of the origin's effective domain #187 and can build on the PSL helpers from Implement #160 Related Origins support (WebAuthn L3 § 5.11) #173.Test plan
cargo build -p libwebauthnandcargo build -p libwebauthn --features virtcargo build --workspace --all-targets --all-featurescargo fmt --all -- --checkcargo clippy --workspace --all-targets --all-features -- -D warningscargo test --workspace(149 tests, 14 new origin parser tests)cargo publish --dry-run -p libwebauthn(no working-tree hacks)