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

Don't save pg.values on span #11

Closed
pauldraper opened this issue Mar 28, 2020 · 8 comments
Closed

Don't save pg.values on span #11

pauldraper opened this issue Mar 28, 2020 · 8 comments
Assignees
Labels
enhancement New feature or request up-for-grabs Good for taking. Extra help will be provided by maintainers

Comments

@pauldraper
Copy link
Contributor

Or at least make it a configuration option.

  1. They are frequently sensitive. (There is more general issue with masking sensitive information, but query values are very often sensitive.)

  2. They can be extremely large. Especially arrays.

@pauldraper pauldraper changed the title Don't log pg.values Don't save pg.values on span Mar 28, 2020
@vmarchaud
Copy link
Member

For the record mysql, ioredis and redis does it too. Mongodb is the only one that "hide" values by replacing them with ?

@vmarchaud vmarchaud transferred this issue from open-telemetry/opentelemetry-js Apr 25, 2020
@vmarchaud vmarchaud added enhancement New feature or request up-for-grabs Good for taking. Extra help will be provided by maintainers labels Apr 25, 2020
@sergioregueira
Copy link
Contributor

sergioregueira commented Aug 13, 2020

Storing pg.values by default will eventually leak sensitive data such as user credentials. This issue should be fixed ASAP.

If you agree, I will immediately submit a PR to make it optional unless a storeValues flag is set to true explicitly following an approach similar to opentelemetry-plugin-mongodb.

@dyladan
Copy link
Member

dyladan commented Aug 14, 2020

What does the spec say about it currently? It was my understanding that data scrubbing was supposed to happen in the collector?

@sergioregueira
Copy link
Contributor

On the one hand, Semantic conventions for General Identity Attributes says the following about enduser.id:

Given the sensitive nature of this information, SDKs and exporters SHOULD drop these attributes by default and then provide a configuration parameter to turn on retention for use cases where the information is required and would not violate any policies or regulations.

On the other hand, Semantic conventions for database Call-level attributes:

  • db.statement may be sanitized to exclude sensitive information.
  • It is recommended to remove embedded credentials from db.connection_string.
  • (no explicit mention to query parameters in the whole page)

IMHO, despite you can filter it out later in the collector/exporter, the data is so sensitive that we should not store it nor send it anywhere unless it is explicitly requested; as MongoDB plugin does with enhancedDatabaseReporting (plugin options).

Anyway, I think that the plugin should warn that it currently logs query parameters by default.

@dyladan
Copy link
Member

dyladan commented Aug 14, 2020

I think that spec wording is sufficient to change to scrub by default. We may want to provide a config option to capture query values, but I agree it is very sensitive and we shouldn't export it if we can avoid it.

@sergioregueira
Copy link
Contributor

sergioregueira commented Aug 14, 2020

In that case, I would like to contribute submitting that pull request before the end of the week.

The plugin will not add that pg.values attribute to the span unless enhancedDatabaseReporting flag is explicitly set to true on plugin config.

I will replicate as well the behavior in the plugins that @vmarchaud said once this one is approved.

@nwalters512
Copy link
Contributor

@pauldraper I think #174 should have resolved this - can this issue be closed now?

@pauldraper
Copy link
Contributor Author

Yes, thanks!

RafalSumislawski added a commit to coralogix/opentelemetry-js-contrib-esbuild-node-plugin that referenced this issue Nov 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request up-for-grabs Good for taking. Extra help will be provided by maintainers
Projects
None yet
Development

No branches or pull requests

5 participants