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

Support for X509IssuerSerial #152

Open
RJPercival opened this issue Jun 3, 2020 · 14 comments
Open

Support for X509IssuerSerial #152

RJPercival opened this issue Jun 3, 2020 · 14 comments

Comments

@RJPercival
Copy link
Contributor

RJPercival commented Jun 3, 2020

It would be useful if signxml could output X509IssuerSerial as well as X509Certificate, for interoperating with systems that only support the former. Similarly, it would be helpful if it could select the correct certificate to use based on the values in this element.

I am aware that sign() has a key_info parameter that makes it possible to provide a KeyInfo element containing both, but it'd be great if this was handled by the library. Otherwise, the caller has to parse the certificate and construct the correct XML themselves.

@RJPercival
Copy link
Contributor Author

Partial duplicate of #39.

@kislyuk
Copy link
Member

kislyuk commented Jun 10, 2020

The spec notes that X509IssuerSerial is deprecated. I couldn't find the reasoning for why, but one issue that I see is that the value is not unique across CAs, so to successfully pin a certificate you'd have to indicate the CA name as well.

This doesn't seem to be exploitable, because the verifier will be pinning the certificate by other means anyway. So in principle we could support this. In light of this being deprecated, can you note the application where you expect to need this, for our reference?

@RJPercival
Copy link
Contributor Author

https://www.w3.org/TR/xmldsig-core1/#sec-X509Data:

The X509IssuerSerial element has been deprecated in favor of the newly-introduced dsig11:X509Digest element. The XML Schema type of the serial number was defined to be an integer, and XML Schema validators may not support integer types with decimal data exceeding 18 decimal digits [XMLSCHEMA-2]. This has proven insufficient, because many Certificate Authorities issue certificates with large, random serial numbers that exceed this limit. As a result, deployments that do make use of this element should take care if schema validation is involved. New deployments should avoid use of the element.

The serial number is unique per issuer, and X509IssuerSerial includes both the issuer DN and the serial number, so that's not a problem.

I'm integrating with a large third-party service that only sends X509IssuerSerial in their XML. I can pass their current certificate into verify(), but this will only work until around the time it's due to expire. At that point, my code will need to be able to seamlessly transition to using the new certificate as soon as they change the issuer/serial number in their XML. Without support for this in signxml, I'll need to parse the XML prior to verification, inspect X509IssuerSerial to find the right certificate, and then pass that into verify().

@kislyuk
Copy link
Member

kislyuk commented Jun 21, 2020

Released a fix in v2.8.0, please test:

def cert_resolver(x509_issuer_name, x509_serial_number, x509_digest):
    with open("cert.pem") as fh:
        return [fh.read()]

signxml.XMLVerifier().verify(signed_data, cert_resolver=cert_resolver)

@kislyuk
Copy link
Member

kislyuk commented Jun 28, 2020

@RJPercival when you have a moment, can you confirm that the new API works for you?

@RJPercival
Copy link
Contributor Author

I've given it a try but ran into this bug in OpenSSL when using self-signed certificates: openssl/openssl#1418.

lib/python3.7/site-packages/signxml/util/__init__.py:216: in _add_cert_to_store
    X509StoreContext(store, cert).verify_certificate()
lib/python3.7/site-packages/OpenSSL/crypto.py:1766: in verify_certificate
    raise self._exception_from_context()
E   OpenSSL.crypto.X509StoreContextError: [20, 0, 'unable to get local issuer certificate']

I guess #141 might get around that, otherwise I'll have to ask the third party I'm integrating with to re-issue their certificate with different keyUsage.

@RJPercival
Copy link
Contributor Author

A nit about the API was that I can pass a crypto.X509 object into verify(), but I can't return one from cert_resolver. This is easy to workaround, since I can just serialize the certificate before returning it, but it's possibly less efficient.

@RJPercival
Copy link
Contributor Author

I've just run into issue #81. While I can use validate_schema=False to workaround this, do you know how much risk, if any, this carries @kislyuk?

@kislyuk
Copy link
Member

kislyuk commented Aug 11, 2020

Not validating against the schema does not carry any security risks that I am aware of, as long as you use only what is signed. It is primarily an interoperability tool.

You could also use a fork of signxml and change the schema file to remove the assertion on the integer size. (Perhaps the solution here is to just use that forked schema in signxml by default.)

@RJPercival
Copy link
Contributor Author

Have you considered a different schema validator? It seems the reason that lxml rejects serial numbers longer than 18 digits is because it's a "minimally-conforming" schema validator, and so isn't required to support more digits than that. However, there are other schema validators, e.g. xmlschema, that do support the extra digits. I assume you're using lxml because it avoids introducing another dependency?

@kislyuk
Copy link
Member

kislyuk commented Aug 12, 2020

Thanks for pointing that out. That's correct, lxml provides a whole bunch of things that signxml uses, including schema validation, so I use it to minimize the number of dependencies.

I'll take a look at xmlschema, thanks. I might also look for a workaround in the lxml configuration or permanently bundle an edited version of the schema.

@kislyuk kislyuk reopened this Aug 12, 2020
@RJPercival
Copy link
Contributor Author

RJPercival commented Jan 21, 2021

@kislyuk I've just discovered that cert_resolver is given the X509Digest content, but not the Algorithm attribute that goes with it. Without knowing which digest algorithm was used, it's rather expensive to generate the digests of local certificates to match against it.

@kislyuk
Copy link
Member

kislyuk commented Jan 21, 2021

This library is expected to be used in an application where there's no ambiguity about the algorithm, just a limited set of rotating or user-specific certs which can unambiguously be identified by their digest. Can you give an example of the situation in which this extra data is necessary?

@RJPercival
Copy link
Contributor Author

RJPercival commented Jan 25, 2021

There are 5 digest algorithms allowed by the XMLDSig standard. The signer can use any of these, but the verifier would have to try all of them with every known certificate in order to find a match without knowing which algorithm was used.

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

No branches or pull requests

2 participants