Remote authenticator and authorizer #234
Conversation
moroten
force-pushed
the
remote-auth
branch
3 times, most recently
from
8e43646 to
47d3807
Compare
|
@EdSchouten Welcome back from Christmas holiday. I forgot to bring this up for discussion yesterday. Any comments? No hurry, take your time. |
|
|
||
| // Deny the request, returning a fixed error message back | ||
| // to the client. | ||
| string deny = 2; |
There was a problem hiding this comment.
Maybe good to document what kind of status code should be sent back to the client? Should it always be the equivalent of Unauthenticated? Same with AuthorizeResponse.deny.
pkg/grpc/remote_authenticator.go
Outdated
| if entry != nil && entry.HasExpired(now) { | ||
| entry = nil | ||
| } | ||
| if entry == nil { |
There was a problem hiding this comment.
if entry == nil || entry.HasExpired(now) {?
pkg/grpc/remote_authenticator.go
Outdated
| // Wait for the remote request to finish. | ||
| select { | ||
| case <-ctx.Done(): | ||
| return nil, util.StatusWrapWithCode(ctx.Err(), codes.Unauthenticated, "Context cancelled") |
There was a problem hiding this comment.
return nil, util.StatusFromContext(ctx)
pkg/grpc/remote_authenticator.go
Outdated
| // Noop | ||
| } | ||
| } | ||
| return entry.response.authMetadata, entry.response.err |
There was a problem hiding this comment.
I don't think this is sound. What happens if entry.response.err is of type codes.Canceled? We end up returning a cancelation error belonging to a different client.
There was a problem hiding this comment.
Would it be correct to use contect.Background() in the request?
There was a problem hiding this comment.
That could work, but has the downside that cancelation isn't propagated immediately. My recommendation would be to stick to ctx, but don't coalesce error results of in-flight RPCs. This is also what we do in other parts of Buildbarn (e.g., BlobReplicator implementations)
| "google.golang.org/grpc/status" | ||
| ) | ||
|
|
||
| type remoteGrpcRequestAuthenticator struct { |
There was a problem hiding this comment.
I'd recommend removing Grpc from the name. That it's for gRPC is already implied based on the package name.
| ctrl, ctx := gomock.WithContext(context.Background(), t) | ||
| md := metadata.New( | ||
| map[string]string{ | ||
| "Authorization": "token", |
There was a problem hiding this comment.
Doesn't gRPC metadata use lowercase header names?
There was a problem hiding this comment.
The Go gRPC library is case insensitive and translates them to lower case.
There was a problem hiding this comment.
Ah, gotcha. It looks like metadata.New() does the normalization to lowercase for you, so it's fine.
pkg/grpc/remote_authorizer.go
Outdated
| @@ -0,0 +1,171 @@ | |||
| package grpc | |||
There was a problem hiding this comment.
I understand why you put this code here, though I would argue that pkg/auth/remote_authorizer.go would be a better spot for this. Or if this ends up introducing cycles, add a pkg/auth/remote or something.
The idea is that pkg/auth contains all authorization related code that can be used irrespective of the kind of transport that was used to make the incoming connection. In our case we can use RemoteAuthorizer both with incoming gRPC and HTTP requests.
What we have in pkg/http and pkg/grpc is the authentication related code, which is quite protocol specific (due to it receiving metadata differently and returning different styles of errors).
There was a problem hiding this comment.
I did have a cycle in the beginning of the development, but apparently not any more.
|
|
||
| // RequestHeadersAuthenticator can be used to grant or deny access to a server | ||
| // based on request headers, typically from an HTTP or gRPC request. | ||
| type RequestHeadersAuthenticator interface { |
There was a problem hiding this comment.
I think this should live in pkg/auth, as it's not specific to any specific transport protocol, right? Just ones that "provide string key/value headers".
Prepare for remote authentication and authorization to avoid future curcular dependency.
The authenticate and authorize tasks can now be sent remotely over gRPC to an external service. This way, custom authentication and authorization does not require a modified builds of the Buildbarn components. To avoid spamming the remote service with calls for every REv2 request and keep the latency low, the verdicts, both allow and deny, are cached for a duration specified in the response from the remote service.
|
I've now rebased and fixed all the review comments. The changes were done in |
|
@EdSchouten friendly ping. Would you like me to click "Resolve conversation"? |
pkg/auth/remote_authenticator.go
Outdated
| func (ce *remoteAuthCacheEntry) IsReady() bool { | ||
| select { | ||
| case <-ce.ready: | ||
| return true | ||
| default: | ||
| return false | ||
| } | ||
| } | ||
|
|
||
| // IsValid returns false if a new remote request should be made. | ||
| func (ce *remoteAuthCacheEntry) IsValid(now time.Time) bool { | ||
| if ce.response == nil { | ||
| // Error response on the remote request, make a new request. | ||
| return false | ||
| } | ||
| return now.Before(ce.response.expirationTime) | ||
| } |
There was a problem hiding this comment.
Maybe move these methods above remoteAuthResponse, so it's more clear what type they belong to?
Also consider making them private, because they are not provided to satisfy a certain interface.
There was a problem hiding this comment.
Removed and created two maps.
pkg/auth/remote_authenticator.go
Outdated
| // NewRemoteAuthenticator creates a new RemoteAuthenticator for incoming | ||
| // requests that forwards headers to a remote service for authentication. The | ||
| // result from the remote service is cached. | ||
| func NewRemoteAuthenticator( |
There was a problem hiding this comment.
NewRemoteRequestHeadersAuthenticator() for consistency with the rest of the code base?
pkg/auth/remote_authenticator.go
Outdated
| for { | ||
| a.lock.Lock() | ||
| entry := a.getAndTouchCacheEntry(requestKey) | ||
| if entry == nil || (entry.IsReady() && !entry.IsValid(now)) { |
There was a problem hiding this comment.
Hmmm... I think this approach is slightly hard to reason about, because we're conflating two things:
- A map of in-flight requests, so that we can dedupe against those.
- A map of cached responses, so that we can elide the call altogether.
Right now we're all trying to do this at once through a single data structure, but that does mean that our bookkeeping of in-flight requests is also bound to the eviction policy of the eviction.Set.
What are your thoughts on having two separate maps?
There was a problem hiding this comment.
Created two maps, much better. Thank you.
| "google.golang.org/grpc/status" | ||
| ) | ||
|
|
||
| type remoteRequestAuthenticator struct { |
There was a problem hiding this comment.
What are your thoughts on calling this requestHeadersAuthenticator as well? Yes, it would be the same name as the underlying interface, but because it's in a different package (auth.RequestHeaderAuthenticator vs. http.NewRequestHeadersAuthenticator()) they would still be sufficiently distinct.
The reason I'm bringing this up is because there isn't really anything "remote" about this type. It would be perfectly fine to have an implementation of auth.RequestHeadersAuthenticator that looks at stuff in a SQLite database.
This also makes me wonder: should we reimplement our JWT auth to be built on top of this interface as well?
There was a problem hiding this comment.
This also makes me wonder: should we reimplement our JWT auth to be built on top of this interface as well?
Is that just to extract a single header into the JWT handler? Maybe I didn't get how you meant.
The authenticate and authorize tasks can now be sent remotely over gRPC to an external service. This way, custom authentication and authorization does not require a modified builds of the Buildbarn components.
To avoid spamming the remote service with calls for every REv2 request and keep the latency low, the verdicts, both allow and deny, are cached for a duration specified in the response from the remote service.