Skip to content
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 #2101

Closed
Tracked by #2603
chris-smith-zocdoc opened this issue Nov 10, 2020 · 9 comments · Fixed by #2767
Closed
Tracked by #2603

Custom authenticator logic #2101

chris-smith-zocdoc opened this issue Nov 10, 2020 · 9 comments · Fixed by #2767

Comments

@chris-smith-zocdoc
Copy link
Contributor

Is your feature request related to a problem? Please describe.
I'd like to add custom authentication logic into our build of the collector, similar to how custom processors may be registered

The current auth module doesn't appear to support this at the moment.
https://github.com/open-telemetry/opentelemetry-collector/tree/master/config/configauth

Describe the solution you'd like
The simplest solution might be to write any custom authentication logic as a processor. I don't believe this is possible currently because the original request information (HTTP Headers) isn't accessible by the processors. If the headers were placed into the Context, then this would be the easiest path forward.

Describe alternatives you've considered
A more complicated alternative would allow custom builds of the collector to register new authentication modules that the existing receivers may use.

cc @jpkrohling

@jpkrohling
Copy link
Member

The original design for the auth was indeed to be a processor, but we switched that to be something that people add to the receivers, so that unauthorized requests wouldn't cause data to flow down the pipeline.

What I think we could do is to expose a registry. Your logic would be an extension that would register itself. Do you think that would work for your case?

@chris-smith-zocdoc
Copy link
Contributor Author

Yeah that should work

@jpkrohling
Copy link
Member

jpkrohling commented Nov 12, 2020

@chris-smith-zocdoc even though this is assigned to me, would you like to give this a try? I'll probably not have time to work on this for the next few weeks, which means that it'll probably be done only next year.

@chris-smith-zocdoc
Copy link
Contributor Author

@christopher-taormina-zocdoc from my team should be able to work on this. @jpkrohling can you provide links to the important sections of code we'll need to understand/modify. Any advice you have for structuring the prs for easier review+merge would also be appreciated.

@jpkrohling
Copy link
Member

Sure. I'm out most of this week, so, here's just a short overview:

  1. configauth would get a registry, AuthenticatorRegistry (map[string]Authenticator)
  2. move the oidc authenticator to an extension, register it with the registry
  3. configauth would only have a property, authenticator, with a value that should match the key from the registry
  4. the authenticator is configured via the extension's config (the whole OIDC part from the configauth moves there)

For backwards compatibility, we might want to read the OIDC config on the last step and configure a new OIDC extension with it, if possible at all.

The whole configauth code is here: https://github.com/open-telemetry/opentelemetry-collector/blob/6b67312d590ca00542167c535794b1442558d422/config/configauth/

@christopher-taormina-zocdoc

@jpkrohling In moving oidc to an extension, are we expecting the configuration to have the authenticator specified in both the receiver config and in the service config?

For example

receivers:
  somereceiver:
    grpc:
      authenticator: oidc

extensions:
  oidc:
    ...

service:
  exetnsions: [oidc]
  pipeline:
    traces/toSomewhere:
      recievers: [somereceiver]
      ...

@jpkrohling
Copy link
Member

Yes, I believe so. The reason is that receivers might potentially have different authenticators.

@christopher-taormina-zocdoc

Cool. To me, it seems a little cumbersome to have to also specify the auth extension in the service extensions as this would be very easy thing to overlook, tho I can't think.of a way to get around that without having a new component for auth layers.

@jpkrohling
Copy link
Member

I can't think.of a way to get around that without having a new component for auth layers.

I think there's really no other way, at least for the moment.

jpkrohling added a commit to jpkrohling/opentelemetry-collector that referenced this issue Mar 23, 2021
Fixes open-telemetry#2101

Signed-off-by: Juraci Paixão Kröhling <[email protected]>
jpkrohling added a commit to jpkrohling/opentelemetry-collector that referenced this issue Apr 8, 2021
Fixes open-telemetry#2101

Signed-off-by: Juraci Paixão Kröhling <[email protected]>
jpkrohling added a commit to jpkrohling/opentelemetry-collector that referenced this issue Apr 12, 2021
Fixes open-telemetry#2101

Signed-off-by: Juraci Paixão Kröhling <[email protected]>
jpkrohling added a commit to jpkrohling/opentelemetry-collector that referenced this issue Apr 14, 2021
Fixes open-telemetry#2101

Signed-off-by: Juraci Paixão Kröhling <[email protected]>
jpkrohling added a commit to jpkrohling/opentelemetry-collector that referenced this issue Apr 14, 2021
Fixes open-telemetry#2101

Signed-off-by: Juraci Paixão Kröhling <[email protected]>
jpkrohling added a commit to jpkrohling/opentelemetry-collector that referenced this issue Apr 22, 2021
Fixes open-telemetry#2101

Signed-off-by: Juraci Paixão Kröhling <[email protected]>
jpkrohling added a commit to jpkrohling/opentelemetry-collector that referenced this issue Apr 22, 2021
Fixes open-telemetry#2101

Signed-off-by: Juraci Paixão Kröhling <[email protected]>
jpkrohling added a commit to jpkrohling/opentelemetry-collector that referenced this issue Apr 22, 2021
Fixes open-telemetry#2101

Signed-off-by: Juraci Paixão Kröhling <[email protected]>
jpkrohling added a commit to jpkrohling/opentelemetry-collector that referenced this issue Apr 23, 2021
Fixes open-telemetry#2101

Signed-off-by: Juraci Paixão Kröhling <[email protected]>
jpkrohling added a commit to jpkrohling/opentelemetry-collector that referenced this issue Apr 27, 2021
Fixes open-telemetry#2101

Signed-off-by: Juraci Paixão Kröhling <[email protected]>
jpkrohling added a commit to jpkrohling/opentelemetry-collector that referenced this issue Apr 29, 2021
Fixes open-telemetry#2101

Signed-off-by: Juraci Paixão Kröhling <[email protected]>
tigrannajaryan pushed a commit that referenced this issue Apr 29, 2021
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:
- [x] More unit tests
- [x] Review README files
- [x] 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
MovieStoreGuy pushed a commit to atlassian-forks/opentelemetry-collector that referenced this issue Nov 11, 2021
* update metrics version set

Signed-off-by: Anthony J Mirabella <[email protected]>

* Prepare for releasing v0.22.0

* Update CHANGELOG for metrics v0.22.0 release

Signed-off-by: Anthony J Mirabella <[email protected]>

* Fixup CHANGELOG linking

Signed-off-by: Anthony J Mirabella <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants