You are viewing a plain text version of this content. The canonical link for it is here.
Posted to wss4j-dev@ws.apache.org by Nandana Mihindukulasooriya <na...@gmail.com> on 2008/04/23 08:54:14 UTC
Error in the logic of retrieving decrypted nodes in the reference list processor
Hi Devs,
In the Reference List processor , we get decrypted nodes using the
following logic.
We keep a list of elements before decryption of an element and we compare it
with list of elements after the element is decrypted. We do this using the
the following code.
final java.util.List ret = new java.util.ArrayList();
for (
final java.util.Iterator bpos = b.iterator();
bpos.hasNext();
) {
final Node bnode = (Node) bpos.next();
final java.lang.String bns = bnode.getNamespaceURI();
final java.lang.String bln = bnode.getLocalName();
boolean found = false;
for (
final java.util.Iterator apos = a.iterator();
apos.hasNext();
) {
final Node anode = (Node) apos.next();
final java.lang.String ans = anode.getNamespaceURI();
final java.lang.String aln = anode.getLocalName();
final boolean nsmatch =
ans == null
? ((bns == null) ? true : false)
: ((bns == null) ? false : ans.equals(bns));
final boolean lnmatch =
aln == null
? ((bln == null) ? true : false)
: ((bln == null) ? false : aln.equals(bln));
if (nsmatch && lnmatch) {
found = true;
}
}
if (!found) {
ret.add(bnode);
}
As we can see, we check the presence of the elements using the qualified
names of the elements. But this not always work. Say for example, we encrypt
the signature of the message and the message also have a endorsing
signature.
List A List B
EncryptedData (Encrypted signature) Signature (Decrypted Signature)
Signature (Endorsing signature) Signature (Endorsing
Signature)
According the above logic, we will not get the decrypted signature as a new
node. So shall we check the new nodes using object references,
final java.util.List ret = new java.util.ArrayList();
for ( final java.util.Iterator bpos = b.iterator();
bpos.hasNext(); ) {
final Node bnode = (Node) bpos.next();
boolean found = false;
for (final java.util.Iterator apos = a.iterator();
apos.hasNext();) {
final Node anode = (Node) apos.next();
if (bnode.equals(anode)) {
found = true;
}
}
if (!found) {
ret.add(bnode);
}
WDYT ?
thanks,
/nandana
Re: Error in the logic of retrieving decrypted nodes in the reference
list processor
Posted by Fred Dushin <fa...@apache.org>.
That sounds promising.
I see this code was taken from the EncryptedKeyProcessor, so
presumably this processor needs to be changed, as well. Even better,
let's put these processors under a common base, and define the common
operation there. (The code is mine, and went in under WSS-57, so I
guess I take responsibility for the bug).
The only thing I'm concerned about is calling equals on a DOM Node
type. I'm wondering if there will be sensitivities to specific DOM
implementations (which can vary widely, across WSS4J deployments),
which may choose to implement equals in idiosyncratic manners.
Maybe this is the more reliable operation to use:
http://java.sun.com/j2se/1.5.0/docs/api/org/w3c/dom/Node.html#isEqualNode(org.w3c.dom.Node)
Next question: Is this blocking for you, and did it show up in RC1
testing? If we do fix it for 1.5.4, I suppose we can fix it on the
1_5_4 branch, and then merge to trunk after the release.
-Fred
PS> RC1 testing against CXF is going okay, though I've had to make
some slight modifications to the POM to get things just right. I'd
like to do an RC2 (with the BouncyCastle changes, as well), if folks
are open to that.
On Apr 23, 2008, at 2:54 AM, Nandana Mihindukulasooriya wrote:
> Hi Devs,
> In the Reference List processor , we get decrypted nodes
> using the following logic.
>
> We keep a list of elements before decryption of an element and we
> compare it with list of elements after the element is decrypted. We
> do this using the the following code.
>
> final java.util.List ret = new java.util.ArrayList();
> for (
> final java.util.Iterator bpos = b.iterator();
> bpos.hasNext();
> ) {
> final Node bnode = (Node) bpos.next();
> final java.lang.String bns = bnode.getNamespaceURI();
> final java.lang.String bln = bnode.getLocalName();
> boolean found = false;
> for (
> final java.util.Iterator apos = a.iterator();
> apos.hasNext();
> ) {
> final Node anode = (Node) apos.next();
> final java.lang.String ans =
> anode.getNamespaceURI();
> final java.lang.String aln = anode.getLocalName();
> final boolean nsmatch =
> ans == null
> ? ((bns == null) ? true : false)
> : ((bns == null) ? false : ans.equals(bns));
> final boolean lnmatch =
> aln == null
> ? ((bln == null) ? true : false)
> : ((bln == null) ? false : aln.equals(bln));
> if (nsmatch && lnmatch) {
> found = true;
> }
> }
> if (!found) {
> ret.add(bnode);
> }
>
> As we can see, we check the presence of the elements using the
> qualified names of the elements. But this not always work. Say for
> example, we encrypt the signature of the message and the message
> also have a endorsing signature.
>
> List A
> List B
> EncryptedData (Encrypted signature) Signature (Decrypted
> Signature)
> Signature (Endorsing signature) Signature
> (Endorsing Signature)
>
> According the above logic, we will not get the decrypted signature
> as a new node. So shall we check the new nodes using object
> references,
>
> final java.util.List ret = new java.util.ArrayList();
> for ( final java.util.Iterator bpos = b.iterator();
> bpos.hasNext(); ) {
> final Node bnode = (Node) bpos.next();
> boolean found = false;
> for (final java.util.Iterator apos = a.iterator();
> apos.hasNext();) {
> final Node anode = (Node) apos.next();
> if (bnode.equals(anode)) {
> found = true;
> }
> }
> if (!found) {
> ret.add(bnode);
> }
>
> WDYT ?
>
> thanks,
> /nandana
>
---------------------------------------------------------------------
To unsubscribe, e-mail: wss4j-dev-unsubscribe@ws.apache.org
For additional commands, e-mail: wss4j-dev-help@ws.apache.org
Re: Error in the logic of retrieving decrypted nodes in the reference list processor
Posted by Nandana Mihindukulasooriya <na...@gmail.com>.
Hi Fred,
So I think I'd propose we try a pointer comparison (==) and see how well
> that works.
>
+1
thanks,
nandana
Re: Error in the logic of retrieving decrypted nodes in the reference
list processor
Posted by Fred Dushin <fa...@apache.org>.
Okay, so what would the relative risk be of using "==" in lieu of
"equals"?
Let's remind ourselves of what the function in question is trying to
do -- to find the relative complement (A - B) of 2 collections of
Nodes, where A is the collection of nodes After decryption, and B is
the collection of nodes Before encryption.
So the question is, will the XMLSec xmlCipher.doFinal(doc,
encBodyData, content) operation have any other side-effects on the
input than doing some pointer surgery on the DOM tree?
I think we could probably assume (with a fat comment around it) that
the changes to the tree will be stretegic and surgical. Or at least
we can make the change on that assumption, and test accordingly.
So I think I'd propose we try a pointer comparison (==) and see how
well that works.
I agree that isSameNode may be the better choice (in a 1.5 scenario).
The equals comparison could prove costly.
Nanada, were you going to make this change on the 1_5_4 fixes branch?
Or did you want me to?
-Fred
On Apr 24, 2008, at 5:36 AM, O hEigeartaigh, Colm wrote:
>
> Hi guys,
>
>> According the documentation "This method tests for equality of
>> nodes, not sameness (i.e., whether the two nodes are references to
>> the same object)
>> which can be tested with Node.isSameNode()" , So IMHO what we want
>> to check is isSameNode instead of isEqualNode. WDYT ?
>
> Both are non-runners for this release, as neither isSameNode() or
> isEqualNode() are available in JDK < 1.5.
>
> Colm.
>
> ________________________________________
> From: Nandana Mihindukulasooriya [mailto:nandana.cse@gmail.com]
> Sent: 24 April 2008 05:28
> To: Fred Dushin; wss4j-dev
> Subject: Re: Error in the logic of retrieving decrypted nodes in the
> reference list processor
>
> Hi Fred,
> I see this code was taken from the EncryptedKeyProcessor, so
> presumably this processor needs to be changed, as well. Even
> better, let's put these processors under a common base, and define
> the common operation there.
>
> Exactly. There is a huge code duplication in those two processors
> and ideally they should have a common base. But i thought this is
> not the best time to do the refactoring as we are only 1 or 2 weeks
> away from the release.
> Maybe this is the more reliable operation to use:
> http://java.sun.com/j2se/1.5.0/docs/api/org/w3c/dom/Node.html#isEqualNode(org.w3c.dom.Node)
>
> According the documentation "This method tests for equality of
> nodes, not sameness (i.e., whether the two nodes are references to
> the same object) which can be tested with Node.isSameNode()" , So
> IMHO what we want to check is isSameNode instead of isEqualNode.
> WDYT ?
> Next question: Is this blocking for you, and did it show up in RC1
> testing? If we do fix it for 1.5.4, I suppose we can fix it on the
> 1_5_4 branch, and then merge to trunk after the release.
>
> Yep, this a blocker for Rampart. Anyway it seems that this code has
> been there since 3/25/07 but now only it causes a problem as we are
> actually using these results for encryption validation recently. So
> shall I commit this to both the branch and the trunk ?
>
> PS> RC1 testing against CXF is going okay, though I've had to make
> some slight modifications to the POM to get things just right. I'd
> like to do an RC2 (with the BouncyCastle changes, as well), if folks
> are open to that.
>
> Yes, That will be great. Testing against Rampart/Axis2 also going
> okay (except for this issue).
>
> regards,
> nandana
>
> ----------------------------
> IONA Technologies PLC (registered in Ireland)
> Registered Number: 171387
> Registered Address: The IONA Building, Shelbourne Road, Dublin 4,
> Ireland
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: wss4j-dev-unsubscribe@ws.apache.org
> For additional commands, e-mail: wss4j-dev-help@ws.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: wss4j-dev-unsubscribe@ws.apache.org
For additional commands, e-mail: wss4j-dev-help@ws.apache.org
RE: Error in the logic of retrieving decrypted nodes in the reference list processor
Posted by "O hEigeartaigh, Colm" <Co...@iona.com>.
Hi guys,
> According the documentation "This method tests for equality of nodes, not sameness (i.e., whether the two nodes are references to the same object)
> which can be tested with Node.isSameNode()" , So IMHO what we want to check is isSameNode instead of isEqualNode. WDYT ?
Both are non-runners for this release, as neither isSameNode() or isEqualNode() are available in JDK < 1.5.
Colm.
________________________________________
From: Nandana Mihindukulasooriya [mailto:nandana.cse@gmail.com]
Sent: 24 April 2008 05:28
To: Fred Dushin; wss4j-dev
Subject: Re: Error in the logic of retrieving decrypted nodes in the reference list processor
Hi Fred,
I see this code was taken from the EncryptedKeyProcessor, so presumably this processor needs to be changed, as well. Even better, let's put these processors under a common base, and define the common operation there.
Exactly. There is a huge code duplication in those two processors and ideally they should have a common base. But i thought this is not the best time to do the refactoring as we are only 1 or 2 weeks away from the release.
Maybe this is the more reliable operation to use:
http://java.sun.com/j2se/1.5.0/docs/api/org/w3c/dom/Node.html#isEqualNode(org.w3c.dom.Node)
According the documentation "This method tests for equality of nodes, not sameness (i.e., whether the two nodes are references to the same object) which can be tested with Node.isSameNode()" , So IMHO what we want to check is isSameNode instead of isEqualNode. WDYT ?
Next question: Is this blocking for you, and did it show up in RC1 testing? If we do fix it for 1.5.4, I suppose we can fix it on the 1_5_4 branch, and then merge to trunk after the release.
Yep, this a blocker for Rampart. Anyway it seems that this code has been there since 3/25/07 but now only it causes a problem as we are actually using these results for encryption validation recently. So shall I commit this to both the branch and the trunk ?
PS> RC1 testing against CXF is going okay, though I've had to make some slight modifications to the POM to get things just right. I'd like to do an RC2 (with the BouncyCastle changes, as well), if folks are open to that.
Yes, That will be great. Testing against Rampart/Axis2 also going okay (except for this issue).
regards,
nandana
----------------------------
IONA Technologies PLC (registered in Ireland)
Registered Number: 171387
Registered Address: The IONA Building, Shelbourne Road, Dublin 4, Ireland
---------------------------------------------------------------------
To unsubscribe, e-mail: wss4j-dev-unsubscribe@ws.apache.org
For additional commands, e-mail: wss4j-dev-help@ws.apache.org
Re: Error in the logic of retrieving decrypted nodes in the reference list processor
Posted by Nandana Mihindukulasooriya <na...@gmail.com>.
Hi Fred,
I see this code was taken from the EncryptedKeyProcessor, so presumably this
> processor needs to be changed, as well. Even better, let's put these
> processors under a common base, and define the common operation there.
>
Exactly. There is a huge code duplication in those two processors and
ideally they should have a common base. But i thought this is not the best
time to do the refactoring as we are only 1 or 2 weeks away from the
release.
Maybe this is the more reliable operation to use:
>
> http://java.sun.com/j2se/1.5.0/docs/api/org/w3c/dom/Node.html#isEqualNode(org.w3c.dom.Node)<http://java.sun.com/j2se/1.5.0/docs/api/org/w3c/dom/Node.html#isEqualNode%28org.w3c.dom.Node%29>
>
According the documentation "This method tests for equality of nodes, not
sameness (i.e., whether the two nodes are references to the same object)
which can be tested with Node.isSameNode()" , So IMHO what we want to check
is isSameNode instead of isEqualNode. WDYT ?
Next question: Is this blocking for you, and did it show up in RC1 testing?
> If we do fix it for 1.5.4, I suppose we can fix it on the 1_5_4 branch, and
> then merge to trunk after the release.
>
Yep, this a blocker for Rampart. Anyway it seems that this code has been
there since 3/25/07 but now only it causes a problem as we are actually
using these results for encryption validation recently. So shall I commit
this to both the branch and the trunk ?
> PS> RC1 testing against CXF is going okay, though I've had to make some
> slight modifications to the POM to get things just right. I'd like to do an
> RC2 (with the BouncyCastle changes, as well), if folks are open to that.
>
Yes, That will be great. Testing against Rampart/Axis2 also going okay
(except for this issue).
regards,
nandana