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

[feature] Dialer: Support for custom NetDial for TLS #745

Closed
lluiscampos opened this issue Dec 9, 2021 · 1 comment
Closed

[feature] Dialer: Support for custom NetDial for TLS #745

lluiscampos opened this issue Dec 9, 2021 · 1 comment

Comments

@lluiscampos
Copy link
Contributor

lluiscampos commented Dec 9, 2021

Is your feature request related to a problem? Please describe.

A clear and concise description of what the problem is - e.g. "I'm always frustrated when [...]"

On the Dialer, I need to use a custom NetDial method for TLS connections to be able to use some advance authentication methods like Mutual TLS using a client key from a Hardware Security Module (HSM). I have some go code wrapping OpenSSL that does the TLS dialing, including the handshake.

This works fine for regular https requests by setting this dial function as DialTLS for net/http.Transport.

The problem in gorilla/websocket is that the current Dialer interface only supports setting one pair of dial methods (NetDial and NetDialContext) which is used both for http and https connections. Later, for https a handshake is performed using TLSClientConfig, but in my case tls.Config fields are not enough to do the handshake, code here.

Describe the solution you'd like

What would the feature look like? How would it work? How would it change the API?

My suggestion is to have two pair of dial functions, NetDial + NetDialContext, and NetDialTLS + NetDialTLSContext. The latter pair would be used on wss/https connections. To avoid unexpected behavior when setting TLSClientConfig, if either NetDialTLS or NetDialTLSContext are set the code would assume that the TLS handshake has been performed there and TLSClientConfig can be ignored.

This API change would be completely backwards compatible, and it will better mimic the API of net/http.Transport (doc here).

I do have a working code for exact this API change that I am suggesting. I want to add some tests before submitting a PR. In the meanwhile, any feedback is welcome 😃

Describe alternatives you've considered

Are there alternatives you've tried, and/or workarounds in-place?

My workaround was to force the url to passed to Dial be ws, so that this specific block is skip... However this only works on certain circumstances and it is very hacky.

lluiscampos added a commit to lluiscampos/websocket that referenced this issue Dec 10, 2021
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 pair of dial methods, NetDialTLS and
NetDialTLSContext which are 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 four dial flavors. See:
https://pkg.go.dev/net/http#Transport

Signed-off-by: Lluis Campos <lluis.campos@northern.tech>
lluiscampos added a commit to lluiscampos/websocket that referenced this issue Dec 10, 2021
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 pair of dial methods, NetDialTLS and
NetDialTLSContext which are 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 four dial flavors. See:
https://pkg.go.dev/net/http#Transport

Signed-off-by: Lluis Campos <lluis.campos@northern.tech>
lluiscampos added a commit to lluiscampos/mender that referenced this issue 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>
lluiscampos added a commit to lluiscampos/websocket that referenced this issue Jan 3, 2022
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

Describe the solution you'd like

What would the feature look like? How would it work? How would it change the API?

My suggestion is to have two pair of dial functions, NetDial + NetDialContext, and NetDialTLS + NetDialTLSContext. The latter pair would be used on wss/https connections. (...)

Based on feedback from the pull request, the resulting change will only add NetDialTLSContext method, dropping NetDialTLS as it is deprecated in net/http.Transport. See #746 (comment)

garyburd pushed a commit that referenced this issue Jan 4, 2022
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

Signed-off-by: Lluis Campos <lluis.campos@northern.tech>
@garyburd garyburd closed this as completed Jan 4, 2022
lluiscampos added a commit to lluiscampos/mender that referenced this issue 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 issue 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants