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

jpkrohling · 2021-07-02T10:36:06Z

This commit modifies the Authenticate method from configauth.ServerAuthenticator to return a context. At this point, there's no further change in the behavior, as a concrete solution is going to be added as part of #2733/#2734.

This should be the only breaking change required for the auth propagation to work, making the collector ready for GA from the auth perspective.

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

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

jpkrohling · 2021-07-02T10:36:28Z

cc @alolita

jpkrohling · 2021-07-02T10:38:23Z

I tried finding @tigrannajaryan's PoC but couldn't. I would like to review it before getting this merged, to make sure we don't need other changes before GA.

tigrannajaryan · 2021-07-05T19:18:52Z

I tried finding @tigrannajaryan's PoC but couldn't. I would like to review it before getting this merged, to make sure we don't need other changes before GA.

Here is the link to the PoC for reference:

here is a VERY incomplete and draft changeset that shows what I was thinking about passthrough auth context: https://github.com/open-telemetry/opentelemetry-collector/compare/main...tigrannajaryan:feature/tigran/auth-passthrough?expand=1
I think it also uncovered that our ServerAuthenticactor.Authenticate() method needs to return a context. We do not have an HTTP Server authentication implementation, so I was not sure how exactly to do it.

jpkrohling · 2021-07-06T08:19:43Z

I reviewed the branch and the only change that would be breaking is the change to the Authenticate function to return a context, which is being covered by this PR here.

I think we are good to go then!

tigrannajaryan · 2021-07-07T15:09:06Z

config/configauth/serverauth.go

@@ -40,7 +40,7 @@ type ServerAuthenticator interface {
 	// When the authentication fails, an error must be returned and the caller must not retry. This function is typically called from interceptors,
 	// on behalf of receivers, but receivers can still call this directly if the usage of interceptors isn't suitable.
 	// The deadline and cancellation given to this function must be respected, but note that authentication data has to be part of the map, not context.
-	Authenticate(ctx context.Context, headers map[string][]string) error
+	Authenticate(ctx context.Context, headers map[string][]string) (context.Context, error)


Can we add an explanation in the comments about what the returned context is expected to be?

I left that part out so that we can define it when we have a first implementation making use of it, but I added the best information we have so far.

Should I define the context keys as part of this PR? It would be basically this: https://github.com/open-telemetry/opentelemetry-collector/pull/3239/files#diff-1c8371e19129e2092ab7abfce04e6558218579f1be29d90adb958d230475584f

In any case, I updated the PR to include a few words about the resulting context.

Sorry about the noise, but I thought it would be better to include the auth keys as part of the API for v1, so, I included it in this PR.

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

jpkrohling · 2021-07-08T19:51:00Z

@tigrannajaryan would you mind taking a look at the latest commits?

jpkrohling · 2021-07-09T19:21:32Z

@bogdandrutu, do you have some time to re-review this one?

bogdandrutu

I would strongly encourage to split this in separate PR (the 3rd commit), so I can more confident merge this PR and have more time thinking about the new API especially because it is a public API

bogdandrutu · 2021-07-09T23:38:55Z

auth/context.go

+type rawType struct{}
+type subjectType struct{}
+type groupsType struct{}


Is it possible to have all three of them in the same time? Can we actually have one key with an enum type?

Maybe have a "AuthContext" struct which embeds different auth types?

Is there a library that provides this?

Is it possible to have all three of them in the same time? Can we actually have one key with an enum type?

Yes, and quite common actually.

Maybe have a "AuthContext" struct which embeds different auth types?

Those are not auth types. The raw auth data might be a JWT, which contains the principal and group memberships. The authenticator (OIDC, for instance) will then place all the three components in the context. Another authenticator might get the subject and groups from the auth server.

I can split this if we need more time, but I think the third commit brings only a minimal API, generic enough to be extended with custom types in the future. The idea about AuthContext has been brought by @tigrannajaryan as well, though:

6b6e713#diff-208f8af800c6aa88673523930b718a5d1a5b3f2a2d2f6edbdcf97171ce3d15ca

Is there a library that provides this?

I'm not aware of any. This is really only about adding auth to a context.Context, but I'm open to using a library if it brings value.

tigrannajaryan · 2021-07-13T18:41:30Z

auth/context.go

+var (
+	rawKey     = rawType{}
+	subjectKey = subjectType{}
+	groupsKey  = groupsType{}
+)
+
+type rawType struct{}
+type subjectType struct{}


Why do we need this in a generic auth/context.go at all? These appear to be specific context data for example by oidc auth extension. Why not declare there on oidc implementation?
I think we should aim for every auth extension to declare its own context data.

These are not specific to the OIDC auth extension. Those three pieces should be common to most authenticators, as they are common parts of any authorization system. For example, we might have a basic auth authenticator, a la Apache httpd: in such case, we would have a .htpasswd that would serve as the user data source (subject), and an optional .htgroups, linking groups with users like Unix' /etc/group. The raw field would be the base64 encoded username:password received by the HTTP server from the client.

I think we should aim for every auth extension to declare its own context data.

I think every extension (not only auth) should be allowed to enhance the context data as they wish so that other components can make use of that information, but we should have a minimal set of options that are common to all authenticators, and this auth/context represents that in my opinion.

Sorry, I still do not understand why this needs to be in the auth package and not in the specific extension's package which needs this. We are not expecting that we can mix auth extensions in any way, right? So why not move these declarations to the extension which plans to inject these values in the context?
I may be missing something but I think different authenticator extensions will need different values. Some authenticators will only need raw auth token others may need a subject or whatever it is called.
I don't understand what do we gain by making these concepts part of the public API of core and not just delegate it to individual auth extensions.

We are not expecting that we can mix auth extensions in any way, right?

We certainly expect auth extensions to provide data that will be consumed by other components in the pipeline. For instance, the routing processor can make decisions based on the principal or group membership (tenant "acme" should go to this Jaeger, tenant "ecorp" should go to that Zipkin, ...).

All auth extensions are expected to have at least the notion of principal (like a username, or a service account name, or ...) Raw can certainly be removed from the generic package, though. I would also keep the group information, as it's also a common aspect of auth no matter the backend used.

I don't understand what do we gain by making these concepts part of the public API of core and not just delegate it to individual auth extensions.

Other components, like the routing processor I mentioned earlier, might provide tighter integration with the auth features, without requiring specific authenticators to exist.

@jpkrohling can you please show how a routing processor could use the auth data? A sample code somewhere would be great to see and would help review this PR.

It cannot right now because of this missing piece, but this is how a configuration would look like for the existing routingprocessor:

receivers: nop: processors: routing: default_exporters: - otlp from_auth: membership table: - value: acme exporters: - jaeger/acme - otlp/acme - value: globex exporters: - otlp/globex exporters: otlp: otlp/acme: otlp/globex: jaeger/acme: endpoint: localhost:14250 service: pipelines: traces: receivers: - nop processors: - routing exporters: - jaeger/acme - otlp/acme - otlp/globex

In this example, the authentication for a hypothetical user "service-a", belonging to the tenant "acme" would cause the authenticator to place "service-a" as the subject/principal and "acme" in the group membership.

The routingprocessor would probably need to group the data by auth/membership, but minimal changes would be required to the current code, probably isolated to this function here: https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/36a7da6a4c29ddf81d118ec159e0a7a1682c6f21/processor/routingprocessor/routing.go#L133-L134 . Instead of extracting the route destination from the context, it would extract from the pdata.

Looking at my notes, I think we did discuss something else though:

the authenticators would place auth data into the context

something at the junction point between receivers and processors would place this into pdata

processors/exporters should rely on the data from pdata instead of the context

Other than that, the general idea remains.

jpkrohling · 2021-07-14T17:07:13Z

Based on today's SIG Collector call, I'm renaming this PR and splitting the first two commits on its own PR (#3618). Things we discussed about this one:

Perhaps we want an interface that the authenticators implement, so that downstream processors/exporters can ask the authenticators about things like subjects/groups?
Or have a generic way to add entries to the context? Like the client package would need.
Or just settle on the names we want in the third commit here.

One thing I did forget during the call is that this context is relevant only for the junction points: they would take data out of the context and place in the new pdata.Context object, which will be part of the pdata.Resource* structs. So, this auth here would need to be part of an internal API only.

bogdandrutu · 2021-07-14T18:17:00Z

Please rebase :)

github-actions · 2021-07-29T03:16:11Z

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

tigrannajaryan · 2021-07-30T10:54:33Z

@jpkrohling now that #3618 is merged do you plan to rebase this?

jpkrohling · 2021-07-30T11:29:56Z

Yes, and your timing is perfect. I just cleared some things up from my queue and I'm working on this right now.

jpkrohling · 2021-07-30T13:21:39Z

Closing this in favor of #3745.

Prepare auth API to return a context

0bf3d63

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

jpkrohling requested review from a team and tigrannajaryan July 2, 2021 10:36

jpkrohling mentioned this pull request Jul 2, 2021

Cleanup/refactor auth capabilities #2603

Closed

5 tasks

bogdandrutu approved these changes Jul 2, 2021

View reviewed changes

tigrannajaryan approved these changes Jul 7, 2021

View reviewed changes

jpkrohling added 2 commits July 8, 2021 10:31

Added a few words about the resulting context

eccd79e

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

Set auth data in the context

ccede3d

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

jpkrohling requested a review from bogdandrutu July 9, 2021 19:21

bogdandrutu reviewed Jul 9, 2021

View reviewed changes

tigrannajaryan self-requested a review July 13, 2021 18:39

tigrannajaryan requested changes Jul 13, 2021

View reviewed changes

dashpole mentioned this pull request Jul 14, 2021

Add e2e pipeline processing duration self-observability metrics #3183

Closed

jpkrohling mentioned this pull request Jul 14, 2021

Prepare auth API to return a context #3618

Merged

jpkrohling changed the title ~~Prepare auth API to return a context~~ Establish the auth context for authenticators to add data to Jul 14, 2021

jpkrohling mentioned this pull request Jul 15, 2021

Auth-related tasks #3629

Closed

3 tasks

jpkrohling added the release:required-for-ga Must be resolved before GA release label Jul 15, 2021

github-actions bot added the Stale label Jul 29, 2021

jpkrohling removed the Stale label Jul 29, 2021

jpkrohling closed this Jul 30, 2021

tigrannajaryan mentioned this pull request Aug 4, 2021

Add auth data to pdata resource #3745

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jpkrohling commented Jul 2, 2021

jpkrohling commented Jul 2, 2021

jpkrohling commented Jul 2, 2021

tigrannajaryan commented Jul 5, 2021

jpkrohling commented Jul 6, 2021

tigrannajaryan Jul 7, 2021

jpkrohling Jul 8, 2021

jpkrohling Jul 8, 2021

jpkrohling Jul 8, 2021 •

edited

Loading

jpkrohling commented Jul 8, 2021

jpkrohling commented Jul 9, 2021

bogdandrutu left a comment

bogdandrutu Jul 9, 2021

bogdandrutu Jul 9, 2021

bogdandrutu Jul 9, 2021

jpkrohling Jul 12, 2021 •

edited

Loading

tigrannajaryan Jul 13, 2021

jpkrohling Jul 14, 2021

tigrannajaryan Jul 21, 2021

jpkrohling Jul 21, 2021

tigrannajaryan Jul 30, 2021

jpkrohling Jul 30, 2021 •

edited

Loading

jpkrohling Jul 30, 2021

jpkrohling commented Jul 14, 2021

bogdandrutu commented Jul 14, 2021

github-actions bot commented Jul 29, 2021

tigrannajaryan commented Jul 30, 2021

jpkrohling commented Jul 30, 2021

jpkrohling commented Jul 30, 2021

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

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

Conversation

jpkrohling commented Jul 2, 2021

jpkrohling commented Jul 2, 2021

jpkrohling commented Jul 2, 2021

tigrannajaryan commented Jul 5, 2021

jpkrohling commented Jul 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpkrohling Jul 8, 2021 • edited Loading

Choose a reason for hiding this comment

jpkrohling commented Jul 8, 2021

jpkrohling commented Jul 9, 2021

bogdandrutu left a comment

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 Jul 12, 2021 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpkrohling Jul 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpkrohling commented Jul 14, 2021

bogdandrutu commented Jul 14, 2021

github-actions bot commented Jul 29, 2021

tigrannajaryan commented Jul 30, 2021

jpkrohling commented Jul 30, 2021

jpkrohling commented Jul 30, 2021

jpkrohling Jul 8, 2021 •

edited

Loading

jpkrohling Jul 12, 2021 •

edited

Loading

jpkrohling Jul 30, 2021 •

edited

Loading