WIP: Add boolean Certificate.is_issued_by(issuer)

[KonstantinShemyak]

Two booleans leaf_cert.is_issued_by(ca_cert) and ca_cert.is_issuer_of(leaf_cert), mirroring each other, are implemented.

The functions return True if the "issuer" field of leaf_cert is the same as the "subject" of ca_cert, and the signature on leaf_cert validates with the public key of ca_cert. If any of these conditions fails, appropriate exception is raised.

Corresponding test vectors are added.

This function will be used in #5093.

[markokr]

What is the point of this try-except?

[KonstantinShemyak]

Good catch! This slipped through the self-review. I'm removing the redundant code and force-pushing the PR branch (for reference, leaving the original PR in branch is_issued_by-v1 ).

[markokr]

Thanks. One more issue - the method name is_issued_by() seem not match the implementation. From name I would expect boolean function instead, without verify:

def is_issued_by(self, issuer_candiate):
    return issuer_candidate.subject == self.issuer

Especially as the == operation between names has tricky specification. Having it as separate method whose responsibility is following spec for that step seems good idea.

Thus I would prefer verify() as separate method. The trouble with that is that in current API the verify is used from public key side, not signature side, which this API should follow. So perhaps rename is_issued_by(issuer_candidate) to is_issuer(subject_certificate) to match it?

I see from other branch that you have Certificate.verify() method there, which does full-cert-store validation. That seems to be mistake - initial cert-store implementation can be simple but the API should support complex cert-store backends with complex policies, which means its better to implement it with separate class with overrideable methods for user.

So my suggestion: lets have Certificate class only have low-level is_issuer() and verify() methods, and have all cert-store logic somewhere else, eg. CertStore.verify(subject_certificate).

Alternative: lets not add anything to Cerfificate class, which stays as plain container with no logic and put all implementation details into CertStore class. The question behind this is - can ca_cert.verify(subject_cert) even viewed as low-level operation? Or should it follow all high level policies, which user may even want to override? If it's a complex operation, it does not belong to low-level class.

[KonstantinShemyak]

One more issue - the method name is_issued_by() seem not match the implementation. From name I would expect boolean function instead, without verify:

def is_issued_by(self, issuer_candiate):
    return issuer_candidate.subject == self.issuer

Hm, but a certificate issued by a CA must have a valid signature. This is different from any other checks which are subjects to policy (such as presence and value of any X.509v3 extensions or particular algorithms used). I'd say that if we check only for issuer_candidate.subject == self.issuer, then anyone would be able to generate certificates for which is_issued_by(any_CA) is True - which is not what users of the library would expect. So I think that the name in fact matches the implementation.

The trouble with that is that in current API the verify is used from public key side, not signature side, which this API should follow. So perhaps rename is_issued_by(issuer_candidate) to is_issuer(subject_certificate) to match it?

Thanks for the pointer! Will do this change.

[KonstantinShemyak]

I see from other branch that you have Certificate.verify() method there, which does full-cert-store validation. That seems to be mistake - initial cert-store implementation can be simple but the API should support complex cert-store backends with complex policies

Let's discuss this question in its own PR 🙂

[KonstantinShemyak]

Branch force-pushed with suggestion by @markokr : along with the initially implemented leaf_cert.is_issued_by(ca_cert), the mirroring ca_cert.is_issuer_of(leaf_cert) is added.

Previous branch point saved as is_issued_by-v2 for reference.

[KonstantinShemyak]

An "unintended" force-push: forgot to add the corresponding abstract method of the Certificate class.

[KonstantinShemyak]

Closing was a mistake, re-opening

[KonstantinShemyak]

Now we have two PRs adding the same functionality: this one and #5367 .

The latter adds one more OpenSSL hook, this PR is Python-only.

#5367 follows behavior of X509_check_issued(), which uses own considerations, which currently are:

1. Check issuer_name(subject) == subject_name(issuer)
2. If akid(subject) exists, check that it matches issuer
3. Check that issuer public key algorithm matches subject signature algorithm
4. If key_usage(issuer) exists, check that it supports certificate signing

In this PR, we:

• check (1) and (3) (the latter must hold true for the signature to verify)
• do not check (2) and (4)
• require the signature to verify.

I had explained earlier that I think that signature verification is what this method is about.

My take is that checks for key_usage and authorityKeyIdentifier belong to policy. Note that OpenSSL by some reason does not check the CA bit 🤷‍♂️ One can think of cases when these bits are contradicting, or when the verification would pass because some of the flags do not exist while the verifier would rely on their alignment. I'm not impressed with logic "X must be equal to Y, but if X is not present, then no problem".

If deemed reasonable, these checks may be added under parameters with reasonable defaults, such as for example cert.is_issued_by(ca_cert, require_ku_bit=True, require_ca_bit=True, require_akid=True).

[KonstantinShemyak]

Here is a screenshot of the rendered updates to the documentation.

[tiran]

I don't think that the check is sufficient. Other PKIX are typically more rigorous and check KU, SKID, and AKID. NSS seems to be more forgiving than OpenSSL. Ryan mentioned recently that macOS is even more strict and Windows even prioritizes AKID/SKID matching over issuer/subject matching. It's unfortunate that even something 'trivial' like "is cert issued by another cert" is full of landmines...

IMHO you should also check for presence of AKID, SKID, and KU, then verify properties like akid.keyid == issuer.skid, akid.serial == issuer.serial, akid.dirname == issuer.issuer, issuer.ku has cert signing flag, and possibly some proxy cert shenanigans. An AKID/SKID mismatch automatically implies cert mismatch.

If I were you I would just use X509_check_issued() followed by X509_verify(). This gives you OpenSSL semantics, which is what most people expect anyway.

[KonstantinShemyak]

@tiran I have given in the comment above reasoning why I think that the mentioned checks are in fact policy checks, and there is more than one way to accept them. Say, CA certificate has no AKID, but the issued one has SKID. Is this OK?

If I were you I would just use X509_check_issued() followed by X509_verify(). This gives you OpenSSL semantics, which is what most people expect anyway.

This would be definitely the way to go if we were building (yet another) OpenSSL wrapper. But if the project's goal is to be your "cryptographic standard library" (and it allows, at least in theory, for other cryptographic backends), then IMO providing the "correct" APIs is a higher priority than making them mimicking OpenSSL. Of course it's up to project maintainers to decide on what is "correct".

[tiran]

The x509 layer is outside the hazmat area. Any API outside the hazmat layer should be safe, sane, and free of pitfalls. Your implementation of is_issued_by() would be a pitfall for me -- and I know a lot about X.509 and PKIX. Even my proposal X509_check_issued() + X509_verify() has pitfalls because it doesn't validate EKU and BC extensions.

IMHO it would be better to add low-level functions for X.509 trust chain processing to the hazmat layer.

[reaperhulk]

If cryptography were to gain a full validator, what do you envision the use case for these functions to be? I am concerned that they (like most things related to X.509 validation) contain a great deal of nuance and that users, who might naturally assume they use the same logic that an X.509 validator uses to do path traversal, will badly shoot themselves.

[schwabe]

I was in the same situation, want to have a "simple" way of checking, "does this certificate" belong to a CA. But X509 validity depends on so many things, like:

• part of TLS connection? Might need correct attributes
• do the usages of the CA allow signing
• are we checking all extensions? (esp. issued by etc)

So even for my simple use case that I was chasing of only wanting a negative "this cert looks wrong, please provide a valid cert for config", you can end getting it wrong very easily. Even a simple assumption like "if signature matches but signer id is different" fails with things like ":CA1(key1, identifier1) => CA2(key2, identifier2) => CA3(key1,identifier3) => cert(sign by CA identifier3)

Why am I showing the cert chain example? Because issued_by is likely used in check validity of a certificate and that is very easy to get wrong because it is even more then only the 'can of worms' issued_by.

So getting somthing like this really right, is very hard.

[KonstantinShemyak]

If cryptography were to gain a full validator, what do you envision the use case for these functions to be? I am concerned that they (like most things related to X.509 validation) contain a great deal of nuance and that users, who might naturally assume they use the same logic that an X.509 validator uses to do path traversal, will badly shoot themselves.

@reaperhulk I agree with your concern. To draw a clearer boundary between policies and technology, I've split pure cryptographic operations into #5950 . Next, we complete this PR with some default policy checks and possibility to add custom ones. Then it would be used for the long-awaited certificate verification in #5093.

[KonstantinShemyak]

I was in the same situation, want to have a "simple" way of checking, "does this certificate" belong to a CA. But X509 validity depends on so many things, like:

* part of TLS connection? Might need correct attributes
* do the usages of the CA allow signing
* are we checking all extensions? (esp. issued by etc)

@schwabe I fully agree that the user of the library typically wants just one "Yes/No" answer to their question "Is this a good certificate of my example.com?". The plan to handle your examples in such interface is:

• Are we establishing a TLS connection? - only caller of the method can know this. There is no way to be aware of the purpose of the call inside the method. So, the user would have to supply this data.
• Usages of the CA, presence/value of extensions? - this is a policy question. The plan is to supply a reasonable default policy and arguments to provide custom policy when needed.

I do not see an approach which would be simpler than this. If there is such, I'd be happy to know!