db.statement sanitization default behavior

[avzis]

What are you trying to achieve?

According to the db spec:

• The value may be sanitized to exclude sensitive information.

It does not strictly describes what should be the default behavior - sanitize the data, or not?

I am thinking that this approach might be risky, because of several cases:

• Users might not be aware of the query collection, and might expose sensitive data.
• In some instrumentations, db.statement was not collected in the beginning and was added (or will be added) in a later stage, when users are already using that instrumentation, which means that suddenly the Instrumentation might start exposing sensitive data without the user knowing about it.
• There are in-consistencies between different packages and instrumentation, some are displaying sanitized data, and some are displaying the original data.

I am suggestion that the spec should clarify what should be the default behavior regarding sanitization.

Additional context.

This issue is talking about missing examples, which might also clarify the behavior

[haddasbronfman]

I would expect the default behavior would be not to present sensitive data.
It can be by using a default serializer function that omits the sensitive data, and it can be by a boolean configuration that the user should set if he wants to see his data.
This way we can make sure that he is aware of what he is doing.

(in js-contrib, redis instrumentation, has a default serializer that returns only the non-sensitive data:
https://github.com/open-telemetry/opentelemetry-js-contrib/blob/main/packages/opentelemetry-redis-common/src/index.ts#L57

and in mongo db we have the same behavior: (https://github.com/open-telemetry/opentelemetry-js-contrib/blob/main/plugins/node/opentelemetry-instrumentation-mongodb/src/instrumentation.ts#L606).

[tombruijn]

I agree that sanitizing the queries and other sensitive data by default would be a good idea. People can always choose to send the raw query if there's a config option for it.

In most cases sending unsanitized queries will likely leak PII data out to other systems. Something that could put companies and people in some legal trouble and open them up for security headaches.
It's also good that the people with access to the instrumented app data can't find user email addresses, passwords, addresses, phone numbers, etc. in traces.

Providing developers with config options to sanitize queries is good, but since it is not the default and configured by the user it may be less well tested and not cover all cases, or be forgotten when the instrumentation is added.

We make sure that anything we detect as an SQL queries is run through our own sql_lexer library to sanitize them before their send to our servers. But we can't do that for everything, like Redis queries, and it would be best to put the query sanitization in the instrumentation packages itself.

[nemoshlag]

I think it should be default sanitized and mentioned explicitly. Instead of removing db.statement attribute completely, can we set its value to "sanitized"? That way users can differentiate between empty statements/lack of instrumentation/sanitization.

[tombruijn]

I wouldn't say it needs to always remove the attribute to sanitize, or replace the value in its entirety. If the instrumentation package can store a sanitized version, that would still be helpful to any person who views the span data.

For example, a sanitized SQL query could look like this SELECT * FROM table WHERE email = "?", where the sanitized value is the question mark. That people can still recognize which queries are being performed, without all the email addresses, passwords and other things one wouldn't want to collect.

[carlosalberto]

Btw OTel Java does this at this moment. Maybe it would be helpful to replicate this behavior in other languages, sharing the parsing rules? See this Java-infused Lex definition: https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/instrumentation-api-semconv/src/main/jflex/SqlSanitizer.jflex

[pellared]

Related thing. I think that if the instrumentation library does not sanitize the db.statement then the attribute should not be added unless explicitly configured (it should be opt-in in that case). This is e.g. what https://github.com/open-telemetry/opentelemetry-dotnet/tree/main/src/OpenTelemetry.Instrumentation.SqlClient does. The default options should be safe.

I know that some security engineers would even say that emitting db.statement should be always disabled by default as there are also "dynamic database structures" where e.g. table names represent usernames (but I think it goes to too far 😉 ).

[arielvalentin]

+1 in favor of sanitizing by default or omitting if the instrumentation (or library) does not support it.

We offer options in the Ruby instrumentations of omit, include, obfuscate (probably should be sanitize) e.g. https://github.com/open-telemetry/opentelemetry-ruby-contrib/blob/main/instrumentation/pg/lib/opentelemetry/instrumentation/pg/instrumentation.rb#L28

It would be great to have standard option names and reduce the likelihood of accidentally exporting sensitive data.

[dubonzi]

+1 in favor of sanitizing by default or omitting if the instrumentation (or library) does not support it.

It would be great to have standard option names and reduce the likelihood of accidentally exporting sensitive data.

Also +1 for sanitizing by default and ommiting if theres no support/implementation for it by the instrumentation.

The Go instrumentation for MongoDB doesn't sanitize the query, I had to warn another team at work about it because they deal with sensitive data and was about to migrate to open telemetry.

(see open-telemetry/opentelemetry-go-contrib#3519)

[crebbo]

This is a huge red flag for capturing db statements by default in tracing data, surely?

Trying to learn more about this behaviour currently, but I'm seeing our DynamoDB payload being captured in tracing span data (db.statement) when using the AWS ADOT collector for lambda out of the box (nodeJS).

Is there an easy way/environment var to stop this behaviour?

I'd previously seen Datadog was doing something similar with their python dd-trace, but they quickly patched this when I highlighted it.

Any pointers appreciated, will keep investigating

EDIT: I guess the best thing to do here is have custom processors which delete/hash known attributes that could contain sensitive data? An environment var to do this also would be nice, but can’t see that.

[nemoshlag]

+1 in favor of sanitizing by default or omitting if the instrumentation (or library) does not support it.

It would be great to have standard option names and reduce the likelihood of accidentally exporting sensitive data.

Also +1
I think this issue requires a decision since the current status where sensitive data can leak by default might prevent users from using otel

[arminru]

From the spec triage meeting: Accepting this one in retrospect as it looks like there's agreement to move forward as per the discussion on the PR.