135429 - redesign smart card cache

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement, P1)

Description

[Ian McGreer]

I made the following observations about the smart card cache in NSS 3.4:

1.  It is only capable of caching certs.  For other objects (CRL's, trust), it
merely indicates whether or not the token has such objects.

2.  The certs are cached within the high-level trust domain cache.  This means
the  high-level cache has to be synchronized with token removals and insertions.
 It also means trust domain code must detect a hardware token explicitly (via
nssToken_SearchCerts) and choose thereby whether or not to search the token, or
accept the response from its cache.

Problem 1 shows that the caching code is not extensible.  Problem 2 creates a
lot of ugly code in trying to keep the high-level cache in sync with what should
be a low-level cache.

For these reasons, I decided to reimplement the smart card cache.  I decided it
should live on the actual token, independent of the high-level structures
utilizing it.  It should be generic: what is cached is simply the object handle
and its attributes.  It should be tightly bound to the token: the token knows
when it has been added/removed/logged in/logged out, and reflect these changes
in the cache immediately.  Because it is bound to the token, high-level code (in
the trust domain) does not have to know about it.

I will attach the code I implemented.

Comment 1

[Ian McGreer]

Attachment: code to implement new smart card cache

Comment 2

[Ian McGreer]

Some explanations:

What the cache stores are nssCryptokiObjectAndAttribute's.  As you might guess,
this is simply a lightweight reference to a token object, and the set of
attributes associated with it.

The cache is created along with the token, via a call to
nssTokenObjectCache_Create.  Nothing is cached at this time; only the knowledge
of what objects are to be cached.

The function clear_cache removes all objects in the cache, but the cache remains
active.

The function nssTokenObjectCache_HaveObjectClass notifies the caller whether or
not a certain cryptoki object is present in the cache.  If it returns true, the
caller will use the methods described below in lieu of the Crypoki equivalents.

The function nssTokenObjectCache_FindObjectsByTemplate is equivalent to
C_FindObjects[Init||Final] for the token.  That is, for object classes that are
being cached, this call is the authoritative response for the token.  If there
are no such objects, it returns immediately.  Otherwise, the token state is
checked via a call to search_for_objects.  The state diagram for the token is
shown in comments above that call.  If matching objects are located, they are
copied out of the cache and returned to the caller.

The function nssTokenObjectCache_GetObjectAttributes is equivalent to
C_GetAttributeValue for cached objects.  There is currently no code utilizing
C_SetAttributeValue, so I did not implement the equivalent yet.

The functions nssTokenObjectCache_ImportObject and
nssTokenObjectCache_RemoveObject handle object import and deletion.

Bob, would you like to review the overall design, and then take a look at the
code?  This involves more lines of code, but I feel it is much more clear.  I
believe we need to do this; having the cache tightly bound to the token will
make it much easier to debug, and relieve strain on higher-level code.  This
would be targeted for 3.5, but there is no milestone yet.

Comment 3

[Robert Relyea]

Hear, hear! This is the logic I felt we needed to implement from the beginning.

The one thing I think I would like to see, though, is an extension of this
caching scheme:

Rather than arbitarily pick tokens, I'd like to see a caching scheme that would
work for all tokens:

We'd pick some threshold (20 objects for instance). At startup we'd as the token
to give us the first 20 certs, trust object, crls, etc. If the token returns
less then 20 of these objects, we cache them in total. If the token returns
exactly 20 objects, we treat the cache as an LRU cache (or a circular cache) and
rotate through whenever we do searched for certs. In the latter case a cache
miss still results in a pk11 find objects call. [in the above example, 20 is
just an example, I'm not sure what a reasonable cache size should be, 10 may be
adequate].

Structurally we should put this cache in 'dev' in the function that actually
implements the find objects for a given token. The cache should always be
functioning, and the higher level code should simply 'search' the token.

In generaly, this is definately the right direction.

bob

Comment 4

[Ian McGreer]

Bob,

Right now, the cache is protected by a single lock, effectively serializing
access to token objects that are cached.  I decided this is OK for now, since
there won't be many objects (maximum is 10), so the searches should be short and
sweet.  In general, I think LRU'ing is something that *should* be done at a
higher level (I have already implemented an LRU certificate cache at the trust
domain level).  At this level, I believe the token should be responsible for
optimization.  For example, CKFW could have an LRU cache of objects, defaulting
to database search for misses.

I think the cache described here should be restricted to the common case of a
slow hardware token that only has a couple of certs, since this cache will speed
up searches for that case.  If the token has a large number of certs, it better
be able to search them in a reasonable amount of time.

Comment 5

[Robert Relyea]

I'm not tied to LRU, but I still think the cache should function 1) in an
automatic basis, and 2) potentially over all the tokens.

Keeping the size down to 10 seems reasonable. rotating through the cache round
robin is fine. 

The point is 1) these should be true caches (the only upper semantic difference
with the upper level code is the fact that the accesses are faster). 2) should
should be able to arbitrarily flush the cache and the code should still work.
Upper level code should be ignorant of the fact that the cache is functioning.

Also we should be caching the 'raw' objects at this level. Caching of the
decoded versions of the object rightly belongs at the next level up.

bob

Comment 6

[Ian McGreer]

Attachment: implement cache for all tokens

Here is the full patch to activate the cache with the tip.  Notes:

1.  I went ahead and turned it on for all tokens.  I have already observed the
beneficial effect of this -- the cache is queried for all tokens for CRL's,
since I don't have any.  This means we save a database query every time we call
CERT_VerifyCert.

2.  This passed the QA, and since the cert7.db's in the QA never have more than
10 certs, the cache was being used in all cases.  I still plan on doing more
testing though.

3.  The most critical changes in this patch are the reimplementations of
PK11_ImportCert and the CRL routines.  I don't have any CRL's, so I haven't
tested it.  Bob, you did some testing with CRL's, right?  How hard would it be
to add a few crlutil tests to tools.sh?

Comment 7

[Ian McGreer]

I have checked in this patch (with a few revisions) so that Bob can test NSS 3.5
clients with it.

Comment 8

[Ian McGreer]

I have done some testing and made some additional fixes.  There are two open issues.

1.  The softoken does not return a value for CKA_NETSCAPE_EMAIL.  This means
email searches cannot go through the cache.  I have modified
PK11_FindCertFromNickname (where OrEmail is implied) to only search the token by
email address if the '@' character is in the nickname string.  I'm also
wondering, can we assume that only the softoken will honor CKA_NETSCAPE_EMAIL
searches?  In that case, we could restrict lookups by email address to only the
internal slot, avoiding wasted lookups on slow tokens.

2.  The S/MIME profile does not go through the cache.  Are we hitting tokens
looking for those objects?  If so, it could be added.

Comment 9

[Wan-Teh Chang]

Changed the QA contact to Bishakha.

Comment 10

[Wan-Teh Chang]

Set priority to P1.  This is important work.

Comment 11

[Ian McGreer]

*** Bug 129295 has been marked as a duplicate of this bug. ***

Comment 12

[Ian McGreer]

This work has been checked in.  Bob reported that smart card handling has
improved.  Marking bug fixed in 3.5.

Comment 13

[Jaime Rodriguez, Jr.]

adt1.0.1+ (on ADT's behalf) for checkin to the 1.0 branch. Pls check this in
asap. thanks! 

Comment 14

[Julien Pierre]

Ian, late response to #8 :

1) Yes, only softoken will respond to CKA_NETSCAPE_EMAIL , as I found the hard 
way . I don't know if checking for @ in the nickname (email address) is 
correct, it sounds like it might be. I'm not certain if all email addresses are 
required to have that character. I'm just not sure of the correct RFC to check 
this. It would be a useful optimization if we could avoid hitting hardware 
tokens in this case, though they should recognize quickly in the search 
template that they don't know this attribute type and return. Keyword in the 
previous sentence is should ...

2) I think we only looking for the S/MIME profile on softoken