[jira] [Updated] (SANTUARIO-536) OutboundXMLSec.processOutMessage incorrectly uses SecurePart.getIdToSign for encryption

["Peter De Maeyer (Jira)" <ji...@apache.org>]

     [ https://issues.apache.org/jira/browse/SANTUARIO-536?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Peter De Maeyer updated SANTUARIO-536:
--------------------------------------
    Description: 
{{OutboundXMLSec.processOutMessage}} incorrectly uses {{SecurePart.getIdToSign}} for encryption.
There are two "if/else if" branches, one for signature and one for encryption.
Both branches however use {{SecurePart.getIdToSign}}, which is correct for the signature branch, but not for the encryption branch.
I believe that the encryption branch must use {{getIdToReference}} instead.
"I believe" because the lack of Javadoc and unit tests makes it impossible to "know" the intent, so I have to make an educated guess based on the following observations:
* {{SecurePart.getIdToReference}} is called nowhere, which smells - I suppose it's intended for _something_.
* Neither {{setIdToSign}} nor {{setIdToReference}} are called anywhere, indicating that there are no tests for it, which explains why this feature is broken.

The impact is that encryption based on matching XML attribute ID does not work as intended.

The workaround is to use the equivalent for signing, but that means you can't choose a different ID for elements that need both signing and encryption.

That being said, the feature "secure elements based on attribute ID" looks half-baked, like it was done in a hurry:
* No unit tests.
* This bug.
* Splitting of encryption ID and sign ID on the {{SecurePart}} API is incomplete: the API is split on {{SecurePart}} in {{get/setIdToSign}} and {{get/setIdToReference}}, but not on {{XMLSecurityProperties}}: there is only a single {{XMLSecurityProperties.get/setIdAttributeNS}}.

I wonder what is the vision is here? Unify encrypt/sign into a single set of "secure" methods that apply to both, or split encrypt and sign APIs completely? If you ask me, unifying makes most sense, but that's not what most of the code looks like...

  was:
{{OutboundXMLSec.processOutMessage}} incorrectly uses {{SecurePart.getIdToSign}} for encryption.
There are two "if/else if" branches, one for signature and one for encryption.
Both branches however use {{SecurePart.getIdToSign}}, which is correct for the signature branch, but not for the encryption branch.
I believe that the encryption branch must use {{getIdToReference}} instead.
"I believe" because the lack of Javadoc and unit tests makes it impossible to "know" the intent, so I have to make an educated guess based on the following observations:
* {{SecurePart.getIdToReference}} is called nowhere, which smells - I suppose it's intended for _something_.
* Neither {{setIdToSign}} nor {{setIdToReference}} are called anywhere, indicating that there are no tests for it, which explains why this feature is broken.

The impact is that encryption based on matching XML attribute ID does not work as intended.

The workaround is to use the equivalent for signing, but that means you can't choose a different ID for elements that need both signing and encryption.


> OutboundXMLSec.processOutMessage incorrectly uses SecurePart.getIdToSign for encryption
> ---------------------------------------------------------------------------------------
>
>                 Key: SANTUARIO-536
>                 URL: https://issues.apache.org/jira/browse/SANTUARIO-536
>             Project: Santuario
>          Issue Type: Bug
>          Components: Java
>    Affects Versions: Java 2.1.5
>            Reporter: Peter De Maeyer
>            Assignee: Colm O hEigeartaigh
>            Priority: Major
>
> {{OutboundXMLSec.processOutMessage}} incorrectly uses {{SecurePart.getIdToSign}} for encryption.
> There are two "if/else if" branches, one for signature and one for encryption.
> Both branches however use {{SecurePart.getIdToSign}}, which is correct for the signature branch, but not for the encryption branch.
> I believe that the encryption branch must use {{getIdToReference}} instead.
> "I believe" because the lack of Javadoc and unit tests makes it impossible to "know" the intent, so I have to make an educated guess based on the following observations:
> * {{SecurePart.getIdToReference}} is called nowhere, which smells - I suppose it's intended for _something_.
> * Neither {{setIdToSign}} nor {{setIdToReference}} are called anywhere, indicating that there are no tests for it, which explains why this feature is broken.
> The impact is that encryption based on matching XML attribute ID does not work as intended.
> The workaround is to use the equivalent for signing, but that means you can't choose a different ID for elements that need both signing and encryption.
> That being said, the feature "secure elements based on attribute ID" looks half-baked, like it was done in a hurry:
> * No unit tests.
> * This bug.
> * Splitting of encryption ID and sign ID on the {{SecurePart}} API is incomplete: the API is split on {{SecurePart}} in {{get/setIdToSign}} and {{get/setIdToReference}}, but not on {{XMLSecurityProperties}}: there is only a single {{XMLSecurityProperties.get/setIdAttributeNS}}.
> I wonder what is the vision is here? Unify encrypt/sign into a single set of "secure" methods that apply to both, or split encrypt and sign APIs completely? If you ask me, unifying makes most sense, but that's not what most of the code looks like...



--
This message was sent by Atlassian Jira
(v8.3.4#803005)