[keyserver] Listen on all addresses in dev mode
ClosedPublic
Actions

Authored by ashoat on Dec 21 2022, 7:39 AM.

Details

Reviewers

• jon
tomek

Commits

rCOMM9355453a76a5: [keyserver] Listen on all addresses in dev mode

Summary

When we were using Apache as a proxy, we only needed the keyserver to listen on localhost. That's because Apache was the only thing connected to Node, and Apache would always connect on localhost.

Now that clients are connecting directly to Node, we need to tell Node to listen on other addresses. Without this change, it's not possible to connect an external device to a keyserver running on a dev machine.

The reason we didn't catch this in D5846 was because the test plan was insufficient. In the future, we need to be even stricter with test plans.

Test Plan

Make sure a physical iOS device is able to connect to the keyserver running on my laptop using port 3000

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

ashoat created this revision.Dec 21 2022, 7:39 AM

Herald added a subscriber: atul. · View Herald TranscriptDec 21 2022, 7:39 AM

ashoat retitled this revision from [keyserver] Listen to all ports in dev mode to [keyserver] Listen on all addresses in dev mode.Dec 21 2022, 7:39 AM

Harbormaster completed remote builds in B14630: Diff 19953.Dec 21 2022, 7:53 AM

ashoat requested review of this revision.Dec 21 2022, 7:53 AM

• jon added inline comments.Dec 21 2022, 8:18 AM

keyserver/src/keyserver.js
167 ↗	(On Diff #19953)	looks like undefined will default to "0.0.0.0" for the socket addr, which will bind to all interfaces. Seems like our desired logic would just be: server.listen(parseInt(process.env.PORT, 10) \|\| 3000); Unless there's a compelling reason to limit the interfaces for docker.

• jon added inline comments.Dec 21 2022, 8:20 AM

keyserver/src/keyserver.js
167 ↗	(On Diff #19953)	Based on what's documented here: https://nodejs.org/api/net.html#serverlistenport-host-backlog-callback

ashoat added inline comments.Dec 21 2022, 11:28 AM

keyserver/src/keyserver.js
167 ↗	(On Diff #19953)	I don't see why we would get rid of `process.env.COMM_LISTEN_ADDR`. Unless there's a compelling reason to limit the interfaces for docker. I think it's generally a good idea for security?

• jon accepted this revision.Dec 21 2022, 1:55 PM

• jon added inline comments.

keyserver/src/keyserver.js
167 ↗	(On Diff #19953)	I think it's generally a good idea for security? Probably, but only certain ports are exposed. I guess I don't feel strongly enough.