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

[Aleksey Sanin]

Hi, Andrew!

Thanks for your comments! Please see my comments interlaced with
yours bellow. I also have one huge request. Can you provide copyright
information for the new files, please? And also I would be glad to
extend the xmlsec authors list if you provide me with a list of
names :)

Aleksey



>> xmlsec-nss comments
>> --------------------------------------------------------
>> 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.
Well, I would need these functions in order to test the code :)
Anyway, I am not sure I understand why the existing versions would
not work. I'll look into this.


>> 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. :-)
> 
Ah... OK, if it is a new code then it makes sense :) Let me look at it
from the new angle :)

>> 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.
> 
Good, this change is dropped.

>> 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.
>>
> :-)
Still in progress... :)

>> 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?
Yes, I remember that discussion. However, I was under impression that
you want to do it in the application itself. Anyway, I wonder if you can
take a look at NSS folks reply to my question about GetBestSlot here

http://groups-beta.google.com/group/netscape.public.mozilla.crypto/browse_thread/thread/8b39688b4d707111/af472f5a8552f8b6?q=Aleksey+Sanin+nss#af472f5a8552f8b6

It might make sense to go directly to NSS instead of creating yet
another level.

> 
>> 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.
> 
Thanks for explanations and let me look at this more.


>> 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.
OK, I see :)