diff --git a/server/src/main/java/org/elasticsearch/rest/RestController.java b/server/src/main/java/org/elasticsearch/rest/RestController.java index cc91dee8535b8..207cda2e5d8ed 100644 --- a/server/src/main/java/org/elasticsearch/rest/RestController.java +++ b/server/src/main/java/org/elasticsearch/rest/RestController.java @@ -263,6 +263,8 @@ private void registerHandlerNoWrap(RestRequest.Method method, String path, RestA if (RESERVED_PATHS.contains(path)) { throw new IllegalArgumentException("path [" + path + "] is a reserved path and may not be registered"); } + // the HTTP OPTIONS method is treated internally, not by handlers, see {@code #handleNoHandlerFound} + assert method != RestRequest.Method.OPTIONS : "There should be no handlers registered for the OPTIONS HTTP method"; handlers.insertOrUpdate( path, new MethodHandlers(path).addMethod(method, version, handler), diff --git a/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java b/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java index 499d3d5e03eb9..b1c4aad730ca1 100644 --- a/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java +++ b/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java @@ -88,6 +88,7 @@ public class RestControllerTests extends ESTestCase { private UsageService usageService; private NodeClient client; private Tracer tracer; + private List methodList; @Before public void setup() { @@ -119,6 +120,7 @@ public void setup() { (request, channel, client) -> { throw new IllegalArgumentException("test error"); } ); httpServerTransport.start(); + methodList = Arrays.stream(RestRequest.Method.values()).filter(m -> m != OPTIONS).toList(); } @After @@ -207,7 +209,7 @@ public void testRequestWithDisallowedMultiValuedHeaderButSameValues() { public void testRegisterAsDeprecatedHandler() { RestController controller = mock(RestController.class); - RestRequest.Method method = randomFrom(RestRequest.Method.values()); + RestRequest.Method method = randomFrom(methodList); String path = "/_" + randomAlphaOfLengthBetween(1, 6); RestHandler handler = (request, channel, client) -> {}; String deprecationMessage = randomAlphaOfLengthBetween(1, 10); @@ -228,10 +230,10 @@ public void testRegisterAsDeprecatedHandler() { public void testRegisterAsReplacedHandler() { final RestController controller = mock(RestController.class); - final RestRequest.Method method = randomFrom(RestRequest.Method.values()); + final RestRequest.Method method = randomFrom(methodList); final String path = "/_" + randomAlphaOfLengthBetween(1, 6); final RestHandler handler = (request, channel, client) -> {}; - final RestRequest.Method replacedMethod = randomFrom(RestRequest.Method.values()); + final RestRequest.Method replacedMethod = randomFrom(methodList); final String replacedPath = "/_" + randomAlphaOfLengthBetween(1, 6); final RestApiVersion current = RestApiVersion.current(); final RestApiVersion previous = RestApiVersion.previous(); @@ -261,8 +263,8 @@ public void testRegisterAsReplacedHandler() { public void testRegisterSecondMethodWithDifferentNamedWildcard() { final RestController restController = new RestController(null, null, circuitBreakerService, usageService, tracer, false); - RestRequest.Method firstMethod = randomFrom(RestRequest.Method.values()); - RestRequest.Method secondMethod = randomFrom(Arrays.stream(RestRequest.Method.values()).filter(m -> m != firstMethod).toList()); + RestRequest.Method firstMethod = randomFrom(methodList); + RestRequest.Method secondMethod = randomFrom(methodList.stream().filter(m -> m != firstMethod).toList()); final String path = "/_" + randomAlphaOfLengthBetween(1, 6); @@ -582,7 +584,7 @@ public void testDispatchBadRequest() { } public void testDoesNotConsumeContent() throws Exception { - final RestRequest.Method method = randomFrom(RestRequest.Method.values()); + final RestRequest.Method method = randomFrom(methodList); restController.registerHandler(new Route(method, "/notconsumed"), new RestHandler() { @Override public void handleRequest(RestRequest request, RestChannel channel, NodeClient client) throws Exception { @@ -895,7 +897,7 @@ public void testRegisterWithReservedPath() { for (String path : RestController.RESERVED_PATHS) { IllegalArgumentException iae = expectThrows(IllegalArgumentException.class, () -> { restController.registerHandler( - new Route(randomFrom(RestRequest.Method.values()), path), + new Route(randomFrom(methodList), path), (request, channel, client) -> channel.sendResponse( new RestResponse(RestStatus.OK, RestResponse.TEXT_CONTENT_TYPE, BytesArray.EMPTY) ) diff --git a/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/xpack/security/authc/HttOptionsNoAuthnIntegTests.java b/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/xpack/security/authc/HttOptionsNoAuthnIntegTests.java new file mode 100644 index 0000000000000..a9bfa8602f97a --- /dev/null +++ b/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/xpack/security/authc/HttOptionsNoAuthnIntegTests.java @@ -0,0 +1,99 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0; you may not use this file except in compliance with the Elastic License + * 2.0. + */ + +package org.elasticsearch.xpack.security.authc; + +import org.elasticsearch.client.Request; +import org.elasticsearch.client.RequestOptions; +import org.elasticsearch.client.Response; +import org.elasticsearch.common.settings.SecureString; +import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.http.CorsHandler; +import org.elasticsearch.http.HttpTransportSettings; +import org.elasticsearch.test.SecurityIntegTestCase; +import org.elasticsearch.test.SecuritySettingsSource; +import org.elasticsearch.xpack.core.security.authc.support.UsernamePasswordToken; + +import java.util.List; + +import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.notNullValue; + +public class HttOptionsNoAuthnIntegTests extends SecurityIntegTestCase { + + @Override + protected boolean addMockHttpTransport() { + return false; // need real http + } + + @Override + protected Settings nodeSettings(int nodeOrdinal, Settings otherSettings) { + final Settings.Builder builder = Settings.builder().put(super.nodeSettings(nodeOrdinal, otherSettings)); + // needed to test preflight requests + builder.put(HttpTransportSettings.SETTING_CORS_ENABLED.getKey(), "true") + .put(HttpTransportSettings.SETTING_CORS_ALLOW_ORIGIN.getKey(), "*"); + return builder.build(); + } + + public void testNoAuthnForResourceOptionsMethod() throws Exception { + Request requestNoCredentials = new Request( + "OPTIONS", + randomFrom("/", "/_cluster/stats", "/some-index", "/index/_stats", "/_stats/flush") + ); + // no "Authorization" request header -> request is unauthenticated + assertThat(requestNoCredentials.getOptions().getHeaders().isEmpty(), is(true)); + // WRONG "Authorization" request header + Request requestWrongCredentials = new Request( + "OPTIONS", + randomFrom("/", "/_cluster/stats", "/some-index", "/index/_stats", "/_stats/flush") + ); + RequestOptions.Builder options = requestWrongCredentials.getOptions().toBuilder(); + options.addHeader( + "Authorization", + UsernamePasswordToken.basicAuthHeaderValue(SecuritySettingsSource.TEST_USER_NAME, new SecureString("WRONG")) + ); + requestWrongCredentials.setOptions(options); + for (Request request : List.of(requestNoCredentials, requestWrongCredentials)) { + Response response = getRestClient().performRequest(request); + assertThat(response.getStatusLine().getStatusCode(), is(200)); + assertThat(response.getHeader("Allow"), notNullValue()); + assertThat(response.getHeader("X-elastic-product"), is("Elasticsearch")); + assertThat(response.getHeader("content-length"), is("0")); + } + } + + public void testNoAuthnForPreFlightRequest() throws Exception { + Request requestNoCredentials = new Request( + "OPTIONS", + randomFrom("/", "/_cluster/stats", "/some-index", "/index/_stats", "/_stats/flush") + ); + RequestOptions.Builder options = requestNoCredentials.getOptions().toBuilder(); + options.addHeader(CorsHandler.ORIGIN, "google.com"); + options.addHeader(CorsHandler.ACCESS_CONTROL_REQUEST_METHOD, "GET"); + requestNoCredentials.setOptions(options); + // no "Authorization" request header -> request is unauthenticated + Request requestWrongCredentials = new Request( + "OPTIONS", + randomFrom("/", "/_cluster/stats", "/some-index", "/index/_stats", "/_stats/flush") + ); + options = requestWrongCredentials.getOptions().toBuilder(); + // WRONG "Authorization" request header + options.addHeader( + "Authorization", + UsernamePasswordToken.basicAuthHeaderValue(SecuritySettingsSource.TEST_USER_NAME, new SecureString("WRONG")) + ); + options.addHeader(CorsHandler.ORIGIN, "google.com"); + options.addHeader(CorsHandler.ACCESS_CONTROL_REQUEST_METHOD, "GET"); + requestWrongCredentials.setOptions(options); + for (Request request : List.of(requestWrongCredentials)) { + Response response = getRestClient().performRequest(request); + assertThat(response.getStatusLine().getStatusCode(), is(200)); + assertThat(response.getHeader("content-length"), is("0")); + } + } + +} diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java index 2f61103175b6f..083333ae0b00e 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java @@ -1656,10 +1656,16 @@ public boolean test(String profile, InetSocketAddress peerAddress) { RemoteHostHeader.process(channel, threadContext); // step 2: Run authentication on the now properly prepared thread-context. // This inspects and modifies the thread context. - authenticationService.authenticate( - httpPreRequest, - ActionListener.wrap(ignored -> listener.onResponse(null), listener::onFailure) - ); + if (httpPreRequest.method() != RestRequest.Method.OPTIONS) { + authenticationService.authenticate( + httpPreRequest, + ActionListener.wrap(ignored -> listener.onResponse(null), listener::onFailure) + ); + } else { + // allow for unauthenticated OPTIONS request + // this includes CORS preflight, and regular OPTIONS that return permitted methods for a given path + listener.onResponse(null); + } }; return getHttpServerTransportWithHeadersValidator( settings, diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/SecurityRestFilter.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/SecurityRestFilter.java index 09e89f17b32e0..ac78b17994985 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/SecurityRestFilter.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/SecurityRestFilter.java @@ -9,6 +9,7 @@ import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.apache.logging.log4j.util.Supplier; +import org.elasticsearch.ElasticsearchSecurityException; import org.elasticsearch.action.ActionListener; import org.elasticsearch.client.internal.node.NodeClient; import org.elasticsearch.common.util.concurrent.ThreadContext; @@ -60,9 +61,14 @@ public RestHandler getConcreteRestHandler() { @Override public void handleRequest(RestRequest request, RestChannel channel, NodeClient client) throws Exception { + // requests with the OPTIONS method should be handled elsewhere, and not by calling {@code RestHandler#handleRequest} + // authn is bypassed for HTTP requests with the OPTIONS method, so this sanity check prevents dispatching unauthenticated requests if (request.method() == Method.OPTIONS) { - // CORS - allow for preflight unauthenticated OPTIONS request - restHandler.handleRequest(request, channel, client); + handleException( + request, + channel, + new ElasticsearchSecurityException("Cannot dispatch OPTIONS request, as they are not authenticated") + ); return; } diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/rest/SecurityRestFilterTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/rest/SecurityRestFilterTests.java index 7de9b6d25f929..c861e133d1535 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/rest/SecurityRestFilterTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/rest/SecurityRestFilterTests.java @@ -22,12 +22,15 @@ import org.elasticsearch.rest.RestHandler; import org.elasticsearch.rest.RestRequest; import org.elasticsearch.rest.RestRequestFilter; +import org.elasticsearch.rest.RestResponse; +import org.elasticsearch.rest.RestStatus; import org.elasticsearch.test.ESTestCase; import org.elasticsearch.test.SecuritySettingsSourceField; import org.elasticsearch.test.rest.FakeRestRequest; import org.elasticsearch.xcontent.DeprecationHandler; import org.elasticsearch.xcontent.NamedXContentRegistry; import org.elasticsearch.xcontent.XContentType; +import org.elasticsearch.xcontent.json.JsonXContent; import org.elasticsearch.xpack.core.security.SecurityContext; import org.elasticsearch.xpack.core.security.authc.Authentication; import org.elasticsearch.xpack.core.security.authc.AuthenticationTestHelper; @@ -38,6 +41,7 @@ import org.elasticsearch.xpack.security.authc.AuthenticationService; import org.elasticsearch.xpack.security.authc.support.SecondaryAuthenticator; import org.junit.Before; +import org.mockito.ArgumentCaptor; import java.util.Base64; import java.util.Collections; @@ -47,7 +51,9 @@ import java.util.concurrent.atomic.AtomicReference; import static org.elasticsearch.test.ActionListenerUtils.anyActionListener; +import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.hasItem; +import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.not; import static org.hamcrest.Matchers.notNullValue; import static org.hamcrest.Matchers.sameInstance; @@ -149,12 +155,17 @@ public void testProcessWithSecurityDisabled() throws Exception { } public void testProcessOptionsMethod() throws Exception { - RestRequest request = mock(RestRequest.class); - when(request.method()).thenReturn(RestRequest.Method.OPTIONS); + FakeRestRequest request = new FakeRestRequest.Builder(NamedXContentRegistry.EMPTY).withMethod(RestRequest.Method.OPTIONS).build(); + when(channel.request()).thenReturn(request); + when(channel.newErrorBuilder()).thenReturn(JsonXContent.contentBuilder()); filter.handleRequest(request, channel, null); - verify(restHandler).handleRequest(request, channel, null); - verifyNoMoreInteractions(channel); + verifyNoMoreInteractions(restHandler); verifyNoMoreInteractions(authcService); + ArgumentCaptor responseArgumentCaptor = ArgumentCaptor.forClass(RestResponse.class); + verify(channel).sendResponse(responseArgumentCaptor.capture()); + RestResponse restResponse = responseArgumentCaptor.getValue(); + assertThat(restResponse.status(), is(RestStatus.INTERNAL_SERVER_ERROR)); + assertThat(restResponse.content().utf8ToString(), containsString("Cannot dispatch OPTIONS request, as they are not authenticated")); } public void testProcessFiltersBodyCorrectly() throws Exception {