-
Notifications
You must be signed in to change notification settings - Fork 107
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
document in the FAQ how client and server might access authenticated identity of the remote #380
Comments
I take it back about an interceptor not being able to set the Peer. The interceptor uses interfaces to model requests, responses, and streams, so it could provide its own implementation that overrides the |
On the client side, users can already use On the client side, we have access to a I do see more demand for server-side authentication. That said, I'm not sure what we get from a gRPC-style API. The gRPC I'm open to other ideas, but I'm not seeing a path forwards that's clearly better than the status quo. Unless we can come up with something compelling, I'd prefer to document the status quo and wait. |
@akshayjshah, thanks for thorough reply! That all seems reasonable. I am perhaps too much in the mindset of how things are done in gRPC, forgetting that Connect just uses Though I do wonder what the value of Since the use of TLS (and client-cert authn with TLS) is available without any other packages or custom middleware/interceptors, it seems awkward that there is no way to access TLS results without resorting to custom trace callbacks or custom middleware. But I concede that creating surface area to model custom authn protocols and results is fair to be out of bounds.
Speaking of documenting the status quo, I think this question (how to retrieve authenticated peer, from client or server) is likely a good candidate for the FAQ on connect.build. |
Personally, I'm of the opinion all of these things can be done in a third party (or first party) module that is provided through interceptors. Similar to the popular grpc-middleware module. I think pairing interceptors with context.Value's can implement all of this functionality without embedding it in core Connect imho. As a user of Connect, one of the appeals is how little surface area and how simple Connect is compare to gRPC. So I think trying to mirror all gRPC concepts brings us back to basically using gRPC. |
@jhump while you're right that net/http.Request doesn't have RemoteAddr, I've adopted a pattern of leveraging http.Server.ConnContext and binding it to the base context. This makes sense since a single connection might make multiple requests. So I've started binding my own ConnContext struct and a RequestContext struct in middleware. Then anything downstream has access to the raw net.Conn and the http.Request. This pattern opens the door pretty wide to every bit of information needed. I'm happy to put together some code samples this weekend if you're curious. |
Actually the
I have to retract this statement. While the interceptor technically could provide its own implementation of |
My 0.02c: While we should do the actual authentication flow outside of Connect ideally, we need some way to surface the authenticated identity to Connect handlers. For example if I want to construct an authorizer for the user principal in order to authorize the RPC, or if I want to set a I also think that service identity and user identity are different forms of identity, and they look different based on the application's requirements, so I agree that Connect shouldn't necessarily try to model this. Not wanting to get too much into implementation, but something open-ended like this (very rough sketch) comes to mind. // AuthInfoProvider does not perform an authentication challenge, it assumes that a challenge
// has already occurred higher level; it merely extracts the provided authentication information
// from the incoming request.
type AuthInfoProvider func(req http.Request, conn net.Conn, /* etc... */) (interface{}, error)
type Peer interface {
AuthInfo() interface{}
} With this, handlers can type guard |
@jhump you're right, I misspoke about I still think it'd be worthwhile to put together some community driver interceptors that provide these helpers. The peer info is a good case imo for an interceptor that's outside of the core here. Granted, I have 0 skin in the game here other than a heavy user of connect-go, but I don't personally see why any of this is necessary in core. JWT, mTLS, all of this can be done through interceptors and |
I create a PR #422 for one simple way (just expose request so we can get request.TLS) to handle this scenario. |
Sorry for the confusion, @ImSingee - this issue hasn't reached enough consensus for PRs.
I think you pretty much nailed it, @jhump - it makes the domain & port used in the URL accessible to client-side interceptors, which makes it feasible to include logs, traces, metrics, etc. I think this is fairly common, b/c the domain + port are useful while remaining relatively low-cardinality. The OpenTelemetry spec agrees, and encourages clients to prefer domain to IP address. This info is definitely less useful for mutual authentication, though.
@saquibmian I'd really like to keep Connect as focused on RPC as possible, deferring generic HTTP concerns to the stdlib or other packages. A big portion of my dissatisfaction with grpc-go is that it's enormous. Beyond the simple fact that different applications will want to model authn/authz with different types, I can see a few problems with supporting this in
There are reasonable ways around this that don't require changes to this package. @mattrobenolt likes |
As an update, I've plowed some more time into my standalone authentication package this weekend. It's now In particular, the same If this approach seems reasonable, we can move it into a proper bufbuild repo. |
The canonical answer to this is now to use |
The
Peer
object only provides an address. For clients, it just echos back the host that was used in the HTTP request, without returning anything about the remote server's identity. It could at least return the resolved IP, to provide more information when multiple A records are available.But it also provides no way to get the authenticated identity, when using TLS. This is easily available in the response object returned from the
http.Client
or the request object provided to thehttp.Handler
.The gRPC version of this type has a generic
AuthInfo
field with an interface type, and users can then try to type assert to a specific implementation. The idea is that different authn mechanisms might be used to authenticate parties (like JWTs or other custom auth cookies for client authn), so the representation needs to be flexible enough so an interceptor could provide the identity (instead of hard-coding the representation used in mutually-authenticated TLS). Speaking of which, there is not a way for an authenticating interceptor to override the peer since there is no exported setter (or constructor which allows setting it).The text was updated successfully, but these errors were encountered: