ausimian
diff --git a/‎AGENTS.md‎
Lines changed: 72 additions & 39 deletions b/‎AGENTS.md‎
Lines changed: 72 additions & 39 deletions
diff --git a/‎lib/castle/peer.ex‎
Lines changed: 81 additions & 44 deletions b/‎lib/castle/peer.ex‎
Lines changed: 81 additions & 44 deletions
@@ -97,14 +97,33 @@ Castle's job is configuration and release management on a running node.
   and removing it if it is not. `plan/2` is the one place the two file paths are
   decided, and it puts both inside that directory under the names they will have
   when they leave. Each is created **exclusively**, so a name already at the
-  path is refused rather than followed or truncated.
-
-  **The rule is create, narrow, then verify.** The verification is not
-  decoration and it is not defence in depth: it is there because five successive
-  attempts to reason about whether a window was harmless were all wrong, and a
-  sixth judgement of the same kind is worth nothing. Do not remove it on the
-  grounds that you can see why the window is safe. That is precisely the
-  sentence that preceded each of the previous five findings.
+  path is refused rather than followed or truncated, and each is **written
+  through the handle that exclusive open returned**.
+
+  **Never reopen a name to place content.** The exclusive open's whole value is
+  that it establishes the name did not exist; closing the handle and reopening
+  the same name by path throws that away, and anything able to create the name
+  in between is handed the configuration. `write_private/2` therefore creates
+  and fills in one movement — there is deliberately no way here to bring one of
+  these files into existence without also placing its content, because
+  separating the two is what let a reopen back in.
+
+  The `chmod` is the one step that still goes by path, because OTP has nothing
+  that sets a mode on an open file: `:file.change_mode/2` and
+  `:file.write_file_info/2` take a name and reject a handle (`:badarg` and
+  `:function_clause`). That is an accepted asymmetry rather than an oversight.
+  By the time it runs the content is already committed to the inode the open
+  created, so a name swapped underneath it does not receive the configuration —
+  it gets narrowed, which is Castle setting some other file to 0600 inside a
+  directory it verified empty and made 0700. A nuisance, not a disclosure.
+
+  **The rule is create, narrow, then verify — and write through the handle you
+  created.** The verification is not decoration and it is not defence in depth:
+  it is there because five successive attempts to reason about whether a window
+  was harmless were all wrong, and a sixth judgement of the same kind is worth
+  nothing. Do not remove it on the grounds that you can see why the window is
+  safe. That is precisely the sentence that preceded each of the previous five
+  findings.
 
   The particular argument it replaced, so nobody reconstructs it: an empty
   directory has nothing behind the window, and permission to traverse a
@@ -145,30 +164,34 @@ Castle's job is configuration and release management on a running node.
   `mkdir` and ignored the `chmod`, where none of this can be honoured and the
   operator's own mode on `sys.config` would not be either.
 
-  Inside the directory the file is still created owner-only at 0600, filled
-  through that, and given the model's mode **last** — `write_like/3` is the two
-  of those together, for a file written once. Not the other way round, which is
-  the obvious reading and is wrong: a `sys.config` at 0440 is an operator
-  declaring their configuration read-only, and a file chmodded to 0440 before
-  being filled cannot be filled. `File.write/2` reopens the path rather than
-  writing through a handle held from creation, so the fill fails `:eacces`
-  against a file its own owner has just made read-only, and the install stops.
-  Do not "simplify" the ordering back.
-
-  0600 satisfies both constraints at once rather than trading between them: it
-  grants nothing to group or other, so every transient state is *narrower* than
-  the destination rather than merely different from it, and it leaves the file
-  writable by its owner while there is writing to do. For the scratch that is
-  until the last of three writes — this module fills it, the peer's pipeline
-  writes the resolved configuration over it, this module writes it again — so
-  the mode goes on after all of them, immediately before the rename. A failure
-  part-way leaves the file narrower than intended, never wider. It is no longer
-  the 0600 that closes the window — the directory is — so treat it as the belt to
-  that braces, and keep it. The two operations that move one of these files into
-  place, the link that publishes the base and the rename that replaces
-  `sys.config`, need permission on the directories rather than on the file, so a
-  restrictive mode never has to be relaxed again; nor does removing what is left
-  in the working directory afterwards.
+  The file is given 0600 on creation and the model's mode **last** —
+  `write_like/3` is `write_private/2` plus that, for a file written once. The
+  ordering matters for the writes that come *after*, not for the first one: a
+  `sys.config` at 0440 is an operator declaring their configuration read-only,
+  and the scratch is written twice more after Castle creates it — the peer's
+  pipeline writes the resolved configuration over it, then this module writes it
+  again — with both of those reopening the name, and a file at 0440 cannot be
+  reopened for writing. So the model's mode goes on last of all, immediately
+  before the rename, and a failure part-way leaves the file narrower than
+  intended rather than wider. Do not "simplify" the ordering back.
+
+  **The scratch's later writes rest on the directory, not on the handle.** One of
+  them is `Config.Provider.write_config!`, which is `File.write/2` in Elixir's
+  own code, in the peer's VM — driving Elixir's pipeline is what this module is
+  for, so that is not ours to change and must not be worked around. Nor may the
+  creation handle be held open across the peer's run to cover them: the peer
+  writes by name in a VM of its own, and a handle kept here would go on pointing
+  at whichever inode the name had when it was opened. Elixir truncates the same
+  inode today; one that wrote a temporary file and renamed it would leave the
+  handle on an orphan and the configuration written through it would silently be
+  nobody's. What protects those writes is that the directory was verified empty,
+  is 0700, and holds only names Castle created.
+
+  The two operations that move one of these files into place, the link that
+  publishes the base and the rename that replaces `sys.config`, need permission
+  on the directories rather than on the file, so a restrictive mode never has to
+  be relaxed again; nor does removing what is left in the working directory
+  afterwards.
 
   `File.write/2` creates with the process umask and never looks at a mode.
   `File.cp/2` *does* carry the mode — and was adopted here for that reason — but
@@ -380,13 +403,23 @@ commit — and the other once with the environment as it ended up. The two
 its symlinks idempotently: a root has to be able to hold two versions.
 
 `Castle.Peer.work_dir/1`, `secure_dir/1`, `write_like/3`, `write_private/2`,
-`create_private/1` and `publish/2` are public for the same kind of reason: what
-they guarantee is about *intermediate* states, and a window nothing can stand in
-is a window nothing can test. One test takes `work_dir/1`, `write_like/3` and
-`publish/2` one at a time and looks at the destination in between — where it finds
-no file, rather than a partial one — then checks that publishing again is refused
-rather than allowed to replace. Another calls `work_dir/1` and finds the
-directory at 0700 and still empty.
+`create_exclusive/1`, `fill/3` and `publish/2` are public for the same kind of
+reason: what they guarantee is about *intermediate* states, and a window nothing
+can stand in is a window nothing can test. One test takes `work_dir/1`,
+`write_like/3` and `publish/2` one at a time and looks at the destination in
+between — where it finds no file, rather than a partial one — then checks that
+publishing again is refused rather than allowed to replace. Another calls
+`work_dir/1` and finds the directory at 0700 and still empty. Call sites use
+`write_private/2`; `create_exclusive/1` and `fill/3` are public only so that the
+window between them can be stood in, and never to be called in sequence by
+anything else.
+
+`fill/3` being separable is what lets a test swap the name between the exclusive
+open and the write, and assert that the content reached the inode that was
+created while the file the name now points at never saw it — with that same test
+asserting the acknowledged cost, that the by-path `chmod` does land on the
+swapped name. With the name left alone the two behaviours are identical, so
+there is no other way to tell them apart.
 
 `secure_dir/1` is public so that the `mkdir`-to-`chmod` window can be stood in:
 a test creates a directory at 0777, plants a `sys.config` symlink inside it, and
 
@@ -658,27 +658,44 @@ defmodule Castle.Peer do
   # the `mkdir` and ignored the `chmod`, where nothing here can be honoured and
   # the operator's mode on `sys.config` would not be honoured either.
   #
-  # Inside the directory the file is still created owner-only, filled through
-  # that, and given the model's mode last. Not the other way round, which is the
-  # obvious reading of "set the mode before there is anything to read" and is
-  # wrong: a `sys.config` at 0440 is an operator declaring their configuration
-  # read-only, and a file chmodded to 0440 before being filled cannot be filled.
-  # `File.write/2` reopens the path rather than writing through a handle held
-  # from creation, so the fill fails with `:eacces` and the install stops. Do not
-  # "simplify" the ordering back.
+  # Inside the directory the bytes go through the handle the exclusive open
+  # returned, and the name is never reopened to place content. That is the point
+  # of opening `:exclusive` rather than a refinement of it: the exclusive open
+  # *establishes* that the name did not exist, and closing the handle to reopen
+  # the same name by path throws that proof away - whatever can create the name
+  # in between is handed the content. Written through the handle, the content can
+  # only ever reach the inode this call created, whatever becomes of the name
+  # afterwards.
   #
-  # 0600 satisfies both constraints at once rather than trading between them. It
-  # grants nothing at all to group or other, so every intermediate state is
-  # narrower than the destination rather than merely different from it, and it
-  # leaves the file writable by its owner for as long as there is writing to do.
-  # Which, for the scratch, is until the last of three writes: this module fills
-  # it, the peer's pipeline writes the resolved configuration over it, and this
-  # module writes it again. The model's mode goes on after all of that, as the
-  # last thing done before the file leaves the working directory under its final
-  # name - which is also why a failure part-way through leaves it narrower than
-  # intended rather than wider. It is no longer the 0600 that closes the window,
-  # the directory is; what it buys now is that a file's own mode is never the
-  # umask's choice, at the cost of one `chmod`.
+  # The mode still goes on by path, because OTP has nothing that sets a mode on
+  # an open file: `:file.change_mode/2` and `:file.write_file_info/2` take a name
+  # and reject a handle - `:badarg` and `:function_clause` respectively. That
+  # asymmetry is deliberate, and it is cheap. By the time the `chmod` runs the
+  # content is already committed to this call's inode, so a name swapped
+  # underneath it does not receive the configuration; it gets narrowed. The worst
+  # it buys is that Castle sets somebody else's file to 0600, inside a directory
+  # it verified empty and made 0700. A nuisance, not a disclosure.
+  #
+  # The mode goes on *after* the content rather than before, and that ordering is
+  # about the writes which come later rather than this one. A `sys.config` at
+  # 0440 is an operator declaring their configuration read-only, and the scratch
+  # is written twice more after this: the peer's pipeline writes the resolved
+  # configuration over it, and this module writes it again. Both of those reopen
+  # the name - `Config.Provider.write_config!` is a `File.write/2` in Elixir's
+  # own code, and Elixir's pipeline is what this module exists to drive - and a
+  # file at 0440 cannot be reopened for writing. So the model's mode goes on last
+  # of all, immediately before the file leaves under its final name, which is
+  # also why a failure part-way through leaves it narrower than intended rather
+  # than wider. Do not "simplify" the ordering back.
+  #
+  # Holding this handle open across the peer's run would not extend the
+  # guarantee to those two writes, and must not be tried. The peer writes in a VM
+  # of its own and by name, while a handle kept here would go on pointing at
+  # whichever inode the name had when it was opened: today Elixir truncates the
+  # same one, but an Elixir that wrote a temporary file and renamed it would
+  # leave this pointing at an orphan, and the configuration written through it
+  # would silently be nobody's. What those two writes rest on is the directory -
+  # verified empty, 0700, and holding only names this call created.
   #
   # Neither of the obvious primitives has any of this. `File.write/2` creates
   # with the process umask and never looks at a mode. `File.cp/2` does carry the
@@ -688,12 +705,16 @@ defmodule Castle.Peer do
   #
   # What is not reproduced is ownership - only the mode bits are. See AGENTS.md.
   #
-  # `work_dir/1` is public along with `write_like/3`, `write_private/2`,
-  # `create_private/1` and `publish/2`, because the intermediate states are the
-  # point: a mode that is only ever correct once the content is written looks
-  # exactly like a mode that was correct all along, a directory that was secured
-  # after it was filled looks exactly like one secured before, and a window
-  # nothing can stand in is a window nothing can test.
+  # `work_dir/1` is public along with `secure_dir/1`, `write_like/3`,
+  # `write_private/2`, `create_exclusive/1`, `fill/3` and `publish/2`, because the
+  # intermediate states are the point: a mode that is only ever correct once the
+  # content is written looks exactly like a mode that was correct all along, a
+  # directory that was secured after it was filled looks exactly like one secured
+  # before, content that went to a reopened name looks exactly like content that
+  # went to the handle it was created with, and a window nothing can stand in is a
+  # window nothing can test. `write_private/2` is what call sites use; the two it
+  # is made of are public so that the window between them can be stood in, and
+  # for no other reason.
 
   # Created rather than ensured: a name that is already there is not adopted, and
   # `File.mkdir/1` is what refuses it - measured against a directory, a regular
@@ -758,43 +779,59 @@ defmodule Castle.Peer do
     with :ok <- write_private(path, bytes), do: carry_mode(model, path)
   end
 
+  # Created and filled in one movement, because separating the two is what put a
+  # reopened name between them. There is no way to bring one of these files into
+  # existence here without also placing its content.
   @doc false
   @spec write_private(Path.t(), iodata()) :: :ok | {:error, String.t()}
   def write_private(path, bytes) do
     with :ok <- private_dir(Path.dirname(path)),
-         :ok <- create_private(path) do
-      write(path, bytes)
+         {:ok, handle} <- create_exclusive(path) do
+      fill(handle, path, bytes)
     end
   end
 
   # Exclusively, so that a name already at this path is refused rather than
   # followed or truncated. `File.write/2` here would do neither safely: pointed at
   # a symlink it truncates what the link points at, chmods *that* to 0600, and
   # fills it with the configuration - and creates the target outright if the link
-  # dangles. All three measured. The mode still goes on afterwards, because there
-  # is still no way to ask for it at creation.
+  # dangles. All three measured. What the open returns is the handle the content
+  # goes through, and the caller's obligation is to `fill/3` it: that is what
+  # keeps the proof this open just established.
   @doc false
-  @spec create_private(Path.t()) :: :ok | {:error, String.t()}
-  def create_private(path) do
-    with :ok <- create_exclusive(path), do: chmod(path, 0o600)
-  end
-
-  defp create_exclusive(path) do
+  @spec create_exclusive(Path.t()) :: {:ok, File.io_device()} | {:error, String.t()}
+  def create_exclusive(path) do
     case File.open(path, [:write, :exclusive, :raw]) do
-      {:ok, handle} -> close(path, handle)
+      {:ok, handle} -> {:ok, handle}
       {:error, reason} -> {:error, "Cannot create #{path}. #{format_error(reason)}"}
     end
   end
 
-  # Closed rather than written through, because what is written to these files is
-  # written more than once and by more than this module - the peer's own pipeline
-  # writes the scratch too - so no handle could serve all of it. Nothing can reach
-  # the path between the close and the write: the directory holding it has been
-  # verified private.
-  defp close(path, handle) do
+  # The content through the handle, the mode by path, and the handle closed
+  # whichever way the write went. A write error is reported ahead of a close
+  # error, being the one that says what actually happened, and the mode is set
+  # only once both have succeeded - so a file that was not written in full is
+  # never given the mode that says it was.
+  @doc false
+  @spec fill(File.io_device(), Path.t(), iodata()) :: :ok | {:error, String.t()}
+  def fill(handle, path, bytes) do
+    written = written(handle, path, bytes)
+    closed = closed(handle, path)
+
+    with :ok <- written, :ok <- closed, do: chmod(path, 0o600)
+  end
+
+  defp written(handle, path, bytes) do
+    case IO.binwrite(handle, bytes) do
+      :ok -> :ok
+      {:error, reason} -> {:error, "Cannot write #{path}. #{format_error(reason)}"}
+    end
+  end
+
+  defp closed(handle, path) do
     case File.close(handle) do
       :ok -> :ok
-      {:error, reason} -> {:error, "Cannot create #{path}. #{format_error(reason)}"}
+      {:error, reason} -> {:error, "Cannot write #{path}. #{format_error(reason)}"}
     end
   end