[xmlsec] xmlsec-nss patch

[Tejkumar Arora]

Aleksey Sanin wrote:

 > Tej,
 >
 > It took less time than I expected to look at your changes :) Almost
 > everything looks
 > great except few minor things listed bellow. I have checked in your
 > patch into
 > the same XMLSEC_NSS_030714 with only one change (item 0) bellow).
 > However, there are a couple items (1) and 2)) that needs to be resolved
 > before we can put this code into the trunk.
 >
 > BTW, I  can't run the xmlsec-nss tests because of certutiland I could
 > not test xmlsec-nss
 > with valgrind :(

That's a bummer. certutil is available in an official NSS
distribution - I did not expect that mozilla-nss package
will not have it.... I'll try to figure something out..

 >
 > Aleksey
 >
 >
 >
 > 0) I have moved the NSS DB reset code from test*.sh scripts to the top
 > level
 > Makefile.am. No need to repeat it again and again. This allowed me to
 > remove
 > xmlsec-config option from the test*.sh scripts. Also I don't think that
 > we want
 > to pass crypto_config to these scripts because nobody else should use it
 > anyway (it is also removed).

Another reason to use xmlsec-config in the *.sh scripts was
to derive the name of the valgrind suppression file (nss.supp,
openssl.supp....). I noticed that you just list them all -
not quite right but it works :).

 > 1) As I mentioned before, I don't have "cryptoutil" installed. Thus
 > other users
 > might miss it too :) We need to find a way to avoid using this tool in
 > the tests scripts.
 > Is it possible to initializa the nss db from xmlSecNssAppInit() instead
 > (if config is not
 > NULL and NSS DB does not exist there, create it)?

I had tried that, but it turns out that a db created
on the fly needs to have the password initialized. The
code in NSS to initialize the password is not exported....
so I'd have to file a NSS bug.
Another reason I went with the approach I took is that
it is more flexible (for example, it allows you to
pre-populate the db with anything you wanted...).
It gives any crypto implementation a chance to run
some start/stop scripts.

 > 2) xmlSecNssAppPkcs12Load(): why do you do these initializations in this
 > function?
 >     PORT_SetUCS2_ASCIIConversionFunction(xmlSecNssAppAscii2UCS2Conv);
 >     SEC_PKCS12EnableCipher(...)
 >     SEC_PKCS12EnableCipher(...)
 > It seems to me that it's a wrong idea. These initializations are global.
 > The function
 > can be called many times from multiple threads. IMO, these
 > initialization calls
 > need to be done once in applicaiton init function (xmlSecNssAppInit?).
 > Please take a look at it.

These are idempotent.... but I agree with you.
I'll take a look.

 > 3) keysstore.c: I wonder why do you need to have simple keys store in
 > default nss
 > keys manager? Why you could not use only NSS keydb?

The generic keysstore interface defines the keystorefindmethod
to return a "xmlsecKeyPtr". In other words, the method is
returning xmlsec objects and not just crypto objects.
The xmlsec object contain a lot more info than
just the crypto handles. So NSS db can't replace the
keystore completely.  I'm using the NSS db as an
alternative source of keys (sort of a "read-through" cache).

I was thinking about speeding up lookups in the simple
keysstore, but it is not easy to replace the list with
a hashtable because many different criteria are used
to lookup a key, and may not all be defined (name, type,
usage, etc..).




 > 4) warnings about __CERT_NewTempCertificate function:
 >     src/nss/app.c:847: warning: implicit declaration of function
 > `__CERT_NewTempCertificate'
 >     x509.c:1494: warning: implicit declaration of function
 > `__CERT_NewTempCertificate'
 > Is it worth it to put a declaration somewhere in xmlsec-nss headers if
 > it does not exist in NSS?

I have this in my list of nss problems to report.
I can put it in xmlsec-nss header as a workaround fow
now.


-Tej