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
…n/net/http/otelhttp to v0.44.0 [security] (main) (grafana#10917)

[![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://togithub.com/open-telemetry/opentelemetry-go-contrib)
| indirect | minor | `v0.42.0` -> `v0.44.0` |

### GitHub Vulnerability Alerts

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

### Summary

OpenTelemetry-Go Contrib has a [handler wrapper
`otelhttp`](https://togithub.com/open-telemetry/opentelemetry-go-contrib/blob/5f7e6ad5a49b45df45f61a1deb29d7f1158032df/instrumentation/net/http/otelhttp/handler.go#L63-L65)
that adds the following labels by deafult that have unbound cardinality:

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

This 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://togithub.com/open-telemetry/opentelemetry-go/blob/v1.12.0/semconv/internal/v2/http.go#L159)
that records every value for HTTP
[method](https://togithub.com/open-telemetry/opentelemetry-go/blob/38e1b499c3da3107694ad2660b3888eee9c8b896/semconv/internal/v2/http.go#L204)
and
[User-Agent](https://togithub.com/open-telemetry/opentelemetry-go/blob/38e1b499c3da3107694ad2660b3888eee9c8b896/semconv/internal/v2/http.go#L223).

[This pull
request](https://togithub.com/open-telemetry/opentelemetry-go-contrib/pull/4277)
released with version 0.44.0 dixes this vulnerability 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.

### Impact

In order to be affected program has to use
[otelhttp.NewHandler](https://togithub.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

This vulnerability is similar but different from these known
vulnerabilities:
-
GHSA-5r5m-65gx-7vrh
([open-telemetry/opentelemetry-go-contrib](https://togithub.com/open-telemetry/opentelemetry-go-contrib))
- GHSA-cg3q-j54f-5p7p
([prometheus/client_golang](https://togithub.com/prometheus/client_golang))

### Workaround for affected versions

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

---

### Memory exhaustion in
github.com/open-telemetry/opentelemetry-go-contrib
[CVE-2023-45142](https://nvd.nist.gov/vuln/detail/CVE-2023-45142) /
[GHSA-rcjv-mgp8-qvmr](https://togithub.com/advisories/GHSA-rcjv-mgp8-qvmr)
/ [GO-2023-2113](https://pkg.go.dev/vuln/GO-2023-2113)

<details>
<summary>More information</summary>

#### Details
Memory exhaustion in github.com/open-telemetry/opentelemetry-go-contrib

#### Severity
Unknown

#### References
-
[https://github.com/open-telemetry/opentelemetry-go-contrib/security/advisories/GHSA-rcjv-mgp8-qvmr](https://togithub.com/open-telemetry/opentelemetry-go-contrib/security/advisories/GHSA-rcjv-mgp8-qvmr)
-
[open-telemetry/opentelemetry-go-contrib#4277

This data is provided by
[OSV](https://osv.dev/vulnerability/GO-2023-2113) and the [Go
Vulnerability Database](https://togithub.com/golang/vulndb) ([CC-BY
4.0](https://togithub.com/golang/vulndb#license)).
</details>

---

### OpenTelemetry-Go Contrib vulnerable to denial of service in otelhttp
due to unbound cardinality metrics
[CVE-2023-45142](https://nvd.nist.gov/vuln/detail/CVE-2023-45142) /
[GHSA-rcjv-mgp8-qvmr](https://togithub.com/advisories/GHSA-rcjv-mgp8-qvmr)

<details>
<summary>More information</summary>

#### Details
##### Summary

OpenTelemetry-Go Contrib has a [handler wrapper
`otelhttp`](https://togithub.com/open-telemetry/opentelemetry-go-contrib/blob/5f7e6ad5a49b45df45f61a1deb29d7f1158032df/instrumentation/net/http/otelhttp/handler.go#L63-L65)
that adds the following labels by deafult that have unbound cardinality:

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

This 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://togithub.com/open-telemetry/opentelemetry-go/blob/v1.12.0/semconv/internal/v2/http.go#L159)
that records every value for HTTP
[method](https://togithub.com/open-telemetry/opentelemetry-go/blob/38e1b499c3da3107694ad2660b3888eee9c8b896/semconv/internal/v2/http.go#L204)
and
[User-Agent](https://togithub.com/open-telemetry/opentelemetry-go/blob/38e1b499c3da3107694ad2660b3888eee9c8b896/semconv/internal/v2/http.go#L223).

[This pull
request](https://togithub.com/open-telemetry/opentelemetry-go-contrib/pull/4277)
released with version 0.44.0 dixes this vulnerability 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.

##### Impact

In order to be affected program has to use
[otelhttp.NewHandler](https://togithub.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

This vulnerability is similar but different from these known
vulnerabilities:
-
GHSA-5r5m-65gx-7vrh
([open-telemetry/opentelemetry-go-contrib](https://togithub.com/open-telemetry/opentelemetry-go-contrib))
- GHSA-cg3q-j54f-5p7p
([prometheus/client_golang](https://togithub.com/prometheus/client_golang))

##### Workaround for affected versions

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

#### Severity
- CVSS Score: 7.5 / 10 (High)
- Vector String: `CVSS:3.1/AV:N/AC:L/PR:N/UI:N/S:U/C:N/I:N/A:H`

#### References
-
[https://github.com/open-telemetry/opentelemetry-go-contrib/security/advisories/GHSA-5r5m-65gx-7vrh](https://togithub.com/open-telemetry/opentelemetry-go-contrib/security/advisories/GHSA-5r5m-65gx-7vrh)
-
[https://github.com/open-telemetry/opentelemetry-go-contrib/security/advisories/GHSA-rcjv-mgp8-qvmr](https://togithub.com/open-telemetry/opentelemetry-go-contrib/security/advisories/GHSA-rcjv-mgp8-qvmr)
-
[https://nvd.nist.gov/vuln/detail/CVE-2023-45142](https://nvd.nist.gov/vuln/detail/CVE-2023-45142)
-
[open-telemetry/opentelemetry-go-contrib#4277
-
[https://github.com/advisories/GHSA-cg3q-j54f-5p7p](https://togithub.com/advisories/GHSA-cg3q-j54f-5p7p)
-
[https://github.com/open-telemetry/opentelemetry-go-contrib](https://togithub.com/open-telemetry/opentelemetry-go-contrib)
-
[https://github.com/open-telemetry/opentelemetry-go-contrib/blob/5f7e6ad5a49b45df45f61a1deb29d7f1158032df/instrumentation/net/http/otelhttp/handler.go#L63-L65](https://togithub.com/open-telemetry/opentelemetry-go-contrib/blob/5f7e6ad5a49b45df45f61a1deb29d7f1158032df/instrumentation/net/http/otelhttp/handler.go#L63-L65)
-
[https://github.com/open-telemetry/opentelemetry-go-contrib/releases/tag/v1.19.0](https://togithub.com/open-telemetry/opentelemetry-go-contrib/releases/tag/v1.19.0)
-
[https://github.com/open-telemetry/opentelemetry-go/blob/38e1b499c3da3107694ad2660b3888eee9c8b896/semconv/internal/v2/http.go#L223](https://togithub.com/open-telemetry/opentelemetry-go/blob/38e1b499c3da3107694ad2660b3888eee9c8b896/semconv/internal/v2/http.go#L223)
-
[https://github.com/open-telemetry/opentelemetry-go/blob/v1.12.0/semconv/internal/v2/http.go#L159](https://togithub.com/open-telemetry/opentelemetry-go/blob/v1.12.0/semconv/internal/v2/http.go#L159)

This data is provided by
[OSV](https://osv.dev/vulnerability/GHSA-rcjv-mgp8-qvmr) and the [GitHub
Advisory Database](https://togithub.com/github/advisory-database)
([CC-BY
4.0](https://togithub.com/github/advisory-database/blob/main/LICENSE.md)).
</details>

---

### 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:eyJjcmVhdGVkSW5WZXIiOiIzNy4xOS4yIiwidXBkYXRlZEluVmVyIjoiMzcuMTkuMiIsInRhcmdldEJyYW5jaCI6Im1haW4ifQ==-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
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://togithub.com/open-telemetry/opentelemetry-go-contrib)
| indirect | minor | `v0.42.0` -> `v0.44.0` |

### GitHub Vulnerability Alerts

####
[CVE-2023-45142](https://togithub.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://togithub.com/open-telemetry/opentelemetry-go/blob/v1.12.0/semconv/internal/v2/http.go#L159)
that records every value for HTTP
[method](https://togithub.com/open-telemetry/opentelemetry-go/blob/38e1b499c3da3107694ad2660b3888eee9c8b896/semconv/internal/v2/http.go#L204)
and
[User-Agent](https://togithub.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://togithub.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://togithub.com/open-telemetry/opentelemetry-go-contrib))
- GHSA-cg3q-j54f-5p7p
([prometheus/client_golang](https://togithub.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