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.
