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.
