Explicitly model all settings and fail on unrecognized ones

Previously, the Settings class allowed other code to query for string
properties, which led to a proliferation of code all over the place making
up new options without any sort of central registry of valid options. This
commit pulls all those options back into the central Settings class and
removes the public get() methods, to discourage future abuses like that.

Furthermore, because we know the full set of options ahead of time, we
now fail loudly if someone enters an unrecognized option, thus preventing
subtle typos. With some template fun, we could probably also dump the full
set of options (with documentation, defaults, etc.) to the command line,
but I'm not doing that yet here.

10 files changed, 218 insertions, 87 deletions

diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc
index d418ab4e4..f6cdedb37 100644
--- a/src/libexpr/eval.cc
+++ b/src/libexpr/eval.cc
@@ -299,7 +299,7 @@ EvalState::EvalState(const Strings & _searchPath, ref<Store> store)
 {
     countCalls = getEnv("NIX_COUNT_CALLS", "0") != "0";
 
-    restricted = settings.get("restrict-eval", false);
+    restricted = settings.restrictEval;
 
     assert(gcInitialised);
 
diff --git a/src/libstore/build.cc b/src/libstore/build.cc
index 1ce23135f..4a7e1a62b 100644
--- a/src/libstore/build.cc
+++ b/src/libstore/build.cc
@@ -34,13 +34,6 @@
 #include <pwd.h>
 #include <grp.h>
 
-/* chroot-like behavior from Apple's sandbox */
-#if __APPLE__
-    #define DEFAULT_ALLOWED_IMPURE_PREFIXES "/System/Library /usr/lib /dev /bin/sh"
-#else
-    #define DEFAULT_ALLOWED_IMPURE_PREFIXES ""
-#endif
-
 /* Includes required for chroot support. */
 #if __linux__
 #include <sys/socket.h>
@@ -1279,7 +1272,7 @@ void DerivationGoal::inputsRealised()
 
     /* Don't repeat fixed-output derivations since they're already
        verified by their output hash.*/
-    nrRounds = fixedOutput ? 1 : settings.get("build-repeat", 0) + 1;
+    nrRounds = fixedOutput ? 1 : settings.buildRepeat + 1;
 
     /* Okay, try to build.  Note that here we don't wait for a build
        slot to become available, since we don't need one if there is a
@@ -1685,9 +1678,7 @@ void DerivationGoal::startBuilder()
 
     /* Are we doing a chroot build? */
     {
-        string x = settings.get("build-use-sandbox",
-            /* deprecated alias */
-            settings.get("build-use-chroot", string("false")));
+        string x = settings.useSandbox;
         if (x != "true" && x != "false" && x != "relaxed")
             throw Error("option ‘build-use-sandbox’ must be set to one of ‘true’, ‘false’ or ‘relaxed’");
         if (x == "true") {
@@ -1744,21 +1735,10 @@ void DerivationGoal::startBuilder()
 
     if (useChroot) {
 
-        string defaultChrootDirs;
-#if __linux__
-        if (worker.store.isInStore(BASH_PATH))
-            defaultChrootDirs = "/bin/sh=" BASH_PATH;
-#endif
-
         /* Allow a user-configurable set of directories from the
            host file system. */
-        PathSet dirs = tokenizeString<StringSet>(
-            settings.get("build-sandbox-paths",
-                /* deprecated alias with lower priority */
-                settings.get("build-chroot-dirs", defaultChrootDirs)));
-        PathSet dirs2 = tokenizeString<StringSet>(
-            settings.get("build-extra-chroot-dirs",
-                settings.get("build-extra-sandbox-paths", string(""))));
+        PathSet dirs = settings.sandboxPaths;
+        PathSet dirs2 = settings.extraSandboxPaths;
         dirs.insert(dirs2.begin(), dirs2.end());
 
         dirsInChroot.clear();
@@ -1790,8 +1770,7 @@ void DerivationGoal::startBuilder()
         for (auto & i : closure)
             dirsInChroot[i] = i;
 
-        string allowed = settings.get("allowed-impure-host-deps", string(DEFAULT_ALLOWED_IMPURE_PREFIXES));
-        PathSet allowedPaths = tokenizeString<StringSet>(allowed);
+        PathSet allowedPaths = settings.allowedImpureHostPrefixes;
 
         /* This works like the above, except on a per-derivation level */
         Strings impurePaths = tokenizeString<Strings>(get(drv->env, "__impureHostDeps"));
@@ -1811,7 +1790,7 @@ void DerivationGoal::startBuilder()
                 }
             }
             if (!found)
-                throw Error(format("derivation ‘%1%’ requested impure path ‘%2%’, but it was not in allowed-impure-host-deps (‘%3%’)") % drvPath % i % allowed);
+                throw Error(format("derivation ‘%1%’ requested impure path ‘%2%’, but it was not in allowed-impure-host-deps") % drvPath % i);
 
             dirsInChroot[i] = i;
         }
@@ -2433,7 +2412,7 @@ void DerivationGoal::runChild()
             /* Mount a new tmpfs on /dev/shm to ensure that whatever
                the builder puts in /dev/shm is cleaned up automatically. */
             if (pathExists("/dev/shm") && mount("none", (chrootRootDir + "/dev/shm").c_str(), "tmpfs", 0,
-                    fmt("size=%s", settings.get("sandbox-dev-shm-size", std::string("50%"))).c_str()) == -1)
+                    fmt("size=%s", settings.sandboxShmSize).c_str()) == -1)
                 throw SysError("mounting /dev/shm");
 
 #if 0
@@ -2596,7 +2575,7 @@ void DerivationGoal::runChild()
             sandboxProfile += "(version 1)\n";
 
             /* Violations will go to the syslog if you set this. Unfortunately the destination does not appear to be configurable */
-            if (settings.get("darwin-log-sandbox-violations", false)) {
+            if (settings.darwinLogSandboxViolations) {
                 sandboxProfile += "(deny default)\n";
             } else {
                 sandboxProfile += "(deny default (with no-log))\n";
@@ -2743,7 +2722,7 @@ void DerivationGoal::registerOutputs()
     InodesSeen inodesSeen;
 
     Path checkSuffix = ".check";
-    bool runDiffHook = settings.get("run-diff-hook", false);
+    bool runDiffHook = settings.runDiffHook;
     bool keepPreviousRound = settings.keepFailed || runDiffHook;
 
     /* Check whether the output paths were created, and grep each
@@ -2981,7 +2960,7 @@ void DerivationGoal::registerOutputs()
                     ? fmt("output ‘%1%’ of ‘%2%’ differs from ‘%3%’ from previous round", i->path, drvPath, prev)
                     : fmt("output ‘%1%’ of ‘%2%’ differs from previous round", i->path, drvPath);
 
-                auto diffHook = settings.get("diff-hook", std::string(""));
+                auto diffHook = settings.diffHook;
                 if (prevExists && diffHook != "" && runDiffHook) {
                     try {
                         auto diff = runProgram(diffHook, true, {prev, i->path});
@@ -2992,7 +2971,7 @@ void DerivationGoal::registerOutputs()
                     }
                 }
 
-                if (settings.get("enforce-determinism", true))
+                if (settings.enforceDeterminism)
                     throw NotDeterministic(msg);
 
                 printError(msg);
diff --git a/src/libstore/crypto.cc b/src/libstore/crypto.cc
index 747483afb..9692dd83b 100644
--- a/src/libstore/crypto.cc
+++ b/src/libstore/crypto.cc
@@ -105,12 +105,12 @@ PublicKeys getDefaultPublicKeys()
 
     // FIXME: filter duplicates
 
-    for (auto s : settings.get("binary-cache-public-keys", Strings())) {
+    for (auto s : settings.binaryCachePublicKeys) {
         PublicKey key(s);
         publicKeys.emplace(key.name, key);
     }
 
-    for (auto secretKeyFile : settings.get("secret-key-files", Strings())) {
+    for (auto secretKeyFile : settings.secretKeyFiles) {
         try {
             SecretKey secretKey(readFile(secretKeyFile));
             publicKeys.emplace(secretKey.name, secretKey.toPublicKey());
diff --git a/src/libstore/download.cc b/src/libstore/download.cc
index 25ccd7d0b..661ee2ed5 100644
--- a/src/libstore/download.cc
+++ b/src/libstore/download.cc
@@ -331,9 +331,9 @@ struct CurlDownloader : public Downloader
         curl_multi_setopt(curlm, CURLMOPT_PIPELINING, CURLPIPE_MULTIPLEX);
         #endif
         curl_multi_setopt(curlm, CURLMOPT_MAX_TOTAL_CONNECTIONS,
-            settings.get("binary-caches-parallel-connections", 25));
+            settings.binaryCachesParallelConnections);
 
-        enableHttp2 = settings.get("enable-http2", true);
+        enableHttp2 = settings.enableHttp2;
 
         wakeupPipe.create();
         fcntl(wakeupPipe.readSide.get(), F_SETFL, O_NONBLOCK);
@@ -573,7 +573,7 @@ Path Downloader::downloadCached(ref<Store> store, const string & url_, bool unpa
 
     string expectedETag;
 
-    int ttl = settings.get("tarball-ttl", 60 * 60);
+    int ttl = settings.tarballTtl;
     bool skip = false;
 
     if (pathExists(fileLink) && pathExists(dataFile)) {
diff --git a/src/libstore/globals.cc b/src/libstore/globals.cc
index 474288b78..62ed0376d 100644
--- a/src/libstore/globals.cc
+++ b/src/libstore/globals.cc
@@ -17,12 +17,23 @@ namespace nix {
    must be deleted and recreated on startup.) */
 #define DEFAULT_SOCKET_PATH "/daemon-socket/socket"
 
+/* chroot-like behavior from Apple's sandbox */
+#if __APPLE__
+    #define DEFAULT_ALLOWED_IMPURE_PREFIXES "/System/Library /usr/lib /dev /bin/sh"
+#else
+    #define DEFAULT_ALLOWED_IMPURE_PREFIXES ""
+#endif
 
 Settings settings;
 
 
 Settings::Settings()
 {
+    deprecatedOptions = StringSet({
+        "build-use-chroot", "build-chroot-dirs", "build-extra-chroot-dirs",
+        "this-option-never-existed-but-who-will-know"
+    });
+
     nixPrefix = NIX_PREFIX;
     nixStore = canonPath(getEnv("NIX_STORE_DIR", getEnv("NIX_STORE", NIX_STORE_DIR)));
     nixDataDir = canonPath(getEnv("NIX_DATA_DIR", NIX_DATA_DIR));
@@ -73,6 +84,32 @@ Settings::Settings()
     showTrace = false;
     enableImportNative = false;
     netrcFile = fmt("%s/%s", nixConfDir, "netrc");
+    useSandbox = "false"; // TODO: make into an enum
+
+#if __linux__
+    sandboxPaths = tokenizeString<StringSet>("/bin/sh=" BASH_PATH);
+#endif
+
+    restrictEval = false;
+    buildRepeat = 0;
+    allowedImpureHostPrefixes = tokenizeString<StringSet>(DEFAULT_ALLOWED_IMPURE_PREFIXES);
+    sandboxShmSize = "50%";
+    darwinLogSandboxViolations = false;
+    runDiffHook = false;
+    diffHook = "";
+    enforceDeterminism = true;
+    binaryCachePublicKeys = Strings();
+    secretKeyFiles = Strings();
+    binaryCachesParallelConnections = 25;
+    enableHttp2 = true;
+    tarballTtl = 60 * 60;
+    signedBinaryCaches = "";
+    substituters = Strings();
+    binaryCaches = Strings();
+    extraBinaryCaches = Strings();
+    trustedUsers = Strings({"root"});
+    allowedUsers = Strings({"*"});
+    printMissing = true;
 }
 
 
@@ -115,39 +152,6 @@ void Settings::set(const string & name, const string & value)
     overrides[name] = value;
 }
 
-
-string Settings::get(const string & name, const string & def)
-{
-    auto i = settings.find(name);
-    if (i == settings.end()) return def;
-    return i->second;
-}
-
-
-Strings Settings::get(const string & name, const Strings & def)
-{
-    auto i = settings.find(name);
-    if (i == settings.end()) return def;
-    return tokenizeString<Strings>(i->second);
-}
-
-
-bool Settings::get(const string & name, bool def)
-{
-    bool res = def;
-    _get(res, name);
-    return res;
-}
-
-
-int Settings::get(const string & name, int def)
-{
-    int res = def;
-    _get(res, name);
-    return res;
-}
-
-
 void Settings::update()
 {
     _get(tryFallback, "build-fallback");
@@ -181,13 +185,71 @@ void Settings::update()
     _get(keepGoing, "keep-going");
     _get(keepFailed, "keep-failed");
     _get(netrcFile, "netrc-file");
+    _get(useSandbox, "build-use-sandbox", "build-use-chroot");
+    _get(sandboxPaths, "build-sandbox-paths", "build-chroot-dirs");
+    _get(extraSandboxPaths, "build-extra-sandbox-paths", "build-extra-chroot-dirs");
+    _get(restrictEval, "restrict-eval");
+    _get(buildRepeat, "build-repeat");
+    _get(allowedImpureHostPrefixes, "allowed-impure-host-deps");
+    _get(sandboxShmSize, "sandbox-dev-shm-size");
+    _get(darwinLogSandboxViolations, "darwin-log-sandbox-violations");
+    _get(runDiffHook, "run-diff-hook");
+    _get(diffHook, "diff-hook");
+    _get(enforceDeterminism, "enforce-determinism");
+    _get(binaryCachePublicKeys, "binary-cache-public-keys");
+    _get(secretKeyFiles, "secret-key-files");
+    _get(binaryCachesParallelConnections, "binary-caches-parallel-connections");
+    _get(enableHttp2, "enable-http2");
+    _get(tarballTtl, "tarball-ttl");
+    _get(signedBinaryCaches, "signed-binary-caches");
+    _get(substituters, "substituters");
+    _get(binaryCaches, "binary-caches");
+    _get(extraBinaryCaches, "extra-binary-caches");
+    _get(trustedUsers, "trusted-users");
+    _get(allowedUsers, "allowed-users");
+    _get(printMissing, "print-missing");
+
+    /* Clear out any deprecated options that might be left, so users know we recognize the option
+       but aren't processing it anymore */
+    for (auto &i : deprecatedOptions) {
+        if (settings.find(i) != settings.end()) {
+            printError(format("warning: deprecated option '%1%' is no longer supported and will be ignored") % i);
+            settings.erase(i);
+        }
+    }
+
+    if (settings.size() != 0) {
+        string bad;
+        for (auto &i : settings)
+            bad += "'" + i.first + "', ";
+        bad.pop_back();
+        bad.pop_back();
+        throw Error(format("unrecognized options: %s") % bad);
+    }
 }
 
+void Settings::checkDeprecated(const string & name)
+{
+    if (deprecatedOptions.find(name) != deprecatedOptions.end())
+        printError(format("warning: deprecated option '%1%' will soon be unsupported") % name);
+}
 
 void Settings::_get(string & res, const string & name)
 {
     SettingsMap::iterator i = settings.find(name);
     if (i == settings.end()) return;
+    checkDeprecated(i->first);
+    settings.erase(i);
+    res = i->second;
+}
+
+void Settings::_get(string & res, const string & name1, const string & name2)
+{
+    SettingsMap::iterator i = settings.find(name1);
+    if (i == settings.end()) i = settings.find(name2);
+    if (i == settings.end()) return;
+    checkDeprecated(i->first);
+    settings.erase(i);
     res = i->second;
 }
 
@@ -196,6 +258,8 @@ void Settings::_get(bool & res, const string & name)
 {
     SettingsMap::iterator i = settings.find(name);
     if (i == settings.end()) return;
+    checkDeprecated(i->first);
+    settings.erase(i);
     if (i->second == "true") res = true;
     else if (i->second == "false") res = false;
     else throw Error(format("configuration option ‘%1%’ should be either ‘true’ or ‘false’, not ‘%2%’")
@@ -207,6 +271,20 @@ void Settings::_get(StringSet & res, const string & name)
 {
     SettingsMap::iterator i = settings.find(name);
     if (i == settings.end()) return;
+    checkDeprecated(i->first);
+    settings.erase(i);
+    res.clear();
+    Strings ss = tokenizeString<Strings>(i->second);
+    res.insert(ss.begin(), ss.end());
+}
+
+void Settings::_get(StringSet & res, const string & name1, const string & name2)
+{
+    SettingsMap::iterator i = settings.find(name1);
+    if (i == settings.end()) i = settings.find(name2);
+    if (i == settings.end()) return;
+    checkDeprecated(i->first);
+    settings.erase(i);
     res.clear();
     Strings ss = tokenizeString<Strings>(i->second);
     res.insert(ss.begin(), ss.end());
@@ -216,6 +294,8 @@ void Settings::_get(Strings & res, const string & name)
 {
     SettingsMap::iterator i = settings.find(name);
     if (i == settings.end()) return;
+    checkDeprecated(i->first);
+    settings.erase(i);
     res = tokenizeString<Strings>(i->second);
 }
 
@@ -224,6 +304,8 @@ template<class N> void Settings::_get(N & res, const string & name)
 {
     SettingsMap::iterator i = settings.find(name);
     if (i == settings.end()) return;
+    checkDeprecated(i->first);
+    settings.erase(i);
     if (!string2Int(i->second, res))
         throw Error(format("configuration setting ‘%1%’ should have an integer value") % name);
 }
diff --git a/src/libstore/globals.hh b/src/libstore/globals.hh
index 0ff18f8b1..d74488a41 100644
--- a/src/libstore/globals.hh
+++ b/src/libstore/globals.hh
@@ -20,14 +20,6 @@ struct Settings {
 
     void set(const string & name, const string & value);
 
-    string get(const string & name, const string & def);
-
-    Strings get(const string & name, const Strings & def);
-
-    bool get(const string & name, bool def);
-
-    int get(const string & name, int def);
-
     void update();
 
     string pack();
@@ -36,6 +28,10 @@ struct Settings {
 
     SettingsMap getOverrides();
 
+    /* TODO: the comments below should be strings and exposed via a nice command-line UI or similar.
+       We should probably replace it with some sort of magic template or macro to minimize the amount
+       of duplication and pain here. */
+
     /* The directory where we store sources and derived files. */
     Path nixStore;
 
@@ -187,6 +183,75 @@ struct Settings {
     /* Whether the importNative primop should be enabled */
     bool enableImportNative;
 
+    /* Whether to enable sandboxed builds (string until we get an enum for true/false/relaxed) */
+    string useSandbox;
+
+    /* The basic set of paths to expose in a sandbox */
+    PathSet sandboxPaths;
+
+    /* Any extra sandbox paths to expose */
+    PathSet extraSandboxPaths;
+
+    /* Whether to allow certain questionable operations (like fetching) during evaluation */
+    bool restrictEval;
+
+    /* The number of times to repeat a build to check for determinism */
+    int buildRepeat;
+
+    /* Which prefixes to allow derivations to ask for access to (primarily for Darwin) */
+    PathSet allowedImpureHostPrefixes;
+
+    /* The size of /dev/shm in the build sandbox (for Linux) */
+    string sandboxShmSize;
+
+    /* Whether to log Darwin sandbox access violations to the system log */
+    bool darwinLogSandboxViolations;
+
+    /* ??? */
+    bool runDiffHook;
+
+    /* ??? */
+    string diffHook;
+
+    /* Whether to fail if repeated builds produce different output */
+    bool enforceDeterminism;
+
+    /* The known public keys for a binary cache */
+    Strings binaryCachePublicKeys;
+
+    /* Secret keys to use for build output signing */
+    Strings secretKeyFiles;
+
+    /* Number of parallel connections to hit a binary cache with when finding out if it contains hashes */
+    int binaryCachesParallelConnections;
+
+    /* Whether to enable HTTP2 */
+    bool enableHttp2;
+
+    /* How soon to expire tarballs like builtins.fetchTarball and (ugh, bad name) builtins.fetchurl */
+    int tarballTtl;
+
+    /* ??? */
+    string signedBinaryCaches;
+
+    /* ??? */
+    Strings substituters;
+
+    /* ??? */
+    Strings binaryCaches;
+
+    /* ??? */
+    Strings extraBinaryCaches;
+
+    /* Who we trust to ask the daemon to do unsafe things */
+    Strings trustedUsers;
+
+    /* ?Who we trust to use the daemon in safe ways */
+    Strings allowedUsers;
+
+    /* ??? */
+    bool printMissing;
+
     /* The hook to run just before a build to set derivation-specific
        build settings */
     Path preBuildHook;
@@ -196,11 +261,16 @@ struct Settings {
     Path netrcFile;
 
 private:
+    StringSet deprecatedOptions;
     SettingsMap settings, overrides;
 
+    void checkDeprecated(const string & name);
+
     void _get(string & res, const string & name);
+    void _get(string & res, const string & name1, const string & name2);
     void _get(bool & res, const string & name);
     void _get(StringSet & res, const string & name);
+    void _get(StringSet & res, const string & name1, const string & name2);
     void _get(Strings & res, const string & name);
     template<class N> void _get(N & res, const string & name);
 };
diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc
index 4c161cfb3..afcda6e2b 100644
--- a/src/libstore/local-store.cc
+++ b/src/libstore/local-store.cc
@@ -44,7 +44,7 @@ LocalStore::LocalStore(const Params & params)
     , reservedPath(dbDir + "/reserved")
     , schemaPath(dbDir + "/schema")
     , trashDir(realStoreDir + "/trash")
-    , requireSigs(trim(settings.get("signed-binary-caches", std::string(""))) != "") // FIXME: rename option
+    , requireSigs(trim(settings.signedBinaryCaches) != "") // FIXME: rename option
     , publicKeys(getDefaultPublicKeys())
 {
     auto state(_state.lock());
@@ -1330,7 +1330,7 @@ void LocalStore::signPathInfo(ValidPathInfo & info)
 {
     // FIXME: keep secret keys in memory.
 
-    auto secretKeyFiles = settings.get("secret-key-files", Strings());
+    auto secretKeyFiles = settings.secretKeyFiles;
 
     for (auto & secretKeyFile : secretKeyFiles) {
         SecretKey secretKey(readFile(secretKeyFile));
diff --git a/src/libstore/store-api.cc b/src/libstore/store-api.cc
index b5934a0d1..603424277 100644
--- a/src/libstore/store-api.cc
+++ b/src/libstore/store-api.cc
@@ -766,13 +766,13 @@ std::list<ref<Store>> getDefaultSubstituters()
         state->stores.push_back(openStore(uri));
     };
 
-    for (auto uri : settings.get("substituters", Strings()))
+    for (auto uri : settings.substituters)
         addStore(uri);
 
-    for (auto uri : settings.get("binary-caches", Strings()))
+    for (auto uri : settings.binaryCaches)
         addStore(uri);
 
-    for (auto uri : settings.get("extra-binary-caches", Strings()))
+    for (auto uri : settings.extraBinaryCaches)
         addStore(uri);
 
     state->done = true;
diff --git a/src/nix-daemon/nix-daemon.cc b/src/nix-daemon/nix-daemon.cc
index 3b43ddfa1..2fcb5b565 100644
--- a/src/nix-daemon/nix-daemon.cc
+++ b/src/nix-daemon/nix-daemon.cc
@@ -865,8 +865,8 @@ static void daemonLoop(char * * argv)
             struct group * gr = peer.gidKnown ? getgrgid(peer.gid) : 0;
             string group = gr ? gr->gr_name : std::to_string(peer.gid);
 
-            Strings trustedUsers = settings.get("trusted-users", Strings({"root"}));
-            Strings allowedUsers = settings.get("allowed-users", Strings({"*"}));
+            Strings trustedUsers = settings.trustedUsers;
+            Strings allowedUsers = settings.allowedUsers;
 
             if (matchUser(user, group, trustedUsers))
                 trusted = true;
diff --git a/src/nix-store/nix-store.cc b/src/nix-store/nix-store.cc
index 0aabe66c5..bb3b430c9 100644
--- a/src/nix-store/nix-store.cc
+++ b/src/nix-store/nix-store.cc
@@ -146,7 +146,7 @@ static void opRealise(Strings opFlags, Strings opArgs)
         unknown = PathSet();
     }
 
-    if (settings.get("print-missing", true))
+    if (settings.printMissing)
         printMissing(ref<Store>(store), willBuild, willSubstitute, unknown, downloadSize, narSize);
 
     if (dryRun) return;