Add auth support for collector #2570

jpkrohling · 2020-10-15T15:40:32Z

Which problem is this PR solving?

The first step towards the multi-tenancy solution: add auth support for both agent and collector.

Short description of the changes

This change uses the OpenTelemetry Collector support for OIDC authentication: on the client side (agent), we send a token with every RPC call. On the server side (collector), we verify this token against a OIDC server. On our side (Jaeger), all we have to do is configure the gRPC client/server settings.
Because of a combination of issues (Agent exporting directly to storage in OpenTelemetry distribution, all-in-one binary #2565 and Cannot start agent and collector on the same machine (OpenTelemetry distribution) #2566), this can only be tested with the Jaeger Agent from this PR against an OpenTelemetry Collector with a Jaeger Receiver, and with the OpenTelemetry Collector with Jaeger exporter sending data to the Jaeger Collector from this PR. For testing, Agent on OpenTelemetry distribution is started with collector #2568 will also be needed.

Testing the Agent part from this PR

Start the OpenTelemetry Collector with this configuration:

receivers:
  jaeger:
    protocols:
      grpc:
        tls_settings:
          cert_file: ./local/self-signed.pem
          key_file: ./local/self-signed-key.pem
        auth:
          oidc:
            issuer_url: https://dev-zkkhf874.eu.auth0.com/
            audience: http://auth.example.com

exporters:
  logging:

service:
  pipelines:
    traces:
      receivers:
        - jaeger
      processors: []
      exporters:
        - logging

And the agent from this PR with the following flags:

"--auth.bearer.token=eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCIsImtpZCI6ImxiWmJrd2JHUXlrWGdub09mRXNRNCJ9.eyJpc3MiOiJodHRwczovL2Rldi16a2toZjg3NC5ldS5hdXRoMC5jb20vIiwic3ViIjoiMUpEUmZJMm43c0JhNnhvMkQ2VEJSVnpjbGs5M25uVUZAY2xpZW50cyIsImF1ZCI6Imh0dHA6Ly9hdXRoLmV4YW1wbGUuY29tIiwiaWF0IjoxNjAyNzU0NDU0LCJleHAiOjE2MDI4NDA4NTQsImF6cCI6IjFKRFJmSTJuN3NCYTZ4bzJENlRCUlZ6Y2xrOTNublVGIiwiZ3R5IjoiY2xpZW50LWNyZWRlbnRpYWxzIn0.R128iWgn68uW_dPOifsYSRI2Yon4_CU-8E8gJeAvoIVQCka3u0S5RZ8C_I8zvLjdcuKePO9PjOp2vh9VcntXAZtc0II3D_K-fAXEsYE6aVM5taJFXnNwb7UAje0yfvMChXTVHfjcJkW8c-i3OrilJ9Ab-8ZSLDaR_a6cuT2c8BGj57tmDV2sWVJwe-dUCOzIPdwocQNK-pC9RsDTyq2uUMpqTRulyKlp51u2PtLd7tBjW1yZokoZcAlsVb4dJm4ZCutYZ4_tkeYSojZKw6JhhSDxcKvTAY4Y7nrMh2pGgjZbll1yzlsL1TyvD0PUWwM24wlDln5O0l4nU1Dzw6F8Wg",
"--reporter.grpc.host-port=localhost:14250",
"--reporter.grpc.tls.enabled=true",
"--reporter.grpc.tls.ca=/home/jpkroehling/Projects/src/github.com/open-telemetry/opentelemetry-collector/local/ca.pem",

Finally, send a trace with, for instance, Jaeger's tracegen:

$ go run ./cmd/tracegen/ -traces 1

Testing the Collector part of this PR

Start the Jaeger Collector with the following flags:

"--collector.auth.oidc.client-id=http://auth.example.com",
"--collector.auth.oidc.issuer-url=https://dev-zkkhf874.eu.auth0.com/",
"--collector.grpc.tls.cert=/home/jpkroehling/Projects/src/github.com/open-telemetry/opentelemetry-collector/local/self-signed.pem",
"--collector.grpc.tls.key=/home/jpkroehling/Projects/src/github.com/open-telemetry/opentelemetry-collector/local/self-signed-key.pem",
"--collector.grpc.tls.enabled=true",

And an OpenTelemetry Collector with this config:

receivers:
  otlp/noauth:
    protocols:
      grpc:
        endpoint: localhost:56680

exporters:
  jaeger/auth:
    endpoint: localhost:14250
    ca_file: ./local/ca.pem
    per_rpc_auth:
      type: bearer
      bearer_token: eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCIsImtpZCI6ImxiWmJrd2JHUXlrWGdub09mRXNRNCJ9.eyJpc3MiOiJodHRwczovL2Rldi16a2toZjg3NC5ldS5hdXRoMC5jb20vIiwic3ViIjoiMUpEUmZJMm43c0JhNnhvMkQ2VEJSVnpjbGs5M25uVUZAY2xpZW50cyIsImF1ZCI6Imh0dHA6Ly9hdXRoLmV4YW1wbGUuY29tIiwiaWF0IjoxNjAyNzU0NDU0LCJleHAiOjE2MDI4NDA4NTQsImF6cCI6IjFKRFJmSTJuN3NCYTZ4bzJENlRCUlZ6Y2xrOTNublVGIiwiZ3R5IjoiY2xpZW50LWNyZWRlbnRpYWxzIn0.R128iWgn68uW_dPOifsYSRI2Yon4_CU-8E8gJeAvoIVQCka3u0S5RZ8C_I8zvLjdcuKePO9PjOp2vh9VcntXAZtc0II3D_K-fAXEsYE6aVM5taJFXnNwb7UAje0yfvMChXTVHfjcJkW8c-i3OrilJ9Ab-8ZSLDaR_a6cuT2c8BGj57tmDV2sWVJwe-dUCOzIPdwocQNK-pC9RsDTyq2uUMpqTRulyKlp51u2PtLd7tBjW1yZokoZcAlsVb4dJm4ZCutYZ4_tkeYSojZKw6JhhSDxcKvTAY4Y7nrMh2pGgjZbll1yzlsL1TyvD0PUWwM24wlDln5O0l4nU1Dzw6F8Wg

service:
  pipelines:
    traces:
      receivers:
        - otlp/noauth
      processors: []
      exporters:
        - jaeger/auth

And finally, send a trace with, for instance, OpenTelemetry's tracegen:

$ go run . -otlp-endpoint localhost:56680 -otlp-insecure -traces 1

Negative tests

Just change the bearer token to something else in either of the cases.

Signed-off-by: Juraci Paixão Kröhling juraci@kroehling.de

jpkrohling · 2020-10-15T19:53:51Z

@yurishkuro are the lint problems known? I don't think they are related to this PR.

yurishkuro · 2020-10-16T00:43:08Z

you're upgrading grpc dependencies (not recommended as part of functional PRs), so the lint errors are related to this PR.

jpkrohling · 2020-10-19T09:42:16Z

I changed the code to not require adding the OpenTelemetry Collector as a dependency to the main module.

Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>

codecov · 2020-10-23T08:19:27Z

Codecov Report

Merging #2570 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2570      +/-   ##
==========================================
+ Coverage   95.31%   95.33%   +0.01%     
==========================================
  Files         208      208              
  Lines        9281     9294      +13     
==========================================
+ Hits         8846     8860      +14     
+ Misses        356      355       -1     
  Partials       79       79

Impacted Files	Coverage Δ
cmd/agent/app/reporter/grpc/builder.go	`95.00% <ø> (ø)`
cmd/agent/app/reporter/grpc/flags.go	`100.00% <100.00%> (ø)`
cmd/collector/app/builder_flags.go	`100.00% <100.00%> (ø)`
plugin/storage/integration/integration.go	`77.90% <0.00%> (+0.55%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9f954fe...8542811. Read the comment docs.

cmd/agent/app/reporter/grpc/flags.go

albertteoh · 2020-10-23T08:35:38Z

cmd/collector/app/builder_flags.go

+	flags.String(authOIDCIssuerURL, "", "the OpenID Connect server to validate the incoming auth tokens")
+	flags.String(authOIDCIssuerCAPath, "", "the path to the issuer's CA")
+	flags.String(authOIDCClientID, "", "this Jaeger's client ID")
+	flags.String(authOIDCUsernameClaim, "", "the username claim in the token, in case the 'sub' field shouldn't be used")


s/shouldn't be/isn't being/ ?

Not sure -- the sub field is always used, unless this flag is set. What's the right way to convey that?

Ah, I see -- maybe something like "The username claim in the token which, if set, overrides the 'sub' field"? (although, I'm not sure what the 'sub' field refers to :P)

sub is a standard field in JWT (JSON Web Tokens), indicating the "subject" (username).

How about this?

the claim in the token that should be used for the username instead of the default 'sub' field.

cmd/opentelemetry/app/receiver/jaegerreceiver/jaeger_receiver.go

albertteoh · 2020-10-23T10:43:03Z

LGTM, just had a few nit comments. I'll defer approval to maintainers.

Thanks for the nice testing instructions! I tried "Testing the Agent part from this PR" and got this error in Jaeger agent:

{"level":"error","ts":1603448778.8667269,"caller":"grpc/reporter.go:74","msg":"Could not send spans over gRPC","error":"rpc error: code = Unknown desc = authentication didn't succeed"

The "Testing the Collector part of this PR" instructions seemed to work though, saw this in OTEL agent (with logging exporter added):

2020-10-23T21:39:04.636+1100	INFO	loggingexporter/logging_exporter.go:296	TraceExporter	{"#spans": 2}

It's quite likely I stuffed something up in the first test, just thought to inform you anyway.

jpkrohling · 2020-10-23T11:23:13Z

{"level":"error","ts":1603448778.8667269,"caller":"grpc/reporter.go:74","msg":"Could not send spans over gRPC","error":"rpc error: code = Unknown desc = authentication didn't succeed"

Did you use the token from the instructions, or did you setup your own Auth0 application? If you are using the token from the example, it's likely expired by now.

Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>

joe-elliott · 2020-10-23T12:35:49Z

cmd/opentelemetry/app/receiver/jaegerreceiver/jaeger_receiver.go

+	if cfg.GRPC != nil && len(cOpts.AuthOIDCIssuerURL) > 0 {
+		// TODO: how can we log a warning if the user fails to specify a conditionally required flag?
+		// if authOIDCIssuerURL is set, authOIDCClientID has to be set as well (and vice-versa).
+		cfg.GRPC.Auth = &configauth.Authentication{


Unsure if it's possible atm, but I feel like an unsigned token mode would be valuable. If you didn't want to muck with oidc but just wanted to generate some simple tokens for multitenancy.

Let's record this as an issue. I don't think people would actually do that in production environments, but I could be wrong.

joe-elliott · 2020-10-23T12:37:54Z

cmd/collector/app/builder_flags.go

@@ -96,6 +110,14 @@ func AddFlags(flags *flag.FlagSet) {
 func AddOTELJaegerFlags(flags *flag.FlagSet) {
 	flags.String(CollectorHTTPHostPort, ports.PortToHostPort(ports.CollectorHTTP), "The host:port (e.g. 127.0.0.1:14268 or :14268) of the collector's HTTP server")
 	flags.String(CollectorGRPCHostPort, ports.PortToHostPort(ports.CollectorGRPC), "The host:port (e.g. 127.0.0.1:14250 or :14250) of the collector's GRPC server")
+
+	// auth-related flags
+	flags.String(authOIDCIssuerURL, "", "the OpenID Connect server to validate the incoming auth tokens")


won't these flags start showing up on the Jaeger collector but be unsupported?

They shouldn't, as they are part of AddOTELJaegerFlags.

AddOtelJaegerFlags is called from the normal AddFlags here:

https://github.com/jaegertracing/jaeger/blob/master/cmd/collector/app/builder_flags.go#L91

You are absolutely right:

$ go run ./cmd/collector/ --help | grep oidc 2020/10/23 22:13:16 maxprocs: Leaving GOMAXPROCS=12: CPU quota undefined --collector.auth.oidc.client-id string this Jaeger's client ID --collector.auth.oidc.groups-claim string the claim containing the group membership, where each group is considered a tenant. When absent, multi-tenancy is disabled. --collector.auth.oidc.issuer-ca-path string the path to the issuer's CA --collector.auth.oidc.issuer-url string the OpenID Connect server to validate the incoming auth tokens --collector.auth.oidc.username-claim string the claim in the token that should be used for the username instead of the default 'sub' field

I should probably suppress the flags from the current collector.

albertteoh · 2020-10-23T15:57:13Z

{"level":"error","ts":1603448778.8667269,"caller":"grpc/reporter.go:74","msg":"Could not send spans over gRPC","error":"rpc error: code = Unknown desc = authentication didn't succeed"

Did you use the token from the instructions, or did you setup your own Auth0 application? If you are using the token from the example, it's likely expired by now.

Yup, I was using the token from the instructions, and probably the cause of the error.

jpkrohling · 2020-10-27T19:53:42Z

I'm changing this to a draft PR, as we probably want to figure out our OpenTelemetry strategy before merging this here.

jpkrohling · 2020-11-05T08:38:39Z

I'm closing this, as we'll likely not need this if we move the direction we agreed.

jpkrohling requested a review from a team as a code owner October 15, 2020 15:40

jpkrohling requested a review from joe-elliott October 15, 2020 15:40

mergify bot assigned jpkrohling Oct 15, 2020

jpkrohling mentioned this pull request Oct 19, 2020

Agent (otel) can't be stopped when traces are in flight #2576

Closed

jpkrohling added 2 commits October 22, 2020 14:11

nosec to authBearerToken

Verified

This commit was created on github.com and signed with GitHub’s verified signature.

GPG key ID: B5690EEEBB952194

Verified
Learn about vigilant mode

63d61be

Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>

albertteoh reviewed Oct 23, 2020

View reviewed changes

Fixes based on the review

8542811

Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>

joe-elliott reviewed Oct 23, 2020

View reviewed changes

jpkrohling marked this pull request as draft October 27, 2020 19:53

jpkrohling closed this Nov 5, 2020

jpkrohling deleted the jpkrohling/multitenancy-add-token-auth-support branch July 28, 2021 19:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add auth support for collector #2570

Add auth support for collector #2570

jpkrohling commented Oct 15, 2020

jpkrohling commented Oct 15, 2020

yurishkuro commented Oct 16, 2020

jpkrohling commented Oct 19, 2020

codecov bot commented Oct 23, 2020 •

edited

Loading

albertteoh Oct 23, 2020 •

edited

Loading

jpkrohling Oct 23, 2020

albertteoh Oct 23, 2020

jpkrohling Oct 23, 2020

jpkrohling Oct 23, 2020 •

edited

Loading

albertteoh commented Oct 23, 2020

jpkrohling commented Oct 23, 2020

joe-elliott Oct 23, 2020

jpkrohling Oct 23, 2020

joe-elliott Oct 23, 2020

jpkrohling Oct 23, 2020

joe-elliott Oct 23, 2020 •

edited

Loading

jpkrohling Oct 23, 2020

albertteoh commented Oct 23, 2020

jpkrohling commented Oct 27, 2020

jpkrohling commented Nov 5, 2020

Add auth support for collector #2570

Add auth support for collector #2570

Conversation

jpkrohling commented Oct 15, 2020

Which problem is this PR solving?

Short description of the changes

Testing the Agent part from this PR

Testing the Collector part of this PR

Negative tests

jpkrohling commented Oct 15, 2020

yurishkuro commented Oct 16, 2020

jpkrohling commented Oct 19, 2020

codecov bot commented Oct 23, 2020 • edited Loading

Codecov Report

albertteoh Oct 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpkrohling Oct 23, 2020 • edited Loading

Choose a reason for hiding this comment

albertteoh commented Oct 23, 2020

jpkrohling commented Oct 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joe-elliott Oct 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

albertteoh commented Oct 23, 2020

jpkrohling commented Oct 27, 2020

jpkrohling commented Nov 5, 2020

codecov bot commented Oct 23, 2020 •

edited

Loading

albertteoh Oct 23, 2020 •

edited

Loading

jpkrohling Oct 23, 2020 •

edited

Loading

joe-elliott Oct 23, 2020 •

edited

Loading