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

net/http: add DialTLSContext to Transport #21526

Closed
rosenhouse opened this issue Aug 18, 2017 · 16 comments

Comments

@rosenhouse
Copy link
Contributor

@rosenhouse rosenhouse commented Aug 18, 2017

I see that http.Transport has the following fields:

  • Dial (deprecated)
  • DialContext
  • DialTLS

I'd like to have a DialTLSContext

Would the maintainers be open to a contribution of this feature to http.Transport?

What version of Go are you using (go version)?

1.8

What operating system and processor architecture are you using (go env)?

linux & mac on amd64

What did you do?

I want to set the DialTLS behavior but have access to the Context of the http Request inside that function, e.g.

func main() {
	tr := http.Transport{
		DialTLSContext: func(ctx context.Context, network, addr string) (net.Conn, err) {
			conn, err := tls.Dial(network, addr, myConfig)
			if err != nil {
				return nil, err
			}
			connState := conn.ConnectionState()
			ok := extraValidation(connState, ctx)
			if !ok {
				return nil, errors.New("extra validation failed")
			}
			return conn
		},
	}
	req, _ := http.NewRequest("GET", "https://example.com", nil)
	tr.RoundTrip(req)
}

What did you expect to see?

the RoundTrip function should behave the same way as if I'd set the DialTLS function, while providing the context.Context to my custom validation function.

What did you see instead?

unknown field 'DialTLSContext' in struct literal of type http.Transport

Reference: related question on golang-nuts

@odeke-em odeke-em changed the title Adding DialTLSContext to http.Transport proposal: net/http: add DialTLSContext to Transport Aug 18, 2017
@gopherbot gopherbot added this to the Proposal milestone Aug 18, 2017
@gopherbot gopherbot added the Proposal label Aug 18, 2017
@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Aug 21, 2017

/cc @bradfitz

@rosenhouse

This comment has been minimized.

Copy link
Contributor Author

@rosenhouse rosenhouse commented Sep 4, 2017

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Sep 4, 2017

Change https://golang.org/cl/61291 mentions this issue: net/http: add DialTLSContext hook to Transport

@sabey

This comment has been minimized.

Copy link

@sabey sabey commented Oct 6, 2017

I'm trying to accomplish something similar to this proposed change and add a DialTLSContext to Transport. I assume your changes only work for http/1.0 requests? similar changes would have to be added to the http2 transport?

@usirsiwal

This comment has been minimized.

Copy link

@usirsiwal usirsiwal commented Feb 9, 2018

We have a very similar need. We are developing a proxy in go and will like the upstream TLS connection to be canceled when the client connection is closed.

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Mar 5, 2018

/cc @bradfitz

@bradfitz

This comment has been minimized.

Copy link
Member

@bradfitz bradfitz commented Apr 2, 2018

I suppose we could, but we already have so many dial hooks there. I guess one more won't kill us.

But I'll also note that tls.Conn has no context support during its Handshake, so the context isn't as useful as it could be. We could fake it with some net.Conn.SetDeadline calls, but the net.Conn interface has no way to get the old read/write deadlines, so we can't restore the deadlines to whatever they might've been either.

/cc @FiloSottile

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Apr 2, 2018

It's unclear what this is useful for, since TLS doesn't support context enough. What does extraValidation do in your example? Where does the context come from?

@sabey

This comment has been minimized.

Copy link

@sabey sabey commented Apr 2, 2018

@bradfitz @rsc One of my use cases is that I am using InsecureSkipVerify = false so that after the Handshake I can implement my own VerifyPeerCertificate with the connection context. That might be similar to what @rosenhouse is doing.

However, my main use case and what I believe the real problem is is that I am implementing my own underlying Dial functionality and I am unable provide context to it when the HTTP Client uses DialTLS.

This probably isn't the proper go way of doing things?, but I personally use it and wouldn't mind the added context if possible since there is a DialContext function.

edit Thinking about this some more, I think it is the right choice especially if you are using Transport and DisableKeepAlives = false - It would then make more sense for each connection to then have their own context instead of worrying about different requests sharing an unknown context.

@rosenhouse

This comment has been minimized.

Copy link
Contributor Author

@rosenhouse rosenhouse commented Apr 3, 2018

Yeah, our use case sounds like @sabey's

The http client would make a request with some extra data attached to its context. The http transport's DialTLSContext callback could use that data to further validate the server certificate.

This wasn't illustrated very well in the original post. Let me try again...

tr := http.Transport{
  DialTLSContext: func(ctx context.Context, network, addr string) (net.Conn, err) {
    conn, err := tls.Dial(network, addr, myConfig)
    if err != nil {
      return nil, err
    }
    if !validation.Check(ctx, conn.ConnectionState()) {
      return nil, errors.New("extra validation failed")
    }
    return conn, nil
  },
}
req, _ := http.NewRequest("GET", "https://example.com", nil)
req = req.WithContext(validation.NewContext(req.Context(), expectedCertData))
tr.RoundTrip(req)

Then validation.Check would compare the server certificate in the connection state with the expectedCertData attached to the request.

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Apr 9, 2018

Per discussions with @bradfitz and proposal review, we don't think this is terribly useful by itself, but it does seem like maybe having only 3 of the 4 combinations is odd. Someone can look into doing this. If it ends up causing a lot of pain, though, we might rethink. But for now, accepted.

@rsc rsc changed the title proposal: net/http: add DialTLSContext to Transport net/http: add DialTLSContext to Transport Apr 9, 2018
@rsc rsc modified the milestones: Proposal, Unplanned Apr 9, 2018
@rosenhouse

This comment has been minimized.

Copy link
Contributor Author

@rosenhouse rosenhouse commented Apr 9, 2018

we don't think this is terribly useful by itself

@rsc could you elaborate? Are there other changes necessary to address the described use case?

@hanbang-wang

This comment has been minimized.

Copy link

@hanbang-wang hanbang-wang commented Jan 4, 2019

Any follow-up? I come across the need for this function now, and it is strange to see that we have deprecated Dial but no deprecated DialTLS. My current works-all-fine solution is to manually patch the src file, but it would be great to see this 2017-issue to be fixed in early 2019.

@juliandroid

This comment has been minimized.

Copy link

@juliandroid juliandroid commented May 16, 2019

Is there a workaround of this issue? I.e. to use context that can be canceled and still do a https requests?

@quite

This comment has been minimized.

Copy link

@quite quite commented Nov 11, 2019

As somehow who ran across the need for this (the peer verification use-case), DialTLSContext() would indeed make the API feel more complete :) The patch is getting stale, and a rebase on master is slowly becoming more and more involved... Are you up for it @rosenhouse ?

@rosenhouse

This comment has been minimized.

Copy link
Contributor Author

@rosenhouse rosenhouse commented Nov 11, 2019

Sorry but I don't have the bandwidth at the moment. Someone else want to run with it?

@gopherbot gopherbot closed this in eb55a0c Nov 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
10 participants
You can’t perform that action at this time.