Simplify PIV card driver #2448
Simplify PIV card driver #2448
Conversation
Simplify the code so that only the AID without version information is used for applet selection. Note that no other AID has ever been used.
- Separate detection of card type from card initialization; extract initialization of the private data from `piv_match_card_continued()` into `piv_init_priv()` - Clearify lock handling; the card is unlocked in the function where it is locked to avoid responsibility passing - `piv_activate()` makes sure that the PIV applet is selected during card matching and initialization.
`sc_pkcs15_read_certificate()` does both, read the file and cache it in memory, 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 APDUs maximum - 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 of the requested object
frankmorgner
force-pushed
the
simplify_piv
branch
from
253017e
to
1f09e30
Compare
|
@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. |
|
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 request introduces 1 alert when merging 720a22a into 33a22c8 - view on LGTM.com new alerts:
|
frankmorgner
force-pushed
the
simplify_piv
branch
from
720a22a
to
99325e6
Compare
|
This pull request introduces 1 alert when merging 99325e6 into 33a22c8 - view on LGTM.com new alerts:
|
|
This pull request introduces 1 alert when merging c9ade70 into 33a22c8 - view on LGTM.com new alerts:
|
I'm not in a hurry. We can merge these changes when SM is done. |
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. |
|
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... |
frankmorgner
force-pushed
the
simplify_piv
branch
from
c9ade70
to
1bb20fe
Compare
|
This pull request introduces 1 alert when merging 1bb20fe into 33a22c8 - view on LGTM.com new alerts:
|
Note that this removes the use of `sc_right_trim()`. There is no need to assume the keyfile contains non printable characters if it is hex encoded.
frankmorgner
force-pushed
the
simplify_piv
branch
from
1bb20fe
to
b572ca6
Compare
|
This pull request introduces 1 alert when merging b572ca6 into 2cc7b10 - view on LGTM.com new alerts:
|
Note: We skip checking for the agency identifier 9999 as it is not required by 800-73-4 anymore. Just use the GUID if its set and the FASC-N otherwise.
Note that SP800-73-4 is now clear on using tag 0x80 as encrypted challenge in mutual authentication, so we don't need to fall back to looking at 0x82 anymore.
|
This pull request introduces 1 alert when merging 5d319c6 into 2cc7b10 - 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