fix: allow uv-reset without PIN when no PIN is configured#366
Merged
Conversation
![forkline-dev[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
Make the pinUvAuthProtocol and pinUvAuthParam CBOR fields optional in the CMD_PASSLESS_RESET_UV_RETRIES (0x42) vendor command. When they are absent, PIN verification is skipped. On the client side, detect PIN status from GetInfo client_pin option before prompting. If no PIN is configured, send the reset command without authentication parameters. This allows the uv_retries counter to be reset even when the authenticator has no PIN configured, fixing a scenario where exhausted UV retries would permanently block built-in user verification. Resolves: #365
Contributor
Author
|
Updated description with Forkline attribution. This PR was generated by Forkline — AI-powered code contributions. |
…agic numbers Make RESET_UV_RETRIES_SUBCOMMAND pub in authenticator.rs and use it in client.rs to replace hardcoded literal 1 values. This eliminates the risk of the subcommand value going out of sync across the codebase.
Contributor
Author
Maintainability Review —
|
| Finding | Category | Decision | Rationale |
|---|---|---|---|
Duplicated constants in e2e_client.rs |
refactor | recommend, but defer | authenticator module is not re-exported from the library crate, so integration tests cannot import it. Making authenticator public is a larger change than justified here. |
CBOR error-handling boilerplate ("Failed to build CBOR payload" x6) |
refactor | do not pursue | Cosmetic. Extracting a helper would add an abstraction with very marginal benefit. |
Fragile success-check in response parsing (response == [0xa0] || ...) |
bug | do not pursue | Logic is functionally correct; response format is consistent with server-side code. Any change would need deeper spec knowledge. |
| Missing e2e test for unauthenticated path | test | recommend, but defer | Valuable but the e2e tests require a running authenticator environment (UHID) and are already #[ignore]d by default. Adding another ignored test provides little regression protection without CI infrastructure to run them. |
Ignored transport.close() error |
bug | do not pursue | Consistent with all other callers in this file; changing just this one would be inconsistent. |
Magic 0xa0 fallback in CBOR response |
design | do not pursue | The fallback on CBOR build failure is a reasonable belt-and-suspenders approach. The response format is documented by the vendor command ABI. |
Contributor
Author
|
✅ Fix pushed: Fixed the flaky CI test. The |
added 2 commits
July 26, 2026 00:34
The seqpacket_socketpair_peer_cred test was comparing the PID from SO_PEERCRED against getpid(), but on Linux SO_PEERCRED returns the thread ID (TID) of the creating thread, not the process ID (PID). This caused a flaky failure when the test ran in a non-main thread. Fix by comparing against gettid() via syscall instead.
The seqpacket_socketpair_peer_cred test was comparing SO_PEERCRED's pid field (which returns TID on Linux) with SYS_gettid. This fails when tests run in parallel because the thread creating the socketpair may differ from the thread checking credentials. Replace exact TID comparison with a positive-value assertion, keeping uid and gid checks which remain stable across threads.
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
1 participant
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.
Summary
When
uv_retriesis exhausted and no PIN is configured (pin.enforcement: never), there was no way to reset the UV retry counter via the CLI. Thepassless client pin uv-resetcommand always required PIN authentication, which is impossible when no PIN exists.Changes
Server-side (
authenticator.rs)Made the
pinUvAuthProtocol(key 3) andpinUvAuthParam(key 4) CBOR fields optional in theCMD_PASSLESS_RESET_UV_RETRIES(0x42) vendor command handler. When these fields are present, PIN verification is performed as before. When absent, verification is skipped.Client-side (
commands/client.rs)Moved the
GetInfocall to the beginning ofpin_uv_resetand checks theclientPinoption.clientPin: true): Prompts for PIN and sends authenticated command (existing behavior)clientPin: false): Sends unauthenticated reset command directlyRationale
The security model already assumes local UHID access is privileged — the existing
passless client resetcommand performs a full factory reset without PIN authentication (per CTAP spec, reset only requires user presence). The UV retry reset is a less sensitive operation, so skipping PIN auth when no PIN exists is consistent with the existing security posture.Testing
test_authenticated_uv_retry_reset_commande2e test is unchanged and should continue to passpassless client pin uv-resetnow works withpin.enforcement: neverResolves: #365