venice icon indicating copy to clipboard operation
venice copied to clipboard

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

Open eolivelli opened this issue 3 years ago • 5 comments

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?

  • [X] Controller: This is the control-plane for Venice. Used to create/update/query stores and their metadata.
  • [X] Router: This is the stateless query-routing layer for serving read requests.
  • [X] Server: This is the component that persists all the store data.
  • [X] VenicePushJob: This is the component that pushes derived data from Hadoop to Venice backend.
  • [X] Thin Client: This is a stateless client users use to query Venice Router for reading store data.
  • [X] Fast Client: This is a stateful client users use to query Venice Server for reading store data.
  • [X] Da Vinci Client: This is an embedded, stateful client that materializes store data locally.
  • [X] Samza: This is the library users use to make nearline updates to store data.
  • [X] Admin Tool: This is the stand-alone client used for ad-hoc operations on Venice.

eolivelli avatar Jan 31 '23 12:01 eolivelli

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.)

ZacAttack avatar Feb 08 '23 21:02 ZacAttack

Probably one approach would be to not rely anymore on DynamicAccessController but on AuthorizerService https://github.com/linkedin/venice/blob/63a7921da1c1814f9d856a6375f84ba392c01abb/internal/venice-common/src/main/java/com/linkedin/venice/authorization/AuthorizerService.java#L25 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.

eolivelli avatar Apr 14 '23 10:04 eolivelli

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

https://github.com/linkedin/venice/blob/63a7921da1c1814f9d856a6375f84ba392c01abb/internal/venice-common/src/main/java/com/linkedin/venice/authorization/AuthorizerService.java#L25

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.

ZacAttack avatar Apr 14 '23 23:04 ZacAttack

Guess we should just get to it.

InDay's coming up 😉 !

FelixGV avatar Apr 15 '23 01:04 FelixGV

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 https://github.com/datastax/venice/pull/4

eolivelli avatar Apr 24 '23 14:04 eolivelli