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

v2 Consider adding allow-list behaviour to auth interceptor #345

Open
bwplotka opened this issue Oct 2, 2020 · 6 comments
Open

v2 Consider adding allow-list behaviour to auth interceptor #345

bwplotka opened this issue Oct 2, 2020 · 6 comments
Labels

Comments

@bwplotka
Copy link
Collaborator

bwplotka commented Oct 2, 2020

AC:

  • Define simple functionality for auth interceptor before stating stable version: How to enable whitelist auth? Any interface changes?

Pulled from #275 for visibility.

Blocker for v2.

@bwplotka bwplotka added the v2 label Oct 2, 2020
@jzelinskie
Copy link

We actually wrote a very simple mixin that you embed in a server the auth interceptor will ignore it -- this is most useful for the healthcheck service: https://pkg.go.dev/github.com/authzed/grpcutil#IgnoreAuthMixin

@bwplotka bwplotka added this to To do in Milestone v2 Aug 6, 2021
@devnev
Copy link
Collaborator

devnev commented Oct 8, 2022

There's also two interceptor packages relating to selectively enabling other interceptors: skip and selector.

So possible interface could something like one of the following:

    auth.UnaryServerInterceptor(authFunc, auth.IgnoreMethods("grpc.health.v1.Health/Check"))
    auth.UnaryServerInterceptor(authFunc, auth.IgnoreServices(health_pb.Health_ServiceDesc)
    selector.UnaryServerInterceptor(auth.UnaryServerInterceptor(authFunc), selector.Not(selector.MatchMethods("grpc.health.v1.Health/Check")))
    selector.UnaryServerInterceptor(auth.UnaryServerInterceptor(authFunc), selector.Not(selector.MatchServices(health_pb.Health_ServiceDesc)))
    skip.UnaryServerInterceptor(auth.UnaryServerInterceptor(authFunc), skip.Methods("grpc.health.v1.Health/Check"))
    skip.UnaryServerInterceptor(auth.UnaryServerInterceptor(authFunc), skip.Services(health_pb.Health_ServiceDesc))

This could also be an argument to adjust the interfaces of the skip and selector packages, or to merge them.

@bwplotka
Copy link
Collaborator Author

Great point. I am then considering closing this issue as it's doable with selector and skip... Question is, is this interface nice and if not, can we improve it before v2?

@bwplotka
Copy link
Collaborator Author

Actually you just mentioned improvements ideas...

  1. Auth is great, but we need selection for many other interceptors so skip/selector works better.
  2. Selector looks nice, it's bit explicit but clean. It also allows skipping "Not" for allow-list.
  3. Skip is nice, but negation of skip would look weird (skip.Not)

So 2?

@bwplotka
Copy link
Collaborator Author

Still, not sure if I like the selector types (match, not) etc. It gets pretty complex pretty soon and requires a lot of maintenance.

I think I would stick to func (context.Context, interceptor.CallMeta) bool "selector". We can consume ...Selector so we can add explicit, simpler selection when needed.

    selector.UnaryServerInterceptor(auth.UnaryServerInterceptor(authFunc), selector.Selector(matchAllButHealthCheck))

I would also propose removing Auth fullMethodInfo to ensure users use selector instead (one thing). Hope that's ok to users like @jzelinskie who would need to move to selector from https://pkg.go.dev/github.com/authzed/grpcutil#IgnoreAuthMixin

@bwplotka
Copy link
Collaborator Author

done in #543

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Development

No branches or pull requests

3 participants