Feat/http connect proxy support #497
Conversation
fix: Add SNI for TLS connections if one is not set Signed-off-by: amir-khassaia <amir.khassaia@gmail.com>
Signed-off-by: amir-khassaia <amir.khassaia@gmail.com>
http_proxy.go
Outdated
| @@ -0,0 +1,84 @@ | |||
| package mqtt | |||
There was a problem hiding this comment.
Not sure if it's best to include this (httpProxy) in the library or get the user to do it in their code? (it does not look there is anything stopping them). Doing it here complicates changes if the user has specific requirements not met by this code; perhaps it would be better to create a proxy example that includes this?
There was a problem hiding this comment.
No, nothing stopping them its just going to be more code to maintain by each consumer - however I can see the argument for this being not the responsibility of the library, in which case an example might suffice too.
netconn.go
Outdated
| @@ -77,7 +77,7 @@ func openConnection(uri *url.URL, tlsc *tls.Config, timeout time.Duration, heade | |||
| return nil, err | |||
| } | |||
|
|
|||
| tlsConn := tls.Client(conn, tlsc) | |||
| tlsConn := tls.Client(conn, tlsConfigWithSni(uri, tlsc)) | |||
There was a problem hiding this comment.
Fine with this - I don't think it will break existing code. However I am going to have a think about the approach because I wonder if using a callback (say func OnConnectionAttempt(broker *url.URL, tlsCfg *tls.Config) *tls.Config) might provide more flexibility?
There was a problem hiding this comment.
Didn't see a reference to this callback on clone of master, if this is more aligned with idiomatic use of the library then it fits better (this can then be an SNI aware connection callback impl) - as long as it's done prior to the handshake should be fine - which looking at the name of it should be the case.
There was a problem hiding this comment.
Its not something that currently exists (would be a new callback). However it would solve solve some other issues too (currently there is only one *tls.Config but you can specify multiple brokers to attempt connections to).
There was a problem hiding this comment.
I think it might be a good idea to have such a call back exposed (func OnConnectionAttempt) and then we can decline the PR and not need this explicit change, allowing the consumer again to set the SNI in the callback as needed, keeping the library clean.
So I think all thats needed is an example of the proxy and a callback and the PR can then be closed allowing other consumers to implement this functionality using those tools.
There was a problem hiding this comment.
I'm happy to add the callback (might be faster as this will touch a number of files). Are you happy to do a demo? (I don't use proxies so it's not something I can really add!).
There was a problem hiding this comment.
Oh I think we got our wires crossed, I've already done the callback unless you think I missed something.
I've used the example app I've included to test against mqtt.googleapis.com with a custom proxy on localhost
…e app to keep the library simple refactor: revert SNI handling and add a pre connect hook instead to keep the library clean and to allow the clients to customize the final tls config that will be in use for connecting Signed-off-by: amir-khassaia <amir.khassaia@gmail.com>
| @@ -321,6 +326,13 @@ func (o *ClientOptions) SetReconnectingHandler(cb ReconnectHandler) *ClientOptio | |||
| return o | |||
| } | |||
|
|
|||
| // SetConnectionAttemptHandler sets the ConnectionAttemptHandler callback to be executed prior | |||
There was a problem hiding this comment.
Think this comment should be something along the lines of:
// SetConnectionAttemptHandler sets the ConnectionAttemptHandler callback to be executed prior
// to each attempt to connect to an MQTT broker. Returns the *tls.Config that will be used when establishing
// the connection (a copy of the tls.Config from ClientOptions will be passed in along with the broker URL). This
// allows connection specific changes to be made to the *tls.Config.
There was a problem hiding this comment.
Thanks, pushed updated comment
|
For future reverence: Verified that @amir-khassaia has signed the ECA (https://accounts.eclipse.org/legal/eca along with the @gmail.com email address in the signed-off footer). I am satisfied that the author of the commit has signed the ECA. |
Signed-off-by: amir-khassaia <amir.khassaia@gmail.com>
|
Thanks very much for your contribution. |
Happy to contribute, I will probably end up consuming once this is released in the future 😉 |
See #496