Custom authenticator logic

[jpkrohling]

Description:
This PR changes the configauth to accept a component.Host, from which it will extract the authenticator to use based on a new authenticator name property.

This is only a draft at the moment, making sure that the general approach is acceptable.

Pending:

• More unit tests
• Review README files
• Comprehensive examples

Link to tracking Issue:
Closes #2101
Contributes to #2603

Testing:
A couple of unit tests were added for the first phase. More comprehensive testing will be done once the main idea is validated.

Documentation:
None so far, README changes are planned

[tigrannajaryan]

A couple things that I am slightly unsure about:

• Do we want authenticators to be extensions? This requires them to be enabled in the config explicitly otherwise "auth" setting of receivers is not valid. Should we consider making authenticators a special type of component that is enabled and started automatically? (This assumes authenticators don't need config settings themselves). What happens if an "auth" is referenced that has no correspondingly named extension? Is that a config loading error?
• I am still trying to make up my mind on whether I like the usage of reflection to find candidate to inject authenticators.
• Not totally sure we want to call this "injection" or use some other more specific term (InitAuth?)

Thoughts?

[jpkrohling]

Do we want authenticators to be extensions? This requires them to be enabled in the config explicitly otherwise "auth" setting of receivers is not valid.

Having the authenticators being extensions allows them to also make use of other extensions if needed. It's also easier for users and auth developers to understand the lifecycle semantics, as they are the same as extensions.

Should we consider making authenticators a special type of component that is enabled and started automatically? (This assumes authenticators don't need config settings themselves).

Authenticators will need config settings. OIDC authenticator needs its own set of properties, which will certainly be different than a hypothetical "htpasswd'' authenticator. As an extension, it needs to be declared as part of the service in order to be started, which I find very convenient for testing scenarios.

What happens if an "auth" is referenced that has no correspondingly named extension? Is that a config loading error?

The InjectAuthenticators implementation may return an error. The initial implementation for the Authentication config object from configauth will return an error if the named extension does not exist. This PR isn't catching that error yet, but it should do that.

Currently, the injection is happening after the extensions are started, but before the pipeline is started. I placed it here so that we could have an explicit injection into pipeline components in the future if necessary. This means that it's not a config loading error, but an error setting up the service/pipeline.

https://github.com/open-telemetry/opentelemetry-collector/pull/2767/files#diff-85ee04e67bda04e6159b92426c87fe161c58d4b5550ef2b0fa7a97214309c125R312-R322

The other place I experimented with was in the config loading part, after the last component is loaded (processors) and before the service config is loaded, where the line 158 is in the snippet below:

opentelemetry-collector/config/config.go

Lines 153 to 166 in 9de400c

	processors, err := loadProcessors(v.GetStringMap(processorsKeyName), factories.Processors)
	if err != nil {
	return nil, err
	}
	config.Processors = processors
	// Load the service and its data pipelines.
	service, err := loadService(rawCfg.Service)
	if err != nil {
	return nil, err
	}
	config.Service = service
	return &config, nil

I am still trying to make up my mind on whether I like the usage of reflection to find candidate to inject authenticators.

Honestly, I'm not 100% happy either, but I can't think of another solution that would work for configs. I played with the idea of registries in the past, storing them as part of the application/service, but that still won't work for configs, which is where we need it right now. Another iteration of the auth mechanism was when it was implemented as a processor, but there are two main downsides: we might want different auth mechanisms for different receivers, and we want to authenticate the request before the protected receiver has access to the data.

Not totally sure we want to call this "injection" or use some other more specific term (InitAuth?)

I'm probably influenced by CDI (Context and Dependency Injection for Java), but I'm certainly open to suggestions of better names.

[tigrannajaryan]

@bogdandrutu please also review.

[gramidt]

@jpkrohling - The structure and implementation make sense on my first pass review this morning. I'll spend additional time this afternoon / evening to dig in further.

[tigrannajaryan]

@bogdandrutu what do you think? This latest version seems cleaner to me than the previous version that used reflection.

[jcchavezs]

LGTM, just left some minor comments.

[tigrannajaryan]

The proposal LGTM. Please rebase.

[codecov]

Codecov Report

Merging #2767 (cdc6244) into main (73b55f0) will decrease coverage by 0.02%.
The diff coverage is 87.06%.

❗ Current head cdc6244 differs from pull request most recent head 84c6f94. Consider uploading reports for the commit 84c6f94 to get more accurate results

@@            Coverage Diff             @@
##             main    #2767      +/-   ##
==========================================
- Coverage   92.06%   92.03%   -0.03%     
==========================================
  Files         313      314       +1     
  Lines       15439    15457      +18     
==========================================
+ Hits        14214    14226      +12     
- Misses        817      821       +4     
- Partials      408      410       +2     

Impacted Files	Coverage Δ
receiver/jaegerreceiver/trace_receiver.go	72.98% <50.00%> (-0.58%)	⬇️
receiver/opencensusreceiver/opencensus.go	82.72% <56.52%> (-6.39%)	⬇️
receiver/otlpreceiver/factory.go	90.90% <75.00%> (+9.77%)	⬆️
extension/authoidcextension/extension.go	98.00% <96.55%> (ø)
config/configauth/authenticator.go	100.00% <100.00%> (ø)
config/configauth/configauth.go	100.00% <100.00%> (ø)
config/configauth/mocks.go	100.00% <100.00%> (ø)
config/configgrpc/configgrpc.go	97.53% <100.00%> (-1.24%)	⬇️
extension/authoidcextension/factory.go	100.00% <100.00%> (ø)
receiver/jaegerreceiver/factory.go	97.80% <100.00%> (+2.10%)	⬆️
... and 19 more

---

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 1be517f...84c6f94. Read the comment docs.

[jpkrohling]

@tigrannajaryan I believe this is now ready: the tests are fixed and documentation is in place.

[gramidt]

@jpkrohling - This is looking great! You mentioned a last manual e2e test to look into a test failure. What was the failure you were looking for?

[jpkrohling]

You mentioned a last manual e2e test to look into a test failure. What was the failure you were looking for?

From my recollection, those were two different things: I wanted, and still want, to get a manual e2e performed to verify that none of the latest changes broke anything. The unit tests should be good enough, but you never know...

The concrete failure I was talking about was a data race in the tests around the Application, but I sorted that one out already.

[jpkrohling]

I just ran a manual e2e test using the existing blog post as starting point, and confirmed that this works as intended: with an invalid token, data isn't processed/exported, whereas data is exporter when a valid token is given. For reference, here's the config I ended up using:

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: account

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: /tmp/certs/cert.pem
          key_file: /tmp/certs/cert-key.pem
        auth:
          # right now, this is the only property within "auth"
          # we could make "auth" a string-based struct, but if we 
          # ever need a property that would not be related to the 
          # authenticator, we'd need to do a breaking change
          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: /tmp/certs/ca.pem
    per_rpc_auth:
      type: bearer
      bearer_token: eyJhbGciOiJSUzI1NiIsInR5cCIgOiAiSldUIiwia2lkIiA6ICJPcHByWEc5RVFkR1VVZnNqNUhUUTJOXzdwUDNWQ25QX2ZIVWFsVVE2bkxzIn0.eyJleHAiOjE2MTk1MTcyMTUsImlhdCI6MTYxOTUxNjkxNSwianRpIjoiMzM5YjM4N2EtYTQwMi00NzI5LTkyMDctODdkM2E3N2IyY2FlIiwiaXNzIjoiaHR0cDovL2xvY2FsaG9zdDo4MDgwL2F1dGgvcmVhbG1zL29wZW50ZWxlbWV0cnkiLCJhdWQiOlsiY29sbGVjdG9yIiwiYWNjb3VudCJdLCJzdWIiOiJlMGMzMWU4Ny1lNzdiLTQ4MTktOGJiZi0zOWM5NTE2OWQ1MjkiLCJ0eXAiOiJCZWFyZXIiLCJhenAiOiJhZ2VudCIsImFjciI6IjEiLCJyZWFsbV9hY2Nlc3MiOnsicm9sZXMiOlsib2ZmbGluZV9hY2Nlc3MiLCJ1bWFfYXV0aG9yaXphdGlvbiJdfSwicmVzb3VyY2VfYWNjZXNzIjp7ImFjY291bnQiOnsicm9sZXMiOlsibWFuYWdlLWFjY291bnQiLCJtYW5hZ2UtYWNjb3VudC1saW5rcyIsInZpZXctcHJvZmlsZSJdfX0sInNjb3BlIjoiZW1haWwgcHJvZmlsZSIsImNsaWVudEhvc3QiOiIxMC4wLjIuMTAwIiwiZW1haWxfdmVyaWZpZWQiOmZhbHNlLCJjbGllbnRJZCI6ImFnZW50IiwicHJlZmVycmVkX3VzZXJuYW1lIjoic2VydmljZS1hY2NvdW50LWFnZW50IiwiY2xpZW50QWRkcmVzcyI6IjEwLjAuMi4xMDAifQ.VnM0sxVMM7K56306vWH9gVS5IGqFfCx2Vy9TslWFKaFCylwwFxgJT34RHkQm861F9b1kkGN_0MHdAI326B92_lQgv1siMfXMo3z3UlBW95IRSHumUTu-cQHfkHNjzYZ2Z6qIJyO6NZ147dRvOVPwFq4jaTwzfWPAqjIMRIT1bjQyJKSRW6lDoubgCvjbk_4rxbg9U8iSzeXNWvwJsPtCQOXiOKs01Zd370F9F1yRhMbhjKNYszPkL8Lu1K214O1Bw2dubZcx9Fue1suuOlX4wCKjBHj3TB80JFOVn4fJsMvEMFNN9dVQ2jSJjSCionPcPDXyDbeyVHx1iO_mNXlRcg
    retry_on_failure:
      enabled: false

  jaeger:
    endpoint: localhost:14250
    insecure: true

service:
  extensions:
    - oidc
  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:
        - jaeger # if data is visible in Jaeger, auth worked

This example requires both Jaeger and Keycloak to be running in the same machine.

[tigrannajaryan]

Thanks @jpkrohling
LGTM, my only remaining comments are about better documenting and making the Authenticate interface easier to understand. We can merge after that.

[tigrannajaryan]

The ctx here is the incoming request context, right? Please document that the interceptor is expected to extract the authentication credentials from the context and authenticate them.

[jpkrohling]

It is the incoming RPC context, yes. The DefaultUnaryInterceptor/DefaultStreamInterceptor indeed extracts the credentials from the headers, but some receivers might have a different authentication method, perhaps by a field in the payload. Therefore, I would leave the "how'' to get the auth data out of this doc here.

[tigrannajaryan]

Can we say "authenticate for example by looking up for the credentials in the incoming RPC request context" or something like that?

[jpkrohling]

Added a couple of lines in this direction:

	// While the context is the typical source of authentication data, the interceptor is free to determine where the auth data should come from. For instance, some
	// receivers might implement an interceptor that looks into the payload instead.

[jpkrohling]

Thanks for the review! Besided the requested godoc changes, I did one more small change, moving the ToServerOpts logic to the configgrpc.

opentelemetry-collector/config/configgrpc/configgrpc.go

Lines 299 to 309 in 785d7ea

	if gss.Auth != nil {
	authenticator, err := configauth.GetAuthenticator(ext, gss.Auth.AuthenticatorName)
	if err != nil {
	return nil, err
	}
	opts = append(opts,
	grpc.UnaryInterceptor(authenticator.GrpcUnaryServerInterceptor),
	grpc.StreamInterceptor(authenticator.GrpcStreamServerInterceptor),
	)
	}

It was necessary to have it in the configauth before, but once we made the configgrpc aware of the extensions, we can handle it there instead. I think it's cleaner this way.

[tigrannajaryan]

I can merge once you confirm that you are OK and done with documenting.

[tigrannajaryan]

Also, please rebase from main.

[jpkrohling]

Rebased and docs updated. I also ran another manual e2e test.

[tigrannajaryan]

@jpkrohling thank you for being patient. This is great!