From ad58f1a8b17baa1e0e979fc8656110d0bdbe8f14 Mon Sep 17 00:00:00 2001 From: Hee Sung Son Date: Sat, 14 Nov 2026 17:39:27 +0900 Subject: [PATCH 01/14] docs(spec): specify the secret key path setting A new setting, host.secret_key_path, puts the host key outside the data directory. The host reads it and never creates it. The spec covers the setting and its checks, the start of the host, the key and secret commands, flats backup and its check of a copy, the docs and the compatibility with an older release. It is item 1 of #224. --- docs/spec/README.md | 1 + docs/spec/secret-key-path.md | 213 +++++++++++++++++++++++++++++++++++ 2 files changed, 214 insertions(+) create mode 100644 docs/spec/secret-key-path.md diff --git a/docs/spec/README.md b/docs/spec/README.md index 8132bbcc..e7449221 100644 --- a/docs/spec/README.md +++ b/docs/spec/README.md @@ -48,6 +48,7 @@ list. | [Autopilot](autopilot.md) | implemented | An opt-in mode in the new Settings › Experiments tab where agents approve or reject pending approvals for the operations the operator allows, with an operation guide for agents | | [Windows support](windows-support.md) | partly implemented | Flats on Windows: a PowerShell installer, a per-user service, Windows file names and locks, and a release archive | | [Secret key recovery](secret-key-recovery.md) | implemented | `flats secret backup-key`, `restore-key` and `reset`: a password-encrypted copy of `secret.key`, its restore against the sealed values, and a reset when both are lost | +| [Secret key path](secret-key-path.md) | planned | `host.secret_key_path` keeps the host key outside the data directory, for example in a Docker secret or a systemd credential: the host reads it and never creates it, and the key commands and `flats backup` use it | | [Public instance mode](public-instance.md) | implemented | An opt-in Experiments mode in which every new flat is created Public on the default public network and no private address is handed out or served | | [Host backup](host-backup.md) | implemented | `flats backup --out` copies a running or stopped host into a directory with a manifest, and `flats backup --verify` checks such a copy: databases as snapshots, write-once blobs in two passes, coherence of the outbox databases of documents, boards and notebooks with `flats.db`, and a self-check that fails to be run again | diff --git a/docs/spec/secret-key-path.md b/docs/spec/secret-key-path.md new file mode 100644 index 00000000..fe5856af --- /dev/null +++ b/docs/spec/secret-key-path.md @@ -0,0 +1,213 @@ +# Secret key path + +Status: planned. + +## Summary + +Flats seals every flat secret and every stored operator key under one 32-byte +key, `/secret.key`. A copy of the data directory holds the sealed values +and the key that opens them. This spec adds one setting, `host.secret_key_path`, +that puts the key in another place, such as a Docker secret or a systemd +credential. With the setting, the host never creates the key. It reads the file +or it refuses to start. + +The request is item 1 of issue #224. Items 2 to 4 (rotation, injected secrets, +audit events) are separate changes, and item 5 (the recovery password) is done +([secret key recovery](secret-key-recovery.md)). + +## Before this change + +- The host reads `/secret.key` at start (`New` in + `internal/core/service.go`). When the file is missing and `flats.db` holds no + sealed value, it creates the key (`loadOrCreateKey`). When the file is missing + and `flats.db` holds sealed values, it refuses to start (`CheckSecretKey`). +- Several places build the path `/secret.key` themselves: the start of the + host, `CheckSecretKey`, the sealing and opening of operator keys + (`SealHostKey`, `OpenHostKey`), `ReadSecretKey`, `RestoreSecretKey`, the + commands `flats key`, `flats secret backup-key`, `restore-key` and `reset`, + and `flats backup` with its check of a copy. +- `flats backup` reads the key at that fixed path. Its spec + ([host backup](host-backup.md), scope and rule 22) says that it must read the + key through the function that this item adds, and that a change of the key + during a copy fails the copy. +- [Configuration](../configuration.md#configjson) lists no key setting. + +## Scope + +In scope: the setting and its checks, the start of the host, every command that +reads or writes the key, `flats backup` and its check of a copy, the docs. + +Out of scope: + +- Key rotation, key IDs, audit events and injected `fetch` secrets (items 2 to + 4 of #224). A rotation that replaces the file during a run has no defined + effect until a restart. +- A check at start that the key opens the stored values. The default path has + none either, and a wrong key shows when a value is used. +- An environment variable, a `flats serve` flag or a console field for the + path. The setting is in `config.json` and is changed with `flats config`. +- A command that moves an existing key or makes a new one. The docs give the + steps and a shell line. +- Anything for the recovery copy. It stays at `/secret.key.recovery`. + +## The setting + +1. `host.secret_key_path` is a string. It is empty by default, and empty means + the key is `/secret.key` as before. A value must be an absolute path + without a NUL byte, as `host.data_dir` must. It is not a `flats serve` flag. + It applies at the next start, and `flats config set` and `unset` change it + offline, like the other host keys. Adding a key with a default leaves + `schema_version` at 5. +2. A value must not lie inside the data directory, and the default path + `/secret.key` is such a value, so it is refused too: the setting exists + to keep the key out of the data directory, and a key at another name inside it + would be copied by `flats backup` and kept by `--exclude-secret-key`. The + check compares the two cleaned absolute paths, and on Windows it ignores + case. It does not resolve links. It runs when the configuration is loaded, so + it also stops `flats config set host.data_dir` when the new directory contains + the key path. The message says to use a path outside `host.data_dir`, or to + leave the setting empty. +3. The location of the key is one function of the data directory and the setting. + Every place that the list under "Before this change" names gets the path from it. It + returns the path and whether the setting chose it. + +## The host + +4. With the setting empty, nothing changes. +5. With the setting, the host reads the file at the path and follows links, as + `ReadSecretKey` does for `secret.key`. It refuses to start, before any network + or worker starts and before it writes anything, when the path does not exist + (a link to nothing is that case), is not a regular file once links are + followed, is not 32 bytes, or cannot be read. The refusal names the path and + the setting. It holds when `flats.db` has no sealed value too. +6. The refusal for a missing file says that Flats never creates a key at a + configured path. It names three ways out: put a key there (the docs show + `head -c 32 /dev/urandom > PATH`), restore it with `flats secret restore-key`, + or clear the setting with `flats config unset host.secret_key_path`. It is a + refusal that exits with status 78, like `CheckSecretKey`. +7. With the setting, the host never creates, writes, renames or deletes the key + file, and it creates no temporary file beside it. A key on a read-only mount, + in a read-only folder or with mode 0444 works. The host does not check the mode + or the owner of the file. It must be readable by the user that runs the host, + which is uid 65532 in the container image. +8. The host reads the key once at start and keeps it in memory, as now. A change + of the file during the run has no effect until the next start. +9. Everything that seals or opens an operator key uses the same location: the + console, `flats key set`, the key inputs of `flats serve` + (`--tailscale-authkey-file`, `FLATS_TAILSCALE_AUTHKEY`) and the import of a + legacy key file. With the setting and no file, none of them creates a key; + each fails with the message of rule 6. +10. The warning at start that the recovery copy no longer matches the key + compares the copy with the key at this location. + +## The commands + +`flats key set`, `rm` and `ls`, and `flats secret backup-key`, `restore-key` and +`reset` find the data directory with `--config PATH` or `--data DIR`. Both ways +load the configuration file that they resolve, when it exists, so the setting +applies in both. With no configuration file, the key is `/secret.key`. + +11. `backup-key` reads the key at the location. The recovery copy and its ID file + stay in the data directory. +12. `restore-key` keeps its rules ([secret key recovery](secret-key-recovery.md), + rules 18 to 24): it runs with the host stopped, needs a missing key, checks + that the recovered key opens every stored value, writes only then, and never + replaces a file. With the setting it writes the key to the configured path. + When it cannot write there, for example on a read-only mount, it writes + nothing and says so, and names `--out`. +13. `restore-key --out FILE` writes the recovered key to the new file FILE + instead of the location, with mode 0600, after the same checks, and never + replaces a file. It makes no change at the location. The output says where + the host must find the key. FILE may not lie inside the data directory. + The operator moves FILE into the secret store and removes it. +14. `reset` keeps its rules (rules 25 to 31 there), and finds the key at the location. It refuses while + the key at the location opens the stored values. With the setting, it says + after it deleted the values that the next start needs a key at the path, + because Flats will not create one. +15. A key at the location that is not a usable key (a directory, a wrong size) + gets the message of the existing commands, with the path of the location. + +## Backup + +16. `flats backup` takes the location from the configuration file that it + resolves, as the commands above do. With the setting, the key is not in the + data directory, so the copy never holds it. The manifest records + `secret_key: "external"`, and the output says that the key is kept at the + path and must be backed up apart from the copy. The manifest keeps `format: + 1`: `external` is a new value of an existing field, and a `--verify` of an + older release reports any value other than `included` as a copy without a + key. +17. With the setting, the command reads the key at the location at step 13 and + again at step 18 of [host backup](host-backup.md), and fails the backup when + the two reads differ or the file is gone (rule 22 there). It runs the key + check of rule 28 there at step 20 against the sealed values of the copy. + A key that cannot be read before step 13 refuses the backup with a message + that names `--exclude-secret-key`. +18. With the setting, `--exclude-secret-key` means that the command does not + read the key: it skips the reads of rule 17 and the key check. The manifest + still records `external`. Without the setting the flag keeps its meaning + (rule 24 of host backup). +19. `flats backup --verify` needs no configuration. For a copy with `secret_key: + "external"` it reports, as a note and not as a failure, that the key is + outside the copy and that the sealed values were not checked against a key. +20. A restore (rule 35 of host backup) of a copy with `secret_key: "external"` + brings back `PATH/config.json`, which holds the setting, and the operator + puts the key at the configured path before the start. The docs say so. + +## Docs + +21. `docs/configuration.md` has the row of the setting, the key lifecycle (what + the key protects, what to back up and what to store apart, how to move an + existing key out of the data directory and back, and what a refusal means), + and the effect on a downgrade (rule 22). `docs/container.md` shows a Docker + secret and a Compose secret with `config set host.secret_key_path`. + `docs/design.md` ("Secrets") says where the key may be. The spec index lists + this page. The out-of-scope notes of [host backup](host-backup.md) and the + rules named above change as rules 16 to 20 say, and + [secret key recovery](secret-key-recovery.md) says that the commands use the + location. + +## Compatibility + +22. No database change, and `schema_version` stays 5. A host without the setting + behaves as before. An older Flats that reads a `config.json` with the new key + refuses it as an unknown key, as for any key it does not know. To go back, the + operator runs `flats config unset host.secret_key_path` with the new Flats + and puts the key at `/secret.key` first. The docs say so. +23. The key file format is unchanged: 32 raw bytes. + +## Completion criteria + +- Configuration tests: a relative path, a NUL byte and a path inside the data + directory are refused, with the value and the reason; the default path + `/secret.key` is refused; an empty value is accepted; `config set`, + `get` and `unset` round trip; `config set host.data_dir` refuses a directory + that contains the key path; every example file of an earlier schema still + migrates; `schema_version` stays 5. +- Host tests: with the setting, a missing file, a link to nothing, a directory, + a file of 31 and of 33 bytes and an unreadable file each stop the start, name + the path and the setting, and leave the data directory and the folder of the + key as they were, also when `flats.db` has no sealed value. A valid key starts + the host, in a read-only folder and with mode 0444, with no file created. The + host opens a sealed flat secret and a stored operator key with that key. With + the setting empty, the first start creates the key as before. +- A test shows that `flats key set` with the setting and no file refuses and + creates nothing. +- Command tests with `--config` and with `--data` and a configuration file: + `backup-key` reads the configured key; `restore-key` writes to the configured + path, never replaces a file, and with `--out` writes a new 0600 file and + changes nothing at the location, and refuses an `--out` inside the data + directory; with a read-only location it writes nothing and names `--out`; + `reset` refuses while the configured key opens the values, and after a reset + the next start refuses until a key is put at the path. +- Backup tests: the manifest has `external` and the copy holds no key; the output + names the path; a key that changes between step 13 and step 18 fails the copy; + `--exclude-secret-key` skips the reads and the check; an unreadable key refuses + the backup and names the flag; `--verify` reports the note; a copy restored + with the key put back at the path starts a host that opens every sealed value. +- Docs: `docs/configuration.md`, `docs/container.md` and `docs/design.md` say + what rules 21 and 22 list, and the documentation checks pass. + +## Open questions + +None. From 616114fb2466c1e7d67e1d0efe8a7321d7b8f305 Mon Sep 17 00:00:00 2001 From: Hee Sung Son Date: Sat, 14 Nov 2026 17:52:35 +0900 Subject: [PATCH 02/14] docs(spec): answer the design review of the secret key path spec A leftover secret.key or a key that resolves into the data directory is refused by the host, the key commands and flats backup, so a copy with an external key never holds it. The key checks run where CheckSecretKey runs today and exit with status 78. The reload lists the key as awaiting a restart. The text states what works for a systemd credential and a Docker secret, the order of a downgrade, the hand edit of an invalid file, the platform conditions of the tests, and the current-state section that the spec index asks for. --- docs/spec/secret-key-path.md | 285 ++++++++++++++++++++++------------- 1 file changed, 181 insertions(+), 104 deletions(-) diff --git a/docs/spec/secret-key-path.md b/docs/spec/secret-key-path.md index fe5856af..9759d8d7 100644 --- a/docs/spec/secret-key-path.md +++ b/docs/spec/secret-key-path.md @@ -15,27 +15,35 @@ The request is item 1 of issue #224. Items 2 to 4 (rotation, injected secrets, audit events) are separate changes, and item 5 (the recovery password) is done ([secret key recovery](secret-key-recovery.md)). -## Before this change +## Current state - The host reads `/secret.key` at start (`New` in `internal/core/service.go`). When the file is missing and `flats.db` holds no sealed value, it creates the key (`loadOrCreateKey`). When the file is missing - and `flats.db` holds sealed values, it refuses to start (`CheckSecretKey`). + and `flats.db` holds sealed values, it refuses to start (`CheckSecretKey`, + which `Start` in `internal/app/serve.go` calls before any network or worker + starts). - Several places build the path `/secret.key` themselves: the start of the host, `CheckSecretKey`, the sealing and opening of operator keys (`SealHostKey`, `OpenHostKey`), `ReadSecretKey`, `RestoreSecretKey`, the commands `flats key`, `flats secret backup-key`, `restore-key` and `reset`, and `flats backup` with its check of a copy. +- A wrong-size key at the default path is a plain error of `core.New` and exits + with status 1. A missing key over sealed values is a refusal with status 78. - `flats backup` reads the key at that fixed path. Its spec ([host backup](host-backup.md), scope and rule 22) says that it must read the key through the function that this item adds, and that a change of the key - during a copy fails the copy. + during a copy fails the copy. Its walk copies `secret.key` from the data + directory as a host file unless `--exclude-secret-key` is set. +- The key commands and `flats backup` need the configuration file. A missing or + invalid file is a refusal with status 78. - [Configuration](../configuration.md#configjson) lists no key setting. ## Scope In scope: the setting and its checks, the start of the host, every command that -reads or writes the key, `flats backup` and its check of a copy, the docs. +reads or writes the key, `flats backup` and its check of a copy, the SIGHUP +reload, the docs. Out of scope: @@ -48,54 +56,80 @@ Out of scope: path. The setting is in `config.json` and is changed with `flats config`. - A command that moves an existing key or makes a new one. The docs give the steps and a shell line. +- The installer and its systemd unit. The docs show a drop-in file. - Anything for the recovery copy. It stays at `/secret.key.recovery`. +- Changes to the default path, including its exit status for a wrong-size key. ## The setting 1. `host.secret_key_path` is a string. It is empty by default, and empty means the key is `/secret.key` as before. A value must be an absolute path without a NUL byte, as `host.data_dir` must. It is not a `flats serve` flag. - It applies at the next start, and `flats config set` and `unset` change it - offline, like the other host keys. Adding a key with a default leaves - `schema_version` at 5. -2. A value must not lie inside the data directory, and the default path - `/secret.key` is such a value, so it is refused too: the setting exists - to keep the key out of the data directory, and a key at another name inside it - would be copied by `flats backup` and kept by `--exclude-secret-key`. The - check compares the two cleaned absolute paths, and on Windows it ignores - case. It does not resolve links. It runs when the configuration is loaded, so - it also stops `flats config set host.data_dir` when the new directory contains - the key path. The message says to use a path outside `host.data_dir`, or to - leave the setting empty. -3. The location of the key is one function of the data directory and the setting. - Every place that the list under "Before this change" names gets the path from it. It - returns the path and whether the setting chose it. + `flats config set` and `unset` change it offline, like the other host keys, + and `flats config show` lists it. It applies at the next start (rule 8). + Adding a key with a default leaves `schema_version` at 5. +2. A value must not lie inside the data directory. The default path + `/secret.key` is such a value and is refused too, because the setting + exists to keep the key out of the data directory. The check compares the two + cleaned absolute paths, ignores case on Windows, and does not resolve links. + It runs when the configuration is loaded, so it also stops + `flats config set host.data_dir` when the new directory contains the key + path. `flats config set` and `unset` load the file first, so they cannot + repair a file that this check refuses. The message says to use a path outside + `host.data_dir` or to leave the setting empty, and to edit `config.json` by + hand. +3. The location of the key is one function of the data directory and the + setting. Every place that "Current state" names gets the path from it. It + returns the path and whether the setting chose it. With the setting, it + refuses in two more cases that the check of rule 2 cannot see. + - A file, a directory or a link exists at `/secret.key`. Two keys would + be in use, and a copy of the data directory would hold the one that the + setting was meant to keep out. The message says to move the file away. + - The key exists, and its path after the operating system resolved every link + lies inside the data directory after the same resolution. A data directory + that is a link, or a key path that is a link into the data directory, ends + here. The resolution does not catch every name that a file system can give + one file, such as a Windows short name, and the docs say to keep the key on + another directory. ## The host 4. With the setting empty, nothing changes. -5. With the setting, the host reads the file at the path and follows links, as - `ReadSecretKey` does for `secret.key`. It refuses to start, before any network - or worker starts and before it writes anything, when the path does not exist - (a link to nothing is that case), is not a regular file once links are - followed, is not 32 bytes, or cannot be read. The refusal names the path and - the setting. It holds when `flats.db` has no sealed value too. +5. With the setting, `Start` runs the checks of rule 3 and of the file at the + place where it calls `CheckSecretKey` today, before any network listener or + worker starts. It refuses when the path does not exist (a link to nothing is + that case), is not a regular file once links are followed, is not 32 bytes, + or cannot be read, and in the cases of rule 3. It holds when `flats.db` has no + sealed value too. Each refusal is an action error that exits with status 78, + so a service manager that does not restart on 78 does not loop on it. The + refusal names the path and the setting. 6. The refusal for a missing file says that Flats never creates a key at a - configured path. It names three ways out: put a key there (the docs show - `head -c 32 /dev/urandom > PATH`), restore it with `flats secret restore-key`, - or clear the setting with `flats config unset host.secret_key_path`. It is a - refusal that exits with status 78, like `CheckSecretKey`. + configured path. It names three ways out. The first is to put a key there; the + docs show `head -c 32 /dev/urandom > PATH`. The second is to restore it with + `flats secret restore-key`. The third is to clear the setting with + `flats config unset host.secret_key_path`. For a link to nothing, the message + names the link and says to repair the link or the mount, or to clear the + setting, and it does not name `restore-key`, which refuses anything at the + path (rule 12). 7. With the setting, the host never creates, writes, renames or deletes the key file, and it creates no temporary file beside it. A key on a read-only mount, in a read-only folder or with mode 0444 works. The host does not check the mode or the owner of the file. It must be readable by the user that runs the host, - which is uid 65532 in the container image. + which is uid 65532 in the container image. What `Start` writes before the + check is what it writes before `CheckSecretKey` today: the host lock, the + migration of `config.json` and `flats.db` with their copies, and the binding of + an interrupted init (rule 11 of [secret key recovery](secret-key-recovery.md)). + The setting adds no write to that list. 8. The host reads the key once at start and keeps it in memory, as now. A change - of the file during the run has no effect until the next start. -9. Everything that seals or opens an operator key uses the same location: the - console, `flats key set`, the key inputs of `flats serve` + of the file during the run has no effect until the next start. A SIGHUP reload + adopts a changed `host.secret_key_path` into its document and lists it as + awaiting a restart (`AwaitRestart`), like the other restart keys, and the + running host keeps the key it read at start. The key joins `restartKeys` in + `internal/core/reload.go` and rule 8 of [config reload](config-reload.md). +9. Everything that seals or opens an operator key uses the same location. That + means the console, `flats key set`, the key inputs of `flats serve` (`--tailscale-authkey-file`, `FLATS_TAILSCALE_AUTHKEY`) and the import of a - legacy key file. With the setting and no file, none of them creates a key; + legacy key file. With the setting and no file, none of them creates a key, and each fails with the message of rule 6. 10. The warning at start that the recovery copy no longer matches the key compares the copy with the key at this location. @@ -104,109 +138,152 @@ Out of scope: `flats key set`, `rm` and `ls`, and `flats secret backup-key`, `restore-key` and `reset` find the data directory with `--config PATH` or `--data DIR`. Both ways -load the configuration file that they resolve, when it exists, so the setting -applies in both. With no configuration file, the key is `/secret.key`. +load the configuration file that they resolve. A missing or invalid file is a +refusal with status 78, as today, so the setting applies in both ways. Every +command of this list gets the location from the function of rule 3 and refuses +in the cases of rule 3. 11. `backup-key` reads the key at the location. The recovery copy and its ID file stay in the data directory. 12. `restore-key` keeps its rules ([secret key recovery](secret-key-recovery.md), - rules 18 to 24): it runs with the host stopped, needs a missing key, checks - that the recovered key opens every stored value, writes only then, and never + rules 18 to 24). It runs with the host stopped, refuses when anything is at the + location (a file, a directory or a link, a dangling link included), checks that + the recovered key opens every stored value, writes only then, and never replaces a file. With the setting it writes the key to the configured path. - When it cannot write there, for example on a read-only mount, it writes - nothing and says so, and names `--out`. -13. `restore-key --out FILE` writes the recovered key to the new file FILE - instead of the location, with mode 0600, after the same checks, and never - replaces a file. It makes no change at the location. The output says where - the host must find the key. FILE may not lie inside the data directory. - The operator moves FILE into the secret store and removes it. -14. `reset` keeps its rules (rules 25 to 31 there), and finds the key at the location. It refuses while - the key at the location opens the stored values. With the setting, it says - after it deleted the values that the next start needs a key at the path, - because Flats will not create one. + Its synced temporary file `.secret-key-*` lies beside the configured path, not + in the data directory, which changes the place that rule 23 there names. When + the command cannot write there, for example on a read-only mount, it writes + nothing, removes any temporary file, and says so and names `--out`. +13. `restore-key --out FILE` writes the recovered key to the new file FILE instead + of the location, with mode 0600, after the checks of rule 12 that concern the + recovered key. It does not require the location to be empty, because the + location may hold a wrong key, and it makes no change at the location. It + never replaces a file. The output says where the host must find the key. FILE + may not lie inside the data directory. The operator moves FILE into the secret + store and removes it. +14. `reset` keeps its rules (rules 25 to 31 there), and finds the key at the + location. It refuses while the key at the location opens the stored values. + With the setting, it says after it deleted the values that the next start + needs a key at the path, because Flats will not create one. 15. A key at the location that is not a usable key (a directory, a wrong size) gets the message of the existing commands, with the path of the location. ## Backup 16. `flats backup` takes the location from the configuration file that it - resolves, as the commands above do. With the setting, the key is not in the - data directory, so the copy never holds it. The manifest records + resolves, as the commands above do, and refuses in the cases of rule 3 before + it writes anything. With the setting, the key is therefore not in the data + directory, and the copy never holds it. The manifest records `secret_key: "external"`, and the output says that the key is kept at the - path and must be backed up apart from the copy. The manifest keeps `format: - 1`: `external` is a new value of an existing field, and a `--verify` of an - older release reports any value other than `included` as a copy without a - key. + path and must be backed up apart from the copy. The manifest keeps + `format: 1`. `external` is a new value of an existing field, and a `--verify` + of an older release reports any value other than `included` as a copy without + a key. 17. With the setting, the command reads the key at the location at step 13 and again at step 18 of [host backup](host-backup.md), and fails the backup when - the two reads differ or the file is gone (rule 22 there). It runs the key - check of rule 28 there at step 20 against the sealed values of the copy. - A key that cannot be read before step 13 refuses the backup with a message - that names `--exclude-secret-key`. + the two reads differ or the file is gone (rule 22 there). At step 20 it checks + the key it read against the sealed values of the copy, as rule 28 there + does. `Run` passes the key bytes to that check, and the standalone `--verify` + has none (rule 19). A key that does not open the sealed values fails the + backup with the kind of failure that a second run cannot clear. A key that + cannot be read before step 13, because it is missing or unreadable, refuses + the backup, and the message names `--exclude-secret-key`. 18. With the setting, `--exclude-secret-key` means that the command does not - read the key: it skips the reads of rule 17 and the key check. The manifest + read the key. It skips the reads of rule 17 and the key check. The manifest still records `external`. Without the setting the flag keeps its meaning (rule 24 of host backup). -19. `flats backup --verify` needs no configuration. For a copy with `secret_key: - "external"` it reports, as a note and not as a failure, that the key is - outside the copy and that the sealed values were not checked against a key. +19. `flats backup --verify` needs no configuration. For a copy with + `secret_key: "external"` it reports, as a note and not as a failure, that the + key is outside the copy and that the sealed values were not checked against a + key. 20. A restore (rule 35 of host backup) of a copy with `secret_key: "external"` brings back `PATH/config.json`, which holds the setting, and the operator puts the key at the configured path before the start. The docs say so. +21. The key commands and `flats backup` run as processes of their own. A key + that only the service process can see, such as a systemd `LoadCredential=` + file under `$CREDENTIALS_DIRECTORY` or a Docker secret that is mounted into + the service container, is missing for them. For such a key, `flats backup` + needs `--exclude-secret-key`, `restore-key` needs `--out`, and the other key + commands cannot run unless the operator gives them the same mount. The docs + say this, and they show a deployment that works for every command, a plain + file that the service user can read, for example `/etc/flats/secret.key` with + mode 0400. ## Docs -21. `docs/configuration.md` has the row of the setting, the key lifecycle (what +22. `docs/configuration.md` has the row of the setting, the key lifecycle (what the key protects, what to back up and what to store apart, how to move an - existing key out of the data directory and back, and what a refusal means), - and the effect on a downgrade (rule 22). `docs/container.md` shows a Docker - secret and a Compose secret with `config set host.secret_key_path`. - `docs/design.md` ("Secrets") says where the key may be. The spec index lists - this page. The out-of-scope notes of [host backup](host-backup.md) and the - rules named above change as rules 16 to 20 say, and - [secret key recovery](secret-key-recovery.md) says that the commands use the - location. + existing key out of the data directory and back, what a refusal means, and the + hand edit that rule 2 asks for), and the effect on a downgrade (rule 24). + `docs/container.md` shows a Docker secret and a Compose secret with + `config set host.secret_key_path`. It also shows a systemd drop-in with + `LoadCredential=` and the limits of rule 21. `docs/design.md` ("Secrets") says + where the key may be. The spec index lists this page. The out-of-scope notes + and rules 22, 24 and 28 of [host backup](host-backup.md), rule 8 of + [config reload](config-reload.md) and rules 18 and 23 of + [secret key recovery](secret-key-recovery.md) change as this spec says. ## Compatibility -22. No database change, and `schema_version` stays 5. A host without the setting +23. No database change, and `schema_version` stays 5. A host without the setting behaves as before. An older Flats that reads a `config.json` with the new key - refuses it as an unknown key, as for any key it does not know. To go back, the - operator runs `flats config unset host.secret_key_path` with the new Flats - and puts the key at `/secret.key` first. The docs say so. -23. The key file format is unchanged: 32 raw bytes. + refuses it as an unknown key, as for any key it does not know. +24. To go back to an older Flats, the operator stops the host and copies the key + to `/secret.key`. Then the operator runs + `flats config unset host.secret_key_path` with the new Flats, and then starts + the older one. The order matters, because the new Flats refuses a file at + `/secret.key` while the setting is on (rule 3), and `config unset` is + not one of the commands that apply that refusal. +25. The key file format is unchanged: 32 raw bytes. ## Completion criteria -- Configuration tests: a relative path, a NUL byte and a path inside the data - directory are refused, with the value and the reason; the default path - `/secret.key` is refused; an empty value is accepted; `config set`, - `get` and `unset` round trip; `config set host.data_dir` refuses a directory - that contains the key path; every example file of an earlier schema still - migrates; `schema_version` stays 5. -- Host tests: with the setting, a missing file, a link to nothing, a directory, - a file of 31 and of 33 bytes and an unreadable file each stop the start, name - the path and the setting, and leave the data directory and the folder of the - key as they were, also when `flats.db` has no sealed value. A valid key starts - the host, in a read-only folder and with mode 0444, with no file created. The - host opens a sealed flat secret and a stored operator key with that key. With - the setting empty, the first start creates the key as before. +Tests that need a file the user cannot read, a read-only folder or mode 0444 run +on Unix as a user that is not the superuser, and skip elsewhere, as the existing +tests of `internal/app` do. + +- Configuration tests. A relative path, a NUL byte and a path inside the data + directory are refused, with the value and the reason. The default path + `/secret.key` is refused. An empty value is accepted. `config set`, + `get` and `unset` round trip. `config set host.data_dir` refuses a directory + that contains the key path. Every example file of an earlier schema still + migrates, and `schema_version` stays 5. An older release refuses a file with + the key as unknown. +- Host tests with the setting. A missing file, a link to nothing, a directory, a + file of 31 and of 33 bytes, an unreadable file, a file at `/secret.key`, + and a key that is a link into the data directory each stop the start before any + listener opens, exit with status 78, name the path and the setting, and leave no + `.secret-key-*` file and no change in the folder of the key. They hold when + `flats.db` has no sealed value. A data directory that is a link to the directory + that holds the key is refused as well. A valid key starts the host, in a + read-only folder and with mode 0444, with no file created. The host opens a + sealed flat secret and a stored operator key with that key. With the setting + empty, the first start creates the key as before. A SIGHUP reload with a changed + value lists the key as awaiting a restart, and the running host keeps its key. - A test shows that `flats key set` with the setting and no file refuses and creates nothing. -- Command tests with `--config` and with `--data` and a configuration file: - `backup-key` reads the configured key; `restore-key` writes to the configured - path, never replaces a file, and with `--out` writes a new 0600 file and - changes nothing at the location, and refuses an `--out` inside the data - directory; with a read-only location it writes nothing and names `--out`; - `reset` refuses while the configured key opens the values, and after a reset - the next start refuses until a key is put at the path. -- Backup tests: the manifest has `external` and the copy holds no key; the output - names the path; a key that changes between step 13 and step 18 fails the copy; - `--exclude-secret-key` skips the reads and the check; an unreadable key refuses - the backup and names the flag; `--verify` reports the note; a copy restored - with the key put back at the path starts a host that opens every sealed value. -- Docs: `docs/configuration.md`, `docs/container.md` and `docs/design.md` say - what rules 21 and 22 list, and the documentation checks pass. +- Command tests with `--config` and with `--data` and a configuration file. A + missing configuration file is a refusal. `backup-key` reads the configured key. + `restore-key` writes to the configured path, creates its temporary file beside + that path, never replaces a file, and with `--out` writes a new 0600 file, + changes nothing at the location, works when the location holds a wrong key, + and refuses an `--out` inside the data directory. With a read-only location it + writes nothing, leaves no temporary file and names `--out`. `reset` refuses + while the configured key opens the values, and after a reset the next start + refuses until a key is put at the path. Each key command refuses in the cases + of rule 3. +- Backup tests. The manifest has `external`, and no file of the copy holds the 32 + bytes of the key (a test scans every file of the copy). The output names the + path. A leftover `/secret.key` and a key that resolves into the data + directory refuse the backup before it writes anything. A key that changes + between step 13 and step 18 fails the copy. A key that does not open the sealed + values fails the backup and says that a second run does not help. + `--exclude-secret-key` skips the reads and the check. A missing or unreadable + key refuses the backup and names the flag. `--verify` reports the note. A copy + restored with the key put back at the path starts a host that opens every + sealed value. +- Docs. `docs/configuration.md`, `docs/container.md` and `docs/design.md` say + what rules 21, 22 and 24 list, and the documentation checks pass. ## Open questions From ffcb83ae8a3d448b79779cc1e20b77caf44af460 Mon Sep 17 00:00:00 2001 From: Hee Sung Son Date: Sat, 14 Nov 2026 17:59:59 +0900 Subject: [PATCH 03/14] docs(spec): correct the secret key path spec after design review round 2 Operator keys read the key file at each use, so the spec says which reads use the key in memory. The backup leaves out and scans for the key, and refuses a missing key in the preflight. The list of earlier writes, the cross references, the Docker secret sentence and the criteria are corrected. flats key ls and rm do not call the location function. --- docs/spec/secret-key-path.md | 107 +++++++++++++++++++++++------------ 1 file changed, 71 insertions(+), 36 deletions(-) diff --git a/docs/spec/secret-key-path.md b/docs/spec/secret-key-path.md index 9759d8d7..315d2bc1 100644 --- a/docs/spec/secret-key-path.md +++ b/docs/spec/secret-key-path.md @@ -88,9 +88,11 @@ Out of scope: - The key exists, and its path after the operating system resolved every link lies inside the data directory after the same resolution. A data directory that is a link, or a key path that is a link into the data directory, ends - here. The resolution does not catch every name that a file system can give - one file, such as a Windows short name, and the docs say to keep the key on - another directory. + here. The resolution does not unify every name that a file system can give + one file. A Windows short name, a hard link and a bind mount of the + directory of the key into the data directory escape it. The backup closes + that gap with its own scan (rule 16), and the docs say to keep the key in a + directory that no link and no mount reaches from the data directory. ## The host @@ -116,16 +118,26 @@ Out of scope: in a read-only folder or with mode 0444 works. The host does not check the mode or the owner of the file. It must be readable by the user that runs the host, which is uid 65532 in the container image. What `Start` writes before the - check is what it writes before `CheckSecretKey` today: the host lock, the - migration of `config.json` and `flats.db` with their copies, and the binding of - an interrupted init (rule 11 of [secret key recovery](secret-key-recovery.md)). - The setting adds no write to that list. -8. The host reads the key once at start and keeps it in memory, as now. A change - of the file during the run has no effect until the next start. A SIGHUP reload - adopts a changed `host.secret_key_path` into its document and lists it as - awaiting a restart (`AwaitRestart`), like the other restart keys, and the - running host keeps the key it read at start. The key joins `restartKeys` in - `internal/core/reload.go` and rule 8 of [config reload](config-reload.md). + check is what it writes before `CheckSecretKey` today. That is the host lock + and the Windows access list, the migration of `config.json` and `flats.db` + with their copies, the move of `network-provider.json` into `backups/`, and the + writes of an interrupted init or legacy migration, as rule 11 of + [secret key recovery](secret-key-recovery.md) lists them and as `prepare` makes + them. The setting adds no write to them. +8. Flat secrets use the key that `core.New` read at start and keeps in memory, + as now. Operator keys do not. `SealHostKey` and `OpenHostKey` read the key + file at each use, as now, for the console, `currentTailscaleKey` and the + enrolment of a waiting key. With the setting, those reads go through the + location of rule 3 and refuse in its cases. A file that is replaced during the + run changes what operator-key actions open at once, and a file that appears at + `/secret.key` during the run makes them refuse. A replacement of the key + during a run has no other defined effect (see "Scope"). The start reads the + file twice, in the check of rule 5 and in `core.New`, and `core.New` makes the + same checks through the same function and returns the same kind of refusal. + A SIGHUP reload adopts a changed `host.secret_key_path` into its document and + lists it as awaiting a restart (`AwaitRestart`), like the other restart keys, + and the running host keeps the key that it holds. The key joins `restartKeys` + in `internal/core/reload.go` and rule 8 of [config reload](config-reload.md). 9. Everything that seals or opens an operator key uses the same location. That means the console, `flats key set`, the key inputs of `flats serve` (`--tailscale-authkey-file`, `FLATS_TAILSCALE_AUTHKEY`) and the import of a @@ -140,8 +152,10 @@ Out of scope: `reset` find the data directory with `--config PATH` or `--data DIR`. Both ways load the configuration file that they resolve. A missing or invalid file is a refusal with status 78, as today, so the setting applies in both ways. Every -command of this list gets the location from the function of rule 3 and refuses -in the cases of rule 3. +command of this list that reads or writes the key (`key set`, `backup-key`, +`restore-key` and `reset`) gets the location from the function of rule 3 and +refuses in the cases of rule 3. `key ls` and `key rm` touch no key and do not +call it, so they still work when the location is in one of those states. 11. `backup-key` reads the key at the location. The recovery copy and its ID file stay in the data directory. @@ -171,9 +185,16 @@ in the cases of rule 3. ## Backup 16. `flats backup` takes the location from the configuration file that it - resolves, as the commands above do, and refuses in the cases of rule 3 before - it writes anything. With the setting, the key is therefore not in the data - directory, and the copy never holds it. The manifest records + resolves, as the commands above do, and refuses in the cases of rule 3 in the + preflight, before it writes anything. With the setting, the key is therefore not + in the data directory. While the setting is on, the walk leaves out + `/secret.key` as it does with `--exclude-secret-key` and lists it in + `excluded`, so a file that appears there during the copy is not copied. When + the command has read the key (rule 17), step 20 also scans every file of the + copy for the 32 bytes of the key, and a hit fails the backup with the kind of + failure that a second run cannot clear. That scan holds for every name that + a file system can give the key file, which the checks of rule 3 do not. The + manifest records `secret_key: "external"`, and the output says that the key is kept at the path and must be backed up apart from the copy. The manifest keeps `format: 1`. `external` is a new value of an existing field, and a `--verify` @@ -186,8 +207,9 @@ in the cases of rule 3. does. `Run` passes the key bytes to that check, and the standalone `--verify` has none (rule 19). A key that does not open the sealed values fails the backup with the kind of failure that a second run cannot clear. A key that - cannot be read before step 13, because it is missing or unreadable, refuses - the backup, and the message names `--exclude-secret-key`. + cannot be read, because it is missing or unreadable, refuses the backup in the + preflight (step 12), before the partial directory exists, and the message + names `--exclude-secret-key`. Step 13 reads the key again for rule 22. 18. With the setting, `--exclude-secret-key` means that the command does not read the key. It skips the reads of rule 17 and the key check. The manifest still records `external`. Without the setting the flag keeps its meaning @@ -201,8 +223,10 @@ in the cases of rule 3. puts the key at the configured path before the start. The docs say so. 21. The key commands and `flats backup` run as processes of their own. A key that only the service process can see, such as a systemd `LoadCredential=` - file under `$CREDENTIALS_DIRECTORY` or a Docker secret that is mounted into - the service container, is missing for them. For such a key, `flats backup` + file under `$CREDENTIALS_DIRECTORY`, or a Docker secret that is mounted into + the service container, is missing for a process outside the service. A + command that runs inside the container with `docker exec` sees the secret. A + one-off container and a process on the host do not. For such a key, `flats backup` needs `--exclude-secret-key`, `restore-key` needs `--out`, and the other key commands cannot run unless the operator gives them the same mount. The docs say this, and they show a deployment that works for every command, a plain @@ -219,9 +243,10 @@ in the cases of rule 3. `config set host.secret_key_path`. It also shows a systemd drop-in with `LoadCredential=` and the limits of rule 21. `docs/design.md` ("Secrets") says where the key may be. The spec index lists this page. The out-of-scope notes - and rules 22, 24 and 28 of [host backup](host-backup.md), rule 8 of - [config reload](config-reload.md) and rules 18 and 23 of - [secret key recovery](secret-key-recovery.md) change as this spec says. + and rules 12, 22, 24, 28, 30, 31 and 35 of [host backup](host-backup.md), rule + 8 of [config reload](config-reload.md), and rules 19, 23 and 31 of + [secret key recovery](secret-key-recovery.md) change as this spec says. Rule + 12 here changes rules 19 and 23 there, and rule 14 here changes rule 31. ## Compatibility @@ -244,11 +269,12 @@ tests of `internal/app` do. - Configuration tests. A relative path, a NUL byte and a path inside the data directory are refused, with the value and the reason. The default path - `/secret.key` is refused. An empty value is accepted. `config set`, - `get` and `unset` round trip. `config set host.data_dir` refuses a directory - that contains the key path. Every example file of an earlier schema still - migrates, and `schema_version` stays 5. An older release refuses a file with - the key as unknown. + `/secret.key` is refused. An empty value is accepted. `config set` and + `unset` round trip, and `config show` lists the key. `config set host.data_dir` + refuses a directory that contains the key path. Every example file of an + earlier schema still migrates, and `schema_version` stays 5. The parser refuses + an unknown key in the `host` object, which is the rule that makes an older + release refuse the new key. The tests build no older binary. - Host tests with the setting. A missing file, a link to nothing, a directory, a file of 31 and of 33 bytes, an unreadable file, a file at `/secret.key`, and a key that is a link into the data directory each stop the start before any @@ -275,13 +301,22 @@ tests of `internal/app` do. - Backup tests. The manifest has `external`, and no file of the copy holds the 32 bytes of the key (a test scans every file of the copy). The output names the path. A leftover `/secret.key` and a key that resolves into the data - directory refuse the backup before it writes anything. A key that changes - between step 13 and step 18 fails the copy. A key that does not open the sealed - values fails the backup and says that a second run does not help. - `--exclude-secret-key` skips the reads and the check. A missing or unreadable - key refuses the backup and names the flag. `--verify` reports the note. A copy - restored with the key put back at the path starts a host that opens every + directory refuse the backup before it writes anything. A hard link to the key + under another name in the data directory fails the backup at step 20 through + the scan of rule 16, with the kind of failure that a second run does not clear. + A `/secret.key` that appears during the copy is not copied and is listed + in `excluded`. A key that changes between step 13 and step 18 fails the copy. A + key that does not open the sealed values fails the backup and says that a + second run does not help. `--exclude-secret-key` skips the reads, the scan and + the check. A missing or unreadable key refuses the backup in the preflight, + makes no partial directory, and names the flag. `--verify` reports the note. A + copy restored with the key put back at the path starts a host that opens every sealed value. +- Operator-key tests with the setting. The console and `flats key set` open and + seal with the configured file at each use. Replacing the file between two uses + changes what they open, and a file that appears at `/secret.key` makes + them refuse (rule 8). `flats key ls` and `flats key rm` work while the location + is in a state that rule 3 refuses. - Docs. `docs/configuration.md`, `docs/container.md` and `docs/design.md` say what rules 21, 22 and 24 list, and the documentation checks pass. From 39872e9510950b6765ca90e063f270b178a8ccb2 Mon Sep 17 00:00:00 2001 From: Hee Sung Son Date: Sat, 14 Nov 2026 18:03:28 +0900 Subject: [PATCH 04/14] feat(config): add host.secret_key_path The setting keeps the host key in another file than /secret.key. It is empty by default, must be an absolute path, and must not lie inside the data directory. The check runs when the file is loaded, so config set refuses a data directory that contains the key path. schema_version stays 5. --- internal/config/config.go | 22 +++- internal/config/config_test.go | 4 +- internal/config/keypath.go | 29 ++++ internal/config/secretkeypath_test.go | 182 ++++++++++++++++++++++++++ 4 files changed, 234 insertions(+), 3 deletions(-) create mode 100644 internal/config/keypath.go create mode 100644 internal/config/secretkeypath_test.go diff --git a/internal/config/config.go b/internal/config/config.go index a123b9cb..c4f70c1f 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -140,6 +140,10 @@ var specs = []spec{ hint: "use the lowercase UUID written by config init"}, {key: "host.data_dir", kind: kindString, required: true, check: checkAbsPath, hint: "use an absolute path"}, + // The host key lives at /secret.key unless this names another file. With a path the + // host never creates the key (docs/spec/secret-key-path.md). + {key: "host.secret_key_path", kind: kindString, def: "", check: checkKeyPath, + hint: "use an absolute path outside host.data_dir, or leave it empty to keep the key in the data directory"}, {key: "host.console_addr", kind: kindString, def: "127.0.0.1:7878", check: checkAddr, override: true, hint: "use host:port with a port from 0 to 65535, such as 127.0.0.1:7878"}, {key: "host.local_addr", kind: kindString, def: "127.0.0.1:7879", check: checkAddr, override: true, @@ -494,6 +498,15 @@ func NewInstanceID() string { return fmt.Sprintf("%x-%x-%x-%x-%x", b[0:4], b[4:6], b[6:8], b[8:10], b[10:16]) } +// checkKeyPath accepts an empty value, which keeps the key in the data directory, +// and otherwise an absolute path. +func checkKeyPath(v any) (any, error) { + if v.(string) == "" { + return "", nil + } + return checkAbsPath(v) +} + func checkAbsPath(v any) (any, error) { s := v.(string) if strings.IndexByte(s, 0) >= 0 { @@ -774,6 +787,8 @@ type HostConfig struct { ConsoleAddr string LocalAddr string ServerRuntime bool + // SecretKeyPath is host.secret_key_path: "" keeps the key at /secret.key. + SecretKeyPath string } type TailscaleConfig struct { @@ -993,7 +1008,7 @@ func (c *Config) fill() error { list := func(k string) []string { return slices.Clone(c.values[k].([]string)) } c.Host = HostConfig{str("host.instance_id"), cleanDataDir(str("host.data_dir")), str("host.console_addr"), - str("host.local_addr"), flag("host.server_runtime")} + str("host.local_addr"), flag("host.server_runtime"), cleanKeyPath(str("host.secret_key_path"))} c.Tailscale = TailscaleConfig{list("tailscale.console_hosts")} c.Network = NetworkConfig{list("network.permitted"), str("network.private_backend"), str("network.default_private"), str("network.default_public")} @@ -1039,6 +1054,11 @@ func (c *Config) fill() error { errs = append(errs, fieldErr("network.default_public", fmt.Sprintf(`add %q to network.permitted or clear the default`, p), "%s is not permitted", p)) } + if p := c.Host.SecretKeyPath; p != "" && insideDir(c.Host.DataDir, p) { + errs = append(errs, fieldErr("host.secret_key_path", + "use a path outside host.data_dir, or leave it empty to keep the key in the data directory; if this file is invalid, edit config.json by hand", + "%s lies inside host.data_dir %s", p, c.Host.DataDir)) + } errs = append(errs, c.checkProxy()...) if !c.Portal.Discovery && len(c.Portal.Relays) == 0 { errs = append(errs, fieldErr("portal.discovery", "list portal.relays or set discovery to true", diff --git a/internal/config/config_test.go b/internal/config/config_test.go index aef3074b..b5901516 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -233,7 +233,7 @@ func TestEffectiveDefaults(t *testing.T) { if err != nil { t.Fatal(err) } - want := HostConfig{testID, cleanDataDir("/var/lib/flats"), "127.0.0.1:7878", "127.0.0.1:7879", true} + want := HostConfig{testID, cleanDataDir("/var/lib/flats"), "127.0.0.1:7878", "127.0.0.1:7879", true, ""} if c.Host != want { t.Errorf("Host = %+v, want %+v", c.Host, want) } @@ -329,7 +329,7 @@ func TestOverrides(t *testing.T) { if err != nil { t.Fatal(err) } - want := HostConfig{testID, cleanDataDir("/var/lib/flats"), "127.0.0.1:18000", "[::1]:18001", false} + want := HostConfig{testID, cleanDataDir("/var/lib/flats"), "127.0.0.1:18000", "[::1]:18001", false, ""} if c.Host != want { t.Errorf("Host = %+v, want %+v", c.Host, want) } diff --git a/internal/config/keypath.go b/internal/config/keypath.go new file mode 100644 index 00000000..cccb7d6e --- /dev/null +++ b/internal/config/keypath.go @@ -0,0 +1,29 @@ +package config + +import ( + "path/filepath" + "runtime" + "strings" +) + +// cleanKeyPath returns host.secret_key_path in clean form, or "" for no path. +func cleanKeyPath(p string) string { + if p == "" { + return "" + } + return filepath.Clean(p) +} + +// insideDir reports whether child is dir or lies below it. It compares the two +// cleaned paths and does not resolve links. On Windows it ignores case. +func insideDir(dir, child string) bool { + dir, child = filepath.Clean(dir), filepath.Clean(child) + if runtime.GOOS == "windows" { + dir, child = strings.ToLower(dir), strings.ToLower(child) + } + rel, err := filepath.Rel(dir, child) + if err != nil { + return false + } + return rel == "." || (rel != ".." && !strings.HasPrefix(rel, ".."+string(filepath.Separator))) +} diff --git a/internal/config/secretkeypath_test.go b/internal/config/secretkeypath_test.go new file mode 100644 index 00000000..3903b445 --- /dev/null +++ b/internal/config/secretkeypath_test.go @@ -0,0 +1,182 @@ +package config + +import ( + "path/filepath" + "runtime" + "strings" + "testing" +) + +// host.secret_key_path keeps the host key out of the data directory (docs/spec/secret-key-path.md, rules 1 and 2). +func TestSecretKeyPathValidation(t *testing.T) { + t.Parallel() + const key = "/etc/flats/secret.key" + l, err := Parse([]byte(fileWith("host", `"secret_key_path": "`+key+`"`))) + if err != nil { + t.Fatal(err) + } + c, err := l.Doc.Effective(nil) + if err != nil { + t.Fatal(err) + } + if want := filepath.Clean(key); c.Host.SecretKeyPath != want { + t.Errorf("SecretKeyPath = %q, want %q", c.Host.SecretKeyPath, want) + } + for name, tc := range map[string]struct { + file string + empty bool // the effective value is empty + }{ + // Empty is the default and keeps the key in the data directory. + "absent": {fileWith(), true}, + "empty": {fileWith("host", `"secret_key_path": ""`), true}, + // A sibling whose name starts with the data directory's name is outside it. + "sibling": {fileWith("host", `"secret_key_path": "/var/lib/flats-keys/secret.key"`), false}, + // Clean resolves the dots before the comparison. + "dots": {fileWith("host", `"secret_key_path": "/var/lib/flats/../keys/secret.key"`), false}, + } { + l, err := Parse([]byte(tc.file)) + if err != nil { + t.Errorf("%s: %v", name, err) + continue + } + c, err := l.Doc.Effective(nil) + if err != nil { + t.Errorf("%s: %v", name, err) + continue + } + if (c.Host.SecretKeyPath == "") != tc.empty { + t.Errorf("%s: SecretKeyPath = %q", name, c.Host.SecretKeyPath) + } + } + for name, value := range map[string]string{ + "relative": "keys/secret.key", + "NUL byte": `/etc/flats/secret\u0000.key`, + "inside": "/var/lib/flats/keys/secret.key", + "the default path": "/var/lib/flats/secret.key", + "the data dir": "/var/lib/flats", + "dots lead inside": "/etc/../var/lib/flats/secret.key", + "trailing slashes": "/var/lib/flats/keys//", + "inside, any depth": "/var/lib/flats/a/b/c/d", + } { + _, err := Parse([]byte(fileWith("host", `"secret_key_path": "`+value+`"`))) + if err == nil { + t.Errorf("%s: Parse accepted %q", name, value) + continue + } + if got := errPath(err); got != "host.secret_key_path" { + t.Errorf("%s: error path = %q, want host.secret_key_path (%v)", name, got, err) + } + } +} + +// The message of a refused path says what to do and that the file is edited by hand (rule 2). +func TestSecretKeyPathMessage(t *testing.T) { + t.Parallel() + _, err := Parse([]byte(fileWith("host", `"secret_key_path": "/var/lib/flats/secret.key"`))) + if err == nil { + t.Fatal("Parse accepted a key path inside the data directory") + } + for _, want := range []string{"outside host.data_dir", "leave it empty", "edit config.json by hand"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("the message lacks %q: %v", want, err) + } + } +} + +func TestSecretKeyPathSetUnset(t *testing.T) { + t.Parallel() + doc, err := New(testID, "/var/lib/flats") + if err != nil { + t.Fatal(err) + } + if err := doc.Set("host.secret_key_path", "/etc/flats/secret.key"); err != nil { + t.Fatal(err) + } + if err := doc.Validate(); err != nil { + t.Fatal(err) + } + if !strings.Contains(string(doc.Encode()), `"secret_key_path": "/etc/flats/secret.key"`) { + t.Errorf("Encode does not hold the key path:\n%s", doc.Encode()) + } + if !strings.Contains(string(doc.Encode()), `"schema_version": 5`) { + t.Errorf("the schema version changed:\n%s", doc.Encode()) + } + // Set checks one key. Validate runs the rule that spans two keys: a new data directory that contains the key path. + if err := doc.Set("host.data_dir", "/etc/flats"); err != nil { + t.Fatal(err) + } + if err := doc.Validate(); errPath(err) != "host.secret_key_path" { + t.Errorf("Validate = %v, want a refusal at host.secret_key_path", err) + } + if err := doc.Set("host.data_dir", "/var/lib/flats"); err != nil { + t.Fatal(err) + } + if err := doc.Set("host.secret_key_path", "/var/lib/flats/secret.key"); err != nil { + t.Fatal(err) + } + if err := doc.Validate(); errPath(err) != "host.secret_key_path" { + t.Errorf("Validate accepted the default path: %v", err) + } + if err := doc.Unset("host.secret_key_path"); err != nil { + t.Fatal(err) + } + if strings.Contains(string(doc.Encode()), "secret_key_path") { + t.Errorf("Unset left the key:\n%s", doc.Encode()) + } + if err := doc.Validate(); err != nil { + t.Errorf("Validate after Unset: %v", err) + } +} + +// `flats config set host.data_dir` saves through the writer, which validates the whole file. A directory that +// contains the key path is refused, and the file and its history stay as they were (rule 2). +func TestSaveRefusesADataDirThatContainsTheKeyPath(t *testing.T) { + t.Parallel() + dir := t.TempDir() + path := filepath.Join(dir, "config.json") + orig := fileWith("host", `"secret_key_path": "/etc/flats/secret.key"`) + hash := writeFile(t, path, orig) + _, err := NewWriter(path).Save(hash, func(d *Document) error { return d.Set("host.data_dir", "/etc/flats") }) + if errPath(err) != "host.secret_key_path" { + t.Fatalf("Save = %v, want a refusal at host.secret_key_path", err) + } + if readString(t, path) != orig || historyFiles(t, dir) != nil { + t.Error("a refused save changed the file or its history") + } +} + +// An older release refuses the new key because the parser refuses any key it does not know (rule 23). The test +// builds no older binary. It shows that the rule holds in the host object. +func TestUnknownHostKeyIsRefused(t *testing.T) { + t.Parallel() + if _, err := Parse([]byte(fileWith("host", `"secret_key_file": "/etc/flats/secret.key"`))); err == nil { + t.Error("Parse accepted an unknown key in the host object") + } +} + +func TestInsideDir(t *testing.T) { + t.Parallel() + cases := []struct { + dir, child string + want bool + }{ + {"/a/b", "/a/b", true}, + {"/a/b", "/a/b/c", true}, + {"/a/b", "/a/b/../b/c", true}, + {"/a/b", "/a/bc", false}, + {"/a/b", "/a", false}, + {"/a/b", "/x/a/b", false}, + } + for _, tc := range cases { + if got := insideDir(filepath.FromSlash(tc.dir), filepath.FromSlash(tc.child)); got != tc.want { + t.Errorf("insideDir(%q, %q) = %v, want %v", tc.dir, tc.child, got, tc.want) + } + } + if runtime.GOOS == "windows" { + if !insideDir(`C:\Data\Flats`, `c:\data\flats\SECRET.KEY`) { + t.Error("Windows paths that differ in case only are different") + } + } else if insideDir("/Data/Flats", "/data/flats/secret.key") { + t.Error("paths that differ in case are the same") + } +} From c9cf5b2e30263467d9ddfcd62be3b5b22982e321 Mon Sep 17 00:00:00 2001 From: Hee Sung Son Date: Sat, 14 Nov 2026 18:18:11 +0900 Subject: [PATCH 05/14] feat(core): read the host key from host.secret_key_path A KeyFile value says where the host key lives, and every place that read or wrote /secret.key now gets the path from it. With the setting, the host reads the file and never creates it: a missing or linked-to-nothing file, a file that is no key, a second key at /secret.key and a key that resolves into the data directory are all refusals with status 78, and they run where CheckSecretKey runs. The key commands, flats backup and the console use the same location, restore-key gets --out, a SIGHUP reload lists the setting as awaiting a restart, and flats backup records the key as external, leaves the data directory's secret.key out and scans the copy for the key bytes. --- internal/app/backupcmd.go | 10 +- internal/app/keycmd.go | 19 ++-- internal/app/keys.go | 23 +++-- internal/app/keys_test.go | 12 +-- internal/app/netbird.go | 4 +- internal/app/secretkeycmd.go | 94 ++++++++++++++----- internal/app/secretkeycmd_test.go | 2 +- internal/app/serve.go | 13 ++- internal/app/setup.go | 4 +- internal/backup/backup.go | 53 +++++++++-- internal/backup/copy.go | 48 ++++++++++ internal/backup/manifest.go | 2 + internal/backup/verify.go | 46 ++++++--- internal/core/console_names_test.go | 14 +-- internal/core/hostkeys.go | 21 ++--- internal/core/keyfile.go | 140 ++++++++++++++++++++++++++++ internal/core/keyrecovery.go | 15 --- internal/core/keyrecovery_test.go | 24 ++--- internal/core/reload.go | 2 +- internal/core/reload_test.go | 23 +++++ internal/core/secrets.go | 12 ++- internal/core/service.go | 22 +++-- internal/core/settings_test.go | 4 +- 23 files changed, 469 insertions(+), 138 deletions(-) create mode 100644 internal/core/keyfile.go diff --git a/internal/app/backupcmd.go b/internal/app/backupcmd.go index 3c48bcf8..b06a2163 100644 --- a/internal/app/backupcmd.go +++ b/internal/app/backupcmd.go @@ -49,12 +49,16 @@ func (b *backupCmd) run(out string, exclude bool) error { if err != nil { return err } - dataDir, err := b.dataDir() + dataDir, keyFile, err := b.location() if err != nil { return err } + keyPath := "" // host.secret_key_path, when the setting is on + if keyFile.Configured() { + keyPath = keyFile.Path() + } res, err := backup.Run(b.ctx, backup.Options{ - DataDir: dataDir, ConfigPath: configPath, Out: out, + DataDir: dataDir, ConfigPath: configPath, Out: out, SecretKeyPath: keyPath, ExcludeSecretKey: exclude, FlatsVersion: buildinfo.Get().Version, }) if err != nil { @@ -71,6 +75,8 @@ func (b *backupCmd) report(dataDir string, res *backup.Result) { switch res.SecretKey { case backup.SecretKeyIncluded: b.printf(" secret.key is in the copy\n") + case backup.SecretKeyExternal: + b.printf(" the secret key is kept at %s, outside the data directory, and is not in the copy; back it up apart from the copy\n", res.KeyPath) case backup.SecretKeyExcluded: b.printf(" secret.key is not in the copy; the recovery copy %s stays, so `flats secret restore-key` can bring the key back\n", "secret.key.recovery") default: diff --git a/internal/app/keycmd.go b/internal/app/keycmd.go index 961e6c2f..3c03643d 100644 --- a/internal/app/keycmd.go +++ b/internal/app/keycmd.go @@ -61,7 +61,7 @@ func KeyCommand(ctx context.Context, args []string, in io.Reader, out io.Writer) } } - dataDir, st, done, err := c.openOffline() + dataDir, keyFile, st, done, err := c.openOffline() if err != nil { return err } @@ -81,10 +81,10 @@ func KeyCommand(ctx context.Context, args []string, in io.Reader, out io.Writer) if err := core.CheckHostKeyValue(id, value); err != nil { return &ActionError{err} } - if err := core.CheckSecretKey(ctx, dataDir, st); err != nil { + if err := core.CheckSecretKey(ctx, keyFile, st); err != nil { return &ActionError{err} } - if err := core.PutHostKeyValue(ctx, dataDir, st, id, value, core.KeySourceCLI, time.Now()); err != nil { + if err := core.PutHostKeyValue(ctx, keyFile, st, id, value, core.KeySourceCLI, time.Now()); err != nil { return err } switch id { @@ -101,19 +101,20 @@ func KeyCommand(ctx context.Context, args []string, in io.Reader, out io.Writer) // openOffline takes the host lock of the selected configuration's data // directory, checks that flats.db belongs to that configuration, and opens // it. done closes the store and releases the lock. -func (c *configCmd) openOffline() (dataDir string, st *store.Store, done func(), err error) { +func (c *configCmd) openOffline() (dataDir string, keyFile core.KeyFile, st *store.Store, done func(), err error) { configPath, _, err := c.paths() if err != nil { - return "", nil, nil, err + return "", core.KeyFile{}, nil, nil, err } _, cfg, err := load(configPath) if err != nil { - return "", nil, nil, err + return "", core.KeyFile{}, nil, nil, err } dataDir = cfg.Host.DataDir + keyFile = core.NewKeyFile(dataDir, cfg.Host.SecretKeyPath) lock, err := lockOffline(dataDir) if err != nil { - return "", nil, nil, err + return "", core.KeyFile{}, nil, nil, err } dbPath := filepath.Join(dataDir, "flats.db") info, err := inspectDB(dbPath) @@ -125,9 +126,9 @@ func (c *configCmd) openOffline() (dataDir string, st *store.Store, done func(), } if err != nil { lock.Close() - return "", nil, nil, err + return "", core.KeyFile{}, nil, nil, err } - return dataDir, st, func() { st.Close(); lock.Close() }, nil + return dataDir, keyFile, st, func() { st.Close(); lock.Close() }, nil } // readKeyValue prompts without echo on a terminal. diff --git a/internal/app/keys.go b/internal/app/keys.go index 2e746acf..12524774 100644 --- a/internal/app/keys.go +++ b/internal/app/keys.go @@ -173,22 +173,22 @@ func readKeyInputs(o Options, legacy bool, deprecatedFile string) (keyInputs, er var keyLabel = map[string]string{core.HostKeyTailscale: "Tailscale auth key", core.HostKeyNetbird: "NetBird setup key"} // resolveTailscaleKey applies the seed rule to the Tailscale auth key. -func resolveTailscaleKey(ctx context.Context, dataDir string, st *store.Store, in keyInput, now time.Time, logf func(string, ...any)) (key string, differs bool, err error) { - return resolveStoredKey(ctx, dataDir, st, core.HostKeyTailscale, in, now, logf) +func resolveTailscaleKey(ctx context.Context, keyFile core.KeyFile, st *store.Store, in keyInput, now time.Time, logf func(string, ...any)) (key string, differs bool, err error) { + return resolveStoredKey(ctx, keyFile, st, core.HostKeyTailscale, in, now, logf) } // resolveStoredKey applies the seed rule to a key Flats keeps (Tailscale, // NetBird): an external value is stored only when flats.db holds no key of // that kind. A stored key always wins; a different external value is // reported. It returns the key to use. -func resolveStoredKey(ctx context.Context, dataDir string, st *store.Store, id string, in keyInput, now time.Time, logf func(string, ...any)) (key string, differs bool, err error) { - stored, _, err := core.StoredHostKey(ctx, dataDir, st, id) +func resolveStoredKey(ctx context.Context, keyFile core.KeyFile, st *store.Store, id string, in keyInput, now time.Time, logf func(string, ...any)) (key string, differs bool, err error) { + stored, _, err := core.StoredHostKey(ctx, keyFile, st, id) if err != nil { return "", false, &ActionError{err} } switch { case stored == "" && in.given(): - if err := core.PutHostKeyValue(ctx, dataDir, st, id, in.value, in.source, now); err != nil { + if err := core.PutHostKeyValue(ctx, keyFile, st, id, in.value, in.source, now); err != nil { return "", false, err } logf("stored the %s from %s in flats.db", keyLabel[id], in.from) @@ -319,7 +319,7 @@ func (h *Host) SetTailscaleKey(ctx context.Context, key string) error { } h.keyMu.Lock() defer h.keyMu.Unlock() - if err := core.PutHostKeyValue(ctx, h.Config.Host.DataDir, h.Store, core.HostKeyTailscale, key, core.KeySourceConsole, time.Now()); err != nil { + if err := core.PutHostKeyValue(ctx, h.keyFile(), h.Store, core.HostKeyTailscale, key, core.KeySourceConsole, time.Now()); err != nil { return err } h.tsKeyDiffers = h.tsKeyExternal != "" && !core.SameKey(h.tsKeyExternal, key) @@ -362,7 +362,7 @@ func (h *Host) SetNetbirdKey(ctx context.Context, key string) error { return fmt.Errorf("%w: %v", core.ErrUnavailable, err) } h.keyMu.Lock() - if err := core.PutHostKeyValue(ctx, h.Config.Host.DataDir, h.Store, core.HostKeyNetbird, key, core.KeySourceConsole, time.Now()); err != nil { + if err := core.PutHostKeyValue(ctx, h.keyFile(), h.Store, core.HostKeyNetbird, key, core.KeySourceConsole, time.Now()); err != nil { h.keyMu.Unlock() return err } @@ -408,7 +408,7 @@ func (h *Host) currentNetbirdKey() string { // currentTailscaleKey is the stored key, for a backend the console turns on. func (h *Host) currentTailscaleKey(ctx context.Context) (string, error) { - v, _, err := core.StoredHostKey(ctx, h.Config.Host.DataDir, h.Store, core.HostKeyTailscale) + v, _, err := core.StoredHostKey(ctx, h.keyFile(), h.Store, core.HostKeyTailscale) return v, err } @@ -455,7 +455,7 @@ func (h *Host) enrollWaiting(ctx context.Context, in keyInputs) { {core.HostKeyZrok, in.zrok, h.connectZrok, h.zrokEnrollable}, } { connected, why := e.usable() - stored, row, err := core.StoredHostKey(ctx, h.Config.Host.DataDir, h.Store, e.id) + stored, row, err := core.StoredHostKey(ctx, h.keyFile(), h.Store, e.id) if err != nil { h.logf("read the waiting %s key: %v", e.id, err) stored = "" @@ -521,3 +521,8 @@ func (h *Host) zrokEnrollable() (bool, string) { // deprecatedAuthKeyFile is credentials.tailscale_authkey_file of the config // this run starts from. func deprecatedAuthKeyFile(c *config.Config) string { return c.Credentials.TailscaleAuthKeyFile } + +// keyFile is where the host key lives: /secret.key, or the file that host.secret_key_path names. +func (h *Host) keyFile() core.KeyFile { + return core.NewKeyFile(h.Config.Host.DataDir, h.Config.Host.SecretKeyPath) +} diff --git a/internal/app/keys_test.go b/internal/app/keys_test.go index d2b190ae..5bcdde43 100644 --- a/internal/app/keys_test.go +++ b/internal/app/keys_test.go @@ -146,11 +146,11 @@ func TestResolveTailscaleKeySeeds(t *testing.T) { ctx := context.Background() var logs []string logf := func(f string, a ...any) { logs = append(logs, fmt.Sprintf(f, a...)) } - key, differs, err := resolveTailscaleKey(ctx, dir, st, keyInput{}, time.Now(), logf) + key, differs, err := resolveTailscaleKey(ctx, core.NewKeyFile(dir, ""), st, keyInput{}, time.Now(), logf) if err != nil || key != "" || differs { t.Fatalf("empty: %q %v %v", key, differs, err) } - key, differs, err = resolveTailscaleKey(ctx, dir, st, keyInput{keyA, core.KeySourceEnv, "FLATS_TAILSCALE_AUTHKEY"}, time.Now(), logf) + key, differs, err = resolveTailscaleKey(ctx, core.NewKeyFile(dir, ""), st, keyInput{keyA, core.KeySourceEnv, "FLATS_TAILSCALE_AUTHKEY"}, time.Now(), logf) if err != nil || key != keyA || differs { t.Fatalf("seed: %q %v %v", key, differs, err) } @@ -158,17 +158,17 @@ func TestResolveTailscaleKeySeeds(t *testing.T) { t.Fatalf("source = %q", k.Source) } // The console replaced it; the unchanged env value no longer wins. - if err := core.PutHostKeyValue(ctx, dir, st, core.HostKeyTailscale, keyB, core.KeySourceConsole, time.Now()); err != nil { + if err := core.PutHostKeyValue(ctx, core.NewKeyFile(dir, ""), st, core.HostKeyTailscale, keyB, core.KeySourceConsole, time.Now()); err != nil { t.Fatal(err) } - key, differs, err = resolveTailscaleKey(ctx, dir, st, keyInput{keyA, core.KeySourceEnv, "FLATS_TAILSCALE_AUTHKEY"}, time.Now(), logf) + key, differs, err = resolveTailscaleKey(ctx, core.NewKeyFile(dir, ""), st, keyInput{keyA, core.KeySourceEnv, "FLATS_TAILSCALE_AUTHKEY"}, time.Now(), logf) if err != nil || key != keyB || !differs { t.Fatalf("stored wins: %q %v %v", key, differs, err) } if k, _ := st.GetHostKey(ctx, core.HostKeyTailscale); k.Source != core.KeySourceConsole { t.Fatalf("the external value overwrote the stored key: %q", k.Source) } - key, differs, _ = resolveTailscaleKey(ctx, dir, st, keyInput{keyB, core.KeySourceFlag, "--tailscale-authkey-file"}, time.Now(), logf) + key, differs, _ = resolveTailscaleKey(ctx, core.NewKeyFile(dir, ""), st, keyInput{keyB, core.KeySourceFlag, "--tailscale-authkey-file"}, time.Now(), logf) if key != keyB || differs { t.Fatalf("same value: %q %v", key, differs) } @@ -367,7 +367,7 @@ func storedTailscaleKey(t *testing.T, dir string) string { t.Fatal(err) } defer st.Close() - v, _, err := core.StoredHostKey(context.Background(), dir, st, core.HostKeyTailscale) + v, _, err := core.StoredHostKey(context.Background(), core.NewKeyFile(dir, ""), st, core.HostKeyTailscale) if err != nil { t.Fatal(err) } diff --git a/internal/app/netbird.go b/internal/app/netbird.go index 5244de63..59151a35 100644 --- a/internal/app/netbird.go +++ b/internal/app/netbird.go @@ -27,7 +27,7 @@ const legacyPeerFile = "legacy-peer.json" // prepareNetbird runs at start before the NetBird backend is built: it drops // the shared peer of earlier versions and resolves the setup key with the // seed rule. -func (h *Host) prepareNetbird(ctx context.Context, dataDir string, st *store.Store, in keyInput, logf func(string, ...any)) error { +func (h *Host) prepareNetbird(ctx context.Context, dataDir string, keyFile core.KeyFile, st *store.Store, in keyInput, logf func(string, ...any)) error { dir := filepath.Join(dataDir, "netbird") if d, err := provider.LinkedNetbird(); err == nil { p, found, err := d.CleanupLegacy(dir) @@ -54,7 +54,7 @@ func (h *Host) prepareNetbird(ctx context.Context, dataDir string, st *store.Sto } else if err != nil && !errors.Is(err, store.ErrNotFound) { return err } - key, differs, err := resolveStoredKey(ctx, dataDir, st, core.HostKeyNetbird, in, time.Now(), logf) + key, differs, err := resolveStoredKey(ctx, keyFile, st, core.HostKeyNetbird, in, time.Now(), logf) if err != nil { return err } diff --git a/internal/app/secretkeycmd.go b/internal/app/secretkeycmd.go index 7a703c71..305fbeba 100644 --- a/internal/app/secretkeycmd.go +++ b/internal/app/secretkeycmd.go @@ -61,15 +61,19 @@ func (k *secretKeyCmd) backup(args []string) error { if err := k.parseFlags(args); err != nil { return err } - dataDir, err := k.dataDir() + dataDir, keyFile, err := k.location() if err != nil { return err } - keyPath := filepath.Join(dataDir, core.SecretKeyFile) - if _, err := os.Lstat(keyPath); errors.Is(err, fs.ErrNotExist) { + keyPath := keyFile.Path() + if _, err := os.Lstat(keyPath); errors.Is(err, fs.ErrNotExist) && !keyFile.Configured() { return actionf("%s is missing: start the host once to create it, or restore it with `flats secret restore-key`", keyPath) } - key, err := core.ReadSecretKey(dataDir) + // With a configured path, Read makes the checks of rule 3 and names the path and the setting in each refusal. + key, err := keyFile.Read() + if keyFile.Configured() && err != nil { + return &ActionError{err} + } if keyUnusable(err, keyPath) { return actionf("%v: move %s away first, because the next start would stop on it", err, keyPath) } @@ -158,16 +162,27 @@ func (k *secretKeyCmd) restore(args []string) error { f := k.flags("restore-key") from := f.String("from", "", "recovery file (default: /secret.key.recovery)") passwordFile := f.String("password-file", "", "read the recovery password from this file") + outFlag := f.String("out", "", "write the recovered key to this new file instead of the key path") if err := k.parseFlags(args); err != nil { return err } - dataDir, st, done, err := k.openOffline() + dataDir, keyFile, st, done, err := k.openOffline() if err != nil { return err } defer done() - keyPath := filepath.Join(dataDir, core.SecretKeyFile) - if _, err := os.Lstat(keyPath); err == nil { + keyPath := keyFile.Path() + // A second key at /secret.key is a refusal, with or without --out. + if err := keyFile.Check(); err != nil { + return &ActionError{err} + } + outPath := "" + if *outFlag != "" { + // --out leaves the location alone, which may hold a wrong key. + if outPath, err = keyOutFile(*outFlag, dataDir); err != nil { + return err + } + } else if _, err := os.Lstat(keyPath); err == nil { return actionf("%s exists, and restore-key never replaces a key: move it away first if it is wrong", keyPath) } else if !errors.Is(err, fs.ErrNotExist) { return &ActionError{err} @@ -206,23 +221,48 @@ func (k *secretKeyCmd) restore(args []string) error { if err := k.ctx.Err(); err != nil { return interrupted(err) } - if err := core.RestoreSecretKey(dataDir, key); err != nil { + if outPath != "" { + if err := writeNewFile(outPath, key); err != nil { + return fmt.Errorf("--out %s: %w; nothing was written at the key path", outPath, err) + } + k.printf("wrote the recovered key to %s with mode 0600 from %s; the host reads its key at %s. Put the key there, or in the secret store behind that path, and remove %s\n", outPath, src, keyPath, outPath) + return nil + } + if err := keyFile.Restore(key); err != nil { if os.IsExist(err) { return actionf("%s appeared while restoring, and restore-key never replaces a key; nothing was written", keyPath) } + if keyFile.Configured() { + return actionf("cannot write the key at %s: %v; nothing was written. Write it to a new file with --out FILE, and put that file in your secret store", keyPath, err) + } return err } k.printf("restored %s from %s; the host can start\n", keyPath, src) return nil } +// keyOutFile checks --out of restore-key: the checks of outFile, and the file may not lie +// anywhere inside the data directory, which would put the key back into what a copy holds. +func keyOutFile(flag, dataDir string) (string, error) { + path, err := outFile(flag, dataDir) + if err != nil { + return "", err + } + dir, derr := filepath.EvalSymlinks(filepath.Dir(path)) + data, serr := filepath.EvalSymlinks(dataDir) + if derr == nil && serr == nil && core.PathInside(data, dir) { + return "", actionf("--out: name a file outside the data directory %s, because a copy of the data directory would hold the key", dataDir) + } + return path, nil +} + func (k *secretKeyCmd) reset(args []string) error { f := k.flags("reset") yes := f.Bool("yes", false, "delete without asking") if err := k.parseFlags(args); err != nil { return err } - dataDir, st, done, err := k.openOffline() + dataDir, keyFile, st, done, err := k.openOffline() if err != nil { return err } @@ -249,8 +289,8 @@ func (k *secretKeyCmd) reset(args []string) error { k.printf("flats.db holds no flat secrets or operator keys; nothing to reset\n") return nil } - keyPath := filepath.Join(dataDir, core.SecretKeyFile) - keyMissing, err := k.resetAllowed(st, dataDir) + keyPath := keyFile.Path() + keyMissing, err := k.resetAllowed(st, keyFile) if err != nil { return err } @@ -295,7 +335,9 @@ func (k *secretKeyCmd) reset(args []string) error { } else { k.printf("deleted %d values.\n", total) } - if keyMissing { + if keyMissing && keyFile.Configured() { + k.printf("The next start needs a key at %s, because Flats never creates a key at a configured path. Put a new key there, for example with `head -c 32 /dev/urandom > %s`, before you start the host.\n", keyPath, keyPath) + } else if keyMissing { why := "to keep a recovery copy of the new key" if _, err := os.Lstat(filepath.Join(dataDir, core.RecoveryKeyFile)); err == nil { why = "because the existing " + core.RecoveryKeyFile + " holds the lost key" @@ -309,16 +351,23 @@ func (k *secretKeyCmd) reset(args []string) error { return nil } -// resetAllowed checks what is at secret.key: reset goes on only when nothing -// is there, or when a valid key there opens none of the sealed values. -func (k *secretKeyCmd) resetAllowed(st *store.Store, dataDir string) (keyMissing bool, err error) { - keyPath := filepath.Join(dataDir, core.SecretKeyFile) +// resetAllowed checks what is at the key's path: reset goes on only when nothing +// is there, or when a valid key there opens none of the sealed values. With a +// configured path it first makes the checks of rule 3 of the spec. +func (k *secretKeyCmd) resetAllowed(st *store.Store, keyFile core.KeyFile) (keyMissing bool, err error) { + keyPath := keyFile.Path() + if err := keyFile.Check(); err != nil { + return false, &ActionError{err} + } if _, err := os.Lstat(keyPath); errors.Is(err, fs.ErrNotExist) { return true, nil } else if err != nil { return false, err } - key, err := core.ReadSecretKey(dataDir) + key, err := keyFile.Read() + if keyFile.Configured() && err != nil { + return false, &ActionError{err} + } if keyUnusable(err, keyPath) { return false, actionf("%v: move %s away first, because the next start would stop on it", err, keyPath) } @@ -354,17 +403,18 @@ func (k *secretKeyCmd) resetAllowed(st *store.Store, dataDir string) (keyMissing return false, nil } -// dataDir resolves the data directory of the selected configuration. -func (c *configCmd) dataDir() (string, error) { +// location resolves the data directory and the host key of the selected +// configuration. A missing or invalid configuration file is a refusal. +func (c *configCmd) location() (string, core.KeyFile, error) { configPath, _, err := c.paths() if err != nil { - return "", err + return "", core.KeyFile{}, err } _, cfg, err := load(configPath) if err != nil { - return "", err + return "", core.KeyFile{}, err } - return cfg.Host.DataDir, nil + return cfg.Host.DataDir, core.NewKeyFile(cfg.Host.DataDir, cfg.Host.SecretKeyPath), nil } // password reads the recovery password from --password-file or the diff --git a/internal/app/secretkeycmd_test.go b/internal/app/secretkeycmd_test.go index 17786c1c..f3ad5edf 100644 --- a/internal/app/secretkeycmd_test.go +++ b/internal/app/secretkeycmd_test.go @@ -87,7 +87,7 @@ func hostWithSecrets(t *testing.T, dir string) (string, []byte, *Host) { if err := h.Svc.SetSecret(ctx, "app", "API_KEY", "hunter2-value", core.ViaCLI); err != nil { t.Fatal(err) } - if err := core.PutHostKeyValue(ctx, dir, h.Store, core.HostKeyZrok, "zrok-token", core.KeySourceCLI, time.Now()); err != nil { + if err := core.PutHostKeyValue(ctx, core.NewKeyFile(dir, ""), h.Store, core.HostKeyZrok, "zrok-token", core.KeySourceCLI, time.Now()); err != nil { t.Fatal(err) } key, err := os.ReadFile(filepath.Join(dir, core.SecretKeyFile)) diff --git a/internal/app/serve.go b/internal/app/serve.go index 567ecfe2..a1a6aa63 100644 --- a/internal/app/serve.go +++ b/internal/app/serve.go @@ -409,6 +409,7 @@ func Start(ctx context.Context, o Options) (*Host, error) { logf("warning: flats serve runs without --config; its flags match %s. Reinstall the service with `flats install --config %s` so it runs from the config file alone", p.path, p.path) } dataDir := cfg.Host.DataDir + keyFile := core.NewKeyFile(dataDir, cfg.Host.SecretKeyPath) st := p.st // The environment, the secret key and the credential files are checked @@ -419,8 +420,10 @@ func Start(ctx context.Context, o Options) (*Host, error) { return nil, err } } - if err := core.CheckSecretKey(ctx, dataDir, st); err != nil { - if errors.Is(err, core.ErrSecretKeyMissing) { + if err := core.CheckSecretKey(ctx, keyFile, st); err != nil { + // A configured key that cannot be used is a refusal too, so a service manager that does not + // restart on status 78 does not loop on it (docs/spec/secret-key-path.md, rule 5). + if errors.Is(err, core.ErrSecretKeyMissing) || errors.Is(err, core.ErrKeyLocation) { return nil, &ActionError{err} } return nil, err @@ -430,7 +433,7 @@ func Start(ctx context.Context, o Options) (*Host, error) { if err != nil { return nil, err } - authKey, differs, err := resolveTailscaleKey(ctx, dataDir, st, keys.tailscale, time.Now(), logf) + authKey, differs, err := resolveTailscaleKey(ctx, keyFile, st, keys.tailscale, time.Now(), logf) if err != nil { return nil, err } @@ -491,7 +494,7 @@ func Start(ctx context.Context, o Options) (*Host, error) { } h.netbirdOptions = provider.NetbirdConfig{Dir: filepath.Join(dataDir, "netbird"), ManagementURL: cfg.Netbird.ManagementURL, SetupKey: h.currentNetbirdKey, Instance: cfg.Host.InstanceID, Logf: logf} - if err := h.prepareNetbird(ctx, dataDir, st, keys.netbird, logf); err != nil { + if err := h.prepareNetbird(ctx, dataDir, keyFile, st, keys.netbird, logf); err != nil { return nil, err } if permits(provider.Netbird) || permits(provider.NetbirdExpose) { @@ -596,7 +599,7 @@ func Start(ctx context.Context, o Options) (*Host, error) { return nil, fmt.Errorf("listen %s: %w", cfg.Host.ConsoleAddr, err) } h.console = managementConsoleURL(h.ln.Addr()) - coreCfg := core.Config{DocsApp: core.DocsApp{FS: docsapp.FS(), Hash: docsapp.Hash(), Entry: docsapp.Entry, ContentModule: docsapp.ContentModule}, DataDir: dataDir, Store: st, Private: h.Private, Lifecycle: mgr, Runtime: rt, Settings: settings, + coreCfg := core.Config{DocsApp: core.DocsApp{FS: docsapp.FS(), Hash: docsapp.Hash(), Entry: docsapp.Entry, ContentModule: docsapp.ContentModule}, DataDir: dataDir, SecretKeyPath: cfg.Host.SecretKeyPath, Store: st, Private: h.Private, Lifecycle: mgr, Runtime: rt, Settings: settings, ConsoleURL: h.ConsoleURL, ConsoleURLFor: h.ConsoleURLFor, PrivateBackend: cfg.Network.PrivateBackend, // "flats" is the console's usual tailnet name: kept free for it, as before. Reserved: []string{"flats"}, ReservedNames: h.consoleReserved, Logf: logf} diff --git a/internal/app/setup.go b/internal/app/setup.go index 17f3624a..0d6bb47f 100644 --- a/internal/app/setup.go +++ b/internal/app/setup.go @@ -947,9 +947,9 @@ func importLegacyKey(ctx context.Context, dataDir string, st *store.Store, plan if err := core.CheckHostKeyValue(core.HostKeyTailscale, v); err != nil { return actionf("%s: %v", from, err) } - stored, _, err := core.StoredHostKey(ctx, dataDir, st, core.HostKeyTailscale) + stored, _, err := core.StoredHostKey(ctx, core.NewKeyFile(dataDir, ""), st, core.HostKeyTailscale) if err != nil || stored != "" { return err } - return core.PutHostKeyValue(ctx, dataDir, st, core.HostKeyTailscale, v, core.KeySourceFlag, time.Now()) + return core.PutHostKeyValue(ctx, core.NewKeyFile(dataDir, ""), st, core.HostKeyTailscale, v, core.KeySourceFlag, time.Now()) } diff --git a/internal/backup/backup.go b/internal/backup/backup.go index 825e036b..c338ae97 100644 --- a/internal/backup/backup.go +++ b/internal/backup/backup.go @@ -16,6 +16,7 @@ import ( "strings" "time" + "github.com/gosuda/flats/internal/core" "github.com/gosuda/flats/internal/notebook" "github.com/gosuda/flats/internal/store" ) @@ -25,7 +26,8 @@ type Options struct { DataDir string // the data directory of the host ConfigPath string // the configuration file, "" when the host has none Out string // the new folder to write - ExcludeSecretKey bool // leave secret.key out (rule 24) + ExcludeSecretKey bool // leave secret.key out (rule 24); with SecretKeyPath, do not read the key + SecretKeyPath string // host.secret_key_path, "" for the key at /secret.key FlatsVersion string // recorded in the manifest Now func() time.Time @@ -47,6 +49,7 @@ type Result struct { Retries int // the retries of rules 4 to 7 Attempts int // the attempts at coherence of rule 16, at least 1 SecretKey string + KeyPath string // the external key's path, when SecretKey is "external" Skipped []Skip Unstable []string } @@ -111,6 +114,20 @@ func Run(ctx context.Context, o Options) (res *Result, err error) { if isUnder(final, data) { return nil, &RefusedError{fmt.Sprintf("the data directory %s lies inside --out %s", data, out)} } + // A configured key (docs/spec/secret-key-path.md, rules 16 and 17): the checks of rule 3, and a key that + // can be read unless --exclude-secret-key says that the command does not read it. These refusals come + // before the partial directory exists. + keyFile := core.NewKeyFile(data, o.SecretKeyPath) + if keyFile.Configured() { + if err := keyFile.Check(); err != nil { + return nil, &RefusedError{err.Error()} + } + if !o.ExcludeSecretKey { + if _, err := keyFile.Read(); err != nil { + return nil, &RefusedError{fmt.Sprintf("%v; --exclude-secret-key makes the backup skip the key", err)} + } + } + } // Last, because it is the one refusal that opens a database, and that can create a -shm file (rule 11). if err := checkSchema(flatsDB); err != nil { return nil, err @@ -136,7 +153,8 @@ func Run(ctx context.Context, o Options) (res *Result, err error) { os.RemoveAll(partial) } }() - c := newCopier(ctx, data, partial, o.ExcludeSecretKey) + // With a configured key the walk leaves out /secret.key as well (rule 16): the key is not part of the data. + c := newCopier(ctx, data, partial, o.ExcludeSecretKey || keyFile.Configured()) c.afterSnap, c.midCopy, c.order = o.afterSnapshot, o.midCopy, o.order if o.maxGroup > 0 { c.maxGroup = o.maxGroup @@ -156,7 +174,7 @@ func Run(ctx context.Context, o Options) (res *Result, err error) { if err != nil { return nil, err } - key0, err := readKeyBytes(data, o.ExcludeSecretKey) + key0, err := readKey(keyFile, o.ExcludeSecretKey) if err != nil { return nil, err } @@ -206,12 +224,15 @@ func Run(ctx context.Context, o Options) (res *Result, err error) { if d := s1.diff(s2); d != "" { return nil, &RerunError{fmt.Errorf("a flat was created, renamed, deleted or restored during the copy (%s)", d)} } - key1, err := readKeyBytes(data, o.ExcludeSecretKey) + key1, err := readKey(keyFile, o.ExcludeSecretKey) if err != nil { + if keyFile.Configured() { + return nil, &RerunError{fmt.Errorf("the secret key at %s cannot be read any more: %w", keyFile.Path(), err)} + } return nil, err } if !bytes.Equal(key0, key1) { - return nil, &RerunError{errors.New("secret.key changed during the copy")} + return nil, &RerunError{fmt.Errorf("the secret key at %s changed during the copy", keyFile.Path())} } if err := o.hook(19); err != nil { return nil, err @@ -223,7 +244,12 @@ func Run(ctx context.Context, o Options) (res *Result, err error) { m.Created = o.Now().UTC().Format(time.RFC3339) m.Attempts = attempts m.Config = configState + // The bytes of a configured key go to the check of step 20: it opens the sealed values of the copy with + // them, and it scans every file of the copy for them (rule 16 and 17 of the spec of the key path). + var externalKey []byte switch { + case keyFile.Configured(): + m.SecretKey, externalKey = SecretKeyExternal, key0 case o.ExcludeSecretKey: m.SecretKey = SecretKeyExcluded case key0 != nil: @@ -231,7 +257,7 @@ func Run(ctx context.Context, o Options) (res *Result, err error) { default: m.SecretKey = SecretKeyAbsent } - if fails, _, err := verifyTree(ctx, partial, m); err != nil { + if fails, _, err := verifyTree(ctx, partial, m, externalKey); err != nil { return nil, err } else if len(fails) > 0 { err := fmt.Errorf("the copy failed its own check: %s: %s", fails[0].Path, fails[0].Reason) @@ -265,6 +291,9 @@ func Run(ctx context.Context, o Options) (res *Result, err error) { published = true res = &Result{Out: final, Files: len(m.Files), Attempts: attempts, Retries: c.retries, SecretKey: m.SecretKey, Skipped: m.Skipped, Unstable: m.Unstable} + if m.SecretKey == SecretKeyExternal { + res.KeyPath = keyFile.Path() + } for _, f := range m.Files { res.Bytes += f.Size } @@ -290,6 +319,18 @@ func checkSchema(flatsDB string) error { return nil } +// readKey returns the bytes of the host key, or nil when the key is excluded or the default file is not +// there. A configured key must be readable, and it is not read at all when it is excluded. +func readKey(k core.KeyFile, excluded bool) ([]byte, error) { + if excluded { + return nil, nil + } + if k.Configured() { + return k.Read() + } + return readKeyBytes(k.DataDir(), false) +} + // readKeyBytes returns the bytes of secret.key, following a link as the host // does, or nil when the file is not there or the key is excluded. func readKeyBytes(data string, excluded bool) ([]byte, error) { diff --git a/internal/backup/copy.go b/internal/backup/copy.go index 852afe7c..188f1b56 100644 --- a/internal/backup/copy.go +++ b/internal/backup/copy.go @@ -1,6 +1,7 @@ package backup import ( + "bytes" "context" "crypto/sha256" "encoding/hex" @@ -257,3 +258,50 @@ func syncTree(root string) error { } return nil } + +// hashScan returns the size and SHA-256 of the file at path, and reports whether the file holds the +// bytes of needle anywhere. A nil needle scans for nothing. It reads the file once, through the pooled +// buffer, and keeps the last len(needle)-1 bytes of each read so that a hit across two reads is found. +func hashScan(path string, needle []byte) (File, bool, error) { + if len(needle) == 0 { + f, err := hashFile(path) + return f, false, err + } + in, err := os.Open(path) + if err != nil { + return File{}, false, err + } + defer in.Close() + h := sha256.New() + bp := bufPool.Get().(*[]byte) + defer bufPool.Put(bp) + buf := *bp + keep := len(needle) - 1 + var size int64 + var found bool + carry := 0 // bytes at the start of buf that come from the earlier read + for { + n, rerr := in.Read(buf[carry:]) + if n > 0 { + h.Write(buf[carry : carry+n]) + size += int64(n) + total := carry + n + if !found && bytes.Contains(buf[:total], needle) { + found = true + } + if total > keep { + copy(buf, buf[total-keep:total]) + carry = keep + } else { + carry = total + } + } + if rerr == io.EOF { + break + } + if rerr != nil { + return File{}, false, rerr + } + } + return File{Size: size, SHA256: hex.EncodeToString(h.Sum(nil))}, found, nil +} diff --git a/internal/backup/manifest.go b/internal/backup/manifest.go index f0bd0d11..48daf5c2 100644 --- a/internal/backup/manifest.go +++ b/internal/backup/manifest.go @@ -23,6 +23,8 @@ const ( SecretKeyIncluded = "included" SecretKeyExcluded = "excluded" SecretKeyAbsent = "absent" + // SecretKeyExternal: host.secret_key_path keeps the key outside the data directory, so the copy never holds it. + SecretKeyExternal = "external" ) // Values of Manifest.Config. diff --git a/internal/backup/verify.go b/internal/backup/verify.go index 3ec9b913..c7f8a6d8 100644 --- a/internal/backup/verify.go +++ b/internal/backup/verify.go @@ -44,7 +44,7 @@ func Verify(ctx context.Context, root string) (*VerifyResult, error) { if err != nil { return nil, err } - fails, notes, err := verifyTree(ctx, root, m) + fails, notes, err := verifyTree(ctx, root, m, nil) if err != nil { return nil, err } @@ -54,8 +54,12 @@ func Verify(ctx context.Context, root string) (*VerifyResult, error) { type verifier struct { root, data string m *Manifest - fails []Failure - notes []string + // extKey is the key of a copy whose key is outside it (secret_key "external"), when the caller has + // the bytes: Run does, a standalone Verify does not. The check opens the sealed values with it, and + // every file of the copy is scanned for it (docs/spec/secret-key-path.md, rules 16 and 17). + extKey []byte + fails []Failure + notes []string } func (v *verifier) fail(path, format string, args ...any) { @@ -63,9 +67,10 @@ func (v *verifier) fail(path, format string, args ...any) { } // verifyTree runs the checks of rules 25 to 28 on the copy in root against the -// manifest m, which Run holds in memory before it writes manifest.json. -func verifyTree(ctx context.Context, root string, m *Manifest) ([]Failure, []string, error) { - v := &verifier{root: root, data: filepath.Join(root, "data"), m: m} +// manifest m, which Run holds in memory before it writes manifest.json. extKey is the bytes of an +// external key, or nil. +func verifyTree(ctx context.Context, root string, m *Manifest, extKey []byte) ([]Failure, []string, error) { + v := &verifier{root: root, data: filepath.Join(root, "data"), m: m, extKey: extKey} flatsDB := filepath.Join(v.data, nameFlatsDB) flatsHere, sidecar := false, false if _, err := os.Stat(flatsDB); err == nil { @@ -151,10 +156,14 @@ func (v *verifier) checkFiles(ctx context.Context) error { return nil } seen[rel] = true - got, err := hashFile(p) + got, holdsKey, err := hashScan(p, v.extKey) if err != nil { return err } + if holdsKey { + // A second run cannot clear this: a name for the key file got into the copy. + v.fails = append(v.fails, Failure{Path: rel, Reason: "holds the bytes of the secret key, which the copy must not hold", Fixed: true}) + } switch { case got.Size != f.Size: v.fail(rel, "has %d bytes, the manifest says %d", got.Size, f.Size) @@ -368,13 +377,22 @@ func (v *verifier) assetHashes(ctx context.Context, flat, typ string) map[string // checkKey is the key check of rule 28: the key in the copy opens every sealed // value of flats.db. It skips the check when the copy has no key. func (v *verifier) checkKey(ctx context.Context) { - if v.m.SecretKey != SecretKeyIncluded { - v.notes = append(v.notes, "the copy holds no secret.key, so the sealed values were not checked against a key") + var key []byte + keyName := "data/secret.key" + switch { + case v.m.SecretKey == SecretKeyIncluded: + var err error + if key, err = core.NewKeyFile(v.data, "").Read(); err != nil { + v.fail("data/secret.key", "is not a usable key: %v", err) + return + } + case v.m.SecretKey == SecretKeyExternal && v.extKey != nil: + key, keyName = v.extKey, "the key at host.secret_key_path" + case v.m.SecretKey == SecretKeyExternal: + v.notes = append(v.notes, "the key is outside the copy, so the sealed values were not checked against a key") return - } - key, err := core.ReadSecretKey(v.data) - if err != nil { - v.fail("data/secret.key", "is not a usable key: %v", err) + default: + v.notes = append(v.notes, "the copy holds no secret.key, so the sealed values were not checked against a key") return } st, err := store.OpenImmutable(filepath.Join(v.data, nameFlatsDB)) @@ -389,6 +407,6 @@ func (v *verifier) checkKey(ctx context.Context) { return } for _, n := range check.Unopened { - v.fails = append(v.fails, Failure{Path: "data/flats.db", Reason: fmt.Sprintf("%s does not open with data/secret.key", n), Fixed: true}) + v.fails = append(v.fails, Failure{Path: "data/flats.db", Reason: fmt.Sprintf("%s does not open with %s", n, keyName), Fixed: true}) } } diff --git a/internal/core/console_names_test.go b/internal/core/console_names_test.go index bfa8b52e..819845fd 100644 --- a/internal/core/console_names_test.go +++ b/internal/core/console_names_test.go @@ -114,30 +114,30 @@ func TestHostKeySealing(t *testing.T) { } defer st.Close() const key = "tskey-auth-k123-abcdef" - if err := PutHostKeyValue(ctx, dir, st, HostKeyTailscale, key, KeySourceCLI, time.Now()); err != nil { + if err := PutHostKeyValue(ctx, NewKeyFile(dir, ""), st, HostKeyTailscale, key, KeySourceCLI, time.Now()); err != nil { t.Fatal(err) } k, err := st.GetHostKey(ctx, HostKeyTailscale) if err != nil || strings.Contains(string(k.Ciphertext), "tskey") { t.Fatalf("stored %+v %v", k, err) } - if v, _, err := StoredHostKey(ctx, dir, st, HostKeyTailscale); err != nil || v != key { + if v, _, err := StoredHostKey(ctx, NewKeyFile(dir, ""), st, HostKeyTailscale); err != nil || v != key { t.Fatalf("StoredHostKey = %q %v", v, err) } // A value cannot be moved to another id. k.ID = HostKeyZrok - if _, err := OpenHostKey(dir, k); err == nil { + if _, err := OpenHostKey(NewKeyFile(dir, ""), k); err == nil { t.Fatal("a sealed value opened under another id") } // Another secret.key cannot open it. - if _, err := OpenHostKey(t.TempDir(), k); err == nil { + if _, err := OpenHostKey(NewKeyFile(t.TempDir(), ""), k); err == nil { t.Fatal("opened without secret.key") } - if v, _, err := StoredHostKey(ctx, dir, st, HostKeyNetbird); err != nil || v != "" { + if v, _, err := StoredHostKey(ctx, NewKeyFile(dir, ""), st, HostKeyNetbird); err != nil || v != "" { t.Fatalf("missing key = %q %v", v, err) } // secret.key gone with a stored key: refuse, as for flat secrets. - if err := CheckSecretKey(ctx, t.TempDir(), st); !errors.Is(err, ErrSecretKeyMissing) || !strings.Contains(err.Error(), "flats key rm") { + if err := CheckSecretKey(ctx, NewKeyFile(t.TempDir(), ""), st); !errors.Is(err, ErrSecretKeyMissing) || !strings.Contains(err.Error(), "flats key rm") { t.Fatalf("CheckSecretKey = %v", err) } // A connection record without a value does not count. @@ -147,7 +147,7 @@ func TestHostKeySealing(t *testing.T) { if err := st.PutHostKey(ctx, store.HostKey{ID: HostKeyZrok, Source: KeySourceConsole, UpdatedAt: time.Now()}); err != nil { t.Fatal(err) } - if err := CheckSecretKey(ctx, t.TempDir(), st); err != nil { + if err := CheckSecretKey(ctx, NewKeyFile(t.TempDir(), ""), st); err != nil { t.Fatalf("connection record: %v", err) } } diff --git a/internal/core/hostkeys.go b/internal/core/hostkeys.go index 4fabf239..2d877876 100644 --- a/internal/core/hostkeys.go +++ b/internal/core/hostkeys.go @@ -8,7 +8,6 @@ import ( "crypto/subtle" "errors" "fmt" - "path/filepath" "regexp" "strings" "time" @@ -84,10 +83,10 @@ func hostKeyAEAD(key []byte) (cipher.AEAD, error) { return cipher.NewGCM(block) } -// SealHostKey encrypts value for id under the data directory's secret.key, -// creating the key file when there is none yet. -func SealHostKey(dataDir, id, value string, source string, now time.Time) (store.HostKey, error) { - key, err := loadOrCreateKey(filepath.Join(dataDir, "secret.key")) +// SealHostKey encrypts value for id under the host key, creating the key file when there is none +// yet. A configured key is never created. +func SealHostKey(keyFile KeyFile, id, value string, source string, now time.Time) (store.HostKey, error) { + key, err := keyFile.LoadOrCreate() if err != nil { return store.HostKey{}, err } @@ -104,11 +103,11 @@ func SealHostKey(dataDir, id, value string, source string, now time.Time) (store } // OpenHostKey decrypts a stored key value. -func OpenHostKey(dataDir string, k store.HostKey) (string, error) { +func OpenHostKey(keyFile KeyFile, k store.HostKey) (string, error) { if !k.HasValue() { return "", errors.New("the key record holds no value") } - key, err := readSecretKey(filepath.Join(dataDir, "secret.key")) + key, err := keyFile.Read() if err != nil { return "", err } @@ -124,7 +123,7 @@ func OpenHostKey(dataDir string, k store.HostKey) (string, error) { } // StoredHostKey returns the value stored for id, or "" with no stored value. -func StoredHostKey(ctx context.Context, dataDir string, st *store.Store, id string) (string, store.HostKey, error) { +func StoredHostKey(ctx context.Context, keyFile KeyFile, st *store.Store, id string) (string, store.HostKey, error) { k, err := st.GetHostKey(ctx, id) if errors.Is(err, store.ErrNotFound) { return "", store.HostKey{}, nil @@ -135,13 +134,13 @@ func StoredHostKey(ctx context.Context, dataDir string, st *store.Store, id stri if !k.HasValue() { return "", k, nil } - v, err := OpenHostKey(dataDir, k) + v, err := OpenHostKey(keyFile, k) return v, k, err } // PutHostKeyValue seals and stores value for id. -func PutHostKeyValue(ctx context.Context, dataDir string, st *store.Store, id, value, source string, now time.Time) error { - k, err := SealHostKey(dataDir, id, value, source, now) +func PutHostKeyValue(ctx context.Context, keyFile KeyFile, st *store.Store, id, value, source string, now time.Time) error { + k, err := SealHostKey(keyFile, id, value, source, now) if err != nil { return err } diff --git a/internal/core/keyfile.go b/internal/core/keyfile.go new file mode 100644 index 00000000..aaf962c9 --- /dev/null +++ b/internal/core/keyfile.go @@ -0,0 +1,140 @@ +package core + +import ( + "errors" + "fmt" + "io/fs" + "os" + "path/filepath" + "runtime" + "strings" + + "github.com/gosuda/flats/internal/durable" +) + +// KeySetting is the name of the setting that moves the host key, as messages name it. +const KeySetting = "host.secret_key_path" + +// ErrKeyLocation reports that host.secret_key_path names a key that the host must not use or cannot +// read: nothing at the path, a second key at /secret.key, a key that resolves into the data +// directory, a file that is no key, or a file that cannot be read. Flats never creates a key at a +// configured path (docs/spec/secret-key-path.md). +var ErrKeyLocation = errors.New(KeySetting + " cannot be used") + +// KeyFile is where the host key lives: /secret.key, or the file that host.secret_key_path +// names. Everything that reads or writes the key gets its path from here. +type KeyFile struct { + dataDir string + path string + configured bool +} + +// NewKeyFile returns the location of the key of the data directory dataDir. configured is +// host.secret_key_path, and "" keeps the key at /secret.key. It touches no file. +func NewKeyFile(dataDir, configured string) KeyFile { + if configured == "" { + return KeyFile{dataDir: dataDir, path: filepath.Join(dataDir, SecretKeyFile)} + } + return KeyFile{dataDir: dataDir, path: configured, configured: true} +} + +// Path is the file that holds the key. +func (k KeyFile) Path() string { return k.path } + +// Configured reports whether host.secret_key_path chose the path. +func (k KeyFile) Configured() bool { return k.configured } + +// DataDir is the data directory that the key belongs to. +func (k KeyFile) DataDir() string { return k.dataDir } + +// Check makes the two checks of a configured key that the lexical check of the setting cannot make +// (rule 3 of the spec). It refuses a file, directory or link at /secret.key, because two keys +// would be in use and a copy of the data directory would hold the one that the setting keeps out. It +// also refuses a key whose resolved path lies inside the resolved data directory. A default key has +// nothing to check. +func (k KeyFile) Check() error { + if !k.configured { + return nil + } + leftover := filepath.Join(k.dataDir, SecretKeyFile) + if _, err := os.Lstat(leftover); err == nil { + return fmt.Errorf("%w: %s is set, and %s exists too; move that file away, because Flats uses the configured key and a copy of the data directory would hold the old one", + ErrKeyLocation, KeySetting, leftover) + } else if !errors.Is(err, fs.ErrNotExist) { + return fmt.Errorf("%w: look at %s: %v", ErrKeyLocation, leftover, err) + } + real, err := filepath.EvalSymlinks(k.path) + if err != nil { + return nil // nothing to resolve; Read says what is wrong with the path + } + dataReal, err := filepath.EvalSymlinks(k.dataDir) + if err != nil { + return nil + } + if PathInside(dataReal, real) { + return fmt.Errorf("%w: %s names %s, which resolves to %s inside the data directory %s; keep the key outside the data directory", + ErrKeyLocation, KeySetting, k.path, real, dataReal) + } + return nil +} + +// Read reads the key. With a configured path it makes the checks of Check first and refuses with an +// error that names the path and the setting. The default path gets the checks that the host has +// always made. +func (k KeyFile) Read() ([]byte, error) { + if !k.configured { + return readSecretKey(k.path) + } + if err := k.Check(); err != nil { + return nil, err + } + if _, err := os.Stat(k.path); err != nil { + if !errors.Is(err, fs.ErrNotExist) { + return nil, fmt.Errorf("%w: %s names %s: %v", ErrKeyLocation, KeySetting, k.path, err) + } + if _, lerr := os.Lstat(k.path); lerr == nil { + return nil, fmt.Errorf("%w: %s names %s, which is a link to nothing; repair the link or the mount, or clear the setting with `flats config unset %s`, and Flats never creates a key at a configured path", + ErrKeyLocation, KeySetting, k.path, KeySetting) + } + return nil, fmt.Errorf("%w: %s names %s, and nothing is there. Flats never creates a key at a configured path. Put a key there (for example `head -c 32 /dev/urandom > %s`), restore it with `flats secret restore-key`, or clear the setting with `flats config unset %s`", + ErrKeyLocation, KeySetting, k.path, k.path, KeySetting) + } + b, err := readSecretKey(k.path) + if err != nil { + return nil, fmt.Errorf("%w: %s: %w", ErrKeyLocation, KeySetting, err) + } + return b, nil +} + +// LoadOrCreate reads the key. A default key is created when there is none. A configured key never is: +// a missing file is a refusal. +func (k KeyFile) LoadOrCreate() ([]byte, error) { + if k.configured { + return k.Read() + } + return loadOrCreateKey(k.path) +} + +// Restore writes key at the path through a synced temporary file beside it, and never replaces +// anything there: os.IsExist reports that the name is taken. With a configured path the temporary +// file lies in the folder of that path. +func (k KeyFile) Restore(key []byte) error { + if err := publishKey(k.path, key); err != nil { + return err + } + return durable.SyncDir(filepath.Dir(k.path)) +} + +// PathInside reports whether p is dir or lies below it, ignoring case on Windows. It compares paths +// that the caller already resolved. +func PathInside(dir, p string) bool { + dir, p = filepath.Clean(dir), filepath.Clean(p) + if runtime.GOOS == "windows" { + dir, p = strings.ToLower(dir), strings.ToLower(p) + } + rel, err := filepath.Rel(dir, p) + if err != nil { + return false + } + return rel == "." || (rel != ".." && !strings.HasPrefix(rel, ".."+string(filepath.Separator))) +} diff --git a/internal/core/keyrecovery.go b/internal/core/keyrecovery.go index c8c6fc2f..066ff243 100644 --- a/internal/core/keyrecovery.go +++ b/internal/core/keyrecovery.go @@ -44,12 +44,6 @@ const ( // ErrRecoveryPassword reports that a password does not open a recovery copy. var ErrRecoveryPassword = errors.New("the password does not open this recovery file") -// ReadSecretKey reads dataDir's secret.key with the checks the host makes at -// start. -func ReadSecretKey(dataDir string) ([]byte, error) { - return readSecretKey(filepath.Join(dataDir, SecretKeyFile)) -} - // KeyID identifies a key without revealing anything that helps open it: the // first 16 bytes of HMAC-SHA-256 over a 19-byte label, with the key as the // HMAC key (docs/spec/secret-key-recovery.md, rule 2). The key is the HMAC @@ -185,15 +179,6 @@ func CheckKey(ctx context.Context, st *store.Store, key []byte) (KeyCheck, error return c, nil } -// RestoreSecretKey writes key as dataDir's secret.key. It never replaces -// anything there: os.IsExist reports that the name is taken. -func RestoreSecretKey(dataDir string, key []byte) error { - if err := publishKey(filepath.Join(dataDir, SecretKeyFile), key); err != nil { - return err - } - return durable.SyncDir(dataDir) -} - // WriteKeyCopy writes data, the recovery copy of key, to dataDir and then // the key's ID beside it, each replacing an older file, and reports whether // a copy was there. A crash between the two leaves an ID that does not match, diff --git a/internal/core/keyrecovery_test.go b/internal/core/keyrecovery_test.go index 56b542e2..43aa490f 100644 --- a/internal/core/keyrecovery_test.go +++ b/internal/core/keyrecovery_test.go @@ -178,14 +178,14 @@ func TestCheckKey(t *testing.T) { t.Fatal(err) } } - if err := PutHostKeyValue(ctx, dir, s.st, HostKeyZrok, "zrok-token", KeySourceCLI, time.Now()); err != nil { + if err := PutHostKeyValue(ctx, NewKeyFile(dir, ""), s.st, HostKeyZrok, "zrok-token", KeySourceCLI, time.Now()); err != nil { t.Fatal(err) } // A record of a connection holds no value and is not counted. if err := s.st.PutHostKey(ctx, store.HostKey{ID: HostKeyNetbird, Source: KeySourceCLI, UpdatedAt: time.Now()}); err != nil { t.Fatal(err) } - key, err := ReadSecretKey(dir) + key, err := NewKeyFile(dir, "").Read() if err != nil { t.Fatal(err) } @@ -219,7 +219,7 @@ func TestRestoreSecretKeyNeverReplaces(t *testing.T) { rand.Read(key) dir := t.TempDir() path := filepath.Join(dir, SecretKeyFile) - if err := RestoreSecretKey(dir, key); err != nil { + if err := NewKeyFile(dir, "").Restore(key); err != nil { t.Fatal(err) } if got, err := os.ReadFile(path); err != nil || !bytes.Equal(got, key) { @@ -230,7 +230,7 @@ func TestRestoreSecretKeyNeverReplaces(t *testing.T) { } other := make([]byte, 32) rand.Read(other) - if err := RestoreSecretKey(dir, other); !os.IsExist(err) { + if err := NewKeyFile(dir, "").Restore(other); !os.IsExist(err) { t.Fatalf("over a key: %v", err) } if got, _ := os.ReadFile(path); !bytes.Equal(got, key) { @@ -241,7 +241,7 @@ func TestRestoreSecretKeyNeverReplaces(t *testing.T) { if err := ostest.Symlink(t, filepath.Join(dangling, "gone"), filepath.Join(dangling, SecretKeyFile)); err != nil { t.Fatal(err) } - if err := RestoreSecretKey(dangling, key); !os.IsExist(err) { + if err := NewKeyFile(dangling, "").Restore(key); !os.IsExist(err) { t.Fatalf("over a dangling link: %v", err) } if _, err := os.Lstat(filepath.Join(dangling, "gone")); !errors.Is(err, os.ErrNotExist) { @@ -320,17 +320,17 @@ func TestMissingKeyRefusalNamesRecovery(t *testing.T) { if err := s.SetSecret(ctx, "app", "API_KEY", "v", ViaCLI); err != nil { t.Fatal(err) } - err := CheckSecretKey(ctx, t.TempDir(), s.st) + err := CheckSecretKey(ctx, NewKeyFile(t.TempDir(), ""), s.st) if !errors.Is(err, ErrSecretKeyMissing) || !strings.Contains(err.Error(), "flats secret restore-key") || !strings.Contains(err.Error(), "flats secret reset") { t.Fatalf("refusal: %v", err) } // With operator keys alone, the way out is `flats key rm`, not reset. keys, dir := newTestService(t) - if err := PutHostKeyValue(ctx, dir, keys.st, HostKeyZrok, "zrok-token", KeySourceCLI, time.Now()); err != nil { + if err := PutHostKeyValue(ctx, NewKeyFile(dir, ""), keys.st, HostKeyZrok, "zrok-token", KeySourceCLI, time.Now()); err != nil { t.Fatal(err) } - err = CheckSecretKey(ctx, t.TempDir(), keys.st) + err = CheckSecretKey(ctx, NewKeyFile(t.TempDir(), ""), keys.st) if msg := fmt.Sprint(err); !errors.Is(err, ErrSecretKeyMissing) || !strings.Contains(msg, "flats secret restore-key") || !strings.Contains(msg, "flats key rm") || strings.Contains(msg, "reset") { t.Fatalf("operator keys alone: %v", err) } @@ -341,10 +341,10 @@ func TestInvalidSecretKeyError(t *testing.T) { if err := os.WriteFile(filepath.Join(dir, SecretKeyFile), []byte("short"), 0o600); err != nil { t.Fatal(err) } - if _, err := ReadSecretKey(dir); !errors.Is(err, ErrInvalidSecretKey) || !strings.Contains(err.Error(), "must be 32 bytes") { + if _, err := NewKeyFile(dir, "").Read(); !errors.Is(err, ErrInvalidSecretKey) || !strings.Contains(err.Error(), "must be 32 bytes") { t.Fatalf("short key: %v", err) } - if _, err := ReadSecretKey(t.TempDir()); errors.Is(err, ErrInvalidSecretKey) || !errors.Is(err, os.ErrNotExist) { + if _, err := NewKeyFile(t.TempDir(), "").Read(); errors.Is(err, ErrInvalidSecretKey) || !errors.Is(err, os.ErrNotExist) { t.Fatalf("missing key: %v", err) } } @@ -352,14 +352,14 @@ func TestInvalidSecretKeyError(t *testing.T) { // A stored operator key that the key does not open names the way out. func TestUnopenedHostKeyNamesRecovery(t *testing.T) { dir := t.TempDir() - k, err := SealHostKey(dir, HostKeyTailscale, "tskey-auth-x", KeySourceCLI, time.Now()) + k, err := SealHostKey(NewKeyFile(dir, ""), HostKeyTailscale, "tskey-auth-x", KeySourceCLI, time.Now()) if err != nil { t.Fatal(err) } if err := os.WriteFile(filepath.Join(dir, SecretKeyFile), bytes.Repeat([]byte{1}, 32), 0o600); err != nil { t.Fatal(err) } - _, err = OpenHostKey(dir, k) + _, err = OpenHostKey(NewKeyFile(dir, ""), k) if err == nil || !strings.Contains(err.Error(), "does not match flats.db") || !strings.Contains(err.Error(), "flats secret restore-key") || !strings.Contains(err.Error(), "flats key rm") { t.Fatalf("open: %v", err) } diff --git a/internal/core/reload.go b/internal/core/reload.go index c77f9d95..a13148d6 100644 --- a/internal/core/reload.go +++ b/internal/core/reload.go @@ -15,7 +15,7 @@ import ( // live instead, and the host identity keys never change on a live host: a // reload refuses a file that changes them. var restartKeys = []string{ - "host.console_addr", "host.local_addr", "host.server_runtime", + "host.console_addr", "host.local_addr", "host.server_runtime", "host.secret_key_path", "metrics.enabled", "metrics.expose_on", "metrics.per_flat", "network.private_backend", "portal.relays", "portal.discovery", "portal.max_active_relays", diff --git a/internal/core/reload_test.go b/internal/core/reload_test.go index d4704ec6..4bc9a770 100644 --- a/internal/core/reload_test.go +++ b/internal/core/reload_test.go @@ -472,3 +472,26 @@ func TestReloadSerializesWithConsoleSave(t *testing.T) { t.Fatal("reload or save result missing") } } + +// A reload adopts a changed host.secret_key_path and lists it as awaiting a restart. The running host keeps the +// key that it read at start (docs/spec/secret-key-path.md, rule 8). +func TestReloadListsTheSecretKeyPathAsAwaitingARestart(t *testing.T) { + s, path := newReloadService(t) + before := string(s.secretKey) + editConfig(t, path, func(d *config.Document) error { + return d.Set("host.secret_key_path", filepath.Join(t.TempDir(), "elsewhere", "secret.key")) + }) + rep, err := s.ReloadConfig(context.Background()) + if err != nil { + t.Fatal(err) + } + if !slices.Equal(rep.AwaitRestart, []string{"host.secret_key_path"}) { + t.Fatalf("await restart %v", rep.AwaitRestart) + } + if len(rep.Applied) != 0 || len(rep.Held) != 0 { + t.Errorf("applied %v, held %v; want none", rep.Applied, rep.Held) + } + if string(s.secretKey) != before { + t.Error("the running host changed its key at a reload") + } +} diff --git a/internal/core/secrets.go b/internal/core/secrets.go index 5624dd3a..23dec8b0 100644 --- a/internal/core/secrets.go +++ b/internal/core/secrets.go @@ -35,9 +35,15 @@ func (e invalidKeyError) Error() string { return string(e) } func (invalidKeyError) Is(target error) bool { return target == ErrInvalidSecretKey } // CheckSecretKey refuses to let a new key be created over existing secrets: -// restore /secret.key together with flats.db from the same backup. -func CheckSecretKey(ctx context.Context, dataDir string, st *store.Store) error { - path := filepath.Join(dataDir, "secret.key") +// restore the key together with flats.db from the same backup. A configured key +// (host.secret_key_path) is read in full, so every way that it can be unusable is a +// refusal here, with or without sealed values: Flats never creates it. +func CheckSecretKey(ctx context.Context, key KeyFile, st *store.Store) error { + if key.Configured() { + _, err := key.Read() + return err + } + path := key.Path() if _, err := os.Lstat(path); !errors.Is(err, os.ErrNotExist) { return nil } diff --git a/internal/core/service.go b/internal/core/service.go index 7c705da2..5e20a68b 100644 --- a/internal/core/service.go +++ b/internal/core/service.go @@ -91,13 +91,16 @@ type Instance interface { // Config wires a Service. type Config struct { - DataDir string - Store *store.Store - Private PrivateNet - Lifecycle LifecycleNet // optional provider manager; independent of legacy adapters - Public PublicNet // nil disables public flats - DocsApp DocsApp - Runtime Runtime // nil disables server flats + DataDir string + // SecretKeyPath is host.secret_key_path: "" keeps the host key at /secret.key, and a path + // is a key that the host reads and never creates. + SecretKeyPath string + Store *store.Store + Private PrivateNet + Lifecycle LifecycleNet // optional provider manager; independent of legacy adapters + Public PublicNet // nil disables public flats + DocsApp DocsApp + Runtime Runtime // nil disables server flats // Settings holds the system settings (config.json). Nil keeps the // frozen defaults in memory. Settings *SettingsSource @@ -251,10 +254,11 @@ func New(ctx context.Context, cfg Config) (*Service, error) { if _, err := s.syncAutopilot(ctx); err != nil { return nil, fmt.Errorf("autopilot settings: %w", err) } - if err := CheckSecretKey(ctx, cfg.DataDir, cfg.Store); err != nil { + keyFile := NewKeyFile(cfg.DataDir, cfg.SecretKeyPath) + if err := CheckSecretKey(ctx, keyFile, cfg.Store); err != nil { return nil, err } - key, err := loadOrCreateKey(filepath.Join(cfg.DataDir, "secret.key")) + key, err := keyFile.LoadOrCreate() if err != nil { return nil, err } diff --git a/internal/core/settings_test.go b/internal/core/settings_test.go index adc40124..15c3b42e 100644 --- a/internal/core/settings_test.go +++ b/internal/core/settings_test.go @@ -127,10 +127,10 @@ func TestMissingSecretKeyWithSecretsRefused(t *testing.T) { if err := e.svc.SetSecret(ctx, "app", "TOKEN", "v", core.ViaCLI); err != nil { t.Fatal(err) } - if err := core.CheckSecretKey(ctx, e.dataDir, e.st); err != nil { + if err := core.CheckSecretKey(ctx, core.NewKeyFile(e.dataDir, ""), e.st); err != nil { t.Fatal(err) } - if err := core.CheckSecretKey(ctx, t.TempDir(), e.st); !errors.Is(err, core.ErrSecretKeyMissing) { + if err := core.CheckSecretKey(ctx, core.NewKeyFile(t.TempDir(), ""), e.st); !errors.Is(err, core.ErrSecretKeyMissing) { t.Fatalf("missing key over secrets: %v", err) } if _, err := core.New(ctx, core.Config{DataDir: t.TempDir(), Store: e.st, Logf: t.Logf}); !errors.Is(err, core.ErrSecretKeyMissing) { From 8587c3c7967ac385917fb0576afd95afdcf622e7 Mon Sep 17 00:00:00 2001 From: Hee Sung Son Date: Sat, 14 Nov 2026 18:25:40 +0900 Subject: [PATCH 06/14] test(core): cover the configured key, and re-read it without the checks in a backup Tests for KeyFile (never created, every refusal, read-only places), the host start with the key outside the data directory, key set, operator keys that read the file at each use, backup-key, restore-key with and without --out, reset, flats backup with an external key (manifest, scan, key changes, refusals, the flag) and the reload. The test of a key that appears in the data directory during a backup showed that the second read of rule 22 repeated the checks of rule 3 and failed the backup. KeyFile.ReadAgain reads without them, and the backup uses it for that read. --- internal/app/secretkeypath_cmd_test.go | 299 +++++++++++++++++++ internal/app/secretkeypath_test.go | 267 +++++++++++++++++ internal/backup/backup.go | 13 +- internal/backup/externalkey_test.go | 363 ++++++++++++++++++++++ internal/core/keyfile.go | 15 + internal/core/keyfile_test.go | 398 +++++++++++++++++++++++++ 6 files changed, 1354 insertions(+), 1 deletion(-) create mode 100644 internal/app/secretkeypath_cmd_test.go create mode 100644 internal/app/secretkeypath_test.go create mode 100644 internal/backup/externalkey_test.go create mode 100644 internal/core/keyfile_test.go diff --git a/internal/app/secretkeypath_cmd_test.go b/internal/app/secretkeypath_cmd_test.go new file mode 100644 index 00000000..cc28396b --- /dev/null +++ b/internal/app/secretkeypath_cmd_test.go @@ -0,0 +1,299 @@ +package app + +import ( + "bytes" + "context" + "os" + "path/filepath" + "slices" + "strings" + "testing" + + "github.com/gosuda/flats/internal/core" + "github.com/gosuda/flats/internal/ostest" +) + +const recoveryPassword = "a long recovery password\n" + +// stoppedHostWithAKeyOutside starts a host with a flat secret and an operator key, stops it, and moves its key out +// of the data directory. It returns the data directory, the configuration, the key bytes and the key path. +func stoppedHostWithAKeyOutside(t *testing.T) (dir, config string, key []byte, keyPath string) { + t.Helper() + dir = t.TempDir() + var h *Host + config, key, h = hostWithSecrets(t, dir) + if err := h.Close(); err != nil { + t.Fatal(err) + } + return dir, config, key, keyOutside(t, dir, config) +} + +func mustRunSecretKey(t *testing.T, config string, args ...string) string { + t.Helper() + out, err := runSecretKey(t, config, args...) + if err != nil { + t.Fatalf("flats secret %s: %v\n%s", args[0], err, out) + } + return out +} + +func TestBackupKeyReadsTheConfiguredKey(t *testing.T) { + dir, config, key, keyPath := stoppedHostWithAKeyOutside(t) + mustRunSecretKey(t, config, "backup-key", "--password-file", passwordFile(t, recoveryPassword)) + copyBytes, err := os.ReadFile(filepath.Join(dir, core.RecoveryKeyFile)) + if err != nil { + t.Fatalf("the recovery copy is not in the data directory: %v", err) + } + if got, err := core.OpenKeyCopy(copyBytes, strings.TrimSpace(recoveryPassword)); err != nil || !bytes.Equal(got, key) { + t.Errorf("the recovery copy opens to %x, %v; want the configured key", got, err) + } + if _, err := os.Lstat(filepath.Join(dir, core.SecretKeyFile)); !os.IsNotExist(err) { + t.Errorf("backup-key put a key in the data directory: %v", err) + } + // A missing configured key is a refusal that names the path and the setting. + if err := os.Remove(keyPath); err != nil { + t.Fatal(err) + } + _, err = runSecretKey(t, config, "backup-key", "--password-file", passwordFile(t, "another long password\n")) + if err == nil || exitCode(err) != ExitConfig || !strings.Contains(err.Error(), keyPath) || !strings.Contains(err.Error(), core.KeySetting) { + t.Errorf("backup-key without the key: %v (status %d)", err, exitCode(err)) + } +} + +func TestRestoreKeyWritesTheConfiguredPath(t *testing.T) { + dir, config, key, keyPath := stoppedHostWithAKeyOutside(t) + mustRunSecretKey(t, config, "backup-key", "--password-file", passwordFile(t, recoveryPassword)) + if err := os.Remove(keyPath); err != nil { + t.Fatal(err) + } + keyDir := filepath.Dir(keyPath) + out := mustRunSecretKey(t, config, "restore-key", "--password-file", passwordFile(t, recoveryPassword)) + if !strings.Contains(out, keyPath) { + t.Errorf("the output does not name the restored path: %s", out) + } + if got, err := os.ReadFile(keyPath); err != nil || !bytes.Equal(got, key) { + t.Errorf("the restored key = %x, %v", got, err) + } + if got := listed(t, keyDir); !slices.Equal(got, []string{"flats.key"}) { + t.Errorf("the key folder holds %v", got) + } + noTemporaryKeyFile(t, dir, keyDir) + if _, err := os.Lstat(filepath.Join(dir, core.SecretKeyFile)); !os.IsNotExist(err) { + t.Errorf("restore-key wrote into the data directory: %v", err) + } + // It never replaces a file. + _, err := runSecretKey(t, config, "restore-key", "--password-file", passwordFile(t, recoveryPassword)) + if err == nil || !strings.Contains(err.Error(), "never replaces") { + t.Errorf("a second restore-key: %v, want a refusal", err) + } + h, err := Start(context.Background(), configModeOptions(config)) + if err != nil { + t.Fatalf("the host does not start with the restored key: %v", err) + } + h.Close() +} + +// --out writes the recovered key to a new file and changes nothing at the location, which may hold a wrong key +// (rule 13). +func TestRestoreKeyOutWritesANewFile(t *testing.T) { + dir, config, key, keyPath := stoppedHostWithAKeyOutside(t) + mustRunSecretKey(t, config, "backup-key", "--password-file", passwordFile(t, recoveryPassword)) + wrong := bytes.Repeat([]byte{3}, 32) + if err := os.WriteFile(keyPath, wrong, 0o600); err != nil { + t.Fatal(err) + } + pw := passwordFile(t, recoveryPassword) + outFile := filepath.Join(t.TempDir(), "recovered.key") + out := mustRunSecretKey(t, config, "restore-key", "--out", outFile, "--password-file", pw) + if !strings.Contains(out, outFile) || !strings.Contains(out, keyPath) { + t.Errorf("the output names neither the file nor the key path: %s", out) + } + if got, err := os.ReadFile(outFile); err != nil || !bytes.Equal(got, key) { + t.Errorf("--out file = %x, %v; want the recovered key", got, err) + } + if !ostest.Windows { + if fi, err := os.Stat(outFile); err != nil || fi.Mode().Perm() != 0o600 { + t.Errorf("--out file mode = %v, %v; want 0600", fi.Mode(), err) + } + } + if got, err := os.ReadFile(keyPath); err != nil || !bytes.Equal(got, wrong) { + t.Errorf("--out changed the location: %x, %v", got, err) + } + // It never replaces a file, and it refuses a place inside the data directory. + if _, err := runSecretKey(t, config, "restore-key", "--out", outFile, "--password-file", pw); err == nil { + t.Error("--out replaced a file") + } + inside := filepath.Join(dir, "recovered.key") + _, err := runSecretKey(t, config, "restore-key", "--out", inside, "--password-file", pw) + if err == nil || exitCode(err) != ExitConfig || !strings.Contains(err.Error(), "outside the data directory") { + t.Errorf("--out inside the data directory: %v (status %d)", err, exitCode(err)) + } + if _, err := os.Lstat(inside); !os.IsNotExist(err) { + t.Errorf("--out wrote inside the data directory: %v", err) + } + // A second key in the data directory is a refusal with or without --out (rule 3). + if err := os.WriteFile(filepath.Join(dir, core.SecretKeyFile), key, 0o600); err != nil { + t.Fatal(err) + } + other := filepath.Join(t.TempDir(), "other.key") + if _, err := runSecretKey(t, config, "restore-key", "--out", other, "--password-file", pw); err == nil || !strings.Contains(err.Error(), "move that file away") { + t.Errorf("--out with a second key in the data directory: %v", err) + } + if _, err := os.Lstat(other); !os.IsNotExist(err) { + t.Errorf("--out wrote a file next to a refusal: %v", err) + } +} + +func TestRestoreKeyInAReadOnlyLocationNamesOut(t *testing.T) { + if ostest.Windows || os.Geteuid() == 0 { + t.Skip("needs Unix modes and a user that is not the superuser") + } + dir, config, _, keyPath := stoppedHostWithAKeyOutside(t) + mustRunSecretKey(t, config, "backup-key", "--password-file", passwordFile(t, recoveryPassword)) + keyDir := filepath.Dir(keyPath) + if err := os.Remove(keyPath); err != nil { + t.Fatal(err) + } + if err := os.Chmod(keyDir, 0o555); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { os.Chmod(keyDir, 0o700) }) + _, err := runSecretKey(t, config, "restore-key", "--password-file", passwordFile(t, recoveryPassword)) + if err == nil || !strings.Contains(err.Error(), "--out") || !strings.Contains(err.Error(), "nothing was written") { + t.Errorf("restore-key into a read-only folder: %v", err) + } + os.Chmod(keyDir, 0o700) + if got := listed(t, keyDir); len(got) != 0 { + t.Errorf("the key folder holds %v", got) + } + noTemporaryKeyFile(t, dir, keyDir) +} + +func TestResetWithAConfiguredKey(t *testing.T) { + ctx := context.Background() + dir, config, _, keyPath := stoppedHostWithAKeyOutside(t) + // The configured key opens every value, so nothing is lost and reset refuses. + _, err := runSecretKey(t, config, "reset", "--yes") + if err == nil || exitCode(err) != ExitConfig || !strings.Contains(err.Error(), "opens all") || !strings.Contains(err.Error(), keyPath) { + t.Fatalf("reset with the key in place: %v (status %d)", err, exitCode(err)) + } + // With the key gone, reset deletes the values and says what the next start needs. + if err := os.Remove(keyPath); err != nil { + t.Fatal(err) + } + out := mustRunSecretKey(t, config, "reset", "--yes") + for _, want := range []string{"deleted 2 values", "The next start needs a key at " + keyPath, "never creates a key at a configured path"} { + if !strings.Contains(out, want) { + t.Errorf("the output lacks %q:\n%s", want, out) + } + } + if _, err := os.Lstat(keyPath); !os.IsNotExist(err) { + t.Errorf("reset created the key: %v", err) + } + if h, err := Start(ctx, configModeOptions(config)); err == nil { + h.Close() + t.Fatal("the host started without a key") + } else if exitCode(err) != ExitConfig { + t.Errorf("start without the key: %v (status %d)", err, exitCode(err)) + } + if err := os.WriteFile(keyPath, bytes.Repeat([]byte{4}, 32), 0o600); err != nil { + t.Fatal(err) + } + h, err := Start(ctx, configModeOptions(config)) + if err != nil { + t.Fatalf("the host does not start with a new key: %v", err) + } + defer h.Close() + if check, err := core.CheckKey(ctx, h.Store, bytes.Repeat([]byte{4}, 32)); err != nil || check.Total != 0 { + t.Errorf("after the reset the host holds %+v, %v; want no sealed value", check, err) + } + if _, err := os.Lstat(filepath.Join(dir, core.SecretKeyFile)); !os.IsNotExist(err) { + t.Errorf("a key appeared in the data directory: %v", err) + } +} + +// flats backup records an external key, leaves it out and scans for it; --verify reports the note (rules 16 to 20). +func TestBackupCommandWithAConfiguredKey(t *testing.T) { + ctx := context.Background() + dir, config, key, keyPath := stoppedHostWithAKeyOutside(t) + out := filepath.Join(t.TempDir(), "copy") + text, err := runBackup(t, "--out", out, "--config", config) + if err != nil { + t.Fatalf("backup: %v\n%s", err, text) + } + if !strings.Contains(text, "the secret key is kept at "+keyPath) || !strings.Contains(text, "back it up apart from the copy") { + t.Errorf("the output does not say where the key is kept:\n%s", text) + } + if _, err := os.Lstat(filepath.Join(out, "data", core.SecretKeyFile)); !os.IsNotExist(err) { + t.Errorf("the copy holds a key file: %v", err) + } + manifest, err := os.ReadFile(filepath.Join(out, "manifest.json")) + if err != nil || !strings.Contains(string(manifest), `"secret_key": "external"`) { + t.Errorf("manifest = %s, %v; want secret_key external", manifest, err) + } + filepath.WalkDir(out, func(p string, d os.DirEntry, err error) error { + if err == nil && !d.IsDir() { + if b, rerr := os.ReadFile(p); rerr == nil && bytes.Contains(b, key) { + t.Errorf("%s holds the bytes of the key", p) + } + } + return nil + }) + verify, err := runBackup(t, "--verify", out) + if err != nil || !strings.Contains(verify, "the key is outside the copy") || !strings.Contains(verify, "ok: ") { + t.Errorf("verify: %v\n%s", err, verify) + } + // A restore needs the key put back at the configured path. Without it, the host refuses; with it, it opens every value. + restoreFrom(t, dir, out) + if err := os.Remove(keyPath); err != nil { + t.Fatal(err) + } + if h, err := Start(ctx, configModeOptions(config)); err == nil { + h.Close() + t.Fatal("the restored host started without the key") + } + if err := os.WriteFile(keyPath, key, 0o600); err != nil { + t.Fatal(err) + } + h, err := Start(ctx, configModeOptions(config)) + if err != nil { + t.Fatalf("the restored host does not start with the key put back: %v", err) + } + defer h.Close() + if check, err := core.CheckKey(ctx, h.Store, key); err != nil || !check.All() || check.Total != 2 { + t.Errorf("the key opens %+v, %v", check, err) + } +} + +func TestBackupCommandRefusesASecondKeyAndSkipsAnUnreadableOne(t *testing.T) { + dir, config, key, keyPath := stoppedHostWithAKeyOutside(t) + if err := os.WriteFile(filepath.Join(dir, core.SecretKeyFile), key, 0o600); err != nil { + t.Fatal(err) + } + out := filepath.Join(t.TempDir(), "copy") + _, err := runBackup(t, "--out", out, "--config", config) + if err == nil || exitCode(err) != ExitConfig || !strings.Contains(err.Error(), "move that file away") { + t.Fatalf("backup with a second key: %v (status %d)", err, exitCode(err)) + } + if _, err := os.Stat(out); !os.IsNotExist(err) { + t.Errorf("a refused backup left %s: %v", out, err) + } + if err := os.Remove(filepath.Join(dir, core.SecretKeyFile)); err != nil { + t.Fatal(err) + } + // A missing key refuses the backup and names the flag. With the flag the backup does not read the key. + if err := os.Remove(keyPath); err != nil { + t.Fatal(err) + } + _, err = runBackup(t, "--out", out, "--config", config) + if err == nil || exitCode(err) != ExitConfig || !strings.Contains(err.Error(), "--exclude-secret-key") { + t.Fatalf("backup without the key: %v (status %d)", err, exitCode(err)) + } + if got := listed(t, filepath.Dir(out)); len(got) != 0 { + t.Errorf("a refused backup made %v", got) + } + text, err := runBackup(t, "--out", out, "--exclude-secret-key", "--config", config) + if err != nil || !strings.Contains(text, "the secret key is kept at") { + t.Errorf("backup with --exclude-secret-key and no key: %v\n%s", err, text) + } +} diff --git a/internal/app/secretkeypath_test.go b/internal/app/secretkeypath_test.go new file mode 100644 index 00000000..1162af47 --- /dev/null +++ b/internal/app/secretkeypath_test.go @@ -0,0 +1,267 @@ +package app + +import ( + "bytes" + "context" + "os" + "path/filepath" + "slices" + "strings" + "testing" + "time" + + "github.com/gosuda/flats/internal/core" + "github.com/gosuda/flats/internal/ostest" +) + +// docs/spec/secret-key-path.md: host.secret_key_path keeps the host key outside the data directory. + +// keyOutside moves the key of the stopped host in dir out of the data directory and sets host.secret_key_path to +// its new place, as the docs describe. It returns the new path. +func keyOutside(t *testing.T, dir, config string) string { + t.Helper() + path := filepath.Join(t.TempDir(), "flats.key") + if err := os.Rename(filepath.Join(dir, core.SecretKeyFile), path); err != nil { + t.Fatal(err) + } + var out bytes.Buffer + if err := ConfigCommand(context.Background(), []string{"set", "host.secret_key_path", path, "--config", config}, &out); err != nil { + t.Fatal(err, out.String()) + } + return path +} + +// listed returns the names in a folder, sorted, or nil when the folder is not there. +func listed(t *testing.T, dir string) []string { + t.Helper() + entries, err := os.ReadDir(dir) + if err != nil { + return nil + } + var out []string + for _, e := range entries { + out = append(out, e.Name()) + } + slices.Sort(out) + return out +} + +// noTemporaryKeyFile fails the test when a synced temporary key file is left in any of the folders. +func noTemporaryKeyFile(t *testing.T, dirs ...string) { + t.Helper() + for _, d := range dirs { + for _, n := range listed(t, d) { + if strings.HasPrefix(n, ".secret-key-") { + t.Errorf("%s holds the temporary key file %s", d, n) + } + } + } +} + +func TestHostStartsWithAConfiguredKeyAndCreatesNothing(t *testing.T) { + ctx := context.Background() + dir := t.TempDir() + config, key, h := hostWithSecrets(t, dir) + if err := h.Close(); err != nil { + t.Fatal(err) + } + keyPath := keyOutside(t, dir, config) + keyDir := filepath.Dir(keyPath) + h2, err := Start(ctx, configModeOptions(config)) + if err != nil { + t.Fatalf("the host does not start with the key outside the data directory: %v", err) + } + defer h2.Close() + if check, err := core.CheckKey(ctx, h2.Store, key); err != nil || !check.All() || check.Total != 2 { + t.Errorf("the key opens %+v, %v; want the flat secret and the operator key", check, err) + } + if v, _, err := core.StoredHostKey(ctx, h2.keyFile(), h2.Store, core.HostKeyZrok); err != nil || v != "zrok-token" { + t.Errorf("the stored operator key = %q, %v", v, err) + } + if !h2.keyFile().Configured() || h2.keyFile().Path() != keyPath { + t.Errorf("the host's key file is %+v", h2.keyFile()) + } + if _, err := os.Lstat(filepath.Join(dir, core.SecretKeyFile)); !os.IsNotExist(err) { + t.Errorf("a key was created in the data directory: %v", err) + } + if got := listed(t, keyDir); !slices.Equal(got, []string{"flats.key"}) { + t.Errorf("the key folder holds %v", got) + } + noTemporaryKeyFile(t, dir, keyDir) +} + +// Every way that the configured key can be unusable stops the start with status 78, names the path and the +// setting, and creates nothing (rules 5 to 7). +func TestStartRefusesAnUnusableConfiguredKey(t *testing.T) { + cases := map[string]func(t *testing.T, dir, keyPath string, key []byte){ + "missing": func(t *testing.T, dir, keyPath string, key []byte) { os.Remove(keyPath) }, + "a directory": func(t *testing.T, dir, keyPath string, key []byte) { + os.Remove(keyPath) + if err := os.Mkdir(keyPath, 0o700); err != nil { + t.Fatal(err) + } + }, + "31 bytes": func(t *testing.T, dir, keyPath string, key []byte) { os.WriteFile(keyPath, key[:31], 0o600) }, + "33 bytes": func(t *testing.T, dir, keyPath string, key []byte) { + os.WriteFile(keyPath, append(slices.Clone(key), 0), 0o600) + }, + "a link to nothing": func(t *testing.T, dir, keyPath string, key []byte) { + os.Remove(keyPath) + if err := ostest.Symlink(t, filepath.Join(filepath.Dir(keyPath), "not-mounted"), keyPath); err != nil { + t.Skipf("no symbolic links here: %v", err) + } + }, + "a second key in the data directory": func(t *testing.T, dir, keyPath string, key []byte) { + if err := os.WriteFile(filepath.Join(dir, core.SecretKeyFile), key, 0o600); err != nil { + t.Fatal(err) + } + }, + "a link into the data directory": func(t *testing.T, dir, keyPath string, key []byte) { + inner := filepath.Join(dir, "somewhere", "k") + if err := os.MkdirAll(filepath.Dir(inner), 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(inner, key, 0o600); err != nil { + t.Fatal(err) + } + os.Remove(keyPath) + if err := ostest.Symlink(t, inner, keyPath); err != nil { + t.Skipf("no symbolic links here: %v", err) + } + }, + "unreadable": func(t *testing.T, dir, keyPath string, key []byte) { + if ostest.Windows || os.Geteuid() == 0 { + t.Skip("needs Unix modes and a user that is not the superuser") + } + if err := os.Chmod(keyPath, 0); err != nil { + t.Fatal(err) + } + }, + } + for name, breakIt := range cases { + t.Run(name, func(t *testing.T) { + dir := t.TempDir() + config, key, h := hostWithSecrets(t, dir) + if err := h.Close(); err != nil { + t.Fatal(err) + } + keyPath := keyOutside(t, dir, config) + breakIt(t, dir, keyPath, key) + keyDir := filepath.Dir(keyPath) + beforeKeys := listed(t, keyDir) + h2, err := Start(context.Background(), configModeOptions(config)) + if err == nil { + h2.Close() + t.Fatal("the host started") + } + if exitCode(err) != ExitConfig { + t.Errorf("exit status %d (%v), want %d", exitCode(err), err, ExitConfig) + } + if !strings.Contains(err.Error(), keyPath) && !strings.Contains(err.Error(), filepath.Join(dir, core.SecretKeyFile)) { + t.Errorf("the message names neither the key path nor the second key: %v", err) + } + if !strings.Contains(err.Error(), core.KeySetting) { + t.Errorf("the message does not name the setting: %v", err) + } + if got := listed(t, keyDir); !slices.Equal(got, beforeKeys) { + t.Errorf("the key folder changed: %v -> %v", beforeKeys, got) + } + noTemporaryKeyFile(t, dir, keyDir) + }) + } +} + +// The refusal holds when flats.db holds no sealed value, and the host never creates the key (rule 5). +func TestStartWithAConfiguredMissingKeyOfAFreshHostCreatesNoKey(t *testing.T) { + dir := t.TempDir() + config := initConfig(t, dir) + keyPath := filepath.Join(t.TempDir(), "secret.key") + var out bytes.Buffer + if err := ConfigCommand(context.Background(), []string{"set", "host.secret_key_path", keyPath, "--config", config}, &out); err != nil { + t.Fatal(err, out.String()) + } + h, err := Start(context.Background(), configModeOptions(config)) + if err == nil { + h.Close() + t.Fatal("the host started and made a key") + } + if exitCode(err) != ExitConfig || !strings.Contains(err.Error(), "never creates a key") { + t.Errorf("err = %v (status %d)", err, exitCode(err)) + } + if got := listed(t, filepath.Dir(keyPath)); len(got) != 0 { + t.Errorf("the key folder holds %v", got) + } + if _, err := os.Lstat(filepath.Join(dir, core.SecretKeyFile)); !os.IsNotExist(err) { + t.Errorf("a key was created in the data directory: %v", err) + } +} + +// `flats key set` with the setting and no file refuses and creates nothing (rule 9). `key ls` and `key rm` touch no key. +func TestKeySetWithAConfiguredMissingKeyRefuses(t *testing.T) { + clearKeyEnv(t) + dir := t.TempDir() + config := initConfig(t, dir) + keyPath := filepath.Join(t.TempDir(), "secret.key") + if err := ConfigCommand(context.Background(), []string{"set", "host.secret_key_path", keyPath, "--config", config}, &bytes.Buffer{}); err != nil { + t.Fatal(err) + } + var out bytes.Buffer + err := KeyCommand(context.Background(), []string{"set", "tailscale", "--config", config}, strings.NewReader(keyA), &out) + if err == nil || exitCode(err) != ExitConfig || !strings.Contains(err.Error(), "never creates a key") { + t.Fatalf("key set: err = %v (status %d)", err, exitCode(err)) + } + if got := listed(t, filepath.Dir(keyPath)); len(got) != 0 { + t.Errorf("the key folder holds %v", got) + } + for _, args := range [][]string{{"ls"}, {"rm", "tailscale"}} { + if err := KeyCommand(context.Background(), append(args, "--config", config), strings.NewReader(""), &out); err != nil { + t.Errorf("key %s with a missing configured key: %v", args[0], err) + } + } +} + +// Operator keys read the key file at each use (rule 8): a replaced file changes what they open, and a second key +// that appears in the data directory makes them refuse. +func TestOperatorKeysReadTheConfiguredFileAtEachUse(t *testing.T) { + ctx := context.Background() + dir := t.TempDir() + config, key, h := hostWithSecrets(t, dir) + if err := h.Close(); err != nil { + t.Fatal(err) + } + keyPath := keyOutside(t, dir, config) + h2, err := Start(ctx, configModeOptions(config)) + if err != nil { + t.Fatal(err) + } + defer h2.Close() + open := func() (string, error) { + v, _, err := core.StoredHostKey(ctx, h2.keyFile(), h2.Store, core.HostKeyZrok) + return v, err + } + if v, err := open(); err != nil || v != "zrok-token" { + t.Fatalf("open = %q, %v", v, err) + } + if err := os.WriteFile(keyPath, bytes.Repeat([]byte{5}, 32), 0o600); err != nil { + t.Fatal(err) + } + if _, err := open(); err == nil { + t.Error("a replaced key file still opened the operator key") + } + if err := os.WriteFile(keyPath, key, 0o600); err != nil { + t.Fatal(err) + } + if v, err := open(); err != nil || v != "zrok-token" { + t.Fatalf("open after the key came back = %q, %v", v, err) + } + if err := os.WriteFile(filepath.Join(dir, core.SecretKeyFile), key, 0o600); err != nil { + t.Fatal(err) + } + if _, err := open(); err == nil || !strings.Contains(err.Error(), "move that file away") { + t.Errorf("a second key in the data directory: err = %v, want a refusal", err) + } + // Sealing a new operator key goes through the same location. + if err := core.PutHostKeyValue(ctx, h2.keyFile(), h2.Store, core.HostKeyTailscale, keyA, core.KeySourceConsole, time.Now()); err == nil { + t.Error("an operator key was sealed while two keys existed") + } +} diff --git a/internal/backup/backup.go b/internal/backup/backup.go index c338ae97..da226098 100644 --- a/internal/backup/backup.go +++ b/internal/backup/backup.go @@ -224,7 +224,7 @@ func Run(ctx context.Context, o Options) (res *Result, err error) { if d := s1.diff(s2); d != "" { return nil, &RerunError{fmt.Errorf("a flat was created, renamed, deleted or restored during the copy (%s)", d)} } - key1, err := readKey(keyFile, o.ExcludeSecretKey) + key1, err := readKeyAgain(keyFile, o.ExcludeSecretKey) if err != nil { if keyFile.Configured() { return nil, &RerunError{fmt.Errorf("the secret key at %s cannot be read any more: %w", keyFile.Path(), err)} @@ -331,6 +331,17 @@ func readKey(k core.KeyFile, excluded bool) ([]byte, error) { return readKeyBytes(k.DataDir(), false) } +// readKeyAgain is readKey for the second read of rule 22, which does not repeat the checks of rule 3. +func readKeyAgain(k core.KeyFile, excluded bool) ([]byte, error) { + if excluded { + return nil, nil + } + if k.Configured() { + return k.ReadAgain() + } + return readKeyBytes(k.DataDir(), false) +} + // readKeyBytes returns the bytes of secret.key, following a link as the host // does, or nil when the file is not there or the key is excluded. func readKeyBytes(data string, excluded bool) ([]byte, error) { diff --git a/internal/backup/externalkey_test.go b/internal/backup/externalkey_test.go new file mode 100644 index 00000000..a3c9e615 --- /dev/null +++ b/internal/backup/externalkey_test.go @@ -0,0 +1,363 @@ +package backup + +import ( + "bytes" + "context" + "crypto/rand" + "errors" + "io/fs" + "os" + "path/filepath" + "slices" + "strings" + "testing" + "time" + + "github.com/gosuda/flats/internal/core" + "github.com/gosuda/flats/internal/ostest" + "github.com/gosuda/flats/internal/store" +) + +// docs/spec/secret-key-path.md, rules 16 to 20: a host whose key is outside the data directory. + +// externalKey moves the fixture's key out of the data directory, to a file of its own, and returns the path and the +// bytes. The bytes are random, so that a scan of the copy for them cannot hit by accident. +func (f *fixture) externalKey() (path string, key []byte) { + f.t.Helper() + key = make([]byte, 32) + if _, err := rand.Read(key); err != nil { + f.t.Fatal(err) + } + path = filepath.Join(f.t.TempDir(), "flats.key") + if err := os.WriteFile(path, key, 0o600); err != nil { + f.t.Fatal(err) + } + if err := os.Remove(filepath.Join(f.data, nameSecretKey)); err != nil { + f.t.Fatal(err) + } + return path, key +} + +// seal stores an operator key in flats.db, sealed under the key at keyPath (a default key in dataDir when keyPath is ""). +func (f *fixture) seal(dataDir, keyPath string) { + f.t.Helper() + k, err := core.SealHostKey(core.NewKeyFile(dataDir, keyPath), core.HostKeyZrok, "zrok-token", core.KeySourceCLI, time.Now()) + if err != nil { + f.t.Fatal(err) + } + st, err := store.Open(filepath.Join(f.data, nameFlatsDB)) + if err != nil { + f.t.Fatal(err) + } + defer st.Close() + if err := st.PutHostKey(context.Background(), k); err != nil { + f.t.Fatal(err) + } + if err := st.Sync(context.Background()); err != nil { + f.t.Fatal(err) + } +} + +// holdsBytes lists the files of a copy that hold needle. +func holdsBytes(t *testing.T, root string, needle []byte) []string { + t.Helper() + var hits []string + err := filepath.WalkDir(root, func(p string, d fs.DirEntry, err error) error { + if err != nil || d.IsDir() { + return err + } + b, err := os.ReadFile(p) + if err != nil { + return err + } + if bytes.Contains(b, needle) { + hits = append(hits, p) + } + return nil + }) + if err != nil { + t.Fatal(err) + } + return hits +} + +func TestRunWithAnExternalKey(t *testing.T) { + t.Parallel() + f := populated(t) + f.seal(f.data, "") // sealed under the key that populated made; the next line moves a key out and reseals + keyPath, key := f.externalKey() + // Seal again under the external key, and drop the value that the old key sealed. + st, err := store.Open(filepath.Join(f.data, nameFlatsDB)) + if err != nil { + t.Fatal(err) + } + if err := st.DeleteHostKey(context.Background(), core.HostKeyZrok); err != nil { + t.Fatal(err) + } + st.Close() + f.seal(f.data, keyPath) + o := f.options("c1") + o.SecretKeyPath = keyPath + res, err := f.run(o) + if err != nil { + t.Fatal(err) + } + if res.SecretKey != SecretKeyExternal || res.KeyPath != keyPath { + t.Errorf("result key = %q at %q, want external at %q", res.SecretKey, res.KeyPath, keyPath) + } + m := readCopyManifest(t, res.Out) + if m.SecretKey != SecretKeyExternal || m.has("data/secret.key") { + t.Errorf("manifest: secret_key %q, files %v", m.SecretKey, m.Files) + } + mustNotExist(t, filepath.Join(res.Out, "data", nameSecretKey)) + if hits := holdsBytes(t, res.Out, key); len(hits) != 0 { + t.Errorf("the copy holds the key in %v", hits) + } + got, err := Verify(context.Background(), res.Out) + if err != nil || len(got.Failures) > 0 { + t.Fatalf("Verify = %+v, %v", got, err) + } + if len(got.Notes) != 1 || !strings.Contains(got.Notes[0], "the key is outside the copy") { + t.Errorf("notes = %v; want the note that the key is outside the copy", got.Notes) + } +} + +// A key that does not open the sealed values fails the backup, and a second run does not help (rule 17). +func TestRunFailsWhenTheExternalKeyDoesNotOpenTheSealedValues(t *testing.T) { + t.Parallel() + f := populated(t) + f.seal(f.data, "") // sealed under the old key, which is about to be replaced by another + keyPath, _ := f.externalKey() + o := f.options("c1") + o.SecretKeyPath = keyPath + _, err := f.run(o) + var rerun *RerunError + if err == nil || errors.As(err, &rerun) || !strings.Contains(err.Error(), "does not open with the key at host.secret_key_path") { + t.Fatalf("err = %v, want a failure that a second run does not clear", err) + } + f.nothingLeft() +} + +// A hard link to the key under another name in the data directory escapes the checks of rule 3. The scan of the +// copy finds it (rule 16). +func TestRunFailsWhenAHardLinkHoldsTheExternalKey(t *testing.T) { + t.Parallel() + f := populated(t) + keyPath, _ := f.externalKey() + if err := os.Link(keyPath, filepath.Join(f.data, "tsnet", "alias.bin")); err != nil { + t.Skipf("no hard links here: %v", err) + } + o := f.options("c1") + o.SecretKeyPath = keyPath + _, err := f.run(o) + var rerun *RerunError + if err == nil || errors.As(err, &rerun) || !strings.Contains(err.Error(), "tsnet/alias.bin") || !strings.Contains(err.Error(), "holds the bytes of the secret key") { + t.Fatalf("err = %v, want a failure that names the file and that a second run does not clear", err) + } + f.nothingLeft() +} + +// A file that appears at /secret.key during the copy is left out and listed (rule 16). +func TestRunLeavesOutAKeyThatAppearsInTheDataDirectoryDuringTheCopy(t *testing.T) { + t.Parallel() + f := populated(t) + keyPath, key := f.externalKey() + o := f.options("c1") + o.SecretKeyPath = keyPath + o.afterStep = func(step int) error { + if step == 14 { + return os.WriteFile(filepath.Join(f.data, nameSecretKey), key, 0o600) + } + return nil + } + res, err := f.run(o) + if err != nil { + t.Fatal(err) + } + m := readCopyManifest(t, res.Out) + if !slices.Contains(m.Excluded, "data/secret.key") || m.has("data/secret.key") { + t.Errorf("excluded %v, files has the key: %v", m.Excluded, m.has("data/secret.key")) + } + if hits := holdsBytes(t, res.Out, key); len(hits) != 0 { + t.Errorf("the copy holds the key in %v", hits) + } +} + +func TestRunRefusesInTheCasesOfRule3(t *testing.T) { + t.Parallel() + t.Run("a second key in the data directory", func(t *testing.T) { + t.Parallel() + f := populated(t) + keyPath, key := f.externalKey() + if err := os.WriteFile(filepath.Join(f.data, nameSecretKey), key, 0o600); err != nil { + t.Fatal(err) + } + o := f.options("c1") + o.SecretKeyPath = keyPath + _, err := f.run(o) + var refused *RefusedError + if !errors.As(err, &refused) || !strings.Contains(err.Error(), "move that file away") { + t.Fatalf("err = %v, want a refusal", err) + } + f.nothingLeft() + }) + t.Run("a key that resolves into the data directory", func(t *testing.T) { + t.Parallel() + f := populated(t) + inner := filepath.Join(f.data, "tsnet", "inner.key") + if err := os.WriteFile(inner, bytes.Repeat([]byte{1}, 32), 0o600); err != nil { + t.Fatal(err) + } + os.Remove(filepath.Join(f.data, nameSecretKey)) + link := filepath.Join(t.TempDir(), "flats.key") + if err := ostest.Symlink(t, inner, link); err != nil { + t.Skipf("no symbolic links here: %v", err) + } + o := f.options("c1") + o.SecretKeyPath = link + _, err := f.run(o) + var refused *RefusedError + if !errors.As(err, &refused) || !strings.Contains(err.Error(), "inside the data directory") { + t.Fatalf("err = %v, want a refusal", err) + } + f.nothingLeft() + }) +} + +// A key that cannot be read refuses the backup in the preflight and names --exclude-secret-key. The flag makes the +// command skip the key (rules 17 and 18). +func TestRunWithAnExternalKeyThatIsMissing(t *testing.T) { + t.Parallel() + f := populated(t) + keyPath, _ := f.externalKey() + if err := os.Remove(keyPath); err != nil { + t.Fatal(err) + } + o := f.options("c1") + o.SecretKeyPath = keyPath + var steps []int + o.afterStep = func(step int) error { steps = append(steps, step); return nil } + _, err := f.run(o) + var refused *RefusedError + if !errors.As(err, &refused) || !strings.Contains(err.Error(), "--exclude-secret-key") || !strings.Contains(err.Error(), keyPath) { + t.Fatalf("err = %v, want a refusal that names the key and the flag", err) + } + if len(steps) != 0 { + t.Errorf("the backup ran steps %v before it refused; the refusal is in the preflight", steps) + } + f.nothingLeft() + + o = f.options("c2") + o.SecretKeyPath, o.ExcludeSecretKey = keyPath, true + res, err := f.run(o) + if err != nil { + t.Fatalf("with --exclude-secret-key: %v", err) + } + if res.SecretKey != SecretKeyExternal { + t.Errorf("secret_key = %q, want external", res.SecretKey) + } +} + +// Rule 22 of host backup applies to the external key: a key that changes or is gone between step 13 and step 18 fails the +// copy. +func TestRunFailsWhenTheExternalKeyChangesOrGoes(t *testing.T) { + t.Parallel() + for name, change := range map[string]func(t *testing.T, path string){ + "changes": func(t *testing.T, path string) { + if err := os.WriteFile(path, bytes.Repeat([]byte{9}, 32), 0o600); err != nil { + t.Fatal(err) + } + }, + "goes": func(t *testing.T, path string) { + if err := os.Remove(path); err != nil { + t.Fatal(err) + } + }, + } { + t.Run(name, func(t *testing.T) { + t.Parallel() + f := populated(t) + keyPath, _ := f.externalKey() + o := f.options("c1") + o.SecretKeyPath = keyPath + o.afterStep = func(step int) error { + if step == 14 { + change(t, keyPath) + } + return nil + } + _, err := f.run(o) + var rerun *RerunError + if !errors.As(err, &rerun) || !strings.Contains(err.Error(), "run the backup again") || !strings.Contains(err.Error(), keyPath) { + t.Fatalf("err = %v, want a failure to run again that names the key", err) + } + f.nothingLeft() + }) + } +} + +// With --exclude-secret-key the command does not read the key, so a change is not noticed and no scan runs (rule 18). +func TestRunWithAnExcludedExternalKeyReadsNothing(t *testing.T) { + t.Parallel() + f := populated(t) + keyPath, key := f.externalKey() + if err := os.Link(keyPath, filepath.Join(f.data, "tsnet", "alias.bin")); err != nil { + t.Skipf("no hard links here: %v", err) + } + o := f.options("c1") + o.SecretKeyPath, o.ExcludeSecretKey = keyPath, true + res, err := f.run(o) + if err != nil { + t.Fatal(err) + } + // The copy holds the key under the name of the hard link: the scan did not run, as the flag says. + if hits := holdsBytes(t, res.Out, key); len(hits) != 1 { + t.Errorf("hits = %v; this test pins that the flag skips the scan", hits) + } +} + +func TestHashScanFindsAKeyAcrossTheBoundaryOfAReadAndHashesTheFile(t *testing.T) { + t.Parallel() + needle := bytes.Repeat([]byte{0xCD}, 32) + const size = 3*256<<10 + 100 + buf := *bufPool.Get().(*[]byte) + chunk := len(buf) + bufPool.Put(&buf) + for name, at := range map[string]int{ + "at the start": 0, + "across the boundary": chunk - 16, + "just before it": chunk - 32, + "just after it": chunk, + "in the second read": chunk + 5000, + "at the very end": size - 32, + } { + t.Run(name, func(t *testing.T) { + t.Parallel() + data := make([]byte, size) + if _, err := rand.Read(data); err != nil { + t.Fatal(err) + } + copy(data[at:], needle) + p := filepath.Join(t.TempDir(), "f") + if err := os.WriteFile(p, data, 0o600); err != nil { + t.Fatal(err) + } + got, found, err := hashScan(p, needle) + if err != nil || !found { + t.Fatalf("hashScan = %v, found %v; want a hit at %d", err, found, at) + } + want, err := hashFile(p) + if err != nil || got != want { + t.Errorf("hashScan hash/size = %+v, hashFile = %+v, %v", got, want, err) + } + // Without the needle nothing is found, and the hash is the same. + plain := bytes.ReplaceAll(data, needle, bytes.Repeat([]byte{1}, 32)) + if err := os.WriteFile(p, plain, 0o600); err != nil { + t.Fatal(err) + } + if _, found, err := hashScan(p, needle); err != nil || found { + t.Errorf("hashScan of a file without the needle: found %v, %v", found, err) + } + }) + } +} diff --git a/internal/core/keyfile.go b/internal/core/keyfile.go index aaf962c9..667e2a28 100644 --- a/internal/core/keyfile.go +++ b/internal/core/keyfile.go @@ -88,6 +88,21 @@ func (k KeyFile) Read() ([]byte, error) { if err := k.Check(); err != nil { return nil, err } + return k.readConfigured() +} + +// ReadAgain reads the key a second time without the checks of Check. A backup that compares two reads of +// a configured key (rule 22 of the backup spec) makes those checks once, in its preflight, and a second key +// that appears in the data directory during the copy is left out of the copy and does not fail the re-read. +func (k KeyFile) ReadAgain() ([]byte, error) { + if !k.configured { + return readSecretKey(k.path) + } + return k.readConfigured() +} + +// readConfigured reads the file of a configured key and names the path and the setting in every refusal. +func (k KeyFile) readConfigured() ([]byte, error) { if _, err := os.Stat(k.path); err != nil { if !errors.Is(err, fs.ErrNotExist) { return nil, fmt.Errorf("%w: %s names %s: %v", ErrKeyLocation, KeySetting, k.path, err) diff --git a/internal/core/keyfile_test.go b/internal/core/keyfile_test.go new file mode 100644 index 00000000..8f64d3cb --- /dev/null +++ b/internal/core/keyfile_test.go @@ -0,0 +1,398 @@ +package core + +import ( + "bytes" + "context" + "errors" + "os" + "path/filepath" + "slices" + "strings" + "testing" + "time" + + "github.com/gosuda/flats/internal/ostest" + "github.com/gosuda/flats/internal/store" +) + +// listDir lists the entries of a folder, to show that a refusal or a read changed nothing. +func listDir(t *testing.T, dir string) []string { + t.Helper() + entries, err := os.ReadDir(dir) + if err != nil { + t.Fatal(err) + } + var out []string + for _, e := range entries { + out = append(out, e.Name()) + } + return out +} + +// needsAnUnprivilegedUnixUser skips a test that depends on a mode the superuser ignores, or on a mode that Windows +// does not have. +func needsAnUnprivilegedUnixUser(t *testing.T) { + t.Helper() + if ostest.Windows || os.Geteuid() == 0 { + t.Skip("needs Unix modes and a user that is not the superuser") + } +} + +func goodKey() []byte { return bytes.Repeat([]byte{7}, 32) } + +func TestDefaultKeyFileKeepsTheOldBehavior(t *testing.T) { + t.Parallel() + dir := t.TempDir() + k := NewKeyFile(dir, "") + if k.Configured() || k.Path() != filepath.Join(dir, SecretKeyFile) || k.DataDir() != dir { + t.Fatalf("KeyFile = %+v", k) + } + if err := k.Check(); err != nil { + t.Errorf("Check of a default key = %v, want nil", err) + } + if _, err := k.Read(); !errors.Is(err, os.ErrNotExist) || errors.Is(err, ErrKeyLocation) { + t.Errorf("Read of a missing default key = %v, want a plain not-exist error", err) + } + key, err := k.LoadOrCreate() + if err != nil || len(key) != 32 { + t.Fatalf("LoadOrCreate = %v, %v; want a new key", len(key), err) + } + if again, err := k.LoadOrCreate(); err != nil || !bytes.Equal(again, key) { + t.Errorf("the second LoadOrCreate = %v; want the same key", err) + } +} + +// With host.secret_key_path the host reads the file and never creates it (rules 5 to 7 of the spec). +func TestConfiguredKeyIsNeverCreated(t *testing.T) { + t.Parallel() + data, keyDir := t.TempDir(), t.TempDir() + path := filepath.Join(keyDir, "secret.key") + k := NewKeyFile(data, path) + if !k.Configured() || k.Path() != path { + t.Fatalf("KeyFile = %+v", k) + } + for name, call := range map[string]func() error{ + "LoadOrCreate": func() error { _, err := k.LoadOrCreate(); return err }, + "Read": func() error { _, err := k.Read(); return err }, + "SealHostKey": func() error { + _, err := SealHostKey(k, HostKeyTailscale, "tskey-auth-x", KeySourceCLI, time.Now()) + return err + }, + } { + err := call() + if !errors.Is(err, ErrKeyLocation) { + t.Errorf("%s: err = %v, want ErrKeyLocation", name, err) + continue + } + for _, want := range []string{path, KeySetting, "never creates", "head -c 32 /dev/urandom", "flats secret restore-key", "flats config unset"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("%s: the message lacks %q: %v", name, want, err) + } + } + } + if got := listDir(t, keyDir); len(got) != 0 { + t.Errorf("the key folder holds %v after the refusals", got) + } + if got := listDir(t, data); len(got) != 0 { + t.Errorf("the data directory holds %v after the refusals", got) + } +} + +func TestConfiguredKeyThatIsNotAKey(t *testing.T) { + t.Parallel() + for name, prepare := range map[string]func(t *testing.T, path string){ + "a directory": func(t *testing.T, path string) { + if err := os.Mkdir(path, 0o700); err != nil { + t.Fatal(err) + } + }, + "31 bytes": func(t *testing.T, path string) { putFile(t, path, bytes.Repeat([]byte{1}, 31)) }, + "33 bytes": func(t *testing.T, path string) { putFile(t, path, bytes.Repeat([]byte{1}, 33)) }, + "a newline after the key": func(t *testing.T, path string) { + putFile(t, path, append(goodKey(), '\n')) + }, + } { + t.Run(name, func(t *testing.T) { + t.Parallel() + data, keyDir := t.TempDir(), t.TempDir() + path := filepath.Join(keyDir, "secret.key") + prepare(t, path) + _, err := NewKeyFile(data, path).Read() + if !errors.Is(err, ErrKeyLocation) || !errors.Is(err, ErrInvalidSecretKey) { + t.Fatalf("err = %v, want ErrKeyLocation and ErrInvalidSecretKey", err) + } + if !strings.Contains(err.Error(), path) || !strings.Contains(err.Error(), KeySetting) { + t.Errorf("the message does not name the path and the setting: %v", err) + } + }) + } +} + +func putFile(t *testing.T, path string, b []byte) { + t.Helper() + if err := os.WriteFile(path, b, 0o600); err != nil { + t.Fatal(err) + } +} + +func TestConfiguredKeyThatIsALinkToNothing(t *testing.T) { + t.Parallel() + data, keyDir := t.TempDir(), t.TempDir() + path := filepath.Join(keyDir, "secret.key") + if err := ostest.Symlink(t, filepath.Join(keyDir, "not-mounted"), path); err != nil { + t.Skipf("no symbolic links here: %v", err) + } + _, err := NewKeyFile(data, path).Read() + if !errors.Is(err, ErrKeyLocation) { + t.Fatalf("err = %v, want ErrKeyLocation", err) + } + if !strings.Contains(err.Error(), "link to nothing") { + t.Errorf("the message does not say that the path is a link to nothing: %v", err) + } + if strings.Contains(err.Error(), "restore-key") { + t.Errorf("the message offers restore-key, which refuses anything at the path: %v", err) + } +} + +// A valid key works from a read-only folder, with mode 0444, and nothing is created (rule 7). +func TestConfiguredKeyInAReadOnlyPlace(t *testing.T) { + needsAnUnprivilegedUnixUser(t) + t.Parallel() + data, keyDir := t.TempDir(), t.TempDir() + path := filepath.Join(keyDir, "secret.key") + putFile(t, path, goodKey()) + if err := os.Chmod(path, 0o444); err != nil { + t.Fatal(err) + } + if err := os.Chmod(keyDir, 0o555); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { os.Chmod(keyDir, 0o700) }) + k := NewKeyFile(data, path) + for i := 0; i < 2; i++ { + got, err := k.LoadOrCreate() + if err != nil || !bytes.Equal(got, goodKey()) { + t.Fatalf("LoadOrCreate = %v, %v", got, err) + } + } + if err := os.Chmod(keyDir, 0o700); err != nil { + t.Fatal(err) + } + if got := listDir(t, keyDir); !slices.Equal(got, []string{"secret.key"}) { + t.Errorf("the key folder holds %v", got) + } +} + +func TestConfiguredKeyThatCannotBeRead(t *testing.T) { + needsAnUnprivilegedUnixUser(t) + t.Parallel() + data, keyDir := t.TempDir(), t.TempDir() + path := filepath.Join(keyDir, "secret.key") + putFile(t, path, goodKey()) + if err := os.Chmod(path, 0); err != nil { + t.Fatal(err) + } + _, err := NewKeyFile(data, path).Read() + if !errors.Is(err, ErrKeyLocation) || !strings.Contains(err.Error(), path) { + t.Errorf("err = %v, want ErrKeyLocation that names the path", err) + } +} + +// Two keys would be in use, and a copy of the data directory would hold one of them (rule 3). +func TestConfiguredKeyRefusesALeftoverKeyInTheDataDirectory(t *testing.T) { + t.Parallel() + for name, prepare := range map[string]func(t *testing.T, leftover string){ + "a file": func(t *testing.T, p string) { putFile(t, p, goodKey()) }, + "a directory": func(t *testing.T, p string) { os.Mkdir(p, 0o700) }, + "a link": func(t *testing.T, p string) { + if err := ostest.Symlink(t, filepath.Join(filepath.Dir(p), "nowhere"), p); err != nil { + t.Skipf("no symbolic links here: %v", err) + } + }, + } { + t.Run(name, func(t *testing.T) { + t.Parallel() + data, keyDir := t.TempDir(), t.TempDir() + path := filepath.Join(keyDir, "secret.key") + putFile(t, path, goodKey()) + leftover := filepath.Join(data, SecretKeyFile) + prepare(t, leftover) + for what, err := range map[string]error{"Check": NewKeyFile(data, path).Check(), "Read": readErr(NewKeyFile(data, path))} { + if !errors.Is(err, ErrKeyLocation) || !strings.Contains(err.Error(), leftover) || !strings.Contains(err.Error(), "move that file away") { + t.Errorf("%s: err = %v, want ErrKeyLocation that names %s", what, err, leftover) + } + } + // With no setting the same file is the key, and nothing is refused. + if err := NewKeyFile(data, "").Check(); err != nil { + t.Errorf("Check of a default key: %v", err) + } + }) + } +} + +func readErr(k KeyFile) error { _, err := k.Read(); return err } + +// A key whose resolved path lies inside the resolved data directory is refused, whatever name it was given (rule 3). +func TestConfiguredKeyThatResolvesIntoTheDataDirectory(t *testing.T) { + t.Parallel() + t.Run("a link into the data directory", func(t *testing.T) { + t.Parallel() + data, keyDir := t.TempDir(), t.TempDir() + inner := filepath.Join(data, "keys", "k") + if err := os.MkdirAll(filepath.Dir(inner), 0o700); err != nil { + t.Fatal(err) + } + putFile(t, inner, goodKey()) + path := filepath.Join(keyDir, "secret.key") + if err := ostest.Symlink(t, inner, path); err != nil { + t.Skipf("no symbolic links here: %v", err) + } + err := readErr(NewKeyFile(data, path)) + if !errors.Is(err, ErrKeyLocation) || !strings.Contains(err.Error(), "inside the data directory") { + t.Errorf("err = %v, want a refusal that says the key resolves inside the data directory", err) + } + }) + t.Run("a data directory that is a link to the key's folder", func(t *testing.T) { + t.Parallel() + real, root := t.TempDir(), t.TempDir() + putFile(t, filepath.Join(real, "secret.key"), goodKey()) + data := filepath.Join(root, "data") + if err := ostest.Symlink(t, real, data); err != nil { + t.Skipf("no symbolic links here: %v", err) + } + // The key is configured at its real path, which lies in what the data directory resolves to. The leftover + // check does not see it, because /secret.key resolves to the same file, so look at it first. + err := readErr(NewKeyFile(data, filepath.Join(real, "elsewhere"))) + if err == nil { + t.Log("the path of the key does not exist, so there is nothing to resolve") + } + err = readErr(NewKeyFile(data, filepath.Join(real, "secret.key"))) + if !errors.Is(err, ErrKeyLocation) { + t.Errorf("err = %v, want ErrKeyLocation", err) + } + }) + t.Run("a key outside the data directory is fine", func(t *testing.T) { + t.Parallel() + data, keyDir := t.TempDir(), t.TempDir() + path := filepath.Join(keyDir, "secret.key") + putFile(t, path, goodKey()) + if got, err := NewKeyFile(data, path).Read(); err != nil || !bytes.Equal(got, goodKey()) { + t.Errorf("Read = %v, %v", got, err) + } + }) +} + +// CheckSecretKey of a configured key refuses with no sealed value in flats.db too, and never creates the key (rule 5). +func TestCheckSecretKeyOfAConfiguredKey(t *testing.T) { + t.Parallel() + data, keyDir := t.TempDir(), t.TempDir() + st, err := store.Open(filepath.Join(data, "flats.db")) + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { st.Close() }) + ctx := context.Background() + path := filepath.Join(keyDir, "secret.key") + if err := CheckSecretKey(ctx, NewKeyFile(data, path), st); !errors.Is(err, ErrKeyLocation) { + t.Errorf("a missing configured key: err = %v, want ErrKeyLocation", err) + } + // The default key, with no sealed value, is left for the start to create. + if err := CheckSecretKey(ctx, NewKeyFile(data, ""), st); err != nil { + t.Errorf("a missing default key with no sealed value: %v", err) + } + putFile(t, path, goodKey()) + if err := CheckSecretKey(ctx, NewKeyFile(data, path), st); err != nil { + t.Errorf("a valid configured key: %v", err) + } + if got := listDir(t, keyDir); !slices.Equal(got, []string{"secret.key"}) { + t.Errorf("the key folder holds %v", got) + } +} + +// Restore writes beside the configured path, never replaces a file, and leaves no temporary file (rule 12). +func TestRestoreOfAConfiguredKey(t *testing.T) { + t.Parallel() + data, keyDir := t.TempDir(), t.TempDir() + path := filepath.Join(keyDir, "secret.key") + k := NewKeyFile(data, path) + if err := k.Restore(goodKey()); err != nil { + t.Fatal(err) + } + if got, err := k.Read(); err != nil || !bytes.Equal(got, goodKey()) { + t.Fatalf("Read = %v, %v", got, err) + } + if err := k.Restore(bytes.Repeat([]byte{9}, 32)); !os.IsExist(err) { + t.Errorf("a second Restore: err = %v, want os.IsExist", err) + } + if got := listDir(t, keyDir); !slices.Equal(got, []string{"secret.key"}) { + t.Errorf("the key folder holds %v; want the key and no temporary file", got) + } + if got := listDir(t, data); len(got) != 0 { + t.Errorf("the data directory holds %v; the temporary file lies beside the configured path", got) + } +} + +func TestRestoreOfAConfiguredKeyInAReadOnlyFolder(t *testing.T) { + needsAnUnprivilegedUnixUser(t) + t.Parallel() + data, keyDir := t.TempDir(), t.TempDir() + if err := os.Chmod(keyDir, 0o555); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { os.Chmod(keyDir, 0o700) }) + k := NewKeyFile(data, filepath.Join(keyDir, "secret.key")) + if err := k.Restore(goodKey()); err == nil { + t.Fatal("Restore wrote into a read-only folder") + } + if err := os.Chmod(keyDir, 0o700); err != nil { + t.Fatal(err) + } + if got := listDir(t, keyDir); len(got) != 0 { + t.Errorf("the failed Restore left %v", got) + } +} + +func TestPathInside(t *testing.T) { + t.Parallel() + for _, tc := range []struct { + dir, p string + want bool + }{ + {"/a/b", "/a/b", true}, + {"/a/b", "/a/b/c", true}, + {"/a/b", "/a/bc", false}, + {"/a/b", "/a", false}, + {"/a/b", "/x/a/b", false}, + } { + if got := PathInside(filepath.FromSlash(tc.dir), filepath.FromSlash(tc.p)); got != tc.want { + t.Errorf("PathInside(%q, %q) = %v, want %v", tc.dir, tc.p, got, tc.want) + } + } +} + +// ReadAgain does not repeat the checks of Check: a second key that appears in the data directory during a backup does +// not fail the re-read, and a missing file still does. +func TestReadAgainSkipsTheChecksOfCheck(t *testing.T) { + t.Parallel() + data, keyDir := t.TempDir(), t.TempDir() + path := filepath.Join(keyDir, "secret.key") + putFile(t, path, goodKey()) + k := NewKeyFile(data, path) + putFile(t, filepath.Join(data, SecretKeyFile), goodKey()) + if _, err := k.Read(); !errors.Is(err, ErrKeyLocation) { + t.Fatalf("Read with a second key: err = %v, want ErrKeyLocation", err) + } + if got, err := k.ReadAgain(); err != nil || !bytes.Equal(got, goodKey()) { + t.Errorf("ReadAgain = %v, %v; want the key", got, err) + } + if err := os.Remove(path); err != nil { + t.Fatal(err) + } + if _, err := k.ReadAgain(); !errors.Is(err, ErrKeyLocation) { + t.Errorf("ReadAgain of a missing key: err = %v, want ErrKeyLocation", err) + } + // A default key is read as before. + if got, err := NewKeyFile(data, "").ReadAgain(); err != nil || !bytes.Equal(got, goodKey()) { + t.Errorf("ReadAgain of the default key = %v, %v", got, err) + } +} From f477bcecd4bfd7ec157995fd25fdd3ee5e2ece2c Mon Sep 17 00:00:00 2001 From: Hee Sung Son Date: Sat, 14 Nov 2026 18:37:59 +0900 Subject: [PATCH 07/14] docs(secrets): document host.secret_key_path and its effect on backup and recovery Adds the key lifecycle to configuration.md, a Docker secret section to container.md, and the settings row, design and CLI notes. Updates the host backup, secret key recovery and config reload specs, and marks the secret key path spec implemented. --- docs/configuration.md | 192 ++++++++++++++++++++++++++++--- docs/container.md | 55 ++++++++- docs/design.md | 9 +- docs/spec/README.md | 2 +- docs/spec/config-reload.md | 5 +- docs/spec/host-backup.md | 48 +++++--- docs/spec/secret-key-path.md | 13 ++- docs/spec/secret-key-recovery.md | 20 +++- docs/surfaces.md | 4 +- 9 files changed, 297 insertions(+), 51 deletions(-) diff --git a/docs/configuration.md b/docs/configuration.md index 2e724f9f..2c14862e 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -12,7 +12,7 @@ The question "who decides this value?" picks the place: | The operator | `config.json` | addresses, system policies, permitted networks, console domains, provider settings | | Flats while running | `flats.db` | flats, drafts, versions, deployments, revision metadata of documents, designs, boards and notebooks, approvals, per-flat provider permission, events, analytics (page views, and per-day usage in `usagedays`), ordinary app environment variables, encrypted secrets, encrypted operator keys, console node records, [Autopilot settings](#autopilot-settings), [public instance settings](#public-instance-settings) | | The flat | `flats//` | uploaded code, revision contents, the flat's SQLite database and FILES, a board's or notebook's current content | -| Nobody may read it in plain text | separate files | `secret.key` | +| Nobody may read it in plain text | separate files | `secret.key`, or the file that [`host.secret_key_path`](#keeping-the-key-outside-the-data-directory) names | | A client machine's user | `flats-client/connection.json` | which Flats host the CLI and `flats mcp` use, saved by `flats connect` | No value is stored in two places. `flats.db` also holds `host_binding`, which @@ -39,6 +39,7 @@ the data directory is. config-history/ previous config.json copies, newest 10 flats.db metadata database (+ -wal, -shm) secret.key key for flat secrets and operator keys, 32 bytes, mode 0600 + (not here when host.secret_key_path names another file) secret.key.recovery that key encrypted under a recovery password (optional) secret.key.recovery.id the ID of the key in that copy backups/ flats.db copies taken before a schema migration @@ -82,7 +83,8 @@ retention deletes revisions. Operator keys (the Tailscale auth key, the NetBird setup key, and a zrok token waiting to be used) are kept in `flats.db`, encrypted with `secret.key`; see -[Operator keys](#operator-keys). `config.json` holds no key and no path to one. +[Operator keys](#operator-keys). `config.json` holds no key. It holds a path to +the key file only when you set `host.secret_key_path`. A rename or deletion writes a journal under `flats/.lifecycle/` before it moves or removes a flat directory, and startup finishes or undoes an interrupted one @@ -106,7 +108,9 @@ Required: - `config.json`; - `flats.db`, together with `secret.key` from the same moment: the flat - secrets and operator keys in `flats.db` need it; + secrets and operator keys in `flats.db` need it. With + [`host.secret_key_path`](#keeping-the-key-outside-the-data-directory) the key + is a file outside the data directory, and you keep it apart from the copy; - `flats//` except the temporary copies, with `restore-journal.json`, revision contents and snapshots, and `flats/.lifecycle/` and `network-retirements/` when present; @@ -159,6 +163,17 @@ recovery copy `secret.key.recovery` stays in it, and anyone who holds the copy c guess its password offline ([Secret key recovery](spec/secret-key-recovery.md#the-recovery-copy)). +With [`host.secret_key_path`](#keeping-the-key-outside-the-data-directory) the key +is not in the data directory, so it is not in the copy. The manifest records +`secret_key: "external"`, and the command prints where the key is: back that file +up apart from the copy. The command reads the key, checks that it opens every +sealed value of the copy, and fails if the key changed during the copy. It +refuses before it writes anything when the key is missing or unreadable, and the +message names `--exclude-secret-key`; with the flag it does not read the key at +all. It also scans every file of the copy for the 32 bytes of the key, under any +name, and fails the backup when it finds them: run it again after you remove the +second copy of the key from the data directory. + The command works only on a data directory that a host of this Flats release wrote. After an upgrade, start the host once so that it migrates `flats.db`. To back up a stopped host of an older release before you start the new one, copy the data @@ -168,7 +183,9 @@ To check a copy, run `flats backup --verify PATH`. It needs no host and no configuration, and it writes nothing. It compares every file with the manifest, checks each database, checks that the outbox databases of documents, boards and notebooks fit `flats.db`, checks that every blob that a flat needs is in the copy, -and checks that `secret.key` opens every sealed value. It prints `ok` or each +and checks that `secret.key` opens every sealed value. For a copy with an +external key it has no key to use: it says in a note that the key is outside the +copy and that it did not check the sealed values. It prints `ok` or each failure, and exits with status 1 on a failure. It refuses, with status 78, a copy whose `flats.db` has another schema version than this Flats expects, and says which Flats version made it. Run `--verify` before every restore. @@ -179,7 +196,9 @@ directory away and copy `PATH/data` to the same path. If `PATH/config.json` and the folder beside it. Then start the host. A copy made with `--exclude-secret-key` needs the key first: put `secret.key` back from where you kept it, or run `flats secret restore-key` with the recovery password -([Recovering a lost secret.key](#recovering-a-lost-secretkey)). Copying over the old directory can +([Recovering a lost secret.key](#recovering-a-lost-secretkey)). A copy with an +external key brings back the `config.json` that holds `host.secret_key_path`: +put the key at that path before you start the host. Copying over the old directory can leave a `-wal` file beside a restored database and corrupt it. A copy at another path is not used, because `host.data_dir` in its `config.json` still names the old path and a service started with `--config` still reads the old file. @@ -319,6 +338,7 @@ written; `flats config unset KEY` removes it. | `host.console_addr` | `127.0.0.1:7878`; console, HTTP API and MCP listener | restart | | `host.local_addr` | `127.0.0.1:7879` | restart | | `host.server_runtime` | `true` | restart | +| `host.secret_key_path` | `""` (the key is `/secret.key`); an absolute path outside `host.data_dir` | offline: `flats config set host.secret_key_path`; a SIGHUP reload lists it as awaiting a restart, and the running host keeps its key | | `metrics.enabled` | `true` | restart | | `metrics.expose_on` | `["localhost"]`; unique entries from `localhost`, `console`, `external`; `[]` exposes nowhere | restart | | `metrics.per_flat` | `true`; set `false` to omit per-flat Prometheus series | restart | @@ -709,11 +729,13 @@ A key reaches Flats in one of these ways: - `flats key set tailscale|zrok|netbird` reads the key from standard input (a prompt without echo on a terminal) or `--from-file PATH`, never from the command line, and replaces a stored one. A zrok token waits for the - next start, which connects it when no account is connected. + next start, which connects it when no account is connected. With + `host.secret_key_path` it needs the key file there and never creates one. - `flats key ls` lists the keys with their state, source and time, never their values. - `flats key rm` removes a stored key or a waiting zrok token and needs no - `secret.key`. Disconnecting a zrok account stays in the console. + `secret.key`; `flats key ls` needs none either. Disconnecting a zrok + account stays in the console. - **At start**, in this order of priority: 1. the flags `--tailscale-authkey-file` (`--authkey-file` for a legacy service), `--zrok-token-file` and `--netbird-setup-key-file`, each naming @@ -746,7 +768,12 @@ on, while `TS_AUTHKEY`, `TS_AUTH_KEY`, `TS_CLIENT_SECRET`, `TS_CLIENT_ID`, read them and bypass Flats. If operator keys are stored and `secret.key` is missing, the host refuses to -start instead of creating a new key (see [Starting](#starting)). +start instead of creating a new key (see [Starting](#starting)). The console, +`flats key set` and the keys given at start read the key file each time they +seal or open a key, so a key file that you replace while the host runs changes +what they open at once. With +[`host.secret_key_path`](#keeping-the-key-outside-the-data-directory) they read +the file that it names. No MCP tool and no agent API route reads or sets a key. The console's routes are protected against cross-site requests, not authenticated: a program that @@ -941,16 +968,136 @@ when only operator keys are stored, remove them with `flats key rm` and register them again. [Recovering a lost secret.key](#recovering-a-lost-secretkey) covers the commands. +With `host.secret_key_path` the host never creates a key, with or without +sealed values. Before any listener opens, it checks the file that the setting +names, and a refusal exits with status 78 and names the path and the setting; +[Keeping the key outside the data directory](#keeping-the-key-outside-the-data-directory) +lists the cases. + +### Keeping the key outside the data directory + +By default `secret.key` lies in the data directory, so a copy of that directory +holds the sealed values and the key that opens them. `host.secret_key_path` +names another file for the key, for example a Docker secret or a file that only +the service user can read. The [Secret key path](spec/secret-key-path.md) spec +has the rules. + +```sh +flats config set host.secret_key_path /etc/flats/secret.key --config PATH +``` + +The value is an absolute path without a NUL byte, and it must not lie inside +`host.data_dir`. Stop the host first, because `flats config set` refuses while +a host runs. The setting applies at the next start. A SIGHUP reload lists it as +awaiting a restart and the running host keeps the key that it holds. + +With the setting, the host only reads the key. It never creates, writes, +renames or deletes the file, and it makes no temporary file beside it, so a key +on a read-only mount works. Flats does not check the mode or the owner of the +file. The user that runs the host must be able to read it (uid `65532` in the +container image). It must be a regular file of exactly 32 bytes, and the host +refuses to start otherwise. Flats never creates a key at a configured path. + +Make a key with 32 random bytes: + +```sh +(umask 077 && head -c 32 /dev/urandom > /etc/flats/secret.key) +``` + +```powershell +$k = New-Object byte[] 32 +[System.Security.Cryptography.RandomNumberGenerator]::Create().GetBytes($k) +[System.IO.File]::WriteAllBytes('C:\ProgramData\flats\secret.key', $k) +``` + +On Windows, give the file an access list for the service account only. Flats +does not set one. + +To move the key of an existing host out of the data directory, stop the host, +move the file, and set the path: + +```sh +mv DATA/secret.key /etc/flats/secret.key +flats config set host.secret_key_path /etc/flats/secret.key --config PATH +``` + +Keep the key in a folder that no link and no mount reaches from the data +directory. The checks below follow links, but they cannot find every name that +a file system gives one file, such as a Windows short name, a hard link or a +bind mount of the folder into the data directory. + +The start refuses, with status 78 and before any listener opens, in each of +these cases. The message names the path and the setting, and it holds when +`flats.db` has no sealed value too: + +- nothing is at the path, or a link there points to nothing; +- the path is not a regular file once links are followed, or it is not 32 + bytes, or the user cannot read it; +- a file, a directory or a link exists at `DATA/secret.key`: two keys would be in + use, and a copy of the data directory would hold the one that you wanted to + keep out. Move that file away; +- the key, after the system resolved every link, lies inside the data + directory. + +`flats key set`, `flats secret backup-key`, `restore-key` and `reset` refuse in +the last two cases too. `flats key ls` and `flats key rm` touch no key and still +work. `flats backup` refuses in them before it writes anything. + +`flats config set` and `unset` load the file first, so they cannot repair a +file that the check refuses, for example a key path that lies inside +`host.data_dir`. Edit `config.json` by hand: remove `host.secret_key_path` or +give it a path outside the data directory. For the same reason, +`flats config set host.data_dir` refuses a directory that contains the key path. + +To put the key back in the data directory, or to go back to an older Flats, +stop the host, copy the key to `DATA/secret.key`, and clear the setting: + +```sh +cp /etc/flats/secret.key DATA/secret.key +flats config unset host.secret_key_path --config PATH +``` + +Copy before you clear. `flats config unset` works while a file is at +`DATA/secret.key`, but a start between the two steps then refuses instead of +creating a new key. An older release refuses a `config.json` that holds +`host.secret_key_path`, as it refuses any key it does not know, so clear the +setting before you start it. + +The key commands and `flats backup` run as processes of their own. A key that +only the service process can see is missing for them. This holds for a systemd +credential, and for a Docker secret that is mounted into the service container +([Docker](container.md#keeping-the-key-outside-the-volume)). For such a key, +`flats backup` needs `--exclude-secret-key`, `flats secret restore-key` needs +`--out FILE` (it writes the recovered key to a new file of mode 0600 and changes +nothing at the path), and the other key commands cannot run unless you give them +the same mount. A plain file that the service user can read, for example +`/etc/flats/secret.key` with mode 0400, works for every command. A drop-in for a +system service that loads a systemd credential: + +```ini +# /etc/systemd/system/flats.service.d/key.conf +[Service] +LoadCredential=secret.key:/etc/flats/secret.key +``` + +```sh +flats config set host.secret_key_path /run/credentials/flats.service/secret.key --config PATH +``` + +`systemd.exec(5)` names the credentials directory of a service. The path in +`config.json` is fixed, so it cannot use `$CREDENTIALS_DIRECTORY`. + ### Recovering a lost secret.key `secret.key` opens every flat secret and stored operator key, and nothing else does. Three commands on the host keep a way back when the file is lost. They work on the data directory, never call the host, and no agent tool or API -route reaches them. +route reaches them. With `host.secret_key_path` they use the file that it +names, and the recovery copy stays in the data directory. ```sh flats secret backup-key [--out PATH] [--password-file PATH] [--config PATH | --data DIR] -flats secret restore-key [--from PATH] [--password-file PATH] [--config PATH | --data DIR] +flats secret restore-key [--from PATH] [--out FILE] [--password-file PATH] [--config PATH | --data DIR] flats secret reset [--yes] [--config PATH | --data DIR] ``` @@ -963,20 +1110,30 @@ flats secret reset [--yes] [--config PATH | --data DIR] instead, without its trailing line ends. The password has at least 12 characters and at most 1024 bytes. Run the command again to change it. Flats keeps no copy of the password. -- `restore-key` needs the host stopped and nothing at `secret.key`. It reads +- `restore-key` needs the host stopped and nothing at `secret.key`, or, with + `host.secret_key_path`, nothing at that path. It reads `secret.key.recovery`, or `--from PATH`, asks for the password, checks that the key opens every sealed value in `flats.db`, and only then writes - `secret.key`. It never replaces a key. When `flats.db` holds no sealed - value, there is nothing to check the key against: the command says so and - writes any key the password opens. It refuses a copy written at a work - factor above 2^18, the age default that `backup-key` and `age -p` use. + `secret.key`, or the file at `host.secret_key_path`. It never replaces a + key. When `flats.db` holds no sealed value, there is nothing to check the key + against: the command says so and writes any key the password opens. It + refuses a copy written at a work factor above 2^18, the age default that + `backup-key` and `age -p` use. With `host.secret_key_path` its synced + temporary file lies beside the configured path. When it cannot write there, + for example on a read-only mount, it writes nothing and names `--out`. + `--out FILE` writes the recovered key to a new file of mode 0600 instead, + after the same checks of the key. It needs no empty path, because the path may + hold a wrong key, and it changes nothing there. `FILE` must not lie in the data + directory. Move `FILE` into your secret store and remove it. - `reset` is for a host that has lost both the key and the password. With the host stopped, and nothing at `secret.key` or a key there that opens none of the values, it lists the flat secrets and operator keys it will delete, asks you to type `reset` (or takes `--yes`), deletes them and leaves an event on each flat. The next start creates a new key: run `backup-key` after it, set the secrets again with `flats secret set` and redeploy, and - register the operator keys again with `flats key set`. + register the operator keys again with `flats key set`. With + `host.secret_key_path` the next start creates no key: put a key at that path + before it, and run `backup-key` after it. A key at `secret.key` that opens any of the values makes `reset` refuse. Remove only the values it does not open instead: operator keys first, with `flats key rm` while the host is stopped, because the start refuses while @@ -995,7 +1152,8 @@ Run it in the data directory with the host stopped and nothing at `secret.key`, because `age` replaces an existing output file. Without the `umask`, `age` creates the file readable by other users; with it the file gets the mode 0600 that Flats uses. This way skips the check against -`flats.db` that `restore-key` makes. A copy made by hand with `age -p` has no +`flats.db` that `restore-key` makes. With `host.secret_key_path`, name that path +after `-o` instead of `secret.key`. A copy made by hand with `age -p` has no ID file, and the host warns about it until `backup-key` writes one. A copy of the data directory includes the recovery file, so anyone holding a diff --git a/docs/container.md b/docs/container.md index 8368e50e..e72f81a7 100644 --- a/docs/container.md +++ b/docs/container.md @@ -264,6 +264,57 @@ who can run `docker` on the host can read the data volume and reach the container, so treat Docker access as operator access and do not give it to agents. +## Keeping the key outside the volume + +By default `secret.key` lives in the `/data` volume, so a copy of the volume +holds every sealed value and the key that opens it. To keep the key apart, mount +it as a file and name it with `host.secret_key_path` +([Keeping the key outside the data directory](configuration.md#keeping-the-key-outside-the-data-directory)). +The host only reads that file and never creates it, so a read-only mount works. +The container user `65532` must be able to read it. A bind-mounted file keeps its +own owner and mode on the host, so give it to that user, for example +`sudo chown 65532 flats.key && chmod 0400 flats.key`. + +Stop the container, move the key out of the volume, and set the path with +one-off containers on the same volume: + +```sh +docker stop flats +docker run --rm -v flats-data:/data:ro alpine cat /data/secret.key > flats.key +docker run --rm -v flats-data:/data alpine rm /data/secret.key +docker run --rm -v flats-data:/data ghcr.io/gosuda/flats:latest config set host.secret_key_path /run/secrets/flats.key +docker run -d --name flats --restart unless-stopped \ + -v flats-data:/data -v "$PWD/flats.key:/run/secrets/flats.key:ro" \ + ghcr.io/gosuda/flats:latest +``` + +(Run `chown` and `chmod` on `flats.key` before the last command.) A new host needs +a key first, because Flats creates none at a configured path: make it with +`head -c 32 /dev/urandom > flats.key` and set the path before the first start. + +With Docker Compose, add the file as a secret to the `flats` service in +`compose.yaml` (or in an override file) and set the same path: + +```yaml +services: + flats: + secrets: + - flats.key +secrets: + flats.key: + file: ./flats.key +``` + +```sh +docker compose run --rm --no-deps flats config set host.secret_key_path /run/secrets/flats.key +``` + +A mounted key is visible only inside the service container. `docker exec flats +flats backup --out …` sees it, but a one-off container and a process on the host +do not. For those, `flats backup` needs `--exclude-secret-key`, and +`flats secret restore-key` needs `--out FILE`. Back up the key file apart from +the volume: a copy of the volume no longer holds it. + ## Upgrading and backups Upgrade by replacing the container and keeping the volume: @@ -287,7 +338,9 @@ docker start flats ``` The backup holds the operator keys and the `secret.key` that opens them; keep -it as private as the keys. +it as private as the keys. With `host.secret_key_path` the key is not in the +volume, and you keep that file apart from the backup +([Keeping the key outside the volume](#keeping-the-key-outside-the-volume)). ## Building the image diff --git a/docs/design.md b/docs/design.md index 7c055689..3e729a3a 100644 --- a/docs/design.md +++ b/docs/design.md @@ -52,7 +52,9 @@ The advisory lock uses the persistent file inode; do not remove or replace it. Successful shutdown and process exit release it, and failed startup cleans up before releasing it. `secret.key` is generated into a private synced temporary file and atomically linked into place without replacing a concurrent winner; -readers see either no key or all 32 bytes. +readers see either no key or all 32 bytes. With `host.secret_key_path` the host +generates nothing: it reads the key file that the setting names and refuses to +start when it cannot. Shutdown drains the management server (5 s; force-close on failure), stops core workers, closes public then private networks, closes SQLite and releases @@ -566,7 +568,10 @@ and separate browser-side CORS/CSP behavior. WASI has no outbound HTTP ABI. ## Secrets Values are sealed with AES-256-GCM (random nonce, AAD = secret name) under -`/secret.key`. Only the console and the local CLI (`flats secret set`, +`/secret.key`, or under the file that `host.secret_key_path` names, which +keeps the key out of the data directory and of a copy of it (the host only reads +that file; see [Keeping the key outside the data directory](configuration.md#keeping-the-key-outside-the-data-directory)). +Only the console and the local CLI (`flats secret set`, loopback only) can set or delete values; APIs and MCP return names and update times only. The CLI is recognized by its `X-Flats-Client: cli` header on the loopback listener, so this stops MCP and remote API clients, not a process diff --git a/docs/spec/README.md b/docs/spec/README.md index e7449221..3bcfcd31 100644 --- a/docs/spec/README.md +++ b/docs/spec/README.md @@ -48,7 +48,7 @@ list. | [Autopilot](autopilot.md) | implemented | An opt-in mode in the new Settings › Experiments tab where agents approve or reject pending approvals for the operations the operator allows, with an operation guide for agents | | [Windows support](windows-support.md) | partly implemented | Flats on Windows: a PowerShell installer, a per-user service, Windows file names and locks, and a release archive | | [Secret key recovery](secret-key-recovery.md) | implemented | `flats secret backup-key`, `restore-key` and `reset`: a password-encrypted copy of `secret.key`, its restore against the sealed values, and a reset when both are lost | -| [Secret key path](secret-key-path.md) | planned | `host.secret_key_path` keeps the host key outside the data directory, for example in a Docker secret or a systemd credential: the host reads it and never creates it, and the key commands and `flats backup` use it | +| [Secret key path](secret-key-path.md) | implemented | `host.secret_key_path` keeps the host key outside the data directory, for example in a Docker secret or a systemd credential: the host reads it and never creates it, and the key commands and `flats backup` use it | | [Public instance mode](public-instance.md) | implemented | An opt-in Experiments mode in which every new flat is created Public on the default public network and no private address is handed out or served | | [Host backup](host-backup.md) | implemented | `flats backup --out` copies a running or stopped host into a directory with a manifest, and `flats backup --verify` checks such a copy: databases as snapshots, write-once blobs in two passes, coherence of the outbox databases of documents, boards and notebooks with `flats.db`, and a self-check that fails to be run again | diff --git a/docs/spec/config-reload.md b/docs/spec/config-reload.md index 3152e239..4147dd8a 100644 --- a/docs/spec/config-reload.md +++ b/docs/spec/config-reload.md @@ -144,8 +144,9 @@ table; a key's bucket follows the table's Applies column. 8. **Restart-required keys.** These keys enter the adopted document and apply at the next start, exactly as they do today: `host.console_addr`, - `host.local_addr`, `host.server_runtime`, `metrics.enabled`, - `metrics.expose_on`, `metrics.per_flat`, `network.private_backend`, + `host.local_addr`, `host.server_runtime`, `host.secret_key_path`, + `metrics.enabled`, `metrics.expose_on`, `metrics.per_flat`, + `network.private_backend`, `portal.relays`, `portal.discovery`, `portal.max_active_relays`, `zrok.environment`, `zrok.namespace`, `netbird.management_url`, `proxy.addr`, and the deprecated `credentials.tailscale_authkey_file`. diff --git a/docs/spec/host-backup.md b/docs/spec/host-backup.md index aff88a87..9b6ead34 100644 --- a/docs/spec/host-backup.md +++ b/docs/spec/host-backup.md @@ -56,10 +56,10 @@ Out of scope: - Scheduling, upload to a remote store, compression, encryption of the copy and incremental copies. - A pause of the running host. The requester chose a copy that checks itself. -- The key path setting and key rotation (#224 items 1 and 2). The command reads - `secret.key` at its fixed path. When those items ship, the command must read - the key through the function that they add, and a rotation during a copy must - fail the copy (rule 22). +- Key rotation (#224 item 2). A rotation during a copy must fail the copy (rule + 22). The key path setting (#224 item 1) is implemented, and the command reads + the key through the location that it adds. [Secret key path](secret-key-path.md) + says how rules 12, 22, 24, 28, 30, 31 and 35 apply to it. ## The copy @@ -167,7 +167,11 @@ these steps and no other: database of a host of this release, which means a file that is no Flats database or a `user_version` other than the one this binary expects (rule 25). A host of an older release migrates its `flats.db` when it starts, and the - refusal says so and names the copy by hand. This check comes last, because + refusal says so and names the copy by hand. With `host.secret_key_path`, it + also refuses, before it opens any database, when the checks of rule 3 of + [secret key path](secret-key-path.md) fail or the key cannot be read, and the + message names `--exclude-secret-key`. That flag skips the read. The database + check comes last, because it is the only refusal that opens a database, and opening the database of a stopped host that left a `-wal` file and no `-shm` creates an empty `-shm` (rule 11). It creates a new @@ -178,7 +182,8 @@ these steps and no other: Each has the file identity that the system gives (the inode on Unix, the file index on Windows) and its creation time where the system has one. It lists the entry names of `flats/.lifecycle/` and each `restore-journal.json` with - its size and modification time. It reads `secret.key`. A journal that exists + its size and modification time. It reads `secret.key`, or the key at the + configured path. A journal that exists now is data and is copied. It is not a failure. 14. **Write-once files, first pass.** It copies the write-once class. A snapshot is the file `-.sqlite` and, in the current format, the directory @@ -271,7 +276,10 @@ these steps and no other: directory. 22. `secret.key` is read at step 13 and again at step 18. When the two reads differ, the command fails the backup, so a key change during the copy, such as - a rotation, never pairs one key with the other key's `flats.db`. + a rotation, never pairs one key with the other key's `flats.db`. With + `host.secret_key_path`, both reads use the configured file, the second one + without the refusal of a file at `/secret.key`, and a file that is + gone at the second read fails the backup the same way. 23. On any failure, and on an interrupt (Ctrl-C or SIGTERM) before step 21, the command removes the partial directory and exits with a failure. `PATH` never holds an incomplete copy. After a crash a `PATH.partial-` can stay. It @@ -288,7 +296,10 @@ flats backup --out PATH [--exclude-secret-key] [--config PATH | --data DIR] [--j With `--exclude-secret-key`, `data/secret.key` is not copied and the manifest records `secret_key: "excluded"`. The recovery copy `secret.key.recovery` and its `.id` file stay, so a copy without the key can still be restored with - `flats secret restore-key`. A holder of the copy can guess the recovery + `flats secret restore-key`. With `host.secret_key_path`, the walk leaves out + `/secret.key` as well and lists it in `excluded`, the manifest records + `secret_key: "external"`, and the flag means that the command does not read + the key ([secret key path](secret-key-path.md), rules 16 to 18). A holder of the copy can guess the recovery password offline, as [secret key recovery](secret-key-recovery.md#the-recovery-copy) says. The output names the folder, the number of files and bytes, whether a secret key @@ -333,7 +344,10 @@ flats backup --verify PATH [--json] every flat secret and every operator key record with a stored value opens with that key. The code is the one of `flats secret restore-key` (rule 22 of its spec), reading the immutable connection of rule 25. When the copy has no - `secret.key`, the command says so and skips this check. + `secret.key`, the command says so and skips this check. With + `secret_key: "external"`, `flats backup --out` checks the copy against the key + that it read, and the standalone `--verify` has no key and reports a note + ([secret key path](secret-key-path.md), rules 17 and 19). 29. The output lists each failure with its path and reason, and ends with the count. It prints `ok` and the Flats version that made the copy when nothing failed. @@ -344,13 +358,17 @@ flats backup --verify PATH [--json] status 2. A refusal exits with status 78, like `flats key`: an existing `PATH`, a missing or unusable folder, a `PATH` inside the data directory, a data directory with no `flats.db`, a `flats.db` that is no Flats database, and - a `user_version` that rule 25 refuses. + a `user_version` that rule 25 refuses, and, with `host.secret_key_path`, a + key that the checks of rule 3 of [secret key path](secret-key-path.md) refuse + or that cannot be read. Any other failure exits with status 1: an unstable host file after 3 copies, a database that stays incoherent after 3 attempts, a change of the layout during the copy (rule 19), a changed key (rule 22), a failed verification, and an - input or output error such as a full disk. A failure that a second run can - clear says so: "run the backup again". -31. A refusal writes nothing at `PATH`, and the only file that it can create + input or output error such as a full disk, and, with `host.secret_key_path`, + a key that does not open the sealed values of the copy and a copy that holds + the bytes of the key. The last two say that a second run does not clear them. + A failure that a second run can clear says so: "run the backup again". +31. A refusal writes nothing at `PATH`, including a refusal of the key, and the only file that it can create anywhere is the empty `-shm` that the schema refusal of rule 12 can leave beside the database of a stopped host. A failure leaves nothing at `PATH`. @@ -381,7 +399,9 @@ flats backup --verify PATH [--json] [configuration](../configuration.md#what-to-back-up) says. A copy made with `--exclude-secret-key` needs `secret.key` put back from where the operator kept it, or `flats secret restore-key` with the recovery password, before the - start. + start. A copy with `secret_key: "external"` brings back `config.json` with + `host.secret_key_path`, and the operator puts the key at that path before + the start. ## Compatibility diff --git a/docs/spec/secret-key-path.md b/docs/spec/secret-key-path.md index 315d2bc1..20e3df91 100644 --- a/docs/spec/secret-key-path.md +++ b/docs/spec/secret-key-path.md @@ -1,6 +1,6 @@ # Secret key path -Status: planned. +Status: implemented. ## Summary @@ -15,7 +15,7 @@ The request is item 1 of issue #224. Items 2 to 4 (rotation, injected secrets, audit events) are separate changes, and item 5 (the recovery password) is done ([secret key recovery](secret-key-recovery.md)). -## Current state +## Before this change - The host reads `/secret.key` at start (`New` in `internal/core/service.go`). When the file is missing and `flats.db` holds no @@ -79,7 +79,7 @@ Out of scope: `host.data_dir` or to leave the setting empty, and to edit `config.json` by hand. 3. The location of the key is one function of the data directory and the - setting. Every place that "Current state" names gets the path from it. It + setting. Every place that "Before this change" names gets the path from it. It returns the path and whether the setting chose it. With the setting, it refuses in two more cases that the check of rule 2 cannot see. - A file, a directory or a link exists at `/secret.key`. Two keys would @@ -240,9 +240,10 @@ call it, so they still work when the location is in one of those states. existing key out of the data directory and back, what a refusal means, and the hand edit that rule 2 asks for), and the effect on a downgrade (rule 24). `docs/container.md` shows a Docker secret and a Compose secret with - `config set host.secret_key_path`. It also shows a systemd drop-in with - `LoadCredential=` and the limits of rule 21. `docs/design.md` ("Secrets") says - where the key may be. The spec index lists this page. The out-of-scope notes + `config set host.secret_key_path`. `docs/configuration.md` also shows a systemd + drop-in with `LoadCredential=` and the limits of rule 21, because the systemd + service is not a container. `docs/design.md` ("Secrets") says where the key may + be. The spec index lists this page. The out-of-scope notes and rules 12, 22, 24, 28, 30, 31 and 35 of [host backup](host-backup.md), rule 8 of [config reload](config-reload.md), and rules 19, 23 and 31 of [secret key recovery](secret-key-recovery.md) change as this spec says. Rule diff --git a/docs/spec/secret-key-recovery.md b/docs/spec/secret-key-recovery.md index 7ff74a5d..56500af7 100644 --- a/docs/spec/secret-key-recovery.md +++ b/docs/spec/secret-key-recovery.md @@ -45,9 +45,11 @@ warning, the start-time refusal message, and the docs. Out of scope: -- The key path setting and key rotation (#224 items 1 and 2). The commands use - the fixed path `/secret.key`. When rotation ships, it must keep the - recovery copy and its key ID current. +- Key rotation (#224 item 2). When rotation ships, it must keep the recovery + copy and its key ID current. The key path setting (#224 item 1) is + implemented: the commands find the key through the location that + [secret key path](secret-key-path.md) adds, and rules 19, 23 and 31 below apply + to that location. The recovery copy and its key ID stay in the data directory. - Audit records (#224 item 4). `reset` leaves an event on each flat whose secrets it deletes, as `flats secret rm` does today. - The console and MCP. No agent tool or API route reaches these commands. @@ -163,7 +165,10 @@ flats secret restore-key [--from PATH] [--password-file PATH] [--config PATH | - runs, and checks that `flats.db` belongs to the configuration. 19. It refuses when anything is at `/secret.key`: a file, a directory or a link, also a link that points nowhere. It never replaces a key. A wrong - key must be moved away first. + key must be moved away first. With `host.secret_key_path`, the same holds for + the configured path, and a file at `/secret.key` is a refusal too. With + `--out FILE`, the command does not look at the configured path + ([secret key path](secret-key-path.md), rules 12 and 13). 20. It reads `--from PATH`, by default `/secret.key.recovery`. A file larger than 64 KiB is refused by its size before any password is asked. The command then asks for the password once; rule 14 does not apply. @@ -180,7 +185,8 @@ flats secret restore-key [--from PATH] [--password-file PATH] [--config PATH | - nothing to check against: the command says so and continues, so in that case it installs any key the password opens. 23. It writes `secret.key` through a synced temporary file `.secret-key-*` - with mode 0600 in the data directory, published under the final name with + with mode 0600 in the folder of the key (the data directory, or the folder of + `host.secret_key_path`), published under the final name with a link that fails when the name exists, as the host does at its first start, and then syncs the directory. When something created `secret.key` after the check in rule 19, the command refuses and removes its temporary @@ -227,7 +233,9 @@ flats secret reset [--yes] [--config PATH | --data DIR] the transaction durable. Operator key records without a stored value stay, such as the record of a connected zrok account. 31. The command does not touch `secret.key`, the recovery copy or its key ID. - When nothing is at `secret.key`, the next start creates a new key, and the + With `host.secret_key_path` the next start creates no key, and the output + says that it needs a key at the path. When nothing is at `secret.key` and the + setting is empty, the next start creates a new key, and the host then warns about an existing recovery copy (rule 5). The output says to run `backup-key` after that start, and to set the secrets and operator keys again with `flats secret set`, followed by a redeploy, and diff --git a/docs/surfaces.md b/docs/surfaces.md index 6f462332..714b370a 100644 --- a/docs/surfaces.md +++ b/docs/surfaces.md @@ -329,8 +329,8 @@ for server flats); `flats config`, `flats key`, and | `preview [--version n]` | Open a Private preview; 0 (default) is the current Draft | | `visibility private\|public [--access view\|edit] [--reason r]` | Request a visibility change; `--access edit` lets anyone edit a Public board | | `rename `, `delete [--reason r]` | Rename; request deletion | -| `env`, `network`, `secret` | Manage server settings; changing `network` origins or `secret` values works only from the host itself (loopback). `secret backup-key`, `restore-key` and `reset` work on the data directory and never call the host ([Recovering a lost secret.key](configuration.md#recovering-a-lost-secretkey)) | -| `backup --out path [--exclude-secret-key] [--config path\|--data dir]`, `backup --verify path` | Copy the data directory of a running or stopped host into a new folder with a manifest, or check such a copy. Works on the data directory and never calls the host ([What to back up](configuration.md#what-to-back-up)) | +| `env`, `network`, `secret` | Manage server settings; changing `network` origins or `secret` values works only from the host itself (loopback). `secret backup-key`, `restore-key [--out file]` and `reset` work on the data directory and never call the host. They find the key at `host.secret_key_path` when it is set, and `restore-key --out` writes the recovered key to a new file instead ([Recovering a lost secret.key](configuration.md#recovering-a-lost-secretkey), [Keeping the key outside the data directory](configuration.md#keeping-the-key-outside-the-data-directory)) | +| `backup --out path [--exclude-secret-key] [--config path\|--data dir]`, `backup --verify path` | Copy the data directory of a running or stopped host into a new folder with a manifest, or check such a copy. With `host.secret_key_path` the key is outside the copy. Works on the data directory and never calls the host ([What to back up](configuration.md#what-to-back-up)) | | `approvals [--status s]` | List approvals (read-only) | | `status`, `install`, `uninstall`, `connect`, `mcp`, `mcp-config` | Host and client setup ([Connecting agents](agents.md)). `connect` chooses the host the CLI and `mcp` use; `mcp-config` prints the agent setup commands | | `agent-setup docs [--agent a] [--scope user\|project] [--dry-run] [--yes] [--undo]` | Make Flats the document tool of Claude Code, Codex or Cursor. It prints a plan and changes files on this machine only after you confirm; `--undo` reverses it ([Use Flats for documents](agents.md#use-flats-for-documents)) | From eb39336983b2f37406a34e24f75a299548da2628 Mon Sep 17 00:00:00 2001 From: Hee Sung Son Date: Sat, 14 Nov 2026 18:43:24 +0900 Subject: [PATCH 08/14] refactor(secrets): share one path helper, merge the backup key reads Moves the inside-a-directory check to config.InsideDir for core and app, folds readKeyAgain into readKey, and tightens one container doc sentence. --- docs/container.md | 8 +++---- internal/app/secretkeycmd.go | 3 ++- internal/app/setup.go | 5 +++-- internal/backup/backup.go | 30 +++++++++------------------ internal/config/config.go | 2 +- internal/config/keypath.go | 4 ++-- internal/config/secretkeypath_test.go | 8 +++---- internal/core/keyfile.go | 19 ++--------------- internal/core/keyfile_test.go | 18 ---------------- 9 files changed, 28 insertions(+), 69 deletions(-) diff --git a/docs/container.md b/docs/container.md index e72f81a7..25fa1d28 100644 --- a/docs/container.md +++ b/docs/container.md @@ -309,11 +309,11 @@ secrets: docker compose run --rm --no-deps flats config set host.secret_key_path /run/secrets/flats.key ``` -A mounted key is visible only inside the service container. `docker exec flats -flats backup --out …` sees it, but a one-off container and a process on the host -do not. For those, `flats backup` needs `--exclude-secret-key`, and +A mounted key is visible only inside the service container. A command that runs +with `docker exec` sees it, but a one-off container and a process on the host do +not. For those, `flats backup` needs `--exclude-secret-key`, and `flats secret restore-key` needs `--out FILE`. Back up the key file apart from -the volume: a copy of the volume no longer holds it. +the volume, because a copy of the volume no longer holds it. ## Upgrading and backups diff --git a/internal/app/secretkeycmd.go b/internal/app/secretkeycmd.go index 305fbeba..d18229f9 100644 --- a/internal/app/secretkeycmd.go +++ b/internal/app/secretkeycmd.go @@ -14,6 +14,7 @@ import ( "golang.org/x/term" + "github.com/gosuda/flats/internal/config" "github.com/gosuda/flats/internal/core" "github.com/gosuda/flats/internal/prompt" "github.com/gosuda/flats/internal/store" @@ -250,7 +251,7 @@ func keyOutFile(flag, dataDir string) (string, error) { } dir, derr := filepath.EvalSymlinks(filepath.Dir(path)) data, serr := filepath.EvalSymlinks(dataDir) - if derr == nil && serr == nil && core.PathInside(data, dir) { + if derr == nil && serr == nil && config.InsideDir(data, dir) { return "", actionf("--out: name a file outside the data directory %s, because a copy of the data directory would hold the key", dataDir) } return path, nil diff --git a/internal/app/setup.go b/internal/app/setup.go index 0d6bb47f..4d67aded 100644 --- a/internal/app/setup.go +++ b/internal/app/setup.go @@ -947,9 +947,10 @@ func importLegacyKey(ctx context.Context, dataDir string, st *store.Store, plan if err := core.CheckHostKeyValue(core.HostKeyTailscale, v); err != nil { return actionf("%s: %v", from, err) } - stored, _, err := core.StoredHostKey(ctx, core.NewKeyFile(dataDir, ""), st, core.HostKeyTailscale) + keyFile := core.NewKeyFile(dataDir, "") // a legacy host has no configuration, so no configured key + stored, _, err := core.StoredHostKey(ctx, keyFile, st, core.HostKeyTailscale) if err != nil || stored != "" { return err } - return core.PutHostKeyValue(ctx, core.NewKeyFile(dataDir, ""), st, core.HostKeyTailscale, v, core.KeySourceFlag, time.Now()) + return core.PutHostKeyValue(ctx, keyFile, st, core.HostKeyTailscale, v, core.KeySourceFlag, time.Now()) } diff --git a/internal/backup/backup.go b/internal/backup/backup.go index da226098..648ac731 100644 --- a/internal/backup/backup.go +++ b/internal/backup/backup.go @@ -174,7 +174,7 @@ func Run(ctx context.Context, o Options) (res *Result, err error) { if err != nil { return nil, err } - key0, err := readKey(keyFile, o.ExcludeSecretKey) + key0, err := readKey(keyFile, o.ExcludeSecretKey, false) if err != nil { return nil, err } @@ -224,7 +224,7 @@ func Run(ctx context.Context, o Options) (res *Result, err error) { if d := s1.diff(s2); d != "" { return nil, &RerunError{fmt.Errorf("a flat was created, renamed, deleted or restored during the copy (%s)", d)} } - key1, err := readKeyAgain(keyFile, o.ExcludeSecretKey) + key1, err := readKey(keyFile, o.ExcludeSecretKey, true) if err != nil { if keyFile.Configured() { return nil, &RerunError{fmt.Errorf("the secret key at %s cannot be read any more: %w", keyFile.Path(), err)} @@ -320,26 +320,16 @@ func checkSchema(flatsDB string) error { } // readKey returns the bytes of the host key, or nil when the key is excluded or the default file is not -// there. A configured key must be readable, and it is not read at all when it is excluded. -func readKey(k core.KeyFile, excluded bool) ([]byte, error) { - if excluded { - return nil, nil - } - if k.Configured() { - return k.Read() - } - return readKeyBytes(k.DataDir(), false) -} - -// readKeyAgain is readKey for the second read of rule 22, which does not repeat the checks of rule 3. -func readKeyAgain(k core.KeyFile, excluded bool) ([]byte, error) { - if excluded { - return nil, nil - } - if k.Configured() { +// there. A configured key must be readable, and it is not read at all when it is excluded. The second +// read of rule 22 (again) does not repeat the checks of rule 3. +func readKey(k core.KeyFile, excluded, again bool) ([]byte, error) { + switch { + case excluded || !k.Configured(): + return readKeyBytes(k.DataDir(), excluded) + case again: return k.ReadAgain() } - return readKeyBytes(k.DataDir(), false) + return k.Read() } // readKeyBytes returns the bytes of secret.key, following a link as the host diff --git a/internal/config/config.go b/internal/config/config.go index c4f70c1f..8bd396f1 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -1054,7 +1054,7 @@ func (c *Config) fill() error { errs = append(errs, fieldErr("network.default_public", fmt.Sprintf(`add %q to network.permitted or clear the default`, p), "%s is not permitted", p)) } - if p := c.Host.SecretKeyPath; p != "" && insideDir(c.Host.DataDir, p) { + if p := c.Host.SecretKeyPath; p != "" && InsideDir(c.Host.DataDir, p) { errs = append(errs, fieldErr("host.secret_key_path", "use a path outside host.data_dir, or leave it empty to keep the key in the data directory; if this file is invalid, edit config.json by hand", "%s lies inside host.data_dir %s", p, c.Host.DataDir)) diff --git a/internal/config/keypath.go b/internal/config/keypath.go index cccb7d6e..3537b4d6 100644 --- a/internal/config/keypath.go +++ b/internal/config/keypath.go @@ -14,9 +14,9 @@ func cleanKeyPath(p string) string { return filepath.Clean(p) } -// insideDir reports whether child is dir or lies below it. It compares the two +// InsideDir reports whether child is dir or lies below it. It compares the two // cleaned paths and does not resolve links. On Windows it ignores case. -func insideDir(dir, child string) bool { +func InsideDir(dir, child string) bool { dir, child = filepath.Clean(dir), filepath.Clean(child) if runtime.GOOS == "windows" { dir, child = strings.ToLower(dir), strings.ToLower(child) diff --git a/internal/config/secretkeypath_test.go b/internal/config/secretkeypath_test.go index 3903b445..9ae7b661 100644 --- a/internal/config/secretkeypath_test.go +++ b/internal/config/secretkeypath_test.go @@ -168,15 +168,15 @@ func TestInsideDir(t *testing.T) { {"/a/b", "/x/a/b", false}, } for _, tc := range cases { - if got := insideDir(filepath.FromSlash(tc.dir), filepath.FromSlash(tc.child)); got != tc.want { - t.Errorf("insideDir(%q, %q) = %v, want %v", tc.dir, tc.child, got, tc.want) + if got := InsideDir(filepath.FromSlash(tc.dir), filepath.FromSlash(tc.child)); got != tc.want { + t.Errorf("InsideDir(%q, %q) = %v, want %v", tc.dir, tc.child, got, tc.want) } } if runtime.GOOS == "windows" { - if !insideDir(`C:\Data\Flats`, `c:\data\flats\SECRET.KEY`) { + if !InsideDir(`C:\Data\Flats`, `c:\data\flats\SECRET.KEY`) { t.Error("Windows paths that differ in case only are different") } - } else if insideDir("/Data/Flats", "/data/flats/secret.key") { + } else if InsideDir("/Data/Flats", "/data/flats/secret.key") { t.Error("paths that differ in case are the same") } } diff --git a/internal/core/keyfile.go b/internal/core/keyfile.go index 667e2a28..a651c9f5 100644 --- a/internal/core/keyfile.go +++ b/internal/core/keyfile.go @@ -6,9 +6,8 @@ import ( "io/fs" "os" "path/filepath" - "runtime" - "strings" + "github.com/gosuda/flats/internal/config" "github.com/gosuda/flats/internal/durable" ) @@ -71,7 +70,7 @@ func (k KeyFile) Check() error { if err != nil { return nil } - if PathInside(dataReal, real) { + if config.InsideDir(dataReal, real) { return fmt.Errorf("%w: %s names %s, which resolves to %s inside the data directory %s; keep the key outside the data directory", ErrKeyLocation, KeySetting, k.path, real, dataReal) } @@ -139,17 +138,3 @@ func (k KeyFile) Restore(key []byte) error { } return durable.SyncDir(filepath.Dir(k.path)) } - -// PathInside reports whether p is dir or lies below it, ignoring case on Windows. It compares paths -// that the caller already resolved. -func PathInside(dir, p string) bool { - dir, p = filepath.Clean(dir), filepath.Clean(p) - if runtime.GOOS == "windows" { - dir, p = strings.ToLower(dir), strings.ToLower(p) - } - rel, err := filepath.Rel(dir, p) - if err != nil { - return false - } - return rel == "." || (rel != ".." && !strings.HasPrefix(rel, ".."+string(filepath.Separator))) -} diff --git a/internal/core/keyfile_test.go b/internal/core/keyfile_test.go index 8f64d3cb..44e5be5d 100644 --- a/internal/core/keyfile_test.go +++ b/internal/core/keyfile_test.go @@ -352,24 +352,6 @@ func TestRestoreOfAConfiguredKeyInAReadOnlyFolder(t *testing.T) { } } -func TestPathInside(t *testing.T) { - t.Parallel() - for _, tc := range []struct { - dir, p string - want bool - }{ - {"/a/b", "/a/b", true}, - {"/a/b", "/a/b/c", true}, - {"/a/b", "/a/bc", false}, - {"/a/b", "/a", false}, - {"/a/b", "/x/a/b", false}, - } { - if got := PathInside(filepath.FromSlash(tc.dir), filepath.FromSlash(tc.p)); got != tc.want { - t.Errorf("PathInside(%q, %q) = %v, want %v", tc.dir, tc.p, got, tc.want) - } - } -} - // ReadAgain does not repeat the checks of Check: a second key that appears in the data directory during a backup does // not fail the re-read, and a missing file still does. func TestReadAgainSkipsTheChecksOfCheck(t *testing.T) { From 8765cb4c0f8313cbc76b87763bc8fecfbe22402e Mon Sep 17 00:00:00 2001 From: Hee Sung Son Date: Sat, 14 Nov 2026 19:45:18 +0900 Subject: [PATCH 09/14] fix(secrets): answer the implementation review of host.secret_key_path Exits 78 when core.New refuses the key, reads the key in a backup without the checks of the preflight, reports a failure that a second run cannot clear ahead of one that it can, gives backup-key and reset the move-away message for an unusable configured key, and names the same-file case. Fixes the systemd, Docker and Compose docs and adds the tests that the completion criteria asked for. --- README.md | 3 +- docs/configuration.md | 15 +++- docs/container.md | 31 +++++--- docs/spec/host-backup.md | 5 +- docs/spec/secret-key-path.md | 6 +- internal/app/secretkeycmd.go | 38 ++++++---- internal/app/secretkeypath_cmd_test.go | 58 ++++++++++++++ internal/app/secretkeypath_test.go | 101 +++++++++++++++++++++++++ internal/app/serve.go | 19 +++-- internal/backup/backup.go | 42 +++++----- internal/backup/externalkey_test.go | 71 +++++++++++------ internal/core/hostkeys.go | 2 +- internal/core/keyfile.go | 38 ++++++++-- internal/core/keyfile_test.go | 37 ++++----- internal/core/keyrecovery.go | 2 +- 15 files changed, 361 insertions(+), 107 deletions(-) diff --git a/README.md b/README.md index e828a111..91387a1f 100644 --- a/README.md +++ b/README.md @@ -369,7 +369,8 @@ and upgrades from older hosts. - **Local secrets.** Secrets are encrypted with the local `secret.key`; secret APIs expose names only. A flat can read and return its own secrets, so deploy code you trust with them. Anyone who can read the data directory - can recover those secrets. + can recover those secrets, unless `host.secret_key_path` keeps the key + elsewhere. - **Back up your data.** Immutable code versions are not data backups. Back up the data directory, including `secret.key`; see [what to back up](docs/configuration.md#what-to-back-up). diff --git a/docs/configuration.md b/docs/configuration.md index 2c14862e..fe91d8f7 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -1071,8 +1071,12 @@ credential, and for a Docker secret that is mounted into the service container `--out FILE` (it writes the recovered key to a new file of mode 0600 and changes nothing at the path), and the other key commands cannot run unless you give them the same mount. A plain file that the service user can read, for example -`/etc/flats/secret.key` with mode 0400, works for every command. A drop-in for a -system service that loads a systemd credential: +`/etc/flats/secret.key` with mode 0400, works for every command. + +A systemd credential suits a system service that you wrote and that runs as its +own user. `flats install` makes a user unit, `flats.service`, that runs as your +own user, and a credential adds no protection there over a file with mode 0400, +so use the plain file for it. A drop-in for a system service: ```ini # /etc/systemd/system/flats.service.d/key.conf @@ -1084,8 +1088,11 @@ LoadCredential=secret.key:/etc/flats/secret.key flats config set host.secret_key_path /run/credentials/flats.service/secret.key --config PATH ``` -`systemd.exec(5)` names the credentials directory of a service. The path in -`config.json` is fixed, so it cannot use `$CREDENTIALS_DIRECTORY`. +A user unit has its drop-in in `~/.config/systemd/user/flats.service.d/key.conf`, +and its credentials directory lies under `$XDG_RUNTIME_DIR/credentials/flats.service/`, +for example `/run/user/1000/credentials/flats.service/secret.key` for uid 1000. +`systemd.exec(5)` names both directories. The path in `config.json` is fixed, so +it cannot use `$CREDENTIALS_DIRECTORY`. ### Recovering a lost secret.key diff --git a/docs/container.md b/docs/container.md index 25fa1d28..66af7d99 100644 --- a/docs/container.md +++ b/docs/container.md @@ -276,24 +276,30 @@ own owner and mode on the host, so give it to that user, for example `sudo chown 65532 flats.key && chmod 0400 flats.key`. Stop the container, move the key out of the volume, and set the path with -one-off containers on the same volume: +one-off containers on the same volume. The last command is the one that you +used to create the container, with the mount of the key added: ```sh docker stop flats -docker run --rm -v flats-data:/data:ro alpine cat /data/secret.key > flats.key +(umask 077; docker run --rm -v flats-data:/data:ro alpine cat /data/secret.key > flats.key) docker run --rm -v flats-data:/data alpine rm /data/secret.key docker run --rm -v flats-data:/data ghcr.io/gosuda/flats:latest config set host.secret_key_path /run/secrets/flats.key +sudo chown 65532 flats.key && chmod 0400 flats.key +docker rm flats docker run -d --name flats --restart unless-stopped \ -v flats-data:/data -v "$PWD/flats.key:/run/secrets/flats.key:ro" \ ghcr.io/gosuda/flats:latest ``` -(Run `chown` and `chmod` on `flats.key` before the last command.) A new host needs -a key first, because Flats creates none at a configured path: make it with -`head -c 32 /dev/urandom > flats.key` and set the path before the first start. +Add the network and port options of your first command to the last one. A new +host needs a key first, because Flats creates none at a configured path: make it +with `head -c 32 /dev/urandom > flats.key` and set the path before the first +start. With Docker Compose, add the file as a secret to the `flats` service in -`compose.yaml` (or in an override file) and set the same path: +`compose.yaml` (or in an override file). Then stop the service, move the key out +of the Compose volume with the container of the service, set the path, and start +the service again: ```yaml services: @@ -306,12 +312,19 @@ secrets: ``` ```sh +docker compose stop flats +container=$(docker compose ps -a -q flats) +(umask 077; docker run --rm --volumes-from "$container":ro alpine:3.22 cat /data/secret.key > flats.key) +docker run --rm --volumes-from "$container" alpine:3.22 rm /data/secret.key +sudo chown 65532 flats.key && chmod 0400 flats.key docker compose run --rm --no-deps flats config set host.secret_key_path /run/secrets/flats.key +docker compose up -d --wait ``` -A mounted key is visible only inside the service container. A command that runs -with `docker exec` sees it, but a one-off container and a process on the host do -not. For those, `flats backup` needs `--exclude-secret-key`, and +A mounted key is visible only inside the container that mounts it. A command that +runs with `docker exec` or `docker compose run` sees it, because they use the +service's mounts. A one-off container from `docker run` and a process on the host +do not. For those, `flats backup` needs `--exclude-secret-key`, and `flats secret restore-key` needs `--out FILE`. Back up the key file apart from the volume, because a copy of the volume no longer holds it. diff --git a/docs/spec/host-backup.md b/docs/spec/host-backup.md index 9b6ead34..a1f39ee7 100644 --- a/docs/spec/host-backup.md +++ b/docs/spec/host-backup.md @@ -169,8 +169,9 @@ these steps and no other: A host of an older release migrates its `flats.db` when it starts, and the refusal says so and names the copy by hand. With `host.secret_key_path`, it also refuses, before it opens any database, when the checks of rule 3 of - [secret key path](secret-key-path.md) fail or the key cannot be read, and the - message names `--exclude-secret-key`. That flag skips the read. The database + [secret key path](secret-key-path.md) fail or the key cannot be read. Only the + refusal for a key that cannot be read names `--exclude-secret-key`, which skips + the read and not the checks. The database check comes last, because it is the only refusal that opens a database, and opening the database of a stopped host that left a `-wal` file and no `-shm` creates an empty `-shm` diff --git a/docs/spec/secret-key-path.md b/docs/spec/secret-key-path.md index 20e3df91..1249ee0b 100644 --- a/docs/spec/secret-key-path.md +++ b/docs/spec/secret-key-path.md @@ -225,8 +225,10 @@ call it, so they still work when the location is in one of those states. that only the service process can see, such as a systemd `LoadCredential=` file under `$CREDENTIALS_DIRECTORY`, or a Docker secret that is mounted into the service container, is missing for a process outside the service. A - command that runs inside the container with `docker exec` sees the secret. A - one-off container and a process on the host do not. For such a key, `flats backup` + command that runs inside the container with `docker exec`, and a one-off + container from `docker compose run`, which mounts the secrets of its service, + sees the secret. A container from plain `docker run` and a process on the + host do not. For such a key, `flats backup` needs `--exclude-secret-key`, `restore-key` needs `--out`, and the other key commands cannot run unless the operator gives them the same mount. The docs say this, and they show a deployment that works for every command, a plain diff --git a/internal/app/secretkeycmd.go b/internal/app/secretkeycmd.go index d18229f9..ddf91e6b 100644 --- a/internal/app/secretkeycmd.go +++ b/internal/app/secretkeycmd.go @@ -70,14 +70,7 @@ func (k *secretKeyCmd) backup(args []string) error { if _, err := os.Lstat(keyPath); errors.Is(err, fs.ErrNotExist) && !keyFile.Configured() { return actionf("%s is missing: start the host once to create it, or restore it with `flats secret restore-key`", keyPath) } - // With a configured path, Read makes the checks of rule 3 and names the path and the setting in each refusal. - key, err := keyFile.Read() - if keyFile.Configured() && err != nil { - return &ActionError{err} - } - if keyUnusable(err, keyPath) { - return actionf("%v: move %s away first, because the next start would stop on it", err, keyPath) - } + key, err := readHostKey(keyFile) if err != nil { return err } @@ -143,7 +136,26 @@ func outFile(flag, dataDir string) (string, error) { return path, nil } -// keyUnusable reports whether err from core.ReadSecretKey means that what is +// readHostKey reads the host key for backup-key and reset. With a configured path it first makes the checks +// of rule 3 of the spec. A key that is no key (the wrong size, a directory, a link to nothing) is a refusal +// that names the path and says to move it away. Any other failure to read a configured key is a refusal too. +func readHostKey(keyFile core.KeyFile) ([]byte, error) { + if err := keyFile.Check(); err != nil { + return nil, &ActionError{err} + } + key, err := keyFile.ReadUnchecked() + switch { + case err == nil: + return key, nil + case keyUnusable(err, keyFile.Path()): + return nil, actionf("%v: move %s away first, because the next start would stop on it", err, keyFile.Path()) + case keyFile.Configured(): + return nil, &ActionError{err} + } + return nil, err +} + +// keyUnusable reports whether err from KeyFile.Read means that what is // at keyPath is no key: the wrong size or type, a target that does not exist // (also behind a Windows junction), or a link that leads to no file through a // loop or a file on its path. A permission error is not, and stays a failure @@ -365,13 +377,7 @@ func (k *secretKeyCmd) resetAllowed(st *store.Store, keyFile core.KeyFile) (keyM } else if err != nil { return false, err } - key, err := keyFile.Read() - if keyFile.Configured() && err != nil { - return false, &ActionError{err} - } - if keyUnusable(err, keyPath) { - return false, actionf("%v: move %s away first, because the next start would stop on it", err, keyPath) - } + key, err := readHostKey(keyFile) if err != nil { return false, err } diff --git a/internal/app/secretkeypath_cmd_test.go b/internal/app/secretkeypath_cmd_test.go index cc28396b..5ed319a8 100644 --- a/internal/app/secretkeypath_cmd_test.go +++ b/internal/app/secretkeypath_cmd_test.go @@ -297,3 +297,61 @@ func TestBackupCommandRefusesASecondKeyAndSkipsAnUnreadableOne(t *testing.T) { t.Errorf("backup with --exclude-secret-key and no key: %v\n%s", err, text) } } + +// A configured path that holds no key (a directory, a file of the wrong size) gets the message of the existing +// commands, with the path of the location (rule 15). +func TestUnusableConfiguredKeyGetsTheMoveAwayMessage(t *testing.T) { + cases := map[string]func(t *testing.T, keyPath string){ + "31 bytes": func(t *testing.T, keyPath string) { + if err := os.WriteFile(keyPath, bytes.Repeat([]byte{1}, 31), 0o600); err != nil { + t.Fatal(err) + } + }, + "a directory": func(t *testing.T, keyPath string) { + if err := os.Remove(keyPath); err != nil { + t.Fatal(err) + } + if err := os.Mkdir(keyPath, 0o700); err != nil { + t.Fatal(err) + } + }, + } + for name, breakIt := range cases { + t.Run(name, func(t *testing.T) { + _, config, _, keyPath := stoppedHostWithAKeyOutside(t) + breakIt(t, keyPath) + for _, args := range [][]string{ + {"backup-key", "--password-file", passwordFile(t, recoveryPassword)}, + {"reset", "--yes"}, + } { + _, err := runSecretKey(t, config, args...) + if err == nil || exitCode(err) != ExitConfig || !strings.Contains(err.Error(), "move "+keyPath+" away first") { + t.Errorf("%s: err = %v (status %d), want a refusal that says to move %s away", args[0], err, exitCode(err), keyPath) + } + } + }) + } +} + +// The key commands find the setting with --data and a configuration file in the data directory, as with --config +// (rule 11 and the commands' preamble). +func TestKeyCommandsFindTheSettingWithData(t *testing.T) { + dir, _, key, keyPath := stoppedHostWithAKeyOutside(t) + var out bytes.Buffer + err := SecretKeyCommand(context.Background(), []string{"backup-key", "--password-file", passwordFile(t, recoveryPassword), "--data", dir}, strings.NewReader(""), &out) + if err != nil { + t.Fatalf("backup-key --data: %v\n%s", err, out.String()) + } + copyBytes, err := os.ReadFile(filepath.Join(dir, core.RecoveryKeyFile)) + if err != nil { + t.Fatal(err) + } + if got, err := core.OpenKeyCopy(copyBytes, strings.TrimSpace(recoveryPassword)); err != nil || !bytes.Equal(got, key) { + t.Errorf("the recovery copy opens to %x, %v; want the configured key", got, err) + } + out.Reset() + err = SecretKeyCommand(context.Background(), []string{"reset", "--yes", "--data", dir}, strings.NewReader(""), &out) + if err == nil || !strings.Contains(err.Error(), keyPath) || !strings.Contains(err.Error(), "opens all") { + t.Errorf("reset --data: err = %v, want a refusal that names the configured key", err) + } +} diff --git a/internal/app/secretkeypath_test.go b/internal/app/secretkeypath_test.go index 1162af47..59124b57 100644 --- a/internal/app/secretkeypath_test.go +++ b/internal/app/secretkeypath_test.go @@ -3,6 +3,9 @@ package app import ( "bytes" "context" + "errors" + "fmt" + "net" "os" "path/filepath" "slices" @@ -265,3 +268,101 @@ func TestOperatorKeysReadTheConfiguredFileAtEachUse(t *testing.T) { t.Error("an operator key was sealed while two keys existed") } } + +// The check of the key comes before any listener opens (rule 5). With both addresses already taken, a start that +// reached a listener would fail with a listen error and not with the refusal of the key. +func TestStartRefusesAKeyBeforeAnyListenerOpens(t *testing.T) { + dir := t.TempDir() + config := initConfig(t, dir) + keyPath := filepath.Join(t.TempDir(), "secret.key") + if err := ConfigCommand(context.Background(), []string{"set", "host.secret_key_path", keyPath, "--config", config}, &bytes.Buffer{}); err != nil { + t.Fatal(err) + } + var taken []string + for range 2 { + ln, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatal(err) + } + defer ln.Close() + taken = append(taken, ln.Addr().String()) + } + h, err := Start(context.Background(), Options{ConfigPath: config, Overrides: map[string]string{ + "host.console_addr": taken[0], "host.local_addr": taken[1], "host.server_runtime": "false"}}) + if err == nil { + h.Close() + t.Fatal("the host started") + } + if exitCode(err) != ExitConfig || !strings.Contains(err.Error(), "never creates a key") || strings.Contains(err.Error(), "listen") { + t.Errorf("err = %v (status %d), want the refusal of the key and no listen error", err, exitCode(err)) + } +} + +// A key on a read-only mount works (rule 7): a folder without write permission, and a key with mode 0444. +func TestHostStartsWithAKeyInAReadOnlyFolder(t *testing.T) { + if ostest.Windows || os.Geteuid() == 0 { + t.Skip("needs Unix modes and a user that is not the superuser") + } + dir := t.TempDir() + config, key, h := hostWithSecrets(t, dir) + if err := h.Close(); err != nil { + t.Fatal(err) + } + keyPath := keyOutside(t, dir, config) + keyDir := filepath.Dir(keyPath) + if err := os.Chmod(keyPath, 0o444); err != nil { + t.Fatal(err) + } + if err := os.Chmod(keyDir, 0o555); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { os.Chmod(keyDir, 0o700) }) + h2, err := Start(context.Background(), configModeOptions(config)) + if err != nil { + t.Fatalf("the host does not start with a key in a read-only folder: %v", err) + } + defer h2.Close() + if check, err := core.CheckKey(context.Background(), h2.Store, key); err != nil || !check.All() { + t.Errorf("the key opens %+v, %v", check, err) + } + if got := listed(t, keyDir); !slices.Equal(got, []string{"flats.key"}) { + t.Errorf("the key folder holds %v", got) + } +} + +// flats config show lists the setting (rule 1). +func TestConfigShowListsTheKeyPath(t *testing.T) { + dir := t.TempDir() + config := initConfig(t, dir) + keyPath := filepath.Join(t.TempDir(), "secret.key") + var out bytes.Buffer + if err := ConfigCommand(context.Background(), []string{"set", "host.secret_key_path", keyPath, "--config", config}, &out); err != nil { + t.Fatal(err) + } + out.Reset() + if err := ConfigCommand(context.Background(), []string{"show", "--config", config}, &out); err != nil { + t.Fatal(err) + } + jsonPath := strings.ReplaceAll(keyPath, `\`, `\\`) // config show prints JSON strings + if !strings.Contains(out.String(), "host.secret_key_path") || !strings.Contains(out.String(), jsonPath) { + t.Errorf("config show does not list the key path:\n%s", out.String()) + } +} + +// Every refusal of the key exits with status 78, also the one that core.New makes after the check at the start +// (rules 5 and 8). +func TestKeyRefusalIsAnActionError(t *testing.T) { + t.Parallel() + for name, err := range map[string]error{ + "a configured key that cannot be used": fmt.Errorf("%w: nothing is there", core.ErrKeyLocation), + "a missing key over sealed values": fmt.Errorf("%w: restore it", core.ErrSecretKeyMissing), + } { + if got := keyRefusal(err); exitCode(got) != ExitConfig || !errors.Is(got, err) { + t.Errorf("%s: exit status %d (%v), want %d", name, exitCode(got), got, ExitConfig) + } + } + other := errors.New("disk full") + if got := keyRefusal(other); got != other { + t.Errorf("another error changed: %v", got) + } +} diff --git a/internal/app/serve.go b/internal/app/serve.go index a1a6aa63..b47c0eb1 100644 --- a/internal/app/serve.go +++ b/internal/app/serve.go @@ -421,12 +421,7 @@ func Start(ctx context.Context, o Options) (*Host, error) { } } if err := core.CheckSecretKey(ctx, keyFile, st); err != nil { - // A configured key that cannot be used is a refusal too, so a service manager that does not - // restart on status 78 does not loop on it (docs/spec/secret-key-path.md, rule 5). - if errors.Is(err, core.ErrSecretKeyMissing) || errors.Is(err, core.ErrKeyLocation) { - return nil, &ActionError{err} - } - return nil, err + return nil, keyRefusal(err) } // Keys given to this run seed flats.db; the stored Tailscale key wins. keys, err := readKeyInputs(o, s.legacy, deprecatedAuthKeyFile(cfg)) @@ -605,7 +600,7 @@ func Start(ctx context.Context, o Options) (*Host, error) { Reserved: []string{"flats"}, ReservedNames: h.consoleReserved, Logf: logf} svc, err := core.New(ctx, coreCfg) if err != nil { - return nil, err + return nil, keyRefusal(err) } h.Svc = svc // The deprecated key file path goes once its key is in flats.db. @@ -951,3 +946,13 @@ func relayLimit(n int64) int { } return int(n) } + +// keyRefusal makes a refusal of the host key an action error, which exits with status 78. A service manager +// that does not restart on 78 does not loop on a key that is missing or cannot be used, whether the check +// at the start or the one in core.New found it (docs/spec/secret-key-path.md, rules 5 and 8). +func keyRefusal(err error) error { + if errors.Is(err, core.ErrSecretKeyMissing) || errors.Is(err, core.ErrKeyLocation) { + return &ActionError{err} + } + return err +} diff --git a/internal/backup/backup.go b/internal/backup/backup.go index 648ac731..205f288d 100644 --- a/internal/backup/backup.go +++ b/internal/backup/backup.go @@ -174,7 +174,7 @@ func Run(ctx context.Context, o Options) (res *Result, err error) { if err != nil { return nil, err } - key0, err := readKey(keyFile, o.ExcludeSecretKey, false) + key0, err := readKey(keyFile, o.ExcludeSecretKey) if err != nil { return nil, err } @@ -224,7 +224,7 @@ func Run(ctx context.Context, o Options) (res *Result, err error) { if d := s1.diff(s2); d != "" { return nil, &RerunError{fmt.Errorf("a flat was created, renamed, deleted or restored during the copy (%s)", d)} } - key1, err := readKey(keyFile, o.ExcludeSecretKey, true) + key1, err := readKey(keyFile, o.ExcludeSecretKey) if err != nil { if keyFile.Configured() { return nil, &RerunError{fmt.Errorf("the secret key at %s cannot be read any more: %w", keyFile.Path(), err)} @@ -260,13 +260,7 @@ func Run(ctx context.Context, o Options) (res *Result, err error) { if fails, _, err := verifyTree(ctx, partial, m, externalKey); err != nil { return nil, err } else if len(fails) > 0 { - err := fmt.Errorf("the copy failed its own check: %s: %s", fails[0].Path, fails[0].Reason) - for _, f := range fails { - if !f.Fixed { - return nil, &RerunError{err} - } - } - return nil, err + return nil, checkError(fails) } if err := o.hook(20); err != nil { return nil, err @@ -319,17 +313,31 @@ func checkSchema(flatsDB string) error { return nil } +// checkError is the error of Run for the failures of the copy's own check. A failure that a second run cannot +// clear is reported before one that it can, and it decides the kind: a mix of the two is not a "run it again". +func checkError(fails []Failure) error { + first := fails[0] + for _, f := range fails { + if f.Fixed { + first = f + break + } + } + err := fmt.Errorf("the copy failed its own check: %s: %s", first.Path, first.Reason) + if first.Fixed { + return fmt.Errorf("%w; a second run does not clear this failure", err) + } + return &RerunError{err} +} + // readKey returns the bytes of the host key, or nil when the key is excluded or the default file is not -// there. A configured key must be readable, and it is not read at all when it is excluded. The second -// read of rule 22 (again) does not repeat the checks of rule 3. -func readKey(k core.KeyFile, excluded, again bool) ([]byte, error) { - switch { - case excluded || !k.Configured(): +// there. A configured key must be readable, and it is not read at all when it is excluded. The preflight +// made the checks of rule 3, so the two reads of rule 22 do not repeat them. +func readKey(k core.KeyFile, excluded bool) ([]byte, error) { + if excluded || !k.Configured() { return readKeyBytes(k.DataDir(), excluded) - case again: - return k.ReadAgain() } - return k.Read() + return k.ReadUnchecked() } // readKeyBytes returns the bytes of secret.key, following a link as the host diff --git a/internal/backup/externalkey_test.go b/internal/backup/externalkey_test.go index a3c9e615..34dcb610 100644 --- a/internal/backup/externalkey_test.go +++ b/internal/backup/externalkey_test.go @@ -5,6 +5,7 @@ import ( "context" "crypto/rand" "errors" + "fmt" "io/fs" "os" "path/filepath" @@ -132,8 +133,9 @@ func TestRunFailsWhenTheExternalKeyDoesNotOpenTheSealedValues(t *testing.T) { o.SecretKeyPath = keyPath _, err := f.run(o) var rerun *RerunError - if err == nil || errors.As(err, &rerun) || !strings.Contains(err.Error(), "does not open with the key at host.secret_key_path") { - t.Fatalf("err = %v, want a failure that a second run does not clear", err) + if err == nil || errors.As(err, &rerun) || !strings.Contains(err.Error(), "does not open with the key at host.secret_key_path") || + !strings.Contains(err.Error(), "a second run does not clear this failure") { + t.Fatalf("err = %v, want a failure that says a second run does not clear it", err) } f.nothingLeft() } @@ -157,29 +159,54 @@ func TestRunFailsWhenAHardLinkHoldsTheExternalKey(t *testing.T) { f.nothingLeft() } -// A file that appears at /secret.key during the copy is left out and listed (rule 16). -func TestRunLeavesOutAKeyThatAppearsInTheDataDirectoryDuringTheCopy(t *testing.T) { +// A file that appears at /secret.key after the preflight is left out and listed (rule 16), whether it appears +// before the survey of step 13 (after step 12) or during the copy of step 14. The reads of the key make no checks of +// rule 3 again, so neither one fails the backup. +func TestRunLeavesOutAKeyThatAppearsInTheDataDirectoryAfterThePreflight(t *testing.T) { t.Parallel() - f := populated(t) - keyPath, key := f.externalKey() - o := f.options("c1") - o.SecretKeyPath = keyPath - o.afterStep = func(step int) error { - if step == 14 { - return os.WriteFile(filepath.Join(f.data, nameSecretKey), key, 0o600) - } - return nil - } - res, err := f.run(o) - if err != nil { - t.Fatal(err) + for _, after := range []int{12, 14} { + t.Run(fmt.Sprintf("after step %d", after), func(t *testing.T) { + t.Parallel() + f := populated(t) + keyPath, key := f.externalKey() + o := f.options("c1") + o.SecretKeyPath = keyPath + o.afterStep = func(step int) error { + if step == after { + return os.WriteFile(filepath.Join(f.data, nameSecretKey), key, 0o600) + } + return nil + } + res, err := f.run(o) + if err != nil { + t.Fatal(err) + } + m := readCopyManifest(t, res.Out) + if !slices.Contains(m.Excluded, "data/secret.key") || m.has("data/secret.key") { + t.Errorf("excluded %v, files has the key: %v", m.Excluded, m.has("data/secret.key")) + } + if hits := holdsBytes(t, res.Out, key); len(hits) != 0 { + t.Errorf("the copy holds the key in %v", hits) + } + }) } - m := readCopyManifest(t, res.Out) - if !slices.Contains(m.Excluded, "data/secret.key") || m.has("data/secret.key") { - t.Errorf("excluded %v, files has the key: %v", m.Excluded, m.has("data/secret.key")) +} + +// A failure that a second run cannot clear decides the kind of the error, also when a failure that it can clear comes +// first (rule 16): "run the backup again" would only repeat the failure. +func TestCheckErrorKinds(t *testing.T) { + t.Parallel() + transient := Failure{Path: "data/flats.db", Reason: "changed"} + fixed := Failure{Path: "data/tsnet/alias.bin", Reason: "holds the bytes of the secret key", Fixed: true} + var rerun *RerunError + if err := checkError([]Failure{transient}); !errors.As(err, &rerun) || !strings.Contains(err.Error(), "run the backup again") { + t.Errorf("a failure that a second run clears: err = %v, want a RerunError", err) } - if hits := holdsBytes(t, res.Out, key); len(hits) != 0 { - t.Errorf("the copy holds the key in %v", hits) + for name, fails := range map[string][]Failure{"only fixed": {fixed}, "transient first": {transient, fixed}} { + err := checkError(fails) + if err == nil || errors.As(err, &rerun) || !strings.Contains(err.Error(), "tsnet/alias.bin") || !strings.Contains(err.Error(), "a second run does not clear this failure") { + t.Errorf("%s: err = %v, want the fixed failure, and no RerunError", name, err) + } } } diff --git a/internal/core/hostkeys.go b/internal/core/hostkeys.go index 2d877876..657eb8cf 100644 --- a/internal/core/hostkeys.go +++ b/internal/core/hostkeys.go @@ -117,7 +117,7 @@ func OpenHostKey(keyFile KeyFile, k store.HostKey) (string, error) { } pt, err := g.Open(nil, k.Nonce, k.Ciphertext, hostKeyAAD(k.ID)) if err != nil { - return "", fmt.Errorf("decrypt the stored %s key: %w; secret.key does not match flats.db: put back the matching secret.key, which `flats secret restore-key` restores from its recovery copy, or remove this key with `flats key rm` while the host is stopped and register it again", k.ID, err) + return "", fmt.Errorf("decrypt the stored %s key: %w; the key at %s does not match flats.db: put back the matching key, which `flats secret restore-key` restores from its recovery copy, or remove this key with `flats key rm` while the host is stopped and register it again", k.ID, err, keyFile.Path()) } return string(pt), nil } diff --git a/internal/core/keyfile.go b/internal/core/keyfile.go index a651c9f5..ed8bdea4 100644 --- a/internal/core/keyfile.go +++ b/internal/core/keyfile.go @@ -6,6 +6,7 @@ import ( "io/fs" "os" "path/filepath" + "runtime" "github.com/gosuda/flats/internal/config" "github.com/gosuda/flats/internal/durable" @@ -18,7 +19,7 @@ const KeySetting = "host.secret_key_path" // read: nothing at the path, a second key at /secret.key, a key that resolves into the data // directory, a file that is no key, or a file that cannot be read. Flats never creates a key at a // configured path (docs/spec/secret-key-path.md). -var ErrKeyLocation = errors.New(KeySetting + " cannot be used") +var ErrKeyLocation = errors.New("the configured secret key cannot be used") // KeyFile is where the host key lives: /secret.key, or the file that host.secret_key_path // names. Everything that reads or writes the key gets its path from here. @@ -57,6 +58,10 @@ func (k KeyFile) Check() error { } leftover := filepath.Join(k.dataDir, SecretKeyFile) if _, err := os.Lstat(leftover); err == nil { + if sameFile(leftover, k.path) { + return fmt.Errorf("%w: %s names %s, which is the same file as %s, so the data directory holds the key; keep the key in a folder that no link reaches from the data directory", + ErrKeyLocation, KeySetting, k.path, leftover) + } return fmt.Errorf("%w: %s is set, and %s exists too; move that file away, because Flats uses the configured key and a copy of the data directory would hold the old one", ErrKeyLocation, KeySetting, leftover) } else if !errors.Is(err, fs.ErrNotExist) { @@ -90,10 +95,10 @@ func (k KeyFile) Read() ([]byte, error) { return k.readConfigured() } -// ReadAgain reads the key a second time without the checks of Check. A backup that compares two reads of -// a configured key (rule 22 of the backup spec) makes those checks once, in its preflight, and a second key -// that appears in the data directory during the copy is left out of the copy and does not fail the re-read. -func (k KeyFile) ReadAgain() ([]byte, error) { +// ReadUnchecked reads the key without the checks of Check. A backup makes those checks once, in its +// preflight, and then reads the key twice (rule 22 of the backup spec). A second key that appears in the data +// directory during the copy is left out of the copy and does not fail those reads. +func (k KeyFile) ReadUnchecked() ([]byte, error) { if !k.configured { return readSecretKey(k.path) } @@ -110,8 +115,8 @@ func (k KeyFile) readConfigured() ([]byte, error) { return nil, fmt.Errorf("%w: %s names %s, which is a link to nothing; repair the link or the mount, or clear the setting with `flats config unset %s`, and Flats never creates a key at a configured path", ErrKeyLocation, KeySetting, k.path, KeySetting) } - return nil, fmt.Errorf("%w: %s names %s, and nothing is there. Flats never creates a key at a configured path. Put a key there (for example `head -c 32 /dev/urandom > %s`), restore it with `flats secret restore-key`, or clear the setting with `flats config unset %s`", - ErrKeyLocation, KeySetting, k.path, k.path, KeySetting) + return nil, fmt.Errorf("%w: %s names %s, and nothing is there. Flats never creates a key at a configured path. Put a key of 32 bytes there%s, restore it with `flats secret restore-key`, or clear the setting with `flats config unset %s`", + ErrKeyLocation, KeySetting, k.path, newKeyHint(k.path), KeySetting) } b, err := readSecretKey(k.path) if err != nil { @@ -138,3 +143,22 @@ func (k KeyFile) Restore(key []byte) error { } return durable.SyncDir(filepath.Dir(k.path)) } + +// newKeyHint is the shell line that makes a key, for a system that has /dev/urandom. Windows gets none: the +// docs give a PowerShell line there. +func newKeyHint(path string) string { + if runtime.GOOS == "windows" { + return "" + } + return fmt.Sprintf(" (for example `head -c 32 /dev/urandom > %s`)", path) +} + +// sameFile reports whether a and b are the same file once links are followed. +func sameFile(a, b string) bool { + fa, err := os.Stat(a) + if err != nil { + return false + } + fb, err := os.Stat(b) + return err == nil && os.SameFile(fa, fb) +} diff --git a/internal/core/keyfile_test.go b/internal/core/keyfile_test.go index 44e5be5d..d4e601fd 100644 --- a/internal/core/keyfile_test.go +++ b/internal/core/keyfile_test.go @@ -6,6 +6,7 @@ import ( "errors" "os" "path/filepath" + "runtime" "slices" "strings" "testing" @@ -84,7 +85,11 @@ func TestConfiguredKeyIsNeverCreated(t *testing.T) { t.Errorf("%s: err = %v, want ErrKeyLocation", name, err) continue } - for _, want := range []string{path, KeySetting, "never creates", "head -c 32 /dev/urandom", "flats secret restore-key", "flats config unset"} { + wants := []string{path, KeySetting, "never creates", "flats secret restore-key", "flats config unset"} + if runtime.GOOS != "windows" { + wants = append(wants, "head -c 32 /dev/urandom") // Windows gets no shell line (the docs give a PowerShell one) + } + for _, want := range wants { if !strings.Contains(err.Error(), want) { t.Errorf("%s: the message lacks %q: %v", name, want, err) } @@ -260,15 +265,11 @@ func TestConfiguredKeyThatResolvesIntoTheDataDirectory(t *testing.T) { if err := ostest.Symlink(t, real, data); err != nil { t.Skipf("no symbolic links here: %v", err) } - // The key is configured at its real path, which lies in what the data directory resolves to. The leftover - // check does not see it, because /secret.key resolves to the same file, so look at it first. - err := readErr(NewKeyFile(data, filepath.Join(real, "elsewhere"))) - if err == nil { - t.Log("the path of the key does not exist, so there is nothing to resolve") - } - err = readErr(NewKeyFile(data, filepath.Join(real, "secret.key"))) - if !errors.Is(err, ErrKeyLocation) { - t.Errorf("err = %v, want ErrKeyLocation", err) + // The key is configured at its real path, which lies in what the data directory resolves to. /secret.key + // is that same file, so the refusal must not tell the operator to move "that file" away: it is the only key. + err := readErr(NewKeyFile(data, filepath.Join(real, "secret.key"))) + if !errors.Is(err, ErrKeyLocation) || !strings.Contains(err.Error(), "the same file") || strings.Contains(err.Error(), "move that file away") { + t.Errorf("err = %v, want a refusal that says the two paths are the same file and does not say to move it away", err) } }) t.Run("a key outside the data directory is fine", func(t *testing.T) { @@ -352,9 +353,9 @@ func TestRestoreOfAConfiguredKeyInAReadOnlyFolder(t *testing.T) { } } -// ReadAgain does not repeat the checks of Check: a second key that appears in the data directory during a backup does +// ReadUnchecked does not repeat the checks of Check: a second key that appears in the data directory during a backup does // not fail the re-read, and a missing file still does. -func TestReadAgainSkipsTheChecksOfCheck(t *testing.T) { +func TestReadUncheckedSkipsTheChecksOfCheck(t *testing.T) { t.Parallel() data, keyDir := t.TempDir(), t.TempDir() path := filepath.Join(keyDir, "secret.key") @@ -364,17 +365,17 @@ func TestReadAgainSkipsTheChecksOfCheck(t *testing.T) { if _, err := k.Read(); !errors.Is(err, ErrKeyLocation) { t.Fatalf("Read with a second key: err = %v, want ErrKeyLocation", err) } - if got, err := k.ReadAgain(); err != nil || !bytes.Equal(got, goodKey()) { - t.Errorf("ReadAgain = %v, %v; want the key", got, err) + if got, err := k.ReadUnchecked(); err != nil || !bytes.Equal(got, goodKey()) { + t.Errorf("ReadUnchecked = %v, %v; want the key", got, err) } if err := os.Remove(path); err != nil { t.Fatal(err) } - if _, err := k.ReadAgain(); !errors.Is(err, ErrKeyLocation) { - t.Errorf("ReadAgain of a missing key: err = %v, want ErrKeyLocation", err) + if _, err := k.ReadUnchecked(); !errors.Is(err, ErrKeyLocation) { + t.Errorf("ReadUnchecked of a missing key: err = %v, want ErrKeyLocation", err) } // A default key is read as before. - if got, err := NewKeyFile(data, "").ReadAgain(); err != nil || !bytes.Equal(got, goodKey()) { - t.Errorf("ReadAgain of the default key = %v, %v", got, err) + if got, err := NewKeyFile(data, "").ReadUnchecked(); err != nil || !bytes.Equal(got, goodKey()) { + t.Errorf("ReadUnchecked of the default key = %v, %v", got, err) } } diff --git a/internal/core/keyrecovery.go b/internal/core/keyrecovery.go index 066ff243..03ce7559 100644 --- a/internal/core/keyrecovery.go +++ b/internal/core/keyrecovery.go @@ -229,7 +229,7 @@ func RecoveryCopyWarning(dataDir string, key []byte) string { if err == nil && strings.TrimSpace(string(b)) == KeyID(key) { return "" } - return fmt.Sprintf("%s may hold another key than %s, so it may not restore this host: run `flats secret backup-key` to write a current copy", copyPath, SecretKeyFile) + return fmt.Sprintf("%s may hold another key than the host key, so it may not restore this host: run `flats secret backup-key` to write a current copy", copyPath) } // readHead reads at most n bytes from the start of the file at path. From 6c452054d68de26063d08f51c201509b8d16a2e3 Mon Sep 17 00:00:00 2001 From: Hee Sung Son Date: Sat, 14 Nov 2026 19:53:04 +0900 Subject: [PATCH 10/14] fix(secrets): finish the wording findings of the implementation review round 2 Uses one key hint for the missing-key refusal and the reset output, so Windows gets no shell line in either. Fixes the chmod line of the Docker steps and one sentence of rule 21, and adds a test that reset refuses a second key in the data directory while the configured key is missing. --- docs/container.md | 6 ++--- docs/spec/secret-key-path.md | 8 +++---- internal/app/secretkeycmd.go | 2 +- internal/app/secretkeypath_cmd_test.go | 31 ++++++++++++++++++++++++++ internal/core/keyfile.go | 8 +++---- 5 files changed, 43 insertions(+), 12 deletions(-) diff --git a/docs/container.md b/docs/container.md index 66af7d99..c522e516 100644 --- a/docs/container.md +++ b/docs/container.md @@ -273,7 +273,7 @@ it as a file and name it with `host.secret_key_path` The host only reads that file and never creates it, so a read-only mount works. The container user `65532` must be able to read it. A bind-mounted file keeps its own owner and mode on the host, so give it to that user, for example -`sudo chown 65532 flats.key && chmod 0400 flats.key`. +`sudo chown 65532 flats.key && sudo chmod 0400 flats.key`. Stop the container, move the key out of the volume, and set the path with one-off containers on the same volume. The last command is the one that you @@ -284,7 +284,7 @@ docker stop flats (umask 077; docker run --rm -v flats-data:/data:ro alpine cat /data/secret.key > flats.key) docker run --rm -v flats-data:/data alpine rm /data/secret.key docker run --rm -v flats-data:/data ghcr.io/gosuda/flats:latest config set host.secret_key_path /run/secrets/flats.key -sudo chown 65532 flats.key && chmod 0400 flats.key +sudo chown 65532 flats.key && sudo chmod 0400 flats.key docker rm flats docker run -d --name flats --restart unless-stopped \ -v flats-data:/data -v "$PWD/flats.key:/run/secrets/flats.key:ro" \ @@ -316,7 +316,7 @@ docker compose stop flats container=$(docker compose ps -a -q flats) (umask 077; docker run --rm --volumes-from "$container":ro alpine:3.22 cat /data/secret.key > flats.key) docker run --rm --volumes-from "$container" alpine:3.22 rm /data/secret.key -sudo chown 65532 flats.key && chmod 0400 flats.key +sudo chown 65532 flats.key && sudo chmod 0400 flats.key docker compose run --rm --no-deps flats config set host.secret_key_path /run/secrets/flats.key docker compose up -d --wait ``` diff --git a/docs/spec/secret-key-path.md b/docs/spec/secret-key-path.md index 1249ee0b..8b20f76a 100644 --- a/docs/spec/secret-key-path.md +++ b/docs/spec/secret-key-path.md @@ -225,10 +225,10 @@ call it, so they still work when the location is in one of those states. that only the service process can see, such as a systemd `LoadCredential=` file under `$CREDENTIALS_DIRECTORY`, or a Docker secret that is mounted into the service container, is missing for a process outside the service. A - command that runs inside the container with `docker exec`, and a one-off - container from `docker compose run`, which mounts the secrets of its service, - sees the secret. A container from plain `docker run` and a process on the - host do not. For such a key, `flats backup` + command that runs inside the container with `docker exec` sees the secret. So + does a one-off container from `docker compose run`, which mounts the secrets + of its service. A container from plain `docker run` and a process on the host + do not. For such a key, `flats backup` needs `--exclude-secret-key`, `restore-key` needs `--out`, and the other key commands cannot run unless the operator gives them the same mount. The docs say this, and they show a deployment that works for every command, a plain diff --git a/internal/app/secretkeycmd.go b/internal/app/secretkeycmd.go index ddf91e6b..825fe654 100644 --- a/internal/app/secretkeycmd.go +++ b/internal/app/secretkeycmd.go @@ -349,7 +349,7 @@ func (k *secretKeyCmd) reset(args []string) error { k.printf("deleted %d values.\n", total) } if keyMissing && keyFile.Configured() { - k.printf("The next start needs a key at %s, because Flats never creates a key at a configured path. Put a new key there, for example with `head -c 32 /dev/urandom > %s`, before you start the host.\n", keyPath, keyPath) + k.printf("The next start needs a key at %s, because Flats never creates a key at a configured path. Put a new key of 32 bytes there%s before you start the host.\n", keyPath, core.NewKeyHint(keyPath)) } else if keyMissing { why := "to keep a recovery copy of the new key" if _, err := os.Lstat(filepath.Join(dataDir, core.RecoveryKeyFile)); err == nil { diff --git a/internal/app/secretkeypath_cmd_test.go b/internal/app/secretkeypath_cmd_test.go index 5ed319a8..0ccc8c0b 100644 --- a/internal/app/secretkeypath_cmd_test.go +++ b/internal/app/secretkeypath_cmd_test.go @@ -355,3 +355,34 @@ func TestKeyCommandsFindTheSettingWithData(t *testing.T) { t.Errorf("reset --data: err = %v, want a refusal that names the configured key", err) } } + +// reset deletes values only when the key at the location is missing or opens none of them. A second key in the data +// directory is a refusal of rule 3 first, also when the configured key is missing: the host would not start with two keys, +// and reset must not delete values on the way (rule 14). +func TestResetRefusesASecondKeyInTheDataDirectoryWhileTheConfiguredKeyIsMissing(t *testing.T) { + dir, config, key, keyPath := stoppedHostWithAKeyOutside(t) + if err := os.Remove(keyPath); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dir, core.SecretKeyFile), key, 0o600); err != nil { + t.Fatal(err) + } + _, err := runSecretKey(t, config, "reset", "--yes") + if err == nil || exitCode(err) != ExitConfig || !strings.Contains(err.Error(), "move that file away") { + t.Fatalf("reset: err = %v (status %d), want the refusal of the second key", err, exitCode(err)) + } + if err := os.Remove(filepath.Join(dir, core.SecretKeyFile)); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(keyPath, key, 0o600); err != nil { + t.Fatal(err) + } + h, err := Start(context.Background(), configModeOptions(config)) + if err != nil { + t.Fatalf("the values did not survive the refused reset: %v", err) + } + defer h.Close() + if check, err := core.CheckKey(context.Background(), h.Store, key); err != nil || !check.All() || check.Total != 2 { + t.Errorf("after the refused reset the key opens %+v, %v; want both values", check, err) + } +} diff --git a/internal/core/keyfile.go b/internal/core/keyfile.go index ed8bdea4..6b689069 100644 --- a/internal/core/keyfile.go +++ b/internal/core/keyfile.go @@ -116,7 +116,7 @@ func (k KeyFile) readConfigured() ([]byte, error) { ErrKeyLocation, KeySetting, k.path, KeySetting) } return nil, fmt.Errorf("%w: %s names %s, and nothing is there. Flats never creates a key at a configured path. Put a key of 32 bytes there%s, restore it with `flats secret restore-key`, or clear the setting with `flats config unset %s`", - ErrKeyLocation, KeySetting, k.path, newKeyHint(k.path), KeySetting) + ErrKeyLocation, KeySetting, k.path, NewKeyHint(k.path), KeySetting) } b, err := readSecretKey(k.path) if err != nil { @@ -144,9 +144,9 @@ func (k KeyFile) Restore(key []byte) error { return durable.SyncDir(filepath.Dir(k.path)) } -// newKeyHint is the shell line that makes a key, for a system that has /dev/urandom. Windows gets none: the -// docs give a PowerShell line there. -func newKeyHint(path string) string { +// NewKeyHint is the shell line that makes a key at path, in the form " (for example `...`)", for a system +// that has /dev/urandom. Windows gets "": the docs give a PowerShell line there. +func NewKeyHint(path string) string { if runtime.GOOS == "windows" { return "" } From d2e57c381c42609738263c7755a3673dbb35a343 Mon Sep 17 00:00:00 2001 From: Hee Sung Son Date: Sat, 14 Nov 2026 19:54:09 +0900 Subject: [PATCH 11/14] test(secrets): cover restore-key --out below the data directory --- internal/app/secretkeypath_cmd_test.go | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/internal/app/secretkeypath_cmd_test.go b/internal/app/secretkeypath_cmd_test.go index 0ccc8c0b..6377a07c 100644 --- a/internal/app/secretkeypath_cmd_test.go +++ b/internal/app/secretkeypath_cmd_test.go @@ -131,6 +131,24 @@ func TestRestoreKeyOutWritesANewFile(t *testing.T) { if _, err := os.Lstat(inside); !os.IsNotExist(err) { t.Errorf("--out wrote inside the data directory: %v", err) } + // A folder below the data directory is inside it too, and so is a link to a folder there. + below := filepath.Join(dir, "below") + if err := os.Mkdir(below, 0o700); err != nil { + t.Fatal(err) + } + outs := map[string]string{"below": filepath.Join(below, "recovered.key")} + if link := filepath.Join(t.TempDir(), "link"); ostest.Symlink(t, below, link) == nil { + outs["a link to a folder below"] = filepath.Join(link, "recovered.key") + } + for name, where := range outs { + _, err := runSecretKey(t, config, "restore-key", "--out", where, "--password-file", pw) + if err == nil || exitCode(err) != ExitConfig || !strings.Contains(err.Error(), "outside the data directory") { + t.Errorf("--out %s: %v (status %d), want a refusal", name, err, exitCode(err)) + } + if _, err := os.Lstat(filepath.Join(below, "recovered.key")); !os.IsNotExist(err) { + t.Errorf("--out %s wrote inside the data directory: %v", name, err) + } + } // A second key in the data directory is a refusal with or without --out (rule 3). if err := os.WriteFile(filepath.Join(dir, core.SecretKeyFile), key, 0o600); err != nil { t.Fatal(err) From afdc191aa4a19e008d1b3a2a75a7ba5d408e9940 Mon Sep 17 00:00:00 2001 From: Hee Sung Son Date: Sat, 14 Nov 2026 19:55:01 +0900 Subject: [PATCH 12/14] test(secrets): test restore-key --out below the data directory on its own --- internal/app/secretkeypath_cmd_test.go | 49 ++++++++++++++++---------- 1 file changed, 31 insertions(+), 18 deletions(-) diff --git a/internal/app/secretkeypath_cmd_test.go b/internal/app/secretkeypath_cmd_test.go index 6377a07c..64445bb3 100644 --- a/internal/app/secretkeypath_cmd_test.go +++ b/internal/app/secretkeypath_cmd_test.go @@ -131,24 +131,6 @@ func TestRestoreKeyOutWritesANewFile(t *testing.T) { if _, err := os.Lstat(inside); !os.IsNotExist(err) { t.Errorf("--out wrote inside the data directory: %v", err) } - // A folder below the data directory is inside it too, and so is a link to a folder there. - below := filepath.Join(dir, "below") - if err := os.Mkdir(below, 0o700); err != nil { - t.Fatal(err) - } - outs := map[string]string{"below": filepath.Join(below, "recovered.key")} - if link := filepath.Join(t.TempDir(), "link"); ostest.Symlink(t, below, link) == nil { - outs["a link to a folder below"] = filepath.Join(link, "recovered.key") - } - for name, where := range outs { - _, err := runSecretKey(t, config, "restore-key", "--out", where, "--password-file", pw) - if err == nil || exitCode(err) != ExitConfig || !strings.Contains(err.Error(), "outside the data directory") { - t.Errorf("--out %s: %v (status %d), want a refusal", name, err, exitCode(err)) - } - if _, err := os.Lstat(filepath.Join(below, "recovered.key")); !os.IsNotExist(err) { - t.Errorf("--out %s wrote inside the data directory: %v", name, err) - } - } // A second key in the data directory is a refusal with or without --out (rule 3). if err := os.WriteFile(filepath.Join(dir, core.SecretKeyFile), key, 0o600); err != nil { t.Fatal(err) @@ -404,3 +386,34 @@ func TestResetRefusesASecondKeyInTheDataDirectoryWhileTheConfiguredKeyIsMissing( t.Errorf("after the refused reset the key opens %+v, %v; want both values", check, err) } } + +// A folder below the data directory is inside it, and so is a link to such a folder: --out refuses both (rule 13). +func TestRestoreKeyOutRefusesAFolderBelowTheDataDirectory(t *testing.T) { + cases := map[string]func(t *testing.T, below string) string{ + "below": func(t *testing.T, below string) string { return filepath.Join(below, "recovered.key") }, + "a link to a folder below": func(t *testing.T, below string) string { + link := filepath.Join(t.TempDir(), "link") + if err := ostest.Symlink(t, below, link); err != nil { + t.Skipf("no symbolic links here: %v", err) + } + return filepath.Join(link, "recovered.key") + }, + } + for name, outAt := range cases { + t.Run(name, func(t *testing.T) { + dir, config, _, _ := stoppedHostWithAKeyOutside(t) + mustRunSecretKey(t, config, "backup-key", "--password-file", passwordFile(t, recoveryPassword)) + below := filepath.Join(dir, "below") + if err := os.Mkdir(below, 0o700); err != nil { + t.Fatal(err) + } + _, err := runSecretKey(t, config, "restore-key", "--out", outAt(t, below), "--password-file", passwordFile(t, recoveryPassword)) + if err == nil || exitCode(err) != ExitConfig || !strings.Contains(err.Error(), "outside the data directory") { + t.Errorf("err = %v (status %d), want a refusal that asks for a file outside the data directory", err, exitCode(err)) + } + if got := listed(t, below); len(got) != 0 { + t.Errorf("--out wrote inside the data directory: %v", got) + } + }) + } +} From b08ad6a8938f52573c11422bc0e81d831fe44f56 Mon Sep 17 00:00:00 2001 From: Hee Sung Son Date: Sat, 14 Nov 2026 20:26:05 +0900 Subject: [PATCH 13/14] test(secrets): cover every key command in the cases of rule 3 and the console operator-key methods --- internal/app/secretkeypath_cmd_test.go | 71 ++++++++++++++++++++++++++ internal/app/secretkeypath_test.go | 39 ++++++++++++++ 2 files changed, 110 insertions(+) diff --git a/internal/app/secretkeypath_cmd_test.go b/internal/app/secretkeypath_cmd_test.go index 64445bb3..50f05951 100644 --- a/internal/app/secretkeypath_cmd_test.go +++ b/internal/app/secretkeypath_cmd_test.go @@ -417,3 +417,74 @@ func TestRestoreKeyOutRefusesAFolderBelowTheDataDirectory(t *testing.T) { }) } } + +// Every command that reads or writes the key refuses in the two states of rule 3 that the setting alone cannot see: a +// second key at /secret.key, and a configured path that resolves into the data directory. It changes nothing then. +// `key ls` and `key rm` touch no key, so they still work in both states (the commands' preamble and rule 3). +func TestEveryKeyCommandRefusesInTheCasesOfRule3(t *testing.T) { + states := map[string]struct { + put func(t *testing.T, dir, keyPath string, key []byte) + want string + }{ + "a second key in the data directory": {func(t *testing.T, dir, keyPath string, key []byte) { + if err := os.WriteFile(filepath.Join(dir, core.SecretKeyFile), key, 0o600); err != nil { + t.Fatal(err) + } + }, "move that file away"}, + "a key path that is a link into the data directory": {func(t *testing.T, dir, keyPath string, key []byte) { + inner := filepath.Join(dir, "inner.key") + if err := os.WriteFile(inner, key, 0o600); err != nil { + t.Fatal(err) + } + if err := os.Remove(keyPath); err != nil { + t.Fatal(err) + } + if err := ostest.Symlink(t, inner, keyPath); err != nil { + t.Skipf("no symbolic links here: %v", err) + } + }, "inside the data directory"}, + } + for name, st := range states { + t.Run(name, func(t *testing.T) { + dir, config, key, keyPath := stoppedHostWithAKeyOutside(t) + outFile := filepath.Join(t.TempDir(), "out.key") + st.put(t, dir, keyPath, key) + refuses := map[string]func() error{ + "backup-key": func() error { + _, err := runSecretKey(t, config, "backup-key", "--password-file", passwordFile(t, recoveryPassword)) + return err + }, + "restore-key": func() error { + _, err := runSecretKey(t, config, "restore-key", "--password-file", passwordFile(t, recoveryPassword)) + return err + }, + "restore-key --out": func() error { + _, err := runSecretKey(t, config, "restore-key", "--out", outFile, "--password-file", passwordFile(t, recoveryPassword)) + return err + }, + "reset": func() error { _, err := runSecretKey(t, config, "reset", "--yes"); return err }, + "key set": func() error { + return KeyCommand(context.Background(), []string{"set", "tailscale", "--config", config}, strings.NewReader(keyA), &bytes.Buffer{}) + }, + } + for cmd, run := range refuses { + if err := run(); err == nil || exitCode(err) != ExitConfig || !strings.Contains(err.Error(), st.want) { + t.Errorf("%s: err = %v (status %d), want a refusal that says %q", cmd, err, exitCode(err), st.want) + } + } + for _, cmd := range [][]string{{"ls"}, {"rm", "tailscale"}} { + if err := KeyCommand(context.Background(), append(cmd, "--config", config), strings.NewReader(""), &bytes.Buffer{}); err != nil { + t.Errorf("key %s in this state: %v", cmd[0], err) + } + } + for _, name := range []string{core.RecoveryKeyFile, core.RecoveryKeyIDFile} { + if _, err := os.Lstat(filepath.Join(dir, name)); !os.IsNotExist(err) { + t.Errorf("a refused command wrote %s: %v", name, err) + } + } + if _, err := os.Lstat(outFile); !os.IsNotExist(err) { + t.Errorf("a refused restore-key --out wrote its file: %v", err) + } + }) + } +} diff --git a/internal/app/secretkeypath_test.go b/internal/app/secretkeypath_test.go index 59124b57..448d1f85 100644 --- a/internal/app/secretkeypath_test.go +++ b/internal/app/secretkeypath_test.go @@ -366,3 +366,42 @@ func TestKeyRefusalIsAnActionError(t *testing.T) { t.Errorf("another error changed: %v", got) } } + +// The console's methods for the operator keys seal and open under the configured file, through the key location of +// the host, and never create a key in the data directory (rules 8 and 9). +func TestConsoleOperatorKeyMethodsUseTheConfiguredFile(t *testing.T) { + ctx := context.Background() + dir := t.TempDir() + config, key, h := hostWithSecrets(t, dir) + if err := h.Close(); err != nil { + t.Fatal(err) + } + keyPath := keyOutside(t, dir, config) + h2, err := Start(ctx, configModeOptions(config)) + if err != nil { + t.Fatal(err) + } + defer h2.Close() + if err := h2.SetTailscaleKey(ctx, keyA); err != nil { + t.Fatalf("SetTailscaleKey: %v", err) + } + if v, err := h2.currentTailscaleKey(ctx); err != nil || v != keyA { + t.Errorf("currentTailscaleKey = %q, %v; want the key that the console stored", v, err) + } + if _, err := os.Lstat(filepath.Join(dir, core.SecretKeyFile)); !os.IsNotExist(err) { + t.Errorf("a console method created a key in the data directory: %v", err) + } + // The key opens only with the configured file: with another key there, the stored value does not open. + if err := os.WriteFile(keyPath, bytes.Repeat([]byte{9}, 32), 0o600); err != nil { + t.Fatal(err) + } + if _, err := h2.currentTailscaleKey(ctx); err == nil { + t.Error("a replaced key file still opened the stored Tailscale key") + } + if err := os.WriteFile(keyPath, key, 0o600); err != nil { + t.Fatal(err) + } + if v, err := h2.currentTailscaleKey(ctx); err != nil || v != keyA { + t.Errorf("after the key came back: %q, %v", v, err) + } +} From f7956fa1b4ba16297c2ba17b1df90a254e296309 Mon Sep 17 00:00:00 2001 From: Hee Sung Son Date: Sat, 14 Nov 2026 21:00:37 +0900 Subject: [PATCH 14/14] fix(config): clean host.secret_key_path in the form that code scanning accepts CodeQL flagged five go/path-injection alerts on the file calls of the configured key path. cleanDataDir already gives the form that its analysis accepts for host.data_dir, so the key path uses it too. The value is validated as an absolute path first, so the result is the same path. --- internal/config/keypath.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/internal/config/keypath.go b/internal/config/keypath.go index 3537b4d6..8b4f2ae3 100644 --- a/internal/config/keypath.go +++ b/internal/config/keypath.go @@ -6,12 +6,13 @@ import ( "strings" ) -// cleanKeyPath returns host.secret_key_path in clean form, or "" for no path. +// cleanKeyPath returns host.secret_key_path in clean form, or "" for no path. It uses the form of +// cleanDataDir, which static analysis accepts as free of ".." traversal for the file calls on the path. func cleanKeyPath(p string) string { if p == "" { return "" } - return filepath.Clean(p) + return cleanDataDir(p) } // InsideDir reports whether child is dir or lies below it. It compares the two