diff --git a/NEWS b/NEWS index 7dfd71f69..0612c1189 100644 --- a/NEWS +++ b/NEWS @@ -36,6 +36,13 @@ New features: `cxpConfigURL` ** `StatusReport`: `certificationProfiles`, `sunsetDate`, `fipsRevision`, `fipsPhysicalSecurityLevel` +* `FidoMetadataDownloader` now sends the `If-None-Match` request header set to + the `"no"` of the cached BLOB, if any, and handles `304 Not Modified` + responses appropriately. +* In `FidoMetadataDownloader` if a BLOB download request returns an HTTP failure + status, but returns an `ETag` response header matching the `"no"` of the + cached BLOB, if any, this is now interpreted like a successful `304 Not + Modified` response. Fixes: diff --git a/webauthn-server-attestation/src/main/java/com/yubico/fido/metadata/FidoMetadataDownloader.java b/webauthn-server-attestation/src/main/java/com/yubico/fido/metadata/FidoMetadataDownloader.java index 09bae4768..fdc23f136 100644 --- a/webauthn-server-attestation/src/main/java/com/yubico/fido/metadata/FidoMetadataDownloader.java +++ b/webauthn-server-attestation/src/main/java/com/yubico/fido/metadata/FidoMetadataDownloader.java @@ -876,7 +876,14 @@ private Optional refreshBlobInternal( try { log.debug("Attempting to download new BLOB..."); - final ByteArray downloadedBytes = download(blobUrl); + final ByteArray downloadedBytes = + download( + blobUrl, + // This should ideally use the value of the ETag response header from when the cached + // BLOB was downloaded, but we don't have anywhere to store that without changing the + // format of the cache serialization. This is good enough as the MDS explicitly + // specifies that the ETag is set to the "no" of the BLOB. + cached.map(cachedBlob -> String.format("%d", cachedBlob.getPayload().getNo()))); final MetadataBLOB downloadedBlob = parseAndVerifyBlob(downloadedBytes, trustRoot); log.debug("New BLOB downloaded."); @@ -906,6 +913,11 @@ private Optional refreshBlobInternal( } return Optional.of(downloadedBlob); + + } catch (NotModified e) { + log.debug("Remote BLOB not modified - using cached BLOB."); + return cached; + } catch (FidoMetadataDownloaderException e) { if (e.getReason() == Reason.BAD_SIGNATURE && cached.isPresent()) { log.warn("New BLOB has bad signature - falling back to cached BLOB."); @@ -1062,6 +1074,17 @@ private Optional readCacheFile(File cacheFile) throws IOException { } private ByteArray download(URL url) throws IOException { + try { + return download(url, Optional.empty()); + } catch (NotModified e) { + final String msg = + "NotModified thrown by download(URL, Optional.empty()). This should be impossible, please file a bug report."; + log.error(msg); + throw new RuntimeException(msg, e); + } + } + + private ByteArray download(URL url, Optional etag) throws IOException, NotModified { URLConnection conn = url.openConnection(); if (conn instanceof HttpsURLConnection) { @@ -1082,6 +1105,38 @@ private ByteArray download(URL url) throws IOException { } } httpsConn.setRequestMethod("GET"); + etag.ifPresent( + et -> { + httpsConn.addRequestProperty("If-None-Match", String.format("\"%s\"", et)); + }); + + if (httpsConn.getResponseCode() != HttpsURLConnection.HTTP_OK) { + switch (httpsConn.getResponseCode()) { + case 304: // Not Modified + log.debug("Received 304 Not Modified response to download request: {}", url); + throw new NotModified(); + } + + log.warn( + "Received non-200 status: {} to download request: {}", + httpsConn.getResponseCode(), + url); + + final String responseEtag = httpsConn.getHeaderField("ETag"); + if (responseEtag != null) { + log.debug("Response ETag: {}", responseEtag); + if (etag.map( + et -> + // ETag header value should be wrapped with double quotes (`etag: "243"`), but + // FIDO MDS returns it like: `etag: 243`. Try both in case that changes in the + // future. + et.equals(responseEtag) || String.format("\"%s\"", et).equals(responseEtag)) + .orElseGet(() -> false)) { + log.debug("Response ETag matches local ETag - interpreting as not modified."); + throw new NotModified(); + } + } + } } return readAll(conn.getInputStream()); @@ -1321,4 +1376,6 @@ private Optional fetchCrlDistributionPoints( CertStore.getInstance("Collection", new CollectionCertStoreParameters(crldpCrls))); } } + + private static class NotModified extends Throwable {} } diff --git a/webauthn-server-attestation/src/test/scala/com/yubico/fido/metadata/FidoMetadataDownloaderSpec.scala b/webauthn-server-attestation/src/test/scala/com/yubico/fido/metadata/FidoMetadataDownloaderSpec.scala index 22b8461da..1b1fb09cf 100644 --- a/webauthn-server-attestation/src/test/scala/com/yubico/fido/metadata/FidoMetadataDownloaderSpec.scala +++ b/webauthn-server-attestation/src/test/scala/com/yubico/fido/metadata/FidoMetadataDownloaderSpec.scala @@ -219,17 +219,19 @@ class FidoMetadataDownloaderSpec private def makeHttpServer( path: String, response: String, + inspectRequest: HttpServletRequest => Any = _ => {}, ): (Server, String, X509Certificate) = - makeHttpServer( - Map(path -> (200, response.getBytes(StandardCharsets.UTF_8))) - ) + makeHttpServer(Map(path -> (request => { + inspectRequest(request) + (200, response.getBytes(StandardCharsets.UTF_8)) + }))) private def makeHttpServer( path: String, response: Array[Byte], ): (Server, String, X509Certificate) = - makeHttpServer(Map(path -> (200, response))) + makeHttpServer(Map(path -> (_ => (200, response)))) private def makeHttpServer( - responses: Map[String, (Int, Array[Byte])] + responses: Map[String, HttpServletRequest => (Int, Array[Byte])] ): (Server, String, X509Certificate) = { val tlsKey = TestAuthenticator.generateEcKeypair() val tlsCert = TestAuthenticator.buildCertificate( @@ -271,8 +273,10 @@ class FidoMetadataDownloaderSpec request: HttpServletRequest, response: HttpServletResponse, ): Unit = { + responses.get(target) match { - case Some((status, responseBody)) => { + case Some(processRequest) => { + val (status, responseBody) = processRequest(request) response.getOutputStream.write(responseBody) response.setStatus(status) } @@ -1119,8 +1123,13 @@ class FidoMetadataDownloaderSpec val (server, serverUrl, httpsCert) = makeHttpServer( Map( - "/blob.jwt" -> (HttpStatus.TOO_MANY_REQUESTS_429, newBlobJwt - .getBytes(StandardCharsets.UTF_8)) + "/blob.jwt" -> (_ => + ( + HttpStatus.TOO_MANY_REQUESTS_429, + newBlobJwt + .getBytes(StandardCharsets.UTF_8), + ) + ) ) ) startServer(server) @@ -1181,10 +1190,17 @@ class FidoMetadataDownloaderSpec val (server, _, httpsCert) = makeHttpServer( Map( - "/chain.pem" -> (200, certChainPem.getBytes( - StandardCharsets.UTF_8 - )), - "/blob.jwt" -> (200, blobJwt.getBytes(StandardCharsets.UTF_8)), + "/chain.pem" -> (_ => + ( + 200, + certChainPem.getBytes( + StandardCharsets.UTF_8 + ), + ) + ), + "/blob.jwt" -> (_ => + (200, blobJwt.getBytes(StandardCharsets.UTF_8)) + ), ) ) startServer(server) @@ -2289,6 +2305,123 @@ class FidoMetadataDownloaderSpec blob should not be null blob.getNo should equal(newBlobNo) } + + withEachLoadMethod(load => { + describe( + """The download request sends the If-None-Match header set to the "no" value of the cached BLOB""" + ) { + val oldBlobExpires = CertValidFrom.plus(10, ChronoUnit.DAYS) + val newBlobExpires = oldBlobExpires.plus(5, ChronoUnit.DAYS) + + val oldBlobJwt = + makeBlob( + List(blobCert), + blobKeypair, + oldBlobExpires.atOffset(ZoneOffset.UTC).toLocalDate, + no = oldBlobNo, + ) + val newBlobJwt = + makeBlob( + List(blobCert), + blobKeypair, + newBlobExpires.atOffset(ZoneOffset.UTC).toLocalDate, + no = newBlobNo, + ) + + def downloader( + currentTime: Instant, + serverUrl: String, + serverCert: X509Certificate, + ): FidoMetadataDownloader = { + FidoMetadataDownloader + .builder() + .expectLegalHeader( + "Kom ihåg att du aldrig får snyta dig i mattan!" + ) + .useTrustRoot(trustRootCert) + .downloadBlob(new URL(s"${serverUrl}/blob.jwt")) + .useBlobCache( + () => + Optional.of( + new ByteArray(oldBlobJwt.getBytes(StandardCharsets.UTF_8)) + ), + _ => {}, + ) + .clock(Clock.fixed(currentTime, ZoneOffset.UTC)) + .useCrls(crls.asJava) + .trustHttpsCerts(serverCert) + .build() + } + + /** Needed to work around some strange scalafmt issues */ + def toQuoted(i: Int): String = String.join("", "\"", i.toString, "\"") + + it("when the cached BLOB is up to date.") { + var requestRan = false + var requestSucceeded = false + val (server, serverUrl, serverCert) = + makeHttpServer( + "/blob.jwt", + newBlobJwt, + request => { + requestRan = true + request.getHeader("If-None-Match") should equal( + toQuoted(oldBlobNo) + ) + // The above assertion won't directly cause the test to fail, since it runs in the server context. + requestSucceeded = true + }, + ) + startServer(server) + + val blob = + load( + downloader( + oldBlobExpires.minus(1, ChronoUnit.DAYS), + serverUrl, + serverCert, + ) + ).getPayload + blob should not be null + if (requestRan) { + blob.getNo should equal(newBlobNo) + requestSucceeded should be(true) + } else { + blob.getNo should equal(oldBlobNo) + } + } + + it("when the cached BLOB is expired.") { + var requestSucceeded = false + val (server, serverUrl, serverCert) = + makeHttpServer( + "/blob.jwt", + newBlobJwt, + request => { + request.getHeader("If-None-Match") should equal( + toQuoted(oldBlobNo) + ) + // The above assertion won't directly cause the test to fail, since it runs in the server context. + requestSucceeded = true + }, + ) + startServer(server) + + val blob = + load( + downloader( + oldBlobExpires.plus(1, ChronoUnit.DAYS), + serverUrl, + serverCert, + ) + ).getPayload + blob should not be null + blob.getNo should equal(newBlobNo) + requestSucceeded should be(true) + } + + } + }) } }