data-prepper icon indicating copy to clipboard operation
data-prepper copied to clipboard

Fix an issue that exception messages are masked

Open oeyh opened this issue 1 year ago • 3 comments

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.

oeyh avatar Apr 12 '24 19:04 oeyh

@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

KarstenSchnitter avatar Apr 13 '24 20:04 KarstenSchnitter

@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 avatar Apr 17 '24 22:04 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.

oeyh avatar Apr 18 '24 03:04 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.

oeyh avatar May 13 '24 20:05 oeyh

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.

dlvenable avatar May 13 '24 21:05 dlvenable