[xmlsec] Big patch to xmlsec in recent OpenOffice.org sources

[Andrew Fan]

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.

> 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.
>
:-( 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.

> 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.

> 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()
>
Yes, you're right. The values have been set to null at initializor and 
finalizor.

> 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.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.
>
I 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?

> 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 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.

> 12) src/nss/x509vfy.c
> StringRead/NameRead functions are gone and I am not sure I see the
> replacement. Am I missing something?
>
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.

Thanks,
Andrew

>
> _______________________________________________
> xmlsec mailing list
> xmlsec at aleksey.com
> http://www.aleksey.com/mailman/listinfo/xmlsec