Add option to disable revocation checking in the FidoMetadataDownloader

[philsmart]

Currently, the metadata blob verification process creates a cert path validator with the default 'true' setting for the PKIXParameters#revocationEnabled parameter (in FidoMetadataDownloader#verifyBlob) enabling the RevocationChecker. This will fail unless the JVM argument -Dcom.sun.security.enableCRLDP=true is set. However, setting this enables CRL checking for the entire service running on the JVM, which is undesirable in some cases. Would it be possible to add an option to the FidoMetadataDownloader builder to disable revocation checking?

We could catch and ignore the UNDETERMINED_REVOCATION_STATUS from the thrown CertPathValidatorException when we load the cached blob, but it seems more sensible to have the option to disable it when verifying the blob. I assume if the CRL DP is disabled, a supplied CRL list would still be used.

[philsmart]

Ah, we can not catch it, because the MetadataService will not build if revocation fails. So an option to disable it would be neat.

[emlun]

Hi! This is a good question. It does make sense to have some way to not require enableCRLDP=true, but the FIDO MDS spec clearly states that CRLs MUST be used:

2. To validate the digital certificates used in the digital signature, the certificate revocation information MUST be available in the form of CRLs at the respective MDS CRL location e.g. More information can be found at https://fidoalliance.org/metadata/

I also think it's rarely a good idea to solve these kinds of issues by disabling security checks. I think there's a likely better solution, I'll get back to that.

Let's start with a possible workaround you could try right away. As you noted, you can supply additional CRLs to the builder, so it is possible to pre-download the CRLs and inject those manually. This way the com.sun.security.enableCRLDP=true setting is not needed; I've verified experimentally that this works. The two certificates in the BLOB's current cert path have the CRL distribution points http://crl.globalsign.com/root-r3.crl and http://crl.globalsign.com/gs/gsextendvalsha2g3r3.crl, so you can pre-download those before calling the builder - no guarantee that these URLs won't change in the future, though. Testing that in the integration tests looks like this:

Details

With these changes you should see FidoMetadataDownloaderIntegrationTest fail, because it still relies on enableCRLDP=true, while FidoMetadataServiceIntegrationTest succeeds because it pre-downloads the CRLs instead:

diff --git a/webauthn-server-attestation/build.gradle.kts b/webauthn-server-attestation/build.gradle.kts
index 1748835d..6059295c 100644
--- a/webauthn-server-attestation/build.gradle.kts
+++ b/webauthn-server-attestation/build.gradle.kts
@@ -39,6 +39,7 @@ dependencies {
   testImplementation(project(":yubico-util-scala"))
   testImplementation("com.fasterxml.jackson.datatype:jackson-datatype-jdk8")
   testImplementation("junit:junit")
+  testImplementation("org.apache.httpcomponents.client5:httpclient5")
   testImplementation("org.bouncycastle:bcpkix-jdk18on")
   testImplementation("org.eclipse.jetty:jetty-server:[9.4.9.v20180320,10)")
   testImplementation("org.mockito:mockito-core")
@@ -58,9 +59,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)
 
diff --git a/webauthn-server-attestation/src/integrationTest/scala/com/yubico/fido/metadata/FidoMetadataServiceIntegrationTest.scala b/webauthn-server-attestation/src/integrationTest/scala/com/yubico/fido/metadata/FidoMetadataServiceIntegrationTest.scala
index 7f8ba27f..145c1289 100644
--- a/webauthn-server-attestation/src/integrationTest/scala/com/yubico/fido/metadata/FidoMetadataServiceIntegrationTest.scala
+++ b/webauthn-server-attestation/src/integrationTest/scala/com/yubico/fido/metadata/FidoMetadataServiceIntegrationTest.scala
@@ -11,6 +11,9 @@ import com.yubico.webauthn.RelyingParty
 import com.yubico.webauthn.TestWithEachProvider
 import com.yubico.webauthn.test.Helpers
 import com.yubico.webauthn.test.RealExamples
+import org.apache.hc.client5.http.impl.classic.HttpClients
+import org.apache.hc.core5.http.HttpHost
+import org.apache.hc.core5.http.io.support.ClassicRequestBuilder
 import org.bouncycastle.jce.provider.BouncyCastleProvider
 import org.junit.runner.RunWith
 import org.scalatest.BeforeAndAfter
@@ -20,9 +23,11 @@ import org.scalatest.tags.Network
 import org.scalatest.tags.Slow
 import org.scalatestplus.junit.JUnitRunner
 
+import java.security.cert.CRL
 import java.time.Clock
 import java.time.ZoneOffset
 import java.util.Optional
+import scala.jdk.CollectionConverters.SeqHasAsJava
 import scala.jdk.CollectionConverters.SetHasAsJava
 import scala.jdk.CollectionConverters.SetHasAsScala
 import scala.jdk.OptionConverters.RichOptional
@@ -38,6 +43,35 @@ class FidoMetadataServiceIntegrationTest
     with TestWithEachProvider {
 
   describe("FidoMetadataService") {
+    val cfact = java.security.cert.CertificateFactory.getInstance("X.509")
+    val crls: List[CRL] = List(
+      cfact.generateCRL(
+        HttpClients
+          .createMinimal()
+          .executeOpen(
+            HttpHost.create("crl.globalsign.com"),
+            ClassicRequestBuilder
+              .get("http://crl.globalsign.com/root-r3.crl")
+              .build(),
+            null,
+          )
+          .getEntity
+          .getContent
+      ),
+      cfact.generateCRL(
+        HttpClients
+          .createMinimal()
+          .executeOpen(
+            HttpHost.create("crl.globalsign.com"),
+            ClassicRequestBuilder
+              .get("http://crl.globalsign.com/gs/gsextendvalsha2g3r3.crl")
+              .build(),
+            null,
+          )
+          .getEntity
+          .getContent
+      ),
+    )
 
     describe("downloaded with default settings") {
       val downloader = FidoMetadataDownloader
@@ -49,6 +83,7 @@ class FidoMetadataServiceIntegrationTest
         .useTrustRootCache(() => Optional.empty(), _ => {})
         .useDefaultBlob()
         .useBlobCache(() => Optional.empty(), _ => {})
+        .useCrls(crls.asJava)
         .build()
       val fidoMds =
         Try(

Now back to the topic of a more permanent solution: maybe FidoMetadataDownloader should just adopt this workaround as an integrated feature (then we would of course read the CRLDP URLs from the certificates instead of hard-coding them). That way users shouldn't need to set com.sun.security.enableCRLDP=true globally, but can still have the revocation checks. It's nice that this also removes the pitfall of missing the enableCRLDP step in the usage guide.

Does that seem like a reasonable solution, and would the above workaround work for you in the meantime?

[philsmart]

Many thanks for this super detailed response (and for pointing out the spec) . You're right, disabling a security check was a bit of a dumb request, although the whole process of revocation checking seems hard and no method ideal.

The question should have been more like your solution: can we isolate this to the downloader. In which case your solution sounds great to me.

For now I will follow your workaround and wire up the required CRLs for our users to test with.

Thank you.