search

Yubico / java-webauthn-server

Server-side Web Authentication library for Java https://www.w3.org/TR/webauthn/#rp-operations
Other
439 stars 137 forks source link

Add option to disable revocation checking in the FidoMetadataDownloader #355

Open philsmart opened 3 months ago

philsmart commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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:

  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://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 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("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 commented 3 months ago

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.