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

Use Unix Domain Socket for the registry #22

Closed
wants to merge 1 commit into from

Conversation

aszlig
Copy link
Contributor

@aszlig aszlig commented Oct 17, 2020

First of all: Thank you for napalm! I've been using naersk since quite a while and I really like not having lots of redundant generated files laying around in the repository :-)

This is an alternative solution to pull request #21, which implements picking a random TCP port for the registry. While I haven't found the exact reason why it has to be randomized, I can only guess that it's to prevent port conflicts in unsandboxed builds.

The implementation also has few ugly workarounds (eg. using lsof to get the port number), but the main issue I see is that even if the port is random, any user/process on the system can still connect to that port.

So instead of picking a random TCP port, let's simply not use IP sockets at all and use ip2unix to force NPM into connecting to the Unix socket.

We're now using a dummy IP address (127.0.0.100) for the registry URL in order to be able to match the registry URL in the ip2unix rule as a distinct host and at the same time prevent silent failure in case we forgot to transform/wrap sockets at some point.

Fixes: #4
Fixes: #10

This is an alternative solution to pull request nix-community#21, which implements
picking a random TCP port for the registry. While I haven't found the
exact reason why it has to be randomized, I can only guess that it's to
prevent port conflicts in unsandboxed builds.

The implementation also has few ugly workarounds (eg. using lsof to get
the port number), but the main issue I see is that even if the port is
random, any user/process on the system can still connect to that port.

So instead of picking a random TCP port, let's simply not use IP sockets
at all and use ip2unix to force NPM into connecting to the Unix socket.

We're now using a dummy IP address (127.0.0.100) for the registry URL in
order to be able to match the registry URL in the ip2unix rule as a
distinct host and at the same time prevent silent failure in case we
forgot to transform/wrap sockets at some point.

Signed-off-by: aszlig <aszlig@nix.build>
Fixes: nix-community#4, nix-community#10
@nmattia
Copy link
Collaborator

nmattia commented Oct 19, 2020

@ciderale: I appear to have missed #21 (comment)! Does this implementation work for you as well?

@aszlig thanks a lot! I didn't know about ip2unix, seems very handy.

I really like not having lots of redundant generated files laying around in the repository :-)

Me too :)

Copy link
Collaborator

@nmattia nmattia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Two suggestions, which I haven't actually tried. WDYT?

@@ -175,15 +178,16 @@ let

echo "Starting napalm registry"

napalm-registry --snapshot ${snapshot} &
registrySock="$TMPDIR/registry.sock"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
registrySock="$TMPDIR/registry.sock"
registrySock="$(mktemp -d)/registry.sock"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why nest this into another directory? We already set this up accordingly in the builder sandbox and if this would interfere with build-specific files, the mktemp variant would have the same issue.

@@ -195,7 +199,8 @@ let
while IFS= read -r c
do
echo "Running npm command: $c"
$c || (echo "$c: failure, aborting" && kill $napalm_REGISTRY_PID && exit 1)
ip2unix -r out,addr=127.0.0.100,path="$registrySock" -- $c \
|| (echo "$c: failure, aborting" && kill $napalm_REGISTRY_PID && exit 1)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
|| (echo "$c: failure, aborting" && kill $napalm_REGISTRY_PID && exit 1)
|| (echo "$c: failure, aborting" && (rm -rf $(dirname $registrySock) || true) && kill $napalm_REGISTRY_PID && exit 1)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The socket file should already be cleaned up by ip2unix, but since the kill signal is SIGTERM it depends on the signal handler of the registry. Nevertheless, if we assume that things are running unsandboxed and are not cleaned up properly by Nix, I'd probably avoid unquoted rm -rf at all costs, eg. more like this (without the extra directory, as mentioned above):

Suggested change
|| (echo "$c: failure, aborting" && kill $napalm_REGISTRY_PID && exit 1)
|| (echo "$c: failure, aborting" && kill "$napalm_REGISTRY_PID" && rm -f "$TMPDIR/registry.sock"; exit 1)

@ciderale
Copy link

@nmattia Unfortunately, the derivation ip2unix (or its dependencies) seems to not work on MacOS. I just tried to just get the tool in a nix shell and it failed (while building the attr derviation). I'm not sure if the problem is easy resolvable. It would be very unfortunate to exclude MacOS for this helpful tool.

I didn't know the ip2unix tool either. I agree that the lsof is not very pretty, but it has the advantage the registry still works with "traditional" host:port scheme. I find that a benefit.

Do I interpret it correctly, that the host argument for the registry must match the argument to ip2unix? In that case, I think it would be better to make that explicit and not relying on the default value in the registry.

@aszlig
Copy link
Contributor Author

aszlig commented Oct 20, 2020

First of all a disclaimer, since everybody seems to be wondering: I'm the author of ip2unix.

@ciderale: Ah, you're correct, I keep forgetting about OS X. There is an issue for that, but since I don't have access to OS X I didn't get around to implement support for OS X.

The reason why I used ip2unix instead of going via a random port is because I have a few dependencies with Git inputs, which I want to redirect/mitm accordingly, so this implementation is just part of some yak shaving.

I didn't know the ip2unix tool either. I agree that the lsof is not very pretty, but it has the advantage the registry still works with "traditional" host:port scheme. I find that a benefit.

As mentioned in the commit message, I could have used localhost, 127.0.0.1 or [::1] but I deliberately didn't so that the connection will fail hard if for some reason NPM connects via IP sockets (eg. via rule mismatch).

@nmattia
Copy link
Collaborator

nmattia commented Oct 20, 2020

Mh, this not working on macOS is definitely a problem; we have a few napalm projects that we build on macOS at $WORK. @aszlig do you reckon you'll get to implement support at some point? In the mean time I'll have a look at #21. Either way, thanks to both of you for the contributions and ideas!

@nmattia
Copy link
Collaborator

nmattia commented Oct 20, 2020

Had a go at it: #24

@aszlig can you let me know if that works for your use case?
@ciderale can you check if npm can connect on your machine?

@ciderale
Copy link

@nmattia #24 works on my machine and I think writing the port to a file is a good solution.

@nmattia
Copy link
Collaborator

nmattia commented Oct 21, 2020

Closing in favor of #24, @aszlig please let me know if that solution works for you!

@nmattia nmattia closed this Oct 21, 2020
@aszlig
Copy link
Contributor Author

aszlig commented Oct 21, 2020

@nmattia: Ah, of course it doesn't, because for me the port has never been an issue since I run all builds sandboxed. As mentioned, it's not about the port but rather to multiplex all kind of hosts. In any way, I'll implement this as a downstream patch and resubmit here once ip2unix has support for OS X.

@nmattia
Copy link
Collaborator

nmattia commented Oct 22, 2020

Ah yes, of course. Got a lot on my plate ATM, I easily get confused! I guess another possibility is to hide the use of ip2unix behind a flag in the derivation, though that might just make things complicated.

I have a few dependencies with Git inputs, which I want to redirect/mitm accordingly

Can you clarify this bit?

@aszlig
Copy link
Contributor Author

aszlig commented Oct 24, 2020

I have a few dependencies with Git inputs, which I want to redirect/mitm accordingly

Can you clarify this bit?

Essentially, my plan is/was to redirect port 443 (in conjunction with fake CA certificates) and 80 via Unix socket to either the registry or another dedicated service, which then will serve Git dependencies to NPM. In hindsight however, I think it's better to just patch NPM instead, which then would also avoid needing such a fake registry in the first place.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants