Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added internal option to fully redact exceptions #3528

Merged
merged 8 commits into from
Feb 15, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions CHANGELOG.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,10 @@ Use subheadings with the "=====" level for adding notes for unreleased changes:

=== Unreleased

[float]
===== Features
* Added internal `safe_exceptions` config option to workaround JVM bugs related to touching exceptions - {pull}3528[#3528]

[float]
===== Bug fixes
* Cleanup extra servlet request attribute used for Spring exception handler - {pull}3527[#3527]
Expand All @@ -44,9 +48,6 @@ Use subheadings with the "=====" level for adding notes for unreleased changes:
[float]
===== Features
* Added a configuration option to use queues in names of spring-rabbit transactions - {pull}3424[#3424]

[float]
===== Features
* Add option to retry JMX metrics capture in case of exception - {pull}3511[#3511]

[float]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -517,6 +517,14 @@ public String toString(Collection<String> value) {
.tags("added[1.44.0]")
.buildWithDefault(false);


private final ConfigurationOption<Integer> safeExceptions = ConfigurationOption.<Boolean>integerOption()
.key("safe_exceptions")
.tags("internal")
.configurationCategory(CORE_CATEGORY)
.dynamic(true)
.buildWithDefault(0);

private final ConfigurationOption<List<WildcardMatcher>> classesExcludedFromInstrumentation = ConfigurationOption
.builder(new ValueConverter<List<WildcardMatcher>>() {

Expand Down Expand Up @@ -1163,6 +1171,15 @@ public boolean isContextPropagationOnly() {
return contextPropagationOnly.get();
}

public boolean isRedactExceptions() {
return (safeExceptions.get() & 1) != 0;
}

@Override
public boolean isUseServletAttributesForExceptionPropagation() {
return (safeExceptions.get() & 2) == 0;
}

public enum CloudProvider {
AUTO,
AWS,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import co.elastic.apm.agent.configuration.CoreConfiguration;
import co.elastic.apm.agent.configuration.MetricsConfiguration;
import co.elastic.apm.agent.configuration.ServerlessConfiguration;
import co.elastic.apm.agent.impl.error.RedactedException;
import co.elastic.apm.agent.impl.metadata.ServiceFactory;
import co.elastic.apm.agent.tracer.service.Service;
import co.elastic.apm.agent.tracer.service.ServiceInfo;
Expand Down Expand Up @@ -446,6 +447,8 @@ private ErrorCapture captureException(long epochMicros, @Nullable Throwable e, E
return null;
}

e = redactExceptionIfRequired(e);

while (e != null && WildcardMatcher.anyMatch(coreConfiguration.getUnnestExceptions(), e.getClass().getName()) != null) {
e = e.getCause();
}
Expand Down Expand Up @@ -995,4 +998,13 @@ public Service createService(String ephemeralId) {
configurationRegistry.getConfig(ServerlessConfiguration.class).runsOnAwsLambda()
);
}

@Override
@Nullable
public Throwable redactExceptionIfRequired(@Nullable Throwable original) {
if (original != null && coreConfiguration.isRedactExceptions() && !(original instanceof RedactedException)) {
return new RedactedException();
}
return original;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
/*
* Licensed to Elasticsearch B.V. under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch B.V. 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 co.elastic.apm.agent.impl.error;

public class RedactedException extends Exception {

public RedactedException() {
super("This exception is a placeholder for an exception which has occurred in the application." +
"The stacktrace corresponds to where elastic APM detected the exception.");
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,9 @@ public class Transaction extends AbstractSpan<Transaction> implements co.elastic
@Nullable
private String frameworkVersion;

@Nullable
private Throwable pendingException;
JonasKunz marked this conversation as resolved.
Show resolved Hide resolved

/**
* Faas
* <p>
Expand Down Expand Up @@ -337,6 +340,7 @@ public void resetState() {
frameworkVersion = null;
faas.resetState();
wasActivated.set(false);
pendingException = null;
// don't clear timerBySpanTypeAndSubtype map (see field-level javadoc)
}

Expand Down Expand Up @@ -535,4 +539,17 @@ private void trackMetrics() {
phaser.readerUnlock();
}
}


@Override
public void setPendingTransactionException(@Nullable Throwable exception) {
this.pendingException = exception;
}

@Nullable
@Override
public Throwable getPendingTransactionException() {
return this.pendingException;
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import co.elastic.apm.agent.impl.ElasticApmTracer;
import co.elastic.apm.agent.impl.context.Request;
import co.elastic.apm.agent.impl.error.ErrorCapture;
import co.elastic.apm.agent.impl.error.RedactedException;
import co.elastic.apm.agent.impl.stacktrace.StacktraceConfiguration;
import co.elastic.apm.agent.impl.transaction.Transaction;
import org.junit.jupiter.api.BeforeEach;
Expand Down Expand Up @@ -107,6 +108,27 @@ void testNonConfiguredWrappingConfigured() {
assertThat(errorCapture.getException()).isInstanceOf(WrapperException.class);
}

@Test
void testExceptionRedaction() {
doReturn(true).when(coreConfiguration).isRedactExceptions();
assertThat(tracer.redactExceptionIfRequired(null)).isNull();

Exception exception = new CustomException();
Throwable redacted = tracer.redactExceptionIfRequired(exception);
assertThat(redacted).isInstanceOf(RedactedException.class);

assertThat(tracer.redactExceptionIfRequired(redacted)).isSameAs(redacted);

ErrorCapture errorCapture = tracer.captureException(exception, tracer.currentContext(), null);
assertThat(errorCapture).isNotNull();
assertThat(errorCapture.getException()).isInstanceOf(RedactedException.class);

ErrorCapture alreadyRedacted = tracer.captureException(redacted, tracer.currentContext(), null);
assertThat(alreadyRedacted).isNotNull();
assertThat(alreadyRedacted.getException()).isSameAs(redacted);
}


private static class NestedException extends Exception {
public NestedException(Throwable cause) {
super(cause);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -253,6 +253,12 @@ public static <HttpServletRequest, HttpServletResponse, ServletContext, ServletC
break;
}
}
if (t2 == null) {
t2 = transaction.getPendingTransactionException();
if(t2 != null) {
overrideStatusCodeOnThrowable = false;
}
}
}

ServletContext servletContext = adapter.getServletContext(httpServletRequest);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ public void onComplete(AsyncEvent event) {

@Override
public void onTimeout(AsyncEvent event) {
throwable = event.getThrowable();
throwable = asyncContextAdviceHelperImpl.getTracer().redactExceptionIfRequired(event.getThrowable());
if (isJBossEap6(event)) {
endTransaction(event);
}
Expand All @@ -98,7 +98,7 @@ public void onTimeout(AsyncEvent event) {

@Override
public void onError(AsyncEvent event) {
throwable = event.getThrowable();
throwable = asyncContextAdviceHelperImpl.getTracer().redactExceptionIfRequired(event.getThrowable());
if (isJBossEap6(event)) {
endTransaction(event);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,10 @@ public JakartaAsyncContextAdviceHelper(Tracer tracer) {
new JakartaAsyncContextAdviceHelper.JakartaApmAsyncListenerAllocator());
}

public Tracer getTracer() {
return tracer;
}

private final class JakartaApmAsyncListenerAllocator implements Allocator<JakartaApmAsyncListener> {
@Override
public JakartaApmAsyncListener createInstance() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ public void onComplete(AsyncEvent event) {

@Override
public void onTimeout(AsyncEvent event) {
throwable = event.getThrowable();
throwable = asyncContextAdviceHelperImpl.getTracer().redactExceptionIfRequired(event.getThrowable());
if (isJBossEap6(event)) {
endTransaction(event);
}
Expand All @@ -97,7 +97,7 @@ public void onTimeout(AsyncEvent event) {

@Override
public void onError(AsyncEvent event) {
throwable = event.getThrowable();
throwable = asyncContextAdviceHelperImpl.getTracer().redactExceptionIfRequired(event.getThrowable());
if (isJBossEap6(event)) {
endTransaction(event);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,10 @@ public JavaxAsyncContextAdviceHelper(Tracer tracer) {
new JavaxAsyncContextAdviceHelper.ApmAsyncListenerAllocator());
}

public Tracer getTracer() {
return tracer;
}

private final class ApmAsyncListenerAllocator implements Allocator<JavaxApmAsyncListener> {
@Override
public JavaxApmAsyncListener createInstance() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,10 @@
import co.elastic.apm.agent.sdk.ElasticApmInstrumentation;
import co.elastic.apm.agent.servlet.Constants;
import co.elastic.apm.agent.servlet.adapter.ServletRequestAdapter;
import co.elastic.apm.agent.tracer.GlobalTracer;
import co.elastic.apm.agent.tracer.Tracer;
import co.elastic.apm.agent.tracer.Transaction;
import co.elastic.apm.agent.tracer.configuration.CoreConfiguration;
import net.bytebuddy.description.method.MethodDescription;
import net.bytebuddy.description.type.TypeDescription;
import net.bytebuddy.matcher.ElementMatcher;
Expand Down Expand Up @@ -63,7 +67,17 @@ public static <HttpServletRequest> void captureRequestError(ServletRequestAdapte
@Nullable HttpServletRequest request,
@Nullable Exception e) {
if (request != null && e != null) {
adapter.setAttribute(request, "co.elastic.apm.exception", e);
Tracer tracer = GlobalTracer.get();
boolean useAttribs = tracer.getConfig(CoreConfiguration.class).isUseServletAttributesForExceptionPropagation();
Throwable maybeRedacted = tracer.redactExceptionIfRequired(e);
if (useAttribs) {
adapter.setAttribute(request, "co.elastic.apm.exception", maybeRedacted);
} else {
Transaction<?> transaction = tracer.currentContext().getTransaction();
if(transaction != null) {
transaction.setPendingTransactionException(maybeRedacted);
}
}
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,11 @@
*/
package co.elastic.apm.agent.springwebmvc.exception;

import co.elastic.apm.agent.configuration.CoreConfiguration;
import co.elastic.apm.agent.springwebmvc.exception.testapp.exception_resolver.ExceptionResolverController;
import co.elastic.apm.agent.springwebmvc.exception.testapp.exception_resolver.ExceptionResolverRuntimeException;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.ValueSource;
import org.springframework.mock.web.MockHttpServletResponse;
import org.springframework.test.context.ContextConfiguration;
import org.springframework.test.web.servlet.MvcResult;
Expand All @@ -30,6 +32,7 @@

import static org.assertj.core.api.Assertions.assertThat;
import static org.junit.Assert.assertEquals;
import static org.mockito.Mockito.doReturn;
import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get;

/**
Expand All @@ -42,8 +45,12 @@
})
public abstract class AbstractExceptionHandlerInstrumentationWithExceptionResolverTest extends AbstractExceptionHandlerInstrumentationTest {

@Test
public void testCallApiWithExceptionThrown() throws Exception {
@ParameterizedTest
@ValueSource(booleans = {true,false})
public void testCallApiWithExceptionThrown(boolean useAttributeBasedPropagation) throws Exception {
CoreConfiguration coreConfig = config.getConfig(CoreConfiguration.class);
doReturn(useAttributeBasedPropagation).when(coreConfig).isUseServletAttributesForExceptionPropagation();

ResultActions resultActions = this.mockMvc.perform(get("/exception-resolver/throw-exception"));
MvcResult result = resultActions.andReturn();
MockHttpServletResponse response = result.getResponse();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -150,4 +150,10 @@ public void reportLog(byte[] log) {
public Service createService(String ephemeralId) {
return tracer.createService(ephemeralId);
}

@Nullable
@Override
public Throwable redactExceptionIfRequired(@Nullable Throwable original) {
return tracer.redactExceptionIfRequired(original);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -125,4 +125,10 @@ public void reportLog(byte[] log) {
public Service createService(String ephemeralId) {
return null;
}

@Nullable
@Override
public Throwable redactExceptionIfRequired(@Nullable Throwable original) {
return original;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -88,4 +88,7 @@ public interface Tracer {

@Nullable
Service createService(String ephemeralId);

@Nullable
Throwable redactExceptionIfRequired(@Nullable Throwable original);
}
Original file line number Diff line number Diff line change
Expand Up @@ -56,4 +56,9 @@ public interface Transaction<T extends Transaction<T>> extends AbstractSpan<T> {
void setFrameworkName(@Nullable String frameworkName);

void setFrameworkVersion(@Nullable String frameworkVersion);

void setPendingTransactionException(Throwable exception);

@Nullable
Throwable getPendingTransactionException();
}
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,8 @@ public interface CoreConfiguration {

TimeDuration getSpanMinDuration();

boolean isUseServletAttributesForExceptionPropagation();

enum EventType {
/**
* Request bodies will never be reported
Expand Down
Loading