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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Tidy up and comment daemon CLI #8180

Merged
merged 1 commit into from
Apr 8, 2023
Merged

Tidy up and comment daemon CLI #8180

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
215 changes: 158 additions & 57 deletions src/nix/daemon.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
///@file

#include "command.hh"
#include "shared.hh"
#include "local-store.hh"
Expand Down Expand Up @@ -34,6 +36,19 @@
using namespace nix;
using namespace nix::daemon;

/**
* Settings related to authenticating clients for the Nix daemon.
*
* For pipes we have little good information about the client side, but
* for Unix domain sockets we do. So currently these options implemented
* mandatory access control based on user names and group names (looked
* up and translated to UID/GIDs in the CLI process that runs the code
* in this file).
*
* No code outside of this file knows about these settings (this is not
* exposed in a header); all authentication and authorization happens in
* `daemon.cc`.
*/
struct AuthorizationSettings : Config {

Setting<Strings> trustedUsers{
Expand All @@ -54,7 +69,9 @@ struct AuthorizationSettings : Config {
> directories that are otherwise inacessible to them.
)"};

/* ?Who we trust to use the daemon in safe ways */
/**
* Who we trust to use the daemon in safe ways
*/
Setting<Strings> allowedUsers{
this, {"*"}, "allowed-users",
R"(
Expand Down Expand Up @@ -112,8 +129,36 @@ static void setSigChldAction(bool autoReap)
throw SysError("setting SIGCHLD handler");
}

/**
* @return Is the given user a member of this group?
*
* @param user User specified by username.
*
* @param group Group the user might be a member of.
*/
static bool matchUser(std::string_view user, const struct group & gr)
{
for (char * * mem = gr.gr_mem; *mem; mem++)
if (user == std::string_view(*mem)) return true;
return false;
}


bool matchUser(const std::string & user, const std::string & group, const Strings & users)
/**
* Does the given user (specified by user name and primary group name)
* match the given user/group whitelist?
*
* If the list allows all users: Yes.
*
* If the username is in the set: Yes.
*
* If the groupname is in the set: Yes.
*
* If the user is in another group which is in the set: yes.
*
* Otherwise: No.
*/
static bool matchUser(const std::string & user, const std::string & group, const Strings & users)
{
if (find(users.begin(), users.end(), "*") != users.end())
return true;
Expand All @@ -126,8 +171,7 @@ bool matchUser(const std::string & user, const std::string & group, const String
if (group == i.substr(1)) return true;
struct group * gr = getgrnam(i.c_str() + 1);
if (!gr) continue;
for (char * * mem = gr->gr_mem; *mem; mem++)
if (user == std::string(*mem)) return true;
if (matchUser(user, *gr)) return true;
}

return false;
Expand All @@ -145,7 +189,9 @@ struct PeerInfo
};


// Get the identity of the caller, if possible.
/**
* Get the identity of the caller, if possible.
*/
static PeerInfo getPeerInfo(int remote)
{
PeerInfo peer = { false, 0, false, 0, false, 0 };
Expand Down Expand Up @@ -179,6 +225,9 @@ static PeerInfo getPeerInfo(int remote)
#define SD_LISTEN_FDS_START 3


/**
* Open a store without a path info cache.
*/
static ref<Store> openUncachedStore()
{
Store::Params params; // FIXME: get params from somewhere
Expand All @@ -187,7 +236,44 @@ static ref<Store> openUncachedStore()
return openStore(settings.storeUri, params);
}

/**
* Authenticate a potential client
*
* @param peer Information about other end of the connection, the client which
* wants to communicate with us.
*
* @return A pair of a `TrustedFlag`, whether the potential client is trusted,
* and the name of the user (useful for printing messages).
*
* If the potential client is not allowed to talk to us, we throw an `Error`.
*/
static std::pair<TrustedFlag, std::string> authPeer(const PeerInfo & peer)
{
TrustedFlag trusted = NotTrusted;

struct passwd * pw = peer.uidKnown ? getpwuid(peer.uid) : 0;
std::string user = pw ? pw->pw_name : std::to_string(peer.uid);

struct group * gr = peer.gidKnown ? getgrgid(peer.gid) : 0;
std::string group = gr ? gr->gr_name : std::to_string(peer.gid);

const Strings & trustedUsers = authorizationSettings.trustedUsers;
const Strings & allowedUsers = authorizationSettings.allowedUsers;

if (matchUser(user, group, trustedUsers))
trusted = Trusted;

if ((!trusted && !matchUser(user, group, allowedUsers)) || group == settings.buildUsersGroup)
throw Error("user '%1%' is not allowed to connect to the Nix daemon", user);

return { trusted, std::move(user) };
}


/**
* Run a server. The loop opens a socket and accepts new connections from that
* socket.
*/
static void daemonLoop()
{
if (chdir("/") == -1)
Expand Down Expand Up @@ -231,23 +317,9 @@ static void daemonLoop()

closeOnExec(remote.get());

TrustedFlag trusted = NotTrusted;
PeerInfo peer = getPeerInfo(remote.get());

struct passwd * pw = peer.uidKnown ? getpwuid(peer.uid) : 0;
std::string user = pw ? pw->pw_name : std::to_string(peer.uid);

struct group * gr = peer.gidKnown ? getgrgid(peer.gid) : 0;
std::string group = gr ? gr->gr_name : std::to_string(peer.gid);

Strings trustedUsers = authorizationSettings.trustedUsers;
Strings allowedUsers = authorizationSettings.allowedUsers;

if (matchUser(user, group, trustedUsers))
trusted = Trusted;

if ((!trusted && !matchUser(user, group, allowedUsers)) || group == settings.buildUsersGroup)
throw Error("user '%1%' is not allowed to connect to the Nix daemon", user);
auto [_trusted, user] = authPeer(peer);
auto trusted = _trusted;

printInfo((std::string) "accepted connection from pid %1%, user %2%" + (trusted ? " (trusted)" : ""),
peer.pidKnown ? std::to_string(peer.pid) : "<unknown>",
Expand Down Expand Up @@ -294,45 +366,74 @@ static void daemonLoop()
}
}

/**
* Forward a standard IO connection to the given remote store.
*
* We just act as a middleman blindly ferry output between the standard
* input/output and the remote store connection, not processing anything.
*
* Loops until standard input disconnects, or an error is encountered.
*/
static void forwardStdioConnection(RemoteStore & store) {
auto conn = store.openConnectionWrapper();
int from = conn->from.fd;
int to = conn->to.fd;

auto nfds = std::max(from, STDIN_FILENO) + 1;
while (true) {
fd_set fds;
FD_ZERO(&fds);
FD_SET(from, &fds);
FD_SET(STDIN_FILENO, &fds);
if (select(nfds, &fds, nullptr, nullptr, nullptr) == -1)
throw SysError("waiting for data from client or server");
if (FD_ISSET(from, &fds)) {
auto res = splice(from, nullptr, STDOUT_FILENO, nullptr, SSIZE_MAX, SPLICE_F_MOVE);
if (res == -1)
throw SysError("splicing data from daemon socket to stdout");
else if (res == 0)
throw EndOfFile("unexpected EOF from daemon socket");
}
if (FD_ISSET(STDIN_FILENO, &fds)) {
auto res = splice(STDIN_FILENO, nullptr, to, nullptr, SSIZE_MAX, SPLICE_F_MOVE);
if (res == -1)
throw SysError("splicing data from stdin to daemon socket");
else if (res == 0)
return;
}
}
}

/**
* Process a client connecting to us via standard input/output
*
* Unlike `forwardStdioConnection()` we do process commands ourselves in
* this case, not delegating to another daemon.
*
* @note `Trusted` is unconditionally passed because in this mode we
* blindly trust the standard streams. Limiting access to those is
* explicitly not `nix-daemon`'s responsibility.
*/
Ericson2314 marked this conversation as resolved.
Show resolved Hide resolved
static void processStdioConnection(ref<Store> store)
{
FdSource from(STDIN_FILENO);
FdSink to(STDOUT_FILENO);
processConnection(store, from, to, Trusted, NotRecursive);
Copy link
Member

Choose a reason for hiding this comment

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

Instead of documenting a limitation, I think we could make it work.

Suggested change
processConnection(store, from, to, Trusted, NotRecursive);
processConnection(store, from, to, store->isTrustedClient(), NotRecursive);

The point of this mode of operation is that we forward everything, so that should probably include informing the client about their trustedness correctly.

Copy link
Member Author

Choose a reason for hiding this comment

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

I actually went with the first one for 3 reasons:

  1. I hope this can be a pure refactor and not change any behavior.
  2. I don't think that change is enough to warrant removing the note: in the case where the underlying store is a LocalStore, for example, it will blindly return Trusted. So standard streams + non-RemoteStore still means an invalidated client.
  3. It is good to kindly ferry along the whether the "next" store trusts the client, but we could do that in both the stdio and non-stdio cases.

So bottom line is I think this is a good idea, but it deserves is own follow-up PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also I remembered that the way it works today is that the trusting of operations in processConnection is based solely on this parameter, but the trusting status returned to the client already takes store->isTrustedClient() into account.

That means in a chained daemon situation that even if an intermediate daemon trusts the the client (and doesn't block any request) the result of store->isTrustedClient() is still the intersection of the trust of each link in the chain.

I actually think that might be good semantics:

  1. Don't block requests on behalf of another store further down the chain, let the first untrusting store do the blocking.
  2. Do accurately report to the eventual client whether privileged requests will actually go all the way through.

}

/**
* Entry point shared between the new CLI `nix daemon` and old CLI
* `nix-daemon`.
*/
static void runDaemon(bool stdio)
{
if (stdio) {
if (auto store = openUncachedStore().dynamic_pointer_cast<RemoteStore>()) {
auto conn = store->openConnectionWrapper();
int from = conn->from.fd;
int to = conn->to.fd;

auto nfds = std::max(from, STDIN_FILENO) + 1;
while (true) {
fd_set fds;
FD_ZERO(&fds);
FD_SET(from, &fds);
FD_SET(STDIN_FILENO, &fds);
if (select(nfds, &fds, nullptr, nullptr, nullptr) == -1)
throw SysError("waiting for data from client or server");
if (FD_ISSET(from, &fds)) {
auto res = splice(from, nullptr, STDOUT_FILENO, nullptr, SSIZE_MAX, SPLICE_F_MOVE);
if (res == -1)
throw SysError("splicing data from daemon socket to stdout");
else if (res == 0)
throw EndOfFile("unexpected EOF from daemon socket");
}
if (FD_ISSET(STDIN_FILENO, &fds)) {
auto res = splice(STDIN_FILENO, nullptr, to, nullptr, SSIZE_MAX, SPLICE_F_MOVE);
if (res == -1)
throw SysError("splicing data from stdin to daemon socket");
else if (res == 0)
return;
}
}
} else {
FdSource from(STDIN_FILENO);
FdSink to(STDOUT_FILENO);
/* Auth hook is empty because in this mode we blindly trust the
standard streams. Limiting access to those is explicitly
not `nix-daemon`'s responsibility. */
processConnection(openUncachedStore(), from, to, Trusted, NotRecursive);
}
auto store = openUncachedStore();

if (auto remoteStore = store.dynamic_pointer_cast<RemoteStore>())
forwardStdioConnection(*remoteStore);
else
processStdioConnection(store);
} else
daemonLoop();
}
Expand Down