Add auth data to pdata resource #3745

jpkrohling · 2021-07-30T13:20:26Z

This PR resolves #2733 by building on top of a previous PoC by @tigrannajaryan, but adapted a bit to allow the inject/extract of auth data to be performed by the configauth package. Other packages, such as the client ones, could use the same approach to inject/extract data.

I tested this with a configuration like this:

extensions:
  oidc:
    # see the blog post on securing the otelcol for information
    # on how to setup an OIDC server and how to generate the TLS certs
    # required for this example
    # https://medium.com/opentelemetry/securing-your-opentelemetry-collector-1a4f9fa5bd6f
    issuer_url: http://localhost:8080/auth/realms/opentelemetry
    audience: collector
    groups_claim: tenant

  bearertokenauth:
    token: "..."

receivers:
  # this is a receiver for the local agent, no auth required
  otlp/noauth:
    protocols:
      grpc:
        endpoint: localhost:4317

  # this is the remote otelcol, auth required
  otlp/auth:
    protocols:
      grpc:
        endpoint: localhost:4318
        tls_settings:
          cert_file: /home/jpkroehling/Projects/src/github.com/open-telemetry/opentelemetry-collector/local/certs/cert.pem
          key_file: /home/jpkroehling/Projects/src/github.com/open-telemetry/opentelemetry-collector/local/certs/cert-key.pem
        auth:
          authenticator: oidc

processors:

exporters:
  # the exporter for the agent pipeline
  # refer to the blog post mentioned above for more info on the token and TLS config
  otlp/auth:
    endpoint: localhost:4318
    ca_file: /home/jpkroehling/Projects/src/github.com/open-telemetry/opentelemetry-collector/local/certs/ca.pem
    retry_on_failure:
      enabled: false
    auth:
      authenticator: bearertokenauth

  logging:

  jaeger:
    endpoint: localhost:14250
    insecure: true

service:
  extensions:
    - oidc
    - bearertokenauth
  pipelines:
    # this represents an otelcol instance running as an agent, perhaps in a sidecar
    # the agent has a bearer token, to authenticate with the remote collector
    traces/agent:
      receivers:
        - otlp/noauth
      processors: []
      exporters:
        - otlp/auth

    # this represents a remote otelcol, requiring auth
    traces/collector:
      receivers:
        - otlp/auth
      processors: []
      exporters:
        - logging
        - jaeger # if data is visible in Jaeger, auth worked

When sending a trace to the agent pipeline, it eventually reaches the collector pipeline, which prints the following to the console:

2021-07-30T15:10:28.703+0200	INFO	loggingexporter/logging_exporter.go:54	TracesExporter	{"#spans": 2, "sub": "c67217c1-1278-4da7-b272-916624d9aa13", "groups": ["acme"]}

This confirms that the data is propagated down the pipeline to the exporter.

There's still a couple of points to work on:

The original proposal by @tigrannajaryan changed files like model/internal/data/protogen/trace/v1/trace.pb.go, which should not be changed. What's the appropriate way to handle this?
Tests and documentation. Once the point above is clarified, I'll work on this.

tigrannajaryan · 2021-08-03T10:02:51Z

config/configauth/pdatacontext.go

+type AuthContext struct {
+	sub   string
+	raw   string
+	group []string
+}


I still don't like that this imposes a rigid structure on what is an authentication information.
In the PoC I gave a an example of a passthrough authenticator that has neither sub, nor raw nor group, but is a header value. Sure, we could shoehorn it into raw, but then virtually anything can be argued to be represented in some raw string form.
IMO, AuthContext should be an interface here and each concrete auth extension will have a specific way to implement it. OIDC auth will have (sub,raw,group) tuple, other auth extensions will have something else instead.

We need a contract with basic fields so that processors can use basic auth primitives, like "username" or "group membership". Otherwise, the contract between components will be on keys within the map, which is way weaker. Usernames (or principal, subject) and group membership (or roles) are the basic constructs in auth/z systems.

Otherwise, the contract between components will be on keys within the map, which is way weaker.

@jpkrohling actually a map may be a good approach. The interaction between the processors and PDataContext is unlikely to be hard-coded to benefit from strong typing. It is most likely going to be config-driven anyway, with the user specifying in the configuration the name of the PDataContext's attribute to use (e.g. in the example of routing processor you posted, the config refers to "membership" attribute directly).

I think we can define AuthContext interface like this:

type AuthContext interface { Equal(other interface{}) bool GetAttribute(attrName string) interface{} }

oidcauthextension.AuthContext then can define GetAttribute such that it return the right values for the 3 supported special attribute names ("principal", "membership", whatever is the third one).

This will allow processors to query for AuthContext attributes.

What do you think?

Sounds reasonable. Let me give it a try.

How does it look now? It does require more work from users to tie the components together, but I don't think it's too bad. We just need to make sure every auth component documents what it produces.

exporter/loggingexporter/logging_exporter.go

tigrannajaryan · 2021-08-03T10:08:20Z

The original proposal by @tigrannajaryan changed files like model/internal/data/protogen/trace/v1/trace.pb.go, which should not be changed. What's the appropriate way to handle this?

Unfortunately I don't see any other way except patching generated *.pb.go files after Protobuf compiler generates them. Ideally the compiler would have a capability like this (i.e. declare a field that is not serializable), but there is no such thing.

bogdandrutu · 2021-08-03T18:10:15Z

model/pdata/traces.go

+func (ms ResourceSpans) PDataContext() PDataContext {
+	return newPDataContext(&(*ms.orig).Context)
+}


This needs to be auto-generated.

bogdandrutu · 2021-08-03T18:10:30Z

model/internal/data/protogen/trace/v1/trace.pb.go

+	// This is expected to be ignored by marshaling/unmarshaling. Double-check that this is true.
+	Context go_opentelemetry_io_collector_model_internal_data.PDataContext
 }


This needs to be auto-generated

That's the part that we don't know how to do nicely. Should we use sed to patch this file as part of make genproto target, after Protobuf compiler does its part of the job? Do we have any other options?

@bogdandrutu, I'll need your help here.

ping @bogdandrutu

bogdandrutu · 2021-08-03T18:11:01Z

model/internal/data/pdatacontext.go

+package data
+
+type PDataContext struct {
+	List []PDataContextKeyValue


Can we use the already defined "AnyValue"/Attributes?

Not for the Key, it needs to be an arbitrary interface{} (and not just a string), like it is done for context.Context, but for the Value it is an interesting idea, since it may make some code that needs to deal with both telemetry attributes and PDataContext easier to write. It is worth exploring.

Did you mean pdata.AttributeValue? If so, that would cause an import cycle that is not allowed.

tigrannajaryan

Thanks @jpkrohling !
I think I like it, with some minor comments/questions.

config/configauth/pdatacontext.go

github-actions · 2021-08-19T03:15:57Z

This PR was marked stale due to lack of activity. It will be closed in 7 days.

github-actions · 2021-08-27T03:15:46Z

This PR was marked stale due to lack of activity. It will be closed in 7 days.

jpkrohling · 2021-08-27T07:37:01Z

This is not stale. Some things are in my queue and I need input from @bogdandrutu, which is likely busy with the moving of the components.

bputt-e · 2021-08-27T22:08:49Z

@jpkrohling how do you plan on making the auth data available to where we can add it to traces/logs/metrics?

We're planning on ingesting multiple tenants from one API endpoint and passing the data off to kafka and taking the auth reference and associating it back to the tenant so we can segregate their data.

Also not sure if relevant or not, but we took inspiration from your medium article and decided to use ory hydra instead, seems to work thus far :)

jpkrohling · 2021-09-01T07:58:08Z

how do you plan on making the auth data available to where we can add it to traces/logs/metrics?

The current idea is to create a new context within the pdata which will contain a map with the auth data. I haven't had much time to work in this, but this is high priority in my queue.

Also not sure if relevant or not, but we took inspiration from your medium article and decided to use ory hydra instead, seems to work thus far :)

Thanks for letting me know!

bputt-e · 2021-09-01T13:41:08Z

@jpkrohling also if it helps provide context, our plans are to use otel collector that ships to kafka that has the grafana-agent read from kafka and post the spans to loki, cortex(prometheus), and tempo. We're hoping we can abstract the clientId and use that as the x-scope-orgId to support multitenancy (for loki/cortext) and still share spans in tempo so that each team can see how their system interacts with others in the ecosystem.

github-actions · 2021-09-09T03:15:58Z

This PR was marked stale due to lack of activity. It will be closed in 7 days.

github-actions · 2021-09-17T03:16:05Z

This PR was marked stale due to lack of activity. It will be closed in 7 days.

github-actions · 2021-09-25T03:15:46Z

This PR was marked stale due to lack of activity. It will be closed in 7 days.

github-actions · 2021-10-06T03:15:54Z

This PR was marked stale due to lack of activity. It will be closed in 7 days.

github-actions · 2021-10-14T03:15:57Z

This PR was marked stale due to lack of activity. It will be closed in 7 days.

tigrannajaryan · 2021-10-14T13:30:59Z

@jpkrohling do you plan to continue working on this?

jpkrohling · 2021-10-14T14:46:08Z

Yes, I'm waiting on @bogdandrutu on this one. Whenever he's got some time, I would like to get some pair programming session with him to sort out the questions and problems behind this.

github-actions · 2021-10-22T03:16:02Z

This PR was marked stale due to lack of activity. It will be closed in 7 days.

CoderPoet · 2021-10-24T11:10:07Z

Is there any progress on this PR?

jpkrohling · 2021-10-25T08:31:22Z

@CoderPoet, I'll be doing a pair programming session with @bogdandrutu this Wed to unblock this.

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

codecov · 2021-10-25T13:11:32Z

Codecov Report

Merging #3745 (126164a) into main (b7e2e33) will decrease coverage by 0.26%.
The diff coverage is 29.78%.

@@            Coverage Diff             @@
##             main    #3745      +/-   ##
==========================================
- Coverage   88.07%   87.80%   -0.27%     
==========================================
  Files         173      176       +3     
  Lines       10188    10235      +47     
==========================================
+ Hits         8973     8987      +14     
- Misses        975     1006      +31     
- Partials      240      242       +2

Impacted Files	Coverage Δ
model/pdata/pdatacontext.go	`0.00% <0.00%> (ø)`
model/pdata/traces.go	`88.88% <0.00%> (-5.23%)`	⬇️
internal/otlptext/databuffer.go	`98.29% <40.00%> (-1.71%)`	⬇️
config/configauth/pdatacontext.go	`41.66% <41.66%> (ø)`
service/internal/authcontext/authcontext.go	`41.66% <41.66%> (ø)`
internal/otlptext/traces.go	`100.00% <100.00%> (ø)`
service/internal/builder/receivers_builder.go	`75.12% <100.00%> (+0.12%)`	⬆️

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 b7e2e33...126164a. Read the comment docs.

jpkrohling · 2021-10-25T13:12:28Z

PR updated, and tested with a branch with the required changes to the OIDC authenticator (as an example, as other server authenticators would need similar changes) with the following config file:

extensions:
  oidc:
    issuer_url: http://localhost:8080/auth/realms/opentelemetry
    audience: account
  oauth2client:
    client_id: agent
    client_secret: 0b5fab57-9b0e-449f-9da7-9ff7e1735df6
    token_url: http://localhost:8080/auth/realms/opentelemetry/protocol/openid-connect/token

receivers:
  otlp:
    protocols:
      grpc:
  otlp/withauth:
    protocols:
      endpoint: localhost:5317
      grpc:
        tls:
          cert_file: /tmp/certs/cert.pem
          key_file: /tmp/certs/cert-key.pem
        auth:
          authenticator: oidc

processors:

exporters:
  logging:
    logLevel: debug
  otlp:
    endpoint: localhost:5317
    ca_file: /tmp/certs/ca.pem
    auth:
      authenticator: oauth2client

service:
  extensions: [oidc, oauth2client]
  pipelines:
    traces/agent:
      receivers: [otlp]
      processors: []
      exporters: [otlp/withauth]
    traces/collector:
      receivers: [otlp/withauth]
      processors: []
      exporters: [logging]

The OAuth server referenced in the OIDC extension is similar to the one used in the blog post.

The logging exporter shows this:

2021-10-25T15:05:06.037+0200	DEBUG	loggingexporter/logging_exporter.go:51	ResourceSpans #0
Pipeline Data Context:
     -> &{eyJhbGciOiJSUzI1NiIsInR5cCIgOiAiSldUIiwia2lkIiA6ICJIb3pxTUYzSGlVX2VPTWEySXBUcHlOSk1ZVWFoWXd4WjNCcy1idGxJaGpnIn0.eyJleHAiOjE2MzUxNjc0MDMsImlhdCI6MTYzNTE2NzEwMywianRpIjoiZDc2MTVhZmMtNGM0Zi00MzMxLTljZjgtZTE0NDlmYjBjNzk1IiwiaXNzIjoiaHR0cDovL2xvY2FsaG9zdDo4MDgwL2F1dGgvcmVhbG1zL29wZW50ZWxlbWV0cnkiLCJhdWQiOlsiY29sbGVjdG9yIiwiYWNjb3VudCJdLCJzdWIiOiI4YmE2MTM3Ny03ZTBkLTQyYmMtYmQwMi0wN2VmNThkYWYxMzAiLCJ0eXAiOiJCZWFyZXIiLCJhenAiOiJhZ2VudCIsImFjciI6IjEiLCJyZWFsbV9hY2Nlc3MiOnsicm9sZXMiOlsib2ZmbGluZV9hY2Nlc3MiLCJ1bWFfYXV0aG9yaXphdGlvbiIsImRlZmF1bHQtcm9sZXMtb3BlbnRlbGVtZXRyeSJdfSwicmVzb3VyY2VfYWNjZXNzIjp7ImFjY291bnQiOnsicm9sZXMiOlsibWFuYWdlLWFjY291bnQiLCJtYW5hZ2UtYWNjb3VudC1saW5rcyIsInZpZXctcHJvZmlsZSJdfX0sInNjb3BlIjoicHJvZmlsZSBlbWFpbCIsImNsaWVudElkIjoiYWdlbnQiLCJjbGllbnRIb3N0IjoiMTAuMC4yLjEwMCIsImVtYWlsX3ZlcmlmaWVkIjpmYWxzZSwicHJlZmVycmVkX3VzZXJuYW1lIjoic2VydmljZS1hY2NvdW50LWFnZW50IiwiY2xpZW50QWRkcmVzcyI6IjEwLjAuMi4xMDAifQ.vOI6e7V2NWxWyjedf818CYUnFyhYWZ42bGPC2zHVPtVWAMxL7pvifKHhktkKF5STsLTFW8enLIbdiIPqNEY8av2anRyVhVdKduJYt3wPWFvLl8VA3tirZthltva5ITIkz0rLCaD09tp-j1wr0i9EyxdUmze3vt0P8tzqJeNxJpMKvOHCWgyGJsVJ3WZxsJkmbecXC1H_dMBxHOqtciu6WwW5jzyKRibfgw69xgCLhWINYg87xcLR-NFVFdAaJ4UW5Dv_7OFswbTlRGRgBTYZQB7DRglu88u9cLFJsxAGZ6m3YviLTKULSzqipBvW2VJGk_7UXaint51tfNxop9XERA 8ba61377-7e0d-42bc-bd02-07ef58daf130 []}

CoderPoet · 2021-10-26T14:44:24Z

service/internal/builder/receivers_builder.go

@@ -182,6 +183,7 @@ func attachReceiverToPipelines(
 	switch dataType {
 	case config.TracesDataType:
 		junction := buildFanoutTraceConsumer(builtPipelines)
+		junction = authcontext.NewTracesInjector(junction)
 		createdReceiver, err = factory.CreateTracesReceiver(ctx, set, cfg, junction)

 	case config.MetricsDataType:


Support only TraceDataType?

For a first iteration, yes. Once we validate the approach and get it out for testing, we might (will) expand it to other data types.

CoderPoet · 2021-10-27T07:35:57Z

Is it necessary to implement a tenantgroupprocessor to implement that each consumer request is tenant-isolated?

CoderPoet · 2021-10-27T07:38:48Z

Is it necessary to implement a tenantgroupprocessor to implement that each consumer request is tenant-isolated?

Or to implement a batch processor that supports tenant isolation

jpkrohling · 2021-10-27T08:04:44Z

Or to implement a batch processor that supports tenant isolation

That is indeed something we talked about in the past. @tigrannajaryan mentioned this problem explicitly, and I think the consensus was that the existing processors that perform grouping (like the batch processor) should at least provide an option that allows users to tell the attribute to use to group the spans.

CoderPoet · 2021-10-27T08:14:58Z

Or to implement a batch processor that supports tenant isolation

That is indeed something we talked about in the past. @tigrannajaryan mentioned this problem explicitly, and I think the consensus was that the existing processors that perform grouping (like the batch processor) should at least provide an option that allows users to tell the attribute to use to group the spans.

Is there a requirement for pipeline tenant isolation? In this way, the retry queue is also tenant isolation and does not block each other

jpkrohling · 2021-10-27T09:46:39Z

Is there a requirement for pipeline tenant isolation?

There's no such requirement as of now. I would really recommend taking a look at the routing processor for more complex multi-tenant requirements, as you can split each tenant into its own pipeline, or even to its own collector instance.

github-actions · 2021-11-04T03:15:59Z

This PR was marked stale due to lack of activity. It will be closed in 7 days.

jpkrohling · 2021-11-04T08:36:48Z

I'm closing this, as it's clear that we will follow the route of #4288/#4319 instead.

jpkrohling mentioned this pull request Jul 30, 2021

Establish the auth context for authenticators to add data to #3552

Closed

tigrannajaryan reviewed Aug 3, 2021

View reviewed changes

bogdandrutu reviewed Aug 3, 2021

View reviewed changes

tigrannajaryan mentioned this pull request Aug 4, 2021

[PROTOTYPE] Combine gRPC and HTTP servers #3765

Closed

jpkrohling force-pushed the jpkrohling/issue2733 branch from 74998d8 to 87cde06 Compare August 10, 2021 11:31

tigrannajaryan reviewed Aug 10, 2021

View reviewed changes

config/configauth/pdatacontext.go Outdated Show resolved Hide resolved

config/configauth/pdatacontext.go Outdated Show resolved Hide resolved

config/configauth/pdatacontext.go Outdated Show resolved Hide resolved

github-actions bot added the Stale label Aug 19, 2021

jpkrohling removed the Stale label Aug 19, 2021

github-actions bot added the Stale label Aug 27, 2021

jpkrohling removed the Stale label Aug 27, 2021

tigrannajaryan mentioned this pull request Sep 2, 2021

Auth-related tasks #3629

Closed

3 tasks

github-actions bot added the Stale label Sep 9, 2021

bogdandrutu removed the Stale label Sep 9, 2021

github-actions bot added the Stale label Sep 17, 2021

jpkrohling removed the Stale label Sep 17, 2021

github-actions bot added the Stale label Sep 25, 2021

bogdandrutu removed the Stale label Sep 28, 2021

github-actions bot added the Stale label Oct 6, 2021

jpkrohling removed the Stale label Oct 6, 2021

github-actions bot added the Stale label Oct 14, 2021

jpkrohling removed the Stale label Oct 14, 2021

github-actions bot added the Stale label Oct 22, 2021

jpkrohling removed the Stale label Oct 22, 2021

jpkrohling added 4 commits October 25, 2021 14:02

Propagate auth data down the pipeline

8e9c369

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

Add auth data to logging exporter

5d85bb1

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

Changed pdata context to be a map

f465342

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

Remove authC

126164a

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

jpkrohling force-pushed the jpkrohling/issue2733 branch from 87cde06 to 126164a Compare October 25, 2021 13:07

CoderPoet reviewed Oct 26, 2021

View reviewed changes

pmm-sumo mentioned this pull request Oct 27, 2021

Extract client IP address and store it into context for OTLP/HTTP #4257

Merged

jpkrohling mentioned this pull request Oct 28, 2021

Auth data as part of the client information #4288

Closed

7 tasks

github-actions bot added the Stale label Nov 4, 2021

jpkrohling closed this Nov 4, 2021

jpkrohling mentioned this pull request Sep 1, 2022

[extension/headerssetter] Add more options for setting headers from the context open-telemetry/opentelemetry-collector-contrib#13761

Closed

Add auth data to pdata resource #3745

Add auth data to pdata resource #3745

Conversation

jpkrohling commented Jul 30, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tigrannajaryan Aug 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tigrannajaryan commented Aug 3, 2021

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 Aug 19, 2021 • edited Loading

Choose a reason for hiding this comment

bogdandrutu Aug 3, 2021 • edited Loading

Choose a reason for hiding this comment

tigrannajaryan Aug 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tigrannajaryan left a comment

Choose a reason for hiding this comment

github-actions bot commented Aug 19, 2021

github-actions bot commented Aug 27, 2021

jpkrohling commented Aug 27, 2021

bputt-e commented Aug 27, 2021

jpkrohling commented Sep 1, 2021

bputt-e commented Sep 1, 2021

github-actions bot commented Sep 9, 2021

github-actions bot commented Sep 17, 2021

github-actions bot commented Sep 25, 2021

github-actions bot commented Oct 6, 2021

github-actions bot commented Oct 14, 2021

tigrannajaryan commented Oct 14, 2021

jpkrohling commented Oct 14, 2021

github-actions bot commented Oct 22, 2021

CoderPoet commented Oct 24, 2021

jpkrohling commented Oct 25, 2021

codecov bot commented Oct 25, 2021

Codecov Report

jpkrohling commented Oct 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CoderPoet commented Oct 27, 2021

CoderPoet commented Oct 27, 2021

jpkrohling commented Oct 27, 2021

CoderPoet commented Oct 27, 2021

jpkrohling commented Oct 27, 2021 • edited Loading

github-actions bot commented Nov 4, 2021

jpkrohling commented Nov 4, 2021

tigrannajaryan Aug 4, 2021 •

edited

Loading

jpkrohling Aug 19, 2021 •

edited

Loading

bogdandrutu Aug 3, 2021 •

edited

Loading

tigrannajaryan Aug 4, 2021 •

edited

Loading

jpkrohling commented Oct 25, 2021 •

edited

Loading

jpkrohling commented Oct 27, 2021 •

edited

Loading