trust: Provide a better way to select service versions

[jku]

#1387 would have less hacky if-elses if the SigningConfig had a proper way to access services by major version:

There's two ways to go about it :

1. We can just add version arguments to get_tlog_urls() and get_tsa_urls(). This separates concerns so that

   • SigningConfig can deal with complexities of services (operators, selectors, etc)
   • the signing code knows which version it prefers to work with

2. instead of returning urls, return Services for all versions and let the signing code deal with choosing the services it wants -- this seems undesirable because it continues to make signing code more complicated

I'll try this out and see if option 1 is enough for us

[jku]

option 1 is just a bit too restrictive: I'll go with this:

• SigningConfig returns urls and major versions (or even actual clients)
• SigningConfig still makes choices based on signingconfig constraints (e.g. validity periods and ensuring only one service version is listed per operator)

I think SigningConfig returning actual clients (like RekorClient) makes some sense: SigningConfig must know the supported major versions so it can also choose the correct client (in cases where there may be e.g. RekorClient and RekorV2Client)

[ramonpetgrave64]

@jku I agree, and then:

So far, in #1387 , three of the if-elses I have I think can be avoided with another of your suggestions: create a new _build_hashed_rekord_request() and send_entry_request() that both the original RekorClient and the new RekorV2Client can share via a new ABC class.

• https://github.com/sigstore/sigstore-python/pull/1387/files#diff-56676202bf27a76693661acf8b78c4ce442ba81eca1f9244493f9c3958c8319fR329
• https://github.com/sigstore/sigstore-python/pull/1387/files#diff-56676202bf27a76693661acf8b78c4ce442ba81eca1f9244493f9c3958c8319fR209
• https://github.com/sigstore/sigstore-python/pull/1387/files#diff-56676202bf27a76693661acf8b78c4ce442ba81eca1f9244493f9c3958c8319fR245

Like this. This is just a mockup, and I have not yet tried if this will work well with our typing linters. But what do you think?

from abc import ABC, abstractmethod
import rekor_types
from sigstore._internal.rekor.v2_types.dev.sigstore.rekor import v2
from sigstore.models import LogEntry
from sigstore.hashes import Hashed
from cryptography.x509 import Certificate
from sigstore.dsse import Envelope

Request = rekor_types.Hashedrekord | rekor_types.Dsse | v2.CreateEntryRequest
HashedRekordRequest = rekor_types.Hashedrekord | v2.CreateEntryRequest
DsseRequest = rekor_types.Dsse | v2.CreateEntryRequest

class RekorLogSubmitter(ABC):

    @abstractmethod
    def create_entry(self, request: Request) -> LogEntry:
        pass

    @abstractmethod
    def _build_hashed_rekord_request(self, hashed_input: Hashed, signature: bytes, certificate: Certificate) -> HashedRekordRequest:
        pass

    @abstractmethod
    def _build_dsse_request(self, envelope: Envelope, certificate: Certificate) -> DsseRequest:
        pass   


class RekorV2Client(RekorLogSubmitter):
    def create_entry(self, request: v2.CreateEntryRequest) -> LogEntry:
        print("post")

    def _build_hashed_rekord_request(self, hashed_input: Hashed, signature: bytes, certificate: Certificate) -> HashedRekordRequest:
        print("hash rekord request")

    def _build_dsse_request(self, envelope: Envelope, certificate: Certificate) -> DsseRequest:
        print("dsse request")

# Also add these new methods to the existing `RekorClient`, and also let it inherit `RekorLogSubmitter`.

[ramonpetgrave64]

I have another draft PR showing the use of a new abstract RekorLogSubmitter class, like I described above.

#1404

I started trying our returning the Service object and I think it looks okay.

• main...ramonpetgrave64:sigstore-python:rekov2-client-base-class-signing-ctx
  • main...ramonpetgrave64:sigstore-python:rekov2-client-base-class-signing-ctx#diff-f1d89987f8e552420257b080d0cdd1ef3d2b2c696e5f97080517ac357d458861R357-R396
  • main...ramonpetgrave64:sigstore-python:rekov2-client-base-class-signing-ctx#diff-56676202bf27a76693661acf8b78c4ce442ba81eca1f9244493f9c3958c8319fR325-R347

diff --git a/sigstore/_internal/trust.py b/sigstore/_internal/trust.py
index ae104e0..431e438 100644
--- a/sigstore/_internal/trust.py
+++ b/sigstore/_internal/trust.py
@@ -357,9 +357,6 @@ class SigningConfig:
     @staticmethod
     def _get_valid_service_url(services: list[Service]) -> str | None:
         for service in services:
-            if service.major_api_version != 1:
-                continue
-
             if not _is_timerange_valid(service.valid_for, allow_expired=False):
                 continue
             return service.url
@@ -376,6 +373,27 @@ class SigningConfig:
             raise Error("No valid Rekor transparency log found in signing config")
         return [url]
 
+    @staticmethod
+    def _is_valid_service(service: Service) -> bool:
+        """
+        Returns whether the given `Service` is valid.
+        """
+        return _is_timerange_valid(service.valid_for, allow_expired=False)
+
+    def get_tlog_services(self) -> list[Service]:
+        """
+        Returns a `Service` for each in the signing config, sorted by major version from highest to lowest.
+        """
+        return sorted(
+            [
+                service
+                for service in self._inner.rekor_tlog_urls
+                if self._is_valid_service(service)
+            ],
+            key=lambda service: service.major_api_version,
+            reverse=True,
+        )
+
     def get_fulcio_url(self) -> str:
         """
         Returns url for the fulcio instance that client should use to get a

         return self._finalize_sign(cert, content, proposed_entry)
@@ -353,9 +322,26 @@ class SigningContext:
         @api private
         """
         signing_config = trust_config.signing_config
+        tlog_service = next(
+            (
+                service
+                for service in signing_config.get_tlog_services()
+                if service.major_api_version in SUPPORTED_REKOR_MAJOR_API_VERSIONS
+            ),
+            None,
+        )
+        if tlog_service is None:
+            raise ValueError(
+                "No supported rekor version supplied. Supporting {SUPPORTED_REKOR_MAJOR_API_VERSIONS}"
+            )
+        if tlog_service.major_api_version == 1:
+            rekor_client = RekorClient(tlog_service.url)
+        else:
+            rekor_client = RekorClient(tlog_service.url)
+
         return cls(
             fulcio=FulcioClient(signing_config.get_fulcio_url()),
-            rekor=RekorClient(signing_config.get_tlog_urls()[0]),
+            rekor=rekor_client,
             trusted_root=trust_config.trusted_root,
             tsa_clients=[
                 TimestampAuthorityClient(url) for url in signing_config.get_tsa_urls()

[jku]

So far, in #1387 , three of the if-elses I have I think can be avoided with another of your suggestions: create a new _build_hashed_rekord_request() and send_entry_request() that both the original RekorClient and the new RekorV2Client can share via a new ABC class.

Yeah, that's roughly what I was hoping would be possible. If we can't have the create_entry method have a shared signature without jumping through hoops I think that's fine -- one if-else is manageable in signing code especially since we know we plan to deprecate v1 at some point.

I started trying our returning the Service object and I think it looks okay.

Yeah, it is an option... I wanted to avoid returnign Services for two reasons:

• Service is a protobuf and would be nice to keep the protobuf handling from spreading all over the code
• I think there is no need for the signing code to become more complicated: the trust module should be able to do all of these service selection decisions and can just provide the signing code that clients it should use

#1407 is my attempt to cover these issues, let me know what you think