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.

Sign up for GitHub

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

Jump to bottom

feat(cmd/agent): add fallback X.509 trusted roots #6340

Merged
merged 4 commits into from
Feb 15, 2024
Merged

feat(cmd/agent): add fallback X.509 trusted roots #6340

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a27bf8e
feat(cmd/agent): add fallback X.509 trusted roots
hainenber Feb 9, 2024
06a6550
chore(doc): add CHANGELOG entry
hainenber Feb 9, 2024
505a136
Update CHANGELOG.md
ptodev Feb 9, 2024
3385ae1
Merge branch 'main' into add-fallback-x509-root-cert
hainenber Feb 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,9 @@ Main (unreleased)

- Updated docs for MSSQL Integration to show additional authentication capabilities. (@StefanKurek)

- `grafana-agent` and `grafana-agent-flow` fallback to default X.509 trusted root certificates
when the `GODEBUG=x509usefallbackroots=1` environment variable is set. (@hainenber)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't documented anywhere (that I can find). Is this something we should add to the HTTP or TLS docs?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the block configuration we normally only document the arguments which are in the block. I haven't seen documentation about environment variables in block documentation. I think we should make a special page for the env vars and link to it from the block docs.

@jkroepke could you please let us know which Agent components you are using in this case? And why is it not sufficient to use an argument such as ca_pem in the tls config block?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

docs for

GODEBUG=x509usefallbackroots=1

https://pkg.go.dev/crypto/x509#SetFallbackRoots

I'm plan to use it for

  • module.http
  • remote.http
  • prometheus.remote_write
  • loki.write

And why is it not sufficient to use an argument such as ca_pem in the tls config block?

The ca_pem block is sufficient. But then I have to ship the whole ca-certificates pem file together with the setup. Using the fallback X.509 trusted roots from go also fits well and would eliminate the requirement of shipping a own ca pem file.

An other benefit is if the root CAs are embedded:

  • grafana-agent could run in a FROM scratch docker image which reduce the attack surface to a minimum. Since grafana may has very high permissions in a Kubernetes cluster, reducing the dependencies to a minimum would increase the security as well.

Configure the ca_pem, but using the trust ca-certificates sounds silly for first time, but using a custom ca certificates file skips the system verifier. The issue with the system verifier under Windows is that go has no capabilities to configure flags for the Windows Crypto API.

Context: golang/go#63238 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jkroepke Thanks for clarifying - this also confirms what I inferred from the issue so far.

@clayton-cornell If we are to document such env vars, we would also need to document how they could interfere with arguments in the River config, and which one takes precedence. I could raise a separate issue for this? I don't think it's a must for now. In the past, I've seen this issue mostly with the HTTP_PROXY env var.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ptodev I'd say drop it into an issue and backlog it for a future doc improvement. It's not critical right now... it's just something I wondered about as I read through the current change, and it would certainly be helpful to have this added at some point.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@clayton-cornell I created a new issue - #6372


v0.39.2 (2024-1-31)
--------------------

Expand Down
4 changes: 4 additions & 0 deletions cmd/grafana-agent-flow/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,10 @@ import (

// Register integrations
_ "github.com/grafana/agent/pkg/integrations/install"

// Embed a set of fallback X.509 trusted roots
// Allows the app to work correctly even when the OS does not provide a verifier or systems roots pool
_ "golang.org/x/crypto/x509roots/fallback"
)

func init() {
Expand Down
4 changes: 4 additions & 0 deletions cmd/grafana-agent/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,10 @@ import (

// Register integrations
_ "github.com/grafana/agent/pkg/integrations/install"

// Embed a set of fallback X.509 trusted roots
// Allows the app to work correctly even when the OS does not provide a verifier or systems roots pool
_ "golang.org/x/crypto/x509roots/fallback"
)

func init() {
Expand Down
1 change: 1 addition & 0 deletions go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -616,6 +616,7 @@ require (
github.com/open-telemetry/opentelemetry-collector-contrib/receiver/prometheusreceiver v0.87.0
github.com/open-telemetry/opentelemetry-collector-contrib/receiver/vcenterreceiver v0.87.0
go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetrichttp v0.42.0
golang.org/x/crypto/x509roots/fallback v0.0.0-20240208163226-62c9f1799c91
k8s.io/apimachinery v0.28.3
)

Expand Down
2 changes: 2 additions & 0 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2513,6 +2513,8 @@ golang.org/x/crypto v0.7.0/go.mod h1:pYwdfH91IfpZVANVyUOhSIPZaFoJGxTFbZhFTx+dXZU
golang.org/x/crypto v0.10.0/go.mod h1:o4eNf7Ede1fv+hwOwZsTHl9EsPFO6q6ZvYR8vYfY45I=
golang.org/x/crypto v0.18.0 h1:PGVlW0xEltQnzFZ55hkuX5+KLyrMYhHld1YHO4AKcdc=
golang.org/x/crypto v0.18.0/go.mod h1:R0j02AL6hcrfOiy9T4ZYp/rcWeMxM3L6QYxlOuEG1mg=
golang.org/x/crypto/x509roots/fallback v0.0.0-20240208163226-62c9f1799c91 h1:Lyizcy9jX02jYR0ceBkL6S+jRys8Uepf7wt1vrz6Ras=
golang.org/x/crypto/x509roots/fallback v0.0.0-20240208163226-62c9f1799c91/go.mod h1:kNa9WdvYnzFwC79zRpLRMJbdEFlhyM5RPFBBZp/wWH8=
golang.org/x/exp v0.0.0-20180321215751-8460e604b9de/go.mod h1:CJ0aWSM057203Lf6IL+f9T1iT9GByDxfZKAQTCR3kQA=
golang.org/x/exp v0.0.0-20180807140117-3d87b88a115f/go.mod h1:CJ0aWSM057203Lf6IL+f9T1iT9GByDxfZKAQTCR3kQA=
golang.org/x/exp v0.0.0-20190121172915-509febef88a4/go.mod h1:CJ0aWSM057203Lf6IL+f9T1iT9GByDxfZKAQTCR3kQA=
Expand Down
Loading