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

Add ability to change allowed number of PIN and PUK retries #155

Merged
merged 1 commit into from
Aug 26, 2024

Conversation

Quantu
Copy link
Contributor

@Quantu Quantu commented Jul 9, 2024

Add the ability to change PIN/PUK retries using SetRetries. Changing the retries does have the side effect on yubikeys of resetting the PIN and PUK to their default values, so I made sure that is very clear in the comments and README.

@Quantu Quantu force-pushed the add_setting_pin_puk_retries branch 3 times, most recently from 46a6e69 to 8017ede Compare July 10, 2024 18:24
@Quantu
Copy link
Contributor Author

Quantu commented Jul 10, 2024

I considered having SetRetries then call ykChangePIN and/or ykChangePUK after calling ykSetRetries if relevant (if PIN or PUK differ from piv.DefaultPIN or piv.DefaultPUK), but that seems a bit too "magic". It also requires you pass the PUK in to a function that doesn't need it for any sort of authentication, which is a little messy. It would just set the PUK to the one provided without validating it is the current PUK.

Leaving the behavior of changing retries resetting the PIN and PUK is in line with Yubico's documentation, as well as the behavior you get when doing the same operation with yubico-piv-tool.

@Quantu
Copy link
Contributor Author

Quantu commented Aug 22, 2024

@ericchiang I know you've been really busy, but if you get a chance to look at this at some point that'd be greatly appreciated. I've been using this already, but it would be really nice to no longer have to use my fork.

Copy link
Collaborator

@ericchiang ericchiang left a comment

Choose a reason for hiding this comment

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

Apologies and thanks for the ping. Looks great! just a single comment. If you want to address that and squash I can merge

v2/piv/piv.go Outdated Show resolved Hide resolved
@Quantu Quantu force-pushed the add_setting_pin_puk_retries branch from 8017ede to 7ee631b Compare August 26, 2024 13:31
@Quantu
Copy link
Contributor Author

Quantu commented Aug 26, 2024

@ericchiang please take a look again, it should be good now.

@ericchiang
Copy link
Collaborator

Thanks! I unfortunately am seeing "This branch cannot be rebased due to too many changes" on my end. Does this have any conflicts with the v2 branch?

@Quantu Quantu force-pushed the add_setting_pin_puk_retries branch from 7ee631b to 4811d50 Compare August 26, 2024 14:45
@Quantu
Copy link
Contributor Author

Quantu commented Aug 26, 2024

Yeah, I had this sitting around before the v2 release and I missed the commit updating the readme. I rebased against v2 and it should be good now.

@ericchiang ericchiang merged commit 2cbba92 into go-piv:v2 Aug 26, 2024
4 checks passed
@ericchiang
Copy link
Collaborator

Thanks for your patience here!

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.

2 participants