Ludovic Courtès <ludovic.cour...@inria.fr> writes:
> Hello Roel,
>
> Roel Janssen <r...@gnu.org> skribis:
>
[...]
>
>> From 00f489d6303720c65571fdf0bc9ee810a20f70e0 Mon Sep 17 00:00:00 2001
>> From: Roel Janssen <r...@gnu.org>
>> Date: Wed, 11 Apr 2018 09:52:11 +0200
>> Subject: [PATCH] guix-daemon: Disable garbage collection for remote hosts.
>>
>> * nix/libstore/gc.cc (collectGarbage): Check for remote connections.
>> * nix/libstore/globals.hh: Add isRemoteConnection setting.
>> * nix/nix-daemon/nix-daemon.cc (performOp): Display appropriate error
>> message;
>> (acceptConnection): Set isRemoteConnection when connection is over TCP.
>
> [...]
>
>> --- a/nix/libstore/gc.cc
>> +++ b/nix/libstore/gc.cc
>> @@ -595,6 +595,10 @@ void LocalStore::removeUnusedLinks(const GCState &
>> state)
>>
>> void LocalStore::collectGarbage(const GCOptions & options, GCResults &
>> results)
>> {
>> + if (settings.isRemoteConnection) {
>> + return;
>> + }
>
> I think this is unnecessary since the daemon already checks for that.
> (It’s also nicer to keep ‘LocalStore’ unaware of the connection
> details.)
Right. It is indeed unnecessary. In the updated patch, I no longer do this.
>
>> diff --git a/nix/nix-daemon/nix-daemon.cc b/nix/nix-daemon/nix-daemon.cc
>> index deb7003d7..65770ba95 100644
>> --- a/nix/nix-daemon/nix-daemon.cc
>> +++ b/nix/nix-daemon/nix-daemon.cc
>> @@ -529,6 +529,11 @@ static void performOp(bool trusted, unsigned int
>> clientVersion,
>> }
>>
>> case wopCollectGarbage: {
>> + if (settings.isRemoteConnection) {
>> + throw Error("Garbage collection is disabled for remote hosts.");
>> + break;
>> + }
>> GCOptions options;
>> options.action = (GCOptions::GCAction) readInt(from);
>> options.pathsToDelete = readStorePaths<PathSet>(from);
>
> I was wondering if we would like to allow some of the ‘GCAction’ values,
> but maybe it’s better to disallow them altogether like this code does.
Could we please start with a “disable any GC” and start allowing cases
on a case-by-case basis? The reason I request this, is because it makes
it a lot easier to reason about it from a sysadmin point-of-view.
I'd like to think of it like this: “Garbage collection is effectively
turned off.”. With the current patch, we can reason about it this way.
>
>> @@ -934,6 +939,7 @@ static void acceptConnection(int fdSocket)
>> connection. Setting these to -1 means: do not change.
>> */
>> settings.clientUid = clientUid;
>> settings.clientGid = clientGid;
>> + settings.isRemoteConnection = (remoteAddr.ss_family !=
>> AF_UNIX);
>
> I think you can make ‘isRemoteConnection’ a static global variable in
> nix-daemon.cc instead of adding it to ‘Settings’. So it would do
> something like:
>
> --8<---------------cut here---------------start------------->8---
> /* Fork a child to handle the connection. */
> startProcess([&]() {
> close(fdSocket);
>
> /* Background the daemon. */
> if (setsid() == -1)
> throw SysError(format("creating a new session"));
>
> /* Restore normal handling of SIGCHLD. */
> setSigChldAction(false);
>
> /* For debugging, stuff the pid into argv[1]. */
> if (clientPid != -1 && argvSaved[1]) {
> string processName = std::to_string(clientPid);
> strncpy(argvSaved[1], processName.c_str(),
> strlen(argvSaved[1]));
> }
>
> isRemoteConnection = …; /* <– this is the new line */
>
> /* Store the client's user and group for this connection. This
> has to be done in the forked process since it is per
> connection. Setting these to -1 means: do not change. */
> settings.clientUid = clientUid;
> settings.clientGid = clientGid;
> --8<---------------cut here---------------end--------------->8---
Right. I implemented it using a static global variable in
nix-daemon.cc. This patch gets shorter and shorter. :)
>
> Last thing: could you add a couple of tests? tests/guix-daemon.sh
> already has tests for ‘--listen’, so you could take inspiration from
> those.
I included a test, but I don't know how I can properly run this test.
Could you elaborate on how I can test the test(s)?
Kind regards,
Roel Janssen
>From b29d3a90e1487ebda5ac5b6bc146f8c95218eab6 Mon Sep 17 00:00:00 2001
From: Roel Janssen <r...@gnu.org>
Date: Thu, 19 Apr 2018 14:01:49 +0200
Subject: [PATCH] guix-daemon: Disable garbage collection for remote hosts.
* nix/nix-daemon/nix-daemon.cc (performOp): Display appropriate error message;
(acceptConnection): Set isRemoteConnection when connection is over TCP.
* tests/guix-daemon.sh: Add a test for the new behavior.
---
nix/nix-daemon/nix-daemon.cc | 10 +++++++++-
tests/guix-daemon.sh | 14 ++++++++++++++
2 files changed, 23 insertions(+), 1 deletion(-)
diff --git a/nix/nix-daemon/nix-daemon.cc b/nix/nix-daemon/nix-daemon.cc
index deb7003d7..782e4acfc 100644
--- a/nix/nix-daemon/nix-daemon.cc
+++ b/nix/nix-daemon/nix-daemon.cc
@@ -54,7 +54,9 @@ static FdSink to(STDOUT_FILENO);
bool canSendStderr;
-
+/* This variable is used to keep track of whether a connection
+ comes from a host other than the host running guix-daemon. */
+static bool isRemoteConnection;
/* This function is called anytime we want to write something to
stderr. If we're in a state where the protocol allows it (i.e.,
@@ -529,6 +531,11 @@ static void performOp(bool trusted, unsigned int clientVersion,
}
case wopCollectGarbage: {
+ if (isRemoteConnection) {
+ throw Error("Garbage collection is disabled for remote hosts.");
+ break;
+ }
+
GCOptions options;
options.action = (GCOptions::GCAction) readInt(from);
options.pathsToDelete = readStorePaths<PathSet>(from);
@@ -934,6 +941,7 @@ static void acceptConnection(int fdSocket)
connection. Setting these to -1 means: do not change. */
settings.clientUid = clientUid;
settings.clientGid = clientGid;
+ isRemoteConnection = (remoteAddr.ss_family != AF_UNIX);
/* Handle the connection. */
from.fd = remote;
diff --git a/tests/guix-daemon.sh b/tests/guix-daemon.sh
index 6f91eb58b..438c79c26 100644
--- a/tests/guix-daemon.sh
+++ b/tests/guix-daemon.sh
@@ -194,6 +194,20 @@ do
kill "$daemon_pid"
done
+# Make sure garbage collection from a TCP connection does not work.
+
+socket="127.0.0.1:9999"
+guix-daemon --listen="$socket" &
+daemon_pid=$!
+
+output=`GUIX_DAEMON_SOCKET="$socket" guix gc`
+if [[ "$output" != *"GUIX_DAEMON_SOCKET=$socket" ]];
+then
+ exit 1
+fi
+
+kill "$daemon_pid"
+
# Log compression.
guix-daemon --listen="$socket" --disable-chroot --debug --log-compression=gzip &
--
2.17.0
