Fix an issue that exception messages are masked

[oeyh]

Description

Show exception messages instead of masking them through the SENSITIVE marker.

I checked all usages of SENSITIVE and EVENT in the repo and seems that all issues are in this S3DlqWriter.java file.

Issues Resolved

Resolves #3375

Check List

• New functionality includes testing.
• New functionality has a documentation issue. Please link to it in this PR.
  • New functionality has javadoc added
• Commits are signed with a real name per the DCO

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[dlvenable]

@oeyh , Did you test this? I'm unsure if the way Log4j operates is to give all the parameters, or just the ones that will be formatted into the string.

If it provides all arguments, you will need to modify the code below to keep any object that inherits from Throwable.

data-prepper/data-prepper-core/src/main/java/org/opensearch/dataprepper/logging/SensitiveArgumentMaskingConverter.java

Lines 41 to 48 in e341d7d

	// TODO: apply masking on Event type parameter only for EVENT marker.
	if (marker != null && SENSITIVE_MARKER_NAMES.contains(marker.getName())) {
	final Object[] maskPatternArguments = new Object[message.getParameters().length];
	Arrays.fill(maskPatternArguments, MASK_PATTERN);
	toAppendTo.append(MessageFormatter.arrayFormat(message.getFormat(), maskPatternArguments).getMessage());
	} else {
	toAppendTo.append(message.getFormattedMessage());
	}

[KarstenSchnitter]

@dlvenable In my experience, you can give Log4j as many arguments as you want and it will add them to the LogEvent. You can retrieve them via message.getParameters used in your example. This will retrieve all parameters, not only the ones necessary for the message format.

I have used this approach in a logging extension for SAP BTP to provide additional custom fields for log messages: https://github.com/SAP/cf-java-logging-support/blob/26565dc37aa30793f1f5a4684e8e17586bdcf59f/cf-java-logging-support-log4j2/src/main/java/com/sap/hcp/cf/log4j2/layout/supppliers/LogEventUtilities.java#L18-L21 and https://github.com/SAP/cf-java-logging-support/blob/26565dc37aa30793f1f5a4684e8e17586bdcf59f/cf-java-logging-support-core/src/main/java/com/sap/hcp/cf/logging/common/serialization/AbstractContextFieldSupplier.java#L22-L27

[oeyh]

@dlvenable @KarstenSchnitter Thanks for the comments. I did a test where I called:

LOG.error(SENSITIVE, "Failed to process content: [{}]", content, e);

and I get these logs with content masked and exception messages logged:

2024-04-17T17:10:47 [simple-pipeline-sink-worker-2-thread-1] ERROR org.opensearch.dataprepper.pipeline.Pipeline - Failed to process content: [******]
" java.lang.RuntimeException: test logging
        at org.opensearch.dataprepper.pipeline.Pipeline.startSourceAndProcessors(Pipeline.java:222) ~[data-prepper-core-2.8.0-SNAPSHOT.jar:?]
        at org.opensearch.dataprepper.pipeline.Pipeline.lambda$execute$2(Pipeline.java:268) ~[data-prepper-core-2.8.0-SNAPSHOT.jar:?]
        at java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:539) [?:?]
        at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264) [?:?]
        at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136) [?:?]
        at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635) [?:?]
        at java.base/java.lang.Thread.run(Thread.java:840) [?:?]

So this should work.

[oeyh]

I'm unsure if the way Log4j operates is to give all the parameters, or just the ones that will be formatted into the string.

Turns out, by default, pattern converter doesn't handle throwables (see this code snippet). So our custom pattern converter will keep the exception message as is.

[KarstenSchnitter]

I am not sure, whether including the exception might expose event data or not. For example the JsonProcessingException could contain a text indicating the spot where the error occurred. This message might contain a snippet of the parsed message. Not sure, if it really behaves that way though.

[oeyh]

That may be a concern that we can address separately. The markers we have today cannot filter sensitive information from stack trace. But we do want to show the stack trace for troubleshooting.

[dlvenable]

The intention of the SENSITIVE market is to say that the incoming data (not exception) may have sensitive data. We may want to add a SENSITIVE_EXCEPTION which also masks the exception part. Then we can use that in places where we believe the exception may have sensitive information.