Sensitive Data Handling

[johnbley]

I present a proposal for a design philosophy around handling potentially-sensitive data in our libraries, using SQL as an example throughout.

[carlosalberto]

Overall great. This feels like a good start 👍

[arminru]

That's a pretty bold statement. Are we sure we will be able to ensure this 100%?

[johnbley]

It's a design goal. Bugs are probably inevitable 😄

[dyladan]

I think armin was asking if this is a design goal we want. The term libraries could mean anything from API to SDK to Instrumentation Libraries. I think the strongest statement we could make would be:

Suggested change

	By default, OpenTelemetry libraries never capture potentially-sensitive data, except for the full URL.
	By default, instrumentation libraries provided by OpenTelemetry make a best effort to never capture potentially-sensitive data, except in cases like the full URL where the data is necessary to the observability of the system.

[arminru]

What are "OpenTelemetry libraries"? Are these the API + SDK implementations or instrumentations/integrations that are provided by the OpenTelemetry project?

[johnbley]

This would include any current and future auto-instrumenter (like Java, .NET, node.js) as well as any manually wrapped libraries(e.g., in a contrib package).

[arminru]

I see. It would make sense to explicitly mention the scope to which this OTEP and its design goals apply to in the document.

[arminru]

Is the Java auto-instrumentation mentioned as an example or does this only apply to the Java auto-instrumentation? Please clarify.
If this is about all kinds of instrumentations, do you intend to redact all set attributes by default?

[johnbley]

Sticking with the sql example, it would also apply to the node.js instrumentation in opentelemetry-js, including the opentelemetry-plugin-mysql and opentelemetry-plugin-postgres packages. It doesn't look like the nascent .NET auto agent would be affected (yet).
No, this wouldn't apply a single rule to all set attributes. We assume manual instrumentation knows what it is doing. Since we are writing the instrumentation for auto-instrumenters, we are aware of the semantics of the attribute values and can apply appropriate rules (e.g., we know that the numbers in http.status_code are not risks, but that a database connection string might contain a password.

[arminru]

Does this work for all sorts of SQL dialects out there?

[johnbley]

Yes, a sql lexer can easily broadly apply to many vendors' dialects, because it's only interested in tokenization and not in syntax/grammar.

[yurishkuro]

This means that sql like SELECT * FROM USERS WHERE ID=123456789 gets turned into
SELECT * FROM USERS WHERE ID=? when it gets put into our db.statement attribute.

I don't see how this can possibly be done efficiently inside the language SDKs, as opposed to scrubbing this in the ingestion pipeline.

[tsloughter]

Only seeing this otep now as it was linked to from another newer otep -- I'd like to see something like this added so going over this one now.

I don't think this is a good example since the db statement should already be parameterized, like an extended query in postgres. Are there many databases that don't support a form of query + args like found in SQL databases?

So for db instrumentation we should document to the user that they should not manually construct queries with dynamic data in general but most importantly for the case of instrumentation, to not do it with sensitive data.

And instead the query parameter attributes need to be either not included at all or have the ability to be filtered by key.

This would require no SQL parsing library and be doable in all of the implementations.

Of course best practices aren't always an option so having the SQL parsing that can scrub all predicates in the collector would be a must have for some.

I also think this relates to span processors and how they need updating to provide functionality like this. I planned for that to be a follow up that builds on my OTEP #186 but since it has not been met with a positive reception so far I may have to rethink that :)

This OTEP is about a "broad agreement" and I agree with that but put the rest here because I think the broad agreement should include span transformations in the export pipeline that doesn't yet exist, and that one of those transformations should be a required configurable attribute scrubber.

[arminru]

Is this something to be configured in the UI of a backend or at the place where the instrumentation is set up? Are these bold red warnings supposed to show up in a log produced by the instrumentation or in the tracing backend's UI?

[johnbley]

Config would remain local to each library (a manual wrapper might provide a dictionary of options as an API parameter, an auto agent might use an environment variable or config file). The warning would appear in the documentation of this config knob.

[dyladan]

In general, the use of "we" here is confusing. Is this describing the approach you've taken at your company, or retroactively describing what you would like to see OpenTelemetry do?

[johnbley]

In general, the use of "we" here is confusing. Is this describing the approach you've taken at your company, or retroactively describing what you would like to see OpenTelemetry do?

I was following the recommendation in the template: "Explain the proposed change as though it was already implemented and you were explaining it to a user" and, yes, using "we" to mean "the whole OpenTelemetry community".

[jmacd]

I'm skeptical about the performance impact on the host application, somewhat, but I'm even less convinced that we're going to build out scrubbers for PII in every client language.

This seems to suggest a configuration where an OTel collector is co-located in the side-car configuration, so that it resides in the same security domain. Then the statement is "yes, by default we scrub the credit card number immediately after it crosses outside your process a locally running sidecar."

[yurishkuro]

+1. Making the promise that scrubbing will be done in-process in every language is a very large scope creep. Doing it once in a sidecar or central tier is much more sustainable and manageable.

[yurishkuro]

There is no consensus that we should be making the promise that scrubbing will be done in-process in every language. It's a significant scope creep. Doing it once in a sidecar or central tier is much more sustainable and manageable.

[bogdandrutu]

@yurishkuro +1 I think we should promise that we have it in the collector, but not in all the libraries.

[johnbley]

I hear and accept the concern around the cost of owning this in each language. I will rework the proposal so that the default system (including the collector) preserves the desired behavior, allowing but not requiring instrumentation libraries to do their own scrubbing. One design concern I have for this, though, is that the collector loses some semantic information that the instrumented process has. For example, we currently use db.statement for "plain" sql and also Mongo, Redis, Geode, Couchbase, etc. queries. Instrumentation libraries of course know which thing they're instrumenting and can apply appropriate logic. Under this design, how will the collector know which semantic transformations to apply?

[yurishkuro]

how will the collector know which semantic transformations to apply?

I think that is a matter for semantic data conventions. On of the other db.*** attributes should provide this clarification.

[arminru]

how will the collector know which semantic transformations to apply?

I think that is a matter for semantic data conventions. On of the other db.*** attributes should provide this clarification.

Exactly. The current spec has a required attribute db.type, which specifies the type of database being called. This attribute will be passed on to the collector untouched.

[lizthegrey]

+1 that data scrubbing, encryption/sanitization (a la https://docs.honeycomb.io/authentication-and-security/secure-tenancy/ or Lightstep's satellites) should be done on client's premises via a collector or satellite, but not necessarily in-process for every telemetry generating SDK.

[MovieStoreGuy]

@lizthegrey ,

In some scenarios, data scrubbing or data validation would need to be done in the application itself due to organisation policies that don't allow for the data to be processed by a third party application.

(Sorry, I have a similar OTEP that I am trying to advocate for)

[tedsuo]

@johnbley we are cleaning up stale OTEP PRs. If there is no further action at this time, we will close this PR in one week. Feel free to open it again when it is time to pick it back up.

[carlosalberto]

OTEPs have been moved to the Specification repository. Please consider re-opening this PR against the new location. Closing.