-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Custom authenticator logic #2767
Custom authenticator logic #2767
Conversation
A couple things that I am slightly unsure about:
Thoughts? |
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.
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.
The 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. 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
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.
I'm probably influenced by CDI (Context and Dependency Injection for Java), but I'm certainly open to suggestions of better names. |
@bogdandrutu please also review. |
eb41e89
to
ecdfe0e
Compare
ecdfe0e
to
e488c66
Compare
@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. |
@bogdandrutu what do you think? This latest version seems cleaner to me than the previous version that used reflection. |
LGTM, just left some minor comments. |
2b92687
to
3208f26
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The proposal LGTM. Please rebase.
1669efb
to
4b27832
Compare
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
@tigrannajaryan I believe this is now ready: the tests are fixed and documentation is in place. |
baa58d0
to
e902663
Compare
@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? |
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 |
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. |
cdc6244
to
3026883
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @jpkrohling
LGTM, my only remaining comments are about better documenting and making the Authenticate interface easier to understand. We can merge after that.
config/configauth/authenticator.go
Outdated
|
||
// UnaryInterceptor is a helper method to provide a gRPC-compatible UnaryInterceptor, typically calling the authenticator's Authenticate method. | ||
UnaryInterceptor(context.Context, interface{}, *grpc.UnaryServerInfo, grpc.UnaryHandler) (interface{}, error) | ||
UnaryInterceptor(ctx context.Context, req interface{}, srvInfo *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (interface{}, error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we say "authenticate for example by looking up for the credentials in the incoming RPC request context" or something like that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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
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. |
I can merge once you confirm that you are OK and done with documenting. |
Also, please rebase from main. |
Fixes open-telemetry#2101 Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
cb26e9b
to
84c6f94
Compare
Rebased and docs updated. I also ran another manual e2e test. |
@jpkrohling thank you for being patient. This is great! |
Description:
This PR changes the configauth to accept a
component.Host
, from which it will extract the authenticator to use based on a new authenticatorname
property.This is only a draft at the moment, making sure that the general approach is acceptable.
Pending:
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