Add TUF Client root resource syncing. #123

patflynn · 2022-09-09T18:51:59Z

WIP towards #60. I will update that issue with next steps as there are a few.

This PR is intentionally incomplete. I want to land code incrementally on this one as it's dense and it will benefit from incremental review.

The code is kept as straight forward as possible to help review the implementation meets spec. I intend to start factoring things out as I implement the rest of the meta and target updating.

There is a coming change in the spec to make the signing public keys PEM encoded as they were supposed to be. This code does not handle that change yet. I will add a test case and modify the certification handling in a subsequent PR.

Links to spec for review are included in the code.

Release Note

does not affect the current library behavior.

Documentation

no doc updates yet.

patflynn · 2022-09-09T18:53:32Z

working on rebase and commit signing.

loosebazooka

Sorry I put a change in here that requires you to rebase again.

I think the failure is that we only optionally include the BC provider if "java.version < 15", and really only the java 17 tests are failing. I think this library just needs to add the provider in always for this TUF stuff and we can get rid of the conditional static initializer in the key parser?

loosebazooka · 2022-09-11T03:27:56Z

sigstore-java/src/main/java/dev/sigstore/tuf/TufClient.java

+
+  private static final int MAX_META_BYTES = 99 * 1024; // 99 KB
+  private static final int MAX_UPDATES =
+      1024; // Limit the update loop to retrieve a max of 1024 subsequent versions.


any reason why?

added mention to spec.

loosebazooka · 2022-09-11T03:28:48Z

sigstore-java/src/main/java/dev/sigstore/tuf/TufClient.java

+    // 5.3.10) Check expiration timestamp in trusted root is higher than fixed update start time. if
+    // not, throw an error.
+    ZonedDateTime expires = trustedRoot.getSignedMeta().getExpiresAsDate();
+    // If the


incomplete comment?

loosebazooka · 2022-09-11T03:34:29Z

sigstore-java/src/main/java/dev/sigstore/tuf/model/TufMeta.java

-  LocalDateTime getExpires();
+  String getExpires();
+
+  default ZonedDateTime getExpiresAsDate() {


can this be a derived value?

you can't map two fields to the same json element (I tried). Is that what you were suggestion?

A derived value is an immutables construct that calculates this once instead of invoking every time: https://immutables.github.io/immutable.html#derived-attributes

hmm. I like the idea! I'm not super psyched about changing this to an abstract class though as it blows up the whole interface hierarchy, we'd have to change all the sub types to abstract as well. WDYT?

loosebazooka · 2022-09-11T03:36:08Z

sigstore-java/src/main/resources/dev/sigstore/tuf/sigstore-tuf-root/1.root.json

@@ -1,130 +1,120 @@
 {


asra was saying something about just skipping the first few roots -- verifying the chain manually and only including N.root.json if you don't want to handle the timezone special cases

Yeah that was an option, but it seemed straight forward to make the client resilient to the different formats. 🤷

Signed-off-by: Patrick Flynn <patrick@chainguard.dev>

loosebazooka

looks good, some test resources have no newlines at EOF, is that intentional?

loosebazooka · 2022-09-12T13:51:43Z

sigstore-java/src/main/java/dev/sigstore/encryption/Keys.java

+          // spec for P-256 curve
+          ECNamedCurveParameterSpec spec = ECNamedCurveTable.getParameterSpec("P-256");
+          // create a KeyFactory with ECDSA (Elliptic Curve Diffie-Hellman) algorithm and use
+          // BouncyCastle


nit: spotless seems to have formatted this funny.

fixed. sorry I thought I had caught all these.

loosebazooka · 2022-09-12T13:52:56Z

sigstore-java/src/main/java/dev/sigstore/tuf/TufClient.java

+    // 5.3.1) record the time at start and use for expiration checks consistently throughout the
+    // update.
+    updateStartTime = ZonedDateTime.now(clock);
+    Root trustedRoot;


nit: does trusted root really need to be defined here if you're just assigning it on the next line? (was there previously some try catch?)

good point. fixed.

loosebazooka · 2022-09-12T13:59:10Z

sigstore-java/src/main/java/dev/sigstore/tuf/TufClient.java

+
+      // 5.3.5) We've taken the liberty to modify 5.3.5 to just validate that the new root meta
+      // matches the version we pulled based off of the pattern {version}.root.json. We know due to
+      // the


nit: goofy comment formatting again

spotless gets me again!

loosebazooka · 2022-09-12T13:59:51Z

sigstore-java/src/main/java/dev/sigstore/tuf/TufClient.java

+      // 5.3.8) persist to repo
+      Path localTrustRoot = localStore.resolve("root.json");
+      if (localTrustRoot.toFile().exists()) {
+        // Backup the old root. (not sure if this is necessary)


should we add an TODO:(#issue) to investigate this?

Backing up the old root chain isn't strictly necessary, just the latest verified one as root.json (as done here) is valid.

sigstore-java/src/main/java/dev/sigstore/tuf/TufClient.java

loosebazooka · 2022-09-12T14:02:59Z

sigstore-java/src/main/java/dev/sigstore/tuf/TufClient.java

+        continue;
+      }
+
+      // key bytes are in Hex not Base64! TUF also lies that their key is PEM Encoded. Don't believe


lol I think @asraa said this is going to change in root 5 or 6? Should we factor that in here?

Haha yes! TUF specification is PEM-encoding, but sigstore's root is currently hex. Will be fixed in v5:
sigstore/root-signing#372

I have a TODO in #60 to fix this :). I didn't want to overly complicate an already big PR.

loosebazooka · 2022-09-12T14:05:56Z

sigstore-java/src/main/java/dev/sigstore/tuf/model/TufMeta.java

-  LocalDateTime getExpires();
+  String getExpires();
+
+  default ZonedDateTime getExpiresAsDate() {


A derived value is an immutables construct that calculates this once instead of invoking every time: https://immutables.github.io/immutable.html#derived-attributes

loosebazooka · 2022-09-12T14:06:46Z

sigstore-java/src/test/java/dev/sigstore/tuf/TufClientTest.java

+
+  @AfterEach
+  void clearLocalMirror() {
+    for (File file : localMirror.toFile().listFiles()) {


I thought @TempDir did cleanup itself?

I think it's test suite scoped? Doesn't seem to clean itself between tests.

Oh I see it's static and you only want to start the remote resource server once?

yeah exactly.

asraa · 2022-09-12T14:18:56Z

looks good, some test resources have no newlines at EOF, is that intentional?

TBH this was intentional: the protocol will check the length (and hash) of the downloaded files against the TUF metadata expected length and hashes (https://theupdateframework.github.io/specification/latest/#metapath-length) and that will change with the newline

asraa · 2022-09-12T14:29:05Z

sigstore-java/src/main/java/dev/sigstore/tuf/TufClient.java

+    // use set to not count the same key multiple times towards the threshold.
+    var goodSigs = new HashSet<>(signatures.size());
+    for (Signature signature : signatures) {
+      var key = rootKeys.get(signature.getKeyId());


shouldn't you need to check that this key ID is also signed off on that particular role?

e.g. we don't want to consider the timestmap key signing the snapshot role as valid, even though they are both key IDs in the root.

probably the code here should change:

var newRootKeys = newRoot.getSignedMeta().getKeys(ROLE); // Verify our new root meta against the new root keys. verifyDelegate( newRootSignatures, newRootRoleMeta.getThreshold(), newRootKeys, newRootMetaBytes);

asraa · 2022-09-12T14:29:30Z

sigstore-java/src/main/java/dev/sigstore/tuf/TufClient.java

+  private void verifyDelegate(
+      List<Signature> signatures,
+      int threshold,
+      Map<String, Key> rootKeys,


note that the role may not be the root role here. it may be the snapshot role.
nit: rename to trustedKeys?

Oh damn! Thank you for noticing this! I significantly refactored the verification method as a result. PTAL. Currently working on another PR with more extensive unit testing for the verification method.

ugh. this response was to your previous comment about making sure the keys were in the role keys. My refactoring also addresses this comment.

asraa

This looks great so far!

Signed-off-by: Patrick Flynn <patrick@chainguard.dev>

patflynn · 2022-09-13T12:15:42Z

this is ready for another look.

asraa

verification logic lgtm!

asraa · 2022-09-13T15:24:41Z

sigstore-java/src/main/java/dev/sigstore/tuf/TufClient.java

+          // key bytes are in Hex not Base64!
+          // TODO(patrick): this will change in a subsequent version. Add code to handle PEM Encoded
+          // keys as well.
+          byte[] keyBytes = Hex.decode(key.getKeyVal().get("public"));


minor comment, and this is style, so feel free to ignore me, i'm only here for logic :)

Why not put the logic for decoding the keybytes into constructTufPublicKey? (as in constructTufPublicKey takes in the whole Key?) that way swapping out the decoding/parsing of the keyval can be contained in the Keys package, since it will change later.

It's a good comment! I left it that way because I have an eye to refactoring the Keys tool to merge the two Keys public key methods once we have PEM encoded keys here.

loosebazooka · 2022-09-13T12:29:28Z

sigstore-java/src/main/java/dev/sigstore/tuf/TufClient.java

+        var key = publicKeys.get(signature.getKeyId());
+        if (key != null) {
+          // key bytes are in Hex not Base64! TUF also lies that their key is PEM Encoded. Don't
+          // believe


spotless got you again.

loosebazooka · 2022-09-13T14:07:28Z

sigstore-java/src/main/java/dev/sigstore/tuf/TufClient.java

+    var goodSigs = new HashSet<>(role.getKeyids().size());
+    // role.getKeyIds() defines the keys allowed to sign for this role.
+    for (String keyid : role.getKeyids()) {
+      Optional<Signature> signatureMaybe =


maybe is implied by the optional type? honestly not sure what's convention here.

loosebazooka · 2022-09-13T17:59:15Z

sigstore-java/src/main/java/dev/sigstore/tuf/TufClient.java

+      Optional<Signature> signatureMaybe =
+          signatures.stream().filter(sig -> sig.getKeyId().equals(keyid)).findFirst();
+      // only verify if we find a signature that matcheds an allowed key id.
+      if (signatureMaybe.isPresent()) {


You could make use of optional.ifPresent(sig -> ...) if you wanted.

changed my mind. It can't throw the checked exceptions from a lambda. Forgot about that.

loosebazooka · 2022-09-13T18:02:45Z

sigstore-java/src/main/java/dev/sigstore/tuf/TufClient.java

+              goodSigs.add(signature.getKeyId());
+            }
+          } catch (SignatureException e) {
+            throw new RuntimeException(e);


Would it make sense to add more context in this exception? Maybe the SignatureException is sufficient, but that it's failing in the TUF context?

k, I moved to wrapping it with TufException

vlsi · 2022-09-13T19:21:00Z

sigstore-java/src/main/java/dev/sigstore/tuf/TufClient.java

+      throws SignatureVerificationException, NoSuchAlgorithmException, InvalidKeyException,
+          InvalidKeySpecException {
+    // use set to not count the same key multiple times towards the threshold.
+    var goodSigs = new HashSet<>(role.getKeyids().size());


In theory it should be like role.getKeyids().size() * 4 / 3, see https://richardstartin.github.io/posts/5-java-mundane-performance-tricks#size-hashmaps-whenever-possible

Signed-off-by: Patrick Flynn <patrick@chainguard.dev>

patflynn requested a review from loosebazooka September 9, 2022 18:51

patflynn changed the title ~~Tuf root update~~ Add TUF Client root resource syncing. Sep 9, 2022

patflynn force-pushed the tuf-root-update branch from f05c4c5 to ab4610a Compare September 9, 2022 19:16

loosebazooka reviewed Sep 12, 2022

View reviewed changes

Patrick Flynn added 3 commits September 12, 2022 08:55

incorporate review comments and rebase

9fd9eb9

Signed-off-by: Patrick Flynn <patrick@chainguard.dev>

rebase onto main

2900854

Signed-off-by: Patrick Flynn <patrick@chainguard.dev>

fix comments

fcc53af

Signed-off-by: Patrick Flynn <patrick@chainguard.dev>

patflynn force-pushed the tuf-root-update branch from 00bc7f6 to fcc53af Compare September 12, 2022 12:59

Patrick Flynn added 4 commits September 12, 2022 09:04

fix formatting, cause it's what I do

d2a69bc

Signed-off-by: Patrick Flynn <patrick@chainguard.dev>

fix javadoc format

d0155e9

Signed-off-by: Patrick Flynn <patrick@chainguard.dev>

more formatting

c57f84a

Signed-off-by: Patrick Flynn <patrick@chainguard.dev>

fix java version specific behavior of constructing tuf public key

7b23b5b

Signed-off-by: Patrick Flynn <patrick@chainguard.dev>

loosebazooka previously approved these changes Sep 12, 2022

View reviewed changes

asraa reviewed Sep 12, 2022

View reviewed changes

appu review comments

714d53a

Signed-off-by: Patrick Flynn <patrick@chainguard.dev>

patflynn dismissed loosebazooka’s stale review via 714d53a September 12, 2022 19:27

Patrick Flynn added 3 commits September 12, 2022 17:03

fix bug where we aren't verifying keys belong to role for verification

db7d442

Signed-off-by: Patrick Flynn <patrick@chainguard.dev>

fix comments

0cbd861

Signed-off-by: Patrick Flynn <patrick@chainguard.dev>

spotless

76757a4

Signed-off-by: Patrick Flynn <patrick@chainguard.dev>

asraa reviewed Sep 13, 2022

View reviewed changes

loosebazooka previously approved these changes Sep 13, 2022

View reviewed changes

vlsi reviewed Sep 13, 2022

View reviewed changes

fix comments and wrap generic sig exception with TufException

4043de1

Signed-off-by: Patrick Flynn <patrick@chainguard.dev>

patflynn dismissed loosebazooka’s stale review via 4043de1 September 13, 2022 20:23

spotless

130d007

Signed-off-by: Patrick Flynn <patrick@chainguard.dev>

better initial hashmap size

b6a9094

Signed-off-by: Patrick Flynn <patrick@chainguard.dev>

loosebazooka approved these changes Sep 13, 2022

View reviewed changes

patflynn merged commit 70704c1 into main Sep 13, 2022

patflynn deleted the tuf-root-update branch September 13, 2022 20:39

patflynn mentioned this pull request Feb 17, 2023

refactor to use consistent snapshots #345

Merged

Add TUF Client root resource syncing. #123

Add TUF Client root resource syncing. #123

Conversation

patflynn commented Sep 9, 2022

Release Note

Documentation

patflynn commented Sep 9, 2022

loosebazooka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loosebazooka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asraa commented Sep 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asraa left a comment

Choose a reason for hiding this comment

patflynn commented Sep 13, 2022

asraa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment