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 <[email protected]> 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
