libp11 and PKCS11_enumerate_slots

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

libp11 and PKCS11_enumerate_slots

Ludovic Rousseau
Hello,

PKCS11_enumerate_slots() enumerate the slots only the first time it is
called. Any subsequent call only returns cached values. It is very
problematic if you want to enumerate AGAIN the slots, because a card
or a reader has been inserted or removed.

I see 3 possible solutions:
1. the application explicitly calls pkcs11_destroy_all_slots() and
then PKCS11_enumerate_slots(). We will need to move
pkcs11_destroy_all_slots() from libp11-int.h to libp11.h (make it
public)
2. PKCS11_enumerate_slots() internally calls
pkcs11_destroy_all_slots() to force a re-enumeration each time it is
called. May be bad for performances.
3. add an argument to PKCS11_enumerate_slots() indicating if we want
to force a re-enumeration. Whould break the ABI :-(

I prefer solution 2 since a function should not return out of date
information if it is possible to be accurate. Patch attached.

What do you think?

Bye,

--
  Dr. Ludovic Rousseau
 For private mail use [hidden email] and not "big brother" Google

_______________________________________________
opensc-devel mailing list
[hidden email]
http://www.opensc.org/cgi-bin/mailman/listinfo/opensc-devel
Reply | Threaded
Open this post in threaded view
|

Re: libp11 and PKCS11_enumerate_slots [u]

Andreas Jellinghaus-2
all solutions are fine with me.

but what about PKCS11_find_token?
it calls PKCS11_enumerate_slots.

If that causes trouble, I'd prefere
a PKCS11_free_slots, so the user of the API
knows that everything is gone now.

Regards, Andreas
_______________________________________________
opensc-devel mailing list
[hidden email]
http://www.opensc.org/cgi-bin/mailman/listinfo/opensc-devel
Reply | Threaded
Open this post in threaded view
|

Re: libp11 and PKCS11_enumerate_slots [u]

Ludovic Rousseau
On 29/08/05, Andreas Jellinghaus [c] <[hidden email]> wrote:
> all solutions are fine with me.
>
> but what about PKCS11_find_token?
> it calls PKCS11_enumerate_slots.

That should not be a problem (except the performance impact). You can
call PKCS11_enumerate_slots() two consecutive times. You should just
have to keep in mind that the previously allocated slots have gone. Is
that your concern?

I had a look at sslengines/engine_pkcs11.c, PKCS11_enumerate_slots()
is called first but only to display some debug information about the
available slots. Then PKCS11_find_token() is called to find a valid
token.
If performance is a concern I think we can remove the call to
PKCS11_enumerate_slots() and use PKCS11_find_token() only.

> If that causes trouble, I'd prefere
> a PKCS11_free_slots, so the user of the API
> knows that everything is gone now.

Your PKCS11_free_slots() would be the same as the already existing
pkcs11_destroy_all_slots() or you need something different?

Bye,

--
  Dr. Ludovic Rousseau
 For private mail use [hidden email] and not "big brother" Google
_______________________________________________
opensc-devel mailing list
[hidden email]
http://www.opensc.org/cgi-bin/mailman/listinfo/opensc-devel
Reply | Threaded
Open this post in threaded view
|

Re: libp11 and PKCS11_enumerate_slots [u]

Ludovic Rousseau
The more I think the more I don't like the idea to have a function
that returns outdated information unless to call another function
first. That's a kind of black magic I don't like.

On 30/08/05, Ludovic Rousseau <[hidden email]> wrote:
> On 29/08/05, Andreas Jellinghaus [c] <[hidden email]> wrote:
> > all solutions are fine with me.
> >
> > but what about PKCS11_find_token?
> > it calls PKCS11_enumerate_slots.
>
> That should not be a problem (except the performance impact).

I modified PKCS11_find_token() to call PKCS11_enumerate_slots() only
if was never called. So the potential performance problem is gone.

> You can
> call PKCS11_enumerate_slots() two consecutive times. You should just
> have to keep in mind that the previously allocated slots have gone. Is
> that your concern?
>
> > If that causes trouble, I'd prefere
> > a PKCS11_free_slots, so the user of the API
> > knows that everything is gone now.
>
> Your PKCS11_free_slots() would be the same as the already existing
> pkcs11_destroy_all_slots() or you need something different?
No answer Andreas?

I don't like the idea of PKCS11_free_slots() because the normal use of
PKCS11_enumerate_slots() would then be:
  PKCS11_free_slots();
  PKCS11_enumerate_slots();
to be sure the list of slot is accurate.

If you just want to get the cached data returned by
PKCS11_enumerate_slots() you can store the functions results the first
time you call it and, in fact, do the caching yourself.

I include a new patch that include the modification of PKCS11_find_token().

Comments welcome, or I commit the patch :-)

Bye,

--
  Dr. Ludovic Rousseau
 For private mail use [hidden email] and not "big brother" Google

_______________________________________________
opensc-devel mailing list
[hidden email]
http://www.opensc.org/cgi-bin/mailman/listinfo/opensc-devel
Reply | Threaded
Open this post in threaded view
|

Re: libp11 and PKCS11_enumerate_slots

Andreas Jellinghaus-2
In reply to this post by Ludovic Rousseau
Sorry for the late answer.

> I had a look at sslengines/engine_pkcs11.c, PKCS11_enumerate_slots()
> is called first but only to display some debug information about the
> available slots. Then PKCS11_find_token() is called to find a valid
> token.
> If performance is a concern I think we can remove the call to
> PKCS11_enumerate_slots() and use PKCS11_find_token() only.

or remove the enumerate_slots() call from find token?
that way you need two steps, but each is more logical:
 - get information about all slots
 - search within that information.

also I find it easier to understand if you have
a function that gathers some information and one
to free it.

I looked at the code, and there are only few places
where slots and nslots are used. so I wonder why we
keep a copy in libp11 private data of these at all.
 - enumerate slots: hand out old result, if already
   called. I think that feature is not important.
 - PKCS11_find_token: we could pass the slots as well.
 - PKCS11_init_token: I don't understand the code.
   why does it call pkcs11_check_token?
   as that call destroys the token structure, we
   need to document all calls to that function,
   I guess.
 - PKCS11_CTX_new initialize nslots to -1 - ignore.

 
> > If that causes trouble, I'd prefere
> > a PKCS11_free_slots, so the user of the API
> > knows that everything is gone now.
>
> Your PKCS11_free_slots() would be the same as the already existing
> pkcs11_destroy_all_slots() or you need something different?

I find "destroy" a bit strange. "free" is the usual name, right?


so what about this API change:
 - remove slots / nslots from private context structure.
 - enumerates_slots will allocate and return slots always.
   it will not return an old copy.
 - user needs to pass the slots to find_token()
 - find token thus does not enumerate_slots() itself.
 - user needs to call destroy_slots() (or free_slots()).

I think each step is now clearer and we will remove
lots of side effects with this. less complexity is good :)

extra benefit: you could keep an old version, get new slots
and compare what changed, before freeing the old ones.

what do you think?

Regards, Andreas
_______________________________________________
opensc-devel mailing list
[hidden email]
http://www.opensc.org/cgi-bin/mailman/listinfo/opensc-devel
Reply | Threaded
Open this post in threaded view
|

Re: libp11 and PKCS11_enumerate_slots

Andreas Jellinghaus-2
In reply to this post by Ludovic Rousseau
On Wednesday 31 August 2005 09:30, Ludovic Rousseau wrote:
> The more I think the more I don't like the idea to have a function
> that returns outdated information unless to call another function
> first. That's a kind of black magic I don't like.

100% agree.

> I don't like the idea of PKCS11_free_slots() because the normal use of
> PKCS11_enumerate_slots() would then be:
>   PKCS11_free_slots();
>   PKCS11_enumerate_slots();
> to be sure the list of slot is accurate.

hmm, why is free and get new data a bad thing?

> If you just want to get the cached data returned by
> PKCS11_enumerate_slots() you can store the functions results the first
> time you call it and, in fact, do the caching yourself.

yes, I think we don't need to cache that information at all,
there are not many users of it anyway. see my other mail:
lets get rid of the internal copy of the slots and simply
pass the slots to find_token function.

> I include a new patch that include the modification of PKCS11_find_token().
>
> Comments welcome, or I commit the patch :-)

Often I think: how much documentation would I need to write to document
something properly, including all side effects, and I use that to measure
complexity. I think enumerate_slots to allocate new ones, pass them
to find_token, and free them yourself is the easiest solution.

and for initialized_token(): sure, now it can't update the token
info on all slots any more. but maybe it would be good to export
that function, too?

looks like there is lot of interesting code I need to read to get
a better picture. and understand all the dependencies.

Regards, Andreas
_______________________________________________
opensc-devel mailing list
[hidden email]
http://www.opensc.org/cgi-bin/mailman/listinfo/opensc-devel
Reply | Threaded
Open this post in threaded view
|

Re: libp11 and PKCS11_enumerate_slots

Ludovic Rousseau
On 31/08/05, Andreas Jellinghaus <[hidden email]> wrote:
> I think enumerate_slots to allocate new ones, pass them
> to find_token, and free them yourself is the easiest solution.

I agree.
I do not make pkcs11_destroy_all_slots() public yet since I don't know
if it is really usefull. It would be better for the API symetry
(allocate & free functions). I will propose a patch later.

> and for initialized_token(): sure, now it can't update the token
> info on all slots any more. but maybe it would be good to export
> that function, too?

You are talking about PKCS11_init_token()? I can't find any
initialized_token() function.

I think PKCS11_init_token() should only update the slots corresponding
to this token and not all the slots.

Patch attached. Changes:
- PKCS11_find_token() now uses a list of slots as parameter
- PKCS11_init_token() only reinit the slots associated with this token
(untested code)
- we need to keep the slots and nslots fields in the private part of
the ctx structure to parse the list in PKCS11_init_token().

Comments?

--
 Dr. Ludovic Rousseau
 For private mail use [hidden email] and not "big brother" Google

_______________________________________________
opensc-devel mailing list
[hidden email]
http://www.opensc.org/cgi-bin/mailman/listinfo/opensc-devel
Reply | Threaded
Open this post in threaded view
|

Re: libp11 and PKCS11_enumerate_slots

Andreas Jellinghaus-2
On Wednesday 31 August 2005 11:25, Ludovic Rousseau wrote:
> I do not make pkcs11_destroy_all_slots() public yet since I don't know
> if it is really usefull. It would be better for the API symetry
> (allocate & free functions). I will propose a patch later.

too late :) but not yet commited. see http://www.opensc.org/files/wip/
for a diff and patched version.

I exported both destroy_slot and destroy_all_sloty. the former is
propably a bad name and not needed. the later is needed for error
handling and cleanup. but we might want a different name?

> You are talking about PKCS11_init_token()? I can't find any
> initialized_token() function.

yes, sorry, I should have cut&pasted and not tried to remmeber
the function name.

> I think PKCS11_init_token() should only update the slots corresponding
> to this token and not all the slots.

agree. does anyone see a problem with such a change?

> Patch attached.

still keeps slots and nslots in the context and
destroys slots in enumerate_slots. I think it is a
lot easier to remove slots and nslots form the context
and have the user call a destroy/free function.

Regards, Andreas
_______________________________________________
opensc-devel mailing list
[hidden email]
http://www.opensc.org/cgi-bin/mailman/listinfo/opensc-devel
Reply | Threaded
Open this post in threaded view
|

Re: libp11 and PKCS11_enumerate_slots

Ludovic Rousseau
On 31/08/05, Andreas Jellinghaus <[hidden email]> wrote:
> On Wednesday 31 August 2005 11:25, Ludovic Rousseau wrote:
> > I do not make pkcs11_destroy_all_slots() public yet since I don't know
> > if it is really usefull. It would be better for the API symetry
> > (allocate & free functions). I will propose a patch later.
>
> too late :) but not yet commited. see http://www.opensc.org/files/wip/
> for a diff and patched version.

Some comments:
- src/libp11.h contains the arguments names for
PKCS11_enumerate_slots(), PKCS11_destroy_all_slots() and
PKCS11_find_token. It is not a real problem but is not homogeneous
with the rest of the file. A cut-n-paste problem? :-)
- your version of PKCS11_destroy_all_slots() does not free the slots[]
array allocated by PKCS11_enumerate_slots(). You should add:
  OPENSSL_free(slots);

> I exported both destroy_slot and destroy_all_sloty. the former is
> propably a bad name and not needed. the later is needed for error
> handling and cleanup. but we might want a different name?

New name proposal: release_slot() ?

> > I think PKCS11_init_token() should only update the slots corresponding
> > to this token and not all the slots.
>
> agree. does anyone see a problem with such a change?

To do this PKCS11_init_token() need to have the list of slots. Two
possibilities:
1. pass slots and nslots as arguments of PKCS11_init_token()
2. get them from ctx (as it was) or from somewhere else

I think solution 1 is better because it makes "clear" that the slot
list will be updated. The API should not hide important side effects.

Comments?

--
  Dr. Ludovic Rousseau
 For private mail use [hidden email] and not "big brother" Google
_______________________________________________
opensc-devel mailing list
[hidden email]
http://www.opensc.org/cgi-bin/mailman/listinfo/opensc-devel
Reply | Threaded
Open this post in threaded view
|

Re: libp11 and PKCS11_enumerate_slots

Andreas Jellinghaus-2
On Thursday 01 September 2005 09:05, Ludovic Rousseau wrote:
> Some comments:
> - src/libp11.h contains the arguments names for
> PKCS11_enumerate_slots(), PKCS11_destroy_all_slots() and
> PKCS11_find_token. It is not a real problem but is not homogeneous
> with the rest of the file. A cut-n-paste problem? :-)

lets say: long term plan is to write the whole doxygen documentation
in libp11.h and then every argument needs a name. so yes it is incosistent,
but also yes: it is the way to go.

> - your version of PKCS11_destroy_all_slots() does not free the slots[]
> array allocated by PKCS11_enumerate_slots(). You should add:
>   OPENSSL_free(slots);

oops, missed that one. thanks.

> > I exported both destroy_slot and destroy_all_sloty. the former is
> > propably a bad name and not needed. the later is needed for error
> > handling and cleanup. but we might want a different name?
>
> New name proposal: release_slot() ?

good idea! will change that.

> To do this PKCS11_init_token() need to have the list of slots. Two
> possibilities:
> 1. pass slots and nslots as arguments of PKCS11_init_token()
> 2. get them from ctx (as it was) or from somewhere else
>
> I think solution 1 is better because it makes "clear" that the slot
> list will be updated. The API should not hide important side effects.

ok, will implement it that way.

Thanks, Andreas
_______________________________________________
opensc-devel mailing list
[hidden email]
http://www.opensc.org/cgi-bin/mailman/listinfo/opensc-devel