API Refactors: Authorization middleware scaffolding #1589
Conversation
This middleware can be used to set up authorization for the whole API surface from a single spot. The middleware provide information on the context for handlers, like the caller address, SPIFFE ID, and X509-SVID. This change also updates the rpccontext package to provide finer grained values and remove specific details on how those values are determined. It also removes the auth check in a bundle handler since the middleware is now in charge of all authorization checks. Signed-off-by: Andrew Harding <andrew.harding@hpe.com>
azdagron
requested review from
amartinezfayo,
APTy,
evan2645 and
mcpherrinm
as code owners
There was a problem hiding this comment.
Thank you for this @azdagron!
My main feedback is around error codes returned.
| authorizer, ok := m.authorizers[methodName] | ||
| if !ok { | ||
| rpccontext.Logger(ctx).Error("Authorization misconfigured; this is a bug") | ||
| return nil, status.Errorf(codes.Internal, "authorization misconfigured") |
There was a problem hiding this comment.
Maybe codes.PermissionDenied is a better code to use here? We may include the method name in the error?
There was a problem hiding this comment.
My intention here was to only return PermissionDenied if we actually determined that the caller did not have permission. In this case, this means that we (SPIRE devs) messed up and didn't set up authorization for a method. Clients may choose to do some action based on the status code and I didn't want to conflate the two. Internal is an indication that something is wrong outside of the callers control that they can't rectify on their own.
I didn't add the method name to the returned error because the caller presumably knows the method name they are calling? I'd be ok adding it though if you think it will help debuggability.
The logger will already have fields added with the method name.
There was a problem hiding this comment.
I think that makes sense.
Regarding including the method name, I think that I will not hurt and can help in the case of debugging this error.
| func callerContextFromContext(ctx context.Context) (context.Context, error) { | ||
| p, ok := peer.FromContext(ctx) | ||
| if !ok { | ||
| return nil, status.Error(codes.Internal, "no peer information available") |
There was a problem hiding this comment.
Looks like we currently return codes.Unauthenticated in this case. Curious to know if you consider that needs to be changed?
There was a problem hiding this comment.
This check is purely defensive. I don't expect this to ever happen, since gRPC takes care of adding the peer to the context. That being said, I think Internal is a clearer code and communicates that something is really wrong internally. A caller might interpret Unauthenticated to mean that they could take some action to become authenticated, but in this case, there isn't anything they can do.
Also fixed up some comments based on PR feedback. Signed-off-by: Andrew Harding <andrew.harding@hpe.com>
|
Build passed on travis but is stuck notifying. Merging anyway. |
This middleware can be used to set up authorization for the whole API surface from a single spot.
The middleware provide information on the context for handlers, like the caller address, SPIFFE ID, and X509-SVID.
This change also updates the rpccontext package to provide finer grained values and remove specific details on how those values are determined.
It also removes the auth check in a bundle handler since the middleware is now in charge of all authorization checks.
Individual authorizers to use with this middleware will land in future PRs.