From 602aa105314cfefaae1f68c27f5b08b15ca751f7 Mon Sep 17 00:00:00 2001 From: Appu Goundan Date: Mon, 18 Nov 2024 13:24:55 -0500 Subject: [PATCH] Fix test_duplicate_sig_keyids This isn't a security concern, our looping didn't actually allow re-use of keys. But we weren't exactly according to spec. Signed-off-by: Appu Goundan --- .../tuf/DuplicateKeyIdsException.java | 52 +++++++++++++++++++ .../main/java/dev/sigstore/tuf/Updater.java | 22 +++++--- tuf-cli/tuf-cli.xfails | 1 - 3 files changed, 67 insertions(+), 8 deletions(-) create mode 100644 sigstore-java/src/main/java/dev/sigstore/tuf/DuplicateKeyIdsException.java diff --git a/sigstore-java/src/main/java/dev/sigstore/tuf/DuplicateKeyIdsException.java b/sigstore-java/src/main/java/dev/sigstore/tuf/DuplicateKeyIdsException.java new file mode 100644 index 00000000..0a227063 --- /dev/null +++ b/sigstore-java/src/main/java/dev/sigstore/tuf/DuplicateKeyIdsException.java @@ -0,0 +1,52 @@ +/* + * Copyright 2022 The Sigstore Authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package dev.sigstore.tuf; + +import dev.sigstore.tuf.model.Signature; +import java.util.List; +import java.util.Locale; +import java.util.stream.Collectors; + +/** + * Thrown when the metadata has signatures from the same key even if the threshold is met. 4.2.1 + */ +public class DuplicateKeyIdsException extends TufException { + + private final List signatures; + private final String keyId; + + public DuplicateKeyIdsException(List signatures, String keyId) { + super( + String.format( + Locale.ROOT, + "The role has multiple signatures with the same key_id. [Signatures: %s, KeyId: %s]", + signatures.stream() + .map(Signature::getSignature) + .collect(Collectors.joining(",", "(", ")")), + keyId)); + this.signatures = signatures; + this.keyId = keyId; + } + + public List getSignatures() { + return signatures; + } + + public String getKeyId() { + return keyId; + } +} 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 d71919ff..9bde6c9c 100644 --- a/sigstore-java/src/main/java/dev/sigstore/tuf/Updater.java +++ b/sigstore-java/src/main/java/dev/sigstore/tuf/Updater.java @@ -42,6 +42,7 @@ import java.util.Optional; import java.util.logging.Level; import java.util.logging.Logger; +import java.util.stream.Collectors; import org.bouncycastle.util.encoders.DecoderException; import org.bouncycastle.util.encoders.Hex; @@ -211,8 +212,7 @@ private boolean hasNewKeys(RootRole oldRole, RootRole newRole) { } void verifyDelegate(Root trustedRoot, SignedTufMeta delegate) - throws SignatureVerificationException, IOException, NoSuchAlgorithmException, - InvalidKeySpecException { + throws SignatureVerificationException, IOException { verifyDelegate( delegate.getSignatures(), trustedRoot.getSignedMeta().getKeys(), @@ -228,6 +228,7 @@ void verifyDelegate(Root trustedRoot, SignedTufMeta delegate) * @param role the key ids and threshold values for role signing * @param verificationMaterial the contents to be verified for authenticity * @throws SignatureVerificationException if there are not enough verified signatures + * @throws IOException if an error occurred parsing a key */ @VisibleForTesting void verifyDelegate( @@ -235,16 +236,23 @@ void verifyDelegate( Map publicKeys, Role role, byte[] verificationMaterial) - throws InvalidKeySpecException, IOException, NoSuchAlgorithmException { + throws IOException { // 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. for (String keyid : role.getKeyids()) { - Optional signatureMaybe = - signatures.stream().filter(sig -> sig.getKeyId().equals(keyid)).findFirst(); + List matchingSignatures = + signatures.stream() + .filter(sig -> sig.getKeyId().equals(keyid)) + .collect(Collectors.toList()); + // check for any duplicate key_ids: + // https://theupdateframework.github.io/specification/latest/#file-formats-object-format + if (matchingSignatures.size() > 1) { + throw new DuplicateKeyIdsException(matchingSignatures, keyid); + } // only verify if we find a signature that matches an allowed key id. - if (signatureMaybe.isPresent()) { - var signature = signatureMaybe.get(); + if (matchingSignatures.size() == 1) { + var signature = matchingSignatures.get(0); // look for the public key that matches the key ID and use it for verification. var key = publicKeys.get(signature.getKeyId()); if (key != null) { diff --git a/tuf-cli/tuf-cli.xfails b/tuf-cli/tuf-cli.xfails index ca109077..77eabad5 100644 --- a/tuf-cli/tuf-cli.xfails +++ b/tuf-cli/tuf-cli.xfails @@ -1,5 +1,4 @@ test_metadata_bytes_match -test_duplicate_sig_keyids test_unusual_role_name[?] test_unusual_role_name[#] test_unusual_role_name[/delegatedrole]