diff --git a/CHANGELOG.md b/CHANGELOG.md index 25c4f193..1d154515 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -117,6 +117,58 @@ this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ### Fixed +- Fixed the files bootroot writes being published by truncating the + destination and writing over it, so a crash or a concurrent reader + could see a half-written file at a name that is supposed to hold a + complete one. Each is now written to a temporary file in the same + directory and renamed into place, so a reader sees either the previous + file or the whole new one: `state.json`, the issued certificate files + and the CA bundle, the `--summary-json` and `--root-token-output` + destinations, `agent.toml`, `.env`, `ca.json` and its OpenBao Agent + template, `openbao.hcl`, the HTTP-01 responder config and template, + the OpenBao Agent configs and their `AppRole` credentials, the + generated compose overrides, and the remote bootstrap artifact. Two + files are not among them and are still written in place, to be fixed + separately: the OpenBao unseal-keys file and the ACME EAB credentials + file. The files a run reads back to resume flush the directory too, + so the published name survives a power loss and not merely a clean + replacement — `state.json`, `.env`, `agent.toml`, the two `init` + outputs, and every credential file the stack logs in with, which takes + an operator or another rotation to put back rather than the next + write. Files that are regenerated on their own — a certificate, a + rendered `ca.json`, a compose override — do not pay for that flush, + because a crash that loses one costs a rewrite rather than an outage. + A destination pointed elsewhere by a symlink keeps being written + through that link wherever it names configuration an operator may have + relocated — `.env`, `ca.json` and its template, `openbao.hcl`, the + responder and OpenBao Agent configs, the compose overrides, + `state.json`, and the two `init` outputs — so the link survives the + write and goes on naming the same file. The `agent.toml` that + `bootroot-remote bootstrap` writes on a target host is written + through a link there for the same reason. A link at an issued + certificate, key or CA bundle path is replaced by the published file + instead, which is what the key file and the control node's own + `agent.toml` have always done and what the certificate beside them + now matches. +- Fixed the mode of every such file being applied after its bytes had + already landed, which left a moment in which a freshly created file + was readable more widely than intended — including the step-ca CA + password, the OpenBao recovery keys, the responder HMAC config and + each `AppRole` `secret_id`. The mode is now applied while the file is + still at its temporary name, so it holds from the moment the file + appears. The modes themselves are unchanged: `0644` for the + certificates and CA bundle, `0600` for the two `init` outputs and for + everything inside the secrets tree, and for a file that already exists + whatever mode it already carries — one narrowed by hand, or by a + restrictive umask when it was created, stays narrowed. Where such a + file is created fresh it now takes a stated mode rather than whatever + the umask decides, which is `0644` for `state.json`, `.env`, + `ca.json`, `openbao.hcl` and the compose overrides; a host running a + non-default umask is the only one that can observe the difference. +- Fixed a `--summary-json` or `--root-token-output` destination whose + symlink chain loops back on itself being accepted by the preflight and + failing at the write, once `reinit` had already wiped OpenBao. It is + refused before the wipe now. - Fixed `bootroot init` treating a closed stdin as an answer. Every `init` prompt read the terminating EOF as an empty line, so a run whose piped answer sequence ran out answered the rest of its prompts diff --git a/docs/en/cli.md b/docs/en/cli.md index c983e8cc..c2f33aa7 100644 --- a/docs/en/cli.md +++ b/docs/en/cli.md @@ -104,6 +104,92 @@ Key points: For detailed rules and condition-specific behavior, see the Overview section [/etc/hosts Mapping](index.md#etchosts-mapping). +## How bootroot writes files + +Nearly every file bootroot produces is published by stage-then-rename: the bytes +go to a temporary file in the destination's own directory, that file is given its +final mode and ownership and then flushed while it is still at its temporary +name, and only then is it renamed over the destination. Three consequences are worth +knowing when you operate around a running stack. + +- **A reader never sees a partial file.** A container mounting the file, a + sidecar re-rendering it, `docker compose` interpolating `.env`, or a + `bootroot-agent` reloading `agent.toml` sees either the previous file or the + complete new one. A write that fails partway leaves the previous file + untouched and removes the temporary. +- **The final mode holds from the moment the file appears.** There is no window + in which a freshly written CA password, recovery key, `secret_id` or + responder HMAC is readable more widely than intended. +- **A rename installs a new inode.** The file at the destination path is a + different inode after every write, so anything holding an open file + descriptor — a `tail -f`, a container that opened the file at start — keeps + reading the old contents until it reopens the path. Bind mounts of a + *directory* follow the rename; a bind mount of a single *file* does not, and + needs the container restarted to pick up a new version. + +Two files are not published this way yet, and none of the three points above +applies to them: `secrets/openbao/unseal-keys.txt`, written by `init +--save-unseal-keys` and `bootroot openbao save-unseal-keys`, and the `eab.json` +written next to each service's `secret_id`. Both are still written over the +destination in place and have their `0600` set afterwards, so a reader can catch +one half-written, and a freshly created one is briefly readable more widely than +that. Converting them is a separate change; the rest of this section describes +the staged writers only. + +Whether the containing directory is flushed after the rename is decided per +file, because that flush costs a disk round trip on every write: + +Flushed, so the published file survives a power loss: + +- `state.json`, `.env`, `agent.toml` +- the `init` `--summary-json` and `--root-token-output` files +- the step-ca CA password and the OpenBao recovery keys +- every `AppRole` `role_id`/`secret_id`, and the remote bootstrap artifact + +Not flushed, because a crash that loses one costs a rewrite rather than an +outage: + +- issued certificates, keys, and the CA bundle +- `ca.json` and its OpenBao Agent template +- `openbao.hcl`, and the HTTP-01 responder config and template +- the OpenBao Agent configs, and the generated compose overrides + +The second list is regenerated on its own: by the next renewal, by the OpenBao +Agent sidecar's next render, or by re-running the command that produced it. The +first is not — bootroot reads it back to resume, or it holds a credential the +stack logs in with, and losing one of those takes an operator or another +rotation to put back rather than the next write. A `role_id` is on the flushed +list with the `secret_id` beside it for that reason: bootroot can read it from +OpenBao again, but only on the next `rotate` run, and until then the agent or +sidecar it belongs to cannot log in. + +Modes are taken from the file already at the destination where it has one, so a +file you narrow by hand stays narrowed across every later write. Only a fresh +create takes bootroot's stated default: `0600` inside the secrets tree and for +the two `init` outputs, `0644` for `state.json`, `.env`, `ca.json`, +`openbao.hcl`, the issued certificates, the CA bundle and the compose +overrides. + +If you point one of these paths at a file elsewhere with a symlink, what happens +depends on which file it is, because a rename replaces the name it is given +rather than following a link at it: + +- **Configuration you may have relocated is written through the link.** For + `.env`, `ca.json` and its template, `openbao.hcl`, the HTTP-01 responder + config and template, the OpenBao Agent configs, the generated compose + overrides, `state.json` and the two `init` output files, bootroot resolves the + link first and publishes to its target, so the link keeps naming the file it + named before. A chain of links that loops back on itself names no target and + is refused. The `agent.toml` that `bootroot-remote bootstrap` writes on a + target host is in this group: that command is what creates the file there, so + a link you put at its `--agent-config-path` is one you arranged yourself. +- **The control node's own `agent.toml`, the issued certificate and key, the CA + bundle, and every credential are published at the path itself**, replacing a + link found there with a regular file. Those files have been published by + rename for several releases, so a link at one of those paths has never + survived the command that creates the file; for a credential, following a link + would also mean a write redirected by whoever could plant one. + ## bootroot infra up Starts OpenBao/PostgreSQL/step-ca/HTTP-01 responder via Docker Compose and @@ -1132,14 +1218,27 @@ into the managed `agent.toml` profile block, threaded through the remote-bootstrap artifact, and surfaced on `DaemonProfileSettings`, so rotation always reapplies the same policy. -Atomicity: the key file is written via stage-then-rename — the bytes -are first written to a sibling temp file created with `O_CREAT|O_EXCL` -and `mode=0600`, the staged file is `chown`d and promoted to `0640` -(when the policy is active), and only then renamed over the -destination. The destination path is therefore never observable at a -mode wider than the final policy: there is no umask-derived `0644` -window before the clamp, and no group-readable window under the -operator's primary gid before the chown lands. +Atomicity: the key file, the certificate and the CA bundle are all +written via stage-then-rename, through the same publish routine that +writes `state.json` and the `init` outputs — the bytes are first +written to a sibling temp file created with `O_CREAT|O_EXCL` at +`mode=0600`, the staged file is `chown`d (when the policy is active) +and set to its final mode — `0640` for the key under an active policy, +`0600` otherwise, `0644` for the certificate and the bundle — and only +then renamed over the destination. Two properties follow. The +destination path is never +observable at a mode wider than the final policy: there is no +umask-derived `0644` window before the clamp, and no group-readable +window under the operator's primary gid before the chown lands. And a +consumer reading the destination during a rotation — a server being +reloaded, or the agent rebuilding its trust store — sees either the +previous file or the complete new one, never a truncated PEM. + +The containing directory is deliberately not flushed after these +renames. A crash that loses one leaves the previous cert, key or +bundle in place and the next renewal reissues, which costs a reissue +rather than an outage; `state.json` and the `init` output files, which +bootroot reads back to resume, do take that flush. ### Interactive behavior @@ -2243,10 +2342,23 @@ operator-managed runbook for those. current process (e.g. mode `0400`), or if the parent directory cannot accept a new file, so a bad path cannot leave the operator with a wiped-and-reinitialised OpenBao plus a failed token write. - New token files are created atomically with mode `0600` via - `OpenOptionsExt::mode` so the freshly minted root token is never - observable on disk with the process umask's default permissions - between create and chmod. Should the post-init write still fail + The token file is written via stage-then-rename: the token goes to + a temporary file in the destination's own directory, born `0600`, + which is flushed and then renamed over the destination, and the + containing directory is flushed after the rename. So the freshly + minted root token is never observable on disk with the process + umask's default permissions, the destination name never holds a + partially written token, and a published token survives a power + loss — it is the only copy of a credential reinit will not mint + again. An existing destination is narrowed to `0600` first, for the + older token that may still be sitting in it. A destination that is a + symlink to a regular file stays supported: the link is resolved and + the token is written to its target, so the rename replaces the file + the preflight judged rather than the link naming it. A chain of links + that loops back on itself names no such file, and the preflight + refuses the path — before the wipe, where the truncating write this + replaced reported `ELOOP` after it. Should the + post-init write still fail (e.g. disk full), the freshly issued token is surfaced on stderr in cleartext (prefixed with `ROOT_TOKEN=`) so it is not lost. - `--enable `: passed through to `init` (e.g. @@ -2271,13 +2383,17 @@ operator-managed runbook for those. unwritable / uncreatable parent. The summary JSON carries the freshly issued root token and unseal keys, so an unwritable destination would recreate the partial-init trap through a - different output channel, and a wider-than-`0600` destination - would briefly leak those secrets on disk between the write and - the post-write chmod. The summary file itself is written - atomically: new files are born `0600` via the create-mode flag, - and any existing destination is tightened to `0600` before the - secret payload is written, so the JSON never lands on disk with - wider permissions. + different output channel, and a wider-than-`0600` destination is + an earlier run's summary — with its own credentials in it — left + readable to every user on the host. The summary file itself is + written the same way as the root token file: staged in a `0600` + temporary in the destination's directory, flushed, renamed into + place, and the containing directory flushed after the rename. The + JSON therefore never lands on disk with wider permissions and the + destination name never holds a partial summary. Any existing + destination is still tightened to `0600` before the replacement is + produced, since the rename publishes a fresh file and leaves the + old one readable until it does. - `--no-eab`: passed through to `init` ### Behavior diff --git a/docs/ko/cli.md b/docs/ko/cli.md index 509a37b3..b4d572de 100644 --- a/docs/ko/cli.md +++ b/docs/ko/cli.md @@ -99,6 +99,84 @@ EAB 회전을 가져와 `agent.toml`을 재렌더하므로 어느 서비스 호 상세 기준과 조건별 설명은 개요의 [/etc/hosts 매핑 설정](index.md#etchosts-매핑-설정)을 참고하세요. +## bootroot의 파일 기록 방식 + +bootroot가 만드는 파일은 두 개를 뺀 나머지 전부가 스테이징 후 이름 +변경(stage-then-rename) 방식으로 게시됩니다. 먼저 대상 파일과 같은 디렉터리에 임시 파일로 바이트를 쓰고, 그 임시 +이름 상태에서 최종 권한·소유권을 설정하고 flush까지 마친 뒤에야 대상 경로로 +rename합니다. 운영 중인 스택 주변에서 알아 두면 좋은 결과는 세 가지입니다. + +- **읽는 쪽이 잘린 파일을 보지 않습니다.** 파일을 마운트한 컨테이너, 템플릿을 + 다시 렌더링하는 사이드카, `.env`를 해석하는 `docker compose`, `agent.toml`을 + 다시 읽는 `bootroot-agent` 모두 이전 파일 아니면 완전한 새 파일만 봅니다. 도중에 + 실패한 쓰기는 이전 파일을 그대로 두고 임시 파일을 제거합니다. +- **최종 권한이 파일이 나타나는 순간부터 적용됩니다.** 갓 기록된 CA 비밀번호, + 복구 키, `secret_id`, 리스폰더 HMAC이 의도보다 넓은 권한으로 노출되는 구간이 + 없습니다. +- **rename은 새 inode를 설치합니다.** 매 쓰기마다 대상 경로의 inode가 바뀌므로, + 파일 디스크립터를 열어 둔 쪽(`tail -f`, 기동 시 파일을 연 컨테이너)은 경로를 + 다시 열기 전까지 이전 내용을 계속 읽습니다. *디렉터리* 바인드 마운트는 rename을 + 따라가지만 단일 *파일* 바인드 마운트는 따라가지 않으므로, 새 버전을 반영하려면 + 컨테이너를 재시작해야 합니다. + +아직 이 방식으로 게시되지 않는 파일이 두 개 있고, 위 세 가지 중 어느 것도 +여기에는 해당하지 않습니다. `init --save-unseal-keys`와 `bootroot openbao +save-unseal-keys`가 기록하는 `secrets/openbao/unseal-keys.txt`, 그리고 서비스별 +`secret_id` 옆에 기록되는 `eab.json`입니다. 둘 다 대상 파일을 그 자리에서 +덮어쓰고 `0600` 권한을 나중에 적용하므로, 읽는 쪽이 잘린 파일을 볼 수 있고 새로 +만들어진 파일은 잠시 그보다 넓은 권한으로 노출됩니다. 이 둘의 전환은 별도 +변경으로 다루며, 이 절의 나머지 내용은 스테이징 방식으로 기록되는 파일에만 +해당합니다. + +rename 후 상위 디렉터리를 flush할지는 파일마다 따로 정합니다. 그 flush는 매 쓰기 +마다 디스크 왕복 한 번을 쓰기 때문입니다. + +flush하는 파일 — 전원이 끊겨도 게시된 파일이 남습니다. + +- `state.json`, `.env`, `agent.toml` +- `init`의 `--summary-json`, `--root-token-output` 파일 +- step-ca CA 비밀번호와 OpenBao 복구 키 +- 모든 `AppRole` `role_id`/`secret_id`, 원격 부트스트랩 아티팩트 + +flush하지 않는 파일 — 크래시로 잃어도 장애가 아니라 재작성 비용에 그칩니다. + +- 발급된 인증서와 키, CA 번들 +- `ca.json`과 그 OpenBao Agent 템플릿 +- `openbao.hcl`, HTTP-01 리스폰더 설정과 템플릿 +- OpenBao Agent 설정, 생성된 compose 오버라이드 + +두 번째 목록은 스스로 다시 만들어집니다. 다음 갱신, OpenBao Agent 사이드카의 다음 +렌더링, 또는 해당 명령의 재실행으로 복구됩니다. 첫 번째 목록은 그렇지 않습니다. +bootroot가 재개를 위해 다시 읽는 파일이거나, 스택이 로그인에 사용하는 자격 증명 +파일이어서, 하나를 잃으면 다음 쓰기가 아니라 운영자나 다음 회전이 있어야 되돌릴 수 +있기 때문입니다. `role_id`가 옆의 `secret_id`와 함께 flush 목록에 있는 것도 같은 +이유입니다. bootroot가 OpenBao에서 다시 읽어올 수는 있지만 그 시점은 다음 `rotate` +실행이고, 그때까지 해당 에이전트나 사이드카는 로그인하지 못합니다. + +권한은 대상 파일에 이미 값이 있으면 그 값을 그대로 씁니다. 손으로 좁혀 둔 파일은 +이후 모든 쓰기에서도 좁은 채로 남습니다. bootroot가 정한 기본값은 새로 만들 때만 +적용되며, 시크릿 트리 안과 `init` 출력 두 파일은 `0600`, `state.json`, `.env`, +`ca.json`, `openbao.hcl`, 발급된 인증서, CA 번들, compose 오버라이드는 `0644` +입니다. + +이 경로 중 하나를 심볼릭 링크로 다른 위치의 파일에 연결해 두었다면 동작은 파일에 +따라 다릅니다. rename은 링크를 따라가는 대신 주어진 이름 자체를 교체하기 +때문입니다. + +- **위치를 옮겨 두었을 수 있는 설정 파일은 링크를 따라 기록합니다.** `.env`, + `ca.json`과 그 템플릿, `openbao.hcl`, HTTP-01 리스폰더 설정과 템플릿, OpenBao + Agent 설정, 생성된 compose 오버라이드, `state.json`, `init` 출력 두 파일은 + 링크를 먼저 해석해 그 대상에 게시하므로 링크는 이전에 가리키던 파일을 그대로 + 가리킵니다. 서로를 가리키며 순환하는 링크는 대상이 없으므로 거부됩니다. + `bootroot-remote bootstrap`이 대상 호스트에 기록하는 `agent.toml`도 여기에 + 속합니다. 그 호스트에서 이 파일을 만드는 것이 바로 이 명령이므로, + `--agent-config-path`에 놓인 링크는 운영자가 직접 마련한 것입니다. +- **컨트롤 노드 자신의 `agent.toml`, 발급된 인증서와 키, CA 번들, 모든 자격 + 증명은 경로 자체에 게시되며**, 그 자리에 있던 링크는 일반 파일로 대체됩니다. + 이들은 이미 여러 릴리스 전부터 rename으로 게시되어 왔으므로 해당 경로의 + 링크는 파일을 만드는 명령을 견딘 적이 없습니다. 자격 증명의 경우 링크를 따라가는 것은 링크를 심을 + 수 있는 쪽으로 쓰기가 우회된다는 뜻이기도 합니다. + ## bootroot infra up Docker Compose로 OpenBao/PostgreSQL/step-ca/HTTP-01 리스폰더를 기동하고 @@ -1107,14 +1185,25 @@ bootstrap`으로도 전달되며, 그곳에서 모든 훅이 순서대로 전달되고, `DaemonProfileSettings`에 노출됩니다 — 회전 시마다 동일한 정책이 다시 적용됩니다. -원자성: 키 파일은 stage-then-rename 방식으로 기록됩니다 — 같은 -디렉터리의 임시 파일을 `O_CREAT|O_EXCL`와 `mode=0600`으로 먼저 -생성한 뒤, (정책이 활성화된 경우) 그 임시 파일에 대해 `chown`을 -적용하고 `0640`으로 승격한 후, 마지막으로 목적지에 `rename`합니다. -따라서 목적지 경로는 최종 정책보다 넓은 모드로 노출되는 순간이 -존재하지 않습니다 — 클램프 전 umask 기반의 `0644` 윈도우도, -chown 전 운영자 기본 gid 하에서 group-readable로 잠시 노출되는 -윈도우도 존재하지 않습니다. +원자성: 키 파일, 인증서, CA 번들 모두 stage-then-rename 방식으로 +기록됩니다 — `state.json`과 `init` 출력 파일을 기록하는 것과 동일한 +게시 루틴을 통과합니다. 같은 디렉터리의 임시 파일을 `O_CREAT|O_EXCL`, +`mode=0600`으로 먼저 생성하고, (정책이 활성화된 +경우) 그 임시 파일에 `chown`을 적용한 뒤 최종 모드(정책이 활성화된 +키는 `0640`, 그렇지 않으면 `0600`, 인증서와 번들은 `0644`)로 +설정하고, 마지막으로 목적지에 `rename`합니다. 두 가지가 따라옵니다. +목적지 경로는 최종 정책보다 넓은 모드로 노출되는 순간이 존재하지 +않습니다 — 클램프 전 umask 기반의 `0644` 윈도우도, chown 전 운영자 +기본 gid 하에서 group-readable로 잠시 노출되는 윈도우도 없습니다. +그리고 회전 중에 목적지를 읽는 소비자 — 리로드되는 서버나 신뢰 +저장소를 다시 구성하는 agent — 는 이전 파일이나 완전한 새 파일 중 +하나만 보게 되며, 잘린 PEM을 보는 일은 없습니다. + +이 rename들 이후에는 상위 디렉터리를 의도적으로 flush하지 않습니다. +rename을 잃는 크래시는 이전 인증서/키/번들을 그대로 남기고 다음 +갱신이 재발급하므로, 장애가 아니라 재발급 한 번의 비용에 그칩니다. +반면 bootroot가 다시 읽어 이어서 진행하는 `state.json`과 `init` +출력 파일은 그 flush를 수행합니다. ### 대화형 동작 @@ -2150,9 +2239,21 @@ bootroot clean --openbao-only --yes 경우(예: 모드 `0400`), 또는 상위 디렉터리가 새 파일을 받아들이지 못하는 경우 reinit이 시작되지 않습니다. 따라서 잘못된 경로로 인해 OpenBao가 초기화된 후 토큰 저장이 실패하는 상황이 발생하지 않습니다. - 새 토큰 파일은 `OpenOptionsExt::mode`를 통해 처음부터 `0600` 모드로 - 생성되므로, 새로 발급된 루트 토큰이 생성과 chmod 사이에 프로세스 - umask 기본 권한으로 노출되는 일이 없습니다. init 이후 쓰기가 그래도 + 토큰 파일은 stage-then-rename 방식으로 기록됩니다: 목적지와 같은 + 디렉터리에 `0600`으로 생성된 임시 파일에 토큰을 쓰고 flush한 뒤 + 목적지로 `rename`하며, rename 이후 상위 디렉터리도 flush합니다. + 따라서 새로 발급된 루트 토큰이 프로세스 umask 기본 권한으로 + 노출되는 순간이 없고, 목적지 이름이 일부만 기록된 토큰을 가리키는 + 순간도 없으며, 공개된 파일은 전원 손실에도 살아남습니다 — reinit이 + 다시 발급해 주지 않는 자격증명의 유일한 사본이기 때문입니다. 기존 + 대상이 있으면 그 안에 남아 있는 이전 토큰을 위해 먼저 `0600`으로 + 좁힙니다. 대상이 일반 파일을 가리키는 심볼릭 링크인 경우도 계속 + 지원됩니다: 링크를 해석해 그 대상 파일에 토큰을 기록하므로, rename은 + 링크가 아니라 사전 검사가 판정한 파일을 교체합니다. 링크가 서로를 + 가리키며 순환하는 경우에는 기록할 대상 파일 자체가 없으므로 사전 + 검사가 해당 경로를 거부합니다 — 이전의 truncate 방식이 wipe 이후에 + `ELOOP`으로 실패하던 지점을, wipe 이전으로 옮긴 것입니다. init 이후 쓰기가 + 그래도 실패하면(예: 디스크 가득) 새로 발급된 토큰을 stderr에 마스킹 없이(`ROOT_TOKEN=` 접두사) 출력하여 잃어버리지 않도록 합니다. - `--enable ` / `--skip ` / `--no-eab`: @@ -2173,11 +2274,16 @@ bootroot clean --openbao-only --yes 불가능/생성 불가능한 경우 reinit이 시작되지 않습니다. summary JSON 에는 새로 발급된 루트 토큰과 unseal key가 포함되므로, 쓰기 불가능한 대상은 partial-init 트랩을 다른 출력 채널을 통해 재현하게 되고, - `0600`보다 넓은 권한을 가진 대상은 쓰기와 사후 chmod 사이에 해당 - 비밀을 디스크에 잠시 노출시킵니다. summary 파일 자체는 원자적으로 - 기록됩니다: 신규 파일은 create-mode 플래그를 통해 `0600`으로 생성 - 되고, 기존 대상은 비밀 페이로드가 쓰이기 전에 `0600`으로 좁혀지므로, - JSON이 더 넓은 권한으로 디스크에 머무는 순간이 존재하지 않습니다. + `0600`보다 넓은 권한을 가진 대상은 이전 실행이 남긴 summary — 그 + 자체로 자격증명을 담고 있는 파일 — 를 호스트의 모든 사용자에게 + 읽히도록 방치한 상태입니다. summary 파일 자체는 루트 토큰 파일과 + 동일한 방식으로 기록됩니다: 목적지와 같은 디렉터리의 `0600` 임시 + 파일에 기록하고 flush한 뒤 목적지로 `rename`하며, rename 이후 상위 + 디렉터리도 flush합니다. 따라서 JSON이 더 넓은 권한으로 디스크에 + 머무는 순간도, 목적지 이름이 일부만 기록된 summary를 가리키는 + 순간도 없습니다. 기존 대상은 여전히 교체본이 만들어지기 전에 + `0600`으로 좁힙니다 — rename은 새 파일을 공개할 뿐, 그때까지 이전 + 파일은 그대로 읽히기 때문입니다. ### 동작 diff --git a/src/bin/bootroot-remote/agent_config.rs b/src/bin/bootroot-remote/agent_config.rs index 041798ef..d8b5db1f 100644 --- a/src/bin/bootroot-remote/agent_config.rs +++ b/src/bin/bootroot-remote/agent_config.rs @@ -195,7 +195,39 @@ pub(super) async fn apply_agent_config_updates( ApplyItemSummary::failed(message), ); } - if let Err(err) = fs::write(&args.agent_config_path, &with_profile).await { + // Published by rename at `0600`, matching the control plane's + // own `agent.toml` writer (`service::local_config`). This is the + // file `bootroot-agent` re-reads on every ACME retry, and the + // truncating write here reopened the #613 window that writer was + // moved off — a reload landing in the gap sees no profile and + // burns a retry. The mode reaching the staged temporary also + // folds the separate chmod, and its own failure arm, into the + // publish. + // + // It takes the directory flush. Nothing on this host regenerates + // `agent.toml`; losing the entry leaves the agent renewing + // against a stale responder HMAC or trust anchor with no signal + // that a re-sync is needed. + // + // A symlink at the destination is resolved first and the target + // is published, unlike the control plane's writers of the same + // file name. The rule there is that `service::local_config` + // creates `agent.toml` by rename, so a link at the path never + // survives the command that creates it and the later editors + // have nothing to preserve. On a bootstrap target this writer is + // the one that creates the file, and the write it replaces + // opened with `O_TRUNC`, which follows a final link — so an + // operator who pointed the destination at a config they keep + // elsewhere has a working installation today, and a bare rename + // would destroy the link, leave its target holding the previous + // profile, and report the item applied. + if let Err(err) = fs_util::atomic_write_through_symlink( + &args.agent_config_path, + with_profile.as_bytes(), + fs_util::KEY_FILE_MODE, + ) + .await + { let message = localized( lang, &format!( @@ -215,26 +247,6 @@ pub(super) async fn apply_agent_config_updates( } return (responder_hmac_status, trust_sync_status); } - if let Err(err) = fs_util::set_key_permissions(&args.agent_config_path).await { - let message = localized( - lang, - &format!( - "agent config chmod failed ({}): {err}", - args.agent_config_path.display() - ), - &format!( - "agent.toml 권한 설정 실패 ({}): {err}", - args.agent_config_path.display() - ), - ); - if responder_changed { - responder_hmac_status = ApplyItemSummary::failed(message.clone()); - } - if trust_changed { - trust_sync_status = ApplyItemSummary::failed(message); - } - return (responder_hmac_status, trust_sync_status); - } } (responder_hmac_status, trust_sync_status) @@ -1344,4 +1356,55 @@ mod tests { "backfill must not introduce the localhost default server: {overridden}" ); } + + /// On a bootstrap target this is the writer that *creates* + /// `agent.toml`, and the truncating write it replaced opened with + /// `O_TRUNC`, which follows a final symlink. So a destination an + /// operator pointed at a config kept elsewhere is written through, + /// which is the opposite answer from the control plane's editors of + /// the same file name — and for the opposite reason: there + /// `service add` creates the file by rename, so a link at the path + /// never survived in the first place. + #[tokio::test] + async fn remote_bootstrap_writes_agent_toml_through_a_symlink() { + let dir = tempfile::tempdir().unwrap(); + let target = dir.path().join("shared-agent.toml"); + let link = dir.path().join("agent.toml"); + std::fs::write(&target, "domain = \"existing.domain\"\n").unwrap(); + std::os::unix::fs::symlink(&target, &link).unwrap(); + + let mut args = test_bootstrap_args(); + args.agent_config_path = link.clone(); + let pulled = PulledSecrets { + secret_id: "secret".to_string(), + eab_kid: None, + eab_hmac: None, + responder_hmac: "responder-hmac".to_string(), + trusted_ca_sha256: vec!["aa:bb".to_string()], + ca_bundle_pem: String::new(), + }; + + let (responder, trust) = apply_agent_config_updates(&args, &pulled, Locale::En).await; + assert!( + responder.error.is_none() && trust.error.is_none(), + "the write must succeed: {responder:?} {trust:?}" + ); + + assert!( + std::fs::symlink_metadata(&link) + .unwrap() + .file_type() + .is_symlink(), + "the operator's link must survive the publish" + ); + let published = std::fs::read_to_string(&target).unwrap(); + assert!( + published.contains("responder-hmac"), + "the link's target must hold the new profile: {published}" + ); + assert!( + published.contains("domain = \"existing.domain\""), + "and the file it edited, not a fresh one: {published}" + ); + } } diff --git a/src/bin/bootroot-remote/io.rs b/src/bin/bootroot-remote/io.rs index 80a560db..14ae91bc 100644 --- a/src/bin/bootroot-remote/io.rs +++ b/src/bin/bootroot-remote/io.rs @@ -101,8 +101,19 @@ pub(super) async fn write_secret_file(path: &Path, contents: &str) -> Result Option { /// Writes a private key file under the given policy. /// -/// The implementation is staging-then-rename: the bytes are first written -/// to a temporary file in the same directory created with `O_CREAT | -/// O_EXCL` and `mode=0600`, the staged file is `chown`d (when policy is -/// active) and promoted to `0640`, and only then is it `rename`d over -/// the destination. The destination path is therefore never observable -/// at a mode wider than the final policy: there is no window where the -/// destination exists at the umask-derived mode (typically `0644`) before -/// the clamp lands, and no window where the file is group-readable under -/// the operator's primary gid before the chown lands. This addresses the -/// atomic-write requirement called out in issue #593. +/// The implementation is staging-then-rename, through the crate's +/// shared [`fs_util::publish_staged_blocking`]: the bytes are first +/// written to a temporary file in the same directory created with +/// `O_CREAT | O_EXCL` and `mode=0600`, the staged file is `chown`d +/// (when policy is active) and promoted to `0640`, and only then is it +/// `rename`d over the destination. The destination path is therefore +/// never observable at a mode wider than the final policy: there is no +/// window where the destination exists at the umask-derived mode +/// (typically `0644`) before the clamp lands, and no window where the +/// file is group-readable under the operator's primary gid before the +/// chown lands. This addresses the atomic-write requirement called out +/// in issue #593. /// /// # Errors /// /// Returns an error if the staging write, chown, chmod, or rename fails. +/// +/// [`fs_util::publish_staged_blocking`]: crate::fs_util::publish_staged_blocking pub async fn write_key_file(path: &Path, key_pem: &str, policy: CertGroupPolicy) -> Result<()> { let dest = path.to_path_buf(); let key_owned = key_pem.to_string(); - tokio::task::spawn_blocking(move || -> Result<()> { - let parent = dest - .parent() - .ok_or_else(|| anyhow::anyhow!("Key path {} has no parent", dest.display()))?; - let file_name = dest - .file_name() - .and_then(|s| s.to_str()) - .ok_or_else(|| anyhow::anyhow!("Key path {} has no file name", dest.display()))?; - - let staged = stage_key_file(parent, file_name, &key_owned, policy)?; - // The staged file is flushed before this rename, but the - // directory holding the new entry deliberately is not flushed - // after it: a crash that loses the rename leaves the previous - // key in place, and the next renewal reissues. That costs a - // reissue, not an outage, which does not buy a disk round trip - // on every key write. Contrast `fs_util::atomic_write_blocking`, - // whose callers read their file back to resume. - std::fs::rename(&staged, &dest).map_err(|err| { - let _ = std::fs::remove_file(&staged); - anyhow::Error::new(err).context(format!( - "Failed to rename {} to {}", - staged.display(), - dest.display() - )) - })?; - Ok(()) - }) - .await - .context("write_key_file task panicked")??; - Ok(()) + let final_mode = if policy.is_active() { + KEY_FILE_MODE_GROUP + } else { + KEY_FILE_MODE_DEFAULT + }; + tokio::task::spawn_blocking(move || publish_staged(&dest, &key_owned, final_mode, policy)) + .await + .context("write_key_file task panicked")? + .with_context(|| format!("Failed to write key file {}", path.display())) } -/// Creates the key staging file at `0600` with `O_CREAT|O_EXCL`, writes -/// the key bytes, applies the policy's chown / chmod while the file is -/// still at its temporary path, and returns the staged path so the caller -/// can `rename` it over the destination. -fn stage_key_file( - parent: &Path, - final_name: &str, - key_pem: &str, - policy: CertGroupPolicy, -) -> Result { - let pid = std::process::id(); - for attempt in 0u32..32 { - let candidate = parent.join(format!(".{final_name}.tmp.{pid}.{attempt}")); - let mut opts = std::fs::OpenOptions::new(); - opts.create_new(true) - .write(true) - .mode(KEY_FILE_MODE_DEFAULT); - match opts.open(&candidate) { - Ok(mut f) => { - if let Err(err) = f.write_all(key_pem.as_bytes()) { - let _ = std::fs::remove_file(&candidate); - return Err(anyhow::Error::new(err) - .context(format!("Failed to write {}", candidate.display()))); - } - if let Err(err) = f.sync_all() { - let _ = std::fs::remove_file(&candidate); - return Err(anyhow::Error::new(err) - .context(format!("Failed to fsync {}", candidate.display()))); - } - drop(f); - if let Some(gid) = policy.gid { - if let Err(err) = std::os::unix::fs::chown(&candidate, None, Some(gid)) { - let _ = std::fs::remove_file(&candidate); - return Err(anyhow::Error::new(err).context(format!( - "Failed to chown {} to gid {gid}", - candidate.display() - ))); - } - if let Err(err) = std::fs::set_permissions( - &candidate, - std::fs::Permissions::from_mode(KEY_FILE_MODE_GROUP), - ) { - let _ = std::fs::remove_file(&candidate); - return Err(anyhow::Error::new(err) - .context(format!("Failed to chmod 0640 on {}", candidate.display()))); - } - } - return Ok(candidate); - } - Err(err) if err.kind() == std::io::ErrorKind::AlreadyExists => {} - Err(err) => { - return Err(anyhow::Error::new(err).context(format!( - "Failed to create staging file in {}", - parent.display() - ))); - } - } - } - anyhow::bail!( - "Failed to allocate a staging file for {} in {} after 32 attempts", - final_name, - parent.display() +/// Stages `contents` beside `dest`, applies the policy's ownership and +/// `mode` while the file is still at its temporary path, and `rename`s +/// it over `dest`. +/// +/// The staging itself is [`fs_util::publish_staged_blocking`], the +/// crate's general-purpose staging publisher; this is where the key, +/// the certificate and the CA bundle state the two decisions that +/// distinguish their publish from `state.json`'s. +/// +/// Ownership comes from the policy, not from whatever is at `dest` +/// ([`StagedOwner::PolicyGroup`]). The rename installs a fresh inode, +/// so a file an earlier writer left owned by another user is +/// republished owned by this one — where the truncating write the +/// certificate and the bundle used to perform kept that owner. +/// Deliberate: the gid these files need is the one `--cert-group` +/// names, and re-reading it off the destination would let a stale owner +/// outlive the policy that replaced it. All three land world-readable +/// or group-readable by that policy, so no consumer loses access to a +/// file it could read before, and the rename needs only the directory's +/// permission — a writer that could not replace the destination before +/// is not made to fail on a chown it has no privilege for. This is the +/// opposite choice from [`fs_util::atomic_write_blocking`], whose files +/// (`0600` agent config, `state.json`, the fast-poll state) have no +/// policy to restate and where a re-owned one the daemon cannot read is +/// an outage. +/// +/// The directory holding the new entry is deliberately not flushed +/// after the rename ([`StagedDurability::RenameOnly`]): a crash that +/// loses it leaves the previous key or certificate in place and the +/// next renewal reissues. That costs a reissue, not an outage, which +/// does not buy a disk round trip on every write. +/// +/// A symlink at `dest` is replaced rather than followed, which is what +/// the key writer has done since #593 and what the certificate and the +/// bundle now do beside it — the point of the conversion was to remove +/// the asymmetry between them, not to relocate it into how a link is +/// treated. The configuration writers take +/// [`fs_util::atomic_write_through_symlink`] instead, for destinations +/// no rename writer has ever owned. +/// +/// [`fs_util::atomic_write_through_symlink`]: crate::fs_util::atomic_write_through_symlink +/// +/// [`fs_util::publish_staged_blocking`]: crate::fs_util::publish_staged_blocking +/// [`fs_util::atomic_write_blocking`]: crate::fs_util::atomic_write_blocking +fn publish_staged(dest: &Path, contents: &str, mode: u32, policy: CertGroupPolicy) -> Result<()> { + fs_util::publish_staged_blocking( + dest, + contents.as_bytes(), + mode, + StagedOwner::PolicyGroup(policy.gid), + StagedDurability::RenameOnly, ) } @@ -467,20 +440,63 @@ fn stage_key_file( /// The cert mode (`0644`) is unchanged regardless of policy; only the /// group ownership is adjusted when `policy` is active. /// +/// Published the same way as the key beside it, through +/// `publish_staged`: the bytes go to a temporary file in the same +/// directory, the mode and the policy's ownership are applied there, +/// and only then is it `rename`d over the destination. A reader — +/// `bootroot-agent`, or the server being reloaded — therefore observes +/// the previous certificate or the complete new one, never a truncated +/// PEM. The containing directory is deliberately not flushed after the +/// rename; see the comment at that rename for why. +/// /// # Errors /// -/// Returns an error if the write, chown, or chmod fails. +/// Returns an error if the staging write, chown, chmod, or rename fails. pub async fn write_cert_file(path: &Path, cert_pem: &str, policy: CertGroupPolicy) -> Result<()> { - fs::write(path, cert_pem) - .await - .with_context(|| format!("Failed to write cert file {}", path.display()))?; - fs::set_permissions(path, std::fs::Permissions::from_mode(CERT_FILE_MODE)) + let dest = path.to_path_buf(); + let cert_owned = cert_pem.to_string(); + tokio::task::spawn_blocking(move || publish_staged(&dest, &cert_owned, CERT_FILE_MODE, policy)) .await - .with_context(|| format!("Failed to set 0644 on {}", path.display()))?; - if let Some(gid) = policy.gid { - chown_path(path, gid).await?; - } - Ok(()) + .context("write_cert_file task panicked")? + .with_context(|| format!("Failed to write cert file {}", path.display())) +} + +/// Writes a CA bundle file under the given policy. +/// +/// The bundle mode ([`CA_BUNDLE_FILE_MODE`], `0644`) is unchanged +/// regardless of policy; only the group ownership is adjusted when +/// `policy` is active. Because the mode is applied to the staged file +/// on every write, a bundle an earlier writer left stricter (the +/// `bootroot-remote` bootstrap path creates it at `0600`) is still +/// republished world-readable. +/// +/// Published exactly as the certificate beside it, through +/// `publish_staged`: a reader — `bootroot-agent` reloading its trust +/// store mid-rotation — observes the previous bundle or the complete +/// new one, never a truncated chain. The containing directory is +/// deliberately not flushed after the rename; a bundle lost to a crash +/// is rewritten by the next rotation, which is the same reasoning the +/// key and the certificate record. +/// +/// Callers that also need the parent directory created go through +/// [`crate::fs_util::write_ca_bundle`]. +/// +/// # Errors +/// +/// Returns an error if the staging write, chown, chmod, or rename fails. +pub async fn write_bundle_file( + path: &Path, + bundle_pem: &str, + policy: CertGroupPolicy, +) -> Result<()> { + let dest = path.to_path_buf(); + let bundle_owned = bundle_pem.to_string(); + tokio::task::spawn_blocking(move || { + publish_staged(&dest, &bundle_owned, CA_BUNDLE_FILE_MODE, policy) + }) + .await + .context("write_bundle_file task panicked")? + .with_context(|| format!("Failed to write CA bundle file {}", path.display())) } /// Ensures the directory containing the private key exists and has the @@ -739,6 +755,129 @@ mod tests { assert_eq!(std::fs::read_to_string(&key).unwrap(), "K"); } + /// The certificate is published by rename like the key beside it, + /// so a reader of the destination never sees a half-written PEM. A + /// changed inode is the observable difference from the `fs::write` + /// this replaced, which truncated the destination in place. + #[tokio::test] + async fn write_cert_file_publishes_a_new_inode() { + let dir = tempfile::tempdir().unwrap(); + let cert = dir.path().join("c.pem"); + write_cert_file(&cert, "FIRST", CertGroupPolicy::none()) + .await + .unwrap(); + let first_inode = std::fs::metadata(&cert).unwrap().ino(); + + write_cert_file(&cert, "SECOND", CertGroupPolicy::none()) + .await + .unwrap(); + + assert_eq!(std::fs::read_to_string(&cert).unwrap(), "SECOND"); + assert_ne!(std::fs::metadata(&cert).unwrap().ino(), first_inode); + let entries: Vec<_> = std::fs::read_dir(dir.path()) + .unwrap() + .map(|e| e.unwrap().file_name()) + .collect(); + assert_eq!(entries, vec![std::ffi::OsString::from("c.pem")]); + } + + /// `0644` regardless of policy, and asserted on the staged file + /// rather than left to the umask — the mode the published name + /// carries must not depend on the umask of whoever ran the rotation. + #[tokio::test] + async fn write_cert_file_uses_0644() { + let dir = tempfile::tempdir().unwrap(); + let cert = dir.path().join("c.pem"); + write_cert_file(&cert, "C", CertGroupPolicy::none()) + .await + .unwrap(); + let mode = std::fs::metadata(&cert).unwrap().permissions().mode() & 0o777; + assert_eq!(mode, CERT_FILE_MODE); + } + + /// A destination an earlier writer left at a stricter mode is + /// republished at `0644`. Staging must not turn "the mode is + /// re-asserted on every write" into "the mode is whatever the + /// previous file had". + #[tokio::test] + async fn write_cert_file_widens_a_stricter_existing_destination() { + let dir = tempfile::tempdir().unwrap(); + let cert = dir.path().join("c.pem"); + std::fs::write(&cert, "OLD").unwrap(); + std::fs::set_permissions(&cert, std::fs::Permissions::from_mode(0o600)).unwrap(); + + write_cert_file(&cert, "NEW", CertGroupPolicy::none()) + .await + .unwrap(); + + let mode = std::fs::metadata(&cert).unwrap().permissions().mode() & 0o777; + assert_eq!(mode, CERT_FILE_MODE); + } + + /// A Unix file name is bytes, not text, and a configured cert path + /// may hold any of them. The writes this replaced never looked, so + /// neither may the publish: the staged file carries a name of the + /// primitive's own choosing and the destination is only ever a + /// `rename` target, so nothing on this path needs it to be valid + /// UTF-8. + /// + /// Linux only: APFS validates file names as UTF-8 and answers + /// `EILSEQ`, so on macOS there is no such destination to write to. + #[cfg(target_os = "linux")] + #[tokio::test] + async fn write_cert_file_accepts_a_non_utf8_file_name() { + use std::os::unix::ffi::OsStrExt as _; + + let dir = tempfile::tempdir().unwrap(); + let name = std::ffi::OsStr::from_bytes(b"c\xffert.pem"); + assert!(name.to_str().is_none(), "the name must not be valid UTF-8"); + let cert = dir.path().join(name); + + write_cert_file(&cert, "PEM", CertGroupPolicy::none()) + .await + .expect("a non-UTF-8 destination is a path, not an error"); + let key = dir.path().join(std::ffi::OsStr::from_bytes(b"k\xffey.pem")); + write_key_file(&key, "KEY", CertGroupPolicy::none()) + .await + .expect("the key publishes through the same staging"); + + assert_eq!(std::fs::read_to_string(&cert).unwrap(), "PEM"); + assert_eq!(std::fs::read_to_string(&key).unwrap(), "KEY"); + let mut entries: Vec<_> = std::fs::read_dir(dir.path()) + .unwrap() + .map(|e| e.unwrap().file_name()) + .collect(); + entries.sort_unstable(); + assert_eq!( + entries, + vec![name.to_os_string(), key.file_name().unwrap().to_os_string()], + "no staged file may be left behind" + ); + } + + /// The certificate declines the directory flush for the same reason + /// the key does: a lost rename costs a reissue, not an outage. Same + /// construction as `write_key_file_does_not_flush_the_directory`, + /// including the skip where the mode does not bite. + #[tokio::test] + async fn write_cert_file_does_not_flush_the_directory() { + let dir = tempfile::tempdir().unwrap(); + let published = dir.path().join("published"); + std::fs::create_dir(&published).unwrap(); + let cert = published.join("c.pem"); + std::fs::set_permissions(&published, std::fs::Permissions::from_mode(0o300)).unwrap(); + if std::fs::File::open(&published).is_ok() { + std::fs::set_permissions(&published, std::fs::Permissions::from_mode(0o700)).unwrap(); + return; + } + + let result = write_cert_file(&cert, "C", CertGroupPolicy::none()).await; + std::fs::set_permissions(&published, std::fs::Permissions::from_mode(0o700)).unwrap(); + + result.expect("the cert writer must not open the directory"); + assert_eq!(std::fs::read_to_string(&cert).unwrap(), "C"); + } + #[tokio::test] async fn write_key_file_with_policy_uses_0640() { let Some(gid) = one_supplementary_test_gid() else { diff --git a/src/commands/ca.rs b/src/commands/ca.rs index 8120e8c3..852723f1 100644 --- a/src/commands/ca.rs +++ b/src/commands/ca.rs @@ -4,12 +4,13 @@ use std::thread::sleep; use std::time::{Duration, Instant}; use anyhow::{Context, Result}; +use bootroot::fs_util; use crate::cli::args::{CaRestartArgs, CaUpdateArgs}; use crate::commands::compose_project::ComposeIdentity; use crate::commands::infra::{collect_container_failures, collect_readiness, run_compose}; use crate::commands::init::{ - RESPONDER_TEMPLATE_DIR, STEPCA_CA_JSON_TEMPLATE_NAME, set_acme_cert_duration, + CA_JSON_FILE_MODE, RESPONDER_TEMPLATE_DIR, STEPCA_CA_JSON_TEMPLATE_NAME, set_acme_cert_duration, }; use crate::i18n::Messages; @@ -110,9 +111,7 @@ fn patch_ca_json( } let updated = serde_json::to_string_pretty(&value).context(messages.error_serialize_ca_json_failed())?; - std::fs::write(path, updated) - .with_context(|| messages.error_write_file_failed(&path.display().to_string()))?; - Ok(()) + publish_ca_json(path, &updated, messages) } fn patch_ca_json_ctmpl( @@ -135,9 +134,34 @@ fn patch_ca_json_ctmpl( let serialized = serde_json::to_string_pretty(&value).context(messages.error_serialize_ca_json_failed())?; let restored = unmask_go_template_directives(&serialized, &directives); - std::fs::write(path, restored) - .with_context(|| messages.error_write_file_failed(&path.display().to_string()))?; - Ok(()) + publish_ca_json(path, &restored, messages) +} + +/// Publishes a patched `ca.json` — or its `.ctmpl` template — by rename. +/// +/// step-ca reads `ca.json` at boot and the `OpenBao` Agent sidecar +/// re-renders it from the template on a fixed interval, so both files +/// have a reader that can arrive mid-write. Truncating in place let +/// either observe half a JSON document, which step-ca answers by +/// refusing to start; the rename leaves the previous document or the +/// complete new one. +/// +/// The mode comes off the destination. Both files are read here before +/// they are written, so one always exists, and `init` — not this patch — +/// is what decides their mode. +/// +/// The directory is not flushed. Both are derived files: `ca.json` is +/// re-rendered from `ca.json.ctmpl` by the sidecar, and the template is +/// rebuilt from `ca.json` by `init`. A crash that loses the entry leaves +/// the previous document in place and costs the next render or a re-run +/// of `bootroot ca update`, not an unrecoverable state. +fn publish_ca_json(path: &Path, contents: &str, messages: &Messages) -> Result<()> { + fs_util::atomic_replace_through_symlink_blocking( + path, + contents.as_bytes(), + fs_util::preserved_mode(path, CA_JSON_FILE_MODE), + ) + .with_context(|| messages.error_write_file_failed(&path.display().to_string())) } /// Replaces each `"{{ ... }}"` JSON-quoted Go template directive in diff --git a/src/commands/dotenv.rs b/src/commands/dotenv.rs index f1482045..e58f2e53 100644 --- a/src/commands/dotenv.rs +++ b/src/commands/dotenv.rs @@ -2,6 +2,7 @@ use std::collections::BTreeMap; use std::path::Path; use anyhow::{Context, Result}; +use bootroot::fs_util; use crate::commands::compose_project::COMPOSE_PROJECT_NAME_ENV; use crate::i18n::Messages; @@ -45,6 +46,25 @@ fn strip_quotes(value: &str) -> String { } /// Writes a `.env` file from key-value pairs. +/// +/// Published by rename through +/// [`fs_util::atomic_write_through_symlink_blocking`]. Two readers make +/// a torn `.env` costly: `docker compose` interpolates it on every +/// invocation, and bootroot itself reads it back to recover the instance +/// name and the assigned host ports. +/// +/// It takes the directory flush for that second reader. The ports and +/// the instance id here are the only record of which containers this +/// tree owns; a crash that loses the entry leaves a later run choosing +/// fresh ones and unable to find the stack it already started, which no +/// re-run of `init` repairs. +/// +/// A symlinked `.env` is resolved before staging. Compose's own +/// convention is to keep one `.env` beside the compose file, so pointing +/// it at a shared file is a thing operators do, and the truncating write +/// this replaced updated that shared file; a bare rename would replace +/// the link and leave every other consumer of the target reading stale +/// ports. pub(crate) fn write_dotenv( path: &Path, entries: &[(&str, &str)], @@ -57,11 +77,29 @@ pub(crate) fn write_dotenv( content.push_str(value); content.push('\n'); } - std::fs::write(path, content) - .with_context(|| messages.error_write_file_failed(&path.display().to_string()))?; + fs_util::atomic_write_through_symlink_blocking( + path, + content.as_bytes(), + dotenv_publish_mode(path), + ) + .with_context(|| messages.error_write_file_failed(&path.display().to_string()))?; Ok(()) } +/// Mode for a `.env` this process creates, when there is no destination +/// to read one from. +/// +/// The truncating write this replaced left the mode to the umask on a +/// create — `0644` in practice — and to the destination on a rewrite. +/// `.env` is mounted into `docker compose`'s own environment and read by +/// every later bootroot invocation, so it is not narrowed here on the +/// way past; see [`fs_util::preserved_mode`]. +const DOTENV_FILE_MODE: u32 = 0o644; + +fn dotenv_publish_mode(path: &Path) -> u32 { + fs_util::preserved_mode(path, DOTENV_FILE_MODE) +} + /// Decides which of `entries` [`load_dotenv_into_env`] would apply, /// given `is_set`, which answers whether a key already has a value in /// the target environment. @@ -115,6 +153,12 @@ pub(crate) fn load_dotenv_into_env(path: &Path, messages: &Messages) -> Result<( } /// Updates a single key in an existing `.env` file, preserving other entries. +/// +/// Publishes by rename, through a symlinked `.env`, and flushes — the +/// same three decisions as [`write_dotenv`], for the same two readers. +/// This is the hotter of the pair — a rotated `POSTGRES_PASSWORD` lands +/// here while compose may be interpolating the file — so the torn read +/// it closes is the one a running stack is most likely to hit. pub(crate) fn update_dotenv_key( path: &Path, key: &str, @@ -149,11 +193,46 @@ pub(crate) fn update_dotenv_key( output.push_str(new_value); output.push('\n'); } - std::fs::write(path, output) - .with_context(|| messages.error_write_file_failed(&path.display().to_string()))?; + fs_util::atomic_write_through_symlink_blocking( + path, + output.as_bytes(), + dotenv_publish_mode(path), + ) + .with_context(|| messages.error_write_file_failed(&path.display().to_string()))?; Ok(()) } +/// Async entry point for [`update_dotenv_key`]. +/// +/// The read, the rewrite and the two flushes all run on a blocking +/// thread rather than a runtime worker. `init`'s database password +/// rotation is the one async caller, and a Tokio worker parked on three +/// disk round trips is a worker polling nothing else — on a +/// current-thread runtime it is the only worker there is. Same pattern +/// as `StateFile::save_async`, for the same reason. +/// +/// # Errors +/// Returns an error under the same conditions as [`update_dotenv_key`], +/// or if the blocking task panics. +pub(crate) async fn update_dotenv_key_async( + path: &Path, + key: &str, + new_value: &str, + messages: &Messages, +) -> Result<()> { + // Owned once, to move into the `'static` closure. The `Messages` + // clone is a byte copy of one locale discriminant. + let (path, key, new_value, messages) = ( + path.to_path_buf(), + key.to_string(), + new_value.to_string(), + messages.clone(), + ); + tokio::task::spawn_blocking(move || update_dotenv_key(&path, &key, &new_value, &messages)) + .await + .context("dotenv update task panicked")? +} + #[cfg(test)] mod tests { use tempfile::tempdir; @@ -308,4 +387,106 @@ mod tests { assert_eq!(map.get("A").unwrap(), "1"); assert_eq!(map.get("B").unwrap(), "2"); } + + /// Both writers publish a fresh inode and leave no staged sibling, + /// so `docker compose` reading concurrently sees one whole `.env` + /// or the other. + #[test] + fn dotenv_writers_publish_by_rename() { + use std::os::unix::fs::MetadataExt; + + let dir = tempdir().unwrap(); + let path = dir.path().join(".env"); + let messages = test_messages(); + + write_dotenv(&path, &[("A", "1")], &messages).unwrap(); + let first_ino = std::fs::metadata(&path).unwrap().ino(); + + write_dotenv(&path, &[("A", "2")], &messages).unwrap(); + let rewritten_ino = std::fs::metadata(&path).unwrap().ino(); + assert_ne!(first_ino, rewritten_ino, "write_dotenv must rename"); + + update_dotenv_key(&path, "A", "3", &messages).unwrap(); + assert_ne!( + rewritten_ino, + std::fs::metadata(&path).unwrap().ino(), + "update_dotenv_key must rename" + ); + assert_eq!( + read_dotenv(&path, &messages).unwrap().get("A").unwrap(), + "3" + ); + + let strays: Vec<_> = std::fs::read_dir(dir.path()) + .unwrap() + .filter_map(|e| e.ok().map(|e| e.file_name())) + .filter(|name| name != ".env") + .collect(); + assert!( + strays.is_empty(), + "staged temporary left behind: {strays:?}" + ); + } + + /// A `.env` an operator narrowed keeps its mode across a rewrite, + /// the way the truncating write left it; only a create takes the + /// umask-equivalent `0644`. + #[test] + fn dotenv_writers_keep_an_existing_mode() { + use std::os::unix::fs::PermissionsExt; + + let dir = tempdir().unwrap(); + let path = dir.path().join(".env"); + let messages = test_messages(); + + write_dotenv(&path, &[("A", "1")], &messages).unwrap(); + assert_eq!( + std::fs::metadata(&path).unwrap().permissions().mode() & 0o777, + DOTENV_FILE_MODE, + "a create takes the stated default" + ); + + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o600)).unwrap(); + update_dotenv_key(&path, "A", "2", &messages).unwrap(); + assert_eq!( + std::fs::metadata(&path).unwrap().permissions().mode() & 0o777, + 0o600, + "a rewrite must not re-widen an operator-narrowed .env" + ); + } + + /// A `.env` an operator pointed at a shared file keeps pointing at + /// it, and that file is what both writers update — the `O_TRUNC` + /// behaviour they replaced. Renaming over the link instead would + /// leave every other consumer of the target reading the ports and + /// the instance id of a stack that no longer exists. + #[test] + fn dotenv_writers_publish_through_a_symlinked_env() { + let dir = tempdir().unwrap(); + let target = dir.path().join("shared.env"); + let link = dir.path().join(".env"); + std::fs::write(&target, "A=seed\n").unwrap(); + std::os::unix::fs::symlink(&target, &link).unwrap(); + let messages = test_messages(); + + write_dotenv(&link, &[("A", "1")], &messages).unwrap(); + assert!( + std::fs::symlink_metadata(&link) + .unwrap() + .file_type() + .is_symlink(), + "write_dotenv must not replace the operator's link" + ); + assert_eq!(std::fs::read_to_string(&target).unwrap(), "A=1\n"); + + update_dotenv_key(&link, "A", "2", &messages).unwrap(); + assert!( + std::fs::symlink_metadata(&link) + .unwrap() + .file_type() + .is_symlink(), + "update_dotenv_key must not replace the operator's link" + ); + assert_eq!(std::fs::read_to_string(&target).unwrap(), "A=2\n"); + } } diff --git a/src/commands/guardrails.rs b/src/commands/guardrails.rs index 5014b219..ed1e87f4 100644 --- a/src/commands/guardrails.rs +++ b/src/commands/guardrails.rs @@ -3,6 +3,7 @@ use std::net::IpAddr; use std::path::{Path, PathBuf}; use anyhow::{Context, Result}; +use bootroot::fs_util; use x509_parser::pem::parse_x509_pem; use crate::commands::init::{ @@ -334,6 +335,40 @@ pub(crate) fn reject_http01_admin_advertise_addr_for_specific_bind( Ok(()) } +/// Mode for a compose override this process creates, when there is no +/// destination to read one from. +/// +/// The truncating writes these replaced left a fresh create to the umask +/// (`0644`) and a rewrite to the destination. The overrides carry a bind +/// address and nothing secret, and `docker compose` reads them as the +/// invoking operator, so the umask's answer stays the default; see +/// [`fs_util::preserved_mode`]. +pub(crate) const COMPOSE_OVERRIDE_MODE: u32 = 0o644; + +/// Publishes a generated compose override by rename. +/// +/// The three exposure overrides below all have the same reader and the +/// same recovery story, so they share one publish. `docker compose` +/// parses the file as YAML on every `up`, `ps` and `down`; a truncating +/// rewrite racing one of those made it fail on a half-written mapping, +/// which for `down` means a stack that will not come down. A rename +/// leaves the previous override or the complete new one. +/// +/// The directory is deliberately **not** flushed. Each of these files is +/// regenerated verbatim from the bind address in `state.json` by the +/// command that writes it, so a crash losing the directory entry costs a +/// re-run of that command rather than anything the operator cannot +/// reconstruct — and `init` publishes enough of these that a disk round +/// trip each is worth declining. +fn publish_compose_override(path: &Path, content: &str, messages: &Messages) -> Result<()> { + fs_util::atomic_replace_through_symlink_blocking( + path, + content.as_bytes(), + fs_util::preserved_mode(path, COMPOSE_OVERRIDE_MODE), + ) + .with_context(|| messages.error_write_file_failed(&path.display().to_string())) +} + /// Generates the compose override file that exposes the HTTP-01 admin API /// on a non-loopback address. /// @@ -360,8 +395,7 @@ services: - \"{bind_addr}:8080\" " ); - fs::write(&override_path, content) - .with_context(|| messages.error_write_file_failed(&override_path.display().to_string()))?; + publish_compose_override(&override_path, &content, messages)?; Ok(override_path) } @@ -543,8 +577,7 @@ services: - \"{bind_addr}:9000\" " ); - fs::write(&override_path, content) - .with_context(|| messages.error_write_file_failed(&override_path.display().to_string()))?; + publish_compose_override(&override_path, &content, messages)?; Ok(override_path) } @@ -1140,8 +1173,7 @@ services: - \"{bind_addr}:8200\" " ); - std::fs::write(&override_path, content) - .with_context(|| messages.error_write_file_failed(&override_path.display().to_string()))?; + publish_compose_override(&override_path, &content, messages)?; Ok(override_path) } diff --git a/src/commands/init.rs b/src/commands/init.rs index 2c76b54b..6efe11ea 100644 --- a/src/commands/init.rs +++ b/src/commands/init.rs @@ -30,7 +30,7 @@ pub(crate) use steps::http01_admin_tls::{ reissue_http01_admin_tls_cert, strip_responder_tls_config, }; pub(crate) use steps::openbao_tls::{reissue_openbao_tls_cert, write_openbao_hcl_plaintext}; -pub(crate) use steps::stepca_setup::set_acme_cert_duration; +pub(crate) use steps::stepca_setup::{CA_JSON_FILE_MODE, set_acme_cert_duration}; pub(crate) use steps::{ compute_ca_bundle_pem, compute_ca_fingerprints, infra_rotate_policy, parse_ttl_to_secs, prompt_yes_no, read_ca_cert_fingerprint, run_init, validate_rotate_bound_cidrs, diff --git a/src/commands/init/steps.rs b/src/commands/init/steps.rs index d6cd6c41..75b3e74f 100644 --- a/src/commands/init/steps.rs +++ b/src/commands/init/steps.rs @@ -13,6 +13,7 @@ pub(crate) mod stepca_setup; use std::path::{Path, PathBuf}; use anyhow::{Context, Result}; +use bootroot::fs_util; use bootroot::openbao::{InitResponse, OpenBaoClient}; pub(crate) use ca_certs::{ compute_ca_bundle_pem, compute_ca_fingerprints, read_ca_cert_fingerprint, @@ -398,11 +399,36 @@ fn rollback_openbao_agent_invocation( ) } +/// Fallback mode for a file being restored that is no longer present. +/// +/// A `RollbackFile` records a destination that existed before `init`, so +/// the restore normally reads the mode back off it. `0644` covers only +/// the case where `init` deleted it outright, and is what the umask gave +/// the truncating write this replaced. +const ROLLBACK_FILE_MODE: u32 = 0o644; + +/// Restores one snapshotted file, or removes it when `init` created it. +/// +/// The restore publishes by rename. This runs on the failure path, where +/// the containers `init` started may still be up and reading the very +/// files being put back — `ca.json`, `password.txt`, the templates — so +/// a truncating restore could hand a half-written document to a service +/// that is already unhappy. It also means an interrupted rollback leaves +/// the pre-`init` file or the `init`-era one, never a shredded third +/// thing that matches neither snapshot. +/// +/// The directory is not flushed: the rollback is undoing work, so losing +/// its last entry to a crash leaves the operator exactly where a crash +/// one moment earlier would have, and a re-run of `init` is the recovery +/// either way. fn rollback_file(file: &RollbackFile, messages: &Messages) -> Result<()> { if let Some(contents) = &file.original { - std::fs::write(&file.path, contents).with_context(|| { - messages.error_restore_file_failed(&file.path.display().to_string()) - })?; + fs_util::atomic_replace_through_symlink_blocking( + &file.path, + contents.as_bytes(), + fs_util::preserved_mode(&file.path, ROLLBACK_FILE_MODE), + ) + .with_context(|| messages.error_restore_file_failed(&file.path.display().to_string()))?; } else if file.path.exists() { std::fs::remove_file(&file.path) .with_context(|| messages.error_remove_file_failed(&file.path.display().to_string()))?; @@ -964,14 +990,13 @@ mod rollback_tests { #[cfg(test)] pub(super) mod test_support { - use std::fs; - use std::os::unix::fs::PermissionsExt; use std::path::{Path, PathBuf}; use super::super::constants::openbao_constants::SECRET_ID_TTL; use super::super::constants::{DEFAULT_CERT_DURATION, DEFAULT_STEPCA_PROVISIONER}; use crate::cli::args::InitArgs; pub(in crate::commands::init::steps) use crate::i18n::test_messages; + use crate::test_support::write_executable; /// Writes a fake `docker` that appends one line per invocation to /// `args_log` and reads nothing from its environment. @@ -1007,9 +1032,7 @@ pub(super) mod test_support { let script = format!( "#!/bin/sh\nset -eu\n{{ printf '%s ' \"$@\"; printf '\\n'; }} >> '{log}'\nexit {exit_code}\n" ); - fs::write(path, script).expect("fake docker script should be written"); - fs::set_permissions(path, fs::Permissions::from_mode(0o700)) - .expect("fake docker script should be executable"); + write_executable(path, script.as_bytes()); } pub(in crate::commands::init::steps) fn default_init_args() -> InitArgs { diff --git a/src/commands/init/steps/http01_admin_tls.rs b/src/commands/init/steps/http01_admin_tls.rs index c683a14f..d2ea50a0 100644 --- a/src/commands/init/steps/http01_admin_tls.rs +++ b/src/commands/init/steps/http01_admin_tls.rs @@ -2,6 +2,7 @@ use std::os::unix::fs::{MetadataExt, PermissionsExt}; use std::path::Path; use anyhow::{Context, Result}; +use bootroot::fs_util; use super::super::constants::{ RESPONDER_CONFIG_DIR, RESPONDER_CONFIG_NAME, RESPONDER_TEMPLATE_DIR, RESPONDER_TEMPLATE_NAME, @@ -222,6 +223,17 @@ pub(crate) fn reissue_http01_admin_tls_cert( /// TLS enabled until the next `bootroot init` issues a fresh certificate. /// /// No-ops when neither file exists (fresh install before first `init`). +/// +/// Each stripped file is published by rename at the mode it already +/// carries — `0600`, set by `responder_setup` when it wrote them — which +/// the `path.exists()` guard below has established is readable. The +/// responder container reads its config at start and the `OpenBao` Agent +/// sidecar re-renders it from the template on a fixed interval, so a +/// truncating rewrite could hand either one a half-stripped file. +/// +/// The directory is not flushed: the next `bootroot init` regenerates +/// both files in full, so a crash that loses the entry costs that re-run +/// and leaves the previous config, with TLS still configured, in place. pub(crate) fn strip_responder_tls_config(secrets_dir: &Path, messages: &Messages) -> Result<()> { let configs = [ secrets_dir @@ -252,8 +264,12 @@ pub(crate) fn strip_responder_tls_config(secrets_dir: &Path, messages: &Messages } else { filtered }; - std::fs::write(path, to_write) - .with_context(|| messages.error_write_file_failed(&path.display().to_string()))?; + fs_util::atomic_replace_through_symlink_blocking( + path, + to_write.as_bytes(), + fs_util::preserved_mode(path, fs_util::KEY_FILE_MODE), + ) + .with_context(|| messages.error_write_file_failed(&path.display().to_string()))?; stripped = true; } } diff --git a/src/commands/init/steps/openbao_setup.rs b/src/commands/init/steps/openbao_setup.rs index b3fceced..1016b518 100644 --- a/src/commands/init/steps/openbao_setup.rs +++ b/src/commands/init/steps/openbao_setup.rs @@ -33,6 +33,7 @@ use crate::cli::args::InitArgs; use crate::commands::compose_project::ComposeIdentity; use crate::commands::constants::CA_TRUST_KEY; use crate::commands::container_name::BootrootContainer; +use crate::commands::guardrails::COMPOSE_OVERRIDE_MODE; use crate::commands::infra::run_compose; use crate::commands::openbao_unseal::read_unseal_keys_from_file; use crate::i18n::Messages; @@ -726,35 +727,32 @@ async fn write_openbao_agent_files( let stepca_role = find_role_output(role_outputs, AppRoleLabel::Stepca, messages)?; let responder_role = find_role_output(role_outputs, AppRoleLabel::Responder, messages)?; + // The four `AppRole` credentials below publish by rename at the + // policy's `0600`, applied to the staged temporary. The `write` + + // `set_key_permissions` pair this replaced left each `secret_id` + // world-readable at its final path for the length of a chmod, in a + // directory the `OpenBao` Agent sidecars are already watching. + // + // All four take the directory flush: `OpenBao` has issued these by + // the time they are written and a `secret_id` is not re-readable, so + // a crash that loses one is not a rewrite but an agent locked out + // until an operator re-runs `init`. This is the reason + // `fs_util::create_owned_credential_noclobber` gives for flushing + // the service-side credential. let stepca_role_id_path = stepca_dir.join(OPENBAO_AGENT_ROLE_ID_NAME); let stepca_secret_id_path = stepca_dir.join(OPENBAO_AGENT_SECRET_ID_NAME); - tokio::fs::write(&stepca_role_id_path, &stepca_role.role_id) - .await - .with_context(|| { - messages.error_write_file_failed(&stepca_role_id_path.display().to_string()) - })?; - tokio::fs::write(&stepca_secret_id_path, &stepca_role.secret_id) - .await - .with_context(|| { - messages.error_write_file_failed(&stepca_secret_id_path.display().to_string()) - })?; - fs_util::set_key_permissions(&stepca_role_id_path).await?; - fs_util::set_key_permissions(&stepca_secret_id_path).await?; + write_agent_credential(&stepca_role_id_path, &stepca_role.role_id, messages).await?; + write_agent_credential(&stepca_secret_id_path, &stepca_role.secret_id, messages).await?; let responder_role_id_path = responder_dir.join(OPENBAO_AGENT_ROLE_ID_NAME); let responder_secret_id_path = responder_dir.join(OPENBAO_AGENT_SECRET_ID_NAME); - tokio::fs::write(&responder_role_id_path, &responder_role.role_id) - .await - .with_context(|| { - messages.error_write_file_failed(&responder_role_id_path.display().to_string()) - })?; - tokio::fs::write(&responder_secret_id_path, &responder_role.secret_id) - .await - .with_context(|| { - messages.error_write_file_failed(&responder_secret_id_path.display().to_string()) - })?; - fs_util::set_key_permissions(&responder_role_id_path).await?; - fs_util::set_key_permissions(&responder_secret_id_path).await?; + write_agent_credential(&responder_role_id_path, &responder_role.role_id, messages).await?; + write_agent_credential( + &responder_secret_id_path, + &responder_role.secret_id, + messages, + ) + .await?; let stepca_agent_config = stepca_dir.join(OPENBAO_AGENT_CONFIG_NAME); let responder_agent_config = responder_dir.join(OPENBAO_AGENT_CONFIG_NAME); @@ -792,18 +790,29 @@ async fn write_openbao_agent_files( &[(responder_template, responder_output)], ca_cert, ); - tokio::fs::write(&stepca_agent_config, stepca_config) - .await - .with_context(|| { - messages.error_write_file_failed(&stepca_agent_config.display().to_string()) - })?; - tokio::fs::write(&responder_agent_config, responder_config) - .await - .with_context(|| { - messages.error_write_file_failed(&responder_agent_config.display().to_string()) - })?; - fs_util::set_key_permissions(&stepca_agent_config).await?; - fs_util::set_key_permissions(&responder_agent_config).await?; + // The two `agent.hcl` files publish by rename at `0600` and decline + // the flush: each sidecar reads its config at start and on restart, + // so a torn read is a container that will not come up, but the file + // is regenerated in full from `state.json` and the template paths on + // the next `init`. + fs_util::atomic_replace_through_symlink( + &stepca_agent_config, + stepca_config.as_bytes(), + fs_util::KEY_FILE_MODE, + ) + .await + .with_context(|| { + messages.error_write_file_failed(&stepca_agent_config.display().to_string()) + })?; + fs_util::atomic_replace_through_symlink( + &responder_agent_config, + responder_config.as_bytes(), + fs_util::KEY_FILE_MODE, + ) + .await + .with_context(|| { + messages.error_write_file_failed(&responder_agent_config.display().to_string()) + })?; Ok(OpenBaoAgentPaths { stepca_agent_config, @@ -898,12 +907,29 @@ services: secrets_path = mount_root.display(), user = user ); - tokio::fs::write(&override_path, contents) - .await - .with_context(|| messages.error_write_file_failed(&override_path.display().to_string()))?; + // Published by rename, not flushed, at the destination's mode or the + // umask's `0644` on a create — the compose-override decisions + // `crate::commands::guardrails` records. + fs_util::atomic_replace_through_symlink( + &override_path, + contents.as_bytes(), + fs_util::preserved_mode(&override_path, COMPOSE_OVERRIDE_MODE), + ) + .await + .with_context(|| messages.error_write_file_failed(&override_path.display().to_string()))?; Ok(Some(override_path)) } +/// Publishes one `OpenBao` Agent `AppRole` credential by rename at +/// `0600`, flushing the containing directory. +/// +/// See the call site for why both decisions go this way. +async fn write_agent_credential(path: &Path, value: &str, messages: &Messages) -> Result<()> { + fs_util::atomic_write(path, value.as_bytes(), fs_util::KEY_FILE_MODE) + .await + .with_context(|| messages.error_write_file_failed(&path.display().to_string())) +} + pub(super) fn apply_openbao_agent_compose_override( compose_file: &Path, override_path: &Path, diff --git a/src/commands/init/steps/openbao_tls.rs b/src/commands/init/steps/openbao_tls.rs index a46bb2d0..9a7591ce 100644 --- a/src/commands/init/steps/openbao_tls.rs +++ b/src/commands/init/steps/openbao_tls.rs @@ -2,6 +2,7 @@ use std::os::unix::fs::{MetadataExt, PermissionsExt}; use std::path::Path; use anyhow::{Context, Result}; +use bootroot::fs_util; use crate::commands::infra::run_docker_with_exec; use crate::commands::init::{ @@ -237,6 +238,37 @@ fn set_openbao_readable_permissions(cert_path: &Path, key_path: &Path) -> Result Ok(()) } +/// Fallback mode for an `openbao.hcl` with no destination to read one +/// from. +/// +/// `0644` is what the umask gave the truncating writes these replaced. +/// The file is bind-mounted into the `OpenBao` container and read by a +/// process that is not the writing operator, so it is not narrowed here; +/// see [`fs_util::preserved_mode`]. +const OPENBAO_HCL_MODE: u32 = 0o644; + +/// Publishes `openbao.hcl` by rename. +/// +/// `OpenBao` reads this file at start and on `SIGHUP`, from inside a +/// container that may already be running when the enable/revert pair +/// below rewrites it. A truncating write let that read land on a +/// half-written HCL document, which `OpenBao` answers by refusing to +/// come up — the failure the operator sees is a container restart loop +/// with a parse error, not a write error from bootroot. +/// +/// The directory is not flushed. Both callers regenerate the whole file +/// from a constant template plus the mount paths, so a crash that loses +/// the entry leaves the previous configuration in place and costs a +/// re-run of the `init` step that produced it. +fn publish_openbao_hcl(path: &Path, content: &str, messages: &Messages) -> Result<()> { + fs_util::atomic_replace_through_symlink_blocking( + path, + content.as_bytes(), + fs_util::preserved_mode(path, OPENBAO_HCL_MODE), + ) + .with_context(|| messages.error_openbao_hcl_write_failed()) +} + /// Rewrites `openbao.hcl` to enable TLS on the API listener. /// /// Replaces `tls_disable = 1` on the `:8200` listener with @@ -293,8 +325,7 @@ ui = true "#, ); - std::fs::write(&hcl_path, content) - .with_context(|| messages.error_openbao_hcl_write_failed())?; + publish_openbao_hcl(&hcl_path, &content, messages)?; println!("{}", messages.info_openbao_hcl_tls_written()); Ok(()) @@ -443,8 +474,7 @@ disable_mlock = true ui = true "#; - std::fs::write(&hcl_path, content) - .with_context(|| messages.error_openbao_hcl_write_failed())?; + publish_openbao_hcl(&hcl_path, content, messages)?; println!("{}", messages.info_openbao_hcl_tls_reverted()); Ok(()) diff --git a/src/commands/init/steps/orchestrator.rs b/src/commands/init/steps/orchestrator.rs index a1f0d045..4febef11 100644 --- a/src/commands/init/steps/orchestrator.rs +++ b/src/commands/init/steps/orchestrator.rs @@ -35,9 +35,9 @@ use super::responder_setup::{ }; use super::secrets::{maybe_register_eab, resolve_init_secrets}; use super::stepca_setup::{ - ensure_step_ca_initialized, reconcile_ca_json_dns_names, resolve_stepca_ca_dns_names, - restart_stepca_openbao_agent, snapshot_stepca_ca_json_template, update_ca_json_with_backup, - write_password_file_with_backup, write_stepca_templates, + CA_JSON_FILE_MODE, ensure_step_ca_initialized, reconcile_ca_json_dns_names, + resolve_stepca_ca_dns_names, restart_stepca_openbao_agent, snapshot_stepca_ca_json_template, + update_ca_json_with_backup, write_password_file_with_backup, write_stepca_templates, }; use crate::cli::args::{InitArgs, InitFeature}; use crate::cli::output::{print_init_plan, print_init_summary}; @@ -64,6 +64,11 @@ use crate::commands::openbao_url::{OPENBAO_HOST_PORT_ENV, effective_openbao_url_ use crate::i18n::Messages; use crate::state::StateFile; +/// Mode for the two operator-facing secret files `init` can be asked to +/// write, `--summary-json` and `--root-token-output`. Both carry root +/// credentials, so both are operator-only. +const SECRET_OUTPUT_FILE_MODE: u32 = 0o600; + /// Returns `args` with `openbao_url` rewritten to the endpoint the /// configured `OpenBao` host port publishes, borrowing them unchanged /// when the CLI value already names it. @@ -273,15 +278,34 @@ async fn diagnose_partial_init( /// write — overwriting a `0644` file leaves it world-readable while /// the secret-bearing JSON is on disk, until the subsequent chmod. /// -/// The same atomic-create discipline used by `write_root_token_file` is -/// applied here: `OpenOptionsExt::mode(0o600)` ensures new files are -/// born `0600`, and an explicit `set_permissions(0o600)` immediately -/// after the write also restricts any pre-existing destination before -/// `write_all` so the secrets never touch a wider-mode file. Reinit's -/// preflight (`validate_summary_json_output_path`) additionally -/// rejects world-/group-readable existing destinations, but this write -/// path is deliberately defensive — `--summary-json` may be invoked -/// from the `init` flow (not just `reinit`) where no preflight runs. +/// The write goes through [`fs_util::atomic_write_blocking`], which +/// stages the JSON in a temporary file in the same directory born +/// `0600`, sets the mode there, flushes it, and only then `rename`s it +/// over the destination. The secrets therefore never touch the +/// destination inode at all, so neither hazard above has a window: the +/// published file is `0600` from the instant the name points at it. +/// +/// The pre-write tightening of an existing destination is kept. It +/// guards what the rename cannot — an older summary, with older +/// credentials in it, sitting world-readable at the path right now. +/// Renaming a fresh inode over it does not narrow that file during the +/// write. `validate_summary_json_output_path` rejects such a +/// destination on both the `init` and the `reinit` path, but it runs +/// before `OpenBao` is touched: the whole of init happens between that +/// judgement and this write, and a file appearing or being widened in +/// that window is exactly what this narrows. +/// +/// The containing directory is flushed after the rename, inside +/// `atomic_write_blocking`. This file is written once during `init` +/// and read by an operator afterwards, possibly as the only record of +/// credentials that cannot be re-derived, so it is worth the disk round +/// trip that makes the published name survive a power loss. +/// +/// A symlinked destination is resolved first +/// ([`fs_util::resolve_symlink_destination`]), so the rename lands on +/// the link's target the way the truncating write's `O_TRUNC` did. +/// Renaming over the link instead would leave the operator without +/// their link and the target holding the previous run's credentials. async fn write_init_summary_json(path: &Path, summary: &InitSummary) -> Result<()> { if let Some(parent) = path.parent() && !parent.as_os_str().is_empty() @@ -290,47 +314,68 @@ async fn write_init_summary_json(path: &Path, summary: &InitSummary) -> Result<( } let payload = serde_json::to_string_pretty(summary)?; let path_buf = path.to_path_buf(); - tokio::task::spawn_blocking(move || -> std::io::Result<()> { - use std::io::Write; - use std::os::unix::fs::{OpenOptionsExt, PermissionsExt}; - // Tighten an existing destination's permissions before any - // secret content is written. No-op for missing files. The - // `OpenOptions::mode` below covers the missing-file case so - // the file is born `0600`. - if path_buf.exists() { - std::fs::set_permissions(&path_buf, std::fs::Permissions::from_mode(0o600))?; - } - let mut file = std::fs::OpenOptions::new() - .create(true) - .truncate(true) - .write(true) - .mode(0o600) - .open(&path_buf)?; - file.write_all(payload.as_bytes())?; - file.sync_all()?; - // Re-assert permissions in case an existing file's mode - // changed between the pre-write set and the open call (e.g. - // unusual filesystem semantics). - std::fs::set_permissions(&path_buf, std::fs::Permissions::from_mode(0o600))?; - Ok(()) + tokio::task::spawn_blocking(move || -> Result<()> { + let dest = fs_util::resolve_symlink_destination(&path_buf)?; + tighten_existing_secret_file(&dest)?; + fs_util::atomic_write_blocking(&dest, payload.as_bytes(), SECRET_OUTPUT_FILE_MODE) }) .await .map_err(|e| anyhow::anyhow!("spawn_blocking for summary json write failed: {e}"))??; Ok(()) } +/// Narrows an existing destination to `0600` before a secret is written +/// over it. A no-op when nothing is there, which is the usual case. +/// +/// The staged write that follows replaces the path with a fresh inode, +/// so this is not about the bytes being written — it is about the ones +/// already at the path. A summary or token file left behind by an +/// earlier run at a wider mode stays readable for as long as it takes +/// the new one to be produced, and that file holds credentials too. +fn tighten_existing_secret_file(path: &Path) -> Result<()> { + use std::os::unix::fs::PermissionsExt; + + if !path.exists() { + return Ok(()); + } + std::fs::set_permissions( + path, + std::fs::Permissions::from_mode(SECRET_OUTPUT_FILE_MODE), + ) + .with_context(|| { + format!( + "Failed to set mode {SECRET_OUTPUT_FILE_MODE:o} on the existing {}", + path.display() + ) + }) +} + /// Persists the freshly generated `OpenBao` root token to `path` with /// mode `0600`. Invoked only when the operator passes /// `bootroot reinit --root-token-output `; persistent root token /// files are not recommended for production and the surrounding code /// validates the destination path before any destructive work begins. /// -/// The file is created via `OpenOptionsExt::mode(0o600)` so a freshly -/// minted root token never exists on disk with the process umask's -/// default permissions (commonly `0644`) between creation and a -/// subsequent `chmod` call. Per-process umask still applies, so an -/// explicit `set_permissions` follows for the existing-file case where -/// `OpenOptionsExt::mode` is a no-op on POSIX. +/// Written exactly as the init summary is, through +/// [`fs_util::atomic_write_blocking`]: the token is staged in a +/// temporary file in the same directory born `0600`, moded, flushed, +/// and `rename`d over the destination. So a freshly minted root token +/// never exists on disk at the process umask's default permissions +/// (commonly `0644`), and never at the destination name in a partial +/// state. An existing destination is tightened first, for the older +/// token that may still be sitting in it — see +/// [`tighten_existing_secret_file`]. +/// +/// The containing directory is flushed after the rename, inside +/// `atomic_write_blocking`. The token is written once and read by an +/// operator afterwards; losing the published name to a power loss +/// means losing the only copy of a credential `reinit` will not mint +/// again, which is worth a disk round trip on a once-per-init write. +/// +/// A symlinked destination is resolved first, as it is for the summary +/// JSON — `validate_root_token_output_path` accepts a link to a regular +/// file on purpose, so the token has to reach the file that preflight +/// judged and not replace the link that named it. async fn write_root_token_file(path: &Path, token: &str) -> Result<()> { if let Some(parent) = path.parent() && !parent.as_os_str().is_empty() @@ -339,19 +384,10 @@ async fn write_root_token_file(path: &Path, token: &str) -> Result<()> { } let path_buf = path.to_path_buf(); let token = token.to_string(); - tokio::task::spawn_blocking(move || -> std::io::Result<()> { - use std::io::Write; - use std::os::unix::fs::{OpenOptionsExt, PermissionsExt}; - let mut file = std::fs::OpenOptions::new() - .create(true) - .truncate(true) - .write(true) - .mode(0o600) - .open(&path_buf)?; - file.write_all(token.as_bytes())?; - file.sync_all()?; - std::fs::set_permissions(&path_buf, std::fs::Permissions::from_mode(0o600))?; - Ok(()) + tokio::task::spawn_blocking(move || -> Result<()> { + let dest = fs_util::resolve_symlink_destination(&path_buf)?; + tighten_existing_secret_file(&dest)?; + fs_util::atomic_write_blocking(&dest, token.as_bytes(), SECRET_OUTPUT_FILE_MODE) }) .await .map_err(|e| anyhow::anyhow!("spawn_blocking for root token write failed: {e}"))??; @@ -823,7 +859,8 @@ async fn run_init_inner( &args.rotate_bound_cidrs, &args.secret_id_ttl, messages, - )?; + ) + .await?; // Rotate the temporary POSTGRES_PASSWORD from .env (written by // `infra install`) before building the summary so that the emitted @@ -974,7 +1011,8 @@ async fn run_init_inner( }); state.openbao_url = https_url; state - .save(&state_path) + .save_async(&state_path) + .await .with_context(|| messages.error_serialize_state_failed())?; // Phase 2 of the infra-agent bring-up: OpenBao now serves TLS, @@ -1019,7 +1057,8 @@ async fn run_init_inner( ); record_http01_admin_infra_cert(&mut state, &secrets_dir, sans, &responder_container); state - .save(&state_path) + .save_async(&state_path) + .await .with_context(|| messages.error_serialize_state_failed())?; } @@ -1139,7 +1178,7 @@ async fn maybe_rotate_env_db_password( secrets_dir: &Path, messages: &Messages, ) -> Result> { - use crate::commands::dotenv::{read_dotenv, update_dotenv_key}; + use crate::commands::dotenv::{read_dotenv, update_dotenv_key_async}; use crate::commands::init::{PATH_STEPCA_DB, PATH_STEPCA_DB_ADMIN}; // Docker Compose reads .env from the compose file's directory. @@ -1289,20 +1328,37 @@ async fn maybe_rotate_env_db_password( // restart. The OpenBao Agent template will eventually overwrite // this, but patching now avoids a window where step-ca would boot // with the old (now-invalid) password. + // + // Published by rename at the mode the file already carries, so a + // step-ca boot or an agent render landing here reads the previous + // document or the whole new one rather than half of either. The + // directory is not flushed: this patch exists only to bridge until + // the sidecar re-renders `ca.json` from its template, which is what + // recovers it if a crash loses the entry. + // + // The result stays discarded, as it was: the KV write above is what + // makes the new DSN authoritative, so a failure to pre-patch the + // rendered file is a missed optimisation, not a failed rotation. if let Ok(mut doc) = serde_json::from_str::(&ca_json_contents) { doc["db"]["dataSource"] = serde_json::Value::String(new_dsn.clone()); if let Ok(updated) = serde_json::to_string_pretty(&doc) { - let _ = tokio::fs::write(&ca_json_path, updated).await; + let mode = fs_util::preserved_mode(&ca_json_path, CA_JSON_FILE_MODE); + let _ = + fs_util::atomic_replace_through_symlink(&ca_json_path, updated.as_bytes(), mode) + .await; } } // Overwrite .env with a dummy password so docker compose doesn't error. - update_dotenv_key( + // Through the async entry point: this is the one async caller of the + // `.env` writer, and that writer now flushes. + update_dotenv_key_async( &env_path, "POSTGRES_PASSWORD", "rotated-use-openbao", messages, - )?; + ) + .await?; // Restart step-ca to pick up the new DSN from the patched ca.json. let identity = ComposeIdentity::resolve(compose_file, None, messages)?; @@ -1381,7 +1437,10 @@ async fn fix_permissions_recursive(dir: &Path) -> Result<()> { Ok(()) } -pub(super) fn write_state_file( +/// Async because the state write is: publishing `state.json` costs +/// three disk round trips, which `StateFile::save_async` keeps off the +/// runtime thread `run_init_inner` runs on. +pub(super) async fn write_state_file( openbao_url: &str, kv_mount: &str, approles: BTreeMap, @@ -1400,12 +1459,13 @@ pub(super) fn write_state_file( rotate_secret_id_ttl, messages, ) + .await } /// Inner implementation that accepts an explicit state-file path for /// testability. #[allow(clippy::too_many_arguments)] // init-time state snapshot: every value is a distinct flag -fn write_state_file_to( +async fn write_state_file_to( state_path: &Path, openbao_url: &str, kv_mount: &str, @@ -1482,7 +1542,8 @@ fn write_state_file_to( last_secret_id_rotation: existing_last_secret_id_rotation, }; state - .save(state_path) + .save_async(state_path) + .await .with_context(|| messages.error_serialize_state_failed())?; Ok(()) } @@ -2028,8 +2089,8 @@ mod tests { /// Regression: `write_state_file_to` must propagate an error when an /// existing state file is corrupted, not silently replace it with a /// fresh state (which would erase stored `openbao_bind_addr`). - #[test] - fn write_state_file_errors_on_corrupted_state() { + #[tokio::test] + async fn write_state_file_errors_on_corrupted_state() { let messages = crate::i18n::test_messages(); let dir = tempfile::tempdir().unwrap(); let state_path = dir.path().join("state.json"); @@ -2043,7 +2104,8 @@ mod tests { &[], "24h", &messages, - ); + ) + .await; assert!( result.is_err(), "corrupted state file must be a hard error, not silently replaced" @@ -2052,8 +2114,8 @@ mod tests { /// `write_state_file_to` preserves `openbao_bind_addr` from an /// existing, valid state file. - #[test] - fn write_state_file_preserves_bind_addr() { + #[tokio::test] + async fn write_state_file_preserves_bind_addr() { let messages = crate::i18n::test_messages(); let dir = tempfile::tempdir().unwrap(); let state_path = dir.path().join("state.json"); @@ -2084,6 +2146,7 @@ mod tests { "24h", &messages, ) + .await .unwrap(); let reloaded = crate::state::StateFile::load(&state_path).unwrap(); assert_eq!( @@ -2098,8 +2161,8 @@ mod tests { /// labels, the rotate roles' `secret_id` TTL (the dead-man /// threshold source), and preserves a previously recorded /// rotation-success timestamp across an init re-run. - #[test] - fn write_state_file_records_rotate_fields_and_preserves_timestamp() { + #[tokio::test] + async fn write_state_file_records_rotate_fields_and_preserves_timestamp() { let messages = crate::i18n::test_messages(); let dir = tempfile::tempdir().unwrap(); let state_path = dir.path().join("state.json"); @@ -2120,6 +2183,7 @@ mod tests { "48h", &messages, ) + .await .unwrap(); let reloaded = crate::state::StateFile::load(&state_path).unwrap(); for label in ["runtime_rotate", "infra_rotate"] { @@ -2148,6 +2212,7 @@ mod tests { "24h", &messages, ) + .await .unwrap(); let reloaded = crate::state::StateFile::load(&state_path).unwrap(); assert!( @@ -2159,8 +2224,8 @@ mod tests { /// `write_state_file_to` preserves `stepca_bind_addr` / /// `stepca_advertise_addr` from an existing, valid state file so /// that an `init` re-run does not erase the step-ca exposure intent. - #[test] - fn write_state_file_preserves_stepca_bind_intent() { + #[tokio::test] + async fn write_state_file_preserves_stepca_bind_intent() { let messages = crate::i18n::test_messages(); let dir = tempfile::tempdir().unwrap(); let state_path = dir.path().join("state.json"); @@ -2191,6 +2256,7 @@ mod tests { "24h", &messages, ) + .await .unwrap(); let reloaded = crate::state::StateFile::load(&state_path).unwrap(); assert_eq!( @@ -2480,4 +2546,379 @@ mod tests { .expect("rebuilt_admin_dsn_for_kv must succeed when KV path is absent"); assert!(rebuilt.is_none(), "absent KV path must yield None"); } + + /// The token arrives by rename from a staged temporary, so the + /// destination name never points at a partially written credential. + /// A changed inode is what separates that from the truncate-in-place + /// open this replaced. + #[tokio::test] + async fn write_root_token_file_publishes_a_new_inode_at_0600() { + use std::os::unix::fs::{MetadataExt, PermissionsExt}; + + let dir = tempfile::tempdir().expect("tempdir"); + let path = dir.path().join("root-token.txt"); + write_root_token_file(&path, "hvs.first") + .await + .expect("first token write"); + let first_inode = std::fs::metadata(&path).expect("stat").ino(); + + write_root_token_file(&path, "hvs.second") + .await + .expect("second token write"); + + assert_eq!(std::fs::read_to_string(&path).expect("read"), "hvs.second"); + assert_ne!(std::fs::metadata(&path).expect("stat").ino(), first_inode); + let mode = std::fs::metadata(&path).expect("stat").permissions().mode() & 0o777; + assert_eq!(mode, SECRET_OUTPUT_FILE_MODE); + let entries: Vec<_> = std::fs::read_dir(dir.path()) + .expect("read_dir") + .map(|e| e.expect("entry").file_name()) + .collect(); + assert_eq!( + entries, + vec![std::ffi::OsString::from("root-token.txt")], + "the staged temporary must not survive the publish" + ); + } + + /// The destination is created when the parent exists but the file + /// does not — the pre-write tightening must not trip over a missing + /// path. + #[tokio::test] + async fn write_root_token_file_creates_a_missing_destination() { + let dir = tempfile::tempdir().expect("tempdir"); + let path = dir.path().join("nested").join("root-token.txt"); + write_root_token_file(&path, "hvs.only") + .await + .expect("token write into a fresh directory"); + assert_eq!(std::fs::read_to_string(&path).expect("read"), "hvs.only"); + } + + /// A destination an earlier run left world-readable is replaced at + /// `0600`, both halves of the write pulling their weight: the + /// tightening narrows the old token sitting there, and the staged + /// publish gives the new one a fresh inode that was never wider + /// than `0600`. `init` reaches this writer with no preflight, so + /// the wide destination is not a case only `reinit` can rule out. + #[tokio::test] + async fn write_root_token_file_replaces_a_world_readable_destination() { + use std::os::unix::fs::{MetadataExt, PermissionsExt}; + + let dir = tempfile::tempdir().expect("tempdir"); + let path = dir.path().join("root-token.txt"); + std::fs::write(&path, "hvs.stale").expect("seed the destination"); + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o644)).expect("chmod"); + let stale_inode = std::fs::metadata(&path).expect("stat").ino(); + + write_root_token_file(&path, "hvs.fresh") + .await + .expect("token write over a world-readable destination"); + + assert_eq!(std::fs::read_to_string(&path).expect("read"), "hvs.fresh"); + let meta = std::fs::metadata(&path).expect("stat"); + assert_eq!(meta.permissions().mode() & 0o777, SECRET_OUTPUT_FILE_MODE); + assert_ne!(meta.ino(), stale_inode, "the publish must be a rename"); + } + + /// A symlinked destination delivers the token to the link's target, + /// which is what `validate_root_token_output_path` accepts a link to + /// a regular file *for*. Renaming over the link would leave the + /// operator without their link and the target holding the previous + /// run's token — a silent loss, since the write reports success. + #[tokio::test] + async fn write_root_token_file_writes_through_a_symlinked_destination() { + use std::os::unix::fs::PermissionsExt; + + let dir = tempfile::tempdir().expect("tempdir"); + let real_dir = dir.path().join("real"); + std::fs::create_dir(&real_dir).expect("mkdir"); + let target = real_dir.join("token.txt"); + std::fs::write(&target, "hvs.stale").expect("seed the target"); + let link = dir.path().join("root-token.txt"); + std::os::unix::fs::symlink(&target, &link).expect("symlink"); + + write_root_token_file(&link, "hvs.fresh") + .await + .expect("token write through a symlink"); + + assert!( + std::fs::symlink_metadata(&link) + .expect("stat") + .file_type() + .is_symlink(), + "the operator's link must survive the write" + ); + assert_eq!( + std::fs::read_to_string(&target).expect("read"), + "hvs.fresh", + "the token must reach the link's target" + ); + let mode = std::fs::metadata(&target) + .expect("stat") + .permissions() + .mode() + & 0o777; + assert_eq!(mode, SECRET_OUTPUT_FILE_MODE); + } + + /// A link whose target does not exist yet is still where the + /// operator wants the token: the truncating write this replaced + /// created the target through the link, so the staged write creates + /// it too. Publishing at the link's own name instead would destroy + /// the link and leave a root token in a directory nobody chose. + #[tokio::test] + async fn write_root_token_file_creates_a_dangling_symlink_target() { + use std::os::unix::fs::PermissionsExt; + + let dir = tempfile::tempdir().expect("tempdir"); + let target = dir.path().join("secure").join("token.txt"); + std::fs::create_dir(dir.path().join("secure")).expect("mkdir"); + let link = dir.path().join("root-token.txt"); + std::os::unix::fs::symlink(&target, &link).expect("symlink"); + + write_root_token_file(&link, "hvs.dangling") + .await + .expect("token write over a dangling link"); + + assert!( + std::fs::symlink_metadata(&link) + .expect("stat") + .file_type() + .is_symlink(), + "the operator's link must survive the write" + ); + assert_eq!( + std::fs::read_to_string(&target).expect("read"), + "hvs.dangling" + ); + let mode = std::fs::metadata(&target) + .expect("stat") + .permissions() + .mode() + & 0o777; + assert_eq!(mode, SECRET_OUTPUT_FILE_MODE); + } + + /// A destination whose links form a cycle has no target to deliver + /// to, and every name in the loop is a link. The truncating write + /// this replaced failed with `ELOOP`; the staged write fails too, + /// rather than renaming the token over the operator's link and + /// reporting success. + #[tokio::test] + async fn write_root_token_file_refuses_a_symlink_cycle() { + let dir = tempfile::tempdir().expect("tempdir"); + let a = dir.path().join("root-token.txt"); + let b = dir.path().join("other.txt"); + std::os::unix::fs::symlink(&b, &a).expect("symlink"); + std::os::unix::fs::symlink(&a, &b).expect("symlink"); + + let err = write_root_token_file(&a, "hvs.looped") + .await + .expect_err("a cyclic destination must not be published"); + assert!( + format!("{err:#}").contains("Too many levels of symbolic links"), + "unexpected error: {err:#}" + ); + for link in [&a, &b] { + assert!( + std::fs::symlink_metadata(link) + .expect("stat") + .file_type() + .is_symlink(), + "{} was replaced by the write", + link.display() + ); + } + } + + /// The summary JSON carries the same credentials, and refuses the + /// same destination for the same reason. + #[tokio::test] + async fn write_init_summary_json_refuses_a_symlink_cycle() { + let dir = tempfile::tempdir().expect("tempdir"); + let a = dir.path().join("summary.json"); + let b = dir.path().join("other.json"); + std::os::unix::fs::symlink(&b, &a).expect("symlink"); + std::os::unix::fs::symlink(&a, &b).expect("symlink"); + + let err = write_init_summary_json(&a, &summary_with_token("hvs.looped")) + .await + .expect_err("a cyclic destination must not be published"); + assert!( + format!("{err:#}").contains("Too many levels of symbolic links"), + "unexpected error: {err:#}" + ); + assert!( + std::fs::symlink_metadata(&a) + .expect("stat") + .file_type() + .is_symlink(), + "the operator's link was replaced by the write" + ); + } + + /// An older summary or token file left world-readable is narrowed + /// before the replacement is produced. The rename cannot do this: + /// it publishes a fresh inode and leaves the old one readable for + /// as long as it takes to write the new one. + #[test] + fn tighten_existing_secret_file_narrows_a_world_readable_destination() { + use std::os::unix::fs::PermissionsExt; + + let dir = tempfile::tempdir().expect("tempdir"); + let path = dir.path().join("summary.json"); + std::fs::write(&path, "{}").expect("seed the destination"); + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o644)).expect("chmod"); + + tighten_existing_secret_file(&path).expect("tighten"); + + let mode = std::fs::metadata(&path).expect("stat").permissions().mode() & 0o777; + assert_eq!(mode, SECRET_OUTPUT_FILE_MODE); + } + + #[test] + fn tighten_existing_secret_file_ignores_a_missing_destination() { + let dir = tempfile::tempdir().expect("tempdir"); + tighten_existing_secret_file(&dir.path().join("absent.json")) + .expect("a missing destination is not an error"); + } + + /// The other secret-bearing `init` output. It shares + /// `write_root_token_file`'s staging, tightening and symlink + /// resolution, and carries the same credentials plus the unseal + /// keys, so it is pinned in its own right rather than by + /// resemblance to the token writer. + fn summary_with_token(root_token: &str) -> InitSummary { + use super::super::super::types::{ResponderCheck, StepCaInitResult}; + + InitSummary { + openbao_url: "http://localhost:8200".to_string(), + kv_mount: "secret".to_string(), + secrets_dir: std::path::PathBuf::from("secrets"), + show_secrets: false, + init_response: true, + root_token: root_token.to_string(), + unseal_keys: vec!["unseal-one".to_string()], + approles: Vec::new(), + stepca_password: "pw".to_string(), + db_dsn: String::new(), + db_dsn_host_original: String::new(), + db_dsn_host_effective: String::new(), + http_hmac: "hmac".to_string(), + eab: None, + step_ca_result: StepCaInitResult::Skipped, + responder_check: ResponderCheck::Skipped, + responder_url: None, + responder_template_path: std::path::PathBuf::from("responder.tmpl"), + responder_config_path: std::path::PathBuf::from("responder.toml"), + openbao_agent_stepca_config_path: std::path::PathBuf::from("agent-stepca.hcl"), + openbao_agent_responder_config_path: std::path::PathBuf::from("agent-responder.hcl"), + openbao_agent_override_path: None, + db_check: DbCheckStatus::Skipped, + } + } + + /// The summary is published by rename at `0600` over a destination + /// an earlier run left world-readable: the tightening narrows the + /// old credentials sitting there, and the fresh inode the rename + /// installs was never wider than `0600`. + #[tokio::test] + async fn write_init_summary_json_replaces_a_world_readable_destination_at_0600() { + use std::os::unix::fs::{MetadataExt, PermissionsExt}; + + let dir = tempfile::tempdir().expect("tempdir"); + let path = dir.path().join("summary.json"); + std::fs::write(&path, "{\"stale\":true}").expect("seed the destination"); + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o644)).expect("chmod"); + let stale_inode = std::fs::metadata(&path).expect("stat").ino(); + + write_init_summary_json(&path, &summary_with_token("hvs.fresh")) + .await + .expect("summary write over a world-readable destination"); + + let written = std::fs::read_to_string(&path).expect("read"); + assert!(written.contains("hvs.fresh"), "got: {written}"); + let meta = std::fs::metadata(&path).expect("stat"); + assert_eq!(meta.permissions().mode() & 0o777, SECRET_OUTPUT_FILE_MODE); + assert_ne!(meta.ino(), stale_inode, "the publish must be a rename"); + let entries: Vec<_> = std::fs::read_dir(dir.path()) + .expect("read_dir") + .map(|e| e.expect("entry").file_name()) + .collect(); + assert_eq!( + entries, + vec![std::ffi::OsString::from("summary.json")], + "the staged temporary must not survive the publish" + ); + } + + /// As for the token: the summary reaches the link's target and the + /// operator's link survives. Renaming over the link would leave the + /// target holding the previous run's credentials while the write + /// reported success. + #[tokio::test] + async fn write_init_summary_json_writes_through_a_symlinked_destination() { + use std::os::unix::fs::PermissionsExt; + + let dir = tempfile::tempdir().expect("tempdir"); + let real_dir = dir.path().join("real"); + std::fs::create_dir(&real_dir).expect("mkdir"); + let target = real_dir.join("summary.json"); + std::fs::write(&target, "{\"stale\":true}").expect("seed the target"); + let link = dir.path().join("summary.json"); + std::os::unix::fs::symlink(&target, &link).expect("symlink"); + + write_init_summary_json(&link, &summary_with_token("hvs.through-link")) + .await + .expect("summary write through a symlink"); + + assert!( + std::fs::symlink_metadata(&link) + .expect("stat") + .file_type() + .is_symlink(), + "the operator's link must survive the write" + ); + let written = std::fs::read_to_string(&target).expect("read"); + assert!(written.contains("hvs.through-link"), "got: {written}"); + let mode = std::fs::metadata(&target) + .expect("stat") + .permissions() + .mode() + & 0o777; + assert_eq!(mode, SECRET_OUTPUT_FILE_MODE); + } + + /// A link whose target does not exist yet is still where the + /// operator wants the summary, exactly as for the token file. + #[tokio::test] + async fn write_init_summary_json_creates_a_dangling_symlink_target() { + use std::os::unix::fs::PermissionsExt; + + let dir = tempfile::tempdir().expect("tempdir"); + std::fs::create_dir(dir.path().join("secure")).expect("mkdir"); + let target = dir.path().join("secure").join("summary.json"); + let link = dir.path().join("summary.json"); + std::os::unix::fs::symlink(&target, &link).expect("symlink"); + + write_init_summary_json(&link, &summary_with_token("hvs.dangling")) + .await + .expect("summary write over a dangling link"); + + assert!( + std::fs::symlink_metadata(&link) + .expect("stat") + .file_type() + .is_symlink(), + "the operator's link must survive the write" + ); + let written = std::fs::read_to_string(&target).expect("read"); + assert!(written.contains("hvs.dangling"), "got: {written}"); + let mode = std::fs::metadata(&target) + .expect("stat") + .permissions() + .mode() + & 0o777; + assert_eq!(mode, SECRET_OUTPUT_FILE_MODE); + } } diff --git a/src/commands/init/steps/responder_setup.rs b/src/commands/init/steps/responder_setup.rs index 4cd80685..2b6e43ca 100644 --- a/src/commands/init/steps/responder_setup.rs +++ b/src/commands/init/steps/responder_setup.rs @@ -17,6 +17,7 @@ use super::InitSecrets; use crate::cli::args::{InitArgs, InitSkipPhase}; use crate::commands::compose_project::ComposeIdentity; use crate::commands::constants::RESPONDER_SERVICE_NAME; +use crate::commands::guardrails::COMPOSE_OVERRIDE_MODE; use crate::commands::infra::run_compose; use crate::i18n::Messages; @@ -37,19 +38,35 @@ pub(super) async fn write_responder_files( let responder_dir = secrets_dir.join(RESPONDER_CONFIG_DIR); fs_util::ensure_secrets_dir(&responder_dir).await?; + // Both files publish by rename at the policy's `0600`, applied to + // the staged temporary — the config carries the responder HMAC, so + // the `write` + `set_key_permissions` pair this replaced left it + // briefly world-readable at its final path. + // + // Neither takes the directory flush. The responder container reads + // the config at start and the `OpenBao` Agent sidecar re-renders it + // from the template, so a torn read matters; but both are rebuilt in + // full by this function on the next `init`, so a crash that loses a + // directory entry costs that re-run. let template_path = templates_dir.join(RESPONDER_TEMPLATE_NAME); let template = build_responder_template(kv_mount, tls_enabled); - tokio::fs::write(&template_path, template) - .await - .with_context(|| messages.error_write_file_failed(&template_path.display().to_string()))?; - fs_util::set_key_permissions(&template_path).await?; + fs_util::atomic_replace_through_symlink( + &template_path, + template.as_bytes(), + fs_util::KEY_FILE_MODE, + ) + .await + .with_context(|| messages.error_write_file_failed(&template_path.display().to_string()))?; let config_path = responder_dir.join(RESPONDER_CONFIG_NAME); let config = build_responder_config(hmac, tls_enabled); - tokio::fs::write(&config_path, config) - .await - .with_context(|| messages.error_write_file_failed(&config_path.display().to_string()))?; - fs_util::set_key_permissions(&config_path).await?; + fs_util::atomic_replace_through_symlink( + &config_path, + config.as_bytes(), + fs_util::KEY_FILE_MODE, + ) + .await + .with_context(|| messages.error_write_file_failed(&config_path.display().to_string()))?; Ok(ResponderPaths { template_path, @@ -159,9 +176,19 @@ services: dir = config_dir.display(), file_name = file_name, ); - tokio::fs::write(&override_path, contents) - .await - .with_context(|| messages.error_write_file_failed(&override_path.display().to_string()))?; + // Published by rename, not flushed, at the destination's mode or the + // umask's `0644` on a create — the same three decisions the + // exposure overrides in `crate::commands::guardrails` record, for + // the same reader: `docker compose` parses this file on every + // invocation, and it is regenerated from `state.json` by the `init` + // step that writes it. + fs_util::atomic_replace_through_symlink( + &override_path, + contents.as_bytes(), + fs_util::preserved_mode(&override_path, COMPOSE_OVERRIDE_MODE), + ) + .await + .with_context(|| messages.error_write_file_failed(&override_path.display().to_string()))?; Ok(Some(override_path)) } diff --git a/src/commands/init/steps/stepca_setup.rs b/src/commands/init/steps/stepca_setup.rs index 81885fb6..e7b3fab7 100644 --- a/src/commands/init/steps/stepca_setup.rs +++ b/src/commands/init/steps/stepca_setup.rs @@ -56,12 +56,22 @@ pub(super) async fn write_stepca_templates( let password_template_path = templates_dir.join(STEPCA_PASSWORD_TEMPLATE_NAME); let password_template = build_password_template(kv_mount); - tokio::fs::write(&password_template_path, password_template) - .await - .with_context(|| { - messages.error_write_file_failed(&password_template_path.display().to_string()) - })?; - fs_util::set_key_permissions(&password_template_path).await?; + // Both templates below publish by rename at `0600` and decline the + // directory flush. The `OpenBao` Agent sidecar re-reads them on a + // fixed interval and re-renders `password.txt` and `ca.json` from + // them, so a torn template is a render failure in a running + // container — but the templates are themselves regenerated in full + // by this function on the next `init`, so losing a directory entry + // to a crash costs that re-run and nothing more. + fs_util::atomic_replace_through_symlink( + &password_template_path, + password_template.as_bytes(), + fs_util::KEY_FILE_MODE, + ) + .await + .with_context(|| { + messages.error_write_file_failed(&password_template_path.display().to_string()) + })?; let ca_json_path = secrets_dir.join("config").join("ca.json"); let ca_json_contents = tokio::fs::read_to_string(&ca_json_path) @@ -76,12 +86,15 @@ pub(super) async fn write_stepca_templates( messages, )?; let ca_json_template_path = templates_dir.join(STEPCA_CA_JSON_TEMPLATE_NAME); - tokio::fs::write(&ca_json_template_path, ca_json_template) - .await - .with_context(|| { - messages.error_write_file_failed(&ca_json_template_path.display().to_string()) - })?; - fs_util::set_key_permissions(&ca_json_template_path).await?; + fs_util::atomic_replace_through_symlink( + &ca_json_template_path, + ca_json_template.as_bytes(), + fs_util::KEY_FILE_MODE, + ) + .await + .with_context(|| { + messages.error_write_file_failed(&ca_json_template_path.display().to_string()) + })?; Ok(StepCaTemplatePaths { password_template_path, @@ -89,6 +102,42 @@ pub(super) async fn write_stepca_templates( }) } +/// Fallback mode for a `ca.json` with no destination to read one from. +/// +/// Every publisher of this file reads it first, so the fallback is +/// unreachable in practice — but a staged publish has to state a mode. +/// `0644` is what the umask gave the truncating writes these replaced, +/// and is the mode `step ca init` leaves on the file it creates. Shared +/// so the three places that patch `ca.json` — here, the `init` +/// orchestrator's password rotation, and `bootroot ca update` — cannot +/// drift on it. +pub(crate) const CA_JSON_FILE_MODE: u32 = 0o644; + +/// Publishes a patched `ca.json` by rename, at the mode it already +/// carries. +/// +/// step-ca reads this file at boot and the `OpenBao` Agent sidecar +/// re-renders it from `ca.json.ctmpl` on a fixed interval, so both +/// callers below are writing a file with a live reader. Truncating in +/// place let step-ca boot against half a JSON document and refuse to +/// start; the rename leaves the previous document or the complete new +/// one. +/// +/// The directory is not flushed. `ca.json` is a rendered file — the +/// sidecar rebuilds it from the template, and `init` rebuilds the +/// template — so a crash that loses the entry costs the next render +/// rather than anything unrecoverable. This mirrors the decision +/// `crate::commands::ca`'s patcher records for the same file. +async fn publish_ca_json(path: &Path, contents: &str, messages: &Messages) -> Result<()> { + fs_util::atomic_replace_through_symlink( + path, + contents.as_bytes(), + fs_util::preserved_mode(path, CA_JSON_FILE_MODE), + ) + .await + .with_context(|| messages.error_write_file_failed(&path.display().to_string())) +} + /// Snapshots `templates/ca.json.ctmpl` for the `init` rollback. /// /// Must be called before `write_stepca_templates` regenerates the file. @@ -374,10 +423,18 @@ pub(super) async fn write_password_file_with_backup( }); } }; - tokio::fs::write(&password_path, password) + // Published by rename at the policy's `0600`, applied to the staged + // temporary so the CA password is never readable at its final path + // under a wider mode — the window the `write` + + // `set_key_permissions` pair this replaced left open. + // + // It takes the directory flush. This password decrypts the root and + // intermediate CA keys sitting beside it; a crash that loses the + // directory entry after step-ca has been handed it leaves keys + // nobody can open, which no re-run of `init` reconstructs. + fs_util::atomic_write(&password_path, password.as_bytes(), fs_util::KEY_FILE_MODE) .await .with_context(|| messages.error_write_file_failed(&password_path.display().to_string()))?; - fs_util::set_key_permissions(&password_path).await?; Ok(RollbackFile { path: password_path, original, @@ -417,9 +474,7 @@ pub(super) async fn update_ca_json_with_backup( let dns_names_changed = set_ca_json_dns_names(&mut value, dns_names); let updated = serde_json::to_string_pretty(&value).context(messages.error_serialize_ca_json_failed())?; - tokio::fs::write(&path, updated) - .await - .with_context(|| messages.error_write_file_failed(&path.display().to_string()))?; + publish_ca_json(&path, &updated, messages).await?; Ok(CaJsonUpdate { rollback: RollbackFile { path, @@ -456,9 +511,7 @@ pub(super) async fn reconcile_ca_json_dns_names( } let updated = serde_json::to_string_pretty(&value).context(messages.error_serialize_ca_json_failed())?; - tokio::fs::write(&path, updated) - .await - .with_context(|| messages.error_write_file_failed(&path.display().to_string()))?; + publish_ca_json(&path, &updated, messages).await?; Ok(true) } diff --git a/src/commands/reinit.rs b/src/commands/reinit.rs index 4a6f20e7..a152d8e5 100644 --- a/src/commands/reinit.rs +++ b/src/commands/reinit.rs @@ -189,7 +189,8 @@ pub(crate) async fn run_reinit(args: &ReinitArgs, messages: &Messages) -> Result &openbao, &effective_secrets_dir, messages, - )?; + ) + .await?; // 11. Bring OpenBao back up via the existing infra up path. let infra_args = InfraUpArgs { @@ -452,7 +453,11 @@ pub(crate) fn snapshot_deployment_intent(state_path: &Path) -> Result Result<()> { let display = path.display().to_string(); @@ -698,7 +720,14 @@ pub(crate) fn validate_summary_json_output_path(path: &Path, messages: &Messages })?; } - let parent = path + // As in `validate_root_token_output_path`: probe the directory the + // staged write will use, which for a symlinked destination is the + // target's, not the link's, and refuse a cycle here rather than + // leaving it to the post-wipe write. + let staged_in = bootroot::fs_util::resolve_symlink_destination(path).map_err(|err| { + anyhow::anyhow!(messages.error_reinit_summary_json_unwritable(&display, &err.to_string())) + })?; + let parent = staged_in .parent() .filter(|p| !p.as_os_str().is_empty()) .map_or_else(|| PathBuf::from("."), Path::to_path_buf); @@ -1178,8 +1207,8 @@ mod tests { // at all — the type itself is the test for the "drop" list. } - #[test] - fn write_minimal_state_rewrites_with_empty_registry_and_preserved_intent() { + #[tokio::test] + async fn write_minimal_state_rewrites_with_empty_registry_and_preserved_intent() { let dir = tempdir().unwrap(); let state_path = dir.path().join("state.json"); state_with_intent().save(&state_path).unwrap(); @@ -1192,7 +1221,9 @@ mod tests { let effective = PathBuf::from("secrets"); let messages = test_messages(); - write_minimal_state(&state_path, &snapshot, &openbao, &effective, &messages).unwrap(); + write_minimal_state(&state_path, &snapshot, &openbao, &effective, &messages) + .await + .unwrap(); let rewritten = StateFile::load(&state_path).unwrap(); assert!( @@ -1891,6 +1922,119 @@ mod tests { ); } + /// The probe follows the destination the staged write will use. A + /// link into a read-only directory has a perfectly writable + /// directory of its own, so probing that one would pass preflight + /// and leave the write to fail after `OpenBao` has been wiped — + /// the trap this check exists to prevent. + #[test] + fn validate_root_token_output_probes_the_link_targets_directory() { + let dir = tempdir().unwrap(); + let ro = dir.path().join("ro"); + fs::create_dir_all(&ro).unwrap(); + let target = ro.join("token"); + let link = dir.path().join("root-token.txt"); + std::os::unix::fs::symlink(&target, &link).unwrap(); + let mut perms = fs::metadata(&ro).unwrap().permissions(); + let original = perms.mode(); + perms.set_mode(0o500); + fs::set_permissions(&ro, perms).unwrap(); + + let messages = test_messages(); + let result = validate_root_token_output_path(&link, &messages); + + // Restore so tempdir cleanup succeeds. + let mut perms = fs::metadata(&ro).unwrap().permissions(); + perms.set_mode(original); + fs::set_permissions(&ro, perms).unwrap(); + + let err = result.expect_err("the target's directory cannot accept the staged file"); + assert!(err.to_string().contains("root-token-output"), "got: {err}"); + } + + /// The same for `--summary-json`, which resolves its destination the + /// same way before staging. + #[test] + fn validate_summary_json_probes_the_link_targets_directory() { + let dir = tempdir().unwrap(); + let ro = dir.path().join("ro"); + fs::create_dir_all(&ro).unwrap(); + let target = ro.join("summary.json"); + let link = dir.path().join("summary.json"); + std::os::unix::fs::symlink(&target, &link).unwrap(); + let mut perms = fs::metadata(&ro).unwrap().permissions(); + let original = perms.mode(); + perms.set_mode(0o500); + fs::set_permissions(&ro, perms).unwrap(); + + let messages = test_messages(); + let result = validate_summary_json_output_path(&link, &messages); + + // Restore so tempdir cleanup succeeds. + let mut perms = fs::metadata(&ro).unwrap().permissions(); + perms.set_mode(original); + fs::set_permissions(&ro, perms).unwrap(); + + let err = result.expect_err("the target's directory cannot accept the staged file"); + assert!(err.to_string().contains("summary-json"), "got: {err}"); + } + + /// A destination whose links form a cycle resolves to nothing the + /// write can publish without destroying a link. The preflight is + /// where that has to be said: `path.exists()` is false for a cycle + /// (the kernel answers `ELOOP`), so the checks above skip it, and + /// without this the run would reach the post-wipe write before + /// anything noticed. + #[test] + fn validate_root_token_output_rejects_a_symlink_cycle() { + let dir = tempdir().unwrap(); + let a = dir.path().join("root-token.txt"); + let b = dir.path().join("other.txt"); + std::os::unix::fs::symlink(&b, &a).unwrap(); + std::os::unix::fs::symlink(&a, &b).unwrap(); + + let messages = test_messages(); + let err = validate_root_token_output_path(&a, &messages) + .expect_err("a cyclic destination must be refused before the wipe"); + assert!(err.to_string().contains("root-token-output"), "got: {err}"); + } + + /// The same for `--summary-json`, which resolves its destination the + /// same way. + #[test] + fn validate_summary_json_rejects_a_symlink_cycle() { + let dir = tempdir().unwrap(); + let a = dir.path().join("summary.json"); + let b = dir.path().join("other.json"); + std::os::unix::fs::symlink(&b, &a).unwrap(); + std::os::unix::fs::symlink(&a, &b).unwrap(); + + let messages = test_messages(); + let err = validate_summary_json_output_path(&a, &messages) + .expect_err("a cyclic destination must be refused before the wipe"); + assert!(err.to_string().contains("summary-json"), "got: {err}"); + } + + /// A link into a directory that does not exist yet is created here, + /// so the post-wipe write does not meet a missing directory. The + /// link's own directory already exists, so only a probe that + /// follows the link can create the right one. + #[test] + fn validate_root_token_output_creates_the_link_targets_missing_directory() { + let dir = tempdir().unwrap(); + let target = dir.path().join("not-yet").join("token"); + let link = dir.path().join("root-token.txt"); + std::os::unix::fs::symlink(&target, &link).unwrap(); + + validate_root_token_output_path(&link, &test_messages()) + .expect("a link into a creatable directory passes preflight"); + + assert!( + dir.path().join("not-yet").is_dir(), + "the directory the staged write will use must exist after preflight" + ); + } + /// Regression for Round 6 reviewer item: when `--summary-json` /// points at an unwritable destination (existing file with mode /// `0400`), the preflight must catch it before the destructive @@ -1922,9 +2066,10 @@ mod tests { /// Regression for Round 7 reviewer item: the summary JSON carries /// the freshly issued root token and unseal keys, so an existing /// world-/group-readable destination must be rejected at preflight. - /// Letting `write_init_summary_json` proceed against a `0644` file - /// would briefly leave the secret payload world-readable on disk - /// between the write and the post-write chmod. + /// The staged write that replaces such a file does not make it + /// acceptable: a `0644` summary already holds credentials from an + /// earlier run, readable to every user on the host until it is + /// replaced. #[test] fn validate_summary_json_rejects_world_readable_existing_file() { let dir = tempdir().unwrap(); @@ -2955,8 +3100,8 @@ mod tests { /// The rewritten `state.json` must record the snapshotted (or /// CLI-default fallback) `secrets_dir` so a subsequent reinit on the /// same tree does not silently regress to the CLI default. - #[test] - fn write_minimal_state_preserves_snapshotted_secrets_dir() { + #[tokio::test] + async fn write_minimal_state_preserves_snapshotted_secrets_dir() { let dir = tempdir().unwrap(); let state_path = dir.path().join("state.json"); state_with_intent().save(&state_path).unwrap(); @@ -2976,6 +3121,7 @@ mod tests { Path::new("secrets-custom"), &messages, ) + .await .unwrap(); let rewritten = StateFile::load(&state_path).unwrap(); assert_eq!( diff --git a/src/commands/rotate.rs b/src/commands/rotate.rs index 8f6c4057..4887919c 100644 --- a/src/commands/rotate.rs +++ b/src/commands/rotate.rs @@ -289,6 +289,7 @@ mod test_support { use std::path::Path; pub(super) use crate::i18n::test_messages; + use crate::test_support::write_executable; /// Writes a fake `docker` at `path` that appends one record per /// invocation to `args_log` and reads nothing from its environment. @@ -319,7 +320,6 @@ mod test_support { exit_code: u8, ) { use std::os::unix::ffi::OsStrExt; - use std::os::unix::fs::PermissionsExt; // The script is assembled as bytes, not as a `String`: a Unix // path is an arbitrary NUL-free byte sequence, and rendering @@ -329,9 +329,7 @@ mod test_support { let mut script = b"#!/bin/sh\nset -eu\nprintf '%s\\0' \"$#\" \"$@\" >> ".to_vec(); script.extend_from_slice(&shell_single_quote(args_log.as_os_str().as_bytes())); script.extend_from_slice(format!("\nexit {exit_code}\n").as_bytes()); - fs::write(path, script).expect("fake docker script should be written"); - fs::set_permissions(path, fs::Permissions::from_mode(0o700)) - .expect("fake docker script should be executable"); + write_executable(path, &script); } /// Quotes `value` as a single POSIX shell word, byte for byte. diff --git a/src/commands/rotate/approle.rs b/src/commands/rotate/approle.rs index 0538eef9..7bf15483 100644 --- a/src/commands/rotate/approle.rs +++ b/src/commands/rotate/approle.rs @@ -125,7 +125,7 @@ pub(super) async fn rotate_approle_secret_id( // invocation — batch, single-service, and infra alike — and only // after the self-mint above, so a failed self-mint cannot suppress // the stale-rotation warning in `bootroot status`. - record_rotation_success(ctx, messages)?; + record_rotation_success(ctx, messages).await?; Ok(()) } @@ -133,13 +133,18 @@ pub(super) async fn rotate_approle_secret_id( /// `approle-secret-id` invocation in `state.json`. A scheduler that /// silently stops firing produces no failure log of its own, so this /// timestamp is the only signal `bootroot status` can watch. -fn record_rotation_success(ctx: &mut RotateContext, messages: &Messages) -> Result<()> { +/// +/// Async because the state write is: publishing `state.json` costs +/// three disk round trips, which `StateFile::save_async` keeps off the +/// runtime thread this rotation runs on. +async fn record_rotation_success(ctx: &mut RotateContext, messages: &Messages) -> Result<()> { let now = time::OffsetDateTime::now_utc() .format(&time::format_description::well_known::Rfc3339) .context("Failed to format the rotation-success timestamp")?; ctx.state.last_secret_id_rotation = Some(now); ctx.state - .save(&ctx.state_file) + .save_async(&ctx.state_file) + .await .with_context(|| messages.error_serialize_state_failed())?; Ok(()) } @@ -504,10 +509,20 @@ async fn ensure_infra_role_id_file( .await .with_context(|| messages.error_openbao_role_id_failed())?; fs_util::ensure_secrets_dir(agent_dir).await?; - tokio::fs::write(&role_id_path, &role_id) + // Published by rename at the policy's `0600`. The `OpenBao` Agent + // sidecar re-reads this file on every `AppRole` re-login, so a + // backfill racing one handed it a truncated `role_id` and a failed + // login; the rename leaves the previous file or the whole new one. + // + // It takes the directory flush, like the `secret_id` written beside + // it. `role_id` is not a secret and is re-readable from `OpenBao` — + // this function is what re-reads it — but only on the next `rotate` + // run, and until then a lost directory entry is a sidecar that + // cannot log in. The early return above means this writes only on a + // backfill, so the round trip is not on any repeated path. + fs_util::atomic_write(&role_id_path, role_id.as_bytes(), fs_util::KEY_FILE_MODE) .await .with_context(|| messages.error_write_file_failed(&role_id_path.display().to_string()))?; - fs_util::set_key_permissions(&role_id_path).await?; Ok(role_id) } @@ -599,7 +614,8 @@ async fn provision_infra_rotate_role( } if state_changed { ctx.state - .save(&ctx.state_file) + .save_async(&ctx.state_file) + .await .with_context(|| messages.error_serialize_state_failed())?; } @@ -698,13 +714,17 @@ async fn ensure_role_id_file( messages.error_write_file_failed(&role_id_path.display().to_string()) })?; } else { + // Inside the secrets tree, published by rename at the policy's + // `0600` and flushed — the same two decisions, for the same + // reasons, as `ensure_infra_role_id_file` above, and the same + // pair the override branch beside it makes. The early return on + // `role_id_path.exists()` means this only ever creates. fs_util::ensure_secrets_dir(service_dir).await?; - tokio::fs::write(&role_id_path, role_id) + fs_util::atomic_write(&role_id_path, role_id.as_bytes(), fs_util::KEY_FILE_MODE) .await .with_context(|| { messages.error_write_file_failed(&role_id_path.display().to_string()) })?; - fs_util::set_key_permissions(&role_id_path).await?; } Ok(()) } diff --git a/src/commands/rotate/helpers.rs b/src/commands/rotate/helpers.rs index 4a872ccc..61027cff 100644 --- a/src/commands/rotate/helpers.rs +++ b/src/commands/rotate/helpers.rs @@ -43,6 +43,26 @@ pub(super) fn ensure_file_exists(path: &Path, messages: &Messages) -> Result<()> } } +/// Writes an operator-only file inside the secrets tree, publishing it +/// by rename at `0600`. +/// +/// Its one caller stages the *new* step-ca CA password here before +/// asking step-ca to re-encrypt its keys with it. The mode is the +/// policy's `0600` rather than the destination's: this is a credential, +/// and a stale wider mode left by an earlier run must not survive the +/// file it was attached to. +/// +/// Applying that mode to the staged temporary also closes the window the +/// `write` + `set_key_permissions` pair this replaced left open, in +/// which the password sat world-readable at its final path. That is the +/// same defect #841's sibling issue tracks for `save_unseal_keys` and +/// `eab::write_key_file`; those two are left to it, but a site being +/// re-plumbed for the torn-read fix anyway does not get to keep the +/// window. +/// +/// It takes the directory flush. Losing the new password after step-ca +/// has re-encrypted its keys with it leaves an intermediate CA key +/// nobody can decrypt — not a rewrite, an unrecoverable CA. pub(super) async fn write_secret_file( path: &Path, contents: &str, @@ -51,10 +71,9 @@ pub(super) async fn write_secret_file( if let Some(parent) = path.parent() { fs_util::ensure_secrets_dir(parent).await?; } - tokio::fs::write(path, contents) + fs_util::atomic_write(path, contents.as_bytes(), fs_util::KEY_FILE_MODE) .await .with_context(|| messages.error_write_file_failed(&path.display().to_string()))?; - fs_util::set_key_permissions(path).await?; Ok(()) } diff --git a/src/commands/rotate/infra_cert.rs b/src/commands/rotate/infra_cert.rs index c47c4313..7bbb7f9b 100644 --- a/src/commands/rotate/infra_cert.rs +++ b/src/commands/rotate/infra_cert.rs @@ -126,7 +126,8 @@ pub(super) async fn rotate_infra_certs( } ctx.state - .save(&state_file) + .save_async(&state_file) + .await .with_context(|| messages.error_serialize_state_failed())?; Ok(()) diff --git a/src/commands/rotate/openbao_recovery.rs b/src/commands/rotate/openbao_recovery.rs index 0b8c5cee..f3b423b7 100644 --- a/src/commands/rotate/openbao_recovery.rs +++ b/src/commands/rotate/openbao_recovery.rs @@ -220,9 +220,22 @@ async fn write_openbao_recovery_output( let payload = serde_json::to_string_pretty(output) .with_context(|| messages.error_serialize_state_failed())?; - tokio::fs::write(path, payload) + // Published by rename at the policy's `0600`, applied to the staged + // temporary so the recovery keys are never observable at the final + // path under a wider mode. + // + // It takes the directory flush. These keys are the only way back + // into a sealed OpenBao and `OpenBao` has already rotated to them by + // the time this runs; a crash that loses the directory entry is not + // a rewrite, it is an unrecoverable barrier. + // + // A symlinked destination is resolved first, as `init`'s two output + // files resolve theirs: `--output` names a path the operator chose, + // the truncating write this replaced delivered through a link there, + // and renaming over the link would leave the keys in a directory + // nobody picked while the target kept the superseded ones. + fs_util::atomic_write_through_symlink(path, payload.as_bytes(), fs_util::KEY_FILE_MODE) .await .with_context(|| messages.error_write_file_failed(&path.display().to_string()))?; - fs_util::set_key_permissions(path).await?; Ok(()) } diff --git a/src/commands/service.rs b/src/commands/service.rs index fa340d17..99a34faa 100644 --- a/src/commands/service.rs +++ b/src/commands/service.rs @@ -8,6 +8,7 @@ mod secrets; use std::path::Path; use anyhow::{Context, Result}; +use bootroot::fs_util; use bootroot::openbao::{OpenBaoClient, SecretIdOptions}; use crate::cli::args::{ServiceAddArgs, ServiceInfoArgs, ServiceUpdateArgs}; @@ -425,6 +426,10 @@ async fn run_service_add_preview( ); } +// One line over the limit since the state persist gained its `.await`: +// the body is a linear apply sequence whose steps depend on each other, +// so splitting it would only move the ordering somewhere less visible. +#[allow(clippy::too_many_lines)] async fn run_service_add_apply( state: &mut StateFile, state_path: &Path, @@ -529,7 +534,8 @@ async fn run_service_add_apply( .services .insert(resolved.service_name.clone(), entry.clone()); state - .save(state_path) + .save_async(state_path) + .await .with_context(|| messages.error_serialize_state_failed())?; // The entry is persisted, so `service remove --delete-artifacts` can // now reach the relocated files; keep them. @@ -1113,8 +1119,39 @@ fn rerender_local_managed_profile(entry: &ServiceEntry) -> Result<()> { } else { next }; - std::fs::write(agent_config_path, next) - .with_context(|| format!("Failed to write {}", agent_config_path.display()))?; + // Published by rename, like the `service add` writer this edits + // behind (`service::local_config`). `agent.toml` is the file + // `bootroot-agent`'s daemon loop re-reads on every ACME retry, and a + // truncating rewrite here reopened exactly the #613 window that + // writer was moved off: a reload landing in the gap sees no profile + // and burns a retry. + // + // The mode comes off the destination, which this function has + // already established exists. A rename installs a fresh inode, so + // stating a constant would re-widen or re-narrow a file the operator + // may have adjusted; `service add` remains the one place that sets + // the `0600` policy mode, and this edit carries whatever is there. + // + // It takes the directory flush. `agent.toml` is not regenerated on a + // timer by anything — losing the entry costs a `service add` re-run + // by an operator who has no signal that it is needed, because the + // agent goes on reading the previous file and renewing against the + // old `cert_group_gid`. + // + // The rename lands on this path, not through a symlink at it — + // `atomic_write_blocking`, not the `_through_symlink` spelling the + // configuration writers take. `service::local_config` has published + // `agent.toml` by rename since #613, so a link an operator puts here + // is already replaced by the `service add` that creates the file; + // resolving it in the writer that only *edits* the file would make + // the two disagree about the same path rather than preserve anything + // that survives a `service add`. + fs_util::atomic_write_blocking( + agent_config_path, + next.as_bytes(), + fs_util::preserved_mode(agent_config_path, fs_util::KEY_FILE_MODE), + ) + .with_context(|| format!("Failed to write {}", agent_config_path.display()))?; Ok(()) } @@ -1231,7 +1268,7 @@ mod tests { OverrideCredentialRollback, ServiceAppRoleMaterialized, build_secret_id_options, build_service_entry, build_service_entry_from_role, display_policy_value, display_wrap_ttl, is_idempotent_remote_rerun, is_policy_only_mismatch, non_policy_fields_match, - policy_fields_match, write_origin_credential_files, + policy_fields_match, rerender_local_managed_profile, write_origin_credential_files, }; use crate::i18n::{Messages, test_messages}; use crate::state::{DeliveryMode, ServiceEntry, ServiceRoleEntry}; @@ -1372,6 +1409,39 @@ mod tests { } } + /// `agent.toml` is published at its own name by every writer that + /// touches it — `service add` has renamed over it since #613 — so + /// this edit does the same rather than resolving a link an operator + /// planted. Pinned because the opposite is a defensible-looking + /// change: it would leave two writers of one file disagreeing about + /// whether a link at that path survives. + #[test] + fn rerender_publishes_agent_toml_at_its_own_name() { + let dir = tempdir().unwrap(); + let target = dir.path().join("real-agent.toml"); + let link = dir.path().join("agent.toml"); + std::fs::write(&target, "# seed\n").unwrap(); + std::os::unix::fs::symlink(&target, &link).unwrap(); + + let mut entry = sample_entry_from_resolved(&sample_resolved()); + entry.agent_config_path = link.clone(); + rerender_local_managed_profile(&entry).unwrap(); + + assert!( + !std::fs::symlink_metadata(&link) + .unwrap() + .file_type() + .is_symlink(), + "the rename publishes at the name, as `service add` does" + ); + assert!(std::fs::read_to_string(&link).unwrap().contains("test-svc")); + assert_eq!( + std::fs::read_to_string(&target).unwrap(), + "# seed\n", + "and leaves what the link pointed at untouched" + ); + } + fn assert_common_fields(entry: &ServiceEntry, resolved: &ResolvedServiceAdd) { assert_eq!(entry.service_name, resolved.service_name); assert_eq!(entry.delivery_mode, resolved.delivery_mode); diff --git a/src/commands/service/approle.rs b/src/commands/service/approle.rs index d2d29707..1b78d22a 100644 --- a/src/commands/service/approle.rs +++ b/src/commands/service/approle.rs @@ -4,7 +4,6 @@ use anyhow::{Context, Result}; use bootroot::fs_util; use bootroot::openbao::{OpenBaoClient, SecretIdOptions}; use bootroot::trust_bootstrap::SERVICE_REISSUE_KV_SUFFIX; -use tokio::fs; use super::{SERVICE_ROLE_PREFIX, ServiceAppRoleMaterialized}; use crate::commands::constants::SERVICE_KV_BASE; @@ -130,7 +129,7 @@ pub(super) async fn write_role_id_file( /// sibling `role_id`) to `path`. /// /// For the default secrets-tree location bootroot owns the directory: -/// it is created `0700`, and the file is plainly (over)written `0600`, +/// it is created `0700`, and the file is published by rename at `0600`, /// replacing any stale file left by a previously removed service. For an /// operator `--secret-id-path` override the directory is agent-owned and /// sits outside the secrets tree, so the write goes through the hardened @@ -149,12 +148,23 @@ async fn write_service_credential_file( .await .with_context(|| messages.error_write_file_failed(&path.display().to_string()))?; } else { + // Inside the root-owned secrets tree, published by rename at the + // policy's `0600` — the same mode the `write` + + // `set_key_permissions` pair this replaced ended at, now applied + // to the staged temporary so it holds from the moment the + // credential appears at its path. + // + // It takes the directory flush, for the reason + // `fs_util::create_owned_credential_noclobber` states for the + // override path beside it: `OpenBao` has already issued this + // `secret_id` by the time it is written, and losing the + // directory entry locks the agent out until an operator + // intervenes rather than costing a rewrite. let parent = path.parent().unwrap_or(Path::new(".")); fs_util::ensure_secrets_dir(parent).await?; - fs::write(path, contents) + fs_util::atomic_write(path, contents.as_bytes(), fs_util::KEY_FILE_MODE) .await .with_context(|| messages.error_write_file_failed(&path.display().to_string()))?; - fs_util::set_key_permissions(path).await?; } Ok(()) } diff --git a/src/commands/service/remote_bootstrap.rs b/src/commands/service/remote_bootstrap.rs index 3fab8ae8..5dc29569 100644 --- a/src/commands/service/remote_bootstrap.rs +++ b/src/commands/service/remote_bootstrap.rs @@ -2,7 +2,6 @@ use std::path::Path; use anyhow::{Context, Result}; use bootroot::fs_util; -use tokio::fs; use super::resolve::ResolvedServiceAdd; use super::{ @@ -270,10 +269,19 @@ async fn write_remote_bootstrap_artifact_file( let artifact_path = artifact_dir.join(REMOTE_BOOTSTRAP_FILENAME); let payload = serde_json::to_string_pretty(artifact) .with_context(|| "Failed to serialize remote bootstrap artifact".to_string())?; - fs::write(&artifact_path, payload) + // Published by rename at the policy's `0600`, applied while the file + // is still at its temporary name so the wrapped token it may carry + // is never readable at the final path under a wider mode. + // + // It takes the directory flush. The artifact holds a single-use + // response-wrapping token that `OpenBao` has already issued and that + // expires on its own clock; losing the directory entry means the + // operator cannot run the bootstrap and cannot get that token back + // either, so `service add --remote` has to be re-run against a + // freshly issued one. + fs_util::atomic_write(&artifact_path, payload.as_bytes(), fs_util::KEY_FILE_MODE) .await .with_context(|| messages.error_write_file_failed(&artifact_path.display().to_string()))?; - fs_util::set_key_permissions(&artifact_path).await?; let remote_run_command = render_remote_run_command(artifact); Ok(RemoteBootstrapResult { bootstrap_file: artifact_path.display().to_string(), diff --git a/src/commands/service/remove.rs b/src/commands/service/remove.rs index 90a45198..b78a9881 100644 --- a/src/commands/service/remove.rs +++ b/src/commands/service/remove.rs @@ -2,6 +2,7 @@ use std::io::IsTerminal; use std::path::{Path, PathBuf}; use anyhow::{Context, Result}; +use bootroot::fs_util; use bootroot::openbao::OpenBaoClient; use bootroot::trust_bootstrap::remove_managed_service_profile; @@ -182,7 +183,8 @@ pub(crate) async fn run_service_remove( &args.service_name, |post_removal| reconcile_dns_aliases(post_removal, &identity, messages), messages, - )?; + ) + .await?; println!("{}", messages.service_remove_success(&args.service_name)); Ok(()) @@ -220,7 +222,11 @@ fn require_service_entry( /// stored role/policy names — is left untouched, so a re-run of /// `service remove` still finds the service and can retry rather than /// failing with `error_service_not_found`. -fn finalize_removal( +/// +/// Async because the persist is: publishing `state.json` costs three +/// disk round trips, which `StateFile::save_async` keeps off the +/// runtime thread `run_service_remove` runs on. +async fn finalize_removal( state: &mut StateFile, state_path: &Path, service_name: &str, @@ -230,7 +236,8 @@ fn finalize_removal( state.services.remove(service_name); reconcile(state)?; state - .save(state_path) + .save_async(state_path) + .await .with_context(|| messages.error_serialize_state_failed()) } @@ -425,7 +432,21 @@ fn strip_managed_profile(path: &Path, service_name: &str) -> Result { if next == current { return Ok(false); } - std::fs::write(path, next).with_context(|| format!("Failed to write {}", path.display()))?; + // Published by rename at the destination's own mode, for the same + // reason as `service::rerender_local_managed_profile`: the file is + // `agent.toml`, the early return above has established it exists, + // and the agent may be re-reading it as this runs. It takes the + // directory flush — losing the strip leaves the agent renewing a + // profile the operator removed, and nothing rewrites the file again + // on its own. A symlink at the path is not resolved either, for the + // reason recorded there: `agent.toml`'s creating writer has renamed + // over the name since #613. + fs_util::atomic_write_blocking( + path, + next.as_bytes(), + fs_util::preserved_mode(path, fs_util::KEY_FILE_MODE), + ) + .with_context(|| format!("Failed to write {}", path.display()))?; Ok(true) } @@ -517,8 +538,8 @@ mod tests { assert_eq!(entry.service_name, "svc"); } - #[test] - fn finalize_removal_persists_entry_removal_after_reconcile_succeeds() { + #[tokio::test] + async fn finalize_removal_persists_entry_removal_after_reconcile_succeeds() { let dir = tempdir().expect("tempdir"); let messages = test_messages(); let state_path = dir.path().join("state.json"); @@ -546,6 +567,7 @@ mod tests { }, &messages, ) + .await .expect("remove"); assert!(!state.services.contains_key("svc")); @@ -560,8 +582,8 @@ mod tests { ); } - #[test] - fn finalize_removal_keeps_on_disk_entry_when_reconcile_fails() { + #[tokio::test] + async fn finalize_removal_keeps_on_disk_entry_when_reconcile_fails() { let dir = tempdir().expect("tempdir"); let messages = test_messages(); let state_path = dir.path().join("state.json"); @@ -579,6 +601,7 @@ mod tests { |_| anyhow::bail!("responder detached"), &messages, ) + .await .expect_err("reconcile failure must propagate"); assert_eq!(err.to_string(), "responder detached"); diff --git a/src/daemon.rs b/src/daemon.rs index d0416687..f8665b3e 100644 --- a/src/daemon.rs +++ b/src/daemon.rs @@ -381,18 +381,28 @@ async fn issue_with_retry( /// target profile. Falls back to the supplied in-memory pair when the /// reload fails or the profile is absent from the reloaded file. /// -/// The fallback path exists because some `agent.toml` writers (the -/// remote bootstrap/fast-poll appliers, operator edits) still rewrite -/// the file non-atomically (truncate-then-write), so a concurrent -/// reader can observe a partial file or one that does not yet contain -/// the named profile. `apply_local_service_configs` writes through -/// [`crate::fs_util::atomic_write`] and does not contribute to the -/// race, but until every writer is hardened the consumer-side -/// fallback is the load-bearing guarantee. Treating those races as -/// transient and reusing the prior in-memory profile keeps the retry -/// budget available for genuine ACME failures, while still honouring -/// `#303`'s intent of picking up freshly-rendered KV values whenever -/// the reload does land on a coherent file. +/// The fallback no longer stands in for an unhardened `agent.toml` +/// writer. Every writer this crate controls now publishes the file by +/// rename from a temporary — `service::local_config` and the +/// `service update` and `service remove --strip-config` editors beside +/// it, `bootroot-remote bootstrap`'s apply, `apply_local_service_configs` +/// through [`crate::fs_util::atomic_write`], and the three `fast_poll` +/// appliers — so none of them can leave a partial file for this reload +/// to read. +/// +/// Two cases remain, and neither is a bootroot writer losing a race. +/// An operator editing the file in place with a truncating editor is +/// still observable half-written, and bootroot has no say in that. And +/// the profile can be genuinely absent rather than momentarily +/// unobservable, since `service remove --strip-config` deletes the +/// managed block outright; there the fallback keeps the in-flight +/// attempt running on the profile it started with rather than failing +/// on a configuration change made mid-attempt. +/// +/// Treating both as transient and reusing the prior in-memory profile +/// keeps the retry budget available for genuine ACME failures, while +/// still honouring `#303`'s intent of picking up freshly-rendered KV +/// values whenever the reload does land on a coherent file. fn reload_profile_or_fallback( config_path: &Path, overrides: &config::CliOverrides, diff --git a/src/fs_util.rs b/src/fs_util.rs index b932fb9a..7f392e6e 100644 --- a/src/fs_util.rs +++ b/src/fs_util.rs @@ -5,7 +5,7 @@ use std::path::{Component, Path, PathBuf}; use anyhow::{Context, Result}; use tokio::fs; -use crate::cert_group::{self, CA_BUNDLE_FILE_MODE, CertGroupPolicy}; +use crate::cert_group::{self, CertGroupPolicy}; pub const KEY_FILE_MODE: u32 = 0o600; const SECRETS_DIR_MODE: u32 = 0o700; @@ -56,6 +56,92 @@ fn parent_dir(path: &Path) -> PathBuf { .map_or_else(|| PathBuf::from("."), Path::to_path_buf) } +/// Resolves the file a staged write should land on when `path` is a +/// symlink, returning `path` unchanged when it is not. +/// +/// A truncating write opens the path with `O_TRUNC`, which follows the +/// final symlink, so a destination pointed at a link has always +/// delivered its bytes to the link's target. [`atomic_write`] and +/// [`atomic_write_blocking`] `rename` over the name they are given +/// instead, which replaces the link itself: the operator's link is +/// gone and the target is left holding whatever was written last. +/// +/// Writers reach this through +/// [`atomic_write_through_symlink`]/[`atomic_replace_through_symlink`] +/// and their blocking halves, which document what takes that spelling +/// and what deliberately keeps the bare rename. It is called directly +/// only where the resolved path itself is needed — `bootroot reinit`'s +/// two output preflights, which judge the target's mode before the +/// destructive step, and the two writers behind them, which narrow that +/// same target before writing a credential over it. +/// +/// Not a security check. It follows whatever the link points at, so a +/// caller whose destination an untrusted user can plant must reject +/// the symlink rather than resolve it (see +/// [`atomic_rewrite_owned_no_symlink`], which does). +/// +/// A dangling link is resolved from its own text rather than from the +/// filesystem, so the write still lands where the operator pointed it. +/// The truncating write's `O_CREAT` created the target through the +/// link; publishing at the link's own name instead would destroy the +/// link and put the file in a directory nobody chose, which for the +/// root token means a credential landing outside the place the +/// operator set aside for it. +/// +/// # Errors +/// Returns an error if the chain does not end within `SYMLOOP_MAX` +/// hops — a cycle, which the truncating write reported as `ELOOP` and +/// which is reported here rather than resolved, since every path in a +/// loop is a link the caller would destroy by renaming over it — or if +/// a link in the chain cannot be read, which leaves the destination +/// unknown for the same reason. +pub fn resolve_symlink_destination(path: &Path) -> Result { + /// Hops allowed before a chain of dangling links is treated as a + /// cycle, matching the kernel's own `SYMLOOP_MAX`. + const MAX_HOPS: u32 = 40; + + fn is_symlink(path: &Path) -> bool { + std::fs::symlink_metadata(path).is_ok_and(|meta| meta.file_type().is_symlink()) + } + + if !is_symlink(path) { + return Ok(path.to_path_buf()); + } + // Resolves the whole chain, and normalises the parent components + // with it, whenever the target exists. + if let Ok(resolved) = std::fs::canonicalize(path) { + return Ok(resolved); + } + let mut current = path.to_path_buf(); + for _ in 0..MAX_HOPS { + let target = std::fs::read_link(¤t).with_context(|| { + format!( + "Failed to read the symlink {} while resolving the destination {}", + current.display(), + path.display() + ) + })?; + current = if target.is_absolute() { + target + } else { + parent_dir(¤t).join(target) + }; + if !is_symlink(¤t) { + return Ok(current); + } + } + // A cycle: every name in it is a link, so there is nothing to + // publish that does not destroy one. The truncating write this + // replaces answered ELOOP here, and the caller keeps that answer — + // returning the path the caller named would have the rename + // silently replace the operator's link with a regular file. + anyhow::bail!( + "Too many levels of symbolic links resolving the destination {}: \ + followed {MAX_HOPS} links without reaching a file", + path.display() + ) +} + /// Flushes the directory holding `path` — its entry list, not the files /// behind it — so the name just published there survives a crash. /// @@ -210,9 +296,6 @@ pub async fn atomic_rewrite_owned_no_symlink( tmp.as_file_mut() .write_all(&payload) .with_context(|| format!("Failed to write temp file for {}", dest.display()))?; - tmp.as_file_mut() - .sync_all() - .with_context(|| format!("Failed to fsync temp file for {}", dest.display()))?; std::fs::set_permissions(tmp.path(), std::fs::Permissions::from_mode(mode)).with_context( || { format!( @@ -227,6 +310,15 @@ pub async fn atomic_rewrite_owned_no_symlink( dest.display() ) })?; + // Flushed after the mode and the ownership, not before them: an + // `fsync` persists the inode as it stands, so a flush taken at + // the bytes leaves a crash able to recover this credential + // world-readable or owned by the wrong uid. See + // `publish_staged_blocking`, which orders the same three steps + // for the same reason. + tmp.as_file_mut() + .sync_all() + .with_context(|| format!("Failed to fsync temp file for {}", dest.display()))?; // `persist` replaces the target *name* via rename(2), which does // not traverse a symlink at the final component, so even a // post-check swap cannot redirect the write. @@ -294,9 +386,6 @@ async fn write_owned_impl(path: &Path, contents: &[u8], mode: u32, publish: Publ tmp.as_file_mut() .write_all(&payload) .with_context(|| format!("Failed to write temp file for {}", dest.display()))?; - tmp.as_file_mut() - .sync_all() - .with_context(|| format!("Failed to fsync temp file for {}", dest.display()))?; std::fs::set_permissions(tmp.path(), std::fs::Permissions::from_mode(mode)).with_context( || { format!( @@ -311,6 +400,12 @@ async fn write_owned_impl(path: &Path, contents: &[u8], mode: u32, publish: Publ dest.display() ) })?; + // Flushed last, after the mode and the ownership, so the inode a + // crash recovers is the one that was published — see + // `publish_staged_blocking`. + tmp.as_file_mut() + .sync_all() + .with_context(|| format!("Failed to fsync temp file for {}", dest.display()))?; match publish { Publish::Replace => { tmp.persist(&dest).map_err(|e| { @@ -389,28 +484,360 @@ pub async fn atomic_write(path: &Path, contents: &[u8], mode: u32) -> Result<()> /// The blocking half of [`atomic_write`], for callers that are not async. /// /// Same guarantees, same order: staged in the destination's directory, -/// written, `sync_all`ed, permissioned, ownership-preserved, renamed, +/// written, ownership-preserved, permissioned, `sync_all`ed, renamed, /// and the directory flushed. Callers in an async context use /// [`atomic_write`] instead, which runs this on a blocking thread. /// +/// One spelling of [`publish_staged_blocking`], the crate's +/// general-purpose staging publisher — see there for the two decisions +/// this one makes ([`StagedOwner::Destination`] and +/// [`StagedDurability::FlushDirectory`]) and why, and for the staged +/// writers that publish outside it. +/// /// # Errors /// Returns an error under the same conditions as [`atomic_write`]. pub fn atomic_write_blocking(path: &Path, contents: &[u8], mode: u32) -> Result<()> { - let parent = parent_dir(path); - // Capture the existing destination's uid/gid (if any) so the - // rename does not strip operator-meaningful ownership. Missing - // file -> None; do not chown the staged file in that case so a - // fresh create keeps process default ownership. - let existing_owner = std::fs::metadata(path).ok().map(|m| (m.uid(), m.gid())); + publish_staged_blocking( + path, + contents, + mode, + StagedOwner::Destination, + StagedDurability::FlushDirectory, + ) +} +/// Publishes `contents` at `path` by rename, **without** flushing the +/// containing directory. +/// +/// [`atomic_write`]'s guarantee against a torn read, without its +/// durability guarantee. This is the right writer for regenerable +/// configuration — a compose override, an `OpenBao` Agent template, a +/// patched `ca.json` — where a reader (a container mounting the file, a +/// sidecar re-rendering it) must never see half a document, but a crash +/// that loses the new directory entry leaves the previous file in place +/// and costs a re-run of the command that produced it rather than an +/// outage. The flush is a disk round trip per write and `init` performs +/// dozens of these, so it is not spent where the file can be rebuilt. +/// +/// Callers whose file is read back to resume, or that hold something +/// that cannot be re-derived, use [`atomic_write`] instead. +/// +/// # Errors +/// Returns an error under the same conditions as [`atomic_write`], +/// except that no directory flush is attempted. +pub async fn atomic_replace(path: &Path, contents: &[u8], mode: u32) -> Result<()> { + // Owned once, to move into the blocking task, exactly as + // `atomic_write` does. + let dest = path.to_path_buf(); + let payload = contents.to_vec(); + tokio::task::spawn_blocking(move || atomic_replace_blocking(&dest, &payload, mode)) + .await + .context("Atomic replace task panicked")? +} + +/// The blocking half of [`atomic_replace`], for callers that are not +/// async. +/// +/// One spelling of [`publish_staged_blocking`] — see there for the two +/// decisions this one makes ([`StagedOwner::Destination`] and +/// [`StagedDurability::RenameOnly`]) and why. +/// +/// # Errors +/// Returns an error under the same conditions as [`atomic_replace`]. +pub fn atomic_replace_blocking(path: &Path, contents: &[u8], mode: u32) -> Result<()> { + publish_staged_blocking( + path, + contents, + mode, + StagedOwner::Destination, + StagedDurability::RenameOnly, + ) +} + +/// [`atomic_write`], resolving a symlink at the final path component +/// first. +/// +/// The truncating write this pair of functions replaces opened the +/// destination with `O_TRUNC`, which follows that link and delivers the +/// bytes to its target. A bare rename replaces the link instead: the +/// operator's link is gone, and whatever it pointed at is left holding +/// the previous contents while the write reports success. Resolving +/// first keeps the file the operator arranged to be written the file +/// that is written. +/// +/// This is the spelling for a destination an operator arranges: the +/// configuration bootroot renders into its own tree and an operator may +/// have pointed elsewhere (`.env`, `ca.json` and its template, +/// `openbao.hcl`, the compose overrides, the responder and `OpenBao` +/// Agent configs, `state.json`), and the output paths they name on the +/// command line (`init`'s two, `rotate openbao-recovery --output`, +/// `bootroot-remote bootstrap`'s `agent.toml` destination — that +/// command is what creates the file on a target host, so a link there +/// is one the operator put in place and the truncating write followed). +/// +/// Two classes deliberately keep [`atomic_write`]'s bare rename: +/// +/// - **Credentials at a path bootroot chose**, inside the secrets tree. +/// A link there is a redirection vector rather than an operator +/// convenience, and the reader reads the path bootroot handed it — +/// which the rename leaves holding the current secret — so following +/// one buys nothing and costs the guarantee. The override credential +/// paths, whose directory an unprivileged user owns, go further and +/// refuse a link outright ([`atomic_rewrite_owned_no_symlink`]). +/// - **Files another writer already publishes by rename**, namely the +/// control node's `agent.toml` (since #613) and the issued cert and +/// key (since #593). A link at those paths does not survive the +/// writer that creates the file, so resolving it in the writers that +/// *edit* the file would make the two disagree rather than preserve +/// anything. +/// +/// Not a security check: see [`resolve_symlink_destination`]. +/// +/// # Errors +/// Returns an error under the same conditions as [`atomic_write`], or +/// if the destination's symlink chain cannot be resolved (a cycle, or +/// an unreadable link). +pub async fn atomic_write_through_symlink(path: &Path, contents: &[u8], mode: u32) -> Result<()> { + let dest = path.to_path_buf(); + let payload = contents.to_vec(); + tokio::task::spawn_blocking(move || { + atomic_write_through_symlink_blocking(&dest, &payload, mode) + }) + .await + .context("Atomic write task panicked")? +} + +/// The blocking half of [`atomic_write_through_symlink`], for callers +/// that are not async. +/// +/// # Errors +/// Returns an error under the same conditions as +/// [`atomic_write_through_symlink`]. +pub fn atomic_write_through_symlink_blocking( + path: &Path, + contents: &[u8], + mode: u32, +) -> Result<()> { + atomic_write_blocking(&resolve_symlink_destination(path)?, contents, mode) +} + +/// [`atomic_replace`], resolving a symlink at the final path component +/// first. +/// +/// The same compatibility decision [`atomic_write_through_symlink`] +/// documents — see there for which destinations take it and which keep +/// the bare rename — without the directory flush. +/// +/// # Errors +/// Returns an error under the same conditions as [`atomic_replace`], or +/// if the destination's symlink chain cannot be resolved. +pub async fn atomic_replace_through_symlink(path: &Path, contents: &[u8], mode: u32) -> Result<()> { + let dest = path.to_path_buf(); + let payload = contents.to_vec(); + tokio::task::spawn_blocking(move || { + atomic_replace_through_symlink_blocking(&dest, &payload, mode) + }) + .await + .context("Atomic replace task panicked")? +} + +/// The blocking half of [`atomic_replace_through_symlink`], for callers +/// that are not async. +/// +/// # Errors +/// Returns an error under the same conditions as +/// [`atomic_replace_through_symlink`]. +pub fn atomic_replace_through_symlink_blocking( + path: &Path, + contents: &[u8], + mode: u32, +) -> Result<()> { + atomic_replace_blocking(&resolve_symlink_destination(path)?, contents, mode) +} + +/// The mode a staged publish should apply at `path`: the mode the file +/// already carries, or `default_mode` when there is no file to read one +/// from. +/// +/// A truncating write left an existing destination's mode alone and let +/// the umask decide a fresh create's. A rename installs a *fresh* inode +/// that inherits neither, so every publish replacing such a write has to +/// state a mode — and stating a constant would silently re-widen a file +/// an operator narrowed by hand, or that a restrictive umask created +/// narrow. Reading the destination's own mode back keeps the rewrite +/// case byte-for-byte as it was; `default_mode` covers only the create, +/// which is the one case a host with a non-default umask can observe. +/// +/// Not for a file with a mode policy of its own — a key, a certificate, +/// the two `init` outputs — where the policy's constant is the answer +/// and a stale mode on disk must not outlive it. +#[must_use] +pub fn preserved_mode(path: &Path, default_mode: u32) -> u32 { + std::fs::metadata(path).map_or(default_mode, |meta| meta.permissions().mode() & 0o7777) +} + +/// Who owns the inode a staged publish renames into place. +/// +/// A rename installs a *fresh* inode, so ownership is never inherited +/// from the file being replaced the way a truncating write left it +/// untouched. A staged publish therefore has to say where the uid/gid +/// comes from, and the two answers below differ because their files +/// do. +/// +/// These two are [`publish_staged_blocking`]'s answers, not the +/// crate's. The override credential writers stage independently and +/// take a third — ownership from the parent directory, or read back +/// through `symlink_metadata` — for the reason recorded there. +#[derive(Clone, Copy)] +pub enum StagedOwner { + /// Carry the destination's uid and gid onto the new inode, and + /// leave a fresh create to the writing process. + /// + /// For files with no ownership policy of their own — a `0600` + /// `agent.toml`, `rotation-state.json`, the fast-poll state, + /// `state.json` — where the operator's or an earlier writer's + /// ownership is the only record of who may read them. Re-owning one + /// to the writer (a `service add` run by root replacing a file the + /// long-running agent reads) is an outage. + Destination, + /// Leave the uid to the writing process and set the group to the + /// `--cert-group` policy's gid, where it names one. + /// + /// For the issued certificate, key and CA bundle, whose group is + /// dictated by that policy and re-asserted on every write: reading + /// the gid off the destination instead would let a stale group + /// outlive the policy that replaced it. All three land world- or + /// group-readable by the policy, so no consumer loses access to a + /// file it could read before. + PolicyGroup(Option), +} + +/// Whether the directory entry a staged publish creates is flushed +/// before the write is reported as done. +/// +/// The flush is a disk round trip on every write, so it is a decision +/// per file rather than a default — see [`sync_parent_dir`]. +#[derive(Clone, Copy)] +pub enum StagedDurability { + /// `sync_parent_dir` after the rename: the file is read back to + /// resume, so the published name has to survive a power loss and + /// not merely a clean replacement. + FlushDirectory, + /// Rename and stop: a crash that loses the new directory entry + /// leaves the previous file in place and costs a rewrite — a + /// reissued certificate, the next sync's `eab.json` — rather than + /// an outage. + RenameOnly, +} + +/// Publishes `contents` at `path` by staging a temporary in the same +/// directory, applying `mode` and `owner` to it there, and `rename`ing +/// it over the destination. +/// +/// This is the crate's general-purpose staging publisher, and every +/// writer of a file at a path bootroot itself owns goes through it — +/// `state.json`, `rotation-state.json`, `agent.toml`, the fast-poll +/// state, the two `init` outputs, the issued certificate, key and CA +/// bundle, and the configuration `init` and the rotation commands +/// generate (`.env`, `ca.json` and its template, `openbao.hcl`, the +/// responder config, the `OpenBao` Agent configs and credentials, the +/// compose overrides). For those the destination name is only ever +/// observed as the previous file or the complete new one, and the final +/// mode holds from the moment the name appears. +/// +/// It is not the crate's only staged publish. The override credential +/// writers — [`create_owned_credential_noclobber`], +/// [`write_owned_file_replace`] and +/// [`atomic_rewrite_owned_no_symlink`], which write a service's +/// `role_id`, `secret_id` and `eab.json` when those are relocated into +/// an operator-provisioned, agent-owned directory — stage and rename a +/// temporary of their own. They reach the same two guarantees by the +/// same means, and differ in what this routine deliberately does not +/// offer: ownership taken from the parent directory (a root process +/// creating a file there would otherwise leave it unreadable to the +/// non-root agent) or read back through `symlink_metadata`, and a +/// publish that refuses a name already present rather than replacing +/// it. Neither policy generalises to the files above, and folding them +/// together would make one of the two callers wrong. +/// +/// Two production writers stage nothing at all and have neither +/// property: `save_unseal_keys`, for `secrets/openbao/unseal-keys.txt`, +/// and [`crate::eab`]'s `write_key_file`, for the secrets-tree +/// `eab.json` beside each service's `secret_id`. Both still write over +/// the destination in place and set `0600` once the bytes are down. +/// Converting them is a separate change; do not read the guarantees +/// above as covering the crate's writes exhaustively until it lands. +/// +/// Callers reach it through one of the four wrappers rather than +/// directly: [`atomic_write`]/[`atomic_write_blocking`] for a file read +/// back to resume, [`atomic_replace`]/[`atomic_replace_blocking`] for +/// one that can be regenerated. `crate::cert_group` is the exception, +/// calling in with [`StagedOwner::PolicyGroup`] for the three files the +/// `--cert-group` policy owns. +/// +/// The staged file is created by `tempfile` at `0600` and reaches +/// `mode` only while it is still at its temporary name, so a wider +/// `mode` is never observable at the destination — the property issue +/// #593 asked of the key file, and which now holds for every caller. +/// The chown runs before the chmod for the same reason: nothing may +/// sit group-readable under the writer's primary gid, even at the +/// temporary name, before the policy's gid lands. The staged file is +/// `fsync`ed last of all, once both have been applied, so a publish +/// that survives a crash carries the ownership and mode it was +/// published with rather than the temporary's defaults. +/// +/// The temporary is removed if any step before the rename fails +/// (`NamedTempFile` deletes on drop), so a failed publish leaves +/// neither a torn destination nor a stray sibling. +/// +/// # Errors +/// Returns an error if the temp file cannot be created, written, +/// chowned, permissioned or renamed, or if the containing directory +/// cannot be flushed under [`StagedDurability::FlushDirectory`]. +pub fn publish_staged_blocking( + path: &Path, + contents: &[u8], + mode: u32, + owner: StagedOwner, + durability: StagedDurability, +) -> Result<()> { + let parent = parent_dir(path); let mut tmp = tempfile::NamedTempFile::new_in(&parent) .with_context(|| format!("Failed to create temp file in {}", parent.display()))?; tmp.as_file_mut() .write_all(contents) .with_context(|| format!("Failed to write temp file for {}", path.display()))?; - tmp.as_file_mut() - .sync_all() - .with_context(|| format!("Failed to fsync temp file for {}", path.display()))?; + match owner { + StagedOwner::Destination => { + // A destination that is missing, or cannot be stat'd, + // leaves the staged file with the writing process's own + // ownership rather than being chowned to a guess. + if let Some((dest_uid, dest_gid)) = + std::fs::metadata(path).ok().map(|m| (m.uid(), m.gid())) + { + let tmp_meta = std::fs::metadata(tmp.path()) + .with_context(|| format!("Failed to stat temp file for {}", path.display()))?; + // Skipping a chown that would change nothing keeps an + // unprivileged writer whose ownership already matches + // from failing on a call it did not need to make. + if tmp_meta.uid() != dest_uid || tmp_meta.gid() != dest_gid { + std::os::unix::fs::chown(tmp.path(), Some(dest_uid), Some(dest_gid)) + .with_context(|| { + format!( + "Failed to preserve existing uid={dest_uid} gid={dest_gid} on {}", + path.display() + ) + })?; + } + } + } + StagedOwner::PolicyGroup(Some(gid)) => { + std::os::unix::fs::chown(tmp.path(), None, Some(gid)).with_context(|| { + format!("Failed to chown {} to gid {gid}", tmp.path().display()) + })?; + } + StagedOwner::PolicyGroup(None) => {} + } std::fs::set_permissions(tmp.path(), std::fs::Permissions::from_mode(mode)).with_context( || { format!( @@ -419,20 +846,17 @@ pub fn atomic_write_blocking(path: &Path, contents: &[u8], mode: u32) -> Result< ) }, )?; - if let Some((dest_uid, dest_gid)) = existing_owner { - let tmp_meta = std::fs::metadata(tmp.path()) - .with_context(|| format!("Failed to stat temp file for {}", path.display()))?; - if tmp_meta.uid() != dest_uid || tmp_meta.gid() != dest_gid { - std::os::unix::fs::chown(tmp.path(), Some(dest_uid), Some(dest_gid)).with_context( - || { - format!( - "Failed to preserve existing uid={dest_uid} gid={dest_gid} on {}", - path.display() - ) - }, - )?; - } - } + // Last of the three, after the bytes *and* the uid/gid/mode: `fsync` + // persists the whole inode, so a flush taken before the chown and + // the chmod leaves those two changes in memory only. A crash could + // then recover a durably named, fully written file wearing the + // temporary's own `0600` and the writer's primary group instead of + // the mode and the policy gid it was published with — the directory + // flush below makes the *name* durable and says nothing about the + // inode it points at. + tmp.as_file_mut() + .sync_all() + .with_context(|| format!("Failed to fsync temp file for {}", path.display()))?; tmp.persist(path).map_err(|e| { anyhow::anyhow!( "Failed to rename temp file to {}: {}", @@ -440,7 +864,10 @@ pub fn atomic_write_blocking(path: &Path, contents: &[u8], mode: u32) -> Result< e.error ) })?; - sync_parent_dir(path)?; + match durability { + StagedDurability::FlushDirectory => sync_parent_dir(path)?, + StagedDurability::RenameOnly => {} + } Ok(()) } @@ -525,7 +952,7 @@ pub async fn write_cert_and_key( /// Writes a CA bundle to disk, creating parent directories as needed. /// -/// Always sets the mode to [`CA_BUNDLE_FILE_MODE`] (`0o644`), +/// Always sets the mode to [`cert_group::CA_BUNDLE_FILE_MODE`] (`0o644`), /// regardless of `policy`. CA bundles are public trust material, and /// re-asserting the mode on every write means a rotation overrides /// any stricter mode left behind by an earlier writer (notably @@ -534,6 +961,13 @@ pub async fn write_cert_and_key( /// `chown`s the file to the policy's gid so cert-group members can /// read the bundle alongside the cert and key. /// +/// The bundle itself is published by +/// [`cert_group::write_bundle_file`], which stages it beside the +/// destination and renames it into place with the mode and owner +/// already applied. The agent re-reads this file to rebuild its trust +/// store while a rotation may be rewriting it, so the destination name +/// must never hold a truncated chain. +/// /// # Errors /// Returns an error if the directory cannot be created, the bundle /// cannot be written, or the mode/owner cannot be applied. @@ -548,31 +982,7 @@ pub async fn write_ca_bundle( fs::create_dir_all(bundle_dir) .await .with_context(|| format!("Failed to create CA bundle dir {}", bundle_dir.display()))?; - fs::write(bundle_path, bundle_pem) - .await - .context("Failed to write CA bundle file")?; - fs::set_permissions( - bundle_path, - std::fs::Permissions::from_mode(CA_BUNDLE_FILE_MODE), - ) - .await - .with_context(|| { - format!( - "Failed to set mode {CA_BUNDLE_FILE_MODE:o} on CA bundle {}", - bundle_path.display() - ) - })?; - if let Some(gid) = policy.gid { - let owned = bundle_path.to_path_buf(); - tokio::task::spawn_blocking(move || -> Result<()> { - std::os::unix::fs::chown(&owned, None, Some(gid)).with_context(|| { - format!("Failed to chown CA bundle {} to gid {gid}", owned.display()) - }) - }) - .await - .context("CA bundle chown task panicked")??; - } - Ok(()) + cert_group::write_bundle_file(bundle_path, bundle_pem, policy).await } #[cfg(test)] @@ -582,6 +992,7 @@ mod tests { use tempfile::tempdir; use super::*; + use crate::cert_group::CA_BUNDLE_FILE_MODE; #[tokio::test] async fn test_ensure_secrets_dir_permissions() { @@ -785,6 +1196,158 @@ mod tests { assert_eq!(contents, "FRESH"); } + /// The bundle arrives by rename from a staged temporary, so an + /// agent rebuilding its trust store mid-rotation reads either the + /// previous chain or the complete new one. A changed inode is what + /// distinguishes that from the `fs::write` this replaced, and an + /// otherwise empty directory is what proves the temporary did not + /// survive the publish. + #[tokio::test] + async fn write_ca_bundle_publishes_a_new_inode_and_leaves_no_temporary() { + use std::os::unix::fs::MetadataExt; + + let dir = tempdir().unwrap(); + let bundle_path = dir.path().join("ca-bundle.pem"); + write_ca_bundle(&bundle_path, "FIRST", CertGroupPolicy::none()) + .await + .unwrap(); + let first_inode = std::fs::metadata(&bundle_path).unwrap().ino(); + + write_ca_bundle(&bundle_path, "SECOND", CertGroupPolicy::none()) + .await + .unwrap(); + + assert_eq!( + fs::read_to_string(&bundle_path).await.unwrap(), + "SECOND", + "the rename must publish the new chain" + ); + assert_ne!(std::fs::metadata(&bundle_path).unwrap().ino(), first_inode); + let entries: Vec<_> = std::fs::read_dir(dir.path()) + .unwrap() + .map(|e| e.unwrap().file_name()) + .collect(); + assert_eq!(entries, vec![std::ffi::OsString::from("ca-bundle.pem")]); + } + + /// `write_ca_bundle` creates the parent directory before staging, + /// so the very first bundle of a deployment lands even though the + /// staged temporary needs a directory to be created in. + #[tokio::test] + async fn write_ca_bundle_creates_a_missing_parent_directory() { + let dir = tempdir().unwrap(); + let bundle_path = dir.path().join("nested").join("ca-bundle.pem"); + + write_ca_bundle(&bundle_path, "BUNDLE", CertGroupPolicy::none()) + .await + .unwrap(); + + assert_eq!(fs::read_to_string(&bundle_path).await.unwrap(), "BUNDLE"); + } + + /// The common case: nothing to resolve, so the caller stages and + /// renames at exactly the path it was given. + #[test] + fn resolve_symlink_destination_passes_a_regular_file_through() { + let dir = tempdir().unwrap(); + let path = dir.path().join("plain.txt"); + std::fs::write(&path, "x").unwrap(); + + assert_eq!(resolve_symlink_destination(&path).unwrap(), path); + assert_eq!( + resolve_symlink_destination(&dir.path().join("absent.txt")).unwrap(), + dir.path().join("absent.txt"), + "a destination that does not exist yet resolves to itself" + ); + } + + /// A link resolves to the file behind it, so the rename that follows + /// replaces the target and leaves the link pointing at it. + #[test] + fn resolve_symlink_destination_follows_a_link_to_its_target() { + let dir = tempdir().unwrap(); + let target = dir.path().join("target.txt"); + std::fs::write(&target, "x").unwrap(); + let link = dir.path().join("link.txt"); + std::os::unix::fs::symlink(&target, &link).unwrap(); + + assert_eq!( + resolve_symlink_destination(&link).unwrap(), + std::fs::canonicalize(&target).unwrap() + ); + } + + /// A dangling link still names where the operator wants the file. + /// `canonicalize` cannot say so — there is nothing to resolve + /// against — so the link text is read instead, absolute and + /// relative alike, and the caller publishes at the target the way + /// the truncating write's `O_CREAT` created it. + #[test] + fn resolve_symlink_destination_follows_a_dangling_link_to_its_target() { + let dir = tempdir().unwrap(); + let absolute = dir.path().join("absolute.txt"); + std::os::unix::fs::symlink(dir.path().join("absent.txt"), &absolute).unwrap(); + assert_eq!( + resolve_symlink_destination(&absolute).unwrap(), + dir.path().join("absent.txt") + ); + + let relative = dir.path().join("relative.txt"); + std::os::unix::fs::symlink("sub/absent.txt", &relative).unwrap(); + assert_eq!( + resolve_symlink_destination(&relative).unwrap(), + dir.path().join("sub").join("absent.txt"), + "a relative link is resolved against its own directory" + ); + } + + /// A chain of dangling links is followed to its end, so the write + /// replaces neither link on the way. + #[test] + fn resolve_symlink_destination_follows_a_dangling_link_chain() { + let dir = tempdir().unwrap(); + let end = dir.path().join("absent.txt"); + let middle = dir.path().join("middle.txt"); + let head = dir.path().join("head.txt"); + std::os::unix::fs::symlink(&end, &middle).unwrap(); + std::os::unix::fs::symlink(&middle, &head).unwrap(); + + assert_eq!(resolve_symlink_destination(&head).unwrap(), end); + } + + /// A cycle has no end to follow to, and every name in it is a link + /// a rename would destroy. The truncating write answered `ELOOP` + /// here; resolution fails rather than handing back a path the + /// caller would publish over. + #[test] + fn resolve_symlink_destination_rejects_a_symlink_cycle() { + let dir = tempdir().unwrap(); + let a = dir.path().join("a.txt"); + let b = dir.path().join("b.txt"); + std::os::unix::fs::symlink(&b, &a).unwrap(); + std::os::unix::fs::symlink(&a, &b).unwrap(); + + let err = resolve_symlink_destination(&a).unwrap_err().to_string(); + assert!( + err.contains("Too many levels of symbolic links"), + "unexpected error: {err}" + ); + assert!( + std::fs::symlink_metadata(&a) + .unwrap() + .file_type() + .is_symlink(), + "resolution must not touch the links it refuses to follow" + ); + + let self_link = dir.path().join("self.txt"); + std::os::unix::fs::symlink(&self_link, &self_link).unwrap(); + assert!( + resolve_symlink_destination(&self_link).is_err(), + "a link pointing at itself is a cycle too" + ); + } + /// `atomic_write` must leave the destination at the supplied mode /// and the requested contents, both for the create case and for /// the overwrite case (rotation). @@ -810,6 +1373,179 @@ mod tests { assert_eq!(mode, KEY_FILE_MODE); } + /// `atomic_replace` publishes the same way `atomic_write` does — + /// fresh inode, requested mode, no torn destination — and differs + /// only in declining the directory flush, which leaves no + /// observable trace to assert on. Pinning the rest here keeps the + /// no-flush spelling from drifting into a plain `fs::write` on the + /// assumption that "not durable" means "not staged". + #[tokio::test] + async fn atomic_replace_creates_and_overwrites_with_mode() { + use std::os::unix::fs::MetadataExt; + + let dir = tempdir().unwrap(); + let path = dir.path().join("compose.override.yml"); + + super::atomic_replace(&path, b"first", 0o644).await.unwrap(); + assert_eq!(fs::read_to_string(&path).await.unwrap(), "first"); + let first_ino = std::fs::metadata(&path).unwrap().ino(); + assert_eq!( + std::fs::metadata(&path).unwrap().permissions().mode() & 0o777, + 0o644 + ); + + super::atomic_replace(&path, b"second", 0o644) + .await + .unwrap(); + assert_eq!(fs::read_to_string(&path).await.unwrap(), "second"); + assert_ne!( + std::fs::metadata(&path).unwrap().ino(), + first_ino, + "a staged publish installs a new inode; an in-place write would not" + ); + + let strays: Vec<_> = std::fs::read_dir(dir.path()) + .unwrap() + .filter_map(|e| e.ok().map(|e| e.file_name())) + .filter(|name| name != "compose.override.yml") + .collect(); + assert!( + strays.is_empty(), + "staged temporary left behind: {strays:?}" + ); + } + + /// The two spellings differ in exactly one observable way, and this + /// pins both halves of it: given a symlinked destination, the + /// `_through_symlink` wrappers deliver to the target and leave the + /// link standing — what `O_TRUNC` did — while the bare wrappers + /// replace the link with the published file, which is what the + /// `agent.toml` and cert/key writers want. + #[tokio::test] + async fn through_symlink_wrappers_deliver_to_the_target_the_bare_ones_replace() { + let dir = tempdir().unwrap(); + + for (name, published) in [("write.env", false), ("replace.env", true)] { + let target = dir.path().join(format!("target-{name}")); + let link = dir.path().join(name); + std::fs::write(&target, b"seed").unwrap(); + std::os::unix::fs::symlink(&target, &link).unwrap(); + + if published { + super::atomic_replace_through_symlink(&link, b"fresh", 0o644) + .await + .unwrap(); + } else { + super::atomic_write_through_symlink(&link, b"fresh", 0o644) + .await + .unwrap(); + } + + assert!( + std::fs::symlink_metadata(&link) + .unwrap() + .file_type() + .is_symlink(), + "{name}: the operator's link must survive the publish" + ); + assert_eq!(std::fs::read_to_string(&target).unwrap(), "fresh"); + assert_eq!( + std::fs::metadata(&link).unwrap().permissions().mode() & 0o777, + 0o644, + "{name}: the mode lands on the target" + ); + } + + let target = dir.path().join("target-bare"); + let link = dir.path().join("bare"); + std::fs::write(&target, b"seed").unwrap(); + std::os::unix::fs::symlink(&target, &link).unwrap(); + + super::atomic_write(&link, b"fresh", 0o644).await.unwrap(); + + assert!( + !std::fs::symlink_metadata(&link) + .unwrap() + .file_type() + .is_symlink(), + "the bare spelling publishes at the name, replacing the link" + ); + assert_eq!(std::fs::read_to_string(&link).unwrap(), "fresh"); + assert_eq!( + std::fs::read_to_string(&target).unwrap(), + "seed", + "and leaves the target alone" + ); + } + + /// A dangling link is followed to where it points, so a first write + /// through one creates the target rather than replacing the link — + /// the `O_CREAT` half of the behaviour being preserved. + #[test] + fn through_symlink_wrappers_create_a_dangling_links_target() { + let dir = tempdir().unwrap(); + let target = dir.path().join("absent.yml"); + let link = dir.path().join("override.yml"); + std::os::unix::fs::symlink(&target, &link).unwrap(); + + super::atomic_replace_through_symlink_blocking(&link, b"fresh", 0o644).unwrap(); + + assert_eq!(std::fs::read_to_string(&target).unwrap(), "fresh"); + assert!( + std::fs::symlink_metadata(&link) + .unwrap() + .file_type() + .is_symlink() + ); + } + + /// A cycle has no target to deliver to, so the publish fails the way + /// the truncating write's `ELOOP` did rather than replacing one of + /// the links with the file. + #[test] + fn through_symlink_wrappers_refuse_a_symlink_cycle() { + let dir = tempdir().unwrap(); + let a = dir.path().join("a.env"); + let b = dir.path().join("b.env"); + std::os::unix::fs::symlink(&b, &a).unwrap(); + std::os::unix::fs::symlink(&a, &b).unwrap(); + + assert!(super::atomic_write_through_symlink_blocking(&a, b"x", 0o644).is_err()); + assert!(super::atomic_replace_through_symlink_blocking(&a, b"x", 0o644).is_err()); + assert!( + std::fs::symlink_metadata(&a) + .unwrap() + .file_type() + .is_symlink(), + "a refused publish must leave the links it would not follow" + ); + } + + /// `preserved_mode` answers with the destination's own mode where + /// there is one, and the caller's default only on a create. A + /// regression here is a rename silently re-widening a file an + /// operator narrowed — the one property the truncating writes these + /// publishes replaced got for free. + #[test] + fn preserved_mode_reads_the_destination_then_falls_back() { + let dir = tempdir().unwrap(); + let path = dir.path().join("state.json"); + + assert_eq!( + super::preserved_mode(&path, 0o644), + 0o644, + "a missing destination takes the caller's default" + ); + + std::fs::write(&path, b"{}").unwrap(); + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o600)).unwrap(); + assert_eq!( + super::preserved_mode(&path, 0o644), + 0o600, + "an existing destination keeps the mode it carries" + ); + } + /// Overwriting an existing file via `atomic_write` must preserve /// the destination's gid. The rename otherwise replaces the inode /// with one owned by the writer's effective uid/gid — locking out @@ -963,6 +1699,61 @@ mod tests { assert_eq!(std::fs::read_to_string(&path).unwrap(), "payload"); } + /// The two ownership answers must stay two answers. A destination + /// seeded with a supplementary gid keeps it under + /// [`StagedOwner::Destination`] — the case + /// `atomic_write_preserves_existing_gid_on_overwrite` covers — and + /// is re-owned to the writer's own gid under + /// [`StagedOwner::PolicyGroup`], where the `--cert-group` policy is + /// the authority on the group and a stale one must not outlive it. + /// Requires a supplementary gid (see `one_supplementary_test_gid`). + #[test] + fn publish_staged_re_owns_under_the_policy_and_preserves_under_destination() { + let Some(gid) = crate::cert_group::one_supplementary_test_gid() else { + return; + }; + let dir = tempdir().unwrap(); + let seed = |name: &str| { + let path = dir.path().join(name); + std::fs::write(&path, "first").unwrap(); + std::os::unix::fs::chown(&path, None, Some(gid)) + .expect("test process must be able to chgrp to a supplementary gid"); + path + }; + let preserved = seed("preserved"); + let re_owned = seed("re-owned"); + let own_gid = std::fs::metadata(&preserved).unwrap().gid(); + assert_eq!(own_gid, gid, "seed gid must take effect"); + + publish_staged_blocking( + &preserved, + b"second", + KEY_FILE_MODE, + StagedOwner::Destination, + StagedDurability::RenameOnly, + ) + .unwrap(); + publish_staged_blocking( + &re_owned, + b"second", + KEY_FILE_MODE, + StagedOwner::PolicyGroup(None), + StagedDurability::RenameOnly, + ) + .unwrap(); + + assert_eq!( + std::fs::metadata(&preserved).unwrap().gid(), + gid, + "StagedOwner::Destination must carry the destination's gid across the rename" + ); + assert_ne!( + std::fs::metadata(&re_owned).unwrap().gid(), + gid, + "StagedOwner::PolicyGroup must not inherit the destination's gid" + ); + } + /// Same pin for the `O_EXCL` credential writer. Its flush lives in /// the `NoClobber` arm of `write_owned_impl` rather than in /// `atomic_write_blocking`, so it regresses independently. diff --git a/src/i18n.rs b/src/i18n.rs index 96288ada..c127f076 100644 --- a/src/i18n.rs +++ b/src/i18n.rs @@ -575,6 +575,14 @@ pub(crate) struct Strings { pub(crate) error_reinit_stepca_password_missing_with_ca_material: &'static str, } +/// `Clone` so a message bundle can cross into a `spawn_blocking` +/// closure without the caller giving up its own — it is one `Locale` +/// discriminant, and every string it reaches is `&'static`, so the +/// clone is a byte copy. Deliberately not `Copy`: the crate passes +/// `&Messages` through several hundred signatures, and +/// `clippy::trivially_copy_pass_by_ref` would demand every one of them +/// change. +#[derive(Clone)] pub(crate) struct Messages { locale: Locale, } diff --git a/src/main.rs b/src/main.rs index c136fa0e..1b467551 100644 --- a/src/main.rs +++ b/src/main.rs @@ -6,6 +6,8 @@ mod cli; mod commands; mod i18n; mod state; +#[cfg(test)] +mod test_support; use clap::Parser; diff --git a/src/state.rs b/src/state.rs index 7f5b7b7f..6eab46e1 100644 --- a/src/state.rs +++ b/src/state.rs @@ -3,11 +3,22 @@ use std::fmt; use std::path::{Path, PathBuf}; use anyhow::{Context, Result}; +use bootroot::fs_util; use clap::ValueEnum; use serde::{Deserialize, Serialize}; const DEFAULT_SECRETS_DIR: &str = "secrets"; const DEFAULT_STATE_FILE: &str = "state.json"; +/// Mode for a `state.json` this process creates. The plain `fs::write` +/// this file used to be published with left the mode to the process +/// umask on a fresh create (`0644` in practice) and to the destination +/// on a rewrite. A staged temporary inherits neither, so a create needs +/// a stated mode, and `0644` is the one the umask produced: the file is +/// an inventory of services, paths and role ids, carrying no secret — +/// the `secret_id` behind `secret_id_path` lives in its own `0600` +/// file. A destination that already exists keeps its own mode instead; +/// see [`StateFile::publish_mode`]. +const STATE_FILE_MODE: u32 = 0o644; pub(crate) const DEFAULT_HOOK_TIMEOUT_SECS: u64 = 30; /// Describes how to reload a service after its infrastructure certificate @@ -122,11 +133,106 @@ impl StateFile { Ok(state) } + /// Publishes `state.json` by renaming a temporary staged in the same + /// directory. + /// + /// `state.json` is what `bootroot` reads back to know what it + /// already did, so a torn write is not a stale record but no record + /// at all: the next run fails to parse it and falls back to nothing. + /// Renaming over the destination means a reader — another `bootroot` + /// invocation, or the next run after a crash — sees either the whole + /// previous version or the whole new one, and two concurrent writers + /// see one version or the other rather than each other's bytes. + /// `bootler` staggers its two rotation units ten minutes apart + /// because this write used to race; that stagger is no longer + /// load-bearing for this file (removing it is `bootler`'s own + /// follow-up). + /// + /// The containing directory is flushed after the rename, inside + /// [`fs_util::atomic_write_blocking`]. This file is read back to + /// resume, so the published name has to survive a power loss and not + /// merely a clean replacement — the same decision `rotation-state.json` + /// takes, and for the same reason. + /// + /// Blocking, deliberately: the staged write, its flush and the + /// directory flush are three disk round trips. Callers in an async + /// context use [`StateFile::save_async`] instead, which runs this + /// same core on a blocking thread — the pattern the rotation-state + /// writers in `commands::trust` establish. This entry point stays + /// for the synchronous callers (`infra install`, `service update`, + /// which run outside any runtime) and for the tests, which must not + /// need a runtime to write a state file. + /// + /// The mode the file is published at is the destination's own where + /// there is one — see [`StateFile::publish_mode`]. + /// + /// A symlinked destination is resolved first, for the same reason + /// the two `init` outputs resolve theirs: the `fs::write` this + /// replaced followed the link and rewrote its target, while a + /// rename replaces the link itself. Nothing bootroot does creates + /// `state.json` as a link, so this is a no-op on every path it + /// takes itself; it is here so an operator who put one there keeps + /// it. pub(crate) fn save(&self, path: &Path) -> Result<()> { - let contents = - serde_json::to_string_pretty(self).context("Failed to serialize state.json")?; - std::fs::write(path, contents) - .with_context(|| format!("Failed to write {}", path.display())) + Self::publish(path, &self.serialize()?) + } + + /// Async entry point for [`StateFile::save`]. + /// + /// The JSON is serialized here, on the async side, so only the + /// owned payload and path cross into the `'static` closure; the + /// staged write, the file flush and the directory flush then run on + /// a blocking thread rather than a runtime worker. Every async + /// caller uses this — a Tokio worker parked on three disk round + /// trips is a worker polling nothing else, and on a current-thread + /// runtime it is the only worker there is. + /// + /// # Errors + /// Returns an error under the same conditions as + /// [`StateFile::save`], or if the blocking task panics. + pub(crate) async fn save_async(&self, path: &Path) -> Result<()> { + let contents = self.serialize()?; + let dest = path.to_path_buf(); + tokio::task::spawn_blocking(move || Self::publish(&dest, &contents)) + .await + .context("State file write task panicked")? + } + + fn serialize(&self) -> Result { + serde_json::to_string_pretty(self).context("Failed to serialize state.json") + } + + /// The blocking core both entry points share: publish the bytes by + /// rename, through a symlinked destination, at the mode the + /// destination carries. + fn publish(path: &Path, contents: &str) -> Result<()> { + fs_util::atomic_write_through_symlink_blocking( + path, + contents.as_bytes(), + Self::publish_mode(path), + ) + .with_context(|| format!("Failed to write {}", path.display())) + } + + /// The mode [`StateFile::save`] publishes at: the mode the + /// destination already carries, or [`STATE_FILE_MODE`] when there + /// is nothing there yet. + /// + /// The write this replaced opened the destination in place, so a + /// `state.json` an operator had narrowed — or that a restrictive + /// umask created narrow — stayed that way across every later save. + /// A staged temporary inherits nothing from the file it replaces, + /// so stating one mode unconditionally would widen theirs on the + /// next write bootroot makes. Reading it off the destination keeps + /// the rename from changing a property the operator set, the same + /// reason `fs_util::atomic_write_blocking` carries the + /// destination's uid and gid across it. + /// + /// A destination that cannot be stat'd is treated as absent: the + /// staged write that follows reports the real error, and guessing a + /// mode here would only replace it with a worse one. + fn publish_mode(path: &Path) -> u32 { + fs_util::preserved_mode(path, STATE_FILE_MODE) } pub(crate) fn secrets_dir(&self) -> &Path { @@ -252,8 +358,181 @@ fn write_serde_string_value( #[cfg(test)] mod tests { + use std::os::unix::fs::{MetadataExt, PermissionsExt}; + use super::*; + fn state_with_url(url: &str) -> StateFile { + StateFile { + openbao_url: url.to_string(), + ..StateFile::default() + } + } + + /// The save replaces the destination name rather than truncating + /// the file behind it, so a reader holding the old path sees the + /// whole previous version. A changed inode is what distinguishes + /// the two: `fs::write` would have kept it. + #[test] + fn save_publishes_a_new_inode_over_an_existing_state_file() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("state.json"); + state_with_url("http://first:8200").save(&path).unwrap(); + let first_inode = std::fs::metadata(&path).unwrap().ino(); + + state_with_url("http://second:8200").save(&path).unwrap(); + + let reloaded = StateFile::load(&path).unwrap(); + assert_eq!(reloaded.openbao_url, "http://second:8200"); + assert_ne!(std::fs::metadata(&path).unwrap().ino(), first_inode); + } + + /// The staged temporary lives in the destination's own directory, + /// so a failure to clean it up would leave a stray file next to + /// `state.json` for the operator to find. + #[test] + fn save_leaves_no_temporary_behind() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("state.json"); + state_with_url("http://localhost:8200").save(&path).unwrap(); + + let entries: Vec<_> = std::fs::read_dir(dir.path()) + .unwrap() + .map(|e| e.unwrap().file_name()) + .collect(); + assert_eq!(entries, vec![std::ffi::OsString::from("state.json")]); + } + + /// A file that did not exist gets the stated create mode, which is + /// the `0644` the umask used to produce. The file is an inventory, + /// not a secret. + #[test] + fn save_creates_a_new_state_file_at_0644() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("state.json"); + state_with_url("http://localhost:8200").save(&path).unwrap(); + + let mode = std::fs::metadata(&path).unwrap().permissions().mode() & 0o777; + assert_eq!(mode, STATE_FILE_MODE); + } + + /// A `state.json` an operator pointed elsewhere with a symlink is + /// still written through the link, as the `fs::write` this replaced + /// did. Renaming over the link would leave them without it and the + /// target holding the previous state. + #[test] + fn save_writes_through_a_symlinked_state_file() { + let dir = tempfile::tempdir().unwrap(); + let target_dir = dir.path().join("shared"); + std::fs::create_dir(&target_dir).unwrap(); + let target = target_dir.join("state.json"); + let link = dir.path().join("state.json"); + std::os::unix::fs::symlink(&target, &link).unwrap(); + + state_with_url("http://linked:8200").save(&link).unwrap(); + + assert!( + std::fs::symlink_metadata(&link) + .unwrap() + .file_type() + .is_symlink(), + "the operator's link must survive the save" + ); + assert_eq!( + StateFile::load(&target).unwrap().openbao_url, + "http://linked:8200" + ); + } + + /// The rename must not change a mode the operator set. A + /// `state.json` narrowed to `0600` — by hand, or by a restrictive + /// umask when it was created — stayed `0600` across the truncating + /// write this replaced, and still does. + #[test] + fn save_keeps_an_existing_state_files_mode() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("state.json"); + state_with_url("http://first:8200").save(&path).unwrap(); + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o600)).unwrap(); + + state_with_url("http://second:8200").save(&path).unwrap(); + + let mode = std::fs::metadata(&path).unwrap().permissions().mode() & 0o777; + assert_eq!(mode, 0o600, "the save widened a narrowed state.json"); + assert_eq!( + StateFile::load(&path).unwrap().openbao_url, + "http://second:8200" + ); + } + + /// The async entry point publishes what the blocking one does, and + /// does it from a runtime whose only worker is the caller's. A + /// current-thread runtime is the check that matters: `save_async` + /// hands the three disk round trips to `spawn_blocking`, so the + /// write completes while that single worker stays free — a direct + /// `save` here would park it for the duration. + #[tokio::test(flavor = "current_thread")] + async fn save_async_publishes_the_same_file_as_save() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("state.json"); + state_with_url("http://first:8200").save(&path).unwrap(); + let first_inode = std::fs::metadata(&path).unwrap().ino(); + + state_with_url("http://second:8200") + .save_async(&path) + .await + .unwrap(); + + assert_eq!( + StateFile::load(&path).unwrap().openbao_url, + "http://second:8200" + ); + assert_ne!(std::fs::metadata(&path).unwrap().ino(), first_inode); + let entries: Vec<_> = std::fs::read_dir(dir.path()) + .unwrap() + .map(|e| e.unwrap().file_name()) + .collect(); + assert_eq!(entries, vec![std::ffi::OsString::from("state.json")]); + } + + /// Both entry points share one blocking core, so the async one + /// inherits every decision made there — including keeping the mode + /// an existing `state.json` carries and resolving a symlinked + /// destination to its target. + #[tokio::test] + async fn save_async_keeps_an_existing_mode_and_follows_a_symlink() { + let dir = tempfile::tempdir().unwrap(); + let target_dir = dir.path().join("shared"); + std::fs::create_dir(&target_dir).unwrap(); + let target = target_dir.join("state.json"); + let link = dir.path().join("state.json"); + state_with_url("http://first:8200").save(&target).unwrap(); + std::fs::set_permissions(&target, std::fs::Permissions::from_mode(0o600)).unwrap(); + std::os::unix::fs::symlink(&target, &link).unwrap(); + + state_with_url("http://second:8200") + .save_async(&link) + .await + .unwrap(); + + assert!( + std::fs::symlink_metadata(&link) + .unwrap() + .file_type() + .is_symlink(), + "the operator's link must survive the save" + ); + assert_eq!( + std::fs::metadata(&target).unwrap().permissions().mode() & 0o777, + 0o600, + "the async save widened a narrowed state.json" + ); + assert_eq!( + StateFile::load(&target).unwrap().openbao_url, + "http://second:8200" + ); + } + #[test] fn delivery_mode_defaults_to_local_file() { let mode = DeliveryMode::default(); diff --git a/src/test_support.rs b/src/test_support.rs new file mode 100644 index 00000000..f0785c3b --- /dev/null +++ b/src/test_support.rs @@ -0,0 +1,166 @@ +//! Helpers shared by the tests that write an executable and then run +//! it, or hand it to production to run. + +use std::io::Write; +use std::os::unix::fs::PermissionsExt; +use std::path::Path; +use std::process::{Command, Stdio}; + +/// Mode every fake is written with: executable by its writer and by +/// production spawning it on that writer's behalf, and by nobody else. +const FAKE_MODE: u32 = 0o700; + +/// Writes `contents` at `path` and makes it executable, without this +/// process ever holding a descriptor open on it for writing. +/// +/// That last part is the reason this helper exists rather than an +/// `fs::write` at each call site. Tests write these fakes on threads of +/// one process, and a `fork` for any spawn — a test's own, or one +/// production makes for a different fake — duplicates every descriptor +/// the process holds at that instant. A duplicate keeps the open file +/// description alive until that child reaches its `exec`, and the +/// kernel refuses to execute a file any process holds open for writing, +/// so a fake whose writer closed it long ago could still be refused +/// with `ETXTBSY` through a stranger's inherited copy +/// (rust-lang/rust#74214). +/// +/// Handing the write to a child process removes the descriptor from +/// this process's table, and a `fork` copies only the forking process's +/// own descriptors. So no fork here can inherit a write descriptor on a +/// fake, whatever the threads are doing: the race is gone rather than +/// waited out, and no spawn of `path` — production's included — needs a +/// retry. +/// +/// # Panics +/// +/// Panics if the writer cannot be spawned, if it fails, or if the mode +/// cannot be applied. +pub(crate) fn write_executable(path: &Path, contents: &[u8]) { + // `$1` carries the destination as an argument rather than through + // the script text: a Unix path is an arbitrary NUL-free byte + // sequence, and one holding a quote, a space or a byte that is not + // UTF-8 reaches `sh` intact this way and needs no quoting. + let mut writer = Command::new("/bin/sh") + .arg("-c") + .arg(r#"cat > "$1""#) + .arg("sh") + .arg(path) + .stdin(Stdio::piped()) + .spawn() + .unwrap_or_else(|err| panic!("the writer for {} must spawn: {err}", path.display())); + + { + let mut stdin = writer + .stdin + .take() + .expect("stdin was piped, so it is present until taken"); + stdin + .write_all(contents) + .unwrap_or_else(|err| panic!("{} must be writable: {err}", path.display())); + // Dropping the pipe is what ends `cat`; the wait below would + // otherwise block on a writer that never sees end of file. + } + + let status = writer + .wait() + .unwrap_or_else(|err| panic!("the writer for {} must be waitable: {err}", path.display())); + assert!( + status.success(), + "the writer for {} must succeed, got {status}", + path.display() + ); + + // A chmod names the path and opens nothing, so it cannot reopen the + // window the write was handed off to close. + std::fs::set_permissions(path, std::fs::Permissions::from_mode(FAKE_MODE)) + .unwrap_or_else(|err| panic!("{} must be made executable: {err}", path.display())); +} + +#[cfg(test)] +mod tests { + use std::ffi::OsString; + use std::os::unix::ffi::OsStringExt; + use std::os::unix::fs::PermissionsExt; + use std::process::Command; + + use super::{FAKE_MODE, write_executable}; + + #[test] + fn a_written_fake_is_byte_identical_and_runnable() { + let dir = tempfile::tempdir().expect("tempdir"); + let fake = dir.path().join("fake"); + let script = b"#!/bin/sh\nprintf 'ran'\n"; + + write_executable(&fake, script); + + assert_eq!(std::fs::read(&fake).expect("the fake is readable"), script); + assert_eq!( + std::fs::metadata(&fake) + .expect("the fake exists") + .permissions() + .mode() + & 0o777, + FAKE_MODE + ); + let output = Command::new(&fake).output().expect("the fake must run"); + assert_eq!(output.stdout, b"ran"); + } + + /// The destination travels as an argument rather than in the script + /// text, so a name the shell would otherwise re-read — a quote, a + /// space, a `$` — lands at the path asked for rather than at + /// another one. + #[test] + fn a_hostile_file_name_lands_at_the_path_asked_for() { + let dir = tempfile::tempdir().expect("tempdir"); + let name = OsString::from_vec(br#"fa'ke $x "docker""#.to_vec()); + + write_executable(&dir.path().join(&name), b"#!/bin/sh\nexit 0\n"); + + assert_eq!(written_names(dir.path()), vec![name]); + } + + /// A Unix file name is bytes, not text, and `TMPDIR` may hold any + /// of them, so the destination must reach the writer as bytes too. + /// + /// The name is only creatable where the filesystem takes it: APFS + /// and other UTF-8-enforcing filesystems answer `EILSEQ`, and there + /// the property is unobservable rather than broken. + #[test] + fn a_non_utf8_file_name_lands_at_the_path_asked_for() { + let dir = tempfile::tempdir().expect("tempdir"); + let name = OsString::from_vec(b"fake\xffdocker".to_vec()); + let fake = dir.path().join(&name); + if std::fs::File::create(&fake).is_err() { + return; + } + + write_executable(&fake, b"#!/bin/sh\nexit 0\n"); + + assert_eq!(written_names(dir.path()), vec![name]); + } + + fn written_names(dir: &std::path::Path) -> Vec { + std::fs::read_dir(dir) + .expect("the directory is readable") + .map(|entry| entry.expect("the entry is readable").file_name()) + .collect() + } + + /// A fake is rewritten in place by some tests, so a second write + /// must leave the file holding the second script alone rather than + /// appending to the first. + #[test] + fn a_second_write_replaces_the_first() { + let dir = tempfile::tempdir().expect("tempdir"); + let fake = dir.path().join("fake"); + + write_executable(&fake, b"#!/bin/sh\nexit 1\n"); + write_executable(&fake, b"#!/bin/sh\nexit 0\n"); + + assert_eq!( + std::fs::read(&fake).expect("the fake is readable"), + b"#!/bin/sh\nexit 0\n" + ); + } +}