Skip to content
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

verify: Support XAdES encapsulated certificates #182

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

peterkelly
Copy link

This change modifies the logic used to find X509 certificates within a Signature element to include EncapsulatedX509Certificate elements.

This makes it possible to use the XMLVerifier with documentsignatures.xml files generated by LibreOffice, which places all certificates in the chain of trust from the signer to the root in EncapsulatedX509Certificate elements.

Related: #139

@kislyuk
Copy link
Member

kislyuk commented Mar 30, 2022

Thank you for your contribution. To proceed with merging, we need unit tests to cover all new code paths introduced by this PR. Could you please add unit tests? Let me know if you run into any difficulties running the test suite.

The XAdES-X-L form includes the full certificate path up to the root
certificate, so that the verifier can accept any signature from a
certificate that has some path to a trusted root. This is described at:

    https://www.w3.org/TR/XAdES/#Syntax_forXAdES-X-L_form

This commit adds three certificates (and a script to re-generate them
if necessary): root, intermediate, and leaf. In the XAdES-X-L test case,
we sign with the leaf certificate but include all three in the
certificate chain. Verifying these works when the root certificate is
given as a trusted root, but fails when it is not considered a trusted
root.
@peterkelly
Copy link
Author

I've added a unit test now (see description in latest commit). Please let me know if this is sufficient.

@kislyuk
Copy link
Member

kislyuk commented Jun 28, 2022

Thanks again for adding a test, and apologies for the delay in getting your contribution reviewed.

I'm OK with merging partial/incremental support for XAdES, especially if it enables specific use cases that people have (like the LibreOffice one you point out). However from a security risk management standpoint, I'd like to sequester XAdES-specific functionality into subclasses that have to be specifically imported and used instead of being on by default. I want to do this because we don't have a full XAdES conformance test suite in place, and I don't have a good enough security analysis of XAdES in hand to be sure that this is safe from new types of substitution/wrapping attacks, etc. for general purpose XMLDSIG verification.

If you can separate this functionality into XAdES specific classes, I'm happy to merge this now, otherwise I'll use your branch as a starting point for a more comprehensive XAdES implementation using e.g. https://github.com/esig/dss/tree/master/dss-xades/src/test/resources/validation and https://signatures-conformance-checker.etsi.org/pub/documents/XAdESConformanceTestChecker_v1.0.pdf as starting points.

@peterkelly
Copy link
Author

peterkelly commented Jul 17, 2022

I think the latter is probably better.

One simple thing I could do if you wanted is to create a subclass of XMLVerifier, say XAdESVerifier, which overrides the _get_certificates added in 908029b. The base XMLVerifier class could provide a version of this method that just returns the certificates that it did before, while XAdESVerifier could override the method to supply the extra certificates.

An alternative approach would be to allow an optional certificate_extractor parameter, with a base class of CertificateExtractor (used by default) and a subclass XAdESCertificateExtractor that can be specified by clients if they need this functionality. I don't know enough about XAdES to say whether this or the former is the better approach.

I want to avoid doing a full implementation of XAdES since it's far beyond the scope of what I need (and couldn't justify the time for), so something like this would be good. I suggest you go ahead using my branch as a starting point, perhaps using the ideas above if appropriate.

@kislyuk kislyuk force-pushed the develop branch 4 times, most recently from 7a5c538 to 6baf513 Compare August 20, 2022 06:52
@kislyuk kislyuk force-pushed the develop branch 6 times, most recently from 6b21a86 to 6879c98 Compare November 14, 2022 00:32
@kislyuk kislyuk force-pushed the develop branch 2 times, most recently from 9717621 to 82ae152 Compare November 27, 2022 22:11
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.

None yet

2 participants