From 6d82e022d80f2b2b99dbb101f4f01acbae61ce60 Mon Sep 17 00:00:00 2001
From: Appu Goundan <appu@google.com>
Date: Thu, 14 Nov 2024 12:17:46 -0500
Subject: [PATCH] Make updater use tuf verifiers

- removes prod data tests as those were breaking due to old key encodings that
  we don't really want to support
- this fixes some tuf-conformance tests related to key types

Signed-off-by: Appu Goundan <appu@google.com>
---
 .../main/java/dev/sigstore/tuf/Updater.java   |  34 +++---
 .../java/dev/sigstore/tuf/UpdaterTest.java    | 110 +-----------------
 tuf-cli/tuf-cli.xfails                        |  18 ---
 3 files changed, 23 insertions(+), 139 deletions(-)

diff --git a/sigstore-java/src/main/java/dev/sigstore/tuf/Updater.java b/sigstore-java/src/main/java/dev/sigstore/tuf/Updater.java
index 071a8f39..fb4c3e77 100644
--- a/sigstore-java/src/main/java/dev/sigstore/tuf/Updater.java
+++ b/sigstore-java/src/main/java/dev/sigstore/tuf/Updater.java
@@ -19,15 +19,15 @@
 
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.hash.Hashing;
-import dev.sigstore.encryption.Keys;
-import dev.sigstore.encryption.signers.Verifiers;
+import dev.sigstore.tuf.encryption.Verifiers;
 import dev.sigstore.tuf.model.*;
 import dev.sigstore.tuf.model.TargetMeta.TargetData;
+import dev.sigstore.tuf.model.Targets;
+import dev.sigstore.tuf.model.Timestamp;
+import dev.sigstore.tuf.model.TufMeta;
 import java.io.IOException;
-import java.nio.charset.StandardCharsets;
 import java.security.InvalidKeyException;
 import java.security.NoSuchAlgorithmException;
-import java.security.PublicKey;
 import java.security.SignatureException;
 import java.security.spec.InvalidKeySpecException;
 import java.time.Clock;
@@ -247,24 +247,23 @@ void verifyDelegate(
         // look for the public key that matches the key ID and use it for verification.
         var key = publicKeys.get(signature.getKeyId());
         if (key != null) {
-          String publicKeyContents = key.getKeyVal().get("public");
-          PublicKey pubKey;
-          // TUF root version 4 and less is raw hex encoded key while 5+ is PEM.
-          // TODO(patrick@chainguard.dev): remove hex handling code once we upgrade the trusted root
-          // to v5.
-          if (publicKeyContents.startsWith("-----BEGIN PUBLIC KEY-----")) {
-            pubKey = Keys.parsePublicKey(publicKeyContents.getBytes(StandardCharsets.UTF_8));
-          } else {
-            pubKey = Keys.constructTufPublicKey(Hex.decode(publicKeyContents), key.getScheme());
-          }
           try {
             // while we error on keys that are not readable, we are intentionally more permissive
             // about signatures. If for ANY reason (except unparsed keys) we cannot validate a
             // signature, we continue as long as we find enough valid signatures within the
             // threshold. We still warn the user as this could be an indicator of data issues
             byte[] signatureBytes = Hex.decode(signature.getSignature());
-            if (verifiers.newVerifier(pubKey).verify(verificationMaterial, signatureBytes)) {
+            if (verifiers.newVerifier(key).verify(verificationMaterial, signatureBytes)) {
               goodSigs.add(signature.getKeyId());
+            } else {
+              log.log(
+                  Level.FINE,
+                  () ->
+                      String.format(
+                          Locale.ROOT,
+                          "TUF: ignored failed signature verification: '%s' for keyid: '%s'",
+                          signature.getSignature(),
+                          signature.getKeyId()));
             }
           } catch (SignatureException e) {
             log.log(
@@ -272,9 +271,10 @@ void verifyDelegate(
                 () ->
                     String.format(
                         Locale.ROOT,
-                        "TUF: ignored unverifiable signature: '%s' for keyid: '%s'",
+                        "TUF: ignored unverifiable signature: '%s' for keyid: '%s', because '%s'",
                         signature.getSignature(),
-                        signature.getKeyId()));
+                        signature.getKeyId(),
+                        e.getMessage()));
           } catch (DecoderException | NoSuchAlgorithmException | InvalidKeyException e) {
             log.log(
                 Level.WARNING,
diff --git a/sigstore-java/src/test/java/dev/sigstore/tuf/UpdaterTest.java b/sigstore-java/src/test/java/dev/sigstore/tuf/UpdaterTest.java
index 841e25c1..8bdeb7e9 100644
--- a/sigstore-java/src/test/java/dev/sigstore/tuf/UpdaterTest.java
+++ b/sigstore-java/src/test/java/dev/sigstore/tuf/UpdaterTest.java
@@ -16,7 +16,6 @@
 package dev.sigstore.tuf;
 
 import static dev.sigstore.json.GsonSupplier.GSON;
-import static dev.sigstore.testkit.tuf.TestResources.UPDATER_REAL_TRUSTED_ROOT;
 import static dev.sigstore.testkit.tuf.TestResources.UPDATER_SYNTHETIC_TRUSTED_ROOT;
 import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
 import static org.junit.jupiter.api.Assertions.assertEquals;
@@ -27,12 +26,11 @@
 
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
-import com.google.common.hash.Hashing;
 import com.google.common.io.Resources;
 import com.google.gson.JsonSyntaxException;
-import dev.sigstore.encryption.signers.Verifier;
-import dev.sigstore.encryption.signers.Verifiers;
 import dev.sigstore.testkit.tuf.TestResources;
+import dev.sigstore.tuf.encryption.Verifier;
+import dev.sigstore.tuf.encryption.Verifiers;
 import dev.sigstore.tuf.model.Hashes;
 import dev.sigstore.tuf.model.ImmutableKey;
 import dev.sigstore.tuf.model.ImmutableRootRole;
@@ -41,7 +39,6 @@
 import dev.sigstore.tuf.model.Role;
 import dev.sigstore.tuf.model.Root;
 import dev.sigstore.tuf.model.Signature;
-import dev.sigstore.tuf.model.TargetMeta;
 import dev.sigstore.tuf.model.Targets;
 import io.github.netmikey.logunit.api.LogCapturer;
 import java.io.File;
@@ -52,8 +49,6 @@
 import java.nio.file.Path;
 import java.security.InvalidKeyException;
 import java.security.NoSuchAlgorithmException;
-import java.security.PublicKey;
-import java.security.SignatureException;
 import java.security.spec.InvalidKeySpecException;
 import java.time.Clock;
 import java.time.Instant;
@@ -123,19 +118,6 @@ static void startRemoteResourceServer() throws Exception {
     System.out.println("TUF local server listening on: " + remoteUrl);
   }
 
-  @Test
-  public void testRootUpdate_fromProdData() throws Exception {
-    setupMirror(
-        "real/prod", "1.root.json", "2.root.json", "3.root.json", "4.root.json", "5.root.json");
-    var updater = createTimeStaticUpdater(localStorePath, UPDATER_REAL_TRUSTED_ROOT);
-    updater.updateRoot();
-    assertStoreContains("root.json");
-    Root oldRoot = TestResources.loadRoot(UPDATER_REAL_TRUSTED_ROOT);
-    Root newRoot = TestResources.loadRoot(localStorePath.resolve("root.json"));
-    assertRootVersionIncreased(oldRoot, newRoot);
-    assertRootNotExpired(newRoot);
-  }
-
   @Test
   public void testRootUpdate_notEnoughSignatures()
       throws IOException, NoSuchAlgorithmException, InvalidKeySpecException, InvalidKeyException {
@@ -603,50 +585,6 @@ public void testTargetsDownload_sha256Only() throws Exception {
     assertDoesNotThrow(updater::update);
   }
 
-  // End to end sanity test on the actual prod sigstore repo.
-  @Test
-  public void testUpdate_fromProdData()
-      throws IOException, NoSuchAlgorithmException, InvalidKeySpecException, InvalidKeyException {
-    setupMirror(
-        "real/prod",
-        "1.root.json",
-        "2.root.json",
-        "3.root.json",
-        "4.root.json",
-        "5.root.json",
-        "69.snapshot.json",
-        "5.targets.json",
-        "timestamp.json",
-        "snapshot.json",
-        "targets.json",
-        "root.json",
-        "targets/0ae7705e02db33e814329746a4a0e5603c5bdcd91c96d072158d71011a2695788866565a2fec0fe363eb72cbcaeda39e54c5fe8d416daf9f3101fdba4217ef35.rekor.pub",
-        "targets/0f99f47dbc26c5f1e3cba0bfd9af4245a26e5cb735d6ef005792ec7e603f66fdb897de985973a6e50940ca7eff5e1849719e967b5ad2dac74a29115a41cf6f21.fulcio_intermediate_v1.crt.pem",
-        "targets/4b20747d1afe2544238ad38cc0cc3010921b177d60ac743767e0ef675b915489bd01a36606c0ff83c06448622d7160f0d866c83d20f0c0f44653dcc3f9aa0bd4.ctfe.pub",
-        "targets/308fd1d1d95d7f80aa33b837795251cc3e886792982275e062409e13e4e236ffc34d676682aa96fdc751414de99c864bf132dde71581fa651c6343905e3bf988.artifact.pub",
-        "targets/0713252a7fd17f7f3ab12f88a64accf2eb14b8ad40ca711d7fe8b4ecba3b24db9e9dffadb997b196d3867b8f9ff217faf930d80e4dab4e235c7fc3f07be69224.fulcio.crt.pem",
-        "targets/e83fa4f427b24ee7728637fad1b4aa45ebde2ba02751fa860694b1bb16059a490328f9985e51cc70e4d237545315a1bc866dc4fdeef2f6248d99cc7a6077bf85.ctfe_2022.pub",
-        "targets/f2e33a6dc208cee1f51d33bbea675ab0f0ced269617497985f9a0680689ee7073e4b6f8fef64c91bda590d30c129b3070dddce824c05bc165ac9802f0705cab6.fulcio_v1.crt.pem");
-    var updater = createTimeStaticUpdater(localStorePath, UPDATER_REAL_TRUSTED_ROOT);
-    updater.update();
-
-    Root oldRoot = TestResources.loadRoot(UPDATER_REAL_TRUSTED_ROOT);
-    TrustedMetaStore metaStore = updater.getMetaStore();
-    TargetStore targetStore = updater.getTargetStore();
-    Root newRoot = metaStore.getRoot(); // should be present
-    assertRootVersionIncreased(oldRoot, newRoot);
-    Targets targets = metaStore.getTargets(); // should be present
-    Map<String, TargetMeta.TargetData> targetsData = targets.getSignedMeta().getTargets();
-    for (String file : targetsData.keySet()) {
-      TargetMeta.TargetData fileData = targetsData.get(file);
-      byte[] fileBytes = targetStore.readTarget(file);
-      assertNotNull(fileBytes, "each file from targets data should be present");
-      assertEquals(fileData.getLength(), fileBytes.length, "file length should match metadata");
-      assertEquals(
-          fileData.getHashes().getSha512(), Hashing.sha512().hashBytes(fileBytes).toString());
-    }
-  }
-
   private static final byte[] TEST_HASH_VERIFYIER_BYTES =
       "testdata".getBytes(StandardCharsets.UTF_8);
   private static final String GOOD_256_HASH =
@@ -941,8 +879,8 @@ public void testUpdate_snapshotsAndTimestampHaveNoSizeAndNoHashesInMeta() throws
 
   @Test
   public void canCreateMultipleUpdaters() throws IOException {
-    createTimeStaticUpdater(localStorePath, UPDATER_REAL_TRUSTED_ROOT);
-    createTimeStaticUpdater(localStorePath, UPDATER_REAL_TRUSTED_ROOT);
+    createTimeStaticUpdater(localStorePath, UPDATER_SYNTHETIC_TRUSTED_ROOT);
+    createTimeStaticUpdater(localStorePath, UPDATER_SYNTHETIC_TRUSTED_ROOT);
   }
 
   static Key newKey(String keyContents) {
@@ -1027,43 +965,7 @@ static void shutdownRemoteResourceServer() throws Exception {
   }
 
   public static final Verifiers.Supplier ALWAYS_VERIFIES =
-      publicKey ->
-          new Verifier() {
-            @Override
-            public PublicKey getPublicKey() {
-              return null;
-            }
-
-            @Override
-            public boolean verify(byte[] artifact, byte[] signature)
-                throws NoSuchAlgorithmException, InvalidKeyException, SignatureException {
-              return true;
-            }
-
-            @Override
-            public boolean verifyDigest(byte[] artifactDigest, byte[] signature)
-                throws NoSuchAlgorithmException, InvalidKeyException, SignatureException {
-              return true;
-            }
-          };
+      (key) -> (Verifier) (artifactDigest, signature) -> true;
   public static final Verifiers.Supplier ALWAYS_FAILS =
-      publicKey ->
-          new Verifier() {
-            @Override
-            public PublicKey getPublicKey() {
-              return null;
-            }
-
-            @Override
-            public boolean verify(byte[] artifact, byte[] signature)
-                throws NoSuchAlgorithmException, InvalidKeyException, SignatureException {
-              return false;
-            }
-
-            @Override
-            public boolean verifyDigest(byte[] artifactDigest, byte[] signature)
-                throws NoSuchAlgorithmException, InvalidKeyException, SignatureException {
-              return false;
-            }
-          };
+      (key) -> (Verifier) (artifactDigest, signature) -> false;
 }
diff --git a/tuf-cli/tuf-cli.xfails b/tuf-cli/tuf-cli.xfails
index c3298724..e22c464f 100644
--- a/tuf-cli/tuf-cli.xfails
+++ b/tuf-cli/tuf-cli.xfails
@@ -1,8 +1,6 @@
 test_metadata_bytes_match
 test_client_downloads_expected_file_in_sub_dir
 test_duplicate_sig_keyids
-test_keytype_and_scheme[rsa/rsassa-pss-sha256]
-test_keytype_and_scheme[ed25519/ed25519]
 test_unusual_role_name[?]
 test_unusual_role_name[#]
 test_unusual_role_name[/delegatedrole]
@@ -26,20 +24,4 @@ test_targetfile_search[targetpath matches wildcard]
 test_targetfile_search[targetpath with separators x]
 test_targetfile_search[targetpath with separators y]
 test_targetfile_search[targetpath is not delegated by all roles in the chain]
-test_root_rotation[1-of-1-key-rotation]
-test_root_rotation[1-of-1-key-rotation-unused-signatures]
-test_root_rotation[3-of-5-sign-with-different-keycombos]
-test_root_rotation[3-of-5-one-key-rotated]
-test_root_rotation[3-of-5-one-key-rotated-with-intermediate-step]
-test_root_rotation[3-of-5-all-keys-rotated-with-intermediate-step]
-test_root_rotation[1-of-3-threshold-increase-to-2-of-3]
-test_root_rotation[2-of-3-threshold-decrease-to-1-of-3]
-test_root_rotation[1-of-2-threshold-increase-to-2-of-2]
-test_non_root_rotations[1-of-1-key-rotation]
-test_non_root_rotations[1-of-1-key-rotation-unused-signatures]
-test_non_root_rotations[3-of-5-sign-first-combo]
-test_non_root_rotations[3-of-5-sign-second-combo]
-test_non_root_rotations[3-of-5-sign-third-combo]
-test_non_root_rotations[3-of-5-sign-fourth-combo]
-test_non_root_rotations[3-of-5-sign-fifth-combo]
 test_snapshot_rollback[with hashes]