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

Add certificate signature verification #2460

Closed
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
@PeterHamilton
Contributor

PeterHamilton commented Oct 30, 2015

This change adds a CertificateValidator to the x509 API. The validator adds methods to validate a pair of certificates and an entire chain of certificates, given a set of certificate trust anchors is provided. A test suite for the CertificateValidator is included.

@PeterHamilton

This comment has been minimized.

Contributor

PeterHamilton commented Oct 30, 2015

So this is definitely still WIP and it shouldn't pass the tests. I'm primarily interested in feedback on the CertificateValidator right now. I based this off of #2387 and also leverage a non-existent cert.tbs_certificate property to finish off the signature verification routine.

I also still have some tests to finish up, but I wanted to push something out before the weekend.

Open questions: does cert.tbs_certificate seem preferable to manually chopping up the certificate bytes? Does it make more sense to just update the backend API to get this (which I believe will require another pull request)?

@alex

This comment has been minimized.

Member

alex commented Oct 30, 2015

Jenkins, ok to test.

@alex

This comment has been minimized.

Member

alex commented Nov 1, 2015

before we go too much farther, we need to think about the full set of options and what not we might care about.

@reaperhulk

This comment has been minimized.

Member

reaperhulk commented Nov 1, 2015

Agreed. For example, this current code appears to assume validate_certificate_chain will present certs in a prebuilt chain order? I believe we discussed that briefly previously, but if that's the case we need something that can, given a bundle of certs, find a valid chain. A validator is of extremely limited utility if it can't do this resolution.

I'd also like the API to have affordances for passing CRL/OCSP as part of the validation process. To avoid making the PR enormous it doesn't have to be implemented now, but nailing down a proposed interface is important.

@PeterHamilton

This comment has been minimized.

Contributor

PeterHamilton commented Nov 1, 2015

@reaperhulk Shuffling a set of certificates to form a valid chain is definitely reasonable. I'll add that in. I'm going to work under the assumption that the certificate set provided to the validator should form one valid chain, not multiple chains. There shouldn't be any extraneous certificates either; all should be a part of the chain.

For CRL/OSCP, I'll start thinking about how that'll look. I agree that that should wait for a second round of patches.

@rowland-smith

This comment has been minimized.

rowland-smith commented Nov 4, 2015

I understand why you'd rather not expose any properties you don't have to. However, I've worked with x509 certificate code at different times over the past 20 years, and I was expecting to find something like signature and tbs_certificate_bytes already there.

In the end, I would be fine with something like a Certificate.verifySignature( self, issuerCertificate ) method as long as it did all the right things, looked up the sig algorithm, pulled out the parameters, used the right VerificationContext for the algorithm, etc. I would expect verifySignature to also tell me, if it failed, why it failed. Were the parameters incompatible with the signatureAlgorithm specified in the Certificate? Was the signatureAlgorithm not supported by the backend? I'm guessing these would be exceptions raised by the underlying verification context anyway.

Also, I'm writing a certificate path validation library of my own, and I really want to get on with it, so if the two properties work as advertised I'm all for a merge.

Just my $.02 :)

@rowland-smith

This comment has been minimized.

rowland-smith commented Nov 4, 2015

I don't need this functionality at this time, but I believe that if someone wants to write generalized signature verification code that they will also need access to the signatureAlgorithm (ASN.1 type AlgorithmIdentifier) from the outer certificate or the inner tbsCertificate in order to extract the OID of the signature algorithm + hash algorithm (for example sha256WithRSAEncryption), and the algorithm parameters used when the public key was signed.

Certificate ::= SEQUENCE {
tbsCertificate TBSCertificate,
signatureAlgorithm AlgorithmIdentifier,
signatureValue BIT STRING }

TBSCertificate ::= SEQUENCE {
version [0] EXPLICIT Version DEFAULT v1,
serialNumber CertificateSerialNumber,
signature AlgorithmIdentifier,
issuer Name,
validity Validity,
subject Name,
subjectPublicKeyInfo SubjectPublicKeyInfo,
issuerUniqueID [1] IMPLICIT UniqueIdentifier OPTIONAL,
-- If present, version MUST be v2 or v3

AlgorithmIdentifier ::= SEQUENCE {
algorithm OBJECT IDENTIFIER,
parameters ANY DEFINED BY algorithm OPTIONAL }

@PeterHamilton

This comment has been minimized.

Contributor

PeterHamilton commented Nov 4, 2015

@rowland-smith This information should be available from the Certificate API already via signature_hash_algorithm property.

@rowland-smith

This comment has been minimized.

rowland-smith commented Nov 4, 2015

After looking at the code, as far as I can tell signature_hash_algorithm only returns a cryptography.hazmat.primitives.hashes.Hash object, and that interface doesn't expose either the signature-algorithm itself (like sha254WithRSAEncryption, etc.) nor the parameters, if there were any, used in creating the signature.

I was able to access the AlgorithmIdentifier.oid and parameters in SubjectPublicKeyInfo using pyasn1:

# Using cryptography...
spkiEncoding = subjectCertificate.public_key().public_bytes( Encoding.DER, 
     PublicFormat.SubjectPublicKeyInfo )
# Using pyasn1...
subjectPublicKeyInfo = decoder.decode( spkiEncoding, SubjectPublicKeyInfo() )
@reaperhulk

This comment has been minimized.

Member

reaperhulk commented Nov 5, 2015

The public key of the certificate provides the type and the signature hash algorithm provides the hash. For RSA all certificates are signed using PKCS1v1.5 padding and for DSA/ECDSA no padding is required. You can see examples of certificate signature checks (on self-signed certificates) by looking at the test_tbs_certificate_bytes methods in test_x509.py

Does this miss anything? If so we definitely need to resolve that :)

@PeterHamilton

This comment has been minimized.

Contributor

PeterHamilton commented Nov 5, 2015

@reaperhulk Yep, that's how I'm doing it here. I just forgot to bring up the key verifier piece when @rowland-smith asked his question.

@rowland-smith

This comment has been minimized.

rowland-smith commented Nov 5, 2015

Agreed, you can determine the signature algorithm type (RSA/DSA/ECDSA) from the python class of the key. I was thinking about situations where someone wants to be very strict in their validation and must check the OID from the outer Certificate.signatureAlgorithm.type or the TBSCertificate.signature.type. That’s why Certificate.signatureAlgorithm is in the outer body.

Also, if actual signature algorithm parameters (Certificate.signatureAlgorithm.parameters | TBSCertificate.signature.parameters) are present they have to currently be extracted from the SubjectPublicKeyInfo (TBSCertificate.signature.parameters) like so:

First use ‘cryptography’...

spkiEncoding = subjectCertificate.public_key().public_bytes( Encoding.DER, PublicFormat.SubjectPublicKeyInfo )

Then use ‘pyasn1’...

subjectPublicKeyInfo = decoder.decode( spkiEncoding, SubjectPublicKeyInfo() )

I think in PKIX parameters are very rare, usually absent or ASN.1 NULL, but I think there is at least one valid signature algorithm that uses parameters (RSA PSS)(?).

Certificate ::= SEQUENCE {
tbsCertificate TBSCertificate,
signatureAlgorithm AlgorithmIdentifier,
signatureValue BIT STRING }

TBSCertificate ::= SEQUENCE {
version [0] EXPLICIT Version DEFAULT v1,
serialNumber CertificateSerialNumber,
signature AlgorithmIdentifier,
issuer Name,
validity Validity,
subject Name,
subjectPublicKeyInfo SubjectPublicKeyInfo,
issuerUniqueID [1] IMPLICIT UniqueIdentifier OPTIONAL,
-- If present, version MUST be v2 or v3

On Nov 4, 2015, at 7:26 PM, Paul Kehrer notifications@github.com wrote:

The public key of the certificate provides the type and the signature hash algorithm provides the hash. For RSA all certificates are signed using PKCS1v1.5 padding and for DSA/ECDSA no padding is required. You can see examples of certificate signature checks (on self-signed certificates) by looking at the test_tbs_certificate_bytes methods in test_x509.py

Does this miss anything? If so we definitely need to resolve that :)


Reply to this email directly or view it on GitHub #2460 (comment).

@PeterHamilton PeterHamilton force-pushed the PeterHamilton:add-certificate-validation branch from ed3ef9f to 560537e Nov 5, 2015

@PeterHamilton

This comment has been minimized.

Contributor

PeterHamilton commented Nov 5, 2015

Ok, I'm still working on a bunch of tests but the CertificateValidator is getting close to a good state I think. In addition to name and signature checks, it examines the basicConstraints and keyUsage extensions for whether or not intermediary certs are capable of signing certificates. It also checks the pathLength to see if the certificate in question can be used to create a chain of sufficient length. Note that the current validation does not check certificate policy extension information for conformance.

I added build_certificate_chain, which will iteratively build out a chain from an arbitrary set of certificates. It breaks certificate name collisions by actually verifying the signatures between certificate pairs as needed. It also will only build one chain for a given certificate set, and the chain must include all certificates in the set. Multiple or branching chains are not currently supported.

@reaperhulk For CRL/OCSP handling, the validate_certificate_chain method has two flags, check_crls and check_ocsp, that when set would in theory trigger the retrieval and checking of CRL/OCSP constraints. For now, if activated they'll throw NotImplementedError. Please chime in if you think there's a better way to handle these. I toyed with adding them as initial arguments to the validator, but I didn't want staleness to be a factor.

Documentation will need to be added for all of this as well. It's on my TODO list.

Question: Is there a hash algorithm available that will support elliptic curves? ECDSA is the only signature algorithm supported for OpenSSL but I'm not entirely sure how to use it in the context of signature verification. The OpenSSL backend routine for building x509 certificates requires a hashes.HashAlgorithm.

@rowland-smith

This comment has been minimized.

rowland-smith commented Nov 5, 2015

I’m pretty sure I had ECDSA/sha256 working last week. And I believe that it is commonly used, but I would have to find some references before I could say that was definitive.

On Nov 5, 2015, at 5:04 PM, Peter Hamilton notifications@github.com wrote:

Ok, I'm still working on a bunch of tests but the CertificateValidator is getting close to a good state I think. In addition to name and signature checks, it examines the basicConstraints and keyUsage extensions for whether or not intermediary certs are capable of signing certificates. It also checks the pathLength to see if the certificate in question can be used to create a chain of sufficient length. Note that the current validation does not check certificate policy extension information for conformance.

I added build_certificate_chain, which will iteratively build out a chain from an arbitrary set of certificates. It breaks certificate name collisions by actually verifying the signatures between certificate pairs as needed.

@reaperhulk https://github.com/reaperhulk For CRL/OCSP handling, the validate_certificate_chain method has two flags, check_crls and check_ocsp, that when set would in theory trigger the retrieval and checking of CRL/OCSP constraints. For now, if activated they'll throw NotImplementedError.

Documentation will need to be added for all of this as well. It's on my TODO list.

Question: is there a hash algorithm available that will support elliptic curves? ECDSA is the only signature algorithm supported for OpenSSL but I'm not entirely sure how to use it in the context of signature verification. The OpenSSL backend routine for building x509 certificates requires a hashes.HashAlgorithm.


Reply to this email directly or view it on GitHub #2460 (comment).

@PeterHamilton

This comment has been minimized.

Contributor

PeterHamilton commented Nov 6, 2015

@rowland-smith I figure there's some way to do it. I tried just passing hashes.SHA256() to ec.ECDSA(...), since it accepts an algorithm argument, but that didn't work.

@rowland-smith

This comment has been minimized.

rowland-smith commented Nov 6, 2015

I’ll be glad to try it myself tomorrow. If you figure it out before then let me know. I’ll reply here once I do the test.

On Nov 5, 2015, at 8:38 PM, Peter Hamilton notifications@github.com wrote:

@rowland-smith https://github.com/rowland-smith I figure there's some way to do it. I tried just passing hashes.SHA256() to ec.ECDSA(...), since it accepts an algorithm argument, but that didn't work.


Reply to this email directly or view it on GitHub #2460 (comment).

@reaperhulk

This comment has been minimized.

Member

reaperhulk commented Nov 6, 2015

The test code previously referenced shows how to do a signature verification on a self-signed ECDSA certificate.

@PeterHamilton

This comment has been minimized.

Contributor

PeterHamilton commented Nov 6, 2015

@reaperhulk Well I'm an idiot. Thanks for that!

@PeterHamilton PeterHamilton force-pushed the PeterHamilton:add-certificate-validation branch from 560537e to d7f5065 Nov 6, 2015

@codecov-io

This comment has been minimized.

codecov-io commented Nov 6, 2015

Current coverage is 100.00%

Merging #2460 into master will not affect coverage as of 6ca9dca

@@            master   #2460   diff @@
======================================
  Files          125     127     +2
  Stmts        13789   13936   +147
  Branches      1472    1488    +16
  Methods          0       0       
======================================
+ Hit          13789   13936   +147
  Partial          0       0       
  Missed           0       0       

Review entire Coverage Diff as of 6ca9dca

Powered by Codecov. Updated on successful CI builds.

@PeterHamilton PeterHamilton force-pushed the PeterHamilton:add-certificate-validation branch from d7f5065 to e4bc012 Nov 11, 2015

@PeterHamilton PeterHamilton changed the title from [WIP] Add certificate validation to Add certificate signature verification Nov 11, 2015

@PeterHamilton

This comment has been minimized.

Contributor

PeterHamilton commented Nov 11, 2015

Ok, I've redesigned the validation API and am splitting this patch in two.

This patch now just contains a CertificateVerificationContext which can be used, like the different AsymmetricVerificationContext objects, to verify that certificates were signed by the certificate associated with the context. You create the context with the signing certificate, use update to load in a new signed certificate to evaluate, and then call verify to verify that it was signed by the signing certificate. The context interface for asymmetric keys is a much cleaner way of representing this functionality, so I replicated it here. There's no abc base for it since there's only one real certificate type to speak of. I know that the current mindset is to keep certificates as just data containers but I could definitely see (down-the-road) adding a verifier method to the certificate API that would return the CertificateVerificationContext for a given certificate.

The next patch will contain all of the certificate chain code that was in the original patch, represented by a CertificateChainVerificationContext that will have the same style of API.

@PeterHamilton PeterHamilton force-pushed the PeterHamilton:add-certificate-validation branch from e4bc012 to 24a767c Nov 11, 2015

@PeterHamilton

This comment has been minimized.

Contributor

PeterHamilton commented Nov 11, 2015

Still working on docs. I pulled the CHANGELOG update to make sure everything else is working.

@alex @reaperhulk If you have a sec, could you look at the errors Jenkins is throwing? The CentOS test runs break when I check for OpenSSL elliptic curve support (when selectively skipping keys that different OpenSSL versions don't support). What other context/version-specific checks should I do to prevent these errors from triggering? Thanks in advance.

@reaperhulk

This comment has been minimized.

Member

reaperhulk commented Nov 13, 2015

You'll need to move the EC tests to a separate function and add the EllipticCurveBackend as a required interface. (The failure is occurring because some OpenSSLs don't have EC at all, so you can't check if it supports a specific curve because the functions to check are themselves missing).

@PeterHamilton PeterHamilton force-pushed the PeterHamilton:add-certificate-validation branch from 24a767c to 6821bb8 Nov 16, 2015

@PeterHamilton

This comment has been minimized.

Contributor

PeterHamilton commented Nov 16, 2015

@reaperhulk - thanks for the suggestion. I split them out, so we'll see if that fixes it.

@anton-ryzhov

This comment has been minimized.

anton-ryzhov commented Jan 26, 2016

Very useful PR, I hope it will be included in next release.

But I've faced with problem. I've tried to validate some certificates issued by built-in CAs (Shipped with certifi package). I've figured that 8 of 151 root CAs doesn't contain keyUsage extension at all — they are too old, created before rfc5280.
And one even doesn't have basicConstraints — created before rfc3280.

There are major CAs VeriSign, GeoTrust, Go Daddy among them — lots of chain checks fails because of this.

Maybe script should check not_valid_before or give some option to skip strict rfc5280 validation.

@reaperhulk reaperhulk removed this from the Thirteenth Release milestone Mar 6, 2016

@PeterHamilton

This comment has been minimized.

Contributor

PeterHamilton commented Mar 28, 2016

@reaperhulk I noticed 1.3 was released last week and I am curious why the milestone tag got removed from this. Is there anything you need from me to get this finally reviewed and merged in?

@reaperhulk

This comment has been minimized.

Member

reaperhulk commented Mar 30, 2016

@PeterHamilton it got removed from the milestone because we didn't have the bandwidth to get it landed in the time frame we wanted for 1.3. Adding to the fourteenth milestone now (sorry about that).

@reaperhulk reaperhulk added this to the Fourteenth Release milestone Mar 30, 2016

@PeterHamilton

This comment has been minimized.

Contributor

PeterHamilton commented Mar 30, 2016

@reaperhulk Not a problem. I just wanted to make sure I hadn't dropped the ball. It'd been a while since I'd checked on the patch. I'll make sure I'm available for questions or rewrites going forward.

What's your timeline for 1.4?

@chadwhitacre chadwhitacre referenced this pull request May 7, 2016

Merged

implement symmetric encryption #3998

8 of 8 tasks complete
@allardhoeve

This comment has been minimized.

allardhoeve commented Aug 10, 2016

Hey, can we merge this?

@reaperhulk

This comment has been minimized.

Member

reaperhulk commented Aug 26, 2016

The lack of action on this PR is embarrassing and I apologize. Unfortunately we're not going to review it for the 1.5 release either, but I will spend some time in the next few days thinking very hard about this one.

@allardhoeve

This comment has been minimized.

allardhoeve commented Aug 31, 2016

Any new insights, @reaperhulk? 😄

@PeterHamilton

This comment has been minimized.

Contributor

PeterHamilton commented Sep 12, 2016

Sorry @reaperhulk, I partially checked out on this one as well. If you still think the feature has a place in cryptography, sweet! We're (slowly) making progress on adding this capability into cursive to support our OpenStack work. Given cursive is OpenStack-specific, I'd still like to see a more generic solution for Python developers.

ExtensionOID.KEY_USAGE).value
if not basic_constraints.ca:
raise InvalidSigningCertificate(

This comment has been minimized.

@tiran

tiran Sep 13, 2016

Contributor

What happens if the certificate has no basic constraint or key usage extensions? These v3 extensions are critical but not mandatory extensions. A cert w/o a BC, KU or EKU can be used as CA cert.

This comment has been minimized.

@PeterHamilton

PeterHamilton Sep 13, 2016

Contributor

While not mandatory in the general case, these extensions are required for conforming CAs. The following quotes are taken from RFC 5280 on X.509 PKI:

https://www.ietf.org/rfc/rfc5280.txt

Regarding the Basic Constraints extension, see Section 4.2.1.9, bottom of page 38:

Conforming CAs MUST include this extension in all CA certificates that contain public keys used to validate digital signatures on certificates and MUST mark the extension as critical in such certificates. This extension MAY appear as a critical or non-critical extension in CA certificates that contain public keys used exclusively for purposes other than validating digital signatures on certificates. Such CA certificates include ones that contain public keys used exclusively for validating digital signatures on CRLs and ones that contain key management public keys used with certificate enrollment protocols. This extension MAY appear as a critical or non-critical extension in end entity certificates.

Regarding the Key Usage extension, see Section 4.2.1.3, top of page 29:

Conforming CAs MUST include this extension in certificates that contain public keys that are used to validate digital signatures on other public key certificates or CRLs. When present, conforming CAs SHOULD mark this extension as critical.

This comment has been minimized.

@anton-ryzhov

anton-ryzhov Oct 17, 2016

@PeterHamilton , see #2460 (comment)
RFC 5280 is to new for some root CA widely used nowadays. You can't rely on existing of this extensions.

return (issuing_certificate.subject == issued_certificate.issuer)
class CertificateVerificationContext(object):

This comment has been minimized.

@tiran

tiran Sep 13, 2016

Contributor

How does the verification context handle unknown critical v3 extensions? Does it raise an exception or does it ignore unknown critical extensions?

This comment has been minimized.

@PeterHamilton

PeterHamilton Sep 13, 2016

Contributor

Right now the context only examines the Basic Constraints and Key Usage extensions for fields required for CA use. Other extensions are ignored.

This comment has been minimized.

@allardhoeve

allardhoeve Sep 13, 2016

I think it is a valid approach to first have basic verification scenarios work and then work on corner cases.

This comment has been minimized.

@tiran

tiran Sep 13, 2016

Contributor

I was talking about unknown critical extensions. By definition a library must fail validation when a certificate contains an unknown X509v3 extension with the CRITICAL bit. @alex told me on IRC that cryptography handles the case internally. It raises an exception on accessing any extension.

def _is_issuing_certificate(issuing_certificate, issued_certificate):
return (issuing_certificate.subject == issued_certificate.issuer)

This comment has been minimized.

@mwielgoszewski

mwielgoszewski Oct 15, 2016

Contributor

This check, in addition to _can_sign_certificates, should incorporate a check to detect if the issuing_certificate is name constrained in any way, and if so, validate that the issued_certificate's subject / subjectAltName(s) match the issuing_certificate's set of name constraints.

@reaperhulk reaperhulk removed this from the Sixteenth Release milestone Nov 19, 2016

@ofek

This comment has been minimized.

Contributor

ofek commented Dec 20, 2016

Where are we on this?

@PeterHamilton

This comment has been minimized.

Contributor

PeterHamilton commented Dec 20, 2016

@ofek I'm not actively working on this. I'm fine with anyone picking this up and working on it going forward.

@allardhoeve

This comment has been minimized.

allardhoeve commented Feb 15, 2017

Is there still any plan to support verification of certificates? This seems like a often-used feature that is currently missing. I'd hate to have to call openssl from the commandline to do this.

@reaperhulk

This comment has been minimized.

Member

reaperhulk commented Feb 15, 2017

There is a desire for it, but it hasn't reached the top of my todo list and it's definitely something that will require extensive conformance and validation tests. If someone else is interested in tackling it we welcome discussion. Aggregating resources that specify precisely how validation should work as well as test vectors would be helpful for anyone interested in pursuing this.

@reaperhulk

This comment has been minimized.

Member

reaperhulk commented Jul 9, 2017

This is being actively worked on by @alex but the PR isn't ready yet. I'm going to go ahead and close this, but hopefully we'll have something in time for cryptography 2.1.

@reaperhulk reaperhulk closed this Jul 9, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment