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

Automatically verify PIN in sign_data to support always-auth keys #326

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

qpernil
Copy link
Contributor

@qpernil qpernil commented Oct 27, 2021

Verify PIN in sign_data to be able to use always-authenticate keys in commands that perform more than one operation, potential fix for #321. This means -averify-pin is not needed any more, any command that signs will verify (and prompt for if not given already) the PIN automatically. The action is retained in case someone wants to just verify the PIN.

@mouse07410
Copy link

Could you please confirm that this PR would automatically prompt for PIN only when the token returns NOT_LOGGED_IN, as opposed to when the driver thinks it should...?

@qpernil
Copy link
Contributor Author

qpernil commented Dec 1, 2021

This is a simplistic solution, simply verifying pin everytime something tries to perform a signature. The pin will be taken from the command line if given, otherwise it will be requested (once). Previously there was no automatic pin verification, you had to specify -averify-pin on the command line to actually verify the pin. The --pin option would only specify the pin value to be used by -averify-pin, not verify it on it's own. So -averify-pin would seem to be able to solve the problem, but the issue was that other commands would perform more than one command with the YubiKey, invalidating the PIN again for always-auth keys. With this solution there will be too many PIN verifications for normal keys, but that is not really a problem as the pin will be kept in memory. This whole reasoning only applies to yubico-piv-tool and not libykcs11.

@qpernil
Copy link
Contributor Author

qpernil commented Dec 2, 2021

See also #338. Still considering security implications of that one.

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

Successfully merging this pull request may close these issues.

2 participants