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

Notify card driver that Reader lock obtained and if card was reset #842

Closed
wants to merge 2 commits into from

Conversation

dengert
Copy link
Member

@dengert dengert commented Jul 30, 2016

This is a replacement for #836. This new PR builds upon #837 and calls the card driver's card_reader_lock_obtained function whenever PCSC (or other reader driver) obtains the lock on the card.
This allows the card driver to take action at the start of a SCardBeginTransaction if needed.

A piv_card_reader_lock_obtained function is also added. If the card was reset, the PIV driver will select the AID again. There are some PIV cards where the PIV AID is not the default.

During sc_lock, if card->reader->ops->lock is called, card->ops->card_reader_lock_obtained will be called.
If PCSC is being used as the reader driver, this occures just after pcsc_lock  has done a SCardBeginTransaction
and our process has exclusive control over the card. The card driver can then determine if the state of the
card has changed, and take action to get the card into an acceptable state.

If card->reader->ops->lock returns SC_ERROR_CARD_RESET, indicating some other process has interefered
with the state of the card. was_reset=1 is passed to card->ops->card_reader_lock_obtained.

Some examples of actions that could be done by the card driver is to select the AID and reset logged_in.

Currently the card driver is not notified. So no default card_reader_lock_obtained is defined in iso7816.c

 Changes to be committed:
	modified:   card.c
	modified:   iso7816.c
	modified:   opensc.h
When sc_lock obtains a reader lock  this function is called
If the card was reset the PIV AID is seletcted and logged_in is reset.
This is need for some PIV cards where the default AID is not the PIV AID
and some other process has reset the card.

 Changes to be committed:
	modified:   card-piv.c
@dengert dengert mentioned this pull request Jul 30, 2016
mouse07410 added a commit to mouse07410/OpenSC that referenced this pull request Jul 31, 2016
mouse07410 added a commit to mouse07410/OpenSC that referenced this pull request Jul 31, 2016
@mouse07410
Copy link
Contributor

mouse07410 commented Jul 31, 2016

@dengert this patch needs to be rebased against the current master. In its current state it doesn't even compile:

Making all in libopensc
  CC       sc.lo
  CC       ctx.lo
  CC       log.lo
  CC       errors.lo
  CC       asn1.lo
  CC       base64.lo
  CC       sec.lo
  CC       card.lo
card.c:259:5: error: use of undeclared identifier 'reader_lock_obtained'
                                reader_lock_obtained = 1;
                                ^
card.c:486:16: error: use of undeclared identifier 'reader_lock_obtained'
        if (r == 0 && reader_lock_obtained == 1  && card->ops->card_reader_lock_obtained)
                      ^
card.c:487:50: error: use of undeclared identifier 'was_reset'; did you mean 'sc_reset'?
                r = card->ops->card_reader_lock_obtained(card, was_reset);
                                                               ^~~~~~~~~
                                                               sc_reset
card.c:363:5: note: 'sc_reset' declared here
int sc_reset(sc_card_t *card, int do_cold_reset)
    ^
card.c:580:5: error: use of undeclared identifier 'reader_lock_obtained'
                                reader_lock_obtained = 1;
                                ^
card.c:753:16: error: use of undeclared identifier 'reader_lock_obtained'
        if (r == 0 && reader_lock_obtained == 1  && card->ops->card_reader_lock_obtained)
                      ^
card.c:754:50: error: use of undeclared identifier 'was_reset'; did you mean 'sc_reset'?
                r = card->ops->card_reader_lock_obtained(card, was_reset);
                                                               ^~~~~~~~~
                                                               sc_reset
card.c:363:5: note: 'sc_reset' declared here
int sc_reset(sc_card_t *card, int do_cold_reset)
    ^
card.c:757:16: error: use of undeclared identifier 'reader_lock_obtained'
        if (r == 0 && reader_lock_obtained == 1  && card->ops->card_reader_lock_obtained)
                      ^
card.c:758:50: error: use of undeclared identifier 'was_reset'; did you mean 'sc_reset'?
                r = card->ops->card_reader_lock_obtained(card, was_reset);
                                                               ^~~~~~~~~
                                                               sc_reset
card.c:363:5: note: 'sc_reset' declared here
int sc_reset(sc_card_t *card, int do_cold_reset)
    ^
8 errors generated.
make[3]: *** [card.lo] Error 1
make[3]: *** Waiting for unfinished jobs....
make[2]: *** [all-recursive] Error 1
make[1]: *** [all-recursive] Error 1
make: *** [all] Error 2

The problem seems to stem from the fact that reader_lock_obtained and was_reset variables are declared not where your patch seems to expect to find them. Your patch adds int reader_lock_obtained = 0; definition only in sc_lock() function, while this variable is referred throughout card.c in #842.

Same for was_reset.

P.S. I understand that it was written for #837, but #837 has been closed and will not be merged (a modification of it is now in the master as 1e82dbe).

mouse07410 added a commit to mouse07410/OpenSC that referenced this pull request Jul 31, 2016
@dengert
Copy link
Member Author

dengert commented Jul 31, 2016

It passed above: "AppVeyor build succeeded" and "Travis CI build passed"
How did you apply the PR? Did you fail to copy a "}"? What do you consider current master? Do you have
1e82dbe from #837 in your master?

@mouse07410
Copy link
Contributor

mouse07410 commented Jul 31, 2016

It passed above: "AppVeyor build succeeded" and "Travis CI build passed"

Yeah, I saw that...

How did you apply the PR? Did you fail to copy a "}"?

By

$ wget https://github.com/OpenSC/OpenSC/pull/842.patch
--2016-07-31 09:07:44--  https://github.com/OpenSC/OpenSC/pull/842.patch
Resolving github.com (github.com)... 192.30.253.113
Connecting to github.com (github.com)|192.30.253.113|:443... connected.
HTTP request sent, awaiting response... 302 Found
Location: https://patch-diff.githubusercontent.com/raw/OpenSC/OpenSC/pull/842.patch [following]
--2016-07-31 09:07:46--  https://patch-diff.githubusercontent.com/raw/OpenSC/OpenSC/pull/842.patch
Resolving patch-diff.githubusercontent.com (patch-diff.githubusercontent.com)... 192.30.253.113
Connecting to patch-diff.githubusercontent.com (patch-diff.githubusercontent.com)|192.30.253.113|:443... connected.
HTTP request sent, awaiting response... 200 OK
Cookie coming from patch-diff.githubusercontent.com attempted to set domain to github.com
Length: unspecified [text/plain]
Saving to: ‘842.patch’

842.patch                    [ <=>                             ]   4.64K  --.-KB/s    in 0s      

Last-modified header missing -- time-stamps turned off.
2016-07-31 09:07:46 (12.1 MB/s) - ‘842.patch’ saved [4753]

$ patch -p1 < 842.patch 
patching file src/libopensc/card.c
patching file src/libopensc/iso7816.c
patching file src/libopensc/opensc.h
patching file src/libopensc/card-piv.c
$

What do you consider current master?

Whatever was merged into my fork the from OpenSC/OpenSC master at the moment.

Do you have 1e82dbe from #837 in your master?

I think so.

But then, I just re-applied everything, and somehow it all compiled now. No explanation that I can see...

Oh well... At least it builds.

@mouse07410
Copy link
Contributor

mouse07410 commented Jul 31, 2016

Reporting. #842 seems to work, as tested with NEO. The only case when it fails is this:

  1. Insert the token and unlock it with Keychain Access (success).
  2. Use the token with OpenSSL/OpenSC CLI (success).
  3. (Now the token appears as locked in Keychain Access).
  4. Attempt to unlock the token with Keychain (failure - cannot elicit prompt for the PIN, clicking on the lock does not produce the prompt, despite that it should).

Here's the log:
opensc-842-neo1.txt

If I explicitly lock the token between steps (1) and (2), everything is fine.

Update
This is what I wonder. The log says

. . . . .
0x7fff7973f000 09:18:03.173 [tokend] card-piv.c:960:piv_get_data: returning with: -1211 (Security status not satisfied)
0x7fff7973f000 09:18:03.173 [tokend] card-piv.c:3196:piv_check_protected_objects: object_test_verify=4, card_issues = 0x00020313
0x7fff7973f000 09:18:03.173 [tokend] card-piv.c:3197:piv_check_protected_objects: returning with: -1214 (PIN code or key incorrect)
0x7fff7973f000 09:18:03.173 [tokend] card-piv.c:3286:piv_pin_cmd: piv_pin_cmd tries_left=5, logged_in=0
0x7fff7973f000 09:18:03.173 [tokend] card-piv.c:3287:piv_pin_cmd: returning with: 0 (Success)
. . . . .

card-piv.c:3197 seems to think that the PIN was given, but tested as incorrect. Can this indicate the confusion of the libopensc-level code that should just check whether the token was or wasn't locked - but instead appears to think that the PIN verification failed, maybe implying that the PIN was prompted for, given, submitted to the token, and rejected?

P.S. Setting disconnect_action = leave; provides a workaround. If the log from that setup would help, I'd be happy to provide it.

mouse07410 added a commit to mouse07410/OpenSC that referenced this pull request Jul 31, 2016
@dengert
Copy link
Member Author

dengert commented Aug 1, 2016

Rather then using the patch there is a way to use git pull of a PR,
Before I submitted the PR, I pulled the current upstream master and rebased
my branch. I closed the old PR, pushed my branch to my origin, then went to
github.com to create the PR.

The github "graphs" "Network" can be very interesting, as you can see where
everyone is with there repositories

I am on vacation now and don't have acess to me developemt environment of
full e-mail file, or I could point you at how to pull a PR.

On Sun, Jul 31, 2016 at 6:13 AM, Mouse [email protected] wrote:

It passed above: "AppVeyor build succeeded" and "Travis CI build passed"

Yeah, I saw that...

How did you apply the PR? Did you fail to copy a "}"?

By

$ wget https://github.com/OpenSC/OpenSC/pull/842.patch
--2016-07-31 #842 09:07:44-- https://github.com/OpenSC/OpenSC/pull/842.patch
Resolving github.com (github.com)... 192.30.253.113
Connecting to github.com (github.com)|192.30.253.113|:443... connected.
HTTP request sent, awaiting response... 302 Found
Location: https://patch-diff.githubusercontent.com/raw/OpenSC/OpenSC/pull/842.patch [following]
--2016-07-31 09:07:46-- https://patch-diff.githubusercontent.com/raw/OpenSC/OpenSC/pull/842.patch
Resolving patch-diff.githubusercontent.com (patch-diff.githubusercontent.com)... 192.30.253.113
Connecting to patch-diff.githubusercontent.com (patch-diff.githubusercontent.com)|192.30.253.113|:443... connected.
HTTP request sent, awaiting response... 200 OK
Cookie coming from patch-diff.githubusercontent.com attempted to set domain to github.com
Length: unspecified [text/plain]
Saving to: ‘842.patch’

842.patch [ <=> ] 4.64K --.-KB/s in 0s

Last-modified header missing -- time-stamps turned off.
2016-07-31 09:07:46 (12.1 MB/s) - ‘842.patch’ saved [4753]

$ patch -p1 < 842.patch
patching file src/libopensc/card.c
patching file src/libopensc/iso7816.c
patching file src/libopensc/opensc.h
patching file src/libopensc/card-piv.c
$

What do you consider current master?

Whatever was merged with OpenSC/OpenSC master at the moment.

Do you have 1e82dbe
1e82dbe
from #837 #837 in your master?

I think so.

But then, I just re-applied everything, and somehow it all compiled now.
No explanation that I can see...

Oh well... At least it builds.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#842 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AA00MS_yWAlXFLDIoiW4cTw09bqGuIfAks5qbJ9xgaJpZM4JY0By
.

@mouse07410
Copy link
Contributor

mouse07410 commented Aug 1, 2016

Rather then using the patch there is a way to use git pull of a PR. Before I submitted the PR, I pulled the current upstream master and rebased my branch.

Yeah... I'm still mastering the art of rebasing, and of pulling PRs from other forks and repos. Though technically there shouldn't be any difference in the effect between pulling the PR through Git and downloading it as a patch and applying directly.

The github "graphs" "Network" can be very interesting, as you can see where everyone is with there repositories

Haven't looked at that before. Will check it out, thanks.

I am on vacation now and don't have access to me development environment of full e-mail file, or I could point you at how to pull a PR.

Oh, no problem. I'm sure this isn't the first time the question surfaced on the 'Net, so somewhere (stackoverflow?) there bound to be an answer, probably more than one. Update. Found how to pull a PR from the upstream. Reasonably simple procedure.

Enjoy your vacation! I'm looking forward to one, but probably not until the winter. :-(

P.S. I wonder what's happening inside the OpenSC libopensc - when disconnect_action = leave; is set, in the above problem scenario everything works as it should: Keychain prompts for the PIN when it's supposed to, etc. But when it's set to = reset; something somewhere somehow seems to lose the ability to understand that an Unlock request (created by clicking on the "Lock" icon of the Keychain) needs to spawn a PIN prompt, especially when the evidence says the token is locked... I just don't get it.

@mouse07410
Copy link
Contributor

mouse07410 commented Aug 1, 2016

Warning shot over the bows: we may have a problem with CAC. I cannot comment more because it locked my CAC and I'll need to get it reset tomorrow. No problem with the NEO token.

Update
Does the code differentiate between 63 XX and 69 83? I.e., does the code understand that this

. . . . .
0x7fff741f8000 15:48:58.859 [tokend] apdu.c:517:sc_transmit: called
0x7fff741f8000 15:48:58.859 [tokend] apdu.c:371:sc_single_transmit: called
0x7fff741f8000 15:48:58.859 [tokend] apdu.c:376:sc_single_transmit: CLA:0, INS:20, P1:0, P2:80, data(0) 0x7fff5f8750f0
0x7fff741f8000 15:48:58.859 [tokend] reader-pcsc.c:269:pcsc_transmit: reader 'SCM Microsystems Inc. SCR 3310'
0x7fff741f8000 15:48:58.859 [tokend] reader-pcsc.c:270:pcsc_transmit:
Outgoing APDU (5 bytes):
00 20 00 80 00 . ...
0x7fff741f8000 15:48:58.859 [tokend] reader-pcsc.c:199:pcsc_internal_transmit: called
0x7fff741f8000 15:48:58.879 [tokend] reader-pcsc.c:279:pcsc_transmit:
Incoming APDU (2 bytes):
69 83 i.
0x7fff741f8000 15:48:58.879 [tokend] apdu.c:386:sc_single_transmit: returning with: 0 (Success)
0x7fff741f8000 15:48:58.879 [tokend] apdu.c:539:sc_transmit: returning with: 0 (Success)
0x7fff741f8000 15:48:58.879 [tokend] card.c:449:sc_unlock: called
0x7fff741f8000 15:48:58.879 [tokend] iso7816.c:121:iso7816_check_sw: Authentication method blocked
. . . . .

means there's no point asking the card for anything else, most of all to verify PIN? I've got 83 transactions 00 20 00 80 00 => 69 83 in the log file for three attempts to unlock the token.

I'll need to check, but am pretty sure tokend is not aware of that.

@dengert
Copy link
Member Author

dengert commented Aug 2, 2016

Need a debug trace showing if verifies are failing and tries left are going
down. and if PIN really gets locked. Is pin caching turned on or off?

On Mon, Aug 1, 2016 at 1:54 PM, Mouse [email protected] wrote:

Warning shot over the bows: we may have a problem with CAC. I cannot
comment more because it locked my CAC and I'll need to get it reset
tomorrow. No problem with the NEO token.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#842 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AA00MVJXkMkFJWMEnuzJhsKTHMFN4mwyks5qblzygaJpZM4JY0By
.

@mouse07410
Copy link
Contributor

Need a debug trace showing if verifies are failing and tries left are going down, and if PIN really gets locked.

I removed disconnect_action = leave; and re-did the thing. It was fine with CAC then.

There's something fishy with how the whole thing behaves related to " = leave". When "leave" is set, it always prompts for the PIN when it's supposed to. But occasionally I get these problems. When "=leave" is not set, it rarely causes this kind of a problem, but exhibits one failure scenario as described in #842 (comment) .

(You'd understand my reluctance to experiment with CAC too much, as getting it reset is not fun.)

Is pin caching turned on or off?

Left at default, so I believe the answer is "turned on, cached for 10 attempts unless CKA_ALWAYS_AUTHENTICATE is set".

@dengert
Copy link
Member Author

dengert commented Aug 3, 2016

Will look at trace late next week.

On Tue, Aug 2, 2016 at 2:15 PM, Mouse [email protected] wrote:

Need a debug trace showing if verifies are failing and tries left are
going down, and if PIN really gets locked.

I removed disconnect_action = leave; and re-did the thing. It was fine
with CAC then.

There's something fishy with how the whole thing behaves related to " =
leave". When "leave" is set, it always prompts for the PIN when it's
supposed to. But occasionally I get these problems. When "=leave" is not
set, it rarely causes this kind of a problem, but exhibits one failure
scenario as described in #842 (comment)
#842 (comment) .

(You'd understand my reluctance to experiment with CAC too much, as
getting it reset is not fun.
)

Is pin caching turned on or off?

Left at default, so I believe the answer is "turned on, cached for 10
attempts unless CKA_ALWAYS_AUTHENTICATE is set".


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#842 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AA00MWz0-8YqZTtumWpzenF7_vstatQWks5qb7NUgaJpZM4JY0By
.

@mouse07410
Copy link
Contributor

Will look at trace late next week.

Great, thank you!

It would be outstanding if you could look at/compare the traces with "=leave;" enabled and not enabled, and figure what's the difference - like, why in the latter case the code does not prompt for the PIN...

@mouse07410
Copy link
Contributor

Update

Tried to connect to DCS via Web browser using CAC authentication. Several times the Web browser prompted me for my PIN, and appeared to be doing something with the token. But no successful connection appeared. Only the last attempt (24 minutes from the beginning) succeeded. Here's the log:
opensc-842-cac4-bad.txt

@dengert
Copy link
Member Author

dengert commented Aug 12, 2016

On 8/8/2016 4:29 PM, Mouse wrote:


  Update
  Tried to connect to DCS via Web browser using CAC
    authentication. Several times the Web browser prompted me for my
    PIN, and appeared to be doing something with the token. But no
    successful connection appeared. Only the last attempt (24
    minutes from the beginning) succeeded. Here's the log:
    opensc-842-cac4-bad.txt


Don't see anything strange in log. All I see after the verify of the
pin  is the pin is verified as tested by 
00 20 00 80 00
90 00

Problem may not be in OpenSC, and nothing shows in the log. 


  —
    You are receiving this because you were mentioned.
    Reply to this email directly, view
      it on GitHub, or mute
      the thread.







  {"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/OpenSC/OpenSC","title":"OpenSC/OpenSC","subtitle":"GitHub repository","main_image_url":"https://assets-cdn.github.com/images/modules/aws/aws-bg.jpg","avatar_image_url":"https://cloud.githubusercontent.com/assets/143418/15842166/7c72db34-2c0b-11e6-9aed-b52498112777.png","action":{"name":"Open in GitHub","url":"https://github.com/OpenSC/OpenSC"}},"updates":{"snippets":[{"icon":"PERSON","message":"@mouse07410 in #842: **Update**\r\n\r\nTried to connect to DCS via Web browser using CAC authentication. Several times the Web browser prompted me for my PIN, and appeared to be doing something with the token. But no successful connection appeared. Only the last attempt (24 minutes from the beginning) succeeded. Here's the log:\r\n[opensc-842-cac4-bad.txt](https://github.com/OpenSC/OpenSC/files/407766/opensc-842-cac4-bad.txt)\r\n"}],"action":{"name":"View Pull Request","url":"https://github.com/OpenSC/OpenSC/pull/842#issuecomment-238383716"}}}


-- 

Douglas E. Engert [email protected]

@dengert
Copy link
Member Author

dengert commented Aug 12, 2016

On 7/31/2016 8:22 AM, Mouse wrote:


  Reporting. #842
    seems to work, as tested with NEO. The only case when it fails
    is this:

    Insert the token and unlock with Keychain Access (success).
    Use the token with OpenSSL/OpenSC CLI (success).
    (Now the token appears as locked in Keychain Access).
    Attempt to unlock the token with Keychain (failure).

  Here's the log: 
    opensc-842-neo1.txt


The OpenSSL/OpenSC CLI  disconnects for the card, and does a card
reset. (opensc.conf must not "leave") so not longer logged_in.
0x7fff7973f000 09:17:50.281 [opensc-pkcs11]
reader-pcsc.c:570:pcsc_disconnect: Yubico Yubikey NEO
OTP+U2F+CCID:SCardDisconnect returned: 0x00000000

tokend finds out it has been reset:
0x7fff7973f000 09:17:54.653 [tokend] reader-pcsc.c:591:pcsc_lock:
Yubico Yubikey NEO OTP+U2F+CCID:SCardBeginTransaction returned:
0x80100068

PIV code then gets called to select the AID again and test if
logged_in.
0x7fff7973f000 09:17:54.655 [tokend]
card-piv.c:3310:piv_card_reader_lock_obtained: called

and verify Lc=0 is sent:
00 20 00 80
63 C5 
and since this is a NEO,  also tries to read 00 CB 3F FF 05 5C 03 5F
C1 09 08 that was determined earlier to be a way to test logged_in
state and determines it is not logged_in any more:
0x7fff7973f000 09:17:54.681 [tokend] card-piv.c:3286:piv_pin_cmd:
piv_pin_cmd tries_left=5, logged_in=0

The above is expected behavior. 

What tokend and keychain do is not in the log, and not an OpenSC
problem.  Sounds like the same problem as in 
opensc-842-cac-4-bad.txt:  tokend and keychain don't know how to
request to login again. 











  —
    You are receiving this because you were mentioned.
    Reply to this email directly, view
      it on GitHub, or mute
      the thread.









-- 

Douglas E. Engert [email protected]

@mouse07410
Copy link
Contributor

What tokend and keychain do is not in the log, and not an OpenSC problem. Sounds like the same problem as in opensc-842-cac-4-bad.txt: tokend and keychain don't know how to request to login again.

I see your point, and cannot argue with your analysis of the provided logs. However, somehow if the disconnect_action = leave; is present - then (even though theoretically "leave" means the token was not reset, and presumably stays logged in) tokend/Keychain/whatever does request to login again. While when the token was reset - that ability somehow gets lost. I still suspect something in OpenSC.

@dengert
Copy link
Member Author

dengert commented Aug 12, 2016

"leave" means don't do a reset upon disconnect. And its an OpenSC
only thing. Other middleware could do a reset, or other process
which used a different opensc.conf or no opensc.conf, as "reset'" is
the default.    But the card could become not logged_in for some
other reasons,  like trying a different AID which opensc does during
card match for some other drivers before the PIV driver. Or a verify
with the wrong PIN would cause it to not be logged_in. 

If you want to prove it is OpenSC, you need add more debugging to
tokend, keychain and pcscd. And you need to show why when "leave" is
set tokend or keychain still asks for  a PIN. What and how are they
testing the state of the card or some internal software kept state.
And why don't they use the same logic when the card is reset.      

On 8/12/2016 9:19 AM, Mouse wrote:



    What tokend and keychain do is not in the log, and not an
      OpenSC problem. Sounds like the same problem as in
      opensc-842-cac-4-bad.txt: tokend and keychain don't know how
      to request to login again.

  I see your point, and cannot argue with your analysis of the
    provided logs. However, somehow if the disconnect_action
      = leave; is present - then (even though theoretically
    "leave" means the token was not reset, and presumably stays
    logged in) tokend/Keychain/whatever does request to login
      again. While when the token was reset - that ability
    somehow gets lost. I still suspect something in OpenSC.
  —
    You are receiving this because you were mentioned.
    Reply to this email directly, view
      it on GitHub, or mute
      the thread.







  {"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/OpenSC/OpenSC","title":"OpenSC/OpenSC","subtitle":"GitHub repository","main_image_url":"https://cloud.githubusercontent.com/assets/143418/17495839/a5054eac-5d88-11e6-95fc-7290892c7bb5.png","avatar_image_url":"https://cloud.githubusercontent.com/assets/143418/15842166/7c72db34-2c0b-11e6-9aed-b52498112777.png","action":{"name":"Open in GitHub","url":"https://github.com/OpenSC/OpenSC"}},"updates":{"snippets":[{"icon":"PERSON","message":"@mouse07410 in #842: \u003e What tokend and keychain do is not in the log, and not an OpenSC problem.  Sounds like the same problem as in opensc-842-cac-4-bad.txt:  tokend and keychain don't know how to request to login again.\r\n\r\nI see your point, and cannot argue with your analysis of the provided logs. However, somehow if the `disconnect_action = leave;` is present - then (even though theoretically \"leave\" means the token was not reset, and presumably stays logged in) tokend/Keychain/whatever *does request to login again*. While when the token was reset - that ability somehow gets lost. I still suspect something in OpenSC."}],"action":{"name":"View Pull Request","url":"https://github.com/OpenSC/OpenSC/pull/842#issuecomment-239458997"}}}


-- 

Douglas E. Engert [email protected]

@mouse07410
Copy link
Contributor

"leave" means don't do a reset upon disconnect. And its an OpenSC only thing. Other mddleware could do a reset, or other process which used a different opensc.conf or no opensc.conf, as "reset'" is the default.

Yes, I realize this. Which is why I'm trying to get this problem resolved, rather than resigning to the "=leave;" workaround.

If you want to prove it is OpenSC, you need add more debugging to tokend, keychain and pcscd.

Adding instrumentation to tokend is quite doable, as it is fully under my control - so I will do it (though I'd like some guidance as to what specific things to log - it seems that the current logging is reasonably extensive?). Adding debugging to pcscd should be possible - but I have neither control over the code, nor the knowledge. Adding debugging to Keychain seems impossible - and Apple does not help at all.

And you need to show why when "leave" is set tokend or keychain still asks for a PIN. What and how are they testing the state of the card or some internal software kept state. And why don't they use the same logic when the card is reset.

I think all of the above is happening within the OpenSC library, before the control flow passes to tokend. But I'll need to figure how to provide you an acceptable trace that contains everything you need to determine the place and cause of the problem.

@dengert
Copy link
Member Author

dengert commented Aug 12, 2016

On 8/12/2016 10:56 AM, Mouse wrote:



    "leave" means don't do a reset upon disconnect. And its an
      OpenSC only thing. Other mddleware could do a reset, or other
      process which used a different opensc.conf or no opensc.conf,
      as "reset'" is the default.

  Yes, I realize this. Which is why I'm trying to get this
    problem resolved, rather than resigning to the "=leave;"
    workaround.

    If you want to prove it is OpenSC, you need add more
      debugging to tokend, keychain and pcscd. 

  Adding instrumentation to tokend is quite doable, as it is
    fully under my control - so I will do it (though I'd like some
    guidance as to what specific things to log - it seems that the
    current logging is reasonably extensive?). Adding debugging to
    pcscd should be possible - but I have neither control over the
    code, nor the knowledge. Adding debugging to Keychain seems
    impossible - and Apple does not help at all.

    And you need to show why when "leave" is set tokend or
      keychain still asks for a PIN. What and how are they testing
      the state of the card or some internal software kept state.
      And why don't they use the same logic when the card is reset.

  I think all of the above is happening within the
    OpenSC library, before the control flow passes to tokend. But
    I'll need to figure how to provide you an acceptable trace that
    contains everything you need to determine the place and cause of
    the problem.


I am not looking at MacOS. If no one wants to help with you
problems, you will have to learn how to read the traces yourself.



  —




  You
    are receiving this because you were mentioned.
    Reply to this email directly, view
      it on GitHub, or mute
      the thread.







  {"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/OpenSC/OpenSC","title":"OpenSC/OpenSC","subtitle":"GitHub repository","main_image_url":"https://cloud.githubusercontent.com/assets/143418/17495839/a5054eac-5d88-11e6-95fc-7290892c7bb5.png","avatar_image_url":"https://cloud.githubusercontent.com/assets/143418/15842166/7c72db34-2c0b-11e6-9aed-b52498112777.png","action":{"name":"Open in GitHub","url":"https://github.com/OpenSC/OpenSC"}},"updates":{"snippets":[{"icon":"PERSON","message":"@mouse07410 in #842: \u003e \"leave\" means don't do a reset upon disconnect. And its an OpenSC only thing. Other mddleware could do a reset, or other process which used a different opensc.conf or no opensc.conf, as \"reset'\" is the default.\r\n\r\nYes, I realize this. Which is why I'm trying to get this problem resolved, rather than resigning to the \"=leave;\" workaround.\r\n\r\n\u003e  If you want to prove it is OpenSC, you need add more debugging to tokend, keychain and pcscd. \r\n\r\nAdding instrumentation to tokend is quite doable, as it is fully under my control - so I will do it (though I'd like some guidance as to what specific things to log - it seems that the current logging is reasonably extensive?). Adding debugging to pcscd should be possible - but I have neither control over the code, nor the knowledge. Adding debugging to Keychain seems impossible - and Apple does not help at all.\r\n\r\n\u003e And you need to show why when \"leave\" is set tokend or keychain still asks for  a PIN. What and how are they testing the state of the card or some internal software kept state.  And why don't they use the same logic when the card is reset.\r\n\r\nI *think* all of the above is happening within the OpenSC library, before the control flow passes to tokend. But I'll need to figure how to provide you an acceptable trace that contains everything you need to determine the place and cause of the problem.\r\n"}],"action":{"name":"View Pull Request","url":"https://github.com/OpenSC/OpenSC/pull/842#issuecomment-239485434"}}}


-- 

Douglas E. Engert [email protected]

@dengert
Copy link
Member Author

dengert commented Aug 12, 2016

@viktorTarasov, @frankmorgner,
I believe this PR is ready to be committed. Can you commit this PR? (I could commit it, but I am letting the 2 of your handle commits.
Thanks.

@mouse07410 has been testing on MacOS, and as far as I can tell this PR is working as expected. His problems appear to be in the tokend and/or keychain, not in this PR.

@frankmorgner
Copy link
Member

I think the Idea behind this PR is OK. Though I'd suggest to use the same logic as in sc_reader_t by implementing an additional lock and unlock call back on the card driver level. So compared with the current changes in question I would suggest the following changes:

  1. rename card_reader_lock_obtained to post_lock
  2. (optionally) create an additional call back for the card driver pre_unlock
  3. call post_lock (pre_unlock) exactly after (before) the reader locked (unlocks) the card. If the card driver wants to check if an applet is selected in the post_lock callback than this needs to be done before card->sm_ctx.ops.open is called.
  4. completely remove the was_reset flag. There are many cases in which the card is not notified about a reset, but still the card looses all of its context. This means that the card driver will always have to assume the worst case whether the was_reset flag was set or not.

@dengert
Copy link
Member Author

dengert commented Aug 14, 2016

1: It could be renamed, but why? Note  card_reader_lock_obtained
is only called when the reader lock is actually obtained,
even though it is called from sc_lock. the name post_lock is
misleading. 

2: That could be done. Could be future modification if and when
needed. 

3: I did not have any cards that use SM. So I was being very careful
to not change the logic for SM, as I can not test it, and have not
looked at it. The logic that was changed, 
https://github.com/viktorTarasov said looked OK.

4: No. The "was_reset" flag  is needed. Your card driver can ignore
it if you don't want to use it.  Not all cards follow all the
standards, even from the same card vendor, and so the PIV driver
with this modification is addressing the reset of the card only at
this time.  And yes there are many other things that can cause the
card state to change other then a reset. One such things is the
OpenSC driver matching  for another  driver will try and select its 
AID, causing one of the Yubico piv-want-to-be tokens to loose the
verified state, even  when the PIV specs specifically state to
ignore unknown AIDs and leave the state unchanged!

I consider your comments as suggestions for the future and not to be
included in the PR.  

On 8/12/2016 6:24 PM, Frank Morgner
  wrote:


  I think the Idea behind this PR is OK. Though I'd suggest to
    use the same logic as in sc_reader_t by
    implementing an additional lock and unlock
    call back on the card driver level. So compared with the current
    changes in question I would suggest the following changes:

    rename card_reader_lock_obtained to post_lock

    (optionally) create an additional call back for the card
      driver pre_unlock

    call post_lock (pre_unlock)
      exactly after (before) the reader locked (unlocks) the card.
      If the card driver wants to check if an applet is selected in
      the post_lock callback than this needs to be
      done before card->sm_ctx.ops.open
      is called.
    completely remove the was_reset flag. There
      are many cases in which the card is not notified about a
      reset, but still the card looses all of its context. This
      means that the card driver will always have to assume the
      worst case whether the was_reset flag was set or
      not.

  —
    You are receiving this because you were mentioned.
    Reply to this email directly, view
      it on GitHub, or mute
      the thread.







  {"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/OpenSC/OpenSC","title":"OpenSC/OpenSC","subtitle":"GitHub repository","main_image_url":"https://cloud.githubusercontent.com/assets/143418/17495839/a5054eac-5d88-11e6-95fc-7290892c7bb5.png","avatar_image_url":"https://cloud.githubusercontent.com/assets/143418/15842166/7c72db34-2c0b-11e6-9aed-b52498112777.png","action":{"name":"Open in GitHub","url":"https://github.com/OpenSC/OpenSC"}},"updates":{"snippets":[{"icon":"PERSON","message":"@frankmorgner in #842: I think the Idea behind this PR is OK. Though I'd suggest to use the same logic as in `sc_reader_t` by implementing an additional `lock` and `unlock` call back on the card driver level. So compared with the current changes in question I would suggest the following changes:\r\n\r\n1. rename `card_reader_lock_obtained` to `post_lock`\r\n2. (optionally) create an additional call back for the card driver `pre_unlock`\r\n3. call `post_lock` (`pre_unlock`) exactly after (before) the reader locked (unlocks) the card. If the card driver wants to check if an applet is selected in the `post_lock` callback than this needs to be done *before* `card-\u003esm_ctx.ops.open` is called.\r\n4. completely remove the `was_reset` flag. There are many cases in which the card is not notified about a reset, but still the card looses all of its context. This means that the card driver will always have to assume the worst case whether the `was_reset` flag was set or not."}],"action":{"name":"View Pull Request","url":"https://github.com/OpenSC/OpenSC/pull/842#issuecomment-239583048"}}}


-- 

Douglas E. Engert [email protected]

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.

None yet

3 participants