PIV detection of AID using Discovery Object before doing select AID - Partial Fix #1243 #1256
Conversation
|
|
||
| /* Some objects will only be present if Histroy object says so */ | ||
| for (i=0; i < PIV_OBJ_LAST_ENUM -1; i++) | ||
| if(piv_objects[i].flags & PIV_OBJECT_NOT_PRESENT) | ||
| priv->obj_cache[i].flags |= PIV_OBJ_CACHE_NOT_PRESENT; | ||
|
|
||
| sc_lock(card); |
There was a problem hiding this comment.
You should not assume that init() is always followed by match_card(), please remove this lock. In the worst case you need to re-do in init() what you've already sent in match_card().
There was a problem hiding this comment.
I think it is the other way around. A successful piv_match_card is always followed by piv_init
It also assumes card->dvr_data allocated in a successful piv_match_card is passed to piv_init
By setting card->dvr_data in piv_match allows any piv routines t have access to a valid ``card->dvr_data`
The lock in line 3048 in piv_init is unlocked in:
3204 sc_unlock(card) ; /* obtained in piv_match */
There was a problem hiding this comment.
Yes, please don't do it. If you want a valid card->drv_data while executing piv_match_card, then sc_lock and sc_unlock in piv_match_card.
There was a problem hiding this comment.
The point was to pass from match to init both the card->drv_data and the lock so any commands done during match don't have to be reissued again, and there can be no interference from other processes between match and init.
We really need to look at how match is done for applications. PIV and OpenPGP are really applications drivers and we need to have a way to support them better. See private note from Martin I forwarded to you about. https://github.com/makinako/OpenFIPS201
There was a problem hiding this comment.
I know that this locking avoids issuing an additional SELECT. At some point I've done this, too. However, match_card was designed to not allocate or lock any internal resources. Other Applet-based card drivers have the same problem and they simply issue an additional SELECT. You can avoid the SELECT in match_card if you maintain a list of ATRs of cards that are known to have a PIV applet.
Also, locking is typically done in the PKCS#15 or the PKCS#11 layer, since you have more knowledge about the context that defines what is a transaction. In my opinion the locking should happen where unlocking is done, too. Otherwise we will loose track at some point and lock the token forever. So please do this only when you're forced to.
There was a problem hiding this comment.
Comment on a few comments in this thread - It is Dangerous to base any PIV (Or other) "card application" identification on ATR alone (or at all), since most cards now support multiple application "Applets" and come with scores of applications in "ready to install" ROM. PIV requires only one (registered) RID/AID and supports a discovery object, other schemes support other registered/standardised methods. People need to use the method set out in the standards/doco rather than trying to second guess by the (albeit easier) ATR. In fact ATR should only be a last worst case option, and it only workled in the past because most cards were single application (which has now changed).
There was a problem hiding this comment.
I can't find any statement that says you should use an ATR. But, as a matter of fact, some cards come with a specific ATR, so one may use it. As example look at SC-HSM's implementation:
static int sc_hsm_match_card(struct sc_card *card)
{
sc_path_t path;
int i, r, type = 0;
i = _sc_match_atr(card, sc_hsm_atrs, &type);
if (i >= 0 && type != SC_CARD_TYPE_SC_HSM_SOC) {
card->type = type;
return 1;
}
sc_path_set(&path, SC_PATH_TYPE_DF_NAME, sc_hsm_aid.value, sc_hsm_aid.len, 0, 0);
r = sc_hsm_select_file(card, &path, NULL);
LOG_TEST_RET(card->ctx, r, "Could not select SmartCard-HSM application");
if (type == SC_CARD_TYPE_SC_HSM_SOC) {
card->type = SC_CARD_TYPE_SC_HSM_SOC;
} else {
card->type = SC_CARD_TYPE_SC_HSM;
}
return 1;
}
It's fast in some cases (matching an ATR), and it's reliable in all of the cases (sometimes needing a SELECT).
src/libopensc/card-piv.c
Outdated
| * putting PIV driver first might help. | ||
| * TODO could be cached too | ||
| */ | ||
| i = piv_find_discovery(card); |
There was a problem hiding this comment.
Are you sure that this object is present on the Yubikey? NIST Special Publication 800-73-4 says it is optional an was not present in the initial release...
There was a problem hiding this comment.
It is present on the 4 Yubico 4.2.6. Even if it is not, the the code goes ahead and tries to do a SELECT AID.
3063 if (i < 0) {
3064 /* Detect by selecting applet */
3065 i = piv_find_aid(card, &aidfile);
3066 }
By trying the piv_find_discovery first it avoids the Yubico issue of resetting the security state even
when selecting the same AID.
There are still problems in that other drivers try to select their AID before the PIV driver gets a chance to see if the PIV is the active application. So to get the full benefit of this
OPESC_DRIVER=PIV-II is needed, and a few extra fixed by Yubico.
There was a problem hiding this comment.
OK, if it's present and does its job, fine.
Just wanted to remind you that the data from the Application Property template is still an option. Note that this is returned from SELECT and GET DATA, i.e. you don't need an extra command to retrieve the full AID on initialization, for example.
There was a problem hiding this comment.
I got a new Yubikey version 4.3.7 yesterday, and it does have a Discovery object.
The point of using the discovery object first is it returns the active AID or fails without
changing the security status. Using a Select AID on Yubico and some other cards causes problems even when the active application is selected again. Yubico has this problem,also see
makinako/OpenFIPS201#1
I think we should be able to allow the user to "enter the PIN in once", to unlock a card, and use the card from other processes without having to reenter the PIN. Using disconnect=leave, and using the discovery object rather SELECT AID will allow this. This will work with single application cards.
But not with current multiple application cards that by policy (PPIV) say if a different application on the same card is selected, the security status is post. So some of the changes are trying to address both multiple and single application cards.
There are other places in OpenSC and code out of our control that interfere with "enter the PIN in once". This includes other OpenSC card drivers using Select AID to test if the card supports their application. A compliant PIV card does not have that problem as NIST 800-73-3 says it ignores SELECT AID for unknown AIDS. But the Yubico does not. Other cards may have the same issue. So OpenSC when testing for a card to not cause issues for other processes or other drivers.
The use of the Discovery object tries to do the above.
There was a problem hiding this comment.
That's a misunderstanding. My suggestion was to
- use SELECT once in piv_init to select the applet and to get the full Application Property template.
- detect the PIV applet with GET DATA for the AID (4F, I believe) in every call of piv_card_reader_lock_obtained
There was a problem hiding this comment.
Even doing a SELECT once can cause the card to lose the login state. The discovery object '7E'
has as the AID. Its enough to show the PIV application is the active application and it does not change the login state. If it fails, the PIV is not the active.
The PIV does not support the '4F' It is not required. I was trying that with the script I sent last week. and none of the cards I have support '4F'
That is why I use the '7E' discovery object that every PIV card I have has one.
I use the get data of the discovery object in every call of piv_card_reader_lock_obtained
Since the OpenPGP has the '4F` it can use that to test. Itr will fail if PIV is active.
src/libopensc/card-piv.c
Outdated
| piv_finish(card); | ||
| /* don't match. Does not have a PIV applet. */ | ||
| sc_unlock(card); | ||
| return 0; |
There was a problem hiding this comment.
OK, fixed and will be in next commit.
Note I did a commit earlier to handle the CKU_CONTEXT_SPECIFIC login.
There was a problem hiding this comment.
Still I see there one space after the two tabs in the above three lines. The block above is indented with tab and 4 spaces.
src/libopensc/card-piv.c
Outdated
| data.pin_type = SC_AC_CHV; | ||
| data.pin_reference = priv->pin_preference; | ||
| /* will try our best to see if logged_in or not */ | ||
| r = piv_pin_cmd(card, &data, NULL); |
There was a problem hiding this comment.
unused assignment.
Why do you need to check for the PIN at all? Maybe only in the case of a reset?
There was a problem hiding this comment.
That is trying to keep the card->logged_in state correct. With the Yubico it is easy to lose the state.
For example: Assuming disconnect=leave
Current process is using PIV application and logged it.
Other process started and some driver sets the AID to something else, and then sets it to
PIV.
Our process uses discovery (7E) to see the PIV is active, but it does not know if the login state has changed unless it tests. The is the test.
There was a problem hiding this comment.
I think this command is not needed. Simply set the PIV's internal tracking to SC_PIN_STATE_UNKNOWN if the applet has been deselected or reset. An application that wants to know the PIN status directly or indirectly calls sc_pin_cmd with SC_PIN_CMD_GET_INFO; this issues the VERIFY command if the card supports it.
Note that we should try to remove the driver specific tracking and put this into the PKCS#11 specific tracking. (i.e. we need to propagate a reset, for example). We should not do a double entry-bookeeping.
There was a problem hiding this comment.
OK, will look at that closer.
There was a problem hiding this comment.
Did you try piv_card_reader_lock_obtained without the PIN status query? I still think that's an superfluous APDU, but if you think that's necessary then please remove the unused assignment (which static code analysis will complain about).
|
@mouse07410 @frankmorgner |
|
I don't like 1d05d89, because it modifies all layers for an application specific problem (PKCS#11) that the driver should normally not be interested in. If I had to implement this, I'd set an additional lock if the signature PIN shall be verified, because the driver knows that no other interfering commands must be sent. I think for the driver there is no need to know any detail of PKCS#11. |
|
You said: " if the signature PIN shall be verified". In the PIV case there is only one PIN. So how can the card driver know it's for a signature? As I said, in the commit: "A card driver pin_cmd code can ignore this flag or can take some action." I could look at the pkcs15 layer with user consent. But that is on the key. Its only when PKCS11 realizes the CKA_ALWAYS_AUTHENTICATE requires a C_Login(CKU_CONTEXT_SPECIFIC) Its up to the PKCS11 calling application to to the C_Login(CKU_CONTEXT_SPECIFIC) followed by the C_Sign or other operation. 1d05d89 tries to do the above and for the PIV card, expects the C_Login(CKU_CONTEXT_SPECIFIC) followed by the C_Sign but if it does not, it will release the lock when it is it sees some other APDU complete. |
Ah, I understand. But how can you tell from |
|
PKCS#11 says if the the key has CKA_ALWAYS_AUTHENTICATE then C_Login(CKU_CONTEXT_SPECIFIC) must by used before use of the key with any of the crypto operation. The mod has some code in pkcs15-sec.c use_key. If it sees a card driver has done the sc_lock before the verify, that it does not need to do a lock. I believe pkcs15-sec.c use_key is called by all the crypto routines. More later.... |
|
Thanks, for the pointer. The PKCS#11 description, indeed, looks good:
Regarding the implementation, I think it would be better to add a new Access Control flag for a context specific verification in |
|
That could work. |
|
No, you would use |
|
Ok will have a look. Will other drivers need to onderstand the
SC_AC_CONTEXT?
On Jan 31, 2018 2:31 PM, "Frank Morgner" <[email protected]> wrote:
No, you would use sc_pkcs15_verify_pin as is, but you need to change pin_obj's
auth_info to auth_info->auth_method = SC_AC_CONTEXT before. This should
then be passed to piv_pin_cmd as data->pin_type. Just check out how the
Minidriver is initializing usage of a session PIN
<https://github.com/OpenSC/OpenSC/blob/master/src/minidriver/minidriver.c#L5650-L5670>
.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1256 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AA00MRJA_hxLU7G-OnzMi871LaIAea7oks5tQM2KgaJpZM4RxCUr>
.
|
Yes, but in case of the session PIN it was enough to modify iso7816.c, where
Maybe we should actually do the opposite, i.e. create a context specific transaction by locking the card on the PKCS#11 level and don't unlock it until the next (cryptographic) operation is completed. I think this should work with the current uses (without locking the token too long and with fixing the PIV's PIN issue) and doesn't require any other change than in @Jakuje, @dengert do you know whether |
|
The |
|
I have reverted the previous 1d05d89 and using the suggestions from @frankmorgner written 906df5f I believe this also addresses @Jakuje comments, as the extra lock added before the verify is released after the verify fails or the next command sent to the card completes. |
|
@mouse07410 Tis has not been rebased yet. I am waiting on comments from @frankmorgner and @Jakuje |
src/libopensc/card-piv.c
Outdated
| @@ -2992,19 +3036,47 @@ static int piv_match_card(sc_card_t *card) | |||
| if (card->type == -1) | |||
| card->type = type; | |||
|
|
|||
| card->drv_data = priv; | |||
| card->drv_data = priv; /* will frre if no match, or pass on to piv_init */ | |||
dengert
force-pushed
the
piv-aid-discovery
branch
from
7599076
to
77a974b
Compare
Actually, in Anyway, I think that doing the locking in the card driver would be the better option, since this seems quite specific to the PIV card. |
|
|
||
| /* If not valid, read, cache and test */ | ||
| if (!(priv->obj_cache[PIV_OBJ_DISCOVERY].flags & PIV_OBJ_CACHE_VALID)) { | ||
| r = piv_process_discovery(card); |
There was a problem hiding this comment.
The comment above states
/* Do not use the cache value but read every time */
But here you're reading the cached value, right?
There was a problem hiding this comment.
No there is a "!" to say if the cache in not valid, call the piv_process_discovery. This routine will test if the cache is valid or not (in this case it will not be valid) then read from the card add it to the cache and parse it find the AID. So the first time the discovery object is needed it will also get cached.
If the cache was valid, then the "else" in line 2685 will force the discovery object to be read from the card. piv_parse_discovery will then make sure the AID in the data read is the PIV AID, thus proving that the PIV application is active.
There was a problem hiding this comment.
The comment above the function let me believe that the function should "not use the cache value", but in your reply (and in your code) you're accessing the cache. I'm just saying that there's an inconsistency. Maybe you just need to change the comment in the code.
There was a problem hiding this comment.
The piv_process_discovery(card); when called will return a cached value if it is cached. If not it will read the discovery object from the card and also addit it to the cache.
So the line if (!(priv->obj_cache[PIV_OBJ_DISCOVERY].flags & PIV_OBJ_CACHE_VALID))
firsts test if it is NOT cached, then calls piv_process_discovery(card);
So in effect the discovery object will be read and also added to the cache.
The code is working as intended.
There was a problem hiding this comment.
Could you then please remove or change the misleading comment /* Do not use the cache value but read every time */?
There was a problem hiding this comment.
Well ... the code and comment makes sense to me. The note, that we are reading the value always is already in the previous comment ([...] So we can not use the cache). This comment is supposed to explain just the two following lines, which it does.
The thing is that if the cache is invalid, we call piv_process_discovery() and it calls piv_get_cached_data(), which in the result reads the data from card again regardless the name of the function (we have invalid cache data or the object was not read yet).
I agree that the comment or the code can be little bit more clear.
But can not we explicitly invalidate the cache and then reduce this block to the call to piv_process_discovery() anyway (that is what we do in the else branch also, isn't it?). Would not it be more readable?
/* invalidate the cache and read the object from card */
priv->obj_cache[PIV_OBJ_DISCOVERY].flags &= ~PIV_OBJ_CACHE_VALID;
r = piv_process_discovery(card);
(I did not test this code, it might have some side effects or issues, but looks much more understandable and we even get some more debug information)
|
The lock_card may or may not be turned. The new code in piv_pin_cmd will make sure that the lock is on before the verify is sent and turned off if the verify fails or after the next command after a successful verify. |
dengert
changed the title
dengert
force-pushed
the
piv-aid-discovery
branch
from
77a974b
to
8cdd512
Compare
|
This PR has been rebased on master. Master now includes #1243 Except for |
src/libopensc/card-piv.c
Outdated
| } | ||
|
|
||
| /* make sure our application is active */ | ||
|
|
||
| /* first see if AID is active AID be reading discovery obkect '7E' */ |
|
@Jakuje FIxed typoes, spaces and tabs |
dengert
force-pushed
the
piv-aid-discovery
branch
from
7898a96
to
aee62c7
Compare
|
I have rebased and squashed the 13 commits down to 5 without reordering. Thus no code was changed, just rebased. @frankmorgner I believe I have addressed all of your issues. So again I would like to see this PR committed. @mouse07410 and @Jakuje have also requested that this PR be committed. It need to be in 0.18.0. |
|
I agree. I was just referring to this PR from OpenSSH bug [1] which seems to get resolved by this PR (in addition to disconnect_action=leave) too so everything will not be for worse in OpenSSH with a new version (though I will have to update my authorized_keys). |
|
@dengert you missed the comment about We can discuss on removing |
I don't know, and would rather not mess with the code that's working. One introduced bug is enough. I also want to add that this PR (the way it is now) addressed one problem I had with SSH. So I request that any more changes are done via different PRs. |
|
https://github.com/frankmorgner I DID FIX THIS. 10 days ago 990f067 and it is now rebased as aee62c7 with a better commit message.
AND piv_init can now be called without piv_match_card having been called. I was not arguing that *_match_card was useful or not. I was arguing that if you are going to call both match_card and init, I did not want to duplicate card commands in both. Some cards need to do a lot of setup before doing any card commands and the only way to pass information from I addressed your issue of the |
|
@dengert you can find the comment above marked with a red sign as requested changes. Basically, you should not match always, instead use proper detection (SELECT or ATR). |
|
You need to consider that most new gen cards (NXP P60 and P80 are good examples) are designed for multiple markets, so the same card is distributed with multiple applets or new ones are soft loaded, (In the case of P60/80 ROM – multiple applets can be enabled via a licence key from ROM) . So the base card has a fixed ATR whilst each applet will have its own AID that it is installed from ROM into the desired AID according to the registered RID.
Therefore a card that supports EMV + PIV + a PKCS#11 proprietary applet + PLAID + DESFire (all with different ISO registered RIDs and AIDs) is possible.
If you are only testing the ATR, you may get problems – using select of the specific applet – you can be certain., using application based ID data (in PIV for example) you can be 100% version specific.
We recently had exactly this problem in Defence, with Windows 10 using ATR alone, which meant it missed the PIV applet on all newer cards, and looked for PKCS#11 applet that did not exist, Microsoft have now issued an interim patch and will be fixing the problem more broadly with a version which checks for PIV directly.
Cheers
__________________________________________________
Graeme Freedman
DotInDots
Smart Card Technical Consultancy & Development
Security Product Development, Licensing & Manufacture
39 Link Rd WANDELLA, NSW, 2550 Australia
Mobile: +61 (0) 4031 13624
Phone: +61 (02) 9983 9777
Skype: Graeme.Freedman
mailto:[email protected]
http:https://www.dotindot.com <http:https://www.dotindot.com/>
ABN: 36 241 512 918
___________________________________________________
From: Frank Morgner [mailto:[email protected]]
Sent: Monday, February 26, 2018 8:28 PM
To: OpenSC/OpenSC
Cc: graeme-freedman; Comment
Subject: Re: [OpenSC/OpenSC] PIV detection of AID using Discovery Object before doing select AID - Partial Fix #1243 (#1256)
@frankmorgner commented on this pull request.
_____
In src/libopensc/card-piv.c <#1256 (comment)> :
/* Some objects will only be present if Histroy object says so */
for (i=0; i < PIV_OBJ_LAST_ENUM -1; i++)
if(piv_objects[i].flags & PIV_OBJECT_NOT_PRESENT)
priv->obj_cache[i].flags |= PIV_OBJ_CACHE_NOT_PRESENT;
+ sc_lock(card);
I can't find any statement that says you should use an ATR. But, as a matter of fact, some cards come with a specific ATR, so one may use it. As example look at SC-HSM <https://github.com/OpenSC/OpenSC/blob/master/src/libopensc/card-sc-hsm.c#L228-L250> 's implementation:
static int sc_hsm_match_card(struct sc_card *card)
{
sc_path_t path;
int i, r, type = 0;
i = _sc_match_atr(card, sc_hsm_atrs, &type);
if (i >= 0 && type != SC_CARD_TYPE_SC_HSM_SOC) {
card->type = type;
return 1;
}
sc_path_set(&path, SC_PATH_TYPE_DF_NAME, sc_hsm_aid.value, sc_hsm_aid.len, 0, 0);
r = sc_hsm_select_file(card, &path, NULL);
LOG_TEST_RET(card->ctx, r, "Could not select SmartCard-HSM application");
if (type == SC_CARD_TYPE_SC_HSM_SOC) {
card->type = SC_CARD_TYPE_SC_HSM_SOC;
} else {
card->type = SC_CARD_TYPE_SC_HSM;
}
return 1;
}
It's fast in some cases (matching an ATR), and it's reliable in all of the cases (sometimes needing a SELECT).
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub <#1256 (comment)> , or mute the thread <https://github.com/notifications/unsubscribe-auth/AfJoLzI82lh14ox8aMn96nPx3O64LU7_ks5tYnkigaJpZM4RxCUr> . <https://github.com/notifications/beacon/AfJoL5OHAOsqMOK0f67gAhLLeDkYoxFUks5tYnkigaJpZM4RxCUr.gif>
|
|
https://github.com/frankmorgner again you are missing the point. You keep trying to tell me to I can not have a match_card that returns 1, because the init might not be called. But then what is the point of calling match_card without calling _init? I am not matching ATRs for all the reasons others are point out to you, PIV is an applet that can be on many cards, and many in the future. We should be flexible enough to deal with future cards without having to code ATRs. The same goes for OpenPGP and all the others @graeme-freedman has also pointed out. Even Microsoft has always treated the PIV like an applet, and selects the applet. They do not have a list of ATRs of cards that might be PIV cards. I have changes #1256 around at least 3 times to try and satisfy your comments over the match_card and init. I am not changing it again. 3 of us have been insting for weeks now to commit #1256. All of us have a special interest in PIV, which you do not. So please commit #1256. |
In order to satisfy some concerns over the use of <card>_match_card and <card>_init, this modification will do that at the cost of additional overhead of repeating some card commands. Hopefully this commit will not be needed. On branch piv-aid-discovery Changes to be committed: modified: card-piv.c
|
In order to get this PR into 0.18.0 I have added one last commit to this PR which I fell in not needed. I would like to here from @viktorTarasov and @martinpaljak and @frankmorgner on Frank's issue with passing information from a card's match_card function to the card's init function. Frank has said don't do it. It was not designed to do it. My stance on this is it is OK to do it. Everywhere in the code the card's match_card function is called and returns 1, the card's init function is called. It has been this way for years. Earlier versions of this PR took advantage of that and allocated card->dvr_data and called sc_lock to avoid having to redo a lot of code in piv_init. The functionality this PR adds is to determine if the card has the PIV AID selected, and/or if the PIV AID can be selected. This requires much more of the PIV internal s be setup and used in piv_match_card and then passed on to piv_init. In order to not pass any information between the two routines, (because Frank was against it), I rewrote the piv_match_card to do very little checking and return 1. All the other processing done in the previous piv_match_card was moved to a new function called piv_match_card_continued and this was called from piv_init. Thus no card->dvr_data was allocated and sc_lock was not called and passed. Frank still did not like this. I said I would not fix this, but have submitted the latest commit 4222036 Piv_match_card_continued is now called twice, once from pic_match_card and then again from piv_init. I would also like to here from @mouse07410 @Jakuje and anyone else who have been testing this PR all along and asking for it to be committed. There is not much else that I can do to addresses Frank's concerns. And this PR with or without 4222036 should be in 0.18.0 |
What can I say? My quick preliminary tests show that this latest commit did not break PIV, and all my cards appear to work... I don't think this is necessary, but if that's what it takes to get this PR merged, so be it. I really do wish we stopped messing with this PR and committed it. Regardless, this PR has a prominent place in my fork, where it's been merged for quite some time. |
|
Adding another ack, that the current version works as expected and I did not encounter any concurrency issues while trying to sing with PIV applet and interfering with openpgp driver from different process. |
| int pin_cmd_verify; | ||
| int context_specific; |
There was a problem hiding this comment.
Maybe add a comment about what this field does and why. Same for other maybe not-so-obvious fields?
|
It is set in line 3453 and reset in line 3299. It is reset after the next card command which should be the crypto operation. If not then the calling application has changed it mind or is not following the PKCS#11 guidelines for using CKU_CONTEXT_SPECFIC. In either case, the extra lock is released: When PKCS#11 determines a key has the CKA_ALWAYS_AUTHENTICATE attribute it will do a C_Login(... CKU_CONTEXT_SPECIFIC ...) as required. This ends up as a PIN verify to the card driver. The driver can use this information as needed. In the PIV case, this will cause the extra lock to be done to make sure there is no interference from other processes. The lock is released when the next command is done. The extra sc_lock and sc_unlock cause the verify command and the next card command to be in the same SCard Transaction. In normal cases the verify is in a transaction by itself and the next command may come at any time. In the CKU_CONTEXT_SPECIFIC case PKCS#11 is saying it will be doing the crypto operation next immediately after the verify. I could add additional comments if the ones at line 3445 are not clear. |
|
Looks OK now. There's still a minor issue here, where a superfluous assignment should be better replaced with a comment. |
|
Yes we could do what you suggested, but need to free any cached data first.
I am reluctant to make any changes right now, as I will not be able to
to test anything for about 2 weeks.
I see "frankmorgner approved these changes 6 minutes ago"
I would like to see the PR committed as the current code works.
…On 3/8/2018 10:37 AM, Jakub Jelen wrote:
***@***.**** commented on this pull request.
--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
In src/libopensc/card-piv.c <#1256 (comment)>:
> + u8 rbuf[256];
+ size_t rbuflen = sizeof(rbuf);
+ u8 * arbuf = rbuf;
+ piv_private_data_t * priv = PIV_DATA(card);
+
+ SC_FUNC_CALLED(card->ctx, SC_LOG_DEBUG_VERBOSE);
+
+ /*
+ * During piv_match or piv_card_reader_lock_obtained,
+ * we use the discovery object to test if card present, and
+ * if PIV AID is active. So we can not use the cache
+ */
+
+ /* If not valid, read, cache and test */
+ if (!(priv->obj_cache[PIV_OBJ_DISCOVERY].flags & PIV_OBJ_CACHE_VALID)) {
+ r = piv_process_discovery(card);
Well ... the code and comment makes sense to me. The note, that we are reading the value always is already in the previous comment (|[...] So we can not use the cache|). This comment is supposed to
explain just the two following lines, which it does.
The thing is that if *the cache is invalid*, we call |piv_process_discovery()| and it calls |piv_get_cached_data()|, which in the result *reads* the data from card again regardless the name of the
function (we have invalid cache data or the object was not read yet).
I agree that the comment or the code can be little bit more clear.
But can not we explicitly invalidate the cache and then reduce this block to the call to |piv_process_discovery()| anyway (that is what we do in the else branch also, isn't it?). Would not it be more
readable?
|/* invalidate the cache and read the object from card */ priv->obj_cache[PIV_OBJ_DISCOVERY].flags &= ~PIV_OBJ_CACHE_VALID; r = piv_process_discovery(card); |
(I did not test this code, it might have some side effects or issues, but looks much more understandable and we even get some more debug information)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#1256 (comment)>, or mute the thread
<https://github.com/notifications/unsubscribe-auth/AA00Ma4hQy7WizobvSQL2boEtnkxeCcdks5tcV5ngaJpZM4RxCUr>.
--
Douglas E. Engert <[email protected]>
|
|
The "superfluous assignment" is not superfluous because in |
|
Can we get this PR committed? |
|
I'd like to see this PR merged. The code works, and is clear enough. We already had one bug introduced in an attempt to perfect what was good enough already. Enough is enough. |
|
Yes, lets merge this. If there will show up some more problems/comments, lets handle them in separate PR/Issues. |
|
Thanks. |
Checklist