PR #1252 review suggestions fixed

anthonydahanne · anthonydahanne · commit 75bdd899ce78 · 2026-07-24T19:30:32.000-04:00
diff --git a/sigstore-cli/src/main/java/dev/sigstore/cli/Verify.java b/sigstore-cli/src/main/java/dev/sigstore/cli/Verify.java
@@ -113,16 +113,6 @@ static class Policy {
       required = false)
   Path workingDirectory;
 
-  @Option(
-      names = {"--allow-non-transparency-log"},
-      description =
-          "verify a bundle that has no transparency-log entry, relying on a signed RFC 3161 "
-              + "timestamp instead (reduced assurance; for instances that do not publish to a "
-              + "transparency log, e.g. GitHub's Sigstore instance for private repositories)",
-      required = false,
-      defaultValue = "false")
-  Boolean allowNonTransparencyLog;
-
   @Override
   public Integer call() throws Exception {
     byte[] digest;
@@ -152,9 +142,6 @@ public Integer call() throws Exception {
               .subjectAlternativeName(StringMatcher.string(policy.certificateSan))
               .build());
     }
-    if (allowNonTransparencyLog) {
-      verificationOptionsBuilder.allowNonTransparencyLogVerification(true);
-    }
     var verificationOptions = verificationOptionsBuilder.build();
 
     KeylessVerifier verifier;
diff --git a/sigstore-java/src/main/java/dev/sigstore/KeylessVerifier.java b/sigstore-java/src/main/java/dev/sigstore/KeylessVerifier.java
@@ -132,43 +132,31 @@ public Builder trustedRootProvider(TrustedRootProvider trustedRootProvider) {
   private AlgorithmRegistry.HashAlgorithm findHashAlgorithm(Bundle bundle)
       throws KeylessVerificationException {
     try {
-      // When the bundle has no transparency-log entry (e.g. a signed-timestamp-only bundle),
-      // derive the hash algorithm without one: DSSE bundles use SHA-256 (as in the dsse:0.0.1
-      // case below), otherwise fall back to the signing certificate's algorithm. Whether such a
-      // bundle is permitted at all is enforced in verify(...) via
-      // VerificationOptions#allowNonTransparencyLogVerification.
-      if (bundle.getEntries().isEmpty()) {
-        if (bundle.getDsseEnvelope().isPresent()) {
-          return AlgorithmRegistry.HashAlgorithm.SHA2_256;
-        }
-        var publicKey = Certificates.getLeaf(bundle.getCertPath()).getPublicKey();
-        return AlgorithmRegistry.getSigningAlgorithm(publicKey).getHashAlgorithm();
-      }
-      var rekorEntry = bundle.getEntries().get(0);
-      var body = rekorEntry.getBodyDecoded();
-      if ("0.0.1".equals(body.getApiVersion())) {
-        if ("hashedrekord".equals(body.getKind())) {
-          var entry = RekorTypes.getHashedRekordV001(rekorEntry);
-          switch (entry.getData().getHash().getAlgorithm()) {
-            case SHA_256:
-              return AlgorithmRegistry.HashAlgorithm.SHA2_256;
-            case SHA_384:
-              return AlgorithmRegistry.HashAlgorithm.SHA2_384;
-            case SHA_512:
-              return AlgorithmRegistry.HashAlgorithm.SHA2_512;
+      // rekor 0.0.1 hashedrekord entries encode the hash algorithm in the entry itself. Others
+      // (rekor 0.0.2, a bundle with no transparency-log entry) derive it from the signing
+      // certificate's public key.
+      if (!bundle.getEntries().isEmpty()) {
+        var rekorEntry = bundle.getEntries().get(0);
+        var body = rekorEntry.getBodyDecoded();
+        if ("0.0.1".equals(body.getApiVersion())) {
+          if ("hashedrekord".equals(body.getKind())) {
+            var entry = RekorTypes.getHashedRekordV001(rekorEntry);
+            switch (entry.getData().getHash().getAlgorithm()) {
+              case SHA_256:
+                return AlgorithmRegistry.HashAlgorithm.SHA2_256;
+              case SHA_384:
+                return AlgorithmRegistry.HashAlgorithm.SHA2_384;
+              case SHA_512:
+                return AlgorithmRegistry.HashAlgorithm.SHA2_512;
+            }
+          } else if ("dsse".equals(body.getKind())) {
+            // dsse:0.0.1 is always sha256
+            return AlgorithmRegistry.HashAlgorithm.SHA2_256;
           }
-        } else if ("dsse".equals(body.getKind())) {
-          // dsse:0.0.1 is always sha256
-          return AlgorithmRegistry.HashAlgorithm.SHA2_256;
         }
-      } else if ("0.0.2".equals(body.getApiVersion())) {
-        // rekor v2 entries must conform to the Algorithm Registry
-        var publicKey = Certificates.getLeaf(bundle.getCertPath()).getPublicKey();
-        var signingAlgorithm = AlgorithmRegistry.getSigningAlgorithm(publicKey);
-        return signingAlgorithm.getHashAlgorithm();
       }
-      throw new KeylessVerificationException(
-          "Unsupported entry type: '" + body.getKind() + ":" + body.getApiVersion() + "'");
+      var publicKey = Certificates.getLeaf(bundle.getCertPath()).getPublicKey();
+      return AlgorithmRegistry.getSigningAlgorithm(publicKey).getHashAlgorithm();
     } catch (JsonParseException | RekorTypeException | UnsupportedAlgorithmException ex) {
       throw new KeylessVerificationException(
           "Could not determine hash algorithm from sigstore bundle", ex);
@@ -211,8 +199,7 @@ public void verify(byte[] artifactDigest, Bundle bundle, VerificationOptions opt
 
     // a trusted CT log
     try {
-      fulcioVerifier.verifySigningCertificate(
-          signingCert, options.allowNonTransparencyLogVerification());
+      fulcioVerifier.verifySigningCertificate(signingCert, options.getCtLogOptions());
     } catch (FulcioVerificationException | IOException ex) {
       throw new KeylessVerificationException(
           "Fulcio certificate was not valid: " + ex.getMessage(), ex);
@@ -223,17 +210,17 @@ public void verify(byte[] artifactDigest, Bundle bundle, VerificationOptions opt
 
     var hashAlgorithm = findHashAlgorithm(bundle);
 
-    // A transparency-log entry is required by default. Bundles from instances that do not publish
-    // to a log (e.g. GitHub's Sigstore instance for private repositories) carry only a signed
-    // timestamp; verifying those requires opting in and relies on the RFC 3161 timestamp (verified
-    // below) for trusted time.
+    // A transparency-log entry is required unless disabled (e.g. a private deployment that verifies
+    // via a signed timestamp instead). We also refuse to ignore an entry that is present, so a
+    // disabled policy cannot silently skip verifying a log entry the bundle actually contains.
     var entries = bundle.getEntries();
-    if (entries.isEmpty() && !options.allowNonTransparencyLogVerification()) {
+    if (entries.isEmpty() && options.getTLogOptions().isEnabled()) {
+      throw new KeylessVerificationException(
+          "No transparency log entry found and transparency-log verification is required");
+    }
+    if (!entries.isEmpty() && !options.getTLogOptions().isEnabled()) {
       throw new KeylessVerificationException(
-          "Bundle does not contain a transparency log entry. If it is from an instance that does "
-              + "not publish to a transparency log, set "
-              + "VerificationOptions.allowNonTransparencyLogVerification(true) to verify using its "
-              + "signed timestamp instead.");
+          "Bundle contains a transparency log entry but transparency-log verification is disabled");
     }
     var rekorEntry = entries.isEmpty() ? null : entries.get(0);
 
diff --git a/sigstore-java/src/main/java/dev/sigstore/VerificationOptions.java b/sigstore-java/src/main/java/dev/sigstore/VerificationOptions.java
@@ -29,22 +29,38 @@ public interface VerificationOptions {
   /** An allow list of certificate identities to match with. */
   List<CertificateMatcher> getCertificateMatchers();
 
-  /**
-   * Whether to allow verifying a bundle that contains no transparency-log entry, relying solely on
-   * a signed RFC 3161 timestamp for trusted time. Defaults to {@code false}, matching the Sigstore
-   * public-good instance where a transparency-log entry is always present and required.
-   *
-   * <p>Enabling this weakens the guarantees: without a transparency-log entry the signature is not
-   * publicly monitorable/auditable. Only enable it for bundles from instances that intentionally do
-   * not publish to a transparency log (for example, GitHub's Sigstore instance for private
-   * repositories), and pair it with a pinned trusted root. A signed timestamp is still required.
-   *
-   * <p>This only relaxes the <em>requirement</em> that a transparency-log entry exists; a
-   * transparency-log entry that <em>is</em> present in the bundle is still fully verified.
-   */
+  /** Transparency-log (rekor) verification policy; defaults to requiring an entry. */
   @Default
-  default boolean allowNonTransparencyLogVerification() {
-    return false;
+  default TLogOptions getTLogOptions() {
+    return TLogOptions.builder().isEnabled(true).build();
+  }
+
+  /** Certificate-transparency (SCT) verification policy; defaults to requiring an SCT. */
+  @Default
+  default CTLogOptions getCtLogOptions() {
+    return CTLogOptions.builder().isEnabled(true).build();
+  }
+
+  /** Transparency-log verification options. */
+  @Immutable
+  interface TLogOptions {
+    /** Whether a transparency-log entry is required and verified. */
+    boolean isEnabled();
+
+    static ImmutableTLogOptions.Builder builder() {
+      return ImmutableTLogOptions.builder();
+    }
+  }
+
+  /** Certificate-transparency verification options. */
+  @Immutable
+  interface CTLogOptions {
+    /** Whether a certificate-transparency log entry (SCT) is required and verified. */
+    boolean isEnabled();
+
+    static ImmutableCTLogOptions.Builder builder() {
+      return ImmutableCTLogOptions.builder();
+    }
   }
 
   /**
diff --git a/sigstore-java/src/main/java/dev/sigstore/bundle/BundleReader.java b/sigstore-java/src/main/java/dev/sigstore/bundle/BundleReader.java
@@ -50,12 +50,7 @@ static Bundle readBundle(Reader jsonReader) throws BundleParseException {
 
     bundleBuilder.mediaType(protoBundle.getMediaType());
 
-    // A bundle may legitimately carry no transparency-log entry (for example, GitHub's Sigstore
-    // instance for private repositories, which relies on a signed RFC 3161 timestamp instead).
-    // Such bundles are parsed here; whether their absence is acceptable is a verification-time
-    // policy decision (see VerificationOptions#allowNonTransparencyLogVerification), not a parse
-    // error. Any tlog entries that ARE present must still be well-formed (inclusion proof
-    // required).
+    // A bundle may skip tlog entries in private environments that don't require them.
     for (var bundleEntry : protoBundle.getVerificationMaterial().getTlogEntriesList()) {
       if (!bundleEntry.hasInclusionProof()) {
         // all consumed bundles must have an inclusion proof
diff --git a/sigstore-java/src/main/java/dev/sigstore/fulcio/client/FulcioVerifier.java b/sigstore-java/src/main/java/dev/sigstore/fulcio/client/FulcioVerifier.java
@@ -16,6 +16,7 @@
 package dev.sigstore.fulcio.client;
 
 import com.google.common.annotations.VisibleForTesting;
+import dev.sigstore.VerificationOptions.CTLogOptions;
 import dev.sigstore.encryption.certificates.Certificates;
 import dev.sigstore.encryption.certificates.transparency.CTLogInfo;
 import dev.sigstore.encryption.certificates.transparency.CTVerificationResult;
@@ -44,7 +45,7 @@
 import java.util.Map;
 import java.util.stream.Collectors;
 
-/** Verifier for fulcio generated signing cerificates */
+/** Verifier for fulcio generated signing certificates */
 public class FulcioVerifier {
   private final List<CertificateAuthority> cas;
   private final List<TransparencyLog> ctLogs;
@@ -150,28 +151,17 @@ private void verifyEmbeddedScts(CertPath certPath) throws FulcioVerificationExce
    */
   public void verifySigningCertificate(CertPath signingCertificate)
       throws FulcioVerificationException, IOException {
-    verifySigningCertificate(signingCertificate, false);
+    verifySigningCertificate(signingCertificate, CTLogOptions.builder().isEnabled(true).build());
   }
 
   /**
-   * Verify a signing certificate, optionally tolerating a trust root that provides no certificate
-   * transparency logs.
-   *
-   * @param signingCertificate the chain to verify
-   * @param allowEmptyCtLogs when {@code true} and the configured trust root declares <em>no</em> CT
-   *     logs, SCT verification is skipped. This is required for Sigstore instances that do not use
-   *     certificate transparency (for example, GitHub's Sigstore instance for private repositories,
-   *     whose trust root carries zero CT logs and whose certificates embed no SCT). When {@code
-   *     false} (the default) a missing CT log is an error, preserving the strict behavior for the
-   *     public-good instance. If the trust root <em>does</em> provide CT logs, SCTs are always
-   *     verified regardless of this flag.
+   * Verify a signing certificate, applying the given certificate-transparency policy. When SCT
+   * verification is disabled (ex: a private deployment without CT logs) the SCT check is skipped.
    */
-  public void verifySigningCertificate(CertPath signingCertificate, boolean allowEmptyCtLogs)
+  public void verifySigningCertificate(CertPath signingCertificate, CTLogOptions ctLogOptions)
       throws FulcioVerificationException, IOException {
     CertPath fullCertPath = validateCertPath(signingCertificate);
-    if (ctLogs.isEmpty() && allowEmptyCtLogs) {
-      // The trust root declares no certificate-transparency logs, so there is nothing to verify an
-      // SCT against. Trusted time is established elsewhere (a signed RFC 3161 timestamp).
+    if (!ctLogOptions.isEnabled()) {
       return;
     }
     verifySct(fullCertPath);
@@ -200,7 +190,8 @@ CertPath validateCertPath(CertPath signingCertificate) throws FulcioVerification
     } catch (NoSuchAlgorithmException e) {
       //
       throw new RuntimeException(
-          "No PKIX CertPathValidator, we probably shouldn't be here, but this seems to be a system library error not a program control flow issue",
+          "No PKIX CertPathValidator, we probably shouldn't be here, but this seems to be a system"
+              + " library error not a program control flow issue",
           e);
     }
 
@@ -220,7 +211,8 @@ CertPath validateCertPath(CertPath signingCertificate) throws FulcioVerification
         pkixParams = new PKIXParameters(Collections.singleton(ca.asTrustAnchor()));
       } catch (InvalidAlgorithmParameterException | CertificateException e) {
         throw new RuntimeException(
-            "Can't create PKIX parameters for fulcioRoot. This should have been checked when generating a verifier instance",
+            "Can't create PKIX parameters for fulcioRoot. This should have been checked when"
+                + " generating a verifier instance",
             e);
       }
       pkixParams.setRevocationEnabled(false);
diff --git a/sigstore-java/src/test/java/dev/sigstore/KeylessVerifierTest.java b/sigstore-java/src/test/java/dev/sigstore/KeylessVerifierTest.java
