Add support for authentication and authorization #554
Conversation
woop
requested review from
davidheryanto,
khorshuheng,
pradithya and
zhilingc
as code owners
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: woop The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest |
1 similar comment
|
/retest |
|
/test test-end-to-end |
|
Does this really only support Google OpenID? It shouldn't be any effort to make the OIDC configurable to support any valid implementation such as Auth0. |
| * Google Open ID Authentication Provider. This provider is used to validate incoming requests to | ||
| * Feast Core. | ||
| */ | ||
| public class GoogleOpenIDAuthenticationProvider implements AuthenticationProvider { |
There was a problem hiding this comment.
This seems like it would support any openid provider. Any reason the package and class can't be made generic?
There was a problem hiding this comment.
Actually, why even have this class at all? Why not just set up the JwtAuthenticationProvider directly?
There was a problem hiding this comment.
This seems like it would support any openid provider. Any reason the package and class can't be made generic?
It can.
Actually, why even have this class at all? Why not just set up the JwtAuthenticationProvider directly?
Honestly I don't recall why this was broken out. It looks like if we generalize cert handling then we can use JwtAuthenticationProvider
|
|
||
| if (securityProperties.getAuthentication().isEnabled()) { | ||
| switch (securityProperties.getAuthentication().getProvider()) { | ||
| case "GoogleOpenID": |
There was a problem hiding this comment.
not to beat a dead horse but this seems to provide little value over directly setting properties of the JwtAuthenticationProvider
| switch (securityProperties.getAuthentication().getProvider()) { | ||
| case "GoogleOpenID": | ||
| providers.add( | ||
| new GoogleOpenIDAuthenticationProvider( |
There was a problem hiding this comment.
Why not use the capabilities of spring to configure the providers rather than build another factory method?
There was a problem hiding this comment.
Can be done. See points above.
| if (securityProperties.getAuthentication().isEnabled() | ||
| && securityProperties.getAuthorization().isEnabled()) { | ||
| switch (securityProperties.getAuthorization().getProvider()) { | ||
| case "KetoAuthorization": |
There was a problem hiding this comment.
same as authentication provider factory comment.
There was a problem hiding this comment.
How would you use Spring in this case?
There was a problem hiding this comment.
It's been awhile since I have done this in Spring Config but basically you dynamically create the @bean which then can accept normal spring config:
@Configuration
public class AuthConfiguration
{
@Value("${feast.auth.authorizationProviderClass}")
private String authorizationProviderClassName;
@Bean
public AuthorizationProvider authorizationProvider()
{
Class<?> authClass = Class.forName(authorizationProviderClassName);
AuthorizationProvider authProvider = AuthorizationProvider.class.cast(authClass.newInstance());
return authProvider;
}
There was a problem hiding this comment.
Yea that looks great, as long as failure is graceful and easy to debug.
Yip, it should be pretty easy to extend to non-Google through config. This was a minimal implementation. The reason I specified it as Google is because our Python SDK uses OIDC tokens that it gets from a local Gcloud SDK and sends that along to Feast for auth. That is very Google specific. Server side can easily be generalized. |
Got it. We have it on our list to add an generic oauth2/openid auth module to the client as well. |
|
replaced by #793 |
What this PR does / why we need it:
First implementation of auth for Feast (related to #504 minimal implementation).
Limitations
Which issue(s) this PR fixes:
Related to #504, but doesn't close the card. This is a minimal implementation.
Does this PR introduce a user-facing change?: