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

[Feature] Allow DynamicAccessController to access the HTTP Request headers in order to implement JWT token authentication #202

Open
9 tasks done
eolivelli opened this issue Jan 31, 2023 · 5 comments
Labels
enhancement New feature or request

Comments

@eolivelli
Copy link
Contributor

Willingness to contribute

Yes. I can contribute a fix for this bug independently.

Feature Request Proposal

My understanding from code is that the Router, the Controller and the Server expose an HTTP endpoint.
In this case implementing token based authentication (like JWT) is pretty easy, because we can simply require the client to pass the token in a HTTP header.

The only thing we need to do is to allow the DynamicAccessController to access the HTTP request headers (and possibly cache the result of the validation but attaching it to the Netty Channel)

The implementation on the clients should be pretty straighforward, as we only need to let the users configure the token and pass it in every HTTP request as an header

Motivation

What is the use case for this feature?

Implementing JWT authentication (and some day also OAuth2)

Details

No response

What component(s) does this bug affect?

  • Controller: This is the control-plane for Venice. Used to create/update/query stores and their metadata.
  • Router: This is the stateless query-routing layer for serving read requests.
  • Server: This is the component that persists all the store data.
  • VenicePushJob: This is the component that pushes derived data from Hadoop to Venice backend.
  • Thin Client: This is a stateless client users use to query Venice Router for reading store data.
  • Fast Client: This is a stateful client users use to query Venice Server for reading store data.
  • Da Vinci Client: This is an embedded, stateful client that materializes store data locally.
  • Samza: This is the library users use to make nearline updates to store data.
  • Admin Tool: This is the stand-alone client used for ad-hoc operations on Venice.
@eolivelli eolivelli added the enhancement New feature or request label Jan 31, 2023
@ZacAttack
Copy link
Contributor

ZacAttack commented Feb 8, 2023

We could do better first class support for this through the components (we'd need to add something into the controller and server to accommodate admin/write/fast client paths) but the router today has something that can be worked with.

RouterServer.addOptionalChannelHandler(String key, ChannelHandler channelHandler) is a method that allows one to pass in channelHandlers that get added to the end of the request pipeline in the router server. Here you could potentially apply custom logic in the request pipeline to validate credentials. Maybe kind of a bummer that it's the last in the pipeline (ideally any security checks should happen first to reduce resource commitment).

First class support will take a bit longer, but I think with the above you could get something working on at least the read path, and maybe the fix we apply will work off this notion (basically expose adding request handlers on the router and server and controller and then add utility for a credential provider in the admin tool/VenicePushJob/fast client/etc.)

@eolivelli
Copy link
Contributor Author

Probably one approach would be to not rely anymore on DynamicAccessController but on AuthorizerService


I tried to build some adapter between DynamicAccessController and AuthorizerService (an AuthorizerService implementation that forwards to the DynamicAccessController) but there are things about ACLs that cannot be easily adapted.

@ZacAttack
Copy link
Contributor

Probably one approach would be to not rely anymore on DynamicAccessController but on AuthorizerService

I tried to build some adapter between DynamicAccessController and AuthorizerService (an AuthorizerService implementation that forwards to the DynamicAccessController) but there are things about ACLs that cannot be easily adapted.

Thats definitely the intended approach, DynamicAccessController is the old interface that we mean to deprecate. Guess we should just get to it.

@FelixGV
Copy link
Contributor

FelixGV commented Apr 15, 2023

Guess we should just get to it.

InDay's coming up 😉 !

@eolivelli
Copy link
Contributor Author

I think that one of the main problems is that we don't have a clear separation between Authentication (mapping a user request to a principal/role) and Authorization (deciding who can do what).

Also the DynamicAccessController and the Authorizer services have another problem, they are used both for performing authorization assertions and to handle ACLs (management)

I think that eventually we need to split those three responsibilities into specific APIs.

I am drafting an AuthenticationService API here in my fork
datastax/venice#4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants