Merge pull request #392 from Yubico/parse-crldp-extension

Fetch CRLDistributionPoints in FidoMetadataDownloader

Author: emlun

NEWS

@@ -1,5 +1,7 @@
 == Version 2.6.0 (unreleased) ==
 
+`webauthn-server-core`:
+
 New features:
 
 * Added method `getParsedPublicKey(): java.security.PublicKey` to
@@ -61,6 +63,14 @@ New features:
     version increase.
 * (Experimental) Added `credProps` extension to assertion extension outputs.
 
+`webauthn-server-attestation`:
+
+New features:
+
+* `FidoMetadataDownloader` now parses the CRLDistributionPoints extension on the
+  application level, so the `com.sun.security.enableCRLDP=true` system property
+  setting is no longer necessary.
+
 
 == Version 2.5.4 ==
 

webauthn-server-attestation/README.adoc

@@ -222,17 +222,6 @@ RegistrationResult result = rp.finishRegistration(/* ... */);
 Set<MetadataBLOBPayloadEntry> metadata = mds.findEntries(result);
 ----------
 
- 5. If you use the SUN provider for the `PKIX` certificate path validation algorithm, which many deployments do by default:
-    set the `com.sun.security.enableCRLDP` system property to `true`.
-    This is required for the SUN `PKIX` provider to support the CRL Distribution Points extension,
-    which is needed in order to verify the BLOB signature.
-+
-For example, this can be done on the JVM command line using a `-Dcom.sun.security.enableCRLDP=true` option.
-See the https://docs.oracle.com/javase/9/security/java-pki-programmers-guide.htm#GUID-EB250086-0AC1-4D60-AE2A-FC7461374746__SECTION-139-623E860E[Java PKI Programmers Guide]
-for details.
-+
-This step may not be necessary if you use a different provider for the `PKIX` certificate path validation algorithm.
-
 
 == Selecting trusted authenticators
 

webauthn-server-attestation/build.gradle.kts

@@ -48,7 +48,12 @@ dependencies {
   testImplementation("org.scalatestplus:junit-4-13_2.13")
   testImplementation("org.scalatestplus:scalacheck-1-16_2.13")
 
-  testImplementation("org.slf4j:slf4j-api")
+  testImplementation("org.slf4j:slf4j-api") {
+    version {
+      strictly("[1.7.25,1.8-a)") // Pre-1.8 version required by slf4j-test
+    }
+  }
+  testRuntimeOnly("uk.org.lidalia:slf4j-test")
 }
 
 val integrationTest = task<Test>("integrationTest") {
@@ -58,9 +63,6 @@ val integrationTest = task<Test>("integrationTest") {
   testClassesDirs = sourceSets["integrationTest"].output.classesDirs
   classpath = sourceSets["integrationTest"].runtimeClasspath
   shouldRunAfter(tasks.test)
-
-  // Required for processing CRL distribution points extension
-  systemProperty("com.sun.security.enableCRLDP", "true")
 }
 tasks["check"].dependsOn(integrationTest)
 

webauthn-server-attestation/src/integrationTest/scala/com/yubico/fido/metadata/FidoMetadataDownloaderIntegrationTest.scala

@@ -1,5 +1,7 @@
 package com.yubico.fido.metadata
 
+import com.yubico.internal.util.CertificateParser
+import com.yubico.webauthn.data.ByteArray
 import org.junit.runner.RunWith
 import org.scalatest.BeforeAndAfter
 import org.scalatest.funspec.AnyFunSpec
@@ -8,7 +10,8 @@ import org.scalatest.tags.Network
 import org.scalatest.tags.Slow
 import org.scalatestplus.junit.JUnitRunner
 
-import java.util.Optional
+import scala.jdk.CollectionConverters.ListHasAsScala
+import scala.jdk.OptionConverters.RichOption
 import scala.util.Success
 import scala.util.Try
 
@@ -21,24 +24,56 @@ class FidoMetadataDownloaderIntegrationTest
     with BeforeAndAfter {
 
   describe("FidoMetadataDownloader with default settings") {
+    // Cache downloaded items to avoid cause unnecessary load on remote servers
+    var trustRootCache: Option[ByteArray] = None
+    var blobCache: Option[ByteArray] = None
     val downloader =
       FidoMetadataDownloader
         .builder()
         .expectLegalHeader(
           "Retrieval and use of this BLOB indicates acceptance of the appropriate agreement located at https://fidoalliance.org/metadata/metadata-legal-terms/"
         )
         .useDefaultTrustRoot()
-        .useTrustRootCache(() => Optional.empty(), _ => {})
+        .useTrustRootCache(
+          () => trustRootCache.toJava,
+          trustRoot => { trustRootCache = Some(trustRoot) },
+        )
         .useDefaultBlob()
-        .useBlobCache(() => Optional.empty(), _ => {})
+        .useBlobCache(
+          () => blobCache.toJava,
+          blob => { blobCache = Some(blob) },
+        )
         .build()
 
     it("downloads and verifies the root cert and BLOB successfully.") {
-      // This test requires the system property com.sun.security.enableCRLDP=true
       val blob = Try(downloader.loadCachedBlob)
       blob shouldBe a[Success[_]]
       blob.get should not be null
     }
+
+    it(
+      "does not encounter any CRLDistributionPoints entries in unknown format."
+    ) {
+      val blob = Try(downloader.loadCachedBlob)
+      blob shouldBe a[Success[_]]
+      val trustRootCert =
+        CertificateParser.parseDer(trustRootCache.get.getBytes)
+      val certChain = downloader
+        .fetchHeaderCertChain(
+          trustRootCert,
+          FidoMetadataDownloader.parseBlob(blobCache.get).getBlob.getHeader,
+        )
+        .asScala :+ trustRootCert
+      for { cert <- certChain } {
+        withClue(
+          s"Unknown CRLDistributionPoints structure in cert [${cert.getSubjectX500Principal}] : ${new ByteArray(cert.getEncoded)}"
+        ) {
+          CertificateParser
+            .parseCrlDistributionPointsExtension(cert)
+            .isAnyDistributionPointUnsupported should be(false)
+        }
+      }
+    }
   }
 
 }

webauthn-server-attestation/src/main/java/com/yubico/fido/metadata/AAGUID.java

@@ -2,9 +2,9 @@
 
 import com.fasterxml.jackson.annotation.JsonCreator;
 import com.fasterxml.jackson.annotation.JsonValue;
+import com.yubico.internal.util.BinaryUtil;
 import com.yubico.internal.util.ExceptionUtil;
 import com.yubico.webauthn.data.ByteArray;
-import com.yubico.webauthn.data.exception.HexException;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 import lombok.AccessLevel;
@@ -105,12 +105,14 @@ private static ByteArray parse(String value) {
     Matcher matcher = AAGUID_PATTERN.matcher(value);
     if (matcher.find()) {
       try {
-        return ByteArray.fromHex(matcher.group(1))
-            .concat(ByteArray.fromHex(matcher.group(2)))
-            .concat(ByteArray.fromHex(matcher.group(3)))
-            .concat(ByteArray.fromHex(matcher.group(4)))
-            .concat(ByteArray.fromHex(matcher.group(5)));
-      } catch (HexException e) {
+        return new ByteArray(
+            BinaryUtil.concat(
+                BinaryUtil.fromHex(matcher.group(1)),
+                BinaryUtil.fromHex(matcher.group(2)),
+                BinaryUtil.fromHex(matcher.group(3)),
+                BinaryUtil.fromHex(matcher.group(4)),
+                BinaryUtil.fromHex(matcher.group(5))));
+      } catch (Exception e) {
         throw new RuntimeException(
             "This exception should be impossible, please file a bug report.", e);
       }