elastic / apm

Elastic Application Performance Monitoring - resources and general issue tracking for Elastic APM.
https://www.elastic.co/apm
Apache License 2.0
384 stars 114 forks source link

chore: update sanitization spec #796

Closed david-luna closed 1 year ago

david-luna commented 1 year ago

This PR tries to clarify in the spec who the agents are already dealing with request cookie info and sanitization. Also it tries to remove ambiguity on how cookie data is placed into transaction.context.request to avoid data redundancy on the payloads sent to APM server

gregkalapos commented 1 year ago

A very related topic is that currently only set-cookie is in the default list of sanitize_field_names (defined just above the changed lines). I suggest we consider adding cookie to that list.

felixbarny commented 1 year ago

I suggest we consider adding cookie to that list.

I think that's handled by agents removing the cookie header in favor of the transaction.context.request.cookies field and also applying transaction.context.request.cookies for that dedicated cookie field. Not sure if that's a cross-agent thing but that's what the Java agent does: https://github.com/elastic/apm-agent-java/blob/main/apm-agent-core/src/main/java/co/elastic/apm/agent/impl/context/SanitizingWebProcessor.java

david-luna commented 1 year ago

Not all agents behave the same. In this discussion we found out implementations differ. In summary:

So implementations that have both cookie header and transaction.context.request.cookies may send duplicated info. The cookie values that are not redacted. The 2nd paragraph of this PR wants to spec to avoid this redundancy

trentm commented 1 year ago

Gah! Sorry I merged this instead of "approving" it. My bad.

That said, it has the required set of approvals and just hasn't waited the mandated 7 days. I don't see a reason to revert it to just merge it a few days later.

@david-luna Did we happen to know off hand which, if any, APM agents currently need to change their behaviour -- i.e. those that should get an implementation issue?