open-telemetry / oteps

OpenTelemetry Enhancement Proposals
https://opentelemetry.io
Apache License 2.0
337 stars 164 forks source link

Scope and roadmap for bringing the existing HTTP semantic conventions for tracing to an initial stable state #174

Closed denisivan0v closed 2 years ago

denisivan0v commented 3 years ago

Semantic conventions for tracing for HTTP are available, but are in an experimental state.

A workgroup focusing on HTTP conventions is requested and will work on bringing the existing semantic conventions for HTTP to a stable state once established.

This documents proposes a scope for an initial stable version of HTTP semantic conventions, as well as a roadmap. It should serve as a starting point for initial discussions in the workgroup and, once agreed on, define the further agenda of the workgroup.

linux-foundation-easycla[bot] commented 3 years ago

CLA Signed

The committers are authorized under a signed CLA.

WillsonHG commented 3 years ago

/easycla

lmolkova commented 3 years ago

Adding security concerns to the scope: http.target has a query string that may have credentials, by default web frameworks/http clients should not expose potentially sensitive information.

Oberon00 commented 3 years ago

@lmolkova

http.target has a query string that may have credentials

I have never heard of anybody including credentials in a query string. HTTP basic auth (user:password@hostname) is already addressed in the spec.

On the other hand, Java frameworks have been using the otherwise very obscure "matrix param" format with semicolons to include a session ID: https://stackoverflow.com/a/23600711/2128694 (URLs like http://www.example.com/context;JSESSIONID=12345?query_param1=ABC may look familiar).

In any case, http.target is security-sensitive even if you remove the query string. Nowadays, things that would previously have been put into query strings are often included in the path through HTTP route templates like /search/<term> instead of /search?q=<term>. User IDs, etc. are also often included.

Hostnames (subdomains) may also be user-specific or tenant-specific (common example are AWS login URLs like 1234567891011.signin.aws.amazon.com/console where the first part is the account ID). IPs are PII. Probably there is more.

lmolkova commented 3 years ago

I have never heard of anybody including credentials in a query string. HTTP basic auth (user:password@hostname) is already addressed in the spec.

https://docs.microsoft.com/en-us/azure/storage/common/storage-sas-overview#sas-token https://docs.aws.amazon.com/AmazonS3/latest/userguide/RESTAuthentication.html#RESTAuthenticationQueryStringAuth

Should we make the target a required attribute if it's sensitive? Perhaps we should define the default level of sanitization and how it can be customzed?

I don't want to go into the technical discussion here. This otep currently summarizes gaps/concerns on current spec on the way to stability, not proposes solutions.

tedsuo commented 2 years ago

just FYI, changing 4xx responses to no longer create error status codes on the server has already happened. I'd suggest putting "Error status default" as in-scope, and leave "configuration" as vNext.

denisivan0v commented 2 years ago

I'd suggest putting "Error status default" as in-scope, and leave "configuration" as vNext.

@tedsuo I've updated the OTEP adding the following in-scope for v1:

4xx responses are no longer create error status codes in case of `SpanKind.SERVER`. 
It seems reasonable to define the same/similar behavior for `SpanKind.CLIENT`.

An the following for the vNext:

In many cases 4xx error criteria depends on the app (e.g., for 404/409). As an
end user, I might want to have an ability to override existing defaults and
define what HTTP status codes count as errors.
MovieStoreGuy commented 2 years ago

So far this addition makes sense as an initial steps towards a first stable release with vNext incrementally adding more improvements.

denisivan0v commented 2 years ago

Thanks @yurishkuro. Within this PR/OTEP I'm aiming to outline the scope for v1.0 (and for vNext), so it's mostly about "we need to decide...". And specific solutions are proposed and being discussed as separated issued/PRs - I've added links to those to this OTEP.

yurishkuro commented 2 years ago

Within this PR/OTEP I'm aiming to outline the scope for v1.0 (and for vNext), so it's mostly about "we need to decide..."

This is not how the PR is written though. You have specific section that contain very concrete (and not yet agreed to) prescriptions like "these are required attributes" or "this is how retries must be handled". They do not at all read like "these are things we need to decide in other tickets". To me it's a blocker for approving this OTEP.

denisivan0v commented 2 years ago

@yurishkuro Fair enough. I've re-phrased sentences to avoid providing concrete prescriptions in this OTEP. Do you now see any section that might be improved further?

denisivan0v commented 2 years ago

@lmolkova Should the span suppression topic be added in scope for v1.0? /cc @tedsuo

lmolkova commented 2 years ago

@lmolkova Should the span suppression topic be added in scope for v1.0

I don't think suppression is a blocker for HTTP semantic conventions.

denisivan0v commented 2 years ago

@tigrannajaryan that's right, items in scope are going to be addressed via separate PRs in the specification repo.

@tedsuo can you please help to merge this PR?