Update cookie auth check to no longer use axios/k8s api server #4045
Conversation
| // Finally, the k8s api server nowadays defaults to allowing anonymous | ||
| // requests, so that rather than returning a 401, a 403 is returned if | ||
| // RBAC does not allow the anonymous user access. An http-only cookie | ||
| // will not result in an anonymous request, so... | ||
| return !this.isAnonymous(response); |
There was a problem hiding this comment.
I think this assumption that I'd made a year ago is actually incorrect, in that the K8s API server will never receive an anonymous request when the auth proxy is being used, since the auth proxy itself redirects to login when unauthenticated. Hence simply removing this condition. It would be required if this function was called when not using the auth proxy, but this is exactly what I fixed (in the LoginForm) in the previous PR, so that it's only used when config.authProxyEnabled is true.
dashboard/src/shared/Auth.ts
Outdated
| // from the grpc-web APIs server, rather than the auth proxy. | ||
| public static is403FromAPIsServer(e: any): boolean { | ||
| const contentType = e.metadata?.headersMap["content-type"] as string[]; | ||
| if (contentType.some(v => v === "application/grpc-web+proto")) { |
There was a problem hiding this comment.
is it always the case that if the exception contains this content type is a 403?
There was a problem hiding this comment.
Just FTR, the grpc-gw fn to determine which is a grpcwerb request is:
func (w *WrappedGrpcServer) IsGrpcWebRequest(req *http.Request) bool {
return req.Method == http.MethodPost && strings.HasPrefix(req.Header.Get("content-type"), "application/grpc-web")
}so perhaps, instead of === we could check the prefix to cover all the possible cases of the grpc-web protocol, namely:
- application/grpc-web+proto
- application/grpc-web+json
- application/grpc-web+thrift
- application/grpc-web-text+proto
- application/grpc-web-text+thrift
There was a problem hiding this comment.
It's not an assumption that if the exception contains this content type, that it is a 403 error. The function itself (is403FromAPIsServer) assumes that the argument is a 403 already, so is checking the content type of the given 403 error. Though it doesn't need to assume that, I suppose?
Maybe that's what you mean? We could (and perhaps should) rename this function as isErrorFromAPIsServer(), since the fact that it's currently being used to check 403 errors only isn't relevant to the function itself? I'll update to that.
There was a problem hiding this comment.
so perhaps, instead of
===we could check the prefix to cover all the possible cases of the grpc-web protocol,
Yep, makes sense, will update to that. Thanks.
There was a problem hiding this comment.
Yeah, that was my question. Thanks for the name change!
Signed-off-by: Michael Nelson <minelson@vmware.com>
Signed-off-by: Michael Nelson <minelson@vmware.com>
absoludity
force-pushed
the
3896-auth-check-2
branch
from
6535efb
to
d0fdef2
Compare
Signed-off-by: Michael Nelson minelson@vmware.com
Description of the change
Follows on from #4043, this time updating the cookie-based authentication so that it no longer uses a request to the k8s API server to determine the authentication status.
I'd originally planned to remove axios from
shared/Auth(with theisAnonymousandis403FromAuthProxyfunctions), but then found they are still used inshared/AxiosInstancewhich is still used by the operator support (which will continue for now to query the k8s API directly).As with the previous PR the pain here was testing IRL locally the various possibilities to find the best solution.
Benefits
Hopefully no more requests to the k8s api server, outside of the operator support (though I'll do a proper audit and test tomorrow).
Possible drawbacks
Applicable issues
Additional information