Skip to content

XAdESVerifier verify CertDigest fix #247

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

msetina
Copy link
Contributor

@msetina msetina commented Mar 18, 2024

Fix for #246. I found out that if I have multiple certificates in cert parameter to sign, the digest check would fail. Since it was signed with signxml, it was strange. Looking at code I found a problem. This is the solution.

@msetina
Copy link
Contributor Author

msetina commented Mar 18, 2024

Test suite fails for nonconformant-X_BE_CONN_10.xml. It has 2 X509Certificate nodes and 2 xades:Cert nodes. Previous code raised a InvalidDigest. The new code checks both Digests as OK as it compares by list index.
By removing "nonconformant-X_BE_CONN_10": InvalidDigest, from test_xades_interop_examples error_conditions, test suit will finish with OK.

@kislyuk
Copy link
Member

kislyuk commented Mar 23, 2024

Please add a PR description with the motivation for this change.

@msetina
Copy link
Contributor Author

msetina commented Mar 23, 2024

Notice that test is changed as one was an error before and now it is not as Digests match.

@kislyuk
Copy link
Member

kislyuk commented Mar 24, 2024

Thanks. I will take a look at that test case and see if this is the correct way to make it work.

@kislyuk kislyuk force-pushed the develop branch 4 times, most recently from 7e7f504 to b3de531 Compare September 20, 2024 16:42
@msetina
Copy link
Contributor Author

msetina commented Jun 6, 2025

Is there a problem with this PR?

@kislyuk
Copy link
Member

kislyuk commented Jun 7, 2025

The problem is that I haven't been able to understand whether this change is the general and safe thing to do.

A deeper analysis including references to relevant parts of the standard would help.

@msetina
Copy link
Contributor Author

msetina commented Aug 3, 2025

You are right to be sceptical. I still think the base implementation is wrong as my case was made by this project and it does not verify.
All I did was change the code to the point where my case cleared.
The test case may have tested for certificate replacement. And this PR does not check that. But the test case was cleared for wrong reason. Current code verifies by first fail which does not work with multiple certificates.
The spec expects to link certicates and digest by issuerserial.

@kislyuk
Copy link
Member

kislyuk commented Aug 7, 2025

Let's keep this open if you don't mind, since as you say it makes one of the test cases pass. I promise I will get to it eventually :)

@kislyuk kislyuk mentioned this pull request Aug 7, 2025
@msetina
Copy link
Contributor Author

msetina commented Aug 8, 2025

I changed the code to be more inline with specification.
The specification ETSI EN 319 132-1 V1.2.1 states

The information in the IssuerSerialV2 element is only a hint, that can help to identify the certificate
whose digest matches the value present in the reference. But the binding information is the digest of the
certificate.

So I changed the code to reflect that. So the available certificates get keys formed in accordace to IssuerSerial and IssuerSerialV2 for the newer definition. Then the Digests are first matched by this key. If not then it follows the notion to find the certificate by matching digest to all available certificates to conform to the statement. This way digest fails only if no Certificates match the digest. Test look OK, but in test cases are examples that do not match by IssueSerial because of encoding issues. Those do not break the digest as digest matches one of the Certificates.

@msetina
Copy link
Contributor Author

msetina commented Aug 8, 2025

To parse the IssueSerialV2 I used asn1crypto. I saw it is used in tests, but it is not dependency. The data in IssueSerialV2 is a complex type and would be hesitant to do it on my own. Hope this is not a problem.

@msetina msetina reopened this Aug 8, 2025
@msetina msetina changed the title XAdESVerifier verify CertDigest by index XAdESVerifier verify CertDigest fix Aug 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants