Handle Unauthenticated OPTIONS requests (elastic#96061)

This address HTTP OPTIONS requests following
the authentication refactoring in elastic#95112.

Relates elastic#95112

server/src/main/java/org/elasticsearch/rest/RestController.java

@@ -210,6 +210,8 @@ private void registerHandlerNoWrap(RestRequest.Method method, String path, RestH
         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, maybeWrappedHandler),

server/src/test/java/org/elasticsearch/rest/RestControllerTests.java

@@ -82,6 +82,7 @@ public class RestControllerTests extends ESTestCase {
     private HierarchyCircuitBreakerService circuitBreakerService;
     private UsageService usageService;
     private NodeClient client;
+    private List<RestRequest.Method> methodList;
 
     @Before
     public void setup() {
@@ -112,6 +113,7 @@ public void setup() {
             (request, channel, client) -> { throw new IllegalArgumentException("test error"); }
         );
         httpServerTransport.start();
+        methodList = Arrays.stream(RestRequest.Method.values()).filter(m -> m != OPTIONS).collect(Collectors.toList());
     }
 
     @After
@@ -171,7 +173,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);
@@ -190,10 +192,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 String deprecationMessage = "["
             + replacedMethod.name()
@@ -220,10 +222,8 @@ public void testRegisterAsReplacedHandler() {
     public void testRegisterSecondMethodWithDifferentNamedWildcard() {
         final RestController restController = new RestController(null, null, circuitBreakerService, usageService);
 
-        RestRequest.Method firstMethod = randomFrom(RestRequest.Method.values());
-        RestRequest.Method secondMethod = randomFrom(
-            Arrays.stream(RestRequest.Method.values()).filter(m -> m != firstMethod).collect(Collectors.toList())
-        );
+        RestRequest.Method firstMethod = randomFrom(methodList);
+        RestRequest.Method secondMethod = randomFrom(methodList.stream().filter(m -> m != firstMethod).collect(Collectors.toList()));
 
         final String path = "/_" + randomAlphaOfLengthBetween(1, 6);
 
@@ -517,7 +517,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 {
@@ -674,7 +674,7 @@ public void testRegisterWithReservedPath() {
         final RestController restController = new RestController(null, client, circuitBreakerService, usageService);
         for (String path : RestController.RESERVED_PATHS) {
             IllegalArgumentException iae = expectThrows(IllegalArgumentException.class, () -> {
-                restController.registerHandler(new Route(randomFrom(RestRequest.Method.values()), path), new RestHandler() {
+                restController.registerHandler(new Route(randomFrom(methodList), path), new RestHandler() {
                     @Override
                     public void handleRequest(RestRequest request, RestChannel channel, NodeClient client) {
                         channel.sendResponse(new BytesRestResponse(RestStatus.OK, BytesRestResponse.TEXT_CONTENT_TYPE, BytesArray.EMPTY));

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.Arrays;
+
+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 : Arrays.asList(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 : Arrays.asList(requestWrongCredentials)) {
+            Response response = getRestClient().performRequest(request);
+            assertThat(response.getStatusLine().getStatusCode(), is(200));
+            assertThat(response.getHeader("content-length"), is("0"));
+        }
+    }
+
+}

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java

@@ -1511,10 +1511,16 @@ && getSslService().isSSLClientAuthEnabled(getSslService().getHttpTransportSSLCon
                 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,

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/rest/SecurityRestFilter.java

@@ -10,6 +10,7 @@
 import org.apache.logging.log4j.Logger;
 import org.apache.logging.log4j.message.ParameterizedMessage;
 import org.apache.logging.log4j.util.Supplier;
+import org.elasticsearch.ElasticsearchSecurityException;
 import org.elasticsearch.Version;
 import org.elasticsearch.action.ActionListener;
 import org.elasticsearch.client.node.NodeClient;
@@ -76,6 +77,17 @@ public void handleRequest(RestRequest request, RestChannel channel, NodeClient c
                 }, e -> handleException("Secondary authentication", request, channel, e)));
             }, e -> handleException("Authentication", request, channel, e)));
         } else {
+            // requests with the OPTIONS method should be handled elsewhere, and not by calling {@code RestHandler#handleRequest}
+            // authn is bypassed for HTTP reqs with the OPTIONS method, so this sanity check prevents dispatching unauthenticated reqs
+            if (licenseState.isSecurityEnabled()) {
+                handleException(
+                    "Options with body",
+                    request,
+                    channel,
+                    new ElasticsearchSecurityException("Cannot dispatch OPTIONS request, as they are not authenticated")
+                );
+                return;
+            }
             if (request.method() != Method.OPTIONS) {
                 HeaderWarning.addWarning(
                     "Elasticsearch built-in security features are not enabled. Without "

x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/rest/SecurityRestFilterTests.java

@@ -24,6 +24,8 @@
 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;
@@ -39,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.Arrays;
 import java.util.Base64;
@@ -48,6 +51,8 @@
 import java.util.concurrent.atomic.AtomicReference;
 
 import static org.elasticsearch.test.ActionListenerUtils.anyActionListener;
+import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.is;
 import static org.hamcrest.Matchers.notNullValue;
 import static org.hamcrest.Matchers.sameInstance;
 import static org.mockito.ArgumentMatchers.any;
@@ -171,12 +176,17 @@ public void testProcessBasicLicense() 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<RestResponse> 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 {