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

Improved Handling of PKCS11 Slots (2) #1970

Merged
merged 7 commits into from Mar 3, 2020

Conversation

frankmorgner
Copy link
Member

This PR extends #1961 to

  • simplify the PKCS#11 state tracking
  • remove duplicated initialization code
  • identifies a slot's reader based on the reader description instead of the reader's handle

Fixes: #1945
Fixes: #1935

Replaces: #1947

frankmorgner and others added 5 commits March 3, 2020 15:17
OpenSC PKCS11 now retains slots even when the reader is removed.
It can do this because existing OpenSC reader handling in ctx.c,
reader-pcsc.c and PC/SC allow OpenSC to do this.

This simplifies the code, and allow a reader to be reinserted
and use the existing slot. This matching is actually done
in reader-pcsc.c because PC/SC returns the unique ids based on
the OS reader names. This is then used as the manufacturerID

By not deleting slots the slot list can only increase which is a
restriction of Firefox. It does not fix all the Firefox issues, but
it does not go into a loop, when all the readers are removed.

The defaults in opensc.conf for max-virtual-readers and slots-per-card
allow for 4 different readers used during one session.

 On branch PKCS11-SLOTS-3
 Changes to be committed:
	modified:   sc-pkcs11.h
	modified:   slot.c
- reader (slot) description is already initialized init_slot_info()
- objects and logins are already released in slot_token_removed
When a reader is removed and reattached, this re-uses the old slot
without relying on the fact that the sc_reader_t is unchanged.
- allows re-attatching a reader to an existing reader object by
resetting the SC_READER_REMOVED flag
- readers that are flagged with SC_READER_REMOVED are not used for
SCardGetStatusChange to avoid SCARD_E_UNKNOWN_READER

fixes OpenSC#1903
@frankmorgner frankmorgner merged commit 649ee27 into OpenSC:master Mar 3, 2020
@Jakuje
Copy link
Member

Jakuje commented Mar 4, 2020

This changed the slot IDs and now I am getting slots 0, 1 and 2 instaed of 0, 4 and 8 (with the virtual slot ids reserved). Not sure if this is good or bad, but this is a change that I do not see described here.

I can think of a case that we would have reader 1 with PIV card and reader 2 with other card, giving slot 0 and 1 respectively. If we swap the PIV card in first slot for OpenPGP card, which creates 2 virtual slots, I am not sure what will happen. Previously, the virtual slot ids were allocated, now it is not clear to me if it will use non-consecutive slot ids, it will orverlap or it will do something els.

This broke the CAC/Coolkey CI, which was expecting these particular slot IDs, so I would like to know whether this was intentional and I should update the configuration or it is a bug.

@dengert
Copy link
Member

dengert commented Mar 4, 2020

#1961 kept the sc_reader_t and sc_pkcs11_slot_t for all the virtual slots and slot IDs even if physical reader was removed. If the same reader is reinserted (and the OS returns the same name to pcsc), the same sc_reader_t, sc_pkcs11_slot_t and slot_ids will be used. If not a new sc_reader_t, sc_pkcs11_slot_t for the virtual readers and slot IDs will be allocated.

frankmorgner added a commit that referenced this pull request Mar 4, 2020
@frankmorgner
Copy link
Member Author

I've restored the original slot ids.

(However, an application should not rely on any hard coded slot ids...)

@Jakuje
Copy link
Member

Jakuje commented Mar 4, 2020

I am not saying the applications should rely on hard-coded slot ids. This is test system running nightly tests. But I was not sure how the above mentioned use case can be handled without having virtual slots the way how they worked before so I asked. As well as I did not see this mentioned in any commit message.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants