Callbacks for snowflake state

[cyBerta]

While integrating IPtProxy into onionmasq I was missing a way to receive a callback once snowflake is ready to go. This would help us to avoid errors when starting onionmasq in the unmanaged PT mode while snowflake is actually not ready yet to be used. As for RiseupVPN I've used a hacky workaround by parsing the logs written to the logfile while starting snowflake to get informed about state changes, but I would love to avoid to replicate that while integrating IPtProxy into TorVPN.

@n8fr8 mentioned that you already provide a patch for the snowflake proxy mode - and while I've read in another issue that you dislike maintaining patches - I wonder if there are good ways to get state feedback from IptProxy.

[tladesignz]

@cohosh, before I start digging into the code without a clue:

• What's your opinion?
• Is there already something which would be easy to expose in IPtProxy?
• If not, would you be willing to provide such a mechanism?

[cohosh]

My understanding is that ready here means that the Snowflake client has started listening for incoming SOCKS connections on the supplied port.

There are a few options here. It is possible to add a simple callback to the patch. Alternatively, we do have some Snowflake event listeners that can be used for this purpose. Using the events library would require just a slight modification to your Snowflake client patch, and I could provide an example of what that would look like.

However, we're definitely getting to the point now where it would be easier to call the Snowflake client as a library rather than maintaining a patch. If you don't mind waiting a few days, I can give an example of what it would look like to remove the necessity of a patch for Snowflake (and Lyrebird). It would reduce the amount of code and I think it could be really nice to maintain.

[tladesignz]

Looking very much forward to the latter!

[n8fr8]

We should make progress on this soon. If @cyBerta just needs a simple status callback, can we provide that?

[cohosh]

Here is the rewrite I put together quickly last week that cleans up a bunch of the functionality and will make it easier to add a callback: https://github.com/cohosh/IPtProxy/tree/library-refactor It's close to done, I just need to add back in proxy support and write some documentation. I'll open an MR for this by the end of the week with a full write up and explanation.

If you want a faster solution, you'll need to modify your patch files, because what you need a callback for is determining when you've started listening for incoming SOCKS connections which is already code in the patch. It can be quite simple to do this. This is what I'd recommend:

Add another input for the callback to the Start function:

+func Start(port *int, iceServersCommas, brokerURL, frontDomainsCommas, ampCacheURL, sqsQueueURL, sqsCredsStr, logFilename *string, logToStateDir, keepLocalAddresses, unsafeLogging *bool, max *int, onReady func()) {

And then call that function once you've called pt.ListenSocks:

@@ -270,11 +246,12 @@ func main() {
        switch methodName {
        case "snowflake":
            // TODO: Be able to recover when SOCKS dies.
-           ln, err := pt.ListenSocks("tcp", "127.0.0.1:0")
+           ln, err := pt.ListenSocks("tcp", net.JoinHostPort("127.0.0.1", strconv.Itoa(*port)))
            if err != nil {
                pt.CmethodError(methodName, err.Error())
                break
            }
+           onReady()

[cohosh]

I just realized that the bindings for gomobile might not support callback functions. In that case, I don't know what the fastest way to support this currently is.

[cohosh]

Here is the promised IPtProxy refactor: https://github.com/tladesignz/IPtProxy/pull/61

It's a lot of API changes, so no rush in looking it over and I really would like your thoughts and feedback on it.

In the meantime, I've learned how to do callbacks with the gomobile bindings, and I'm happy to advise on a short term fix for this issue now that I've figured out how to do it properly. Just let me know if you want further input from me here :)

[n8fr8]

I just realized that the bindings for gomobile might not support callback functions. In that case, I don't know what the fastest way to support this currently is.

I believe we have Go->JNI callback functions already in IPtProxy

[cohosh]

I just realized that the bindings for gomobile might not support callback functions. In that case, I don't know what the fastest way to support this currently is.

I believe we have Go->JNI callback functions already in IPtProxy

Yeah, after posting that I saw them in the proxy patch and figured out how to implement them: https://github.com/tladesignz/IPtProxy/blob/master/snowflake.patch#L112

[tladesignz]

Ok. Thanks to @cohosh, we have a refined version of IPtProxy now, which removes the dependency on patches and has an improved interface: https://github.com/tladesignz/IPtProxy/tree/refactor

The only thing which is still missing is the OnClientConnected callback in Snowflake Proxy, but @cohosh is already in the process of getting that into Snowflake: https://gitlab.torproject.org/tpo/anti-censorship/pluggable-transports/snowflake/-/merge_requests/424

As soon as that is available, I'll release an update to IPtProxy!

While this rewrite will not contain a callback for transport states, this should become unnecessary, as @cohosh explains:

The way this refactor is currently written, Start won't return until pt.ListenSocks is called, which is what was causing the problems that prompted the above issue. Before, snowflakeclient.Start was called in a goroutine, so the function could return before the SOCKS listener was ready to accept connections.

@cyBerta, please let me know, if this is not sufficient for your needs.

@cohosh, please let me know, when a new version of Snowflake with the OnClientConnected callback is available!

[cohosh]

While this rewrite will not contain a callback for transport states, this should become unnecessary, as @cohosh explains:

The way this refactor is currently written, Start won't return until pt.ListenSocks is called, which is what was causing the problems that prompted the above issue. Before, snowflakeclient.Start was called in a goroutine, so the function could return before the SOCKS listener was ready to accept connections.

Yep! There is an exception here, which is that if there was a failure that caused an early return from Start, there will not be a listener. But, this isn't a matter of waiting. If there is a failure to connect, it's because there was a fundamental problem with starting the transport. This can also be checked with a call to Port(), which will return 0 if the transport was not successfully started. This is where returning an error or error code (from either Start or Port) might be useful to callers.

[tladesignz]

started. This is where returning an error or error code (from either Start or Port) might be useful to callers.

Ahaha. You reminded me of a TODO there... :-)

[tladesignz]

Ok, I added returning errors to Start. That's actually handled pretty well by gomobile: In the JVM, its translated to a throw, in Objective-C, it's a BOOL return value plus an optional error pointer argument and in Swift that is translated to a throw also. Nice! https://github.com/tladesignz/IPtProxy/commit/22496af173500675784d82fb552c47a2944d272f

I also fixed some crashes: https://github.com/tladesignz/IPtProxy/commit/c8c7e67720d24fc2555330271484c068cb5838e6

So, if Start returns, you'll have a pretty good chance, that the transport will start.

Of course there can be more errors, but these are happening in coroutines, so aren't easily reportable, except via the log file.

I wonder if it would make sense to report them in some callback? 🤔

[cohosh]

This looks great! I could see the case for something like a OnTransportStopped callback that will execute when the copy loop ends and the transport is no longer listening for incoming SOCKS connections.