You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@santuario.apache.org by Eric Johnson <er...@tibco.com> on 2012/12/13 00:07:48 UTC
Re: [jira] [Commented] (SANTUARIO-349) Update JCP dsig code to simplify
serialization
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 ]
>
> 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.
Re: [jira] [Commented] (SANTUARIO-349) Update JCP dsig code to
simplify serialization
Posted by Colm O hEigeartaigh <co...@apache.org>.
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 <er...@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
Re: [jira] [Commented] (SANTUARIO-349) Update JCP dsig code to simplify
serialization
Posted by Eric Johnson <er...@tibco.com>.
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
>> ]
>>
>> 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.