Be more permissive for signatures in TUF metadata

Don't fail if a signature doesn't validate, just move on to the next and as long as we meet the threshold we are fine Signed-off-by: Appu Goundan <[email protected]>
sigstore · Feb 12, 2024 · 033f476 · 033f476
1 parent e4f9359
commit 033f476
Show file tree

Hide file tree

Showing 7 changed files with 337 additions and 8 deletions.
diff --git a/sigstore-java/build.gradle.kts b/sigstore-java/build.gradle.kts
@@ -57,6 +57,9 @@ dependencies {
     testImplementation("com.squareup.okhttp3:mockwebserver:4.12.0")
     testImplementation("net.sourceforge.htmlunit:htmlunit:2.70.0")
     testImplementation("org.eclipse.jetty:jetty-server:11.0.19")
+
+    testImplementation("io.github.netmikey.logunit:logunit-core:2.0.0")
+    testRuntimeOnly("io.github.netmikey.logunit:logunit-jul:2.0.0")
 }
 
 protobuf {

diff --git a/sigstore-java/src/main/java/dev/sigstore/tuf/Updater.java b/sigstore-java/src/main/java/dev/sigstore/tuf/Updater.java
@@ -37,6 +37,9 @@
 import java.util.Locale;
 import java.util.Map;
 import java.util.Optional;
+import java.util.logging.Level;
+import java.util.logging.Logger;
+import org.bouncycastle.util.encoders.DecoderException;
 import org.bouncycastle.util.encoders.Hex;
 
 /**
@@ -52,6 +55,8 @@ public class Updater {
   // spec.
   private static final int MAX_UPDATES = 1024;
 
+  private static final Logger log = Logger.getLogger(Updater.class.getName());
+
   private Clock clock;
   private Verifiers.Supplier verifiers;
   private MetaFetcher fetcher;
@@ -197,8 +202,7 @@ void verifyDelegate(
       Map<String, Key> publicKeys,
       Role role,
       byte[] verificationMaterial)
-      throws SignatureVerificationException, NoSuchAlgorithmException, InvalidKeyException,
-          InvalidKeySpecException, IOException {
+      throws InvalidKeySpecException, IOException, NoSuchAlgorithmException {
     // use set to not count the same key multiple times towards the threshold.
     var goodSigs = new HashSet<>(role.getKeyids().size() * 4 / 3);
     // role.getKeyIds() defines the keys allowed to sign for this role.
@@ -221,13 +225,20 @@ void verifyDelegate(
           } else {
             pubKey = Keys.constructTufPublicKey(Hex.decode(publicKeyContents), key.getScheme());
           }
-          byte[] signatureBytes = Hex.decode(signature.getSignature());
           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
+            byte[] signatureBytes = Hex.decode(signature.getSignature());
             if (verifiers.newVerifier(pubKey).verify(verificationMaterial, signatureBytes)) {
               goodSigs.add(signature.getKeyId());
             }
-          } catch (SignatureException e) {
-            throw new TufException(e);
+          } catch (SignatureException
+              | DecoderException
+              | NoSuchAlgorithmException
+              | InvalidKeyException e) {
+            log.log(Level.FINE, e, () -> String.format("TUF: ignored invalid signature: '%s' for keyid: '%s'", signature.getSignature(), keyid));
           }
         }
       }

diff --git a/sigstore-java/src/test/java/dev/sigstore/tuf/UpdaterTest.java b/sigstore-java/src/test/java/dev/sigstore/tuf/UpdaterTest.java
@@ -17,7 +17,11 @@
 
 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.*;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.junit.jupiter.api.Assertions.fail;
 
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
@@ -27,8 +31,19 @@
 import dev.sigstore.encryption.signers.Verifier;
 import dev.sigstore.encryption.signers.Verifiers;
 import dev.sigstore.testkit.tuf.TestResources;
-import dev.sigstore.tuf.model.*;
+import dev.sigstore.tuf.model.Hashes;
+import dev.sigstore.tuf.model.ImmutableKey;
+import dev.sigstore.tuf.model.ImmutableRootRole;
+import dev.sigstore.tuf.model.ImmutableSignature;
+import dev.sigstore.tuf.model.Key;
+import dev.sigstore.tuf.model.Role;
 import dev.sigstore.tuf.model.Root;
+import dev.sigstore.tuf.model.Signature;
+import dev.sigstore.tuf.model.Snapshot;
+import dev.sigstore.tuf.model.TargetMeta;
+import dev.sigstore.tuf.model.Targets;
+import dev.sigstore.tuf.model.Timestamp;
+import io.github.netmikey.logunit.api.LogCapturer;
 import java.io.File;
 import java.io.IOException;
 import java.net.URL;
@@ -57,8 +72,13 @@
 import org.eclipse.jetty.util.resource.Resource;
 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
-import org.junit.jupiter.api.*;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.RegisterExtension;
 import org.junit.jupiter.api.io.TempDir;
+import org.slf4j.event.Level;
 
 class UpdaterTest {
 
@@ -69,6 +89,9 @@ class UpdaterTest {
   @TempDir Path localStorePath;
   @TempDir static Path localMirrorPath;
 
+  @RegisterExtension
+  LogCapturer logs = LogCapturer.create().captureForType(Updater.class, Level.DEBUG);
+
   @BeforeAll
   static void startRemoteResourceServer() throws Exception {
     remote = new Server();
@@ -142,6 +165,30 @@ public void testRootUpdate_newRootHasUnknownFields() throws Exception {
     assertEquals(5, root.getSignedMeta().getVersion());
   }
 
+  @Test
+  public void testRootUpdate_newRootHasEmptySignatures() throws Exception {
+    setupMirror("synthetic/root-update-with-empty-signature", "2.root.json");
+    var updater = createTimeStaticUpdater(localStorePath, UPDATER_SYNTHETIC_TRUSTED_ROOT);
+
+    updater.updateRoot();
+    Root root = TestResources.loadRoot(localStorePath.resolve("root.json"));
+    assertEquals(2, root.getSignedMeta().getVersion());
+    logs.assertContains(
+        "TUF: ignored invalid signature: '' for keyid: '0b5108e406f6d2f59ef767797b314be99d35903950ba43a2d51216eeeb8da98c'");
+  }
+
+  @Test
+  public void testRootUpdate_newRootHasInvalidSignatures() throws Exception {
+    setupMirror("synthetic/root-update-with-invalid-signature", "2.root.json");
+    var updater = createTimeStaticUpdater(localStorePath, UPDATER_SYNTHETIC_TRUSTED_ROOT);
+
+    updater.updateRoot();
+    Root root = TestResources.loadRoot(localStorePath.resolve("root.json"));
+    assertEquals(2, root.getSignedMeta().getVersion());
+    logs.assertContains(
+        "TUF: ignored invalid signature: 'abcd123' for keyid: '0b5108e406f6d2f59ef767797b314be99d35903950ba43a2d51216eeeb8da98c'");
+  }
+
   @Test
   public void testRootUpdate_expiredRoot()
       throws IOException, NoSuchAlgorithmException, InvalidKeySpecException, InvalidKeyException {

diff --git a/...rc/test/resources/dev/sigstore/tuf/synthetic/root-update-with-empty-signature/2.root.json b/...rc/test/resources/dev/sigstore/tuf/synthetic/root-update-with-empty-signature/2.root.json
@@ -0,0 +1,119 @@
+{
+  "signed": {
+    "_type": "root",
+    "spec_version": "1.0",
+    "version": 2,
+    "expires": "2023-05-13T14:35:58Z",
+    "keys": {
+      "0b5108e406f6d2f59ef767797b314be99d35903950ba43a2d51216eeeb8da98c": {
+        "keytype": "ecdsa-sha2-nistp256",
+        "scheme": "ecdsa-sha2-nistp256",
+        "keyid_hash_algorithms": [
+          "sha256",
+          "sha512"
+        ],
+        "keyval": {
+          "public": "-----BEGIN PUBLIC KEY-----\nMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEKzH3HI+8f9hYlrwNynmWtYrdp7kT\n5B13ZcaQJd2gbMw3MXUwAMWksxAjNXXXselrztKQLKEJkj0CRPiXFhtdWg==\n-----END PUBLIC KEY-----\n"
+        }
+      },
+      "7aecf5f0720acfb4fa873896ba05a2d8914f5b6ca90d26ac8bc0f1e491378740": {
+        "keytype": "ecdsa-sha2-nistp256",
+        "scheme": "ecdsa-sha2-nistp256",
+        "keyid_hash_algorithms": [
+          "sha256",
+          "sha512"
+        ],
+        "keyval": {
+          "public": "-----BEGIN PUBLIC KEY-----\nMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEs1Stkp5CNyERUPWDa9KF47KjECsx\noobAYi8NUUh5+0Rl34nYR3Y/2IQWu8l2pi9f73Qqsq3kk1cGQMCKRJu1wA==\n-----END PUBLIC KEY-----\n"
+        }
+      },
+      "9354bd3deaa572ed06306ddfad457037918534ece677cf962526a6fd40112d7a": {
+        "keytype": "ecdsa-sha2-nistp256",
+        "scheme": "ecdsa-sha2-nistp256",
+        "keyid_hash_algorithms": [
+          "sha256",
+          "sha512"
+        ],
+        "keyval": {
+          "public": "-----BEGIN PUBLIC KEY-----\nMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEJsV+S1syZdtx5HjiFN5YqRAqD2By\n4R0xDtXptW+UJlJQdfQCGAHvqtpac0edkcWVREhktEqIMbCaYSd75E/JRA==\n-----END PUBLIC KEY-----\n"
+        }
+      },
+      "a041140325d05d8a7643d5649a8c4296f8e6b020fb73bf83c52319b1a7230a40": {
+        "keytype": "ecdsa-sha2-nistp256",
+        "scheme": "ecdsa-sha2-nistp256",
+        "keyid_hash_algorithms": [
+          "sha256",
+          "sha512"
+        ],
+        "keyval": {
+          "public": "-----BEGIN PUBLIC KEY-----\nMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEEoZaB1Hu8VvuqgHvwX1mAITts2Zi\ntHhs3suizfA/XDmetnA9BoXhPpLmPJ1n+47xr4Gdr5mcrBzLbM+WcXIs9Q==\n-----END PUBLIC KEY-----\n"
+        }
+      },
+      "a9c5c80b93210eeb34e6264b4b261ff6899d4dbfb8e308f8546722a2bae30687": {
+        "keytype": "ecdsa-sha2-nistp256",
+        "scheme": "ecdsa-sha2-nistp256",
+        "keyid_hash_algorithms": [
+          "sha256",
+          "sha512"
+        ],
+        "keyval": {
+          "public": "-----BEGIN PUBLIC KEY-----\nMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEbGNtqWi9Xu7romi12qG+fHYj4SCp\nUCKAOJxXKagVyQNlS6TdJCMHWOJ+0BReT1lQsw6J/SMtc9a5J6Vj7fksBw==\n-----END PUBLIC KEY-----\n"
+        }
+      },
+      "fca39ff47a3a91605f2c56501e84b4fe3b9a66b96a022275e866bd19353f93e6": {
+        "keytype": "ecdsa-sha2-nistp256",
+        "scheme": "ecdsa-sha2-nistp256",
+        "keyid_hash_algorithms": [
+          "sha256",
+          "sha512"
+        ],
+        "keyval": {
+          "public": "-----BEGIN PUBLIC KEY-----\nMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEfcbhZ0zElnB5dqJBzKiVlofRXBh/\n2snZw32WDcUvl3+7UEtRvmTGZSaAxYCGmAc1EO2j5MGk5wkNkuwiVesd0g==\n-----END PUBLIC KEY-----\n"
+        }
+      }
+    },
+    "roles": {
+      "root": {
+        "keyids": [
+          "fca39ff47a3a91605f2c56501e84b4fe3b9a66b96a022275e866bd19353f93e6",
+          "0b5108e406f6d2f59ef767797b314be99d35903950ba43a2d51216eeeb8da98c",
+          "a041140325d05d8a7643d5649a8c4296f8e6b020fb73bf83c52319b1a7230a40"
+        ],
+        "threshold": 2
+      },
+      "snapshot": {
+        "keyids": [
+          "9354bd3deaa572ed06306ddfad457037918534ece677cf962526a6fd40112d7a"
+        ],
+        "threshold": 1
+      },
+      "targets": {
+        "keyids": [
+          "a9c5c80b93210eeb34e6264b4b261ff6899d4dbfb8e308f8546722a2bae30687"
+        ],
+        "threshold": 1
+      },
+      "timestamp": {
+        "keyids": [
+          "7aecf5f0720acfb4fa873896ba05a2d8914f5b6ca90d26ac8bc0f1e491378740"
+        ],
+        "threshold": 1
+      }
+    },
+    "consistent_snapshot": true
+  },
+  "signatures": [
+    {
+      "keyid": "0b5108e406f6d2f59ef767797b314be99d35903950ba43a2d51216eeeb8da98c",
+      "sig": ""
+    },
+    {
+      "keyid": "a041140325d05d8a7643d5649a8c4296f8e6b020fb73bf83c52319b1a7230a40",
+      "sig": "3046022100845e6b95ccf906b7c44e5993384ecca0efefb0ce9495e9d125856ef4640c5906022100fc4ae0c7f5d32dcccb76b87240f8795d176b10497cced966aac4b8e3db71d0fa"
+    },
+    {
+      "keyid": "fca39ff47a3a91605f2c56501e84b4fe3b9a66b96a022275e866bd19353f93e6",
+      "sig": "3045022024637aad4a82ec9416527d2bd54255c56b86ff0c1a8a316d0282ce8f0e18d797022100f51cffa088083bc3c76fe0a26746b99bf49a3b19c4692a12133872a477b6f226"
+    }
+  ]
+}
diff --git a/...resources/dev/sigstore/tuf/synthetic/root-update-with-empty-signature/README.md b/...resources/dev/sigstore/tuf/synthetic/root-update-with-empty-signature/README.md
@@ -0,0 +1,15 @@
+# Setup test data
+
+```shell
+cp ../test-template/2.root.json 2.root.json
+```
+
+edit the values of signatures so they are wrong, but still match the threshold
+```diff
+  "signatures": [
+    {
+      "keyid": "0b5108e406f6d2f59ef767797b314be99d35903950ba43a2d51216eeeb8da98c",
++      "sig": ""
+-      "sig": "304502204ee7d150bbbf40dc641d1a208be4708be14022da6a86883d2c5a7282eda2659802210095a15450c1e63ff20bd5164979007fbea8a7deea68ebba7a67f8cd2901b686ca"
+    },
+```