remove allowed auth uris, add more logging

Author: aschey-forpeople

apps/bfd-server-ng/src/main/java/gov/cms/bfd/server/ng/CertificateUtil.java

@@ -0,0 +1,77 @@
+package gov.cms.bfd.server.ng;
+
+import jakarta.servlet.http.HttpServletRequest;
+import java.net.URLDecoder;
+import java.nio.charset.StandardCharsets;
+import java.util.Arrays;
+import java.util.Optional;
+import lombok.RequiredArgsConstructor;
+import org.apache.commons.lang3.StringUtils;
+import org.jetbrains.annotations.NotNull;
+import org.springframework.core.env.Environment;
+import org.springframework.stereotype.Component;
+
+/** Utility for handling server certificates. */
+@RequiredArgsConstructor
+@Component
+public class CertificateUtil {
+  private final Configuration configuration;
+  private final Environment environment;
+
+  private static final String LEAF_CERT_HEADER = "X-Amzn-Mtls-Clientcert";
+  private static final String CLIENT_CERT_ALIAS_ATTRIBUTE = "CLIENT_CERT_ALIAS";
+
+  /**
+   * Returns whether the current configuration is allowed to bypass auth.
+   *
+   * @return boolean
+   */
+  public boolean canBypassAuth() {
+    return Arrays.stream(environment.getActiveProfiles())
+        .allMatch(Configuration::canProfileBypassAuth);
+  }
+
+  /**
+   * Returns the cert alias from the request, if found.
+   *
+   * @param request request
+   * @return cert alias
+   */
+  public Optional<String> getAliasFromCert(@NotNull HttpServletRequest request) {
+    final var rawLeafCert = request.getHeader(LEAF_CERT_HEADER);
+    if (rawLeafCert == null) {
+      return Optional.empty();
+    }
+    // We need to replace these characters with their URL-encoding counterparts because AWS
+    // considers them "safe" and therefore does not encode them when sending the leaf certificate
+    // from the client certificate in the header. So, when we try to URL Decode them, they get lost.
+    final var encodedLeafCert =
+        rawLeafCert.replace("+", "%2b").replace("=", "%3d").replace("/", "%2f");
+    var leafCert =
+        StringUtils.deleteWhitespace(URLDecoder.decode(encodedLeafCert, StandardCharsets.UTF_8));
+
+    final var clientCertsToAliases = configuration.getClientCertsToAliases();
+    return Optional.ofNullable(clientCertsToAliases.getOrDefault(leafCert, null));
+  }
+
+  /**
+   * Attaches the alias to the request, so it can be reused.
+   *
+   * @param request request
+   * @param certAlias alias
+   */
+  public void attachCertAliasToRequest(@NotNull HttpServletRequest request, String certAlias) {
+    request.setAttribute(CLIENT_CERT_ALIAS_ATTRIBUTE, certAlias);
+  }
+
+  /**
+   * Gets the alias from the request attribute if it was set previously.
+   *
+   * @param request request
+   * @return alias
+   */
+  public Optional<String> getAliasAttribute(@NotNull HttpServletRequest request) {
+    var alias = request.getAttribute(CLIENT_CERT_ALIAS_ATTRIBUTE);
+    return Optional.ofNullable(alias).map(a -> (String) a);
+  }
+}

apps/bfd-server-ng/src/main/java/gov/cms/bfd/server/ng/Configuration.java

@@ -30,19 +30,6 @@ public class Configuration {
   /** Identifies which Spring profiles indicate that the server is being run on a local machine. */
   private static final List<String> ALLOWED_LOCAL_PROFILES = List.of("local", "sqlprofile");
 
-  /** Identifiers which URLS can bypass auth. */
-  private static final List<String> BYPASS_AUTH_URLS =
-      List.of(
-          "/v3/fhir/swagger-ui",
-          "/v3/fhir/swagger-ui/*",
-          "/v3/fhir/api-docs",
-          "/actuator",
-          // We can allow the basic health endpoint, but other actuator endpoints may show
-          // things we don't want to expose, so /actuator/* should not be allowed
-          "/actuator/health",
-          "/actuator/health/*",
-          "/favicon.ico");
-
   // Getters should only be generated for configuration properties, not dependencies
   @Getter(value = AccessLevel.NONE)
   @Autowired
@@ -80,16 +67,6 @@ public static boolean canProfileBypassAuth(String profile) {
     return ALLOWED_LOCAL_PROFILES.contains(profile.toLowerCase());
   }
 
-  /**
-   * Determines if the URL requires auth.
-   *
-   * @param url current url
-   * @return boolean
-   */
-  public static boolean canUrlBypassAuth(String url) {
-    return BYPASS_AUTH_URLS.stream().anyMatch(u -> validateUrlGlob(url, u));
-  }
-
   private static boolean validateUrlGlob(String url, String glob) {
     if (glob.endsWith("*")) {
       return url.startsWith(glob.substring(0, glob.length() - 1));

apps/bfd-server-ng/src/main/java/gov/cms/bfd/server/ng/V3Server.java

@@ -5,6 +5,7 @@
 import ca.uhn.fhir.rest.server.RestfulServer;
 import gov.cms.bfd.server.ng.interceptor.BanUnsupportedHttpMethodsInterceptor;
 import gov.cms.bfd.server.ng.interceptor.ExceptionHandlingInterceptor;
+import gov.cms.bfd.server.ng.interceptor.LoggingInterceptor;
 import gov.cms.bfd.server.openapi.OpenApiInterceptor;
 import jakarta.servlet.annotation.WebServlet;
 import java.util.List;
@@ -36,8 +37,9 @@ public void initialize() {
 
     this.setFhirContext(FhirContext.forR4());
     this.registerProviders(resourceProviders);
-    this.registerInterceptor(new BanUnsupportedHttpMethodsInterceptor());
 
+    this.registerInterceptor(new LoggingInterceptor());
+    this.registerInterceptor(new BanUnsupportedHttpMethodsInterceptor());
     this.registerInterceptor(new ExceptionHandlingInterceptor());
     this.registerInterceptor(new OpenApiInterceptor());
   }

apps/bfd-server-ng/src/main/java/gov/cms/bfd/server/ng/controller/GlobalExceptionController.java

@@ -36,7 +36,7 @@ public String handleError(HttpServletRequest request) {
             .atWarn()
             .setMessage(responseMessage)
             .addKeyValue("statusCode", statusCode)
-            .addKeyValue("path", request.getRequestURI())
+            .addKeyValue("originalUri", originalUri)
             .log();
         return responseMessage;
       }

apps/bfd-server-ng/src/main/java/gov/cms/bfd/server/ng/filter/AuthenticationFilter.java

@@ -1,19 +1,15 @@
 package gov.cms.bfd.server.ng.filter;
 
-import gov.cms.bfd.server.ng.Configuration;
+import gov.cms.bfd.server.ng.CertificateUtil;
 import jakarta.servlet.FilterChain;
 import jakarta.servlet.ServletException;
 import jakarta.servlet.annotation.WebFilter;
 import jakarta.servlet.http.HttpServletRequest;
 import jakarta.servlet.http.HttpServletResponse;
 import java.io.IOException;
-import java.net.URLDecoder;
-import java.nio.charset.StandardCharsets;
-import java.util.Arrays;
 import lombok.RequiredArgsConstructor;
-import org.apache.commons.lang3.StringUtils;
 import org.jetbrains.annotations.NotNull;
-import org.springframework.core.env.Environment;
+import org.springframework.core.annotation.Order;
 import org.springframework.stereotype.Component;
 import org.springframework.web.filter.OncePerRequestFilter;
 
@@ -23,23 +19,17 @@
  */
 @Component
 @RequiredArgsConstructor
+// Ensure this runs first
+@Order(1)
 @WebFilter(filterName = "AuthenticationFilter")
 public class AuthenticationFilter extends OncePerRequestFilter {
   private static final String MISSING_INVALID_HEADER_MSG = "Missing or invalid certificate header.";
-  private static final String LEAF_CERT_HEADER = "X-Amzn-Mtls-Clientcert";
 
-  private final Configuration configuration;
-  private final Environment environment;
+  private final CertificateUtil certificateUtil;
 
   @Override
-  protected boolean shouldNotFilter(HttpServletRequest request) {
-    var path = request.getRequestURI();
-    // Some URLs like the Swagger UI shouldn't require auth
-    if (Configuration.canUrlBypassAuth(path)) {
-      return true;
-    }
-    return Arrays.stream(environment.getActiveProfiles())
-        .allMatch(Configuration::canProfileBypassAuth);
+  protected boolean shouldNotFilter(@NotNull HttpServletRequest request) {
+    return certificateUtil.canBypassAuth();
   }
 
   @Override
@@ -49,27 +39,13 @@ public void doFilterInternal(
       @NotNull FilterChain filterChain)
       throws IOException, ServletException {
 
-    final var rawLeafCert = request.getHeader(LEAF_CERT_HEADER);
-    if (rawLeafCert == null) {
-      response.sendError(400, MISSING_INVALID_HEADER_MSG);
-      return;
-    }
-
-    final var clientCertsToAliases = configuration.getClientCertsToAliases();
-    // We need to replace these characters with their URL-encoding counterparts because AWS
-    // considers them "safe" and therefore does not encode them when sending the leaf certificate
-    // from the client certificate in the header. So, when we try to URL Decode them, they get lost.
-    final var encodedLeafCert =
-        rawLeafCert.replace("+", "%2b").replace("=", "%3d").replace("/", "%2f");
-    final var leafCert =
-        StringUtils.deleteWhitespace(URLDecoder.decode(encodedLeafCert, StandardCharsets.UTF_8));
+    final var certAlias = certificateUtil.getAliasFromCert(request);
 
-    final var certAlias = clientCertsToAliases.getOrDefault(leafCert, null);
-    if (certAlias == null) {
+    if (certAlias.isEmpty()) {
       response.sendError(400, MISSING_INVALID_HEADER_MSG);
       return;
     }
-
+    certificateUtil.attachCertAliasToRequest(request, certAlias.get());
     filterChain.doFilter(request, response);
   }
 }

apps/bfd-server-ng/src/main/java/gov/cms/bfd/server/ng/filter/MdcFilter.java

@@ -0,0 +1,50 @@
+package gov.cms.bfd.server.ng.filter;
+
+import gov.cms.bfd.server.ng.CertificateUtil;
+import jakarta.servlet.Filter;
+import jakarta.servlet.FilterChain;
+import jakarta.servlet.ServletException;
+import jakarta.servlet.ServletRequest;
+import jakarta.servlet.ServletResponse;
+import jakarta.servlet.annotation.WebFilter;
+import jakarta.servlet.http.HttpServletRequest;
+import java.io.IOException;
+import lombok.RequiredArgsConstructor;
+import org.slf4j.MDC;
+import org.springframework.core.annotation.Order;
+import org.springframework.stereotype.Component;
+
+/** Filter for attaching MDC properties. */
+@Component
+@RequiredArgsConstructor
+// This should run directly after the auth filter
+@Order(2)
+@WebFilter(filterName = "MdcFilter")
+public class MdcFilter implements Filter {
+  private final CertificateUtil certificateUtil;
+  private static final String URI = "uri";
+  private static final String REQUEST_ID = "requestId";
+  private static final String CLIENT = "client";
+  private static final String REMOTE_ADDRESS = "remoteAddress";
+
+  @Override
+  public void doFilter(
+      ServletRequest servletRequest, ServletResponse servletResponse, FilterChain filterChain)
+      throws IOException, ServletException {
+
+    if (servletRequest instanceof HttpServletRequest httpRequest) {
+      MDC.put(URI, httpRequest.getRequestURI());
+      MDC.put(REQUEST_ID, httpRequest.getRequestId());
+      MDC.put(REMOTE_ADDRESS, httpRequest.getRemoteAddr());
+      var aliasAttribute = certificateUtil.getAliasAttribute(httpRequest);
+      aliasAttribute.ifPresent((attr) -> MDC.put(CLIENT, attr));
+    }
+    filterChain.doFilter(servletRequest, servletResponse);
+
+    // Clean up to prevent leaks
+    MDC.remove(URI);
+    MDC.remove(REQUEST_ID);
+    MDC.remove(CLIENT);
+    MDC.remove(REMOTE_ADDRESS);
+  }
+}

apps/bfd-server-ng/src/main/java/gov/cms/bfd/server/ng/interceptor/ExceptionHandlingInterceptor.java

@@ -8,15 +8,17 @@
 import jakarta.servlet.http.HttpServletRequest;
 import jakarta.servlet.http.HttpServletResponse;
 import java.io.IOException;
+import lombok.RequiredArgsConstructor;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
 /***
  * Custom exception handling interceptor since the default one is not very customizable.
  */
+@RequiredArgsConstructor
 @Interceptor
 public class ExceptionHandlingInterceptor {
-  Logger logger = LoggerFactory.getLogger(ExceptionHandlingInterceptor.class);
+  private static final Logger LOGGER = LoggerFactory.getLogger(ExceptionHandlingInterceptor.class);
 
   /**
    * Handles the server exception.
@@ -38,6 +40,7 @@ public boolean handleException(
     if (exception.getStatusCode() < 500) {
       return true;
     }
+    // Force a default error message for any unexpected errors
     response.sendError(500, "An unexpected error occurred");
     return false;
   }

apps/bfd-server-ng/src/main/java/gov/cms/bfd/server/ng/interceptor/LoggingInterceptor.java

@@ -0,0 +1,69 @@
+package gov.cms.bfd.server.ng.interceptor;
+
+import ca.uhn.fhir.interceptor.api.Hook;
+import ca.uhn.fhir.interceptor.api.Interceptor;
+import ca.uhn.fhir.interceptor.api.Pointcut;
+import ca.uhn.fhir.rest.api.server.RequestDetails;
+import ca.uhn.fhir.rest.server.exceptions.BaseServerResponseException;
+import ca.uhn.fhir.rest.server.servlet.ServletRequestDetails;
+import jakarta.servlet.http.HttpServletRequest;
+import jakarta.servlet.http.HttpServletResponse;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.slf4j.spi.LoggingEventBuilder;
+
+/**
+ * Interceptor for logging HAPI FHIR requests. HAPI FHIR's built-in interceptor doesn't support
+ * setting the log level or structured logging.
+ */
+@Interceptor
+public class LoggingInterceptor {
+  private static final Logger LOGGER = LoggerFactory.getLogger(LoggingInterceptor.class);
+
+  /**
+   * Log exceptions.
+   *
+   * @param requestDetails requestDetails
+   * @param exception exception
+   * @param request request
+   * @param response response
+   * @return boolean
+   */
+  @Hook(Pointcut.SERVER_HANDLE_EXCEPTION)
+  public boolean handleException(
+      RequestDetails requestDetails,
+      BaseServerResponseException exception,
+      HttpServletRequest request,
+      HttpServletResponse response) {
+
+    addCommonAttrs(
+            LOGGER
+                .atError()
+                .setMessage(exception.getMessage())
+                .addKeyValue("stackTrace", exception.getStackTrace())
+                .addKeyValue("statusCode", exception.getStatusCode()),
+            requestDetails)
+        .log();
+
+    return true;
+  }
+
+  /**
+   * Log successful requests.
+   *
+   * @param requestDetails request details
+   */
+  @Hook(Pointcut.SERVER_PROCESSING_COMPLETED_NORMALLY)
+  public void processingCompletedNormally(ServletRequestDetails requestDetails) {
+    addCommonAttrs(LOGGER.atInfo().setMessage("processed request"), requestDetails).log();
+  }
+
+  private LoggingEventBuilder addCommonAttrs(
+      LoggingEventBuilder eventBuilder, RequestDetails requestDetails) {
+    var operationType = requestDetails.getRestOperationType();
+    return eventBuilder
+        .addKeyValue("resource", requestDetails.getResourceName())
+        .addKeyValue("operation", requestDetails.getOperation())
+        .addKeyValue("operationType", operationType == null ? "" : operationType.getCode());
+  }
+}