-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Changes from all commits
e59db1c
0059d15
b787c85
63f8140
da88393
3497c1c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,93 @@ | ||
# This file is dual licensed under the terms of the Apache License, Version | ||
# 2.0, and the BSD License. See the LICENSE file in the root of this repository | ||
# for complete details. | ||
|
||
from __future__ import absolute_import, division, print_function | ||
|
||
from cryptography.hazmat.primitives.asymmetric import ec, padding, rsa | ||
from cryptography.x509 import Certificate | ||
from cryptography.x509.oid import ExtensionOID | ||
|
||
|
||
class InvalidCertificate(Exception): | ||
pass | ||
|
||
|
||
class InvalidSigningCertificate(Exception): | ||
pass | ||
|
||
|
||
def _can_sign_certificates(certificate): | ||
basic_constraints = certificate.extensions.get_extension_for_oid( | ||
ExtensionOID.BASIC_CONSTRAINTS).value | ||
key_usage = certificate.extensions.get_extension_for_oid( | ||
ExtensionOID.KEY_USAGE).value | ||
|
||
if not basic_constraints.ca: | ||
raise InvalidSigningCertificate( | ||
"The certificate is not marked as a CA in its BasicConstraints " | ||
"extension." | ||
) | ||
elif not key_usage.key_cert_sign: | ||
raise InvalidSigningCertificate( | ||
"The certificate public key is not marked for verifying " | ||
"certificates in its KeyUsage extension." | ||
) | ||
else: | ||
return True | ||
|
||
|
||
def _is_issuing_certificate(issuing_certificate, issued_certificate): | ||
return (issuing_certificate.subject == issued_certificate.issuer) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This check, in addition to |
||
|
||
|
||
class CertificateVerificationContext(object): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How does the verification context handle unknown critical v3 extensions? Does it raise an exception or does it ignore unknown critical extensions? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right now the context only examines the Basic Constraints and Key Usage extensions for fields required for CA use. Other extensions are ignored. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it is a valid approach to first have basic verification scenarios work and then work on corner cases. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was talking about unknown critical extensions. By definition a library must fail validation when a certificate contains an unknown X509v3 extension with the |
||
def __init__(self, certificate): | ||
if not isinstance(certificate, Certificate): | ||
raise InvalidCertificate( | ||
"The signing certificate must be a Certificate." | ||
) | ||
_can_sign_certificates(certificate) | ||
|
||
self._signing_cert = certificate | ||
|
||
def update(self, certificate): | ||
""" | ||
Processes the provided certificate. Raises an exception if the | ||
certificate is invalid. | ||
""" | ||
if not isinstance(certificate, Certificate): | ||
raise InvalidCertificate( | ||
"The signed certificate must be a Certificate." | ||
) | ||
|
||
self._signed_cert = certificate | ||
|
||
def verify(self): | ||
""" | ||
Verifies the signature of the certificate provided to update against | ||
the certificate associated with the context. Raises an exception if | ||
the verification process fails. | ||
""" | ||
if not _is_issuing_certificate(self._signing_cert, self._signed_cert): | ||
raise InvalidCertificate( | ||
"The certificate issuer does not match the subject name of " | ||
"the context certificate." | ||
) | ||
|
||
signature_hash_algorithm = self._signed_cert.signature_hash_algorithm | ||
signature_bytes = self._signed_cert.signature | ||
signer_public_key = self._signing_cert.public_key() | ||
|
||
if isinstance(signer_public_key, rsa.RSAPublicKey): | ||
verifier = signer_public_key.verifier( | ||
signature_bytes, padding.PKCS1v15(), signature_hash_algorithm) | ||
elif isinstance(signer_public_key, ec.EllipticCurvePublicKey): | ||
verifier = signer_public_key.verifier( | ||
signature_bytes, ec.ECDSA(signature_hash_algorithm)) | ||
else: | ||
verifier = signer_public_key.verifier( | ||
signature_bytes, signature_hash_algorithm) | ||
|
||
verifier.update(self._signed_cert.tbs_certificate_bytes) | ||
verifier.verify() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
Regarding the Key Usage extension, see Section 4.2.1.3, top of page 29:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.