Skip to content
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

proposal: attach remote address to context in HTTP3 request #4198

Closed
oncilla opened this issue Dec 12, 2023 · 10 comments · Fixed by #4208
Closed

proposal: attach remote address to context in HTTP3 request #4198

oncilla opened this issue Dec 12, 2023 · 10 comments · Fixed by #4208

Comments

@oncilla
Copy link
Contributor

oncilla commented Dec 12, 2023

Hi there,

We are trying to move our control plane from gRPC to connect-go/HTTP3 (see scionproto/scion#4434).
We rely on a feature of gRPC that lets us access the peer address when handling requests.

This is currently not possible with HTTP3, because the remote address is only passed as a regular string on the HTTP request of the stdlib type.

By attaching the remote address to the HTTP request context, we would solve our use case. But I also believe that it is more generally useful for implementing server code.

Enabling this can be done with a very minimal patch. If you would be interested, I will create a PR to upstrem it:

diff --git a/http3/server.go b/http3/server.go
index ac2e32a6..a4c4e327 100644
--- a/http3/server.go
+++ b/http3/server.go
@@ -115,6 +115,8 @@ func (k *contextKey) String() string { return "quic-go/http3 context value " + k
 // type *http3.Server.
 var ServerContextKey = &contextKey{"http3-server"}
 
+var RemoteAddrContextKey = &contextKey{"remote-addr"}
+
 type requestError struct {
 	err       error
 	streamErr ErrCode
@@ -597,6 +599,7 @@ func (s *Server) handleRequest(conn quic.Connection, str quic.Stream, decoder *q
 	ctx := str.Context()
 	ctx = context.WithValue(ctx, ServerContextKey, s)
 	ctx = context.WithValue(ctx, http.LocalAddrContextKey, conn.LocalAddr())
+	ctx = context.WithValue(ctx, RemoteAddrContextKey, conn.RemoteAddr())
 	req = req.WithContext(ctx)
 	r := newResponseWriter(str, conn, s.logger)
 	if req.Method == http.MethodHead {
@marten-seemann
Copy link
Member

Hi @oncilla!

Not opposed to your change, but I have two questions first:

  1. Why can't you use the address from the http.Request.RemoteAddr?
  2. Is there any precedent for using a context key? How does this work on HTTP/2?

@oncilla
Copy link
Contributor Author

oncilla commented Dec 12, 2023

Hi @marten-seemann

Thanks for even considering this proposal.

1. Why can't you use the address from the http.Request.RemoteAddr?

Our addresses cannot easily be represented in the http.Request.RemoteAddr because it is not simply an IP:port.
We have additional data that is part of the address (ISD-AS number, data plane path) that does not fit IP:port.

Of course, that is very specific to our use case, but I think as soon as an address is not just IP:port it is not possible with the current mechanism.

2. Is there any precedent for using a context key?

There are two similar cases that I would consider related and set somewhat of a precedence:

  1. The http.LocalAddrContextKey from the standard library already contains the local address:
	// LocalAddrContextKey is a context key. It can be used in
	// HTTP handlers with Context.Value to access the local
	// address the connection arrived on.
	// The associated value will be of type net.Addr.
	LocalAddrContextKey = &contextKey{"local-addr"}

This already allows the http handler implementations to extract the local address from the request context.
It feels natural to use the same mechanism to also expose the remote address to the http handler.

  1. The peer.FromContext function in grpc-go
    supports extracting the peer information (which contains the remote address).
type Peer struct {
	// Addr is the peer address.
	Addr [net](https://pkg.go.dev/net).[Addr](https://pkg.go.dev/net#Addr)
	// LocalAddr is the local address.
	LocalAddr [net](https://pkg.go.dev/net).[Addr](https://pkg.go.dev/net#Addr)
	// AuthInfo is the authentication information of the transport.
	// It is nil if there is no transport security being used.
	AuthInfo [credentials](https://pkg.go.dev/google.golang.org/[email protected]/credentials).[AuthInfo](https://pkg.go.dev/google.golang.org/[email protected]/credentials#AuthInfo)
}

gRPC handlers have full access to the remote address through the context.

How does this work on HTTP/2?

So far, we have only used HTTP/2 in combination with grpc-go, where this feature is available.

I have not investigated how to do this with http2 of the stdlib in too much detail. From a brief skim, it looks like we would need to patch it too to achieve this.

@marten-seemann
Copy link
Member

Thank you for your detailed explanation!

Our addresses cannot easily be represented in the http.Request.RemoteAddr because it is not simply an IP:port.
We have additional data that is part of the address (ISD-AS number, data plane path) that does not fit IP:port.

Does it need to though? The server just sets the string to whatever the quic.Connection returns:

req.RemoteAddr = conn.RemoteAddr().String()

If you're using a custom net.PacketConn (which I assume you're doing, otherwise you would just get an IP:port), that should allow you to get exactly the information that you need, doesn't it?

@oncilla
Copy link
Contributor Author

oncilla commented Dec 13, 2023

If you're using a custom net.PacketConn (which I assume you're doing, otherwise you would just get an IP:port), that should allow you to get exactly the information that you need, doesn't it?

We are using a custom net.PacketConn, exactly.

I'm thinking about this in the context of SCION.

There, we have a SCION address, which looks something like "[1-ff00:0:110,2001:db8::1]:443".
But on a connection, we also have to include the data plane path that the packet traverses on the network, which essentially is a big blob of bytes. On the server side, I want to be able to do some actions based on the dataplane path that the packet took.

Now, we could encode the dataplane path in the string output. But that would negatively affect all the other places where the string representation is used. (E.g., in logging output).

Probably we could use other techniques, like a singleton that keeps track of the real remote address.
But attaching it to the context feels cleaner.

I realize that this is somewhat specific to our use case, but I had the suspicion that having the real remote address attached to the context would also be useful to other project.

@marten-seemann
Copy link
Member

How would it be attached to the context though? I assume the way we'd implement this would be equivalent to what we do right now for the http.Request.RemoteAddr, so that wouldn't solve the problem that you need to encode the SCION address somehow.

@oncilla
Copy link
Contributor Author

oncilla commented Dec 13, 2023

The diff in the initial post is all that is required for it to work (we have a patched version of quic-go that works). The remote address has all the required things and nothing needs to be encoded additionally.

@marten-seemann
Copy link
Member

I'm confused now. If the remote quic.Connection.RemoteAddr has all that you need, why can you use it when it's saved as a context key and not when the exact same value is saved on the http.Request.RemoteAddr?

@oncilla
Copy link
Contributor Author

oncilla commented Dec 13, 2023

In quic.Connection.RemoteAddr, we have net.Addr which is an interface. Inside of the interface is an address snet.UDPAddr with all the data. If we pass that to the context, no data is lost.

In http.Request.RemoteAddr, we have a string, which contains the string representation of the net.Addr of quic.Connection.RemoteAddr. Here, we have data loss.

@marten-seemann
Copy link
Member

Oh right, obviously! Sorry for missing the obvious! I'm wondering why http.Request.RemoteAddr is a string in the first place...

Your proposal sounds reasonable to me. Would you mind sending a PR? Please make sure to add a comment explaining why you can't use the field on the request (i.e. string vs net.Addr).

oncilla added a commit to oncilla/quic-go that referenced this issue Dec 14, 2023
Add the remote address of the underlying packet connection to the
HTTP request context. This is useful for applications that need access
to the actual remote address (wrapped in a net.Addr) rather than just
its string representation.

Fixes quic-go#4198
@oncilla
Copy link
Contributor Author

oncilla commented Dec 14, 2023

@marten-seemann

I'm wondering why http.Request.RemoteAddr is a string in the first place...

Me too :)

Your proposal sounds reasonable to me. Would you mind sending a PR? Please make sure to add a comment explaining why you can't use the field on the request (i.e. string vs net.Addr).

Sweet. PR is out.

Thanks for the swift feedback on this proposal 👍

marten-seemann pushed a commit that referenced this issue Dec 16, 2023
* http3: add remote address to request context

Add the remote address of the underlying packet connection to the
HTTP request context. This is useful for applications that need access
to the actual remote address (wrapped in a net.Addr) rather than just
its string representation.

Fixes #4198

* add an integration test to the self test suite.

I was not sure how deep we want to go to assure the right value is set.
For now, it asserts that a net.Addr is present in the context.

Due to the dynamic nature of the requests, it is a bit harder to know
exactly how the remote address will look like. IPv4 vs IPv6, random high
port. I think it is fine to only assert that the value is present.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants