require both attic options, follow attic's cache-name rule, read the token from a file

docs/swarm/builder.md

@@ -267,12 +267,16 @@ inside the 61 minutes the controller waits. `attic` checks the server's certific
 against the system roots and pins nothing, so a Let's Encrypt certificate
 works.
 
+Set both options or neither: evaluation refuses a domain without a cache name.
+The cache name follows attic's rule, 1 to 50 letters, digits, `-`, `_` or `+`,
+starting with a letter or digit, and evaluation refuses any other name.
+
 The push token is the operator's to write. The builder reads it on every push
-from `secret/swarm/services/attic/push-token` (field `value`), writes an `attic`
+from `secret/swarm/services/attic/push-token` (field `value`), writes it to
-config file holding it in the unit's private runtime directory (`/run/swarm-builder`,
+a file and an `attic` config naming that file (`token-file`) in the unit's private
-mode `0700`, the file `0600`), points `attic` at it with `XDG_CONFIG_HOME` and
+runtime directory (`/run/swarm-builder`, mode `0700`, both files `0600`), points
-removes it after the push. The token is in no argument or variable, so no
+`attic` at the config with `XDG_CONFIG_HOME` and removes both after the push.
-process listing shows it.
+The token is in no argument or variable, so no process listing shows it.
 
 The reply's `cached` says whether the push finished. A push that fails, times out
 or can't read the token leaves the build green with `cached` false and logs a

nix/host-modules/swarm-attic-options.nix

@@ -12,7 +12,8 @@
         Name of the attic server, with no scheme: the swarm builder pushes to
         `https://<domain>`, and trusts the certificate the system roots do, so
         a Let's Encrypt certificate works and none is pinned. Set together with
-        {option}`services.hyperhive.swarm.attic.cache`.
+        {option}`services.hyperhive.swarm.attic.cache`; evaluation fails when only
+        one is set.
 
         Null pushes nothing: the swarm builder does not push, and the closure
         record says it is not cached.
@@ -24,13 +25,15 @@
     };
 
     cache = lib.mkOption {
-      type = lib.types.nullOr lib.types.str;
+      # attic's own rule for a cache name; the builder checks the same at start.
+      type = lib.types.nullOr (lib.types.strMatching "^[A-Za-z0-9][A-Za-z0-9_+-]{0,49}$");
       default = null;
       example = "grid";
       description = ''
         Name of the cache on {option}`services.hyperhive.swarm.attic.domain`
         that the swarm builder pushes closures to. The push token must be
-        allowed to push to it.
+        allowed to push to it. The name follows attic's rule: 1 to 50 letters,
+        digits, `-`, `_` or `+`, starting with a letter or digit.
       '';
     };
   };

nix/host-modules/swarm-builder.nix

@@ -90,8 +90,8 @@ let
   hyperhiveFlake = hyperhiveCfg.c0re.hyperhiveFlake;
   nixpkgsFlake = hyperhiveCfg.c0re.nixpkgsFlake;
 
-  # Either half alone reaches the daemon too, which refuses to start on it.
+  # The assertion below keeps the two options together.
-  pushesToCache = atticCfg.domain != null || atticCfg.cache != null;
+  pushesToCache = atticCfg.domain != null;
 
   privateNetwork = true;
 in
@@ -133,6 +133,14 @@ in
           host's /var/lib/swarm-bao-pki/ and name them in those two options.
         '';
       }
+      {
+        assertion = (atticCfg.domain == null) == (atticCfg.cache == null);
+        message = ''
+          services.hyperhive.swarm.attic.domain and services.hyperhive.swarm.attic.cache
+          must be set together: the swarm builder pushes to a cache on a server, and
+          either alone names nothing to push to.
+        '';
+      }
       {
         assertion = lib.all oneWord (lib.attrValues storeFiles);
         message = ''
@@ -257,10 +265,8 @@ in
               SWARM_BUILDER_FORGE_URL = "https://${forgeCfg.domain}";
               SWARM_BUILDER_CONFIG_ORG = "agent-configs";
             }
-            // lib.optionalAttrs (atticCfg.domain != null) {
+            // lib.optionalAttrs pushesToCache {
               SWARM_BUILDER_ATTIC_DOMAIN = atticCfg.domain;
-            }
-            // lib.optionalAttrs (atticCfg.cache != null) {
               SWARM_BUILDER_ATTIC_CACHE = atticCfg.cache;
             }
             // lib.optionalAttrs (storeFiles ? builder-bao-server-ca) {
@@ -276,7 +282,7 @@ in
               DynamicUser = true;
               CacheDirectory = unit;
               # /run/swarm-builder, mode 0700 and owned by the dynamic user: the
-              # push writes its token-bearing `attic` config under it.
+              # push writes its token file and `attic` config under it.
               RuntimeDirectory = unit;
               RuntimeDirectoryMode = "0700";
               # Bare ids: the system credentials nspawn handed in.

nix/module-eval/builder.nix

@@ -39,6 +39,21 @@ let
     m:
     lib.any (a: !a.assertion && lib.hasInfix "deploy.builder.enable requires" a.message) m.assertions;
 
+  # The one fixture that fires the cache assertion: a domain with no cache name.
+  halfCache = hive {
+    deploy.bao.enable = true;
+    deploy.builder.enable = true;
+    swarm.attic.domain = "cache.example.org";
+  };
+  refusedHalfCache =
+    m:
+    lib.any (
+      a:
+      !a.assertion && lib.hasInfix "swarm.attic.domain and services.hyperhive.swarm.attic.cache" a.message
+    ) m.assertions;
+  # Only the option is read, so the host around it is never evaluated.
+  cacheNamed = name: (hive { swarm.attic.cache = name; }).services.hyperhive.swarm.attic.cache;
+
   natsHere = shared.natsQueueOnly;
 
   autheliaHere = hive {
@@ -139,6 +154,30 @@ let
         && !(withoutCache.environment ? SWARM_BUILDER_ATTIC_DOMAIN)
         && !(withoutCache.environment ? SWARM_BUILDER_ATTIC_CACHE);
     }
+    {
+      # A domain with no cache name would crash-loop the builder at start, so
+      # evaluation refuses it. `withBuilder`, with both set, is the control: the
+      # same assertion list holds the entry and it passes.
+      name = "the builder refuses a cache domain without a cache name";
+      ok =
+        refusedHalfCache halfCache
+        && !(refusedHalfCache withBuilder)
+        && lib.any (a: lib.hasInfix "swarm.attic.domain and" a.message) withBuilder.assertions;
+    }
+    {
+      # attic refuses a cache name outside its rule, which would fail every push.
+      # `grid` is the control that the lookup reads a name.
+      name = "the cache name must follow attic's rule";
+      ok =
+        cacheNamed "grid" == "grid"
+        && cacheNamed "A-b_c+d" == "A-b_c+d"
+        && lib.all (n: !(builtins.tryEval (cacheNamed n)).success) [
+          "grid.v2"
+          "-grid"
+          ""
+          (lib.concatStrings (lib.genList (_: "a") 51))
+        ];
+    }
     {
       # The minting unit goes inactive after each run, so a start that pulls it
       # runs it again and mints a leaf that is missing; an active

swarm-builder/src/push.rs

@@ -1,10 +1,11 @@
 //! The push of a built closure to the binary cache, with `attic push`.
 //!
 //! The cache is an attic instance outside the swarm. Its push token is read
-//! from the secret store per push and written to an `attic` config file in a
+//! from the secret store per push and written to a file in a
-//! directory only the builder's unit can enter, which `attic` finds through
+//! directory only the builder's unit can enter, beside a config that names it with
-//! `XDG_CONFIG_HOME`. `attic login <name> <url> <token>` is not used: it puts
+//! `token-file`. `attic` finds the config through `XDG_CONFIG_HOME`.
-//! the token in the process list. `attic` verifies the server's certificate
+//! `attic login <name> <url> <token>` is not used: it puts the token in the
+//! process list. `attic` verifies the server's certificate
 //! against the system roots.
 //!
 //! A push that does not happen or does not finish leaves the build green with
@@ -54,13 +55,15 @@ impl Remote {
             (Some(domain), Some(cache)) => (domain, cache),
             _ => bail!("the cache domain and the cache name must be set together"),
         };
-        let plain = |s: &str| {
+        let plain_domain = !domain.is_empty()
-            !s.is_empty()
+            && domain
-                && s.bytes()
+                .bytes()
-                    .all(|b| b.is_ascii_alphanumeric() || matches!(b, b'.' | b'-' | b'_' | b':'))
+                .all(|b| b.is_ascii_alphanumeric() || matches!(b, b'.' | b'-' | b':'));
-        };
+        if !plain_domain {
-        if !plain(&domain) || !plain(&cache) {
+            bail!("the cache domain {domain:?} is not a host name with an optional port");
-            bail!("the cache domain {domain:?} and name {cache:?} must be plain names");
+        }
+        if !cache_name_ok(&cache) {
+            bail!("the cache name {cache:?} is not one attic accepts");
         }
         let Some(state_dir) = state_dir else {
             bail!("a cache is set, but the unit has no runtime directory to hold its config");
@@ -106,7 +109,7 @@ impl Remote {
     }
 
     /// `attic push` of `out_path`, reading its config from `config_home`. The
-    /// token is in the file there and in no argument or variable.
+    /// token is in a file beside it and in no argument or variable.
     fn command(&self, config_home: &Path, out_path: &str) -> tokio::process::Command {
         let mut cmd = tokio::process::Command::new(&self.attic);
         cmd.arg("push")
@@ -121,6 +124,16 @@ impl Remote {
     }
 }
 
+/// Whether `name` is a cache name attic accepts, which is
+/// `^[A-Za-z0-9][A-Za-z0-9-_+]{0,49}$`. `services.hyperhive.swarm.attic.cache`
+/// states the same rule in nix.
+fn cache_name_ok(name: &str) -> bool {
+    let mut bytes = name.bytes();
+    bytes.next().is_some_and(|b| b.is_ascii_alphanumeric())
+        && name.len() <= 50
+        && bytes.all(|b| b.is_ascii_alphanumeric() || matches!(b, b'-' | b'_' | b'+'))
+}
+
 /// Numbers the config directories of one process's pushes.
 static NEXT: std::sync::atomic::AtomicU64 = std::sync::atomic::AtomicU64::new(0);
 
@@ -128,14 +141,14 @@ static NEXT: std::sync::atomic::AtomicU64 = std::sync::atomic::AtomicU64::new(0)
 struct ConfigHome(PathBuf);
 
 impl ConfigHome {
-    /// `<parent>/push-<n>/attic/config.toml`, readable by this user alone.
+    /// `<parent>/push-<n>/attic/config.toml`, which names `token` beside it, both
+    /// readable by this user alone.
     fn write(parent: &Path, domain: &str, token: &str) -> std::io::Result<Self> {
         use std::io::Write as _;
         use std::os::unix::fs::{DirBuilderExt as _, OpenOptionsExt as _};
-        // A quote or a control character would end the TOML string early.
+        if token.is_empty() || token.bytes().any(|b| b < b' ') {
-        if token.is_empty() || token.bytes().any(|b| b == b'"' || b == b'\\' || b < b' ') {
             return Err(std::io::Error::other(
-                "the push token is empty or holds a character the config cannot",
+                "the push token is empty or holds a control character",
             ));
         }
         let home = Self(parent.join(format!(
@@ -144,19 +157,30 @@ impl ConfigHome {
             NEXT.fetch_add(1, std::sync::atomic::Ordering::Relaxed)
         )));
         let dir = home.0.join("attic");
+        let token_file = dir.join("token");
+        let token_path = token_file.to_string_lossy();
+        // A quote or backslash in the path would end the TOML string early.
+        if token_path.contains(['"', '\\']) {
+            return Err(std::io::Error::other(
+                "the runtime directory's path holds a quote or a backslash",
+            ));
+        }
         std::fs::DirBuilder::new()
             .recursive(true)
             .mode(0o700)
             .create(&dir)?;
-        let mut file = std::fs::OpenOptions::new()
+        let private = |name: &str| {
-            .write(true)
+            std::fs::OpenOptions::new()
-            .create_new(true)
+                .write(true)
-            .mode(0o600)
+                .create_new(true)
-            .open(dir.join("config.toml"))?;
+                .mode(0o600)
+                .open(dir.join(name))
+        };
+        private("token")?.write_all(token.as_bytes())?;
         write!(
-            file,
+            private("config.toml")?,
             "default-server = \"{SERVER}\"\n\n[servers.{SERVER}]\n\
-             endpoint = \"https://{domain}\"\ntoken = \"{token}\"\n"
+             endpoint = \"https://{domain}\"\ntoken-file = \"{token_path}\"\n"
         )?;
         Ok(home)
     }
@@ -378,19 +402,26 @@ mod tests {
     }
 
     #[test]
-    fn the_token_is_in_a_private_config_file_that_is_removed_after() {
+    fn the_token_is_in_a_private_file_the_config_names_and_both_are_removed_after() {
         let parent = std::env::temp_dir().join(format!("push-test-{}", std::process::id()));
         std::fs::create_dir_all(&parent).expect("parent");
         let home = ConfigHome::write(&parent, "cache.example.org", TOKEN).expect("written");
         let file = home.0.join("attic/config.toml");
         let text = std::fs::read_to_string(&file).expect("reads");
-        assert!(text.contains(&format!("token = \"{TOKEN}\"")), "{text}");
+        let token_file = home.0.join("attic/token");
+        assert!(
+            text.contains(&format!("token-file = \"{}\"", token_file.display())),
+            "{text}"
+        );
+        assert!(!text.contains(TOKEN), "{text}");
+        assert_eq!(std::fs::read_to_string(&token_file).expect("reads"), TOKEN);
         assert!(
             text.contains("endpoint = \"https://cache.example.org\""),
             "{text}"
         );
         let mode = |p: &Path| std::fs::metadata(p).expect("stat").permissions().mode() & 0o777;
         assert_eq!(mode(&file), 0o600);
+        assert_eq!(mode(&token_file), 0o600);
         assert_eq!(mode(&home.0), 0o700);
         let root = home.0.clone();
         drop(home);
@@ -399,13 +430,42 @@ mod tests {
     }
 
     #[test]
-    fn a_token_that_would_end_the_config_string_is_refused() {
+    fn an_empty_token_or_one_with_a_control_character_is_refused() {
         let parent = std::env::temp_dir();
-        for bad in ["", "a\"b", "a\\b", "a\nb"] {
+        for bad in ["", "a\nb", "a\0b"] {
             assert!(ConfigHome::write(&parent, "h", bad).is_err(), "{bad:?}");
         }
     }
 
+    #[test]
+    fn a_path_that_would_end_the_config_string_is_refused() {
+        let parent = std::env::temp_dir().join("a\"b");
+        assert!(ConfigHome::write(&parent, "h", TOKEN).is_err());
+        assert!(!parent.exists());
+    }
+
+    #[test]
+    fn the_cache_name_rule_is_attics() {
+        let fifty = "a".repeat(50);
+        for ok in ["grid", "a", "A-b_c+d", "0", fifty.as_str()] {
+            assert!(cache_name_ok(ok), "{ok:?}");
+        }
+        let fifty_one = "a".repeat(51);
+        for bad in [
+            "",
+            "grid.v2",
+            "-x",
+            "_x",
+            "+x",
+            "a:b",
+            "a b",
+            "a/b",
+            fifty_one.as_str(),
+        ] {
+            assert!(!cache_name_ok(bad), "{bad:?}");
+        }
+    }
+
     #[tokio::test]
     async fn a_path_that_is_not_a_store_path_is_not_pushed() {
         let cache = cache_with("true");
@@ -432,6 +492,9 @@ mod tests {
         assert!(Remote::from_settings(None, some("grid"), state()).is_err());
         assert!(Remote::from_settings(some("https://cache"), some("grid"), state()).is_err());
         assert!(Remote::from_settings(some("cache"), some("a\"b"), state()).is_err());
+        assert!(Remote::from_settings(some("cache"), some("grid.v2"), state()).is_err());
+        assert!(Remote::from_settings(some("cache"), some("-grid"), state()).is_err());
+        assert!(Remote::from_settings(some("cache"), some("g_r+i-d"), state()).is_ok());
         assert!(Remote::from_settings(some("cache"), some("grid"), None).is_err());
     }
 }