From 88be9f681bf26c45a201fe4ff2937631b0e25d54 Mon Sep 17 00:00:00 2001 From: Jonathan Wei Date: Mon, 16 Apr 2018 18:09:56 -0700 Subject: [PATCH 1/3] Fix HTTP OPTIONS request auth handling (#5638) * Fix HTTP OPTIONS request auth handling * PR comment * More PR comments * Fix * PR comment --- docs/content/configuration/auth.md | 1 + .../ITBasicAuthConfigurationTest.java | 12 +++ .../security/AllowOptionsResourceFilter.java | 84 +++++++++++++++++++ .../io/druid/server/security/AuthConfig.java | 36 ++++---- .../server/security/AuthenticationUtils.java | 10 +++ .../main/java/io/druid/cli/CliOverlord.java | 2 + .../CoordinatorJettyServerInitializer.java | 3 +- .../MiddleManagerJettyServerInitializer.java | 1 + .../cli/QueryJettyServerInitializer.java | 2 + .../cli/RouterJettyServerInitializer.java | 2 + 10 files changed, 136 insertions(+), 17 deletions(-) create mode 100644 server/src/main/java/io/druid/server/security/AllowOptionsResourceFilter.java diff --git a/docs/content/configuration/auth.md b/docs/content/configuration/auth.md index 358a54431b80..2fecfc1a86c3 100644 --- a/docs/content/configuration/auth.md +++ b/docs/content/configuration/auth.md @@ -9,6 +9,7 @@ layout: doc_page |`druid.auth.authenticationChain`|JSON List of Strings|List of Authenticator type names|["allowAll"]|no| |`druid.escalator.type`|String|Type of the Escalator that should be used for internal Druid communications. This Escalator must use an authentication scheme that is supported by an Authenticator in `druid.auth.authenticationChain`.|"noop"|no| |`druid.auth.authorizers`|JSON List of Strings|List of Authorizer type names |["allowAll"]|no| +|`druid.auth.allowUnauthenticatedHttpOptions`|Boolean|If true, skip authentication checks for HTTP OPTIONS requests. This is needed for certain use cases, such as supporting CORS pre-flight requests. Note that disabling authentication checks for OPTIONS requests will allow unauthenticated users to determine what Druid endpoints are valid (by checking if the OPTIONS request returns a 200 instead of 404), so enabling this option may reveal information about server configuration, including information about what extensions are loaded (if those extensions add endpoints).|false|no| ## Enabling Authentication/Authorization diff --git a/integration-tests/src/test/java/io/druid/tests/security/ITBasicAuthConfigurationTest.java b/integration-tests/src/test/java/io/druid/tests/security/ITBasicAuthConfigurationTest.java index dd9344846f11..572b565fbb9b 100644 --- a/integration-tests/src/test/java/io/druid/tests/security/ITBasicAuthConfigurationTest.java +++ b/integration-tests/src/test/java/io/druid/tests/security/ITBasicAuthConfigurationTest.java @@ -219,6 +219,18 @@ public void testAuthConfiguration() throws Exception LOG.info("Testing Avatica query on router with incorrect credentials."); testAvaticaAuthFailure(routerUrl); + + LOG.info("Checking OPTIONS requests on services..."); + testOptionsRequests(adminClient); + } + + private void testOptionsRequests(HttpClient httpClient) + { + makeRequest(httpClient, HttpMethod.OPTIONS, config.getCoordinatorUrl() + "/status", null); + makeRequest(httpClient, HttpMethod.OPTIONS, config.getIndexerUrl() + "/status", null); + makeRequest(httpClient, HttpMethod.OPTIONS, config.getBrokerUrl() + "/status", null); + makeRequest(httpClient, HttpMethod.OPTIONS, config.getHistoricalUrl() + "/status", null); + makeRequest(httpClient, HttpMethod.OPTIONS, config.getRouterUrl() + "/status", null); } private void testAvaticaQuery(String url) diff --git a/server/src/main/java/io/druid/server/security/AllowOptionsResourceFilter.java b/server/src/main/java/io/druid/server/security/AllowOptionsResourceFilter.java new file mode 100644 index 000000000000..9ec87415593a --- /dev/null +++ b/server/src/main/java/io/druid/server/security/AllowOptionsResourceFilter.java @@ -0,0 +1,84 @@ +/* + * Licensed to Metamarkets Group Inc. (Metamarkets) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. Metamarkets licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package io.druid.server.security; + +import javax.servlet.Filter; +import javax.servlet.FilterChain; +import javax.servlet.FilterConfig; +import javax.servlet.ServletException; +import javax.servlet.ServletRequest; +import javax.servlet.ServletResponse; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; +import javax.ws.rs.HttpMethod; +import java.io.IOException; + +public class AllowOptionsResourceFilter implements Filter +{ + private final boolean allowUnauthenticatedHttpOptions; + + public AllowOptionsResourceFilter( + boolean allowUnauthenticatedHttpOptions + ) + { + this.allowUnauthenticatedHttpOptions = allowUnauthenticatedHttpOptions; + } + + @Override + public void init(FilterConfig filterConfig) throws ServletException + { + + } + + @Override + public void doFilter( + ServletRequest request, ServletResponse response, FilterChain chain + ) throws IOException, ServletException + { + HttpServletRequest httpReq = (HttpServletRequest) request; + + // Druid itself doesn't explictly handle OPTIONS requests, no resource handler will authorize such requests. + // so this filter catches all OPTIONS requests and authorizes them. + if (HttpMethod.OPTIONS.equals(httpReq.getMethod())) { + if (httpReq.getAttribute(AuthConfig.DRUID_AUTHENTICATION_RESULT) == null) { + // If the request already had credentials and authenticated successfully, keep the authenticated identity. + // Otherwise, allow the unauthenticated request. + if (allowUnauthenticatedHttpOptions) { + httpReq.setAttribute( + AuthConfig.DRUID_AUTHENTICATION_RESULT, + new AuthenticationResult(AuthConfig.ALLOW_ALL_NAME, AuthConfig.ALLOW_ALL_NAME, null) + ); + } else { + ((HttpServletResponse) response).sendError(HttpServletResponse.SC_UNAUTHORIZED); + } + } + + httpReq.setAttribute(AuthConfig.DRUID_AUTHORIZATION_CHECKED, true); + } + + chain.doFilter(request, response); + } + + @Override + public void destroy() + { + + } +} diff --git a/server/src/main/java/io/druid/server/security/AuthConfig.java b/server/src/main/java/io/druid/server/security/AuthConfig.java index af4768a4d5e0..9fe71afef9bc 100644 --- a/server/src/main/java/io/druid/server/security/AuthConfig.java +++ b/server/src/main/java/io/druid/server/security/AuthConfig.java @@ -23,6 +23,7 @@ import com.fasterxml.jackson.annotation.JsonProperty; import java.util.List; +import java.util.Objects; public class AuthConfig { @@ -40,17 +41,19 @@ public class AuthConfig public AuthConfig() { - this(null, null); + this(null, null, false); } @JsonCreator public AuthConfig( @JsonProperty("authenticatorChain") List authenticationChain, - @JsonProperty("authorizers") List authorizers + @JsonProperty("authorizers") List authorizers, + @JsonProperty("allowUnauthenticatedHttpOptions") boolean allowUnauthenticatedHttpOptions ) { this.authenticatorChain = authenticationChain; this.authorizers = authorizers; + this.allowUnauthenticatedHttpOptions = allowUnauthenticatedHttpOptions; } @JsonProperty @@ -59,6 +62,9 @@ public AuthConfig( @JsonProperty private List authorizers; + @JsonProperty + private final boolean allowUnauthenticatedHttpOptions; + public List getAuthenticatorChain() { return authenticatorChain; @@ -69,12 +75,18 @@ public List getAuthorizers() return authorizers; } + public boolean isAllowUnauthenticatedHttpOptions() + { + return allowUnauthenticatedHttpOptions; + } + @Override public String toString() { return "AuthConfig{" + - "authenticatorChain='" + authenticatorChain + '\'' + - ", authorizers='" + authorizers + '\'' + + "authenticatorChain=" + authenticatorChain + + ", authorizers=" + authorizers + + ", allowUnauthenticatedHttpOptions=" + allowUnauthenticatedHttpOptions + '}'; } @@ -87,23 +99,15 @@ public boolean equals(Object o) if (o == null || getClass() != o.getClass()) { return false; } - AuthConfig that = (AuthConfig) o; - - if (getAuthenticatorChain() != null - ? !getAuthenticatorChain().equals(that.getAuthenticatorChain()) - : that.getAuthenticatorChain() != null) { - return false; - } - return getAuthorizers() != null ? getAuthorizers().equals(that.getAuthorizers()) : that.getAuthorizers() == null; - + return isAllowUnauthenticatedHttpOptions() == that.isAllowUnauthenticatedHttpOptions() && + Objects.equals(getAuthenticatorChain(), that.getAuthenticatorChain()) && + Objects.equals(getAuthorizers(), that.getAuthorizers()); } @Override public int hashCode() { - int result = getAuthenticatorChain() != null ? getAuthenticatorChain().hashCode() : 0; - result = 31 * result + (getAuthorizers() != null ? getAuthorizers().hashCode() : 0); - return result; + return Objects.hash(getAuthenticatorChain(), getAuthorizers(), isAllowUnauthenticatedHttpOptions()); } } diff --git a/server/src/main/java/io/druid/server/security/AuthenticationUtils.java b/server/src/main/java/io/druid/server/security/AuthenticationUtils.java index cabaa828274c..bd6b2be7680f 100644 --- a/server/src/main/java/io/druid/server/security/AuthenticationUtils.java +++ b/server/src/main/java/io/druid/server/security/AuthenticationUtils.java @@ -27,6 +27,16 @@ public class AuthenticationUtils { + public static void addAllowOptionsFilter(ServletContextHandler root, boolean allowUnauthenticatedHttpOptions) + { + FilterHolder holder = new FilterHolder(new AllowOptionsResourceFilter(allowUnauthenticatedHttpOptions)); + root.addFilter( + holder, + "/*", + null + ); + } + public static void addAuthenticationFilterChain( ServletContextHandler root, List authenticators diff --git a/services/src/main/java/io/druid/cli/CliOverlord.java b/services/src/main/java/io/druid/cli/CliOverlord.java index f070c8b5cdbb..829c1a3e7d4d 100644 --- a/services/src/main/java/io/druid/cli/CliOverlord.java +++ b/services/src/main/java/io/druid/cli/CliOverlord.java @@ -328,6 +328,8 @@ public void initialize(Server server, Injector injector) authenticators = authenticatorMapper.getAuthenticatorChain(); AuthenticationUtils.addAuthenticationFilterChain(root, authenticators); + AuthenticationUtils.addAllowOptionsFilter(root, authConfig.isAllowUnauthenticatedHttpOptions()); + JettyServerInitUtils.addExtensionFilters(root, injector); diff --git a/services/src/main/java/io/druid/cli/CoordinatorJettyServerInitializer.java b/services/src/main/java/io/druid/cli/CoordinatorJettyServerInitializer.java index 5aefbd4182c8..572a06296f48 100644 --- a/services/src/main/java/io/druid/cli/CoordinatorJettyServerInitializer.java +++ b/services/src/main/java/io/druid/cli/CoordinatorJettyServerInitializer.java @@ -125,8 +125,9 @@ public void initialize(Server server, Injector injector) authenticators = authenticatorMapper.getAuthenticatorChain(); AuthenticationUtils.addAuthenticationFilterChain(root, authenticators); - JettyServerInitUtils.addExtensionFilters(root, injector); + AuthenticationUtils.addAllowOptionsFilter(root, authConfig.isAllowUnauthenticatedHttpOptions()); + JettyServerInitUtils.addExtensionFilters(root, injector); // Check that requests were authorized before sending responses AuthenticationUtils.addPreResponseAuthorizationCheckFilter( diff --git a/services/src/main/java/io/druid/cli/MiddleManagerJettyServerInitializer.java b/services/src/main/java/io/druid/cli/MiddleManagerJettyServerInitializer.java index 2eaa4d154fe1..bc4d3a54ed92 100644 --- a/services/src/main/java/io/druid/cli/MiddleManagerJettyServerInitializer.java +++ b/services/src/main/java/io/druid/cli/MiddleManagerJettyServerInitializer.java @@ -71,6 +71,7 @@ public void initialize(Server server, Injector injector) authenticators = authenticatorMapper.getAuthenticatorChain(); AuthenticationUtils.addAuthenticationFilterChain(root, authenticators); + AuthenticationUtils.addAllowOptionsFilter(root, authConfig.isAllowUnauthenticatedHttpOptions()); JettyServerInitUtils.addExtensionFilters(root, injector); diff --git a/services/src/main/java/io/druid/cli/QueryJettyServerInitializer.java b/services/src/main/java/io/druid/cli/QueryJettyServerInitializer.java index 0bc34f17fee9..158495b8544f 100644 --- a/services/src/main/java/io/druid/cli/QueryJettyServerInitializer.java +++ b/services/src/main/java/io/druid/cli/QueryJettyServerInitializer.java @@ -99,6 +99,8 @@ public void initialize(Server server, Injector injector) authenticators = authenticatorMapper.getAuthenticatorChain(); AuthenticationUtils.addAuthenticationFilterChain(root, authenticators); + AuthenticationUtils.addAllowOptionsFilter(root, authConfig.isAllowUnauthenticatedHttpOptions()); + JettyServerInitUtils.addExtensionFilters(root, injector); // Check that requests were authorized before sending responses diff --git a/services/src/main/java/io/druid/cli/RouterJettyServerInitializer.java b/services/src/main/java/io/druid/cli/RouterJettyServerInitializer.java index abe20a2d66e2..930d1191e828 100644 --- a/services/src/main/java/io/druid/cli/RouterJettyServerInitializer.java +++ b/services/src/main/java/io/druid/cli/RouterJettyServerInitializer.java @@ -105,6 +105,8 @@ public void initialize(Server server, Injector injector) authenticators = authenticatorMapper.getAuthenticatorChain(); AuthenticationUtils.addAuthenticationFilterChain(root, authenticators); + AuthenticationUtils.addAllowOptionsFilter(root, authConfig.isAllowUnauthenticatedHttpOptions()); + JettyServerInitUtils.addExtensionFilters(root, injector); // Check that requests were authorized before sending responses From 5a4235d1a8e143ae7c8114b69be50ec86d5d3a63 Mon Sep 17 00:00:00 2001 From: jon-wei Date: Tue, 17 Apr 2018 00:08:04 -0700 Subject: [PATCH 2/3] Compile fixes --- .../test/java/io/druid/server/QueryResourceTest.java | 12 ++++++------ .../druid/server/http/DatasourcesResourceTest.java | 2 +- .../http/security/ResourceFilterTestHelper.java | 2 +- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/server/src/test/java/io/druid/server/QueryResourceTest.java b/server/src/test/java/io/druid/server/QueryResourceTest.java index b099f002d267..945aa7ec39b7 100644 --- a/server/src/test/java/io/druid/server/QueryResourceTest.java +++ b/server/src/test/java/io/druid/server/QueryResourceTest.java @@ -247,13 +247,13 @@ public Access authorize(AuthenticationResult authenticationResult, Resource reso new DefaultGenericQueryMetricsFactory(jsonMapper), new NoopServiceEmitter(), testRequestLogger, - new AuthConfig(null, null), + new AuthConfig(), authMapper ), jsonMapper, jsonMapper, queryManager, - new AuthConfig(null, null), + new AuthConfig(), authMapper, new DefaultGenericQueryMetricsFactory(jsonMapper) ); @@ -354,13 +354,13 @@ public Access authorize(AuthenticationResult authenticationResult, Resource reso new DefaultGenericQueryMetricsFactory(jsonMapper), new NoopServiceEmitter(), testRequestLogger, - new AuthConfig(null, null), + new AuthConfig(), authMapper ), jsonMapper, jsonMapper, queryManager, - new AuthConfig(null, null), + new AuthConfig(), authMapper, new DefaultGenericQueryMetricsFactory(jsonMapper) ); @@ -475,13 +475,13 @@ public Access authorize(AuthenticationResult authenticationResult, Resource reso new DefaultGenericQueryMetricsFactory(jsonMapper), new NoopServiceEmitter(), testRequestLogger, - new AuthConfig(null, null), + new AuthConfig(), authMapper ), jsonMapper, jsonMapper, queryManager, - new AuthConfig(null, null), + new AuthConfig(), authMapper, new DefaultGenericQueryMetricsFactory(jsonMapper) ); diff --git a/server/src/test/java/io/druid/server/http/DatasourcesResourceTest.java b/server/src/test/java/io/druid/server/http/DatasourcesResourceTest.java index 96e0b583f2e0..0e5e09939f8e 100644 --- a/server/src/test/java/io/druid/server/http/DatasourcesResourceTest.java +++ b/server/src/test/java/io/druid/server/http/DatasourcesResourceTest.java @@ -236,7 +236,7 @@ public Access authorize(AuthenticationResult authenticationResult1, Resource res inventoryView, null, null, - new AuthConfig(null, null), + new AuthConfig(), authMapper ); Response response = datasourcesResource.getQueryableDataSources("full", null, request); diff --git a/server/src/test/java/io/druid/server/http/security/ResourceFilterTestHelper.java b/server/src/test/java/io/druid/server/http/security/ResourceFilterTestHelper.java index ed39de6452ef..4e66f72d5664 100644 --- a/server/src/test/java/io/druid/server/http/security/ResourceFilterTestHelper.java +++ b/server/src/test/java/io/druid/server/http/security/ResourceFilterTestHelper.java @@ -182,7 +182,7 @@ public void configure(Binder binder) for (Key key : mockableKeys) { binder.bind((Key) key).toInstance(EasyMock.createNiceMock(key.getTypeLiteral().getRawType())); } - binder.bind(AuthConfig.class).toInstance(new AuthConfig(null, null)); + binder.bind(AuthConfig.class).toInstance(new AuthConfig()); } } ); From 1f6b818583fa01eef44ec8334176f58be0c0ae03 Mon Sep 17 00:00:00 2001 From: jon-wei Date: Tue, 17 Apr 2018 10:32:37 -0700 Subject: [PATCH 3/3] Compile --- services/src/main/java/io/druid/cli/CliOverlord.java | 2 ++ .../src/main/java/io/druid/cli/QueryJettyServerInitializer.java | 2 ++ 2 files changed, 4 insertions(+) diff --git a/services/src/main/java/io/druid/cli/CliOverlord.java b/services/src/main/java/io/druid/cli/CliOverlord.java index 829c1a3e7d4d..d04bc7b175da 100644 --- a/services/src/main/java/io/druid/cli/CliOverlord.java +++ b/services/src/main/java/io/druid/cli/CliOverlord.java @@ -89,6 +89,7 @@ import io.druid.server.http.RedirectInfo; import io.druid.server.initialization.jetty.JettyServerInitUtils; import io.druid.server.initialization.jetty.JettyServerInitializer; +import io.druid.server.security.AuthConfig; import io.druid.server.security.AuthenticationUtils; import io.druid.server.security.Authenticator; import io.druid.server.security.AuthenticatorMapper; @@ -318,6 +319,7 @@ public void initialize(Server server, Injector injector) final ObjectMapper jsonMapper = injector.getInstance(Key.get(ObjectMapper.class, Json.class)); final AuthenticatorMapper authenticatorMapper = injector.getInstance(AuthenticatorMapper.class); + final AuthConfig authConfig = injector.getInstance(AuthConfig.class); List authenticators = null; AuthenticationUtils.addSecuritySanityCheckFilter(root, jsonMapper); diff --git a/services/src/main/java/io/druid/cli/QueryJettyServerInitializer.java b/services/src/main/java/io/druid/cli/QueryJettyServerInitializer.java index 158495b8544f..8fa30b1c36cb 100644 --- a/services/src/main/java/io/druid/cli/QueryJettyServerInitializer.java +++ b/services/src/main/java/io/druid/cli/QueryJettyServerInitializer.java @@ -33,6 +33,7 @@ import io.druid.server.initialization.jetty.JettyServerInitUtils; import io.druid.server.initialization.jetty.JettyServerInitializer; import io.druid.server.initialization.jetty.LimitRequestsFilter; +import io.druid.server.security.AuthConfig; import io.druid.server.security.AuthenticationUtils; import io.druid.server.security.Authenticator; import io.druid.server.security.AuthenticatorMapper; @@ -89,6 +90,7 @@ public void initialize(Server server, Injector injector) final ObjectMapper jsonMapper = injector.getInstance(Key.get(ObjectMapper.class, Json.class)); final AuthenticatorMapper authenticatorMapper = injector.getInstance(AuthenticatorMapper.class); + final AuthConfig authConfig = injector.getInstance(AuthConfig.class); List authenticators = null; AuthenticationUtils.addSecuritySanityCheckFilter(root, jsonMapper);