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

fix(soap): remove logging of soap request and response #3848

Merged

Conversation

mathias-vandaele
Copy link
Collaborator

Description

Remove logs that could be showing sensitive data (request, response)

Related issues

closes #https://github.com/camunda/team-connectors/issues/965

Checklist

  • PR has a milestone or the no milestone label.

@mathias-vandaele mathias-vandaele requested a review from a team as a code owner January 14, 2025 09:28
@mathias-vandaele mathias-vandaele self-assigned this Jan 14, 2025
@mathias-vandaele mathias-vandaele added this to the 8.7.0-alpha4 milestone Jan 14, 2025
@mathias-vandaele mathias-vandaele force-pushed the 965-soap-connector-is-logging-sensitive-information branch from ea53aaf to 0ce52ff Compare January 14, 2025 09:35
sbuettner
sbuettner previously approved these changes Jan 14, 2025
@@ -138,7 +138,6 @@ private ClientInterceptor[] buildInterceptors(
List<ClientInterceptor> interceptorList = new ArrayList<>();
handleAuthentication(authentication).ifPresent(interceptorList::add);
handleSoapHeader(soapHeader, namespaces).ifPresent(interceptorList::add);
interceptorList.add(new LoggingInterceptor());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand correctly, the customer does not want sensitive information to be logged.

However, the logging interceptor only uses the log level DEBUG, so the actual problem is on the customer side. I would vote for keeping this logging so that the debugging of connector issues is possible with the log.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Log/store PII is also forbidden (GDPR etc.), which might happen if we keep this line. Regardless of the log level, we just can't store this information.
You might be right, but I just wanted confirmation here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right in general.

In the last days, I worked with a customer on an issue and we required spring security filter details to be logged.

Spring solves this like this:

  1. All logging is TRACE
  2. There is a parameter that you have to set so that the logging will happen.

If we introduce a parameter, we can ensure that no confidential stuff will be logged "by mistake". Also, we are aligned with industry standards then.

This is pretty much what @mathias-vandaele suggested in the first place, so sorry for re-iterating

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All logging is TRACE
There is a parameter that you have to set so that the logging will happen.

Yea, I get this part :D But we can still log PII (using TRACE), which is from my knowledge simply forbidden (trace or info is not relevant).

I might be wrong though, so feel free to ignore my comment.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, apache http logs everything when being set to log level debug, so it is not forbidden :)

From my experience, the operations team has to ensure that they run compliant.

Copy link
Contributor

@jonathanlukas jonathanlukas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mathias-vandaele mathias-vandaele force-pushed the 965-soap-connector-is-logging-sensitive-information branch from 124ff7b to 881c6ad Compare January 20, 2025 08:37
@mathias-vandaele mathias-vandaele added this pull request to the merge queue Jan 20, 2025
Merged via the queue into main with commit 69db6bf Jan 20, 2025
14 checks passed
@mathias-vandaele mathias-vandaele deleted the 965-soap-connector-is-logging-sensitive-information branch January 20, 2025 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants