search

grpc-ecosystem / go-grpc-middleware

Golang gRPC Middlewares: interceptor chaining, auth, logging, retries and more.
Apache License 2.0
6.26k stars 690 forks source link

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

Open bwplotka opened 3 years ago

bwplotka commented 3 years ago

AC:

Pulled from https://github.com/grpc-ecosystem/go-grpc-middleware/issues/275 for visibility.

Blocker for v2.

jzelinskie commented 3 years ago

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

devnev commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

done in https://github.com/grpc-ecosystem/go-grpc-middleware/pull/543