Zed lake service launched by Zui is accepting remote connections

[philrz]

tl;dr

In GA Zui v1.5.0 and older, the Zed lake service was launched by Zui with zed serve -l localhost:9867 such that client connections would only be permitted from localhost. The changes in #2934 resulted in this becoming zed serve -l :9867 such that in GA Zui v1.6.0 and newer Zui's local lake service has been open to connections from remote addresses as well. Hopefully default desktop configs are such that firewalls would prevent such incoming connections regardless, but we should likely revert this change in the interest of good security hygiene. Unfortunately, other tool changes (probably the same ones that resulted in the change in #2934) require us to do a bit more than just put back the original code.

Details

The change from #2934 in question is commit e0e3a7d where this line that formerly said this.addr(), changed to:

zui/packages/zed-node/src/lake.ts

Line 41 in 9c98969

':' + this.port,

If we revert just that change back to this.addr() and then try to do something with the lake such as load data, we now get an error:

Web searches regarding this error brings up Node.js issue nodejs/node#40702 wherein this comment seems like the best summary I've seen. In brief, it appears that the move to Node v18 in #2972 has made it such that now when the zed-node client seeks to connect to localhost, new Node behavior has made it such that the connection is now attempted to the IPv6 address ::1 and since the Zed service is listening only on the IPv4 address the connection is refused.

I've been experimenting with some of the fixes/workarounds described in nodejs/node#40702 and am weighing the trade-offs. I'll add a comment next with findings thus far.

[philrz]

Between my research and discussions with the team thus far, there's a few ways I see to potentially address this.

1. The Zed lake service could be enhanced to allow for listening for incoming connections on both IPv4 127.0.0.1 and IPv6 ::1. However, the Zed Dev team is currently tied up with other priorities, so consensus was to shelve this approach for now.

2. We could have the zed-node client connect to the local Zed lake service directly via IP 127.0.0.1. Indeed, many of the users in the comments of the Node issues I've linked to above seem to favor this approach, and it also was the approach favored by the Brim team when discussing this issue as a group. I've made a Zui branch zed-lake-only-localhost where I've made the necessary changes to get the app working with this change and make all tests pass. While I was ready to move forward with this approach, after spending more time with it as a user, I'm not in love with how this surfaces 127.0.0.1 in the UI where it used to say localhost, as the latter seems more self-documenting of it reflecting a connection to a "local lake". Granted, I'm guessing our more technical users would still understand, and maybe our less technical users wouldn't need to care. But it gave me reason to pause.

3. We could attempt to restore the prior Node behavior. This seems to be the second most discussed workaround in the Node issues linked above and can apparently be achieved by setting dns.setDefaultResultOrder('ipv4first'). I've made a Zui branch zed-lake-localhost-dns-order where I've made the necessary changes to get the app working with this change and make all tests pass. Compared to the connect-directly-to-127.0.0.1 workaround, there's a few things I like about it:

   • It's fewer lines of code.
   • It still shows http://localhost:9867 in the UI.
   • We never claimed to have any concerns about how Node behaved this way in the past, so in this regard feels like a safe place to land for now.

   On the negative side, the team seemed to have an instinctive aversion the approach when I floated it as an option, I guess because it seems hacky. (I'll also note that I'm not a pro at TypeScript, so maybe there's a less ugly way to make the config change.)

4. I learned late in all this about Happy Eyeballs (also discussed in the Node issues linked above) that seems like it would have the potential to be the cleanest solution. As I understand it, the client would attempt to connect using both IPv4 and IPv6 and hence should allow the client to still request a connection to localhost and ultimately still successfully connect to 127.0.0.1 even if it tries and fails to connect to ::1 as well. Based on what I've read in the Node v20 release notes, between the existence of the autoSelectFamily option of socket.connect() and net.getDefaultAutoSelectFamily() defaulting to true, I had the impression that if we moved up to such a newer Node release this might "just work". However, I've made a Zui branch happy-eyeballs which I tested when running Node v20.12.0 and still saw the same ECONNREFUSED. Maybe there's more to it than I think. I can see the zed-node client connection relies on node-fetch and there's an open issue Happy Eyeballs node-fetch/node-fetch#1297 and closed-without-merge PR Implement Happy Eyeballs node-fetch/node-fetch#1326, so maybe the base support for Happy Eyeballs in Node is not adequate? I'm still early in understanding all this.

[nwt]

While I was ready to move forward with this approach, after spending more time with it as a user, I'm not in love with how this surfaces 127.0.0.1 in the UI where it used to say localhost, as the latter seems more self-documenting of it reflecting a connection to a "local lake".

@philrz: It's easy enough for the UI to change http://127.0.0.1:9867 to http://localhost:9867 so how about that?

[philrz]

Thanks @nwt. That seems like a reasonable suggestion. Unfortunately, upon closer inspection it seems the side effects of the connect-directly-to-127.0.0.1 approach go a bit deeper than just presentation. While we can eliminate much of the problem by having the app initiate its own connections to 127.0.0.1 under the covers, I now realize that as long as the app keeps the Node client with the new behavior in play users can still run into the problem.

Consider the attached video which I took with the zed-lake-only-localhost branch and a Zui Insiders dev build based on the same. As a user, let's say I want to connect from my Zui Insiders to the lake running behind regular Zui. This may not come often, but given how we position the two apps as something that can be run side-by-side it could indeed happen. I add a remote lake config to http://localhost:9867 which seems like an innocent enough thing to do. But then when I try to load data, of course that means the ECONNREFUSED happens again.

Repro.mp4

This doesn't happen with the zed-lake-localhost-dns-order approach.

[nwt]

Zui is currently using Electron 28, with Node v18 in the main process. Upgrading to Electron 29 or 30 will give us Node v20. That seems like the best way to get the DNS-related behavior we want (i.e., if a name resolves to both A and AAAA records, then try both).

If upgrading Electron isn't practical right now, another way to get the behavior we want is to use Electron's net.fetch() API in the main process instead of node-fetch by doing something like this:

diff --git a/apps/zui/src/core/main/main-object.ts b/apps/zui/src/core/main/main-object.ts
index 1392194e2..02d7ba843 100644
--- a/apps/zui/src/core/main/main-object.ts
+++ b/apps/zui/src/core/main/main-object.ts
@@ -2,7 +2,7 @@ import {app} from "electron"
 import keytar from "keytar"
 import {EventEmitter} from "events"
 import os from "os"
-import {Client, Lake} from "@brimdata/zed-node"
+import {Lake} from "@brimdata/zed-node"
 import {Store as ReduxStore} from "redux"
 import url from "url"
 import {
@@ -22,6 +22,7 @@ import * as zdeps from "../../electron/zdeps"
 import {MainArgs, mainDefaults} from "../../electron/run-main/args"
 import createSession, {Session} from "../../electron/session"
 import {getAppMeta, AppMeta} from "../../electron/meta"
+import {ZedClient} from "../../electron/zed-client"
 import {createMainStore} from "../../js/state/stores/create-main-store"
 import {AppDispatch, State} from "../../js/state/types"
 import {PathName, getPath} from "../../js/api/core/get-path"
@@ -135,7 +136,7 @@ export class MainObject {
     const lakeData = Lakes.id(lakeId)(this.store.getState())
     const lake = createLake(lakeData)
     const auth = await this.dispatch(getAuthToken(lake))
-    return new Client(lake.getAddress(), {auth})
+    return new ZedClient(lake.getAddress(), {auth})
   }

   async createDefaultClient() {
diff --git a/apps/zui/src/electron/zed-client.ts b/apps/zui/src/electron/zed-client.ts
new file mode 100644
index 000000000..b8d2d6408
--- /dev/null
+++ b/apps/zui/src/electron/zed-client.ts
@@ -0,0 +1,9 @@
+import {Client} from "@brimdata/zed-node"
+import {net} from "electron"
+
+export class ZedClient extends Client {
+  // eslint-disable-next-line
+  // @ts-ignore
+  // eslint-disable-next-line
+  public fetch = (...args: any[]) => net.fetch(...args);
+}