Add options to exclude the C14N Transform element in signatures #274
Conversation
This patch adds a flag 'exclude_c14n_transform_element' (boolean, defaults to False) to XMLSigner.sign(). If set to True, the transform is still applied, but it is not added to the Transforms element in ds:SignedInfo/ds:Reference. Conversely, this patch adds an option to XMLVerifier.verify() to specify a default canonicalization algorithm in the case where SignedInfo does not include the c14n Transform information. Rationale: In most cases, the Transforms element should indeed contain every modification made to the source data before creating or verifying the signature, and indeed, the necessity to add a default algorithm in verify() shows why. However, there are specifications that make use of XML Signatures, where the canonicalization algorithm is fixed, and which even go so far as to forbid naming the c14n algorithm in the Transforms element. This patch aims to make it possible to support those specifications.
|
And while I had searched the open issues I failed to notice there was a similar (old) PR to do this already... my apologies. My problem isn't with any implementation, though, but with a specification, namely this one: https://docs.peppol.eu/edelivery/smp/Peppol-EDN-Service-Metadata-Publishing-1.4.0-2025-02-06.pdf Which states (section 5.5.1):
And yes, I think the first one is a rather bad requirement, but as of right now, it is what is is, unfortunately. Perhaps subclassing is indeed the way to go, though i'm generally weary of overriding _methods()... |
|
@tjeb thanks for your contribution. This implementation is much better than #104 for many reasons. You produced the references I asked for, you limited your changes to the specific feature needed, you incorporated your changes into both the signer and the verifier, and you made new standalone test cases for both values of the new setting. I'm happy to merge this, but I'd like to ask for a couple small tweaks:
|
Rather than a named argument, the option which default c14n method should be used for references can now be set as a SignatureConfiguration value Additional changes: - ruff formatted the changes from the previous commit - amended the test to also test for a scenario where verification would fail
|
Addressed the first 2 points; for the second, I have lifted the existing hard-coded I did change the default to CANONICAL_XML_1_1 (it was 1_0), because this PR surfaced a discrepancy between the defaults of the signer and that of the verifier. If it is important that this remains at 1_0 I have no problem changing that back. I also amended the test a bit, to actually trigger a failed verification because of c14n. The only issue I have is with the third point: I understand asserting this would be good, but I'm not sure how, and more importantly, what to do when that fails. BTW, in the meantime I also had a discussion with one of the authors of the specification I'm implementing, and I may have convinced them that the spec is wrong. So at some point in the future there is a chance that my personal use-case for this PR will disappear. Though given that there was a similar PR there may be other use-cases. |
|
This is great, thanks! Looking at the code, my point 3 was a bit incoherent, as there already is a default, so you're right that there is no guidance as to what to do when the c14n transform node is missing. Merging. |
…Security#274) This patch adds a flag 'exclude_c14n_transform_element' (boolean, defaults to False) to XMLSigner.sign(). If set to True, the transform is still applied, but it is not added to the Transforms element in ds:SignedInfo/ds:Reference. Conversely, this patch adds an option to XMLVerifier.verify() to specify a default canonicalization algorithm in the case where SignedInfo does not include the c14n Transform information. Rationale: In most cases, the Transforms element should indeed contain every modification made to the source data before creating or verifying the signature, and indeed, the necessity to add a default algorithm in verify() shows why. However, there are specifications that make use of XML Signatures, where the canonicalization algorithm is fixed, and which even go so far as to forbid naming the c14n algorithm in the Transforms element. This patch aims to make it possible to support those specifications.
2 participants
This patch adds a flag 'exclude_c14n_transform_element' (boolean, defaults to False) to XMLSigner.sign().
If set to True, the transform is still applied, but it is not added to the Transforms element in ds:SignedInfo/ds:Reference.
Conversely, this patch adds an option to XMLVerifier.verify() to specify a default canonicalization algorithm in the case where SignedInfo does not include the c14n Transform information.
Rationale:
In most cases, the Transforms element should indeed contain every modification made to the source data before creating or verifying the signature, and indeed, the necessity to add a default algorithm in verify() shows why. However, there are specifications that make use of XML Signatures, where the canonicalization algorithm is fixed, and which even go so far as to forbid naming the c14n algorithm in the Transforms element.
This patch aims to make it possible to support those specifications.