Aleksey Sanin wrote:
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.
Thanks.
2) src/nss/akmngr.c I would really love to merge xmlSecNssAppliedKeysMngr and xmlSecNssDefaultKeysMngr if there is no objections.
Good.
:-( Pardon me for my lazy and time limitation when I implemented the patch. However, it's in our plan to implement the apps. I'd be grateful if you would help us on the implementation. :-) If you have no time, please let me know what urgent you need these implementations.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.
In fact, it is a complete new implement. After Tej contribute his engine, I don't think his impl meet the requirements, so I didn't made much changes over this file. Yes, I forgot to add the compiler conditions before klass definition and *GetKlass* functions. Yes, it is a little hard to read, I'm ready to answer any questions. :-)
5) src/nss/keytrans.c Does this file eliminates need for kt_rsa.c?
Yes.
6) src/nss/keywrappers.c Does this file eliminates need for kw_aes.c and kw_des.c?
Yes.
Yes, you're right. The values have been set to null at initializor and finalizor.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.
Thanks.
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.cI think we have talked about the topic at 2003, and you also give me the idea that binding the slot with mechanisms. OpenOffice really use this function in order to support multiple crypto database/slot. I don't think it is the crypto layer responsibility, crypto layer have provide the getBest for general usage, and it also provide the funtions to specify a certain slot. Here, I just use the specified slot while I don't want to get best from a dozen, I want to get the one that I want. Crypto layer has provide multiple choices and the ways, why xmlsec only permit just one way, and close anyother doors?
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.
I think, xxxNodeWrite() is used by xxxXmlWrite. In the xmlSecNssKeyDataX509XmlWrite(), no xxxNodeWrite() is called. So I think they're useless. Then why I get rid of the xxxNodeWrite(), and create a new wheel of xxxXmlWrite()? In the older implementation, xml writer will get rid of the old and create a completely new signature/encryption template regardless what ever in it. It is not the expected experience. Sometimes, it is necessary to keep the template unchanged during signning or at least keep its original struct and add a little necessary stuff. However, I have to say that my implement also have the drawbacks over the requirement, I faild to find any usable control info from keyInfoCtx to character the template.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?
I'm not use the NameRead function, StringRead funtion is only used by NameRead, so both of them are gone. I couldn't see any necessary to call NameRead before handling nss name.12) src/nss/x509vfy.c StringRead/NameRead functions are gone and I am not sure I see the replacement. Am I missing something?
Thanks, Andrew
_______________________________________________ xmlsec mailing list [email protected] http://www.aleksey.com/mailman/listinfo/xmlsec
_______________________________________________ xmlsec mailing list [email protected] http://www.aleksey.com/mailman/listinfo/xmlsec
