Fix #95, correct key ordering for CBOR encoding#96
Merged
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes issue #95 by adjusting the serialization order of map keys to meet CTAP canonical CBOR encoding requirements.
- Reorders fields in Ctap2CredentialType and Ctap2PublicKeyCredentialDescriptor so that keys are sorted by string length.
- Adds regression tests to validate proper CBOR serialization.
- Updates the hex dependency version in Cargo.toml.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| libwebauthn/src/proto/ctap2/model.rs | Reordered struct fields and added tests to enforce canonical CBOR ordering. |
| libwebauthn/Cargo.toml | Bumped the hex dependency version from 0.4.2 to 0.4.3. |
Comments suppressed due to low confidence (1)
libwebauthn/Cargo.toml:44
- The hex dependency update to version 0.4.3 should be verified for compatibility with the rest of the dependency graph.
+hex = "0.4.3"
Comment on lines
142
to
+143
| pub struct Ctap2PublicKeyCredentialDescriptor { | ||
| pub r#type: Ctap2PublicKeyCredentialType, | ||
| pub id: ByteBuf, | ||
| pub r#type: Ctap2PublicKeyCredentialType, |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Member
Author
There was a problem hiding this comment.
Sadly this is nonsense, the change does exactly what the comment states, and the code suggestion reverses it.
kalvdans
approved these changes
May 5, 2025
msirringhaus
reviewed
May 5, 2025
AlfioEmanueleFresta
added a commit
that referenced
this pull request
May 6, 2025
## Changes See [list of commits](https://github.com/linux-credentials/libwebauthn/pull/97/commits) for individual changes. ## Testing Tested successfully with a Pixel 8 Pro running Android 15. Creation & assertion are successful using Bitwarden on the phone. ## Motivation I was originally getting a HTTP 400 error upon connecting to Google's caBLE servers: ``` DEBUG libwebauthn::transport::cable::tunnel: Connecting to tunnel server connect_url="wss://cable.ua5v.com/cable/connect/078f8a/4daa63321c9414caecee728764b0b3c9" DEBUG rustls::anchors: add_parsable_certificates processed 148 valid and 0 invalid certs DEBUG tokio_tungstenite::tls::encryption::rustls: Added 148/148 native root certificates (ignored 0) DEBUG rustls::client::hs: No cached session for DnsName("cable.ua5v.com") DEBUG rustls::client::hs: Not resuming any session DEBUG rustls::client::hs: Using ciphersuite TLS13_AES_256_GCM_SHA384 DEBUG rustls::client::tls13: Not resuming DEBUG rustls::client::tls13: TLS1.3 encrypted extensions: [] DEBUG rustls::client::hs: ALPN protocol is None ERROR libwebauthn::transport::cable::tunnel: Failed to connect to tunnel server e=Http(Response { status: 400, version: HTTP/1.1, headers: {"content-length": "1451", "content-security-policy-report-only": "script-src 'none'; form-action 'none'; frame-src 'none'; report-uri https://csp.withgoogle.com/csp/goa-55573edb", "content-type": "text/html; charset=utf-8", "cross-origin-opener-policy-report-only": "same-origin; report-to=\"goa-55573edb\"", "report-to": "{\"group\":\"goa-55573edb\",\"max_age\":2592000,\"endpoints\":[{\"url\":\"https://csp.withgoogle.com/csp/report-to/goa-55573edb\"}]}", "x-content-type-options": "nosniff", "x-frame-options": "SAMEORIGIN", "x-xss-protection": "0", "date": "Mon, 05 May 2025 10:40:43 GMT", "alt-svc": "h3=\":443\"; ma=2592000,h3-29=\":443\"; ma=2592000", "connection": "close"}, body: Some([10, 60, 33, 68, 79, 67, 84, 89, 80, 69, 32, 104, 116, 109, 108, 62, 10, 60, 104, 116, 109, 108, 32, 108, 97, 110, 103, 61, 101, 110, 62, 10, 32, 32, 60, 109, 101, 116, 97, ...]) }) thread 'main' panicked at libwebauthn/examples/webauthn_cable.rs:98:60: called `Result::unwrap()` on an `Err` value: Transport(ConnectionFailed) note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace ``` This was fixed by setting the `fido.cable` subprotocol on the WSS request. Then, we were failing to decode the CBOR payload received over the tunnel: ``` TRACE libwebauthn::transport::cable::tunnel: Trimmed padding decrypted_frame=[...] decrypted_frame_len=97 ERROR libwebauthn::transport::cable::tunnel: Failed to decode initial message e=SerdeDeCustom ERROR libwebauthn::transport::cable::tunnel: Failed to process initial message e=Transport(ConnectionFailed) thread 'main' panicked at libwebauthn/examples/webauthn_cable.rs:136:6: called `Result::unwrap()` on an `Err` value: Transport(TransportUnavailable) note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace ``` This was caused by missing fields in `CableInitialMessage`, which now includes a list of features (CTAP and digital credentials) as strings Once added, I was getting an error for missing supported PI/UV auth protocols: ``` DEBUG webauthn_make_credential{dev=CableChannel}:user_verification:ctap2_get_info: libwebauthn::proto::ctap2::protocol: CTAP2 GetInfo successful TRACE webauthn_make_credential{dev=CableChannel}:user_verification:ctap2_get_info: libwebauthn::proto::ctap2::protocol: ctap_response=Ctap2GetInfoResponse { versions: ["FIDO_2_0", "FIDO_2_1"], extensions: Some(["prf"]), aaguid: [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0], options: Some({"up": true, "rk": true, "plat": false, "uv": true}), max_msg_size: None, pin_auth_protos: None, max_credential_count: None, max_credential_id_length: None, transports: Some(["internal", "hybrid"]), algorithms: None, max_blob_array: None, force_pin_change: None, min_pin_length: None, firmware_version: None, max_cred_blob_length: None, max_rpids_for_setminpinlength: None, preferred_platform_uv_attempts: None, uv_modality: None, certifications: None, remaining_discoverable_creds: None, vendor_proto_config_cmds: None, attestation_formats: None, uv_count_since_last_pin_entry: None, long_touch_for_reset: None, enc_identifier: None, transports_for_reset: None, pin_complexity_policy: None, pin_complexity_policy_url: None, max_pin_length: None } ERROR webauthn_make_credential{dev=CableChannel}:user_verification: libwebauthn::webauthn: No supported PIN/UV auth protocols found get_info_response.pin_auth_protos=None thread 'main' panicked at libwebauthn/examples/webauthn_cable.rs:136:6: called `Result::unwrap()` on an `Err` value: Ctap(Other) note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace ``` So I modified `select_uv_proto` to allow empty an empty GetInfo `pinUvAuthProtocols` field if no UV is required. Now, the connection would get interrupted midway through the transaction. Checking `adb logcat`, I could see this is due to #95: ``` 05-05 13:48:22.616 7975 3808 E Fido : bfvv: the data in the message can't be decoded into CBOR 05-05 13:48:22.616 7975 3808 E Fido : at bfvm.f(:com.google.android.gms@251633035@25.16.33 (260400-750736914):2416) 05-05 13:48:22.616 7975 3808 E Fido : at bgty.a(:com.google.android.gms@251633035@25.16.33 (260400-750736914):235) 05-05 13:48:22.616 7975 3808 E Fido : at bgtr.run(:com.google.android.gms@251633035@25.16.33 (260400-750736914):640) 05-05 13:48:22.616 7975 3808 E Fido : at java.lang.Thread.run(Thread.java:1012) 05-05 13:48:22.616 7975 3808 E Fido : Caused by: gqbb: Error in decoding CborValue from bytes 05-05 13:48:22.616 7975 3808 E Fido : at gqbj.b(:com.google.android.gms@251633035@25.16.33 (260400-750736914):355) 05-05 13:48:22.616 7975 3808 E Fido : at gqbj.a(:com.google.android.gms@251633035@25.16.33 (260400-750736914):1) 05-05 13:48:22.616 7975 3808 E Fido : at gqbi.q(:com.google.android.gms@251633035@25.16.33 (260400-750736914):19) 05-05 13:48:22.616 7975 3808 E Fido : at bfvm.f(:com.google.android.gms@251633035@25.16.33 (260400-750736914):1318) 05-05 13:48:22.616 7975 3808 E Fido : ... 3 more 05-05 13:48:22.616 7975 3808 E Fido : Caused by: gqbb: Error in decoding CborValue from bytes 05-05 13:48:22.616 7975 3808 E Fido : at gqbj.b(:com.google.android.gms@251633035@25.16.33 (260400-750736914):355) 05-05 13:48:22.616 7975 3808 E Fido : at gqbj.b(:com.google.android.gms@251633035@25.16.33 (260400-750736914):243) 05-05 13:48:22.616 7975 3808 E Fido : ... 6 more 05-05 13:48:22.616 7975 3808 E Fido : Caused by: gqbb: Error in decoding CborValue from bytes 05-05 13:48:22.616 7975 3808 E Fido : at gqbj.b(:com.google.android.gms@251633035@25.16.33 (260400-750736914):355) 05-05 13:48:22.616 7975 3808 E Fido : at gqbj.b(:com.google.android.gms@251633035@25.16.33 (260400-750736914):305) 05-05 13:48:22.616 7975 3808 E Fido : ... 7 more 05-05 13:48:22.616 7975 3808 E Fido : Caused by: gqax: Keys in CBOR Map not in strictly ascending natural order: 05-05 13:48:22.616 7975 3808 E Fido : Previous key: "type" 05-05 13:48:22.616 7975 3808 E Fido : Current key: "alg" 05-05 13:48:22.616 7975 3808 E Fido : at gqbj.b(:com.google.android.gms@251633035@25.16.33 (260400-750736914):237) 05-05 13:48:22.616 7975 3808 E Fido : ... 8 more ``` So I rebased on top of `master` which includes fix #96. Then UX was encountering request timeouts, so I updated the code to support user-defined timeout values. Finally, pre-flight requests were timing out because Android shows UI for pre-flight requests. These are not supported, as they should be silent. ``` ERROR webauthn_get_assertion{dev=CableChannel}:ctap2_get_assertion: libwebauthn::transport::cable::channel: CBOR response recv timeout elapsed=deadline has elapsed timeout=2s DEBUG webauthn_get_assertion{dev=CableChannel}: libwebauthn::proto::ctap2::preflight: Pre-flight: Filtering out Ctap2PublicKeyCredentialDescriptor { id: [233, 220, 252, 82, 44, 76, 57, 246, 46, 170, 235, 207, 111, 237, 29, 118], type: PublicKey, transports: None }, because of error: Transport(Timeout) INFO webauthn_get_assertion{dev=CableChannel}: libwebauthn::proto::ctap2::preflight: Credential list AFTER preflight: [] WARN webauthn_get_assertion{dev=CableChannel}: libwebauthn::webauthn: Preflight removed all credentials from the allow-list. Sending dummy request and erroring out. ``` As pre-flights are an optional operation, they are now skipped for Hybrid transport authenticators.
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.
See #95 for more context.
CTAP canonical CBOR encoding requires map keys to be sorted from shorter key-length, to longer key-lenght.
No trivial root-cause fix due to serde_cbor, but thankfully the CBOR data structures with string keys in libwebauthn are limited.
Added tests to prevent regressions.
Test vectors
As decoded by cbor.me.
Ctap2CredentialType
Ctap2PublicKeyCredentialDescriptor