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

Minor refactoring of Fulcio SCT handling #1259

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 1 addition & 22 deletions sigstore/_internal/fulcio/client.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,13 +33,8 @@
CertificateSigningRequest,
load_pem_x509_certificate,
)
from cryptography.x509.certificate_transparency import SignedCertificateTimestamp

from sigstore._internal import USER_AGENT
from sigstore._internal.sct import (
UnexpectedSctCountException,
_get_precertificate_signed_certificate_timestamps,
)
from sigstore._utils import B64Str
from sigstore.oidc import IdentityToken

Expand All @@ -51,14 +46,6 @@
TRUST_BUNDLE_ENDPOINT = "/api/v2/trustBundle"


class FulcioSCTError(Exception):
"""
Raised on errors when constructing a `FulcioSignedCertificateTimestamp`.
"""

pass


class ExpiredCertificate(Exception):
"""An error raised when the Certificate is expired."""

Expand All @@ -69,7 +56,6 @@ class FulcioCertificateSigningResponse:

cert: Certificate
chain: List[Certificate]
sct: SignedCertificateTimestamp


@dataclass(frozen=True)
Expand Down Expand Up @@ -148,14 +134,7 @@ def post(
cert = load_pem_x509_certificate(certificates[0].encode())
chain = [load_pem_x509_certificate(c.encode()) for c in certificates[1:]]

try:
# The SignedCertificateTimestamp should be accessed by the index 0
sct = _get_precertificate_signed_certificate_timestamps(cert)[0]

except UnexpectedSctCountException as ex:
raise FulcioClientError(ex)

return FulcioCertificateSigningResponse(cert, chain, sct)
return FulcioCertificateSigningResponse(cert, chain)


class FulcioTrustBundle(_Endpoint):
Expand Down
35 changes: 16 additions & 19 deletions sigstore/_internal/sct.py
Original file line number Diff line number Diff line change
Expand Up @@ -148,32 +148,28 @@ def _get_issuer_cert(chain: List[Certificate]) -> Certificate:
return issuer


class UnexpectedSctCountException(Exception):
"""
Number of percerts scts is wrong
"""

pass


def _get_precertificate_signed_certificate_timestamps(
def _get_signed_certificate_timestamp(
certificate: Certificate,
) -> PrecertificateSignedCertificateTimestamps:
# Try to retrieve the embedded SCTs within the cert.
) -> SignedCertificateTimestamp:
"""Retrieve the embedded SCT from the certificate.

Raise VerificationError if certificate does not contain exactly one SCT
"""
try:
precert_scts_extension = certificate.extensions.get_extension_for_class(
timestamps = certificate.extensions.get_extension_for_class(
PrecertificateSignedCertificateTimestamps
).value
except ExtensionNotFound:
raise ValueError(
"No PrecertificateSignedCertificateTimestamps found for the certificate"
raise VerificationError(
"Certificate does not contain a signed certificate timestamp extension"
)

if len(precert_scts_extension) != 1:
raise UnexpectedSctCountException(
f"Unexpected embedded SCT count in response: {len(precert_scts_extension)} != 1"
if len(timestamps) != 1:
raise VerificationError(
f"Expected one certificate timestamp, found {len(timestamps)}."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: no periods in exceptons

Suggested change
f"Expected one certificate timestamp, found {len(timestamps)}."
f"Expected one certificate timestamp, found {len(timestamps)}"

)
return precert_scts_extension
sct: SignedCertificateTimestamp = timestamps[0]
return sct


def _cert_is_ca(cert: Certificate) -> bool:
Expand All @@ -187,7 +183,6 @@ def _cert_is_ca(cert: Certificate) -> bool:


def verify_sct(
sct: SignedCertificateTimestamp,
cert: Certificate,
chain: List[Certificate],
ct_keyring: CTKeyring,
Expand All @@ -201,6 +196,8 @@ def verify_sct(
log to sign SCTs).
"""

sct = _get_signed_certificate_timestamp(cert)

issuer_key_id = None
if sct.entry_type == LogEntryType.PRE_CERTIFICATE:
# If we're verifying an SCT for a precertificate, we need to
Expand Down
12 changes: 3 additions & 9 deletions sigstore/sign.py
Original file line number Diff line number Diff line change
Expand Up @@ -161,21 +161,15 @@ def _signing_cert(
certificate_request, self._identity_token
)

# Verify the SCT
sct = certificate_response.sct
cert = certificate_response.cert
chain = certificate_response.chain

verify_sct(
sct,
cert,
chain,
certificate_response.cert,
certificate_response.chain,
self._signing_ctx._trusted_root.ct_keyring(KeyringPurpose.SIGN),
)

_logger.debug("Successfully verified SCT...")

return cert
return certificate_response.cert

def _finalize_sign(
self,
Expand Down
3 changes: 0 additions & 3 deletions sigstore/verify/verifier.py
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@
from sigstore._internal.rekor import _hashedrekord_from_parts
from sigstore._internal.rekor.client import RekorClient
from sigstore._internal.sct import (
_get_precertificate_signed_certificate_timestamps,
verify_sct,
)
from sigstore._internal.timestamp import TimestampSource, TimestampVerificationResult
Expand Down Expand Up @@ -339,10 +338,8 @@ def _verify_common_signing_cert(
chain = self._verify_chain_at_time(cert_ossl, vts)

# (2): verify the signing certificate's SCT.
sct = _get_precertificate_signed_certificate_timestamps(cert)[0]
try:
verify_sct(
sct,
cert,
[parent_cert.to_cryptography() for parent_cert in chain],
self._trusted_root.ct_keyring(KeyringPurpose.VERIFY),
Expand Down
Loading