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

Dialer: add optional method NetDialTLSContext #746

Merged
merged 1 commit into from Jan 4, 2022

Conversation

lluiscampos
Copy link
Contributor

@lluiscampos lluiscampos commented Dec 10, 2021

Fixes issue: #745

With the previous interface, NetDial and NetDialContext were used for
both TLS and non-TLS TCP connections, and afterwards TLSClientConfig was
used to do the TLS handshake.

While this API works for most cases, it prevents from using more advance
authentication methods during the TLS handshake, as this is out of the
control of the user.

This commits introduces another a new dial method, NetDialTLSContext,
which is used when dialing for TLS/TCP. The code then assumes that the
handshake is done there and TLSClientConfig is not used.

This API change is fully backwards compatible and it better aligns with
net/http.Transport API, which has these two dial flavors. See:
https://pkg.go.dev/net/http#Transport

@lluiscampos
Copy link
Contributor Author

The first commit is unrelated to the issue at hand, but otherwise the CI was failing on gofmt for latest Go version 1.17.

lluiscampos added a commit to lluiscampos/mender that referenced this pull request Dec 10, 2021
By switching to the "enhanced" API for websocket.Dialer from
mendersoftware's fork.

There is a limitation in current gorilla/websocket.Dialer API in that
the user cannot specify a dial method for TLS/TCP connections. The TLS
handshake is always done by the library based on user's TLSClientConfig,
but that is not enough for Mender as we need it to be done via OpenSSL
(aka our dial wrapper for TLS) so that advance auth features like
getting the keys from HSM.

This commit switches to mendersoftware's fork and modifies the code
accordingly (one line change!).

The patch has been submitted upstream. See:
* gorilla/websocket#745
* gorilla/websocket#746

Changelog: None
No changelog, commit 84204a3 claims to support websockets, this commit
just fixes a bug there which has not been released.

Signed-off-by: Lluis Campos <lluis.campos@northern.tech>
@garyburd
Copy link
Contributor

garyburd commented Jan 1, 2022

  • Remove DialTLS field. The corresponding field in net/http Transport is deprecated, so there's no need to add it here.
  • Convert map of tests to slice of tests for consistency with other tests.
	tests := []struct {
                name              string 
		server            *httptest.Server // server to use
		netDial           func(network, addr string) (net.Conn, error)
		netDialContext    func(ctx context.Context, network, addr string) (net.Conn, error)
		netDialTLS        func(network, addr string) (net.Conn, error)
		netDialTLSContext func(ctx context.Context, network, addr string) (net.Conn, error)
		tlsClientConfig   *tls.Config
	}{
		 name: "HTTP server, all NetDial* defined, shall use NetDialContext",
	         server: server,
                 …
		

Fixes issue: gorilla#745

With the previous interface, NetDial and NetDialContext were used for
both TLS and non-TLS TCP connections, and afterwards TLSClientConfig was
used to do the TLS handshake.

While this API works for most cases, it prevents from using more advance
authentication methods during the TLS handshake, as this is out of the
control of the user.

This commits introduces another a new dial method, NetDialTLSContext,
which is used when dialing for TLS/TCP. The code then assumes that the
handshake is done there and TLSClientConfig is not used.

This API change is fully backwards compatible and it better aligns with
net/http.Transport API, which has these two dial flavors. See:
https://pkg.go.dev/net/http#Transport

Signed-off-by: Lluis Campos <lluis.campos@northern.tech>
@lluiscampos
Copy link
Contributor Author

Thanks @garyburd for your feedback. I updated now the pull request following your suggestion. I updated the commit message accordingly too.

  • Remove DialTLS field. The corresponding field in net/http Transport is deprecated, so there's no need to add it here.

As Dialer already had Dial and DialContext I thought it was good to add the two... but being this a new field, makes sense not to add the deprecated one.

* Convert map of tests to slice of tests for consistency with other tests.

Roger that 👍

@lluiscampos lluiscampos changed the title Dialer: add optional methods NetDialTLS and NetDialTLSContext Dialer: add optional method NetDialTLSContext Jan 3, 2022
@garyburd garyburd merged commit 9111bb8 into gorilla:master Jan 4, 2022
@garyburd
Copy link
Contributor

garyburd commented Jan 4, 2022

Thank you for your contribution. 👍 for the included test.

lluiscampos added a commit to lluiscampos/mender that referenced this pull request Jan 10, 2022
Now that upstream have merged our PR. Slightly modify our code
accordingly.

See:
* gorilla/websocket#745
* gorilla/websocket#746

Changelog: None

Signed-off-by: Lluis Campos <lluis.campos@northern.tech>
lluiscampos added a commit to lluiscampos/mender that referenced this pull request Jan 10, 2022
Now that upstream have merged our PR. Slightly modify our code
accordingly.

See:
* gorilla/websocket#745
* gorilla/websocket#746

Changelog: None

Signed-off-by: Lluis Campos <lluis.campos@northern.tech>
@lluiscampos lluiscampos deleted the issue-745-net-dial-tls branch October 21, 2022 17:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants