Skip to content

fix: release PIV connection during PIN prompts - avoid "the smart card has been reset" error#54700

Merged
Joerger merged 16 commits intomasterfrom
joerger/piv-reconnect-retry
May 23, 2025
Merged

fix: release PIV connection during PIN prompts - avoid "the smart card has been reset" error#54700
Joerger merged 16 commits intomasterfrom
joerger/piv-reconnect-retry

Conversation

@Joerger
Copy link
Copy Markdown
Contributor

@Joerger Joerger commented May 10, 2025

Changelog: Fix an issue with Hardware Key Support on Windows where a command would fail if the PIN prompt was not answered within 5 seconds.

Changes:

  • Instead of having piv-go check if PIN is required, always try signing without PIN to determine if PIN is required. This makes it possible to prompt for PIN and verify it in a new connection, which won't have timed out.
  • Refactor PIV signature method into steps. In addition to simplifying the logic considerably (e.g. decoupled PIN and touch prompts), this allows for the connection to be released/reconnected between retries. It also enables signatures to occur concurrently while still ensuring the prompts do not.
  • Refactor PIV connection sharing logic to enable any PIV request to reconnect and retry after the 5 second timeout
    • Removed this change as it bloated this PR and is generally not necessary now that the YubiKey connection is released during the PIN prompt. However, it is still theoretically possible to get the "the smart card has been reset" error and this PR adds no way to reconnect and retry. This can be addressed in a follow up PR.

Please see go-piv/piv-go#172, go-piv/piv-go#173, and go-piv/piv-go#174 for more context.

Fixes #54332

@github-actions github-actions Bot requested review from EdwardDowling and tigrato May 10, 2025 02:11
@Joerger Joerger force-pushed the joerger/piv-reconnect-retry branch 2 times, most recently from 6697809 to d04e686 Compare May 12, 2025 20:50
@Joerger Joerger marked this pull request as draft May 13, 2025 02:12
tigrato
tigrato previously approved these changes May 13, 2025
@tigrato tigrato self-requested a review May 13, 2025 21:28
@tigrato tigrato dismissed their stale review May 13, 2025 21:29

re-drafted

@Joerger Joerger force-pushed the joerger/piv-reconnect-retry branch from fb46c6e to 7f7ae68 Compare May 14, 2025 18:53
@Joerger Joerger changed the base branch from master to joerger/skip-validate-piv-cert May 14, 2025 18:55
@Joerger Joerger force-pushed the joerger/piv-reconnect-retry branch from 7f7ae68 to 6f52942 Compare May 14, 2025 18:55
@Joerger Joerger marked this pull request as ready for review May 16, 2025 01:22
@github-actions github-actions Bot requested review from capnspacehook and kimlisa May 16, 2025 01:23
@kimlisa kimlisa removed their request for review May 16, 2025 03:12
@Joerger Joerger requested a review from rosstimothy May 16, 2025 18:19
@Joerger Joerger force-pushed the joerger/piv-reconnect-retry branch from d0e33c2 to 9c1f726 Compare May 16, 2025 18:21
Base automatically changed from joerger/skip-validate-piv-cert to master May 16, 2025 18:59
@Joerger
Copy link
Copy Markdown
Contributor Author

Joerger commented May 16, 2025

Reviewers, I took out a substantial portion of the initial changes (the retry on pcscResetCardErr logic), see the PR description for details. I'm working on removing some outdated comments about the pcscResetCardErr, but otherwise it's ready for review. Sorry if any of you were in the middle of reviewing.

- Add exclusiveOperationMu mutex and test.
@Joerger Joerger force-pushed the joerger/piv-reconnect-retry branch from 59dcc34 to 91846da Compare May 16, 2025 20:02
@Joerger Joerger changed the title fix: retry signature on "the smart card has been reset" error fix: release PIV connection during PIN prompts - avoid "the smart card has been reset" error May 16, 2025
Comment thread api/utils/keys/piv/yubikey.go Outdated
Comment thread api/utils/keys/piv/yubikey.go Outdated
Comment thread api/utils/keys/piv/yubikey.go
@Joerger Joerger force-pushed the joerger/piv-reconnect-retry branch from bc2dedb to 9c434b4 Compare May 19, 2025 20:25
@Joerger Joerger requested a review from rosstimothy May 19, 2025 21:01
Comment thread api/utils/keys/piv/yubikey.go Outdated
Comment thread api/utils/keys/piv/yubikey.go Outdated
Comment thread api/utils/keys/piv/yubikey.go Outdated
@Joerger Joerger requested review from rosstimothy and tigrato May 21, 2025 17:17
Comment thread api/utils/keys/piv/yubikey.go Outdated
@Joerger Joerger requested a review from rosstimothy May 22, 2025 00:01
@Joerger
Copy link
Copy Markdown
Contributor Author

Joerger commented May 22, 2025

@tigrato friendly ping to review

@public-teleport-github-review-bot public-teleport-github-review-bot Bot removed the request for review from capnspacehook May 23, 2025 17:12
@Joerger Joerger added this pull request to the merge queue May 23, 2025
Merged via the queue into master with commit 7c975ce May 23, 2025
41 checks passed
@Joerger Joerger deleted the joerger/piv-reconnect-retry branch May 23, 2025 19:03
@backport-bot-workflows
Copy link
Copy Markdown
Contributor

@Joerger See the table below for backport results.

Branch Result
branch/v16 Create PR
branch/v17 Create PR

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

PIV PIN prompt silently times out after several seconds (Windows)

3 participants