Merge pull request #9850 from obsidiansystems/missing-store-urls

Ensure all store types support "real" URIs

src/libstore/dummy-store.cc

@@ -18,9 +18,12 @@ struct DummyStoreConfig : virtual StoreConfig {
 
 struct DummyStore : public virtual DummyStoreConfig, public virtual Store
 {
-    DummyStore(const std::string scheme, const std::string uri, const Params & params)
+    DummyStore(std::string_view scheme, std::string_view authority, const Params & params)
         : DummyStore(params)
-    { }
+    {
+        if (!authority.empty())
+            throw UsageError("`%s` store URIs must not contain an authority part %s", scheme, authority);
+    }
 
     DummyStore(const Params & params)
         : StoreConfig(params)

src/libstore/http-binary-cache-store.cc

@@ -39,15 +39,20 @@ private:
 public:
 
     HttpBinaryCacheStore(
-        const std::string & scheme,
+        std::string_view scheme,
-        const Path & _cacheUri,
+        PathView _cacheUri,
         const Params & params)
         : StoreConfig(params)
         , BinaryCacheStoreConfig(params)
         , HttpBinaryCacheStoreConfig(params)
         , Store(params)
         , BinaryCacheStore(params)
-        , cacheUri(scheme + "://" + _cacheUri)
+        , cacheUri(
+            std::string { scheme }
+            + "://"
+            + (!_cacheUri.empty()
+                ? _cacheUri
+                : throw UsageError("`%s` Store requires a non-empty authority in Store URL", scheme)))
     {
         while (!cacheUri.empty() && cacheUri.back() == '/')
             cacheUri.pop_back();

src/libstore/legacy-ssh-store.cc

@@ -28,8 +28,18 @@ struct LegacySSHStore::Connection : public ServeProto::BasicClientConnection
     bool good = true;
 };
 
+LegacySSHStore::LegacySSHStore(
+    std::string_view scheme,
+    std::string_view host,
+    const Params & params)
+    : LegacySSHStore{scheme, LegacySSHStoreConfig::extractConnStr(scheme, host), params}
+{
+}
 
-LegacySSHStore::LegacySSHStore(const std::string & scheme, const std::string & host, const Params & params)
+LegacySSHStore::LegacySSHStore(
+    std::string_view scheme,
+    std::string host,
+    const Params & params)
     : StoreConfig(params)
     , CommonSSHStoreConfig(params)
     , LegacySSHStoreConfig(params)
@@ -42,8 +52,8 @@ LegacySSHStore::LegacySSHStore(const std::string & scheme, const std::string & h
         ))
     , master(
         host,
-        sshKey,
+        sshKey.get(),
-        sshPublicHostKey,
+        sshPublicHostKey.get(),
         // Use SSH master only if using more than 1 connection.
         connections->capacity() > 1,
         compress,

src/libstore/legacy-ssh-store.hh

@@ -41,7 +41,17 @@ struct LegacySSHStore : public virtual LegacySSHStoreConfig, public virtual Stor
 
     static std::set<std::string> uriSchemes() { return {"ssh"}; }
 
-    LegacySSHStore(const std::string & scheme, const std::string & host, const Params & params);
+    LegacySSHStore(
+        std::string_view scheme,
+        std::string_view host,
+        const Params & params);
+
+private:
+    LegacySSHStore(
+        std::string_view scheme,
+        std::string host,
+        const Params & params);
+public:
 
     ref<Connection> openConnection();
 

src/libstore/local-binary-cache-store.cc

@@ -28,9 +28,13 @@ private:
 
 public:
 
+    /**
+     * @param binaryCacheDir `file://` is a short-hand for `file:///`
+     * for now.
+     */
     LocalBinaryCacheStore(
-        const std::string scheme,
+        std::string_view scheme,
-        const Path & binaryCacheDir,
+        PathView binaryCacheDir,
         const Params & params)
         : StoreConfig(params)
         , BinaryCacheStoreConfig(params)

src/libstore/local-store.cc

@@ -463,10 +463,20 @@ LocalStore::LocalStore(const Params & params)
 }
 
 
-LocalStore::LocalStore(std::string scheme, std::string path, const Params & params)
+LocalStore::LocalStore(
-    : LocalStore(params)
+    std::string_view scheme,
+    PathView path,
+    const Params & _params)
+    : LocalStore([&]{
+        // Default `?root` from `path` if non set
+        if (!path.empty() && _params.count("root") == 0) {
+            auto params = _params;
+            params.insert_or_assign("root", std::string { path });
+            return params;
+        }
+        return _params;
+    }())
 {
-    throw UnimplementedError("LocalStore");
 }
 
 

src/libstore/local-store.hh

@@ -137,12 +137,15 @@ public:
      * necessary.
      */
     LocalStore(const Params & params);
-    LocalStore(std::string scheme, std::string path, const Params & params);
+    LocalStore(
+        std::string_view scheme,
+        PathView path,
+        const Params & params);
 
     ~LocalStore();
 
     static std::set<std::string> uriSchemes()
-    { return {}; }
+    { return {"local"}; }
 
     /**
      * Implementations of abstract store API methods.

src/libstore/s3-binary-cache-store.cc

@@ -269,8 +269,8 @@ struct S3BinaryCacheStoreImpl : virtual S3BinaryCacheStoreConfig, public virtual
     S3Helper s3Helper;
 
     S3BinaryCacheStoreImpl(
-        const std::string & uriScheme,
+        std::string_view uriScheme,
-        const std::string & bucketName,
+        std::string_view bucketName,
         const Params & params)
         : StoreConfig(params)
         , BinaryCacheStoreConfig(params)
@@ -281,6 +281,8 @@ struct S3BinaryCacheStoreImpl : virtual S3BinaryCacheStoreConfig, public virtual
         , bucketName(bucketName)
         , s3Helper(profile, region, scheme, endpoint)
     {
+        if (bucketName.empty())
+            throw UsageError("`%s` store requires a bucket name in its Store URI", uriScheme);
         diskCache = getNarInfoDiskCache();
     }
 

src/libstore/ssh-store-config.cc

@@ -0,0 +1,24 @@
+#include <regex>
+
+#include "ssh-store-config.hh"
+
+namespace nix {
+
+std::string CommonSSHStoreConfig::extractConnStr(std::string_view scheme, std::string_view _connStr)
+{
+    if (_connStr.empty())
+        throw UsageError("`%s` store requires a valid SSH host as the authority part in Store URI", scheme);
+
+    std::string connStr{_connStr};
+
+    std::smatch result;
+    static std::regex v6AddrRegex("^((.*)@)?\\[(.*)\\]$");
+
+    if (std::regex_match(connStr, result, v6AddrRegex)) {
+        connStr = result[1].matched ? result.str(1) + result.str(3) : result.str(3);
+    }
+
+    return connStr;
+}
+
+}

src/libstore/ssh-store-config.hh

@@ -24,6 +24,29 @@ struct CommonSSHStoreConfig : virtual StoreConfig
           to be used on the remote machine. The default is `auto`
           (i.e. use the Nix daemon or `/nix/store` directly).
         )"};
+
+    /**
+     * The `parseURL` function supports both IPv6 URIs as defined in
+     * RFC2732, but also pure addresses. The latter one is needed here to
+     * connect to a remote store via SSH (it's possible to do e.g. `ssh root@::1`).
+     *
+     * This function now ensures that a usable connection string is available:
+     *
+     * - If the store to be opened is not an SSH store, nothing will be done.
+     *
+     * - If the URL looks like `root@[::1]` (which is allowed by the URL parser and probably
+     *   needed to pass further flags), it
+     *   will be transformed into `root@::1` for SSH (same for `[::1]` -> `::1`).
+     *
+     * - If the URL looks like `root@::1` it will be left as-is.
+     *
+     * - In any other case, the string will be left as-is.
+     *
+     * Will throw an error if `connStr` is empty too.
+     */
+    static std::string extractConnStr(
+        std::string_view scheme,
+        std::string_view connStr);
 };
 
 }

src/libstore/ssh-store.cc

@@ -32,9 +32,10 @@ struct SSHStoreConfig : virtual RemoteStoreConfig, virtual CommonSSHStoreConfig
 
 class SSHStore : public virtual SSHStoreConfig, public virtual RemoteStore
 {
-public:
+    SSHStore(
-
+        std::string_view scheme,
-    SSHStore(const std::string & scheme, const std::string & host, const Params & params)
+        std::string host,
+        const Params & params)
         : StoreConfig(params)
         , RemoteStoreConfig(params)
         , CommonSSHStoreConfig(params)
@@ -44,14 +45,24 @@ public:
         , host(host)
         , master(
             host,
-            sshKey,
+            sshKey.get(),
-            sshPublicHostKey,
+            sshPublicHostKey.get(),
             // Use SSH master only if using more than 1 connection.
             connections->capacity() > 1,
             compress)
     {
     }
 
+public:
+
+    SSHStore(
+        std::string_view scheme,
+        std::string_view host,
+        const Params & params)
+        : SSHStore{scheme, SSHStoreConfig::extractConnStr(scheme, host), params}
+    {
+    }
+
     static std::set<std::string> uriSchemes() { return {"ssh-ng"}; }
 
     std::string getUri() override
@@ -141,7 +152,10 @@ class MountedSSHStore : public virtual MountedSSHStoreConfig, public virtual SSH
 {
 public:
 
-    MountedSSHStore(const std::string & scheme, const std::string & host, const Params & params)
+    MountedSSHStore(
+        std::string_view scheme,
+        std::string_view host,
+        const Params & params)
         : StoreConfig(params)
         , RemoteStoreConfig(params)
         , CommonSSHStoreConfig(params)

src/libstore/ssh.cc

@@ -6,7 +6,11 @@
 
 namespace nix {
 
-SSHMaster::SSHMaster(const std::string & host, const std::string & keyFile, const std::string & sshPublicHostKey, bool useMaster, bool compress, int logFD)
+SSHMaster::SSHMaster(
+    std::string_view host,
+    std::string_view keyFile,
+    std::string_view sshPublicHostKey,
+    bool useMaster, bool compress, int logFD)
     : host(host)
     , fakeSSH(host == "localhost")
     , keyFile(keyFile)

src/libstore/ssh.hh

@@ -39,7 +39,11 @@ private:
 
 public:
 
-    SSHMaster(const std::string & host, const std::string & keyFile, const std::string & sshPublicHostKey, bool useMaster, bool compress, int logFD = -1);
+    SSHMaster(
+        std::string_view host,
+        std::string_view keyFile,
+        std::string_view sshPublicHostKey,
+        bool useMaster, bool compress, int logFD = -1);
 
     struct Connection
     {

src/libstore/store-api.cc

@@ -21,7 +21,6 @@
 #include "users.hh"
 
 #include <nlohmann/json.hpp>
-#include <regex>
 
 using json = nlohmann::json;
 
@@ -1321,9 +1320,7 @@ std::shared_ptr<Store> openFromNonUri(const std::string & uri, const Store::Para
                 warn("'%s' does not exist, so Nix will use '%s' as a chroot store", stateDir, chrootStore);
             } else
                 debug("'%s' does not exist, so Nix will use '%s' as a chroot store", stateDir, chrootStore);
-            Store::Params params2;
+            return std::make_shared<LocalStore>("local", chrootStore, params);
-            params2["root"] = chrootStore;
-            return std::make_shared<LocalStore>(params2);
         }
         #endif
         else
@@ -1333,42 +1330,12 @@ std::shared_ptr<Store> openFromNonUri(const std::string & uri, const Store::Para
     } else if (uri == "local") {
         return std::make_shared<LocalStore>(params);
     } else if (isNonUriPath(uri)) {
-        Store::Params params2 = params;
+        return std::make_shared<LocalStore>("local", absPath(uri), params);
-        params2["root"] = absPath(uri);
-        return std::make_shared<LocalStore>(params2);
     } else {
         return nullptr;
     }
 }
 
-// The `parseURL` function supports both IPv6 URIs as defined in
-// RFC2732, but also pure addresses. The latter one is needed here to
-// connect to a remote store via SSH (it's possible to do e.g. `ssh root@::1`).
-//
-// This function now ensures that a usable connection string is available:
-// * If the store to be opened is not an SSH store, nothing will be done.
-// * If the URL looks like `root@[::1]` (which is allowed by the URL parser and probably
-//   needed to pass further flags), it
-//   will be transformed into `root@::1` for SSH (same for `[::1]` -> `::1`).
-// * If the URL looks like `root@::1` it will be left as-is.
-// * In any other case, the string will be left as-is.
-static std::string extractConnStr(const std::string &proto, const std::string &connStr)
-{
-    if (proto.rfind("ssh") != std::string::npos) {
-        std::smatch result;
-        std::regex v6AddrRegex("^((.*)@)?\\[(.*)\\]$");
-
-        if (std::regex_match(connStr, result, v6AddrRegex)) {
-            if (result[1].matched) {
-                return result.str(1) + result.str(3);
-            }
-            return result.str(3);
-        }
-    }
-
-    return connStr;
-}
-
 ref<Store> openStore(const std::string & uri_,
     const Store::Params & extraParams)
 {
@@ -1377,10 +1344,7 @@ ref<Store> openStore(const std::string & uri_,
         auto parsedUri = parseURL(uri_);
         params.insert(parsedUri.query.begin(), parsedUri.query.end());
 
-        auto baseURI = extractConnStr(
+        auto baseURI = parsedUri.authority.value_or("") + parsedUri.path;
-            parsedUri.scheme,
-            parsedUri.authority.value_or("") + parsedUri.path
-        );
 
         for (auto implem : *Implementations::registered) {
             if (implem.uriSchemes.count(parsedUri.scheme)) {

src/libstore/store-api.hh

@@ -901,7 +901,14 @@ std::list<ref<Store>> getDefaultSubstituters();
 struct StoreFactory
 {
     std::set<std::string> uriSchemes;
-    std::function<std::shared_ptr<Store> (const std::string & scheme, const std::string & uri, const Store::Params & params)> create;
+    /**
+     * The `authorityPath` parameter is `<authority>/<path>`, or really
+     * whatever comes after `<scheme>://` and before `?<query-params>`.
+     */
+    std::function<std::shared_ptr<Store> (
+        std::string_view scheme,
+        std::string_view authorityPath,
+        const Store::Params & params)> create;
     std::function<std::shared_ptr<StoreConfig> ()> getConfig;
 };
 
@@ -916,7 +923,7 @@ struct Implementations
         StoreFactory factory{
             .uriSchemes = T::uriSchemes(),
             .create =
-                ([](const std::string & scheme, const std::string & uri, const Store::Params & params)
+                ([](auto scheme, auto uri, auto & params)
                  -> std::shared_ptr<Store>
                  { return std::make_shared<T>(scheme, uri, params); }),
             .getConfig =

src/libstore/uds-remote-store.cc

@@ -40,12 +40,13 @@ UDSRemoteStore::UDSRemoteStore(const Params & params)
 
 
 UDSRemoteStore::UDSRemoteStore(
-    const std::string scheme,
+    std::string_view scheme,
-    std::string socket_path,
+    PathView socket_path,
     const Params & params)
     : UDSRemoteStore(params)
 {
-    path.emplace(socket_path);
+    if (!socket_path.empty())
+        path.emplace(socket_path);
 }
 
 
@@ -54,6 +55,7 @@ std::string UDSRemoteStore::getUri()
     if (path) {
         return std::string("unix://") + *path;
     } else {
+        // unix:// with no path also works. Change what we return?
         return "daemon";
     }
 }

src/libstore/uds-remote-store.hh

@@ -28,7 +28,10 @@ class UDSRemoteStore : public virtual UDSRemoteStoreConfig
 public:
 
     UDSRemoteStore(const Params & params);
-    UDSRemoteStore(const std::string scheme, std::string path, const Params & params);
+    UDSRemoteStore(
+        std::string_view scheme,
+        PathView path,
+        const Params & params);
 
     std::string getUri() override;
 

src/libstore/unix/local-overlay-store.hh

@@ -92,7 +92,7 @@ class LocalOverlayStore : public virtual LocalOverlayStoreConfig, public virtual
 public:
     LocalOverlayStore(const Params & params);
 
-    LocalOverlayStore(std::string scheme, std::string path, const Params & params)
+    LocalOverlayStore(std::string_view scheme, PathView path, const Params & params)
         : LocalOverlayStore(params)
     {
         if (!path.empty())

tests/functional/read-only-store.sh

@@ -10,6 +10,9 @@ happy () {
     # We can do a read-only query just fine with a read-only store
     nix --store local?read-only=true path-info $dummyPath
 
+    # `local://` also works.
+    nix --store local://?read-only=true path-info $dummyPath
+
     # We can "write" an already-present store-path a read-only store, because no IO is actually required
     nix-store --store local?read-only=true --add dummy
 }