Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

signer: Handle the "yubikey auth required" case #528

Merged
merged 1 commit into from
Feb 3, 2025

Conversation

jku
Copy link
Member

@jku jku commented Jan 24, 2025

There is now light infrastructure for handling PKCS errors... I'm only handling the one that seems to trip people up since there are so many possible errors: we can't possibly do a good job of handling all of them.

Couple of review notes:

  • I can't directly test this one: The touch policy on a yubikey can only be set when the certificate is created and and the touch policy on my keys is the default "never" -- I'd have to overwrite my key or buy a new yubikey for this. I have tested the code by adding handling for another case (CKR_ARGUMENTS_BAD) and that works fine so I trust that this one works too
  • I was a little surprised I have to access a private field (e.__context__) to get the original exception... but that seems the only way

Fixes #527

There is now light infrastructure for handling PKCS errors...
I'm only handling the one that seems to trip people up since there are
so many possible errors: we can't possibly do a good job of handling all
of them.
@jku jku requested a review from kommendorkapten as a code owner January 24, 2025 09:54
Copy link
Member

@kommendorkapten kommendorkapten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good. I don't have a yubi with touch policy either, co can not test.

@jku jku merged commit 8fc209a into theupdateframework:main Feb 3, 2025
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve error message if touch is not provided
2 participants