santuario-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Colm O hEigeartaigh <cohei...@apache.org>
Subject Re: [jira] [Commented] (SANTUARIO-349) Update JCP dsig code to simplify serialization
Date Tue, 18 Dec 2012 14:05:47 GMT
Hi Eric,

> q1) It is quite curious that the new marshaling code produces invalid
signatures. Looking at the unmarshalling code, that code does
> not enforce the proper element and namespace names. If it did, it would
catch the mistake I made. Should I add such enforcement?
> (Otherwise, it is apparently possible to unmarshal an XML Signature
element that doesn't conform to the specification at all, because
> the caller can use arbitrary names and namespaces for some of the
elements). Should I file this as a separate bug?

Yes please file this as a separate bug - we should definately be enforcing
the correct namespaces as well.

> q2) Do you think I should add javax.xml.validation API calls to perform
schema validation to the produced signatures in some/all of
> the test cases that call XMLSignature.sign()? That would have caught the
problem you discovered.

Yes please do.

Thanks,

Colm.

On Mon, Dec 17, 2012 at 9:44 PM, Eric Johnson <eric@tibco.com> wrote:

> Hmmm - still looking for feedback on two questions:
>
>
> q1) It is quite curious that the new marshaling code produces invalid
> signatures. Looking at the unmarshalling code, that code does not enforce
> the proper element and namespace names. If it did, it would catch the
> mistake I made. Should I add such enforcement? (Otherwise, it is apparently
> possible to unmarshal an XML Signature element that doesn't conform to the
> specification at all, because the caller can use arbitrary names and
> namespaces for some of the elements). Should I file this as a separate bug?
>
> q2) Do you think I should add javax.xml.validation API calls to perform
> schema validation to the produced signatures in some/all of the test cases
> that call XMLSignature.sign()? That would have caught the problem you
> discovered.
>
> I've made the dispatching changes that I outlined below, which you'll see
> in the next version of the patch. The change disentangles the serialization
> code, so upon reflection I just went ahead and made the change.
>
> Eric.
>
>
> On 12/12/12 3:07 PM, Eric Johnson wrote:
>
>> Hi Colm,
>>
>> Quick responses & questions:
>>
>> First, responses:
>> a) I've added the appropriate headers on the new files.
>>
>> b) Glad you caught the typo.
>>
>> c) I'll have to try running the WSS4J tests too, to see if I can figure
>> that out - at least I'm hoping it is as easy as just running mvn test?
>>
>> d) I have no idea how spaces snuck into the patch, because I have Eclipse
>> configured for spaces. Sigh.
>>
>> e) Structure - the "Marshaller" class is an odd construct that I'm not
>> entirely happy with. Now that I think about it, I'm going to make two
>> improvements:
>>
>> e.1) Move all the static marshall methods that it calls (such as
>> DOMX509IssuerSerial.marshal(), DOMX509Data.marshal(), ...) out of the DOM
>> specific class and into a generic class (perhaps Marshaller). This removes
>> them from the DOM specific code.
>>
>> e.2) Change the Marshaller.marshall() code and replace the if/else
>> if/else if... logic by a search through an array for a class match that
>> then invokes a method.
>>
>> XmlWriter gets a new method:
>>     void marshallObject(XMLStructure toMarshall, String dsPrefix,
>> XMLCryptoContext context).
>>
>> The concrete implementation of this method then spins through a list, and
>> when it finds an isInstance() match, invokes a registered function to do
>> the marshalling.
>>
>> Three questions:
>>
>> q1) It is quite curious that the new marshaling code produces invalid
>> signatures. Looking at the unmarshalling code, that code does not enforce
>> the proper element and namespace names. If it did, it would catch the
>> mistake I made. Should I add such enforcement? (Otherwise, it is apparently
>> possible to unmarshal an XML Signature element that doesn't conform to the
>> specification at all, because the caller can use arbitrary names and
>> namespaces for some of the elements). Should I file this as a separate bug?
>>
>> q2) Do you think I should add javax.xml.validation API calls to perform
>> schema validation to the produced signatures in some/all of the test cases
>> that call XMLSignature.sign()? That would have caught the problem you
>> discovered.
>>
>> q3) Do you have objections/concerns about the proposed change to the
>> "dispatch" logic of the marshalling code?
>>
>> Eric
>>
>>
>> On 12/8/12 7:03 AM, Colm O hEigeartaigh (JIRA) wrote:
>>
>>>      [ https://issues.apache.org/**jira/browse/SANTUARIO-349?**
>>> page=com.atlassian.jira.**plugin.system.issuetabpanels:**
>>> comment-tabpanel&**focusedCommentId=13527159#**comment-13527159<https://issues.apache.org/jira/browse/SANTUARIO-349?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13527159#comment-13527159>]
>>>
>>> Colm O hEigeartaigh commented on SANTUARIO-349:
>>> ------------------------------**-----------------
>>>
>>> Hi Eric,
>>>
>>> As if to prove my point, I found a bug in some downstream testing in
>>> another project (WSS4) ;-)
>>>
>>> In DOMTransform, the following lines:
>>>
>>> +        xwriter.writeStartElement(**dsPrefix,
>>> +                localName.equals("Transforms") ? "Transforms" :
>>> "CanonicalizationMethod", XMLSignature.XMLNS);
>>>
>>> should be:
>>>
>>> +        xwriter.writeStartElement(**dsPrefix,
>>> +                localName.equals("Transforms") ? "Transform" :
>>> "CanonicalizationMethod", XMLSignature.XMLNS);
>>>
>>> It was writing out ds:Transforms/ds:Transforms. Not sure why this wasn't
>>> caught in the Santuario tests...perhaps they are not as thorough as they
>>> should be.
>>>
>>> I am getting a lot of errors in the WSS4J streaming tests that use the
>>> DOM code to create signatures, and the streaming code to validate them with
>>> this patch applied. I will need to dig deeper as to whether this is a bug
>>> in the streaming code, or in the patch.
>>>
>>> Could you resubmit the patch with this fix + with the appropriate Apache
>>> headers on the new files as before? (and no tabs as well please).
>>>
>>> Colm.
>>>
>>
>


-- 
Colm O hEigeartaigh

Talend Community Coder
http://coders.talend.com

Mime
View raw message