HTTP Span Attributes: http.url must not contain username / password

[pellared]

As is stated in the recent specification change :

http.url MUST NOT contain credentials passed via URL in form of https://username:[email protected]/. In such case the attribute's value should be https://www.example.com/

[mateuszrzeszutek]

I don't think that otel-java contains any usage of the http.url attribute -- the javaagent is the correct place for this issue.

[pellared]

@mateuszrzeszutek I think this example could be reviewed just to make sure that others do not copy-paste potentially unsafe code.

[Hangzhi]

@jkwatson @pellared Hello, I'm a student applying for Outreachy 2021 and in the process of contributing to the community. This issue seems like a good start. Could you please assign this issue to me?

[anuraaga]

Hi @Hangzhi - thanks for the help on this. You can find where we set the attribute, with no scrubbing of credentials I think, here

https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/tracer/HttpClientTracer.java#L186

[pellared]

@Hangzhi I think you can also look at this: open-telemetry/opentelemetry-java#3118 which should be also simpler😉

[Hangzhi]

@pellared Thanks! I'll work on that.

[iNikem]

There are two ways to solve this: either to force every instrumentation to take care removing user info from the url, or put that responsibility onto HttpClientAttributesExtractor. I think the latter is much better. But then the question arises should HttpClientAttributesExtractor#url return String which we then convert to URL, check userInfo and convert back to String; or should it return URL/URI. I like the latter more. And the majority of libraries can do that, except for ApacheHttpClient instrumentations which has access only to string form of url. Thus they will have to parse it into URL/URI. But all those conversions are 95% of the time redundant, because there is now user info anyway.

@open-telemetry/java-maintainers, your thoughts?

[mateuszrzeszutek]

Some HTTP clients return their own URI types - I briefly took a look at the instrumentations we have and saw Uri, GenericUrl, URIAuthority + ClassicHttpRequest (one of Apache instrumentations), URL, HttpUrl.

Just a wild idea: maybe we could add a wrapper interface HttpUrl that exposes scheme(), host(), ..., fragment() and is able to bridge all those URL implementations? So that the extractor methods could look like

public HttpUrl url(Request request) {
  java.net.URI uri = request.getUrl();
  return HttpUrl.from(uri);
}

[anuraaga]

Using URI would be nice if we do have good representation in the instrumentations - looks like @mateuszrzeszutek has found that it may not be the case though.

Since we expect to not have to parse in most cases, I suspect we could avoid the parse and do a very simple scan to find userinfo within the String if URI seems impractical.

Not a huge fan in introducing the hundredth Java interface for URL :P

[trask]

I suspect we could avoid the parse and do a very simple scan to find userinfo within the String

👍

[MALPI]

Hey folks,

I would be happy to contribute here.

[mateuszrzeszutek]

Thanks! I'll assign this issue to you.