From c2bc41d15eed31f89076913f641298ded5219a4f Mon Sep 17 00:00:00 2001 From: david-leifker <114954101+david-leifker@users.noreply.github.com> Date: Fri, 3 Nov 2023 12:29:31 -0500 Subject: [PATCH] fix(health): fix health check url authentication (#9117) --- .../authentication/AuthenticationRequest.java | 12 ++++ .../filter/AuthenticationFilter.java | 13 ++++- .../HealthStatusAuthenticator.java | 55 +++++++++++++++++++ .../src/main/resources/application.yml | 2 + metadata-service/health-servlet/build.gradle | 22 -------- .../openapi/config/SpringWebConfig.java | 2 - .../health}/HealthCheckController.java | 30 ++++++---- metadata-service/war/build.gradle | 1 - .../webapp/WEB-INF/openapiServlet-servlet.xml | 2 +- settings.gradle | 1 - 10 files changed, 101 insertions(+), 39 deletions(-) create mode 100644 metadata-service/auth-impl/src/main/java/com/datahub/authentication/authenticator/HealthStatusAuthenticator.java delete mode 100644 metadata-service/health-servlet/build.gradle rename metadata-service/{health-servlet/src/main/java/com/datahub/health/controller => openapi-servlet/src/main/java/io/datahubproject/openapi/health}/HealthCheckController.java (79%) diff --git a/metadata-auth/auth-api/src/main/java/com/datahub/authentication/AuthenticationRequest.java b/metadata-auth/auth-api/src/main/java/com/datahub/authentication/AuthenticationRequest.java index 91f15f9d5ae61..5673bac5442b2 100644 --- a/metadata-auth/auth-api/src/main/java/com/datahub/authentication/AuthenticationRequest.java +++ b/metadata-auth/auth-api/src/main/java/com/datahub/authentication/AuthenticationRequest.java @@ -1,6 +1,8 @@ package com.datahub.authentication; import com.datahub.plugins.auth.authentication.Authenticator; +import lombok.Getter; + import java.util.Map; import java.util.Objects; import java.util.TreeMap; @@ -13,14 +15,24 @@ * Currently, this class only hold the inbound request's headers, but could certainly be extended * to contain additional information like the request parameters, body, ip, etc as needed. */ +@Getter public class AuthenticationRequest { private final Map caseInsensitiveHeaders; + private final String servletInfo; + private final String pathInfo; + public AuthenticationRequest(@Nonnull final Map requestHeaders) { + this("", "", requestHeaders); + } + + public AuthenticationRequest(@Nonnull String servletInfo, @Nonnull String pathInfo, @Nonnull final Map requestHeaders) { Objects.requireNonNull(requestHeaders); caseInsensitiveHeaders = new TreeMap<>(String.CASE_INSENSITIVE_ORDER); caseInsensitiveHeaders.putAll(requestHeaders); + this.servletInfo = servletInfo; + this.pathInfo = pathInfo; } /** diff --git a/metadata-service/auth-filter/src/main/java/com/datahub/auth/authentication/filter/AuthenticationFilter.java b/metadata-service/auth-filter/src/main/java/com/datahub/auth/authentication/filter/AuthenticationFilter.java index e15918a813158..8c7b3ac8b98f0 100644 --- a/metadata-service/auth-filter/src/main/java/com/datahub/auth/authentication/filter/AuthenticationFilter.java +++ b/metadata-service/auth-filter/src/main/java/com/datahub/auth/authentication/filter/AuthenticationFilter.java @@ -2,6 +2,7 @@ import com.datahub.authentication.authenticator.AuthenticatorChain; import com.datahub.authentication.authenticator.DataHubSystemAuthenticator; +import com.datahub.authentication.authenticator.HealthStatusAuthenticator; import com.datahub.authentication.authenticator.NoOpAuthenticator; import com.datahub.authentication.token.StatefulTokenService; import com.datahub.plugins.PluginConstant; @@ -29,6 +30,7 @@ import java.nio.file.Path; import java.nio.file.Paths; import java.util.Collections; +import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Optional; @@ -148,7 +150,7 @@ private void buildAuthenticatorChain() { } private AuthenticationRequest buildAuthContext(HttpServletRequest request) { - return new AuthenticationRequest(Collections.list(request.getHeaderNames()) + return new AuthenticationRequest(request.getServletPath(), request.getPathInfo(), Collections.list(request.getHeaderNames()) .stream() .collect(Collectors.toMap(headerName -> headerName, request::getHeader))); } @@ -242,7 +244,14 @@ private void registerNativeAuthenticator(AuthenticatorChain authenticatorChain, final Authenticator authenticator = clazz.newInstance(); // Successfully created authenticator. Now init and register it. log.debug(String.format("Initializing Authenticator with name %s", type)); - authenticator.init(configs, authenticatorContext); + if (authenticator instanceof HealthStatusAuthenticator) { + Map authenticatorConfig = new HashMap<>(Map.of(SYSTEM_CLIENT_ID_CONFIG, + this.configurationProvider.getAuthentication().getSystemClientId())); + authenticatorConfig.putAll(Optional.ofNullable(internalAuthenticatorConfig.getConfigs()).orElse(Collections.emptyMap())); + authenticator.init(authenticatorConfig, authenticatorContext); + } else { + authenticator.init(configs, authenticatorContext); + } log.info(String.format("Registering Authenticator with name %s", type)); authenticatorChain.register(authenticator); } catch (Exception e) { diff --git a/metadata-service/auth-impl/src/main/java/com/datahub/authentication/authenticator/HealthStatusAuthenticator.java b/metadata-service/auth-impl/src/main/java/com/datahub/authentication/authenticator/HealthStatusAuthenticator.java new file mode 100644 index 0000000000000..5749eacf5d25d --- /dev/null +++ b/metadata-service/auth-impl/src/main/java/com/datahub/authentication/authenticator/HealthStatusAuthenticator.java @@ -0,0 +1,55 @@ +package com.datahub.authentication.authenticator; + +import com.datahub.authentication.Actor; +import com.datahub.authentication.ActorType; +import com.datahub.authentication.Authentication; +import com.datahub.authentication.AuthenticationException; +import com.datahub.authentication.AuthenticationRequest; +import com.datahub.authentication.AuthenticatorContext; +import com.datahub.plugins.auth.authentication.Authenticator; +import lombok.extern.slf4j.Slf4j; + +import javax.annotation.Nonnull; +import javax.annotation.Nullable; +import java.util.Collections; +import java.util.Map; +import java.util.Objects; +import java.util.Set; + +import static com.datahub.authentication.AuthenticationConstants.SYSTEM_CLIENT_ID_CONFIG; + + +/** + * This Authenticator is used for allowing access for unauthenticated health check endpoints + * + * It exists to support load balancers, liveness/readiness checks + * + */ +@Slf4j +public class HealthStatusAuthenticator implements Authenticator { + private static final Set HEALTH_ENDPOINTS = Set.of( + "/openapi/check/", + "/openapi/up/" + ); + private String systemClientId; + + @Override + public void init(@Nonnull final Map config, @Nullable final AuthenticatorContext context) { + Objects.requireNonNull(config, "Config parameter cannot be null"); + this.systemClientId = Objects.requireNonNull((String) config.get(SYSTEM_CLIENT_ID_CONFIG), + String.format("Missing required config %s", SYSTEM_CLIENT_ID_CONFIG)); + } + + @Override + public Authentication authenticate(@Nonnull AuthenticationRequest context) throws AuthenticationException { + Objects.requireNonNull(context); + if (HEALTH_ENDPOINTS.stream().anyMatch(prefix -> String.join("", context.getServletInfo(), context.getPathInfo()).startsWith(prefix))) { + return new Authentication( + new Actor(ActorType.USER, systemClientId), + "", + Collections.emptyMap() + ); + } + throw new AuthenticationException("Authorization not allowed. Non-health check endpoint."); + } +} diff --git a/metadata-service/configuration/src/main/resources/application.yml b/metadata-service/configuration/src/main/resources/application.yml index b817208672e08..91b10a75c922e 100644 --- a/metadata-service/configuration/src/main/resources/application.yml +++ b/metadata-service/configuration/src/main/resources/application.yml @@ -11,6 +11,8 @@ authentication: # Key used to validate incoming tokens. Should typically be the same as authentication.tokenService.signingKey signingKey: ${DATAHUB_TOKEN_SERVICE_SIGNING_KEY:WnEdIeTG/VVCLQqGwC/BAkqyY0k+H8NEAtWGejrBI94=} salt: ${DATAHUB_TOKEN_SERVICE_SALT:ohDVbJBvHHVJh9S/UA4BYF9COuNnqqVhr9MLKEGXk1O=} + # Required for unauthenticated health check endpoints - best not to remove. + - type: com.datahub.authentication.authenticator.HealthStatusAuthenticator # Normally failures are only warnings, enable this to throw them. logAuthenticatorExceptions: ${METADATA_SERVICE_AUTHENTICATOR_EXCEPTIONS_ENABLED:false} diff --git a/metadata-service/health-servlet/build.gradle b/metadata-service/health-servlet/build.gradle deleted file mode 100644 index 6095f724b3cd4..0000000000000 --- a/metadata-service/health-servlet/build.gradle +++ /dev/null @@ -1,22 +0,0 @@ -apply plugin: 'java' - -dependencies { - - implementation project(':metadata-service:factories') - - implementation externalDependency.guava - implementation externalDependency.reflections - implementation externalDependency.springBoot - implementation externalDependency.springCore - implementation externalDependency.springDocUI - implementation externalDependency.springWeb - implementation externalDependency.springWebMVC - implementation externalDependency.springBeans - implementation externalDependency.springContext - implementation externalDependency.slf4jApi - compileOnly externalDependency.lombok - implementation externalDependency.antlr4Runtime - implementation externalDependency.antlr4 - - annotationProcessor externalDependency.lombok -} \ No newline at end of file diff --git a/metadata-service/openapi-servlet/src/main/java/io/datahubproject/openapi/config/SpringWebConfig.java b/metadata-service/openapi-servlet/src/main/java/io/datahubproject/openapi/config/SpringWebConfig.java index 71e8c79a2275a..e4f49df90c392 100644 --- a/metadata-service/openapi-servlet/src/main/java/io/datahubproject/openapi/config/SpringWebConfig.java +++ b/metadata-service/openapi-servlet/src/main/java/io/datahubproject/openapi/config/SpringWebConfig.java @@ -44,7 +44,6 @@ public GroupedOpenApi defaultOpenApiGroup() { .group("default") .packagesToExclude( "io.datahubproject.openapi.operations", - "com.datahub.health", "io.datahubproject.openapi.health" ).build(); } @@ -55,7 +54,6 @@ public GroupedOpenApi operationsOpenApiGroup() { .group("operations") .packagesToScan( "io.datahubproject.openapi.operations", - "com.datahub.health", "io.datahubproject.openapi.health" ).build(); } diff --git a/metadata-service/health-servlet/src/main/java/com/datahub/health/controller/HealthCheckController.java b/metadata-service/openapi-servlet/src/main/java/io/datahubproject/openapi/health/HealthCheckController.java similarity index 79% rename from metadata-service/health-servlet/src/main/java/com/datahub/health/controller/HealthCheckController.java rename to metadata-service/openapi-servlet/src/main/java/io/datahubproject/openapi/health/HealthCheckController.java index c200e63e0d497..c90603bf88c31 100644 --- a/metadata-service/health-servlet/src/main/java/com/datahub/health/controller/HealthCheckController.java +++ b/metadata-service/openapi-servlet/src/main/java/io/datahubproject/openapi/health/HealthCheckController.java @@ -1,5 +1,6 @@ -package com.datahub.health.controller; +package io.datahubproject.openapi.health; +import com.google.common.base.Supplier; import com.google.common.base.Suppliers; import com.linkedin.gms.factory.config.ConfigurationProvider; import io.swagger.v3.oas.annotations.tags.Tag; @@ -9,7 +10,6 @@ import java.util.List; import java.util.Map; import java.util.concurrent.TimeUnit; -import java.util.function.Supplier; import org.opensearch.action.admin.cluster.health.ClusterHealthRequest; import org.opensearch.action.admin.cluster.health.ClusterHealthResponse; @@ -27,7 +27,7 @@ @RestController -@RequestMapping("/check") +@RequestMapping("/") @Tag(name = "HealthCheck", description = "An API for checking health of GMS and its clients.") public class HealthCheckController { @Autowired @@ -41,6 +41,12 @@ public HealthCheckController(ConfigurationProvider config) { this::getElasticHealth, config.getHealthCheck().getCacheDurationSeconds(), TimeUnit.SECONDS); } + @GetMapping(path = "/check/ready", produces = MediaType.APPLICATION_JSON_VALUE) + public ResponseEntity getCombinedHealthCheck(String... checks) { + return ResponseEntity.status(getCombinedDebug(checks).getStatusCode()) + .body(getCombinedDebug(checks).getStatusCode().is2xxSuccessful()); + } + /** * Combined health check endpoint for checking GMS clients. * For now, just checks the health of the ElasticSearch client @@ -48,11 +54,10 @@ public HealthCheckController(ConfigurationProvider config) { * that component). The status code will be 200 if all components are okay, and 500 if one or more components are not * healthy. */ - @GetMapping(path = "/ready", produces = MediaType.APPLICATION_JSON_VALUE) - public ResponseEntity>> getCombinedHealthCheck(String... checks) { - + @GetMapping(path = "/debug/ready", produces = MediaType.APPLICATION_JSON_VALUE) + public ResponseEntity>> getCombinedDebug(String... checks) { Map>> healthChecks = new HashMap<>(); - healthChecks.put("elasticsearch", this::getElasticHealthWithCache); + healthChecks.put("elasticsearch", this::getElasticDebugWithCache); // Add new components here List componentsToCheck = checks != null && checks.length > 0 @@ -67,7 +72,6 @@ public ResponseEntity>> getCombinedHealthChec .get()); } - boolean isHealthy = componentHealth.values().stream().allMatch(resp -> resp.getStatusCode() == HttpStatus.OK); if (isHealthy) { return ResponseEntity.ok(componentHealth); @@ -75,12 +79,18 @@ public ResponseEntity>> getCombinedHealthChec return ResponseEntity.status(HttpStatus.SERVICE_UNAVAILABLE).body(componentHealth); } + @GetMapping(path = "/check/elastic", produces = MediaType.APPLICATION_JSON_VALUE) + public ResponseEntity getElasticHealthWithCache() { + return ResponseEntity.status(getElasticDebugWithCache().getStatusCode()) + .body(getElasticDebugWithCache().getStatusCode().is2xxSuccessful()); + } + /** * Checks the memoized cache for the latest elastic health check result * @return The ResponseEntity containing the health check result */ - @GetMapping(path = "/elastic", produces = MediaType.APPLICATION_JSON_VALUE) - public ResponseEntity getElasticHealthWithCache() { + @GetMapping(path = "/debug/elastic", produces = MediaType.APPLICATION_JSON_VALUE) + public ResponseEntity getElasticDebugWithCache() { return this.memoizedSupplier.get(); } diff --git a/metadata-service/war/build.gradle b/metadata-service/war/build.gradle index 122c2b9d5357b..54e95fdcfe579 100644 --- a/metadata-service/war/build.gradle +++ b/metadata-service/war/build.gradle @@ -17,7 +17,6 @@ dependencies { runtimeOnly project(':metadata-service:servlet') runtimeOnly project(':metadata-service:auth-servlet-impl') runtimeOnly project(':metadata-service:graphql-servlet-impl') - runtimeOnly project(':metadata-service:health-servlet') runtimeOnly project(':metadata-service:openapi-servlet') runtimeOnly project(':metadata-service:openapi-entity-servlet') runtimeOnly project(':metadata-service:openapi-analytics-servlet') diff --git a/metadata-service/war/src/main/webapp/WEB-INF/openapiServlet-servlet.xml b/metadata-service/war/src/main/webapp/WEB-INF/openapiServlet-servlet.xml index 3077cfb062638..fb2bc6c0336cd 100644 --- a/metadata-service/war/src/main/webapp/WEB-INF/openapiServlet-servlet.xml +++ b/metadata-service/war/src/main/webapp/WEB-INF/openapiServlet-servlet.xml @@ -3,7 +3,7 @@ xmlns:context="http://www.springframework.org/schema/context" xsi:schemaLocation="http://www.springframework.org/schema/beans http://www.springframework.org/schema/beans/spring-beans-3.0.xsd http://www.springframework.org/schema/context http://www.springframework.org/schema/context/spring-context-3.0.xsd"> - + diff --git a/settings.gradle b/settings.gradle index 52de461383b5e..d2844fe00cdbc 100644 --- a/settings.gradle +++ b/settings.gradle @@ -8,7 +8,6 @@ include 'metadata-service:auth-config' include 'metadata-service:auth-impl' include 'metadata-service:auth-filter' include 'metadata-service:auth-servlet-impl' -include 'metadata-service:health-servlet' include 'metadata-service:restli-api' include 'metadata-service:restli-client' include 'metadata-service:restli-servlet-impl'