Merge pull request #520 from sigstore/validate-sct-fix

Fix validate SCTs when cert chain is just leaf

Author: loosebazooka

sigstore-java/src/main/java/dev/sigstore/KeylessSigner2.java

@@ -307,10 +307,9 @@ private void renewSigningCertificate()
                   tokenInfo.getIdToken(),
                   signer.sign(
                       tokenInfo.getSubjectAlternativeName().getBytes(StandardCharsets.UTF_8))));
-      fulcioVerifier.verifyCertChain(signingCert);
       // TODO: this signing workflow mandates SCTs, but fulcio itself doesn't, figure out a way to
       // allow that to be known
-      fulcioVerifier.verifySct(signingCert);
+      fulcioVerifier.verifySigningCertificate(signingCert);
       this.signingCert = signingCert;
       signingCertPemBytes = Certificates.toPemBytes(signingCert.getLeafCertificate());
     } finally {

sigstore-java/src/main/java/dev/sigstore/KeylessVerifier2.java

@@ -157,22 +157,14 @@ public void verify(byte[] artifactDigest, KeylessVerificationRequest request)
               + Hex.toHexString(request.getKeylessSignature().getDigest()));
     }
 
-    // verify the certificate chains up to a trusted root (fulcio)
+    // verify the certificate chains up to a trusted root (fulcio) and contains a valid SCT
     try {
-      fulcioVerifier.verifyCertChain(signingCert);
+      fulcioVerifier.verifySigningCertificate(signingCert);
     } catch (FulcioVerificationException | IOException ex) {
       throw new KeylessVerificationException(
           "Fulcio certificate was not valid: " + ex.getMessage(), ex);
     }
 
-    // make the sure a crt is signed by the certificate transparency log (embedded only)
-    try {
-      fulcioVerifier.verifySct(signingCert);
-    } catch (FulcioVerificationException ex) {
-      throw new KeylessVerificationException(
-          "Fulcio certificate SCT was not valid: " + ex.getMessage(), ex);
-    }
-
     // verify the certificate identity if options are present
     if (request.getVerificationOptions().getCertificateIdentities().size() > 0) {
       try {

sigstore-java/src/main/java/dev/sigstore/fulcio/client/FulcioVerifier2.java

@@ -15,6 +15,7 @@
  */
 package dev.sigstore.fulcio.client;
 
+import com.google.common.annotations.VisibleForTesting;
 import dev.sigstore.encryption.certificates.Certificates;
 import dev.sigstore.encryption.certificates.transparency.CTLogInfo;
 import dev.sigstore.encryption.certificates.transparency.CTVerificationResult;
@@ -25,11 +26,13 @@
 import java.io.IOException;
 import java.security.InvalidAlgorithmParameterException;
 import java.security.NoSuchAlgorithmException;
+import java.security.cert.CertPath;
 import java.security.cert.CertPathValidator;
 import java.security.cert.CertPathValidatorException;
 import java.security.cert.CertificateEncodingException;
 import java.security.cert.CertificateException;
 import java.security.cert.PKIXParameters;
+import java.security.cert.X509Certificate;
 import java.security.spec.InvalidKeySpecException;
 import java.time.Instant;
 import java.util.ArrayList;
@@ -86,14 +89,9 @@ private FulcioVerifier2(
     this.ctVerifier = ctVerifier;
   }
 
-  /**
-   * Verify that an SCT associated with a Singing Certificate is valid and signed by the configured
-   * CT-log public key.
-   *
-   * @param signingCertificate containing the SCT metadata to verify
-   * @throws FulcioVerificationException if verification fails for any reason
-   */
-  public void verifySct(SigningCertificate signingCertificate) throws FulcioVerificationException {
+  @VisibleForTesting
+  void verifySct(SigningCertificate signingCertificate, CertPath rebuiltCert)
+      throws FulcioVerificationException {
     if (ctLogs.size() == 0) {
       throw new FulcioVerificationException("No ct logs were provided to verifier");
     }
@@ -102,18 +100,17 @@ public void verifySct(SigningCertificate signingCertificate) throws FulcioVerifi
       throw new FulcioVerificationException(
           "Detached SCTs are not supported for validating certificates");
     } else if (signingCertificate.getEmbeddedSct().isPresent()) {
-      verifyEmbeddedScts(signingCertificate);
+      verifyEmbeddedScts(rebuiltCert);
     } else {
       throw new FulcioVerificationException("No valid SCTs were found during verification");
     }
   }
 
-  private void verifyEmbeddedScts(SigningCertificate signingCertificate)
-      throws FulcioVerificationException {
-    var certs = signingCertificate.getCertificates();
+  private void verifyEmbeddedScts(CertPath rebuiltCert) throws FulcioVerificationException {
+    @SuppressWarnings("unchecked")
+    var certs = (List<X509Certificate>) rebuiltCert.getCertificates();
     CTVerificationResult result;
     try {
-      // even though we're sending the whole chain, this method only checks SCTs on the leaf cert
       result = ctVerifier.verifySignedCertificateTimestamps(certs, null, null);
     } catch (CertificateEncodingException cee) {
       throw new FulcioVerificationException(
@@ -144,13 +141,25 @@ private void verifyEmbeddedScts(SigningCertificate signingCertificate)
 
   /**
    * Verify that a cert chain is valid and chains up to the trust anchor (fulcio public key)
-   * configured in this validator.
+   * configured in this validator. Also verify that the leaf certificate contains at least one valid
+   * SCT
    *
    * @param signingCertificate containing the certificate chain
    * @throws FulcioVerificationException if verification fails for any reason
    */
-  public void verifyCertChain(SigningCertificate signingCertificate)
+  public void verifySigningCertificate(SigningCertificate signingCertificate)
       throws FulcioVerificationException, IOException {
+    CertPath reconstructedCert = reconstructValidCertPath(signingCertificate);
+    verifySct(signingCertificate, reconstructedCert);
+  }
+
+  /**
+   * Find a valid cert path that chains back up to the trusted root certs and reconstruct a
+   * certificate path combining the provided un-trusted certs and a known set of trusted and
+   * intermediate certs.
+   */
+  CertPath reconstructValidCertPath(SigningCertificate signingCertificate)
+      throws FulcioVerificationException {
     CertPathValidator cpv;
     try {
       cpv = CertPathValidator.getInstance("PKIX");
@@ -171,8 +180,6 @@ public void verifyCertChain(SigningCertificate signingCertificate)
 
     Map<String, String> caVerificationFailure = new LinkedHashMap<>();
 
-    System.out.println(Certificates.toPemString(signingCertificate.getCertPath()));
-
     for (var ca : validCAs) {
       PKIXParameters pkixParams;
       try {
@@ -190,10 +197,12 @@ public void verifyCertChain(SigningCertificate signingCertificate)
           new Date(signingCertificate.getLeafCertificate().getNotBefore().getTime());
       pkixParams.setDate(dateInValidityPeriod);
 
+      CertPath rebuiltCert;
       try {
         // build a cert chain with the root-chain in question and the provided leaf
-        var rebuiltCert =
+        rebuiltCert =
             Certificates.appendCertPath(ca.getCertPath(), signingCertificate.getLeafCertificate());
+
         // a result is returned here, but we ignore it
         cpv.validate(rebuiltCert, pkixParams);
       } catch (CertPathValidatorException
@@ -203,8 +212,8 @@ public void verifyCertChain(SigningCertificate signingCertificate)
         // verification failed
         continue;
       }
+      return rebuiltCert;
       // verification passed so just end this method
-      return;
     }
     String errors =
         caVerificationFailure.entrySet().stream()

sigstore-java/src/test/java/dev/sigstore/fulcio/client/FulcioVerifier2Test.java

@@ -17,13 +17,15 @@
 
 import com.google.common.io.Resources;
 import com.google.protobuf.util.JsonFormat;
+import dev.sigstore.bundle.BundleFactory;
 import dev.sigstore.encryption.certificates.transparency.SerializationException;
 import dev.sigstore.proto.trustroot.v1.TrustedRoot;
 import dev.sigstore.trustroot.ImmutableLogId;
 import dev.sigstore.trustroot.ImmutableTransparencyLog;
 import dev.sigstore.trustroot.ImmutableTransparencyLogs;
 import dev.sigstore.trustroot.SigstoreTrustedRoot;
 import java.io.IOException;
+import java.io.StringReader;
 import java.nio.charset.StandardCharsets;
 import java.security.InvalidAlgorithmParameterException;
 import java.security.NoSuchAlgorithmException;
@@ -37,11 +39,8 @@
 public class FulcioVerifier2Test {
   private static String sctBase64;
   private static String certs;
-  private static String certs2;
-  private static byte[] fulcioRoot;
-  private static byte[] ctfePub;
-  private static byte[] badCtfePub;
   private static String certsWithEmbeddedSct;
+  private static String bundleFile;
 
   private static SigstoreTrustedRoot trustRoot;
 
@@ -56,23 +55,14 @@ public static void loadResources() throws IOException {
             Resources.getResource("dev/sigstore/samples/fulcio-response/valid/cert.pem"),
             StandardCharsets.UTF_8);
 
-    certs2 =
+    certsWithEmbeddedSct =
         Resources.toString(
-            Resources.getResource("dev/sigstore/samples/certs/cert-githuboidc.pem"),
+            Resources.getResource("dev/sigstore/samples/fulcio-response/valid/certWithSct.pem"),
             StandardCharsets.UTF_8);
 
-    fulcioRoot =
-        Resources.toByteArray(
-            Resources.getResource("dev/sigstore/samples/fulcio-response/valid/fulcio.crt.pem"));
-    ctfePub =
-        Resources.toByteArray(
-            Resources.getResource("dev/sigstore/samples/fulcio-response/valid/ctfe.pub"));
-    badCtfePub =
-        Resources.toByteArray(Resources.getResource("dev/sigstore/samples/keys/test-rsa.pub"));
-
-    certsWithEmbeddedSct =
+    bundleFile =
         Resources.toString(
-            Resources.getResource("dev/sigstore/samples/fulcio-response/valid/certWithSct.pem"),
+            Resources.getResource("dev/sigstore/samples/bundles/bundle-with-leaf-cert.sigstore"),
             StandardCharsets.UTF_8);
   }
 
@@ -95,16 +85,18 @@ public void detachedSctNotSupported() throws Exception {
     var signingCertificate = SigningCertificate.newSigningCertificate(certs, sctBase64);
     var ex =
         Assertions.assertThrows(
-            FulcioVerificationException.class, () -> fulcioVerifier.verifySct(signingCertificate));
+            FulcioVerificationException.class,
+            () -> fulcioVerifier.verifySct(signingCertificate, signingCertificate.getCertPath()));
     Assertions.assertEquals(
-        ex.getMessage(), "Detached SCTs are not supported for validating certificates");
+        "Detached SCTs are not supported for validating certificates", ex.getMessage());
   }
 
   @Test
   public void testVerifySct_nullCtLogKey()
       throws IOException, SerializationException, CertificateException, InvalidKeySpecException,
           NoSuchAlgorithmException, InvalidAlgorithmParameterException {
-    var signingCertificate = SigningCertificate.newSigningCertificate(certs, sctBase64);
+    var signingCertificate =
+        SigningCertificate.newSigningCertificate(certsWithEmbeddedSct, sctBase64);
     var fulcioVerifier =
         FulcioVerifier2.newFulcioVerifier(
             trustRoot.getCAs(),
@@ -113,7 +105,7 @@ public void testVerifySct_nullCtLogKey()
                 .build());
 
     try {
-      fulcioVerifier.verifySct(signingCertificate);
+      fulcioVerifier.verifySigningCertificate(signingCertificate);
       Assertions.fail();
     } catch (FulcioVerificationException fve) {
       Assertions.assertEquals("No ct logs were provided to verifier", fve.getMessage());
@@ -126,7 +118,7 @@ public void testVerifySct_noSct() throws Exception {
     var fulcioVerifier = FulcioVerifier2.newFulcioVerifier(trustRoot);
 
     try {
-      fulcioVerifier.verifySct(signingCertificate);
+      fulcioVerifier.verifySct(signingCertificate, signingCertificate.getCertPath());
       Assertions.fail();
     } catch (FulcioVerificationException fve) {
       Assertions.assertEquals("No valid SCTs were found during verification", fve.getMessage());
@@ -138,8 +130,16 @@ public void validSigningCertAndEmbeddedSct() throws Exception {
     var signingCertificate = SigningCertificate.newSigningCertificate(certsWithEmbeddedSct, null);
     var fulcioVerifier = FulcioVerifier2.newFulcioVerifier(trustRoot);
 
-    fulcioVerifier.verifyCertChain(signingCertificate);
-    fulcioVerifier.verifySct(signingCertificate);
+    fulcioVerifier.verifySigningCertificate(signingCertificate);
+  }
+
+  @Test
+  public void validBundle() throws Exception {
+    var bundle = BundleFactory.readBundle(new StringReader(bundleFile));
+    var fulcioVerifier = FulcioVerifier2.newFulcioVerifier(trustRoot);
+
+    Assertions.assertEquals(1, bundle.getCertPath().getCertificates().size());
+    fulcioVerifier.verifySigningCertificate(SigningCertificate.from(bundle.getCertPath()));
   }
 
   @Test
@@ -161,7 +161,8 @@ public void invalidEmbeddedSct() throws Exception {
 
     var fve =
         Assertions.assertThrows(
-            FulcioVerificationException.class, () -> fulcioVerifier.verifySct(signingCertificate));
+            FulcioVerificationException.class,
+            () -> fulcioVerifier.verifySct(signingCertificate, signingCertificate.getCertPath()));
     Assertions.assertEquals("No valid SCTs were found, all(1) SCTs were invalid", fve.getMessage());
   }
 }