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

fulcio: remove detached SCT support #1236

Merged
merged 3 commits into from
Dec 3, 2024
Merged
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
6 changes: 6 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,12 @@ All versions prior to 0.9.0 are untracked.
Trusted Root contains one or more Timestamp Authorities
([#1216](https://github.com/sigstore/sigstore-python/pull/1216))

### Removed

* Support for "detached" SCTs has been fully removed, aligning
sigstore-python with other sigstore clients
([#1236](https://github.com/sigstore/sigstore-python/pull/1236))

### Fixed

* Fixed a CLI parsing bug introduced in 3.5.1 where a warning about
Expand Down
2 changes: 0 additions & 2 deletions sigstore/_internal/fulcio/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,12 @@
"""

from .client import (
DetachedFulcioSCT,
ExpiredCertificate,
FulcioCertificateSigningResponse,
FulcioClient,
)

__all__ = [
"DetachedFulcioSCT",
"ExpiredCertificate",
"FulcioCertificateSigningResponse",
"FulcioClient",
Expand Down
167 changes: 14 additions & 153 deletions sigstore/_internal/fulcio/client.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,30 +19,21 @@
from __future__ import annotations

import base64
import datetime
import json
import logging
import struct
from abc import ABC
from dataclasses import dataclass
from enum import IntEnum
from typing import List
from urllib.parse import urljoin

import requests
from cryptography.hazmat.primitives import hashes, serialization
from cryptography.hazmat.primitives import serialization
from cryptography.x509 import (
Certificate,
CertificateSigningRequest,
load_pem_x509_certificate,
)
from cryptography.x509.certificate_transparency import (
LogEntryType,
SignatureAlgorithm,
SignedCertificateTimestamp,
Version,
)
from pydantic import BaseModel, ConfigDict, Field, field_validator
from cryptography.x509.certificate_transparency import SignedCertificateTimestamp

from sigstore._internal import USER_AGENT
from sigstore._internal.sct import (
Expand All @@ -60,33 +51,6 @@
TRUST_BUNDLE_ENDPOINT = "/api/v2/trustBundle"


class SCTHashAlgorithm(IntEnum):
"""
Hash algorithms that are valid for SCTs.

These are exactly the same as the HashAlgorithm enum in RFC 5246 (TLS 1.2).

See: https://datatracker.ietf.org/doc/html/rfc5246#section-7.4.1.4.1
"""

NONE = 0
MD5 = 1
SHA1 = 2
SHA224 = 3
SHA256 = 4
SHA384 = 5
SHA512 = 6

def to_cryptography(self) -> hashes.HashAlgorithm:
"""
Converts this `SCTHashAlgorithm` into a `cryptography.hashes` object.
"""
if self != SCTHashAlgorithm.SHA256:
raise FulcioSCTError(f"unexpected hash algorithm: {self!r}")

return hashes.SHA256()


class FulcioSCTError(Exception):
"""
Raised on errors when constructing a `FulcioSignedCertificateTimestamp`.
Expand All @@ -95,76 +59,6 @@ class FulcioSCTError(Exception):
pass


class DetachedFulcioSCT(BaseModel):
"""
Represents a "detached" SignedCertificateTimestamp from Fulcio.
"""

model_config = ConfigDict(populate_by_name=True, arbitrary_types_allowed=True)

version: Version = Field(..., alias="sct_version")
log_id: bytes = Field(..., alias="id")
timestamp: datetime.datetime
digitally_signed: bytes = Field(..., alias="signature")
extension_bytes: bytes = Field(..., alias="extensions")

@field_validator("timestamp")
def _validate_timestamp(cls, v: datetime.datetime) -> datetime.datetime:
return v.replace(tzinfo=datetime.timezone.utc)

@field_validator("digitally_signed", mode="before")
def _validate_digitally_signed(cls, v: bytes) -> bytes:
digitally_signed = base64.b64decode(v)

if len(digitally_signed) <= 4:
raise ValueError("impossibly small digitally-signed struct")

return digitally_signed

@field_validator("log_id", mode="before")
def _validate_log_id(cls, v: bytes) -> bytes:
return base64.b64decode(v)

@field_validator("extension_bytes", mode="before")
def _validate_extensions(cls, v: bytes) -> bytes:
return base64.b64decode(v)

@property
def entry_type(self) -> LogEntryType:
"""
Returns the kind of CT log entry this detached SCT is signing for.
"""
return LogEntryType.X509_CERTIFICATE

@property
def signature_hash_algorithm(self) -> hashes.HashAlgorithm:
"""
Returns the hash algorithm used in this detached SCT's signature.
"""
hash_ = SCTHashAlgorithm(self.digitally_signed[0])
return hash_.to_cryptography()

@property
def signature_algorithm(self) -> SignatureAlgorithm:
"""
Returns the signature algorithm used in this detached SCT's signature.
"""
return SignatureAlgorithm(self.digitally_signed[1])

@property
def signature(self) -> bytes:
"""
Returns the raw signature inside the detached SCT.
"""
(sig_size,) = struct.unpack("!H", self.digitally_signed[2:4])
if len(self.digitally_signed[4:]) != sig_size:
raise FulcioSCTError(
f"signature size mismatch: expected {sig_size} bytes, "
f"got {len(self.digitally_signed[4:])}"
)
return self.digitally_signed[4:]


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

Expand Down Expand Up @@ -238,22 +132,12 @@ def post(
raise FulcioClientError(text["message"]) from http_error
raise FulcioClientError from http_error

if resp.json().get("signedCertificateEmbeddedSct"):
sct_embedded = True
try:
certificates = resp.json()["signedCertificateEmbeddedSct"]["chain"][
"certificates"
]
except KeyError:
raise FulcioClientError("Fulcio response missing certificate chain")
else:
sct_embedded = False
Copy link
Contributor

Choose a reason for hiding this comment

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

For instances that don’t use a CT log, the certificate chain is written to signedCertificateDetachedSct. To continue to support these local deployments, you’d need to still read the chain from either field, but only check the SCT if embedded.

Cc @codysoyland who added that for Sigstore-go

Copy link
Member

Choose a reason for hiding this comment

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

Is it not a problem if the server can just "downgrade" to detached sct like that?

Or are we supposed to always require embedded SCT if the trust root contains at least one CT log, but otherwise be ok with detached SCT?

Copy link
Member Author

Choose a reason for hiding this comment

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

Right now sigstore-python doesn't allow non-CT BYO uses at all, so allowing signedCertificateDetachedSct while removing detached SCT support would mean a larger refactor 😅

Given that, I think we're OK to remove this outright, since signedCertificateDetachedSct-without-CT is a use case we already didn't support. But I'm curious if this matches @jku's understanding 🙂

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it not a problem if the server can just "downgrade" to detached sct like that?

Or are we supposed to always require embedded SCT if the trust root contains at least one CT log, but otherwise be ok with detached SCT?

For Sigstore-go, it’s a threshold configured by the user. For cosign, SCTs are required unless opted out explicitly by the user.

If you require CT now, then agreed this would not be a breaking change.

Copy link
Member

Choose a reason for hiding this comment

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

@woodruffw sure, that sounds good.

try:
certificates = resp.json()["signedCertificateDetachedSct"]["chain"][
"certificates"
]
except KeyError:
raise FulcioClientError("Fulcio response missing certificate chain")
try:
certificates = resp.json()["signedCertificateEmbeddedSct"]["chain"][
"certificates"
]
except KeyError:
raise FulcioClientError("Fulcio response missing certificate chain")

# Cryptography doesn't have chain verification/building built in
# https://github.com/pyca/cryptography/issues/2381
Expand All @@ -264,35 +148,12 @@ def post(
cert = load_pem_x509_certificate(certificates[0].encode())
chain = [load_pem_x509_certificate(c.encode()) for c in certificates[1:]]

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

except UnexpectedSctCountException as ex:
raise FulcioClientError(ex)

else:
# If we don't have any embedded SCTs, then we might be dealing
# with a Fulcio instance that provides detached SCTs.

# The detached SCT is a base64-encoded payload, which in turn
# is a JSON representation of the SignedCertificateTimestamp
# in RFC 6962 (subsec. 3.2).
try:
sct_b64 = resp.json()["signedCertificateDetachedSct"][
"signedCertificateTimestamp"
]
except KeyError:
raise FulcioClientError(
"Fulcio response did not include a detached SCT"
)

try:
sct = DetachedFulcioSCT.model_validate_json(base64.b64decode(sct_b64))
except Exception as exc:
# Ideally we'd catch something less generic here.
raise FulcioClientError from exc
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)
Comment on lines +151 to +156
Copy link
Member

Choose a reason for hiding this comment

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

itching to refactor these methods a bit, using them is overly complicated and it's still incomplete (you could get ValueError in addition to this unnecessary internal error)... but that's not really related to this PR

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I'd love to have these cleaned up. Let's do a follow-up for that 🙂


return FulcioCertificateSigningResponse(cert, chain, sct)

Expand Down
Loading
Loading