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 _______________________________________________ xmlsec mailing list [EMAIL PROTECTED] http://www.aleksey.com/mailman/listinfo/xmlsec
