Provision to include request context(query params, headers) in AuthenticatorFunc

[neelayu]

Is your feature request related to a problem? Please describe.
A facility to access request params such as query string in addition to already existing headers would be useful in the AuthenticatorFunc

Describe alternatives you've considered
Pass the queryParams along with headers in the map passed to the above func. Probably not the best approach.

Additional context
Authenticators such as apiToken validators would need to pick the token from either the header or query string. In such case, the current implementation wont be sufficient.
We need a more generic way of handling this.

@jpkrohling

[jpkrohling]

How about we have a list of properties that the authenticator should propagate down via the context? The config would look like this:

receivers:
  myhttpreceiver:
    auth:
      propagate:
        headers:
        - x-tenant-id
        query:
        - access_token

Today, we'd just blindly propagate everything, which is user-friendly but potentially resource-intensive. This change would be resource efficient, but will not be as user-friendly as before. Also, if the payload changes and a new header is sent, the collector's config needs to be changed to make use of that.

Or perhaps we should propagate everything, except when this new propagate node is provided? It would provide the best of two worlds, in my opinion.

[neelayu]

While writing an authenticator for our distribution, I actually thought of using something similar to above in the extensions.myauth section. However, I realized that the only thing which the func receives is the header map. So it didnt work out.

Or perhaps we should propagate everything, except when this new propagate node is provided? It would provide the best of two worlds, in my opinion.

This is indeed a good approach. We make propagate as an optional field in the config. This way any existing configuration wont break. Existing authenticators will continue to receive only the headers. If propagate field is present, we must also give the option of specifying all headers or query params.

There are a few things which we should make a note of-

• Do we keep the function signature same?

  type AuthenticatorFunc func(ctx context.Context, headers map[string][]string) (context.Context, error)
  

• If so, we need to figure out the possible sources where the auth data could be present. By convention, usually queryParams and headers should suffice. However, if in any uncommon case where the auth data is present in the request body, how should the func signature look like? If we change it, then there would be breaking changes.
• The above points are valid for HTTP receivers only. For gRPC receivers, any change we propose should have minimal impact.

[jpkrohling]

Note that the propagation happens in the confighttp/configgrpc code, not in the authenticator itself.

Do we keep the function signature same?

Yes, the only change is to the contents of the incoming map.

However, if in any uncommon case where the auth data is present in the request body, how should the func signature look like? If we change it, then there would be breaking changes.

Do you have an example of that?

For gRPC receivers, any change we propose should have minimal impact

gRPC would be affected, as the list would restrict what we get from its metadata.

[neelayu]

Yes I understood that this would be a part of confighttp/configgrpc

Regarding auth data in the body, only one concrete instance I have is wrt to statsd receiver. It is over UDP and accepts plaintext statsd format metrics data. I know that auth is not a part of that configuration. One possible way of sending auth(tokens) would be via tags and we would need that tag to be parsed which is present as request body itself. I
Note: This may not be required as UDP and HTTP(TCP) are completely different protocols, but it is a use case that we want to explore at our organization.

gRPC would be affected, as the list would restrict what we get from its metadata.

But for gRPC auth, we wont have propagate field right?

[jpkrohling]

we would need that tag to be parsed which is present as request body itself

Good point. I can see how this would become a problem down the chain, so, let's consider this in the future if we ever need it. I believe that the proposal from this issue here could be easily changed to add body propagation. We'd need a way to drop that after we used it, but that's also something that can be done in the future.

But for gRPC auth, we wont have propagate field right?

Both HTTP and gRPC share the same auth object:

opentelemetry-collector/config/confighttp/confighttp.go

Lines 61 to 62 in 7cdf8c7

	// Auth configuration for outgoing HTTP calls.
	Auth *configauth.Authentication `mapstructure:"auth,omitempty"`

opentelemetry-collector/config/configgrpc/configgrpc.go

Lines 98 to 99 in 7cdf8c7

	// Auth configuration for outgoing RPCs.
	Auth *configauth.Authentication `mapstructure:"auth,omitempty"`

opentelemetry-collector/config/configauth/configauth.go

Lines 31 to 35 in 7cdf8c7

	// Authentication defines the auth settings for the receiver.
	type Authentication struct {
	// AuthenticatorID specifies the name of the extension to use in order to authenticate the incoming data point.
	AuthenticatorID config.ComponentID `mapstructure:"authenticator"`
	}

My idea is to add the Propagate option to this last struct.

[neelayu]

Agreed. My contention was that for grpc, propagate will not be used since it has only headers. However, upon thinking I realized, it can do the same logic of picking up only those fields which are mentioned from the metadata of the request.

[bogdandrutu]

@jpkrohling the list of "requirements" can be embedded in the code. Something like the "AuthExtension" can have a "Requirements()" API that tells the HTTP/GRPC what infos are required?

[orloffv]

please don't forget the Host property

[jpkrohling]

Something like the "AuthExtension" can have a "Requirements()" API that tells the HTTP/GRPC what infos are required?

What if the requirement is coming from a processor down the line? Or perhaps even from an exporter (for routing capabilities, like described in #4814)

[bogdandrutu]

@jpkrohling maybe can use "Host" as the intermediary, components can "record their needs"? Not sold necessary on the "Host" but you can come up with a solution :)

[bogdanov1609]

Hello, friendly reminder :)

Is there any finished plan for this issue?

[jpkrohling]

Not yet. It's in my queue, but I have a few things to complete before going to this one.

[neelayu]

@jpkrohling I took the liberty to work on this. Kindly check #5080

[jpkrohling]

I'll take a look!

[bogdanov1609]

Friendly reminder on this one :)

[jpkrohling]

I'm back from my leave, I have this on my queue.

[jpkrohling]

This took a little longer than planned, but it's finally there for review.

[TylerHelmuth]

The current implementation of this feature changes only confighttp, so I'm switching the milestone for now. We can switch it back if we decide via the PR that it should live in configauth instead.