Andrew, Chandler!
I started reviewing xmlsec-nss patch and it looks great! But it is also really huge and it would take me time to process it :) Please find bellow my comments after first pass.
Aleksey
xmlsec-nss comments -------------------------------------------------------- 1) src/nss/digests.c src/nss/hmac.c src/nss/pkikeys.c src/nss/signatures.c src/nss/x509.c src/nss/x509vfy.c
More detailed error message inluding nspr error code. Looks good, checked in.
2) src/nss/akmngr.c I would really love to merge xmlSecNssAppliedKeysMngr and xmlSecNssDefaultKeysMngr if there is no objections.
3) src/nss/crypto.c, xmlSecCryptoGetFunctions_nss() I wonder why the patch sets all the App xmlsec-nss functions to NULL. This means that xmlsec command line utility will not run with xmlsec-nss at all.
4) src/nss/ciphers.c Quite a lot of things are moved around and this makes patch hard to read. Also almost all "#ifndef XMLSEC_NO_<ALG>" were removed thus it is hard to strip down xmlsec if needed.
5) src/nss/keytrans.c Does this file eliminates need for kt_rsa.c?
6) src/nss/keywrappers.c Does this file eliminates need for kw_aes.c and kw_des.c?
7) src/nss/pkikeys.c, xmlSecNSSPKIKeyDataCtxDup() I don't think there is a need to set ctxDst values to NULL here because it is already done in xmlSecNSSPKIKeyDataCtxFree()
8) src/nss/pkikeys.c, xmlSecNssPKIKeyDataAdoptKey() and xmlSecNssPKIAdoptKey() Good idea to check input public/private key types! Checked in.
9) src/nss/symkeys.c, conversion to NSS SymKey I like the idea! I just need to meditate on this code a little bit and run tests.
10) src/nss/tokens.c
If I understand the patch correctly, it provides a way for application
to select crypto slot on which xmlsec would perform the crypto
operation for each signature/encryption separately. NSS already has
built in controls for defining "best" slot for a given crypto algorithm
but this is done globaly for all application. I do see the difference.
However, it is hard for me to believe that a user would really go and specify a crypto slot (device) before each operation. It is much more
likely that this would be done in ten levels of preferences menu and
it will be done globaly.
Is it a requirement for you to support have such level of customization? I feel that this is more of crypto layer responsibility and I would love not to go into such details on xmlsec-crypto layer.
11) src/nss/x509.c Why the certificate xxxNodeWrite() functions were removed? Is it really necessary? How one would write certs into the output xml file?
12) src/nss/x509vfy.c StringRead/NameRead functions are gone and I am not sure I see the replacement. Am I missing something?
_______________________________________________ xmlsec mailing list [email protected] http://www.aleksey.com/mailman/listinfo/xmlsec
