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

net/http: support way to modify Request context data from net.Conn etc #20956

Open
Freeaqingme opened this Issue Jul 8, 2017 · 8 comments

Comments

Projects
None yet
4 participants
@Freeaqingme

Freeaqingme commented Jul 8, 2017

Hi,

I've got a connection that implements the net.Conn interface. It's a custom implementation that implements the proxyprotocol v2. Part of the protocol is that the header, besides original remote address can contain additional metadata about its original form (e.g. if ssl was used, what ciphers, etc. But also things like network namespace).

I'm using the standard net/http tooling, and it provides no direct access to the underlying net.conn. That's been discussed in multiple issues, so don't want to spark that discussion yet again. I am however looking for a way to access this metadata.

The only option I'm aware of that currently exists is to Hijack() the response. That's suboptimal, however, because the reason I'm using net/http is so I don't have to implement HTTP myself :) (and there's no way to Unhijack() the connection again).

Unless there already is a workaround available that I'm not aware of, please do consider this a feature request for a way of transferring data from the underlying net.Conn to the http.Request object.

Thanks,

Dolf

  • Freeaqingme

//cc @bradfitz

@bradfitz

This comment has been minimized.

Member

bradfitz commented Jul 14, 2017

There's kinda a way for you to do it already. It's either gross or clever, but I'll leave that judgement up to you:

The net/http server set the Request.Context() context values http.LocalAddrContextKey and http.ServerContextKey. The LocalAddrContextKey one is set like:

func (c *conn) serve(ctx context.Context) {
        c.remoteAddr = c.rwc.RemoteAddr().String()
        ctx = context.WithValue(ctx, LocalAddrContextKey, c.rwc.LocalAddr())

Note that you control what c.rwc (your net.Conn impl) returns, and LocalAddr returns a net.Addr, an interface type. So as long as you implement Addr faithfully, you can also implement other stuff.

Related issue #16220 was about letting people adjust the base context but it fizzled out due to lack of real need.

Perhaps we could have an optional http.Server hook to let people modify the Request.Context or append context values before the Handler runs, with the hook getting the net.Conn, etc.

That would require some thought and design. I'll repurpose this bug about that if you don't mind.

/cc @tombergan

@bradfitz bradfitz changed the title from net/http: Support passing data from net.Conn to http.Request to net/http: support way to modify Request context data from net.Conn etc Jul 14, 2017

@bradfitz bradfitz added this to the Go1.10 milestone Jul 14, 2017

@tombergan

This comment has been minimized.

Contributor

tombergan commented Jul 15, 2017

Related issue #16220 was about letting people adjust the base context but it fizzled out due to lack of real need.

#18997 is going to add that support. But I don't think that well help in this example, because the hook doesn't get the net.Conn. Do you think it should actually get the net.Conn? I know we've resisted adding references to net.Conn in the past.

@bradfitz

This comment has been minimized.

Member

bradfitz commented Jul 15, 2017

@tombergan, the desire to get at "the" conn never goes away, so I think it's time, with sufficiently scary names and docs. For instance, for TLS connections, do we give them both the raw connection and the TLS wrapper conn? With what names? RawEncryptedConn net.Conn sounds nice and scary for the low-level one.

   // RawEncryptedConn is the bottommost net.Conn, under the TLS connection if any.
   // It's usually of type *net.TCPConn, unless you have a weird setup.
   // You must not assume you can read or write from it, since it's likely wrapped in TLS,
   // and you don't know which HTTP protocol you're speaking. Oh, you also shouldn't
   // Close it if you don't want to break multiple requests in flight. We make no promises
   // about what will happen with this if you Read, Write, or Close it, and we reserve
   // the right to break your code if you do so. This is provided so you can get at things
   // you need to get at. We trust you to make good life decisions.
   // Enjoy.
   RawEncryptedConn net.Conn

Or something.

@Freeaqingme

This comment has been minimized.

Freeaqingme commented Oct 28, 2017

@bradfitz My suggestion would be to call it RawConn or something, so as to not make it specifically about encrypted connections. Besides that, I do like your comment :)

How can we get this ball rolling again?

@bradfitz

This comment has been minimized.

Member

bradfitz commented Nov 1, 2017

I just got back to work today after some months of paternity leave. Not coincidentally, today is also the Go 1.10 feature freeze date, so this will need to be pumped to Go 1.11.

@bradfitz bradfitz modified the milestones: Go1.10, Go1.11 Nov 1, 2017

@odeke-em

This comment has been minimized.

Member

odeke-em commented Jun 22, 2018

@Freeaqingme if you are interested/available, would you like to spin up a CL to get the ball rolling? The usage and tests will give folks a palatable point to start with. /cc @meirf @rs just in case y'all need this functionality too or are interested.

@bradfitz bradfitz modified the milestones: Go1.11, Unplanned Jun 22, 2018

@bradfitz

This comment has been minimized.

Member

bradfitz commented Jun 22, 2018

@Freeaqingme, was the hacky workaround I suggested in #20956 (comment) workable?

That's not to say we can't do more here, if somebody wants to pick it up.

@Freeaqingme

This comment has been minimized.

Freeaqingme commented Jun 27, 2018

This kinda fell off my radar swamped by other projects. Though there shouldn't be a reason for your suggestion not to work.

I'll see if I have some this weekend to gobble up a CL with a method to more conveniently retrieve the underlying conn (which is not necessarily encrypted but can still get a very scary name)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment