-
Notifications
You must be signed in to change notification settings - Fork 51
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
Documentation updates for release 1.11. #126
Conversation
Test Results: Windows 2 files 2 suites 4s ⏱️ Results for commit b47bcff. ♻️ This comment has been updated with latest results. |
Test Results: Ubuntu 2 files 2 suites 9s ⏱️ Results for commit b47bcff. ♻️ This comment has been updated with latest results. |
Test Results: MacOS 2 files 2 suites 3s ⏱️ Results for commit b47bcff. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some links and info, reworded things, and moved a few bullet points around.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of the new APDU articles hadn't made it into the TOC yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reworked this article to improve clarity and flow. Added lots of relevant links.
@@ -117,10 +120,12 @@ namespace Yubico.YubiKey.Piv | |||
/// management key (hex): 01 02 03 04 05 06 07 08 | |||
/// 01 02 03 04 05 06 07 08 | |||
/// 01 02 03 04 05 06 07 08 | |||
/// | |||
/// </code> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had to add these to fix a weird problem where the rest of the remarks section (lines 127-145) was formatted as a code box instead of a regular paragraph.
|
||
The YubiKey keeps track of failed biometric matches and will block the biometric authentication if there are more than three such failures. In that case the client should use the PIN verification as soon as possible. The number of remaining biometric matches is returned in the command response's `AttemptsRemaining` property. The value is present only after a failed match. | ||
The YubiKey keeps track of failed biometric matches and will block biometric authentication if there are more than three such failures. In that case, the client should use the [PIV PIN verification](#verify) as soon as possible. The number of remaining biometric verification attempts is returned in the command response's `AttemptsRemaining` property. The value is present only after a failed match. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming this was referring to the PIV PIN, but please correct if wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is correct 👍🏻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as soon as possible
is there a time aspect to this?
If not, perhaps it should read:
In that case, the client must use the PIV PIN verification in order to unblock the Yubikey and proceed operations.
Yubico.YubiKey/docs/users-manual/sdk-programming-guide/pin-complexity-policy.md
Show resolved
Hide resolved
- none - for checking the biometric state | ||
- 02 00 - if temporary PIN has been requested | ||
- 03 00 - if biometric verification has been requested | ||
- none - the biometric state has been checked |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should change this a little bit.
If the instruction is called without any data, it queries the YubiKey for the verification state of biometrics. In the case of unverified state, the YubiKey will respond with 63 CX
, in the case of a verified state 90 00
.
When the state is unverified, the biometrics should not be used, but the PIN verification should be used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. But isn't this addressed below in the document on line 55 and onwards?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the info about PIV PIN verification and a link to the relevant APDU page to line 59: 0d76f0b. Let me know if this is sufficient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reviewed the implementation documentation again and the addition in 0d76f0b could be improved, mentioning that: If all the biometric match retries have been used up, then a 0x6983 error is returned instead. That error is called AuthenticationMethodBlocked
.
For the line 25 in this review (where I marked this comment), the line is changed from "for checking the biometric state" to "the biometric state has been checked". I don't think this is accurate as when we call the instruction nothing has been checked yet - we are instructing the YubiKey to response to us what the current verification state of biometrics is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for catching that, Adam! The tense of the other two lines threw me.
Changes made here: ab28b45. Let me know if this covers it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks very good now :) Thank you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the changes, it looks good to me. Before merging, the verify-uv file for the "none" case could be updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work @equijano21 . I think it looks very good. It's like it actually makes sense to the end user now somehow... :)
@@ -22,9 +22,9 @@ CLA | INS | P1 | P2 | Lc | Data | Le | |||
00 | 20 | 00 | 96 | *variable* | *variable* | (absent) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
00 | 20 | 00 | 96 | *variable* | *variable* | (absent) | |
| CLA | INS | P1 | P2 | Lc | Data | Le | | |
|:---:|:---:|:--:|:--:|:----------:|:----------:|:--------:| | |
| 00 | 20 | 00 | 96 | *variable* | *variable* | (absent) | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The table seems to be missing the |s and correct number of padding columns making it not a fully valid markdown table. I have pasted a suggestion which should work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Weirdly enough, it doesn't seem to matter whether those extra "|" are there or not--the table still looks and behaves the same (when changing the window size). All of the tables in all of the PIV APDU files are formatted this way, but if you want me to change it, I can do it for everything. Just let me know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, no need. I guess that most markdown processors handle the invalid format still.
I'm in the process of doing in in another commit where I address all of the incorrectly formatted tables in all the other markdown files.
- none - for checking the biometric state | ||
- 02 00 - if temporary PIN has been requested | ||
- 03 00 - if biometric verification has been requested | ||
- none - the biometric state has been checked |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. But isn't this addressed below in the document on line 55 and onwards?
|
||
The YubiKey keeps track of failed biometric matches and will block the biometric authentication if there are more than three such failures. In that case the client should use the PIN verification as soon as possible. The number of remaining biometric matches is returned in the command response's `AttemptsRemaining` property. The value is present only after a failed match. | ||
The YubiKey keeps track of failed biometric matches and will block biometric authentication if there are more than three such failures. In that case, the client should use the [PIV PIN verification](#verify) as soon as possible. The number of remaining biometric verification attempts is returned in the command response's `AttemptsRemaining` property. The value is present only after a failed match. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as soon as possible
is there a time aspect to this?
If not, perhaps it should read:
In that case, the client must use the PIV PIN verification in order to unblock the Yubikey and proceed operations.
Yubico.YubiKey/docs/users-manual/sdk-programming-guide/pin-complexity-policy.md
Show resolved
Hide resolved
Thanks, Dennis! Really appreciate the feedback :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Improving sensitive-data.md to address #70
Description
Documentation updates for release 1.11.
Type of change
API doc (.cs) and user's manual doc (.md and .yml) updates.
How has this been tested?
Built docs locally.