- Notifications
You must be signed in to change notification settings - Fork799
Simplify PIV card driver#2448
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:master
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Simplify the code so that only the AID without version information isused for applet selection. Note that no other AID has ever been used.
- Separate detection of card type from card initialization; extractinitialization of the private data from `piv_match_card_continued()`into `piv_init_priv()`- Clearify lock handling; the card is unlocked in the function where itis locked to avoid responsibility passing- `piv_activate()` makes sure that the PIV applet is selected duringcard matching and initialization.
`sc_pkcs15_read_certificate()` does both, read the file and cache it inmemory, so no need to duplicate this.
- let sc_format_apdu_ex decide APDU case (should always be 3/4, because piv_general_io is always called with something to send or receive)- no need to force the Le to be 256 max, because the lower level do recognize that PIV only supports short length and truncate the Le accordingly- let sc_transmit_apdu() do the card locking- honour the reader's maximum transceive size when setting Lc/Le
- typically, 8 bytes returned no need to request more than short APDUsmaximum- don't use goto if no cleanup is done
saves build time and space in binary
no need to iterate over all objects, if we know the exact location ofthe requested object
dengert commentedDec 1, 2021
@frankmorgner You have a lot of changes to PIV driver. If you feel these are really worth committing, go ahead. I don't have much time over the next month and a half to look at these. |
mouse07410 commentedDec 1, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
PIV driver evolved with workarounds for many weird behavior aberrations manifested in different tokens on the market. By now probably even@dengert doesn't remember exactly what was done for what specific problem. It would be unfortunate if these simplifications "simplified out" a workaround, whose effect would surface several months after deployment, and then take several more months to hunt and fix (again). After people already spent plenty of time and efforts to nail those down - and who has free time nowadays to engage in such an effort again? This is my major concern. I understand the value of the code being manageable with less efforts. But I prefer better-working and less-manageable to very-manageable and broken. |
This pull requestintroduces 1 alert when merging720a22a into33a22c8 -view on LGTM.com new alerts:
|
This pull requestintroduces 1 alert when merging99325e6 into33a22c8 -view on LGTM.com new alerts:
|
This pull requestintroduces 1 alert when mergingc9ade70 into33a22c8 -view on LGTM.com new alerts:
|
frankmorgner commentedDec 3, 2021
I'm not in a hurry. We can merge these changes when SM is done. |
frankmorgner commentedDec 3, 2021
Your concerns are heard. I have local tests and we have CI tests. Feel free to run some tests yourself or (better) extend the CI tests with the missing use cases. |
mouse07410 commentedDec 3, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
L> Your concerns are heard. I appreciate it.
I strongly doubt that every corner case and every workaround implemented during the life of this driver has a corresponding test. It would probably be impossible to even write such a test for a generic PIV - it would likely require the same specific physical token, for which the workaround was implemented.
There are two problems. One is time - the same reason you'd rather spend less time and efforts on maintaining the code. I simply don't have enough time to become a co-maintainer of the PIV driver (and very likely don't have enough expertise - my primary interest was getting the whole thing to work with the PIV and CAC tokens). If the purpose of this driver (and OpenSC at large) was to validate various tokens, it would be trivially easy: just implement the strict PIV, and if a token fails - is a bad non-compliant token. But the goal is to make all these non-compliant (to different degrees) tokens work. As you yourself remember, it was pretty hard... I for one don't have time now to redo all that work... And I can't even judge the changes, saying without having to offer supporting evidence: this change is OK but that one is likely to break token X... |
This pull requestintroduces 1 alert when merging1bb20fe into33a22c8 -view on LGTM.com new alerts:
|
Note that this removes the use of `sc_right_trim()`. There is no need toassume the keyfile contains non printable characters if it is hexencoded.
This pull requestintroduces 1 alert when mergingb572ca6 into2cc7b10 -view on LGTM.com new alerts:
|
Note: We skip checking for the agency identifier 9999 as it is notrequired by 800-73-4 anymore. Just use the GUID if its set and theFASC-N otherwise.
Note that SP800-73-4 is now clear on using tag 0x80 as encryptedchallenge in mutual authentication, so we don't need to fall back tolooking at 0x82 anymore.
This pull requestintroduces 1 alert when merging5d319c6 into2cc7b10 -view on LGTM.com new alerts:
|
This PR is work in progress. The changes are meant to simplify the PIV card driver, more commits may be added later... Feedback is welcome at any time.
Checklist