Fix an issue that exception messages are masked
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
- [x] 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 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
@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.
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.
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.
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.