Address PR comments

src/main/java/network/brightspots/rcv/HartCvrReader.java

@@ -45,16 +45,15 @@ boolean verifyHashIfNeeded(File cvrXml) {
       File signatureXml = new File(cvrXml.getAbsolutePath() + ".sig.xml");
       if (signatureXml.exists()) {
         try {
-          isHashVerified = SecuritySignatureValidation.verifyPublicKeySignature(
+          SecuritySignatureValidation.ensureSignatureIsValid(
                   SecurityConfig.getRsaPublicKey(), signatureXml, cvrXml);
-
-          if (!isHashVerified) {
-            Logger.severe("Incorrect hash %s of %s",
-                    signatureXml.getAbsolutePath(), cvrXml.getAbsolutePath());
-          }
+          isHashVerified = true;
         } catch (SecuritySignatureValidation.CouldNotVerifySignatureException e) {
           Logger.severe("Failure while trying to verify hash %s of %s: \n%s",
                   signatureXml.getAbsolutePath(), cvrXml.getAbsolutePath(), e.getMessage());
+        } catch (SecuritySignatureValidation.VerificationFailedException e) {
+          Logger.severe("Incorrect hash %s of %s",
+                  signatureXml.getAbsolutePath(), cvrXml.getAbsolutePath());
         }
       } else {
         Logger.severe("A cryptographic signature is required at %s, but it was not found.",

src/main/java/network/brightspots/rcv/SecuritySignatureValidation.java

@@ -49,17 +49,18 @@
 
 class SecuritySignatureValidation {
   /**
-   * This function returns whether the signature matches validation.
-   * It throws an error if it is unable to perform this check, for reasons including:
+   * Throws an exception if the signature is invalid.
+   * Throws CouldNotVerifySignature if it is unable to run the validation, for reasons including:
    * 1. The public key in the signed file does not match the expected key
    * 2. The canonicalization or hashing algorithm is not supported
-   * 3. The corresponding file has been edited and its hash no longer
-   * matches what's found in signatureKeyFile
+   * 3. The corresponding file has been edited and its hash no longer matches what's found in signatureKeyFile
+   * Throws VerificationFailedException if the signature verification successfully ran, but
+   * the corresponding signature was invalid.
    */
-  public static boolean verifyPublicKeySignature(
+  public static void ensureSignatureIsValid(
           RsaKeyValue publicKey,
           File signatureKeyFile,
-          File dataFile) throws CouldNotVerifySignatureException {
+          File dataFile) throws CouldNotVerifySignatureException, VerificationFailedException {
     // Load the .sig.xml file into a HartSignature object
     HartSignature hartSignature;
     try {
@@ -68,12 +69,9 @@ public static boolean verifyPublicKeySignature(
       throw new CouldNotVerifySignatureException("Failed to read files: " + e.getMessage());
     }
 
-    // Checks that will throw exceptions if they don't pass
     ensurePublicKeyMatchesExpectedValue(hartSignature, publicKey, signatureKeyFile);
     ensureDigestMatchesExpectedValue(dataFile, hartSignature);
-
-    // Check that might throw an exception if its unable to perform the signature validation.
-    return checkSignature(hartSignature, publicKey);
+    ensureSignatureMatches(hartSignature, publicKey);
   }
 
   /**
@@ -91,7 +89,9 @@ private static void ensureDigestMatchesExpectedValue(File dataFile, HartSignatur
               "Signed file was %s but you provided %s".formatted(actualFilename, expectedFilename));
     }
 
-    byte[] actualDigestBytes = FileUtils.getHashBytes(dataFile, "SHA-256");
+    String algorithm = javaAlgorithmForW3AlgorithmUrl(
+            hartSignature.signedInfo.reference.digestMethod.algorithm);
+    byte[] actualDigestBytes = FileUtils.getHashBytes(dataFile, algorithm);
     String actualDigest = Base64.getEncoder().encodeToString(actualDigestBytes);
     String expectedDigest = hartSignature.signedInfo.reference.digestValue;
 
@@ -102,6 +102,21 @@ private static void ensureDigestMatchesExpectedValue(File dataFile, HartSignatur
     }
   }
 
+  /**
+   * The .sig.xml files use a w3.org URL to reference an algorithm, whereas Java
+   * uses just SHA-X. Convert the w3.org URL to the Java algorithm name .
+   */
+  private static String javaAlgorithmForW3AlgorithmUrl(String w3AlgorithmUrl)
+          throws CouldNotVerifySignatureException {
+    return switch (w3AlgorithmUrl) {
+        case "http://www.w3.org/2001/04/xmlenc#sha512" -> "SHA-512";
+        case "http://www.w3.org/2001/04/xmlenc#sha256" -> "SHA-256";
+        case "http://www.w3.org/2000/09/xmldsig#sha1" -> "SHA-1";
+        default -> throw new CouldNotVerifySignatureException(
+                "Unsupported digest algorithm: %s".formatted(w3AlgorithmUrl));
+    };
+  }
+
 
   /**
    * Does the signature file match the expected public key, which is encoded in the
@@ -129,8 +144,8 @@ private static void ensurePublicKeyMatchesExpectedValue(
    * Once all checks of tampering have been performed, we can check the signature itself.
    * If this returns false, it is still a severe, blocking error.
    */
-  private static boolean checkSignature(HartSignature hartSignature, RsaKeyValue rsaKeyValue)
-          throws CouldNotVerifySignatureException {
+  private static void ensureSignatureMatches(HartSignature hartSignature, RsaKeyValue rsaKeyValue)
+          throws CouldNotVerifySignatureException, VerificationFailedException {
     // Decode Base64
     byte[] modulusBytes = Base64.getDecoder().decode(rsaKeyValue.modulus);
     byte[] exponentBytes = Base64.getDecoder().decode(rsaKeyValue.exponent);
@@ -166,7 +181,9 @@ private static boolean checkSignature(HartSignature hartSignature, RsaKeyValue r
     // Verify the signature
     try {
       signature.update(canonicalizedBytes);
-      return signature.verify(signatureBytes);
+      if (!signature.verify(signatureBytes)) {
+        throw new VerificationFailedException("Signature did not match.");
+      }
     } catch (SignatureException e) {
       throw new CouldNotVerifySignatureException("Signature failure: %s" + e.getMessage());
     }
@@ -239,9 +256,19 @@ private static <T> T readFromXml(File xmlFile, Class<T> classType) throws IOExce
     }
   }
 
+  // Used when something prevents the signature verification from running,
+  // either because of an error or because the verification result would be invalid even if
+  // it succeeded (e.g. the signature was valid, but the hash did not match).
   static class CouldNotVerifySignatureException extends Exception {
     CouldNotVerifySignatureException(String message) {
       super(message);
     }
   }
+
+  // Used when the signature verification successfully ran, but the signatures did not match.
+  static class VerificationFailedException extends Exception {
+    VerificationFailedException(String message) {
+      super(message);
+    }
+  }
 }

src/test/java/network/brightspots/rcv/SecurityTests.java

@@ -18,10 +18,9 @@
 package network.brightspots.rcv;
 
 import static network.brightspots.rcv.SecuritySignatureValidation.CouldNotVerifySignatureException;
-import static network.brightspots.rcv.SecuritySignatureValidation.verifyPublicKeySignature;
+import static network.brightspots.rcv.SecuritySignatureValidation.VerificationFailedException;
+import static network.brightspots.rcv.SecuritySignatureValidation.ensureSignatureIsValid;
 import static network.brightspots.rcv.SecurityXmlParsers.RsaKeyValue;
-import static org.junit.jupiter.api.Assertions.assertFalse;
-import static org.junit.jupiter.api.Assertions.assertTrue;
 
 import com.fasterxml.jackson.databind.DeserializationFeature;
 import com.fasterxml.jackson.dataformat.xml.XmlMapper;
@@ -78,16 +77,17 @@ static RsaKeyValue defaultPublicKey() {
 
   @Test
   @DisplayName("Succeeds using the default, valid files")
-  void testValidationSucceeds() throws CouldNotVerifySignatureException {
-    assertTrue(verifyPublicKeySignature(
-            defaultPublicKey(), DEFAULT_SIGNATURE_FILE, DEFAULT_DATA_FILE));
+  void testValidationSucceeds() throws
+          CouldNotVerifySignatureException, VerificationFailedException {
+    ensureSignatureIsValid(defaultPublicKey(), DEFAULT_SIGNATURE_FILE, DEFAULT_DATA_FILE);
   }
 
   @Test
   @DisplayName("Verification fails when the signature is incorrect")
-  void testValidationFailure() throws CouldNotVerifySignatureException {
+  void testValidationFailure() {
     File incorrectSignatureFile = new File(TEST_FOLDER + "/incorrect_signature.xml.sig.xml");
-    assertFalse(verifyPublicKeySignature(
+    Assertions.assertThrows(VerificationFailedException.class, () ->
+            ensureSignatureIsValid(
             defaultPublicKey(), incorrectSignatureFile, DEFAULT_DATA_FILE));
   }
 
@@ -100,19 +100,19 @@ void testModifiedDataFile() throws IOException {
     Files.writeString(modifiedDataFile.toPath(), "not the original data");
 
     Assertions.assertThrows(CouldNotVerifySignatureException.class, () ->
-        verifyPublicKeySignature(defaultPublicKey(), DEFAULT_SIGNATURE_FILE, modifiedDataFile)
+        ensureSignatureIsValid(defaultPublicKey(), DEFAULT_SIGNATURE_FILE, modifiedDataFile)
     );
   }
 
   @Test
   @DisplayName("Succeeds when data file is in a different folder (but has the right filename)")
-  void testSuccessIfFilenameMatches() throws CouldNotVerifySignatureException, IOException {
+  void testSuccessIfFilenameMatches()
+          throws CouldNotVerifySignatureException, VerificationFailedException, IOException {
     File modifiedDataFile = new File(TEMP_DIRECTORY.getAbsolutePath() + "/test.xml");
     modifiedDataFile.deleteOnExit();
     Files.copy(DEFAULT_DATA_FILE.toPath(), modifiedDataFile.toPath());
 
-    assertTrue(verifyPublicKeySignature(
-            defaultPublicKey(), DEFAULT_SIGNATURE_FILE, modifiedDataFile));
+    ensureSignatureIsValid(defaultPublicKey(), DEFAULT_SIGNATURE_FILE, modifiedDataFile);
   }
 
   @Test
@@ -123,15 +123,15 @@ void testFailsIfFilenameDiffers() throws IOException {
     Files.copy(DEFAULT_DATA_FILE.toPath(), modifiedDataFile.toPath());
 
     Assertions.assertThrows(CouldNotVerifySignatureException.class, () ->
-        verifyPublicKeySignature(defaultPublicKey(), DEFAULT_SIGNATURE_FILE, modifiedDataFile)
+        ensureSignatureIsValid(defaultPublicKey(), DEFAULT_SIGNATURE_FILE, modifiedDataFile)
     );
   }
 
   @Test
   @DisplayName("Exception is thrown when the file is signed with an unsupported key")
   void testUnexpectedPublicKey() {
     File incorrectPublicKey = new File(TEST_FOLDER + "/incorrect_public_key.txt");
-    Assertions.assertThrows(CouldNotVerifySignatureException.class, () -> verifyPublicKeySignature(
+    Assertions.assertThrows(CouldNotVerifySignatureException.class, () -> ensureSignatureIsValid(
             getPublicKeyFor(incorrectPublicKey),
             DEFAULT_SIGNATURE_FILE,
             DEFAULT_DATA_FILE)