Mailing-List: contact dev-help@santuario.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@santuario.apache.org
Received-SPF: pass (nike.apache.org: domain of eric@tibco.com designates
 63.100.100.143 as permitted sender)
Message-ID: <50C90E44.4060209@tibco.com>
Date: Wed, 12 Dec 2012 15:07:48 -0800
From: Eric Johnson <eric@tibco.com>
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7;
 rv:17.0) Gecko/17.0 Thunderbird/17.0
MIME-Version: 1.0
To: Apache Santuario Dev <dev@santuario.apache.org>
Subject: Re: [jira] [Commented] (SANTUARIO-349) Update JCP dsig code to
 simplify
 serialization
References: <JIRA.12617168.1353527711575@arcas>
 <JIRA.12617168.1353527711575.7810.1354979001418@arcas>
In-Reply-To: <JIRA.12617168.1353527711575.7810.1354979001418@arcas>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit

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.