Skip to content

Commit

Permalink
Just log 401 stacktraces (elastic#55774)
Browse files Browse the repository at this point in the history 
Ensure stacktraces of 401 errors for unauthenticated users are logged
but not returned in the response body.
  • Loading branch information
@albertzaharovits
albertzaharovits authored Jun 10, 2020
1 parent 44c3bb2 commit c57ccd9
Show file tree
Hide file tree
Showing 5 changed files with 83 additions and 27 deletions.
2 changes: 1 addition & 1 deletion server/src/main/java/org/elasticsearch/ElasticsearchException.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down
48 changes: 29 additions & 19 deletions server/src/main/java/org/elasticsearch/rest/BytesRestResponse.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -92,8 +94,20 @@ public BytesRestResponse(RestChannel channel, Exception e) throws IOException {
}

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();
}
Expand All @@ -117,28 +131,24 @@ public RestStatus status() {
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 {
Expand Down
10 changes: 9 additions & 1 deletion ...ugin/security/src/main/java/org/elasticsearch/xpack/security/rest/SecurityRestFilter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
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;
Expand All @@ -19,6 +20,7 @@
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;
Expand Down Expand Up @@ -82,8 +84,14 @@ public void handleRequest(RestRequest request, RestChannel channel, NodeClient c

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<?>) () ->
Expand Down
1 change: 0 additions & 1 deletion x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/SecurityTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,6 @@ public void testCustomRealmExtensionConflict() throws Exception {
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);
Expand Down
49 changes: 44 additions & 5 deletions ...security/src/test/java/org/elasticsearch/xpack/security/rest/SecurityRestFilterTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

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;
Expand All @@ -22,6 +23,7 @@
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;
Expand All @@ -39,11 +41,15 @@

import java.util.Base64;
import java.util.Collections;
import java.util.HashMap;
import java.util.Map;
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;
Expand Down Expand Up @@ -141,21 +147,54 @@ public void testProcessBasicLicense() throws Exception {
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(Collections.unmodifiableMap(new HashMap<String, String>() {{
put("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);
}

Expand Down

0 comments on commit c57ccd9

Please sign in to comment.