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

Remove high cardanility metrics from otelhttp #4277

Merged
merged 9 commits into from
Sep 7, 2023

Conversation

MadVikingGod
Copy link
Contributor

@MadVikingGod MadVikingGod commented Sep 5, 2023

This does not upgrade to the latest semantic convention, that will be a different PR.

This removes the following attributes from the metrics that go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp generates:

  • net.sock.peer.addr
  • net.sock.peer.port
  • http.user_agent
  • enduser.id
  • http.client_ip

This also only allows http.request.method to be known HTTP methods, or _OTHER.

Resolve open-telemetry/opentelemetry-go#3765

@codecov
Copy link

codecov bot commented Sep 5, 2023

Codecov Report

Merging #4277 (3541453) into main (b6fc62f) will increase coverage by 0.0%.
The diff coverage is 82.6%.

Impacted file tree graph

@@          Coverage Diff           @@
##            main   #4277    +/-   ##
======================================
  Coverage   79.4%   79.5%            
======================================
  Files        166     166            
  Lines      10376   10677   +301     
======================================
+ Hits        8246    8491   +245     
- Misses      1996    2045    +49     
- Partials     134     141     +7     
Files Changed Coverage
...stful/otelrestful/internal/semconvutil/httpconv.go 82.6%
...gonic/gin/otelgin/internal/semconvutil/httpconv.go 82.6%
...rilla/mux/otelmux/internal/semconvutil/httpconv.go 82.6%
...ack/echo/otelecho/internal/semconvutil/httpconv.go 82.6%
...on.v1/otelmacaron/internal/semconvutil/httpconv.go 82.6%
...ace/otelhttptrace/internal/semconvutil/httpconv.go 82.6%
...net/http/otelhttp/internal/semconvutil/httpconv.go 82.6%
instrumentation/net/http/otelhttp/handler.go 100.0%

internal/shared/semconvutil/httpconv.go.tmpl Outdated Show resolved Hide resolved
internal/shared/semconvutil/httpconv.go.tmpl Outdated Show resolved Hide resolved
internal/shared/semconvutil/httpconv.go.tmpl Outdated Show resolved Hide resolved
@MrAlias MrAlias merged commit 50ca48f into open-telemetry:main Sep 7, 2023
27 checks passed
@MadVikingGod MadVikingGod added this to the v0.44.0 milestone Sep 7, 2023
@davendu
Copy link

davendu commented Sep 12, 2023

Weird. Why @dmathieu 's previous suggestion is not applied to this PR:

I don't think this is the proper approach. It's adding some weird complexity, and whether an attribute has too much cardinality or not is very subjective.

The solution being discussed in open-telemetry/opentelemetry-specification#3061 seems much better.

And what's the community's suggestion towards similar attributes? My own instrumentation library also needs some high cardanility attributes, and not sure if I should remove them or filter through the view.

@dmathieu
Copy link
Member

This PR solves from a potential memory leak issue, since the values from those attributes are coming from untrusted sources.
We should be able to bring back those attributes once something like open-telemetry/opentelemetry-go#4457 is stable.

@FaranIdo
Copy link
Contributor

@dmathieu @pellared @MadVikingGod do you plan to add a similar solution to otelgrpc to avoid high cardinality there as well? (I think reported here: #3071)

rhnasc pushed a commit to inloco/loki that referenced this pull request Apr 12, 2024
…elhttp to v0.44.0 [SECURITY] (main) (grafana#11002)

[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
|
[go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp](https://github.com/open-telemetry/opentelemetry-go-contrib)
| indirect | minor | `v0.42.0` -> `v0.44.0` |

### GitHub Vulnerability Alerts

####
[CVE-2023-45142](https://github.com/open-telemetry/opentelemetry-go-contrib/security/advisories/GHSA-5r5m-65gx-7vrh)

### Summary

This handler wrapper
https://github.com/open-telemetry/opentelemetry-go-contrib/blob/5f7e6ad5a49b45df45f61a1deb29d7f1158032df/instrumentation/net/http/otelhttp/handler.go#L63-L65
out of the box adds labels

- `http.user_agent`
- `http.method`

that have unbound cardinality. It leads to the server's potential memory
exhaustion when many malicious requests are sent to it.

### Details

HTTP header User-Agent or HTTP method for requests can be easily set by
an attacker to be random and long. The library internally uses
[httpconv.ServerRequest](https://github.com/open-telemetry/opentelemetry-go/blob/v1.12.0/semconv/internal/v2/http.go#L159)
that records every value for HTTP
[method](https://github.com/open-telemetry/opentelemetry-go/blob/38e1b499c3da3107694ad2660b3888eee9c8b896/semconv/internal/v2/http.go#L204)
and
[User-Agent](https://github.com/open-telemetry/opentelemetry-go/blob/38e1b499c3da3107694ad2660b3888eee9c8b896/semconv/internal/v2/http.go#L223).

### PoC

Send many requests with long randomly generated HTTP methods or/and User
agents (e.g. a million) and observe how memory consumption increases
during it.

### Impact

In order to be affected, the program has to configure a metrics
pipeline, use
[otelhttp.NewHandler](https://github.com/open-telemetry/opentelemetry-go-contrib/blob/5f7e6ad5a49b45df45f61a1deb29d7f1158032df/instrumentation/net/http/otelhttp/handler.go#L63-L65)
wrapper, and does not filter any unknown HTTP methods or User agents on
the level of CDN, LB, previous middleware, etc.

### Others

It is similar to already reported vulnerabilities
-
GHSA-5r5m-65gx-7vrh
([open-telemetry/opentelemetry-go-contrib](https://github.com/open-telemetry/opentelemetry-go-contrib))
- GHSA-cg3q-j54f-5p7p
([prometheus/client_golang](https://github.com/prometheus/client_golang))

### Workaround for affected versions

As a workaround to stop being affected
[otelhttp.WithFilter()](https://pkg.go.dev/go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp/filters)
can be used, but it requires manual careful configuration to not log
certain requests entirely.

For convenience and safe usage of this library, it should by default
mark with the label `unknown` non-standard HTTP methods and User agents
to show that such requests were made but do not increase cardinality. In
case someone wants to stay with the current behavior, library API should
allow to enable it.

The other possibility is to disable HTTP metrics instrumentation by
passing
[`otelhttp.WithMeterProvider`](https://pkg.go.dev/go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp#WithMeterProvider)
option with
[`noop.NewMeterProvider`](https://pkg.go.dev/go.opentelemetry.io/otel/metric/noop#NewMeterProvider).

### Solution provided by upgrading

In PR
[open-telemetry/opentelemetry-go-contrib#4277,
released with package version 0.44.0, the values collected for attribute
`http.request.method` were changed to be restricted to a set of
well-known values and other high cardinality attributes were removed.

### References

-
[open-telemetry/opentelemetry-go-contrib#4277
-
https://github.com/open-telemetry/opentelemetry-go-contrib/releases/tag/v1.19.0

---

### Configuration

📅 **Schedule**: Branch creation - "" (UTC), Automerge - At any time (no
schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/grafana/loki).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4xOS4yIiwidXBkYXRlZEluVmVyIjoiMzcuMzEuNSIsInRhcmdldEJyYW5jaCI6Im1haW4ifQ==-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Metrics memory leak v1.12.0/v0.35.0 and up
7 participants