Just log 401 stacktraces (#55774)

Ensure stacktraces of 401 errors for unauthenticated users are logged
but not returned in the response body.

server/src/main/java/org/elasticsearch/ElasticsearchException.java

@@ -88,7 +88,7 @@ public class ElasticsearchException extends RuntimeException implements ToXConte
     private static final String REASON = "reason";
     private static final String CAUSED_BY = "caused_by";
     private static final ParseField SUPPRESSED = new ParseField("suppressed");
-    private static final String STACK_TRACE = "stack_trace";
+    public static final String STACK_TRACE = "stack_trace";
     private static final String HEADER = "header";
     private static final String ERROR = "error";
     private static final String ROOT_CAUSE = "root_cause";

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

@@ -19,8 +19,8 @@
 
 package org.elasticsearch.rest;
 
-import org.apache.logging.log4j.Logger;
 import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
 import org.apache.logging.log4j.message.ParameterizedMessage;
 import org.apache.logging.log4j.util.Supplier;
 import org.elasticsearch.ElasticsearchException;
@@ -46,6 +46,8 @@ public class BytesRestResponse extends RestResponse {
 
     private static final String STATUS = "status";
 
+    private static final Logger SUPPRESSED_ERROR_LOGGER = LogManager.getLogger("rest.suppressed");
+
     private final RestStatus status;
     private final BytesReference content;
     private final String contentType;
@@ -92,8 +94,20 @@ public class BytesRestResponse extends RestResponse {
     }
 
     public BytesRestResponse(RestChannel channel, RestStatus status, Exception e) throws IOException {
+        ToXContent.Params params = paramsFromRequest(channel.request());
+        if (params.paramAsBoolean(REST_EXCEPTION_SKIP_STACK_TRACE, REST_EXCEPTION_SKIP_STACK_TRACE_DEFAULT) && e != null) {
+            // log exception only if it is not returned in the response
+            Supplier<?> messageSupplier = () -> new ParameterizedMessage("path: {}, params: {}",
+                    channel.request().rawPath(), channel.request().params());
+            if (status.getStatus() < 500) {
+                SUPPRESSED_ERROR_LOGGER.debug(messageSupplier, e);
+            } else {
+                SUPPRESSED_ERROR_LOGGER.warn(messageSupplier, e);
+            }
+        }
         this.status = status;
-        try (XContentBuilder builder = build(channel, status, e)) {
+        try (XContentBuilder builder = channel.newErrorBuilder()) {
+            build(builder, params, status, channel.detailedErrorsEnabled(), e);
             this.content = BytesReference.bytes(builder);
             this.contentType = builder.contentType().mediaType();
         }
@@ -117,28 +131,24 @@ public class BytesRestResponse extends RestResponse {
         return this.status;
     }
 
-    private static final Logger SUPPRESSED_ERROR_LOGGER = LogManager.getLogger("rest.suppressed");
-
-    private static XContentBuilder build(RestChannel channel, RestStatus status, Exception e) throws IOException {
-        ToXContent.Params params = channel.request();
-        if (params.paramAsBoolean("error_trace", !REST_EXCEPTION_SKIP_STACK_TRACE_DEFAULT)) {
+    private ToXContent.Params paramsFromRequest(RestRequest restRequest) {
+        ToXContent.Params params = restRequest;
+        if (params.paramAsBoolean("error_trace", !REST_EXCEPTION_SKIP_STACK_TRACE_DEFAULT) && false == skipStackTrace()) {
             params =  new ToXContent.DelegatingMapParams(singletonMap(REST_EXCEPTION_SKIP_STACK_TRACE, "false"), params);
-        } else if (e != null) {
-            Supplier<?> messageSupplier = () -> new ParameterizedMessage("path: {}, params: {}",
-                    channel.request().rawPath(), channel.request().params());
-
-            if (status.getStatus() < 500) {
-                SUPPRESSED_ERROR_LOGGER.debug(messageSupplier, e);
-            } else {
-                SUPPRESSED_ERROR_LOGGER.warn(messageSupplier, e);
-            }
         }
+        return params;
+    }
+
+    protected boolean skipStackTrace() {
+        return false;
+    }
 
-        XContentBuilder builder = channel.newErrorBuilder().startObject();
-        ElasticsearchException.generateFailureXContent(builder, params, e, channel.detailedErrorsEnabled());
+    private void build(XContentBuilder builder, ToXContent.Params params, RestStatus status,
+                       boolean detailedErrorsEnabled, Exception e) throws IOException {
+        builder.startObject();
+        ElasticsearchException.generateFailureXContent(builder, params, e, detailedErrorsEnabled);
         builder.field(STATUS, status.getStatus());
         builder.endObject();
-        return builder;
     }
 
     static BytesRestResponse createSimpleErrorResponse(RestChannel channel, RestStatus status, String errorMessage) throws IOException {

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.message.ParameterizedMessage;
 import org.apache.logging.log4j.util.Supplier;
+import org.elasticsearch.ExceptionsHelper;
 import org.elasticsearch.action.ActionListener;
 import org.elasticsearch.client.node.NodeClient;
 import org.elasticsearch.common.util.concurrent.ThreadContext;
@@ -19,6 +20,7 @@ import org.elasticsearch.rest.RestChannel;
 import org.elasticsearch.rest.RestHandler;
 import org.elasticsearch.rest.RestRequest;
 import org.elasticsearch.rest.RestRequest.Method;
+import org.elasticsearch.rest.RestStatus;
 import org.elasticsearch.xpack.core.security.rest.RestRequestFilter;
 import org.elasticsearch.xpack.security.authc.AuthenticationService;
 import org.elasticsearch.xpack.security.authc.support.SecondaryAuthenticator;
@@ -82,8 +84,14 @@ public class SecurityRestFilter implements RestHandler {
 
     private void handleException(String actionType, RestRequest request, RestChannel channel, Exception e) {
         logger.debug(new ParameterizedMessage("{} failed for REST request [{}]", actionType, request.uri()), e);
+        final RestStatus restStatus = ExceptionsHelper.status(e);
         try {
-            channel.sendResponse(new BytesRestResponse(channel, e));
+            channel.sendResponse(new BytesRestResponse(channel, restStatus, e) {
+
+                @Override
+                protected boolean skipStackTrace() { return restStatus == RestStatus.UNAUTHORIZED; }
+
+            });
         } catch (Exception inner) {
             inner.addSuppressed(e);
             logger.error((Supplier<?>) () ->

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

@@ -160,7 +160,6 @@ public class SecurityTests extends ESTestCase {
         assertEquals("Realm type [" + FileRealmSettings.TYPE + "] is already registered", e.getMessage());
     }
 
-
     public void testAuditEnabled() throws Exception {
         Settings settings = Settings.builder().put(XPackSettings.AUDIT_ENABLED.getKey(), true).build();
         Collection<Object> components = createComponents(settings);

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

@@ -7,6 +7,7 @@ package org.elasticsearch.xpack.security.rest;
 
 import com.nimbusds.jose.util.StandardCharset;
 import org.apache.lucene.util.SetOnce;
+import org.elasticsearch.ElasticsearchException;
 import org.elasticsearch.action.ActionListener;
 import org.elasticsearch.client.node.NodeClient;
 import org.elasticsearch.common.bytes.BytesArray;
@@ -22,6 +23,7 @@ import org.elasticsearch.rest.BytesRestResponse;
 import org.elasticsearch.rest.RestChannel;
 import org.elasticsearch.rest.RestHandler;
 import org.elasticsearch.rest.RestRequest;
+import org.elasticsearch.rest.RestResponse;
 import org.elasticsearch.rest.RestStatus;
 import org.elasticsearch.test.ESTestCase;
 import org.elasticsearch.test.SecuritySettingsSourceField;
@@ -44,6 +46,9 @@ import java.util.Set;
 import java.util.concurrent.atomic.AtomicReference;
 
 import static org.elasticsearch.xpack.core.security.support.Exceptions.authenticationError;
+import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.is;
+import static org.hamcrest.Matchers.not;
 import static org.hamcrest.Matchers.notNullValue;
 import static org.hamcrest.Matchers.sameInstance;
 import static org.mockito.Matchers.any;
@@ -141,21 +146,52 @@ public class SecurityRestFilterTests extends ESTestCase {
         verifyZeroInteractions(channel, authcService);
     }
 
-    public void testProcessAuthenticationError() throws Exception {
-        RestRequest request = mock(RestRequest.class);
-        Exception exception = authenticationError("failed authc");
+    public void testProcessAuthenticationFailedNoTrace() throws Exception {
+        filter = new SecurityRestFilter(licenseState, threadContext, authcService, secondaryAuthenticator, restHandler, false);
+        testProcessAuthenticationFailed(randomBoolean() ? authenticationError("failed authn") : authenticationError("failed authn with " +
+                "cause", new ElasticsearchException("cause")), RestStatus.UNAUTHORIZED, true, true, false);
+        testProcessAuthenticationFailed(randomBoolean() ? authenticationError("failed authn") : authenticationError("failed authn with " +
+                "cause", new ElasticsearchException("cause")), RestStatus.UNAUTHORIZED, true, false, false);
+        testProcessAuthenticationFailed(randomBoolean() ? authenticationError("failed authn") : authenticationError("failed authn with " +
+                "cause", new ElasticsearchException("cause")), RestStatus.UNAUTHORIZED, false, true, false);
+        testProcessAuthenticationFailed(randomBoolean() ? authenticationError("failed authn") : authenticationError("failed authn with " +
+                "cause", new ElasticsearchException("cause")), RestStatus.UNAUTHORIZED, false, false, false);
+        testProcessAuthenticationFailed(new ElasticsearchException("dummy"), RestStatus.INTERNAL_SERVER_ERROR, false, false, false);
+        testProcessAuthenticationFailed(new IllegalArgumentException("dummy"), RestStatus.BAD_REQUEST, true, false, false);
+        testProcessAuthenticationFailed(new ElasticsearchException("dummy"), RestStatus.INTERNAL_SERVER_ERROR, false, true, false);
+        testProcessAuthenticationFailed(new IllegalArgumentException("dummy"), RestStatus.BAD_REQUEST, true, true, true);
+    }
+
+    private void testProcessAuthenticationFailed(Exception authnException, RestStatus expectedRestStatus, boolean errorTrace,
+                                                 boolean detailedErrorsEnabled, boolean traceExists) throws Exception {
+        RestRequest request;
+        if (errorTrace != !ElasticsearchException.REST_EXCEPTION_SKIP_STACK_TRACE_DEFAULT || randomBoolean()) {
+            request = new FakeRestRequest.Builder(NamedXContentRegistry.EMPTY)
+                    .withParams(Map.of("error_trace", Boolean.toString(errorTrace))).build();
+        } else {
+            // sometimes do not fill in the default value
+            request = new FakeRestRequest.Builder(NamedXContentRegistry.EMPTY).build();
+        }
         doAnswer((i) -> {
             ActionListener callback =
                 (ActionListener) i.getArguments()[1];
-            callback.onFailure(exception);
+            callback.onFailure(authnException);
             return Void.TYPE;
         }).when(authcService).authenticate(eq(request), any(ActionListener.class));
+        RestChannel channel = mock(RestChannel.class);
+        when(channel.detailedErrorsEnabled()).thenReturn(detailedErrorsEnabled);
         when(channel.request()).thenReturn(request);
         when(channel.newErrorBuilder()).thenReturn(JsonXContent.contentBuilder());
         filter.handleRequest(request, channel, null);
         ArgumentCaptor<BytesRestResponse> response = ArgumentCaptor.forClass(BytesRestResponse.class);
         verify(channel).sendResponse(response.capture());
-        assertEquals(RestStatus.UNAUTHORIZED, response.getValue().status());
+        RestResponse restResponse = response.getValue();
+        assertThat(restResponse.status(), is(expectedRestStatus));
+        if (traceExists) {
+            assertThat(restResponse.content().utf8ToString(), containsString(ElasticsearchException.STACK_TRACE));
+        } else {
+            assertThat(restResponse.content().utf8ToString(), not(containsString(ElasticsearchException.STACK_TRACE)));
+        }
         verifyZeroInteractions(restHandler);
     }