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

crypto/tls: add DialContextWithDialer #18482

Open
akalin-keybase opened this issue Dec 31, 2016 · 16 comments

Comments

@akalin-keybase
Copy link

@akalin-keybase akalin-keybase commented Dec 31, 2016

In go 1.7, Dialer now has DialContext, which takes a context.Context and uses it for expiration.

However, DialWithDialer just calls Dial on its given Dialer, and doesn't take a context.Context.

To be backwards compatible, probably need to provide a separate function, say, DialContextWithDialer, that is like DialWithDialer except it takes a context.Context and calls DialContext, and have DialWithDialer call DialContextWithDialer with context.Background().

It looks like as a workaround, callers can write their own DialContextWithDialer function and copy most of DialWithDialer.

@bradfitz

This comment has been minimized.

Copy link
Member

@bradfitz bradfitz commented Dec 31, 2016

The review period for Go 1.7 APIs ended 6 months ago.

The review period for Go 1.8 APIs ended approximately 3 weeks ago.

We can consider something new in crypto/tls for Go 1.9.

For now, you'll just have to dial first, and then use https://golang.org/pkg/crypto/tls/#Client to start the client handshake on the already-connected TCP (or whatever) Conn.

@bradfitz bradfitz changed the title [crypto/tls] DialWithDialer doesn't take a context argument crypto/tls: add API to Dial with a context? Dec 31, 2016
@bradfitz bradfitz added this to the Go1.9Maybe milestone Dec 31, 2016
@bradfitz bradfitz modified the milestones: Go1.10, Go1.9Maybe Jun 7, 2017
@rosenhouse

This comment has been minimized.

Copy link
Contributor

@rosenhouse rosenhouse commented Aug 17, 2017

I believe I have a use-case for this feature and would be curious if there's a suggested work-around.

We're trying to configure a http.Transport such that the client does extra validation of the server-provided certificate before sending any data. We want this client to compare server-provided certificate fields against values present on the req.Context. We see Transport.DialContext() but there's no Transport.DialTLSContext(), and setting a DialContext function that does TLS internally looks like it won't work.

Edit: perhaps I'm asking in the wrong issue -- this is bigger than just the crypto/tls package...

@gobwas

This comment has been minimized.

Copy link

@gobwas gobwas commented Nov 3, 2017

Hello! Sorry for "pinging", but will this issue be landed in 1.10? Milestone is 1.10 but no NeedsFix label... Im okay with the work-around, but it looks like little dirty when I need to copy logic of adding tls.Config.ServerName from tls.DialWithDialer().

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Nov 4, 2017

@gobwas There has been no definite decision to implement this, and there is no patch. At this point I would say that it is unlikely to be fixed for 1.10. Sorry.

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

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Feb 10, 2018

Change https://golang.org/cl/93255 mentions this issue: crypto/tls: add DialContextWithDialer function

@maja42

This comment has been minimized.

Copy link

@maja42 maja42 commented Jun 28, 2018

Is there currently a workaround for this issue?

@filipochnik

This comment has been minimized.

Copy link

@filipochnik filipochnik commented Jun 28, 2018

@maja42 As @akalin-keybase said

It looks like as a workaround, callers can write their own DialContextWithDialer function and copy most of DialWithDialer.

I submitted a patch implementing this a few months ago but I'm not sure if there's a decision to add this to the API.

@bradfitz is there a decision one way or the other?

@FiloSottile FiloSottile changed the title crypto/tls: add API to Dial with a context? crypto/tls: add DialContextWithDialer Sep 5, 2018
@bradfitz

This comment has been minimized.

Copy link
Member

@bradfitz bradfitz commented Oct 2, 2018

@FiloSottile, I'm going to remove the Proposal-Crypto review so this gets some attention during the proposal review meetings (which exclude Crypto).

This is more standard library API than it is crypto, and you also seem to be backlogged on other crypto stuff.

@bradfitz

This comment has been minimized.

Copy link
Member

@bradfitz bradfitz commented Oct 5, 2018

Counter proposal:

What if we add a new crypto/tls.Dialer type, with two fields:

// Dialer dials TLS connections given a configuration and a Dialer for the
// underlying connection. 
type Dialer struct {
     Config    *Config
     NetDialer *net.Dialer
}

Then the new Dialer can have Dial and DialContext methods with the normal signatures.

Thoughts?

@FiloSottile

This comment has been minimized.

Copy link
Member

@FiloSottile FiloSottile commented Oct 9, 2018

crypto/tls.Dialer LGTM, with deprecation of crypto/tls.DialWithDialer in favor of crypto/tls.Dialer.Dial.

If this is approved, I'll implement it in the TLS 1.3 dev cycle.

@akalin-keybase

This comment has been minimized.

Copy link
Author

@akalin-keybase akalin-keybase commented Oct 11, 2018

Works for me!

@jsha

This comment has been minimized.

Copy link

@jsha jsha commented Oct 29, 2018

Another possibility: What about adding a context.Context field on tls.Config? This has a few advantages I can see:

  • Doesn't require new methods in crypto/tls
  • Easier to plumb through from net/http, which has a TLSClientConfig field on Transport
  • Allows the context to be applied to handshaking, not just TCP dialing. In Boulder, we just found a connection leak when a remote host times out a handshake.
@bradfitz

This comment has been minimized.

Copy link
Member

@bradfitz bradfitz commented Oct 29, 2018

@jsha, that would violate the context rules of not storing them in long-lived or shared structures, and *tls.Config is generally both.

I don't understand your handshake concern: we already do the handshake under the provided timeout. That wouldn't change with the design proposed above. The handshake would still be under the context's deadline.

@jsha

This comment has been minimized.

Copy link

@jsha jsha commented Oct 29, 2018

@jsha, that would violate the context rules of not storing them in long-lived or shared structures, and *tls.Config is generally both.

Ah, good point. I was looking primarily at the conn.Client method, where *tls.Config is short-lived and non-shared. Is it accurate to say that *tls.Config is short-lived and non-shared when used by a client, but not when used by a server?

I don't understand your handshake concern: we already do the handshake under the provided timeout. That wouldn't change with the design proposed above. The handshake would still be under the context's deadline.

The Handshake concern applies only in code where you build your own TCP connection then hand it off to crypto/tls for the handshake:

netConn, _ := dialer.DialContext(ctx, "tcp", hostPort)
conn := tls.Client(netConn, config) 
conn.Handshake()

It's not strictly related to implementing DialContextWithDialer, but it's another place in crypto/tls that appears to need the context treatment, so I wanted to suggest an idea that might kill two birds with one stone. I'm also happy to file a separate issue about Handshake.

@jsha

This comment has been minimized.

Copy link

@jsha jsha commented Oct 29, 2018

Is it accurate to say that *tls.Config is short-lived and non-shared when used by a client, but not when used by a server?

Nevermind on this; I looked again and realized that yes, clients will typically reuse a *tls.Config many times, though it is not required by the API.

@jlaffaye

This comment has been minimized.

Copy link

@jlaffaye jlaffaye commented Apr 23, 2019

Any progress on this issue ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.