Skip to content

Commit

Permalink
Handle the case of missing EKU in _is_preissuer (#658)
Browse files Browse the repository at this point in the history
RFC 6962 stipulate that the TBSCertificate can be either signed by:
- A special purpose CA Precertificate Signing Certificate with an EKU
- The final CA Certificate with no mandatory EKU
In _is_preissuer was failing on the later if no EKU was present

In both case, the certificate must be a valid CA

Resolves: #658
Signed-off-by: Cyril Cordoui <[email protected]>
  • Loading branch information
ccordoui committed Jun 20, 2023
1 parent d3e7fa1 commit dc1428c
Show file tree
Hide file tree
Showing 4 changed files with 42 additions and 14 deletions.
8 changes: 7 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ All versions prior to 0.9.0 are untracked.
reducing the number of cryptographic operations and network roundtrips
required when signing more than one input
([#645](https://github.com/sigstore/sigstore-python/pull/645))

* `sigstore sign` now uses an ephemeral P-256 keypair, rather than P-384
([#662](https://github.com/sigstore/sigstore-python/pull/662))

Expand All @@ -69,6 +69,12 @@ All versions prior to 0.9.0 are untracked.
`sigstore.oidc.detect_credential` API
([#641](https://github.com/sigstore/sigstore-python/pull/641))

* Fixed a case where `sigstore sign` could fail using a private instance
(and `sigstore verrify`) due to a missing due to a missing `ExtendedKeyUsage`
in the CA
We now enforce the fact that the TBSPrecertificate signer must be a valid CA
([#658](https://github.com/sigstore/sigstore-python/pull/658))

## [1.1.2]

### Fixed
Expand Down
34 changes: 30 additions & 4 deletions sigstore/_internal/sct.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@

from cryptography.hazmat.primitives import hashes, serialization
from cryptography.hazmat.primitives.asymmetric import ec, rsa
from cryptography.x509 import Certificate, ExtendedKeyUsage
from cryptography.x509 import Certificate, ExtendedKeyUsage, ExtensionNotFound
from cryptography.x509.certificate_transparency import (
LogEntryType,
SignedCertificateTimestamp,
Expand All @@ -37,7 +37,13 @@
KeyringLookupError,
KeyringSignatureError,
)
from sigstore._utils import DERCert, KeyID, key_id
from sigstore._utils import (
DERCert,
InvalidCertError,
KeyID,
cert_is_ca,
key_id,
)
from sigstore.errors import Error

logger = logging.getLogger(__name__)
Expand Down Expand Up @@ -127,7 +133,11 @@ def _pack_digitally_signed(


def _is_preissuer(issuer: Certificate) -> bool:
ext_key_usage = issuer.extensions.get_extension_for_class(ExtendedKeyUsage)
try:
ext_key_usage = issuer.extensions.get_extension_for_class(ExtendedKeyUsage)
# If we do not have any EKU, we certainly do not have CT Ext
except ExtensionNotFound:
return False

return ExtendedKeyUsageOID.CERTIFICATE_TRANSPARENCY in ext_key_usage.value

Expand All @@ -139,6 +149,16 @@ def _get_issuer_cert(chain: List[Certificate]) -> Certificate:
return issuer


def _cert_is_ca(cert: Certificate) -> bool:
logger.debug(f"Found {cert.subject} as issuer, verifying if it is a ca")
try:
cert_is_ca(cert)
except InvalidCertError as e:
logger.debug(f"Invalid {cert.subject}: failed to validate as a CA: {e}")
return False
return True


class InvalidSCTError(Error):
"""
Raised during SCT verification if an SCT is invalid in some way.
Expand Down Expand Up @@ -231,7 +251,13 @@ def verify_sct(
# find its issuer in the chain and calculate a hash over
# its public key information, as part of the "binding" proof
# that ties the issuer to the final certificate.
issuer_pubkey = _get_issuer_cert(chain).public_key()
issuer_cert = _get_issuer_cert(chain)
issuer_pubkey = issuer_cert.public_key()

if not _cert_is_ca(issuer_cert):
raise InvalidSCTError(
f"Invalid issuer pubkey basicConstraint (not a CA): {issuer_pubkey}"
)

if not isinstance(issuer_pubkey, (rsa.RSAPublicKey, ec.EllipticCurvePublicKey)):
raise InvalidSCTError(
Expand Down
12 changes: 4 additions & 8 deletions sigstore/_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -226,28 +226,24 @@ def cert_is_ca(cert: Certificate) -> bool:
# No BasicConstrains means that this can't possibly be a CA.
return False

digital_signature = False
key_cert_sign = False
try:
key_usage = cert.extensions.get_extension_for_oid(ExtensionOID.KEY_USAGE)
key_cert_sign = key_usage.value.key_cert_sign # type: ignore
digital_signature = key_usage.value.digital_signature # type: ignore
except ExtensionNotFound:
raise InvalidCertError("invalid X.509 certificate: missing KeyUsage")

# If all three states are set, this is a CA.
if ca and key_cert_sign and digital_signature:
if ca and key_cert_sign:
return True

# Non-CA in the Sigstore ecosystem have `digitalSignature` but neither of
# the CA states.
if digital_signature and not (ca or key_cert_sign):
if not (ca or key_cert_sign):
return False

# Anything else is an invalid state that should never occur.
raise InvalidCertError(
f"invalid certificate states: KeyUsage.digitalSignature={digital_signature}, "
f"KeyUsage.keyCertSign={key_cert_sign}, BasicConstraints.ca={ca}"
f"invalid certificate states: KeyUsage.keyCertSign={key_cert_sign}"
f", BasicConstraints.ca={ca}"
)


Expand Down
2 changes: 1 addition & 1 deletion test/unit/test_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,6 @@ def test_cert_is_ca(x509_testcase, testcase, valid):
"bogus-root-noncritical-bc.pem",
"bogus-root-invalid-ku.pem",
"bogus-root-missing-ku.pem",
"bogus-leaf-invalid-ku.pem",
],
)
def test_cert_is_ca_invalid_states(x509_testcase, testcase):
Expand All @@ -135,6 +134,7 @@ def test_cert_is_ca_invalid_states(x509_testcase, testcase):
("bogus-root.pem", True),
("bogus-intermediate.pem", False),
("bogus-leaf.pem", False),
("bogus-leaf-invalid-ku.pem", False),
],
)
def test_cert_is_root_ca(x509_testcase, testcase, valid):
Expand Down

0 comments on commit dc1428c

Please sign in to comment.