Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hi! This is a good question. It does make sense to have some way to not require enableCRLDP=true, but the [FIDO MDS spec](https://fidoalliance.org/specs/mds/fido-metadata-service-v3.0-ps-20210518.html#metadata-blob-object-processing-rules) clearly states that CRLs MUST be used: #617

Closed
Sltaaa3bergar opened this issue May 29, 2024 · 0 comments

Comments

@Sltaaa3bergar
Copy link

          Hi! This is a good question. It does make sense to have some way to not require `enableCRLDP=true`, but the [FIDO MDS spec](https://fidoalliance.org/specs/mds/fido-metadata-service-v3.0-ps-20210518.html#metadata-blob-object-processing-rules) clearly states that CRLs MUST be used:
  1. 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:https://crl.globalsign.com/root-r3.crl and http:https://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:https://crl.globalsign.com/root-r3.crl")
+              .build(),
+            null,
+          )
+          .getEntity
+          .getContent
+      ),
+      cfact.generateCRL(
+        HttpClients
+          .createMinimal()
+          .executeOpen(
+            HttpHost.create("crl.globalsign.com"),
+            ClassicRequestBuilder
+              .get("http:https://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?

Originally posted by @emlun in Yubico/java-webauthn-server#355 (comment)

@dainnilsson dainnilsson closed this as not planned Won't fix, can't repro, duplicate, stale May 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants