Use HTTP cache headers in MDS BLOB downloads

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:
 

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

@@ -876,7 +876,14 @@ private Optional<MetadataBLOB> 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<MetadataBLOB> 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<ByteArray> 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<String> 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<CertStore> fetchCrlDistributionPoints(
           CertStore.getInstance("Collection", new CollectionCertStoreParameters(crldpCrls)));
     }
   }
+
+  private static class NotModified extends Throwable {}
 }

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)
+        }
+
+      }
+    })
   }
 
 }