Merge pull request 'Check the guard can read its own key, and say so when it cannot' (#10) from key-readable into main

Reviewed-on: #10
Reviewed-by: branderson <brad@branderson.io>

Dockerfile

@@ -20,8 +20,15 @@ RUN apt-get update \
 # A uid of its own. It needs no privilege: it opens outbound connections and
 # writes one directory. Running as root would be granting root to a program
 # holding an SSH key to another machine, for no reason at all.
-RUN useradd --system --create-home --home-dir /var/lib/dejima-guard \
-            --shell /usr/sbin/nologin --uid 10001 guard
+#
+# 10001 IS PART OF THE INTERFACE, not an implementation detail, and the gid is
+# pinned rather than left to whatever useradd picks. Compose bind-mounts a file
+# secret with the host's ownership and mode, so the key has to be chown'd to
+# this uid on the host - a number that moved between builds would break every
+# deployment that had done so.
+RUN groupadd --system --gid 10001 guard \
+ && useradd --system --create-home --home-dir /var/lib/dejima-guard \
+            --shell /usr/sbin/nologin --uid 10001 --gid 10001 guard
 
 COPY bin/dejima-guard bin/dejima-guard-loop bin/dejima-guard-install-key /usr/local/bin/
 RUN chmod 0755 /usr/local/bin/dejima-guard \

README.md

@@ -144,8 +144,21 @@ compose will refuse to start with a secret it cannot find.
 
 ```sh
 chmod 600 secrets/guard_key secrets/config
+sudo chown 10001:10001 secrets/guard_key secrets/config
 ```
 
+**The `chown` is not optional, and it is the first thing that goes wrong
+without it.** The image runs as uid 10001 rather than root, and compose mounts
+a file secret with the host's ownership and mode — so a key that is `0600` and
+yours is a key the container cannot open. ssh reports that as
+`Permission denied (publickey,password)`, which looks exactly like the devbox
+rejecting the key, and sends you to the far end to debug a file on this one.
+`dejima-guard` checks the key itself before connecting and says so plainly, but
+the `chown` is what fixes it.
+
+10001 is fixed in the `Dockerfile` and is part of the interface, so it is safe
+to write down here.
+
 **5. Start it, then prove the alert path works.**
 
 ```sh

bin/dejima-guard

@@ -250,6 +250,39 @@ require_config() {
     case "$h" in
         CHANGE-ME*) die "host is still the example placeholder ($h) - edit your config" ;;
     esac
+    # The key is checked here, where the answer is certain, rather than left to
+    # ssh - which reports a key it could not open as
+    # "Permission denied (publickey,password)", indistinguishable from a devbox
+    # that rejected it. That sends an operator to the far end to debug a file on
+    # this one.
+    #
+    # Containers make it the expected first failure rather than an unlikely one.
+    # This image runs as its own uid, compose bind-mounts a file secret with the
+    # host's ownership and mode, and a private key is 0600 to somebody else - so
+    # a correctly built deployment cannot read its own key until the ownership
+    # is made to match.
+    local k
+    k=$(cfg ssh_key)
+    if [ -n "$k" ]; then
+        [ -e "$k" ] || die "ssh_key $k does not exist"
+        if [ ! -r "$k" ]; then
+            say "$PROG: cannot read ssh_key $k"
+            say "  this runs as uid $(id -u); the file is uid $(stat -c %u "$k" 2>/dev/null || echo '?')," \
+                "mode $(stat -c %a "$k" 2>/dev/null || echo '?')"
+            say ""
+            # Phrased around the uid this is actually running as, which is the
+            # right answer whether that is a container's own account or an
+            # operator's. Claiming "the image runs as" would be false half the
+            # time, in a message whose only job is to be believed.
+            say "  The key has to be readable by the account this runs as. In a"
+            say "  container that is not yours: compose mounts the secret with"
+            say "  the host's ownership. On the host, beside your compose file:"
+            say ""
+            say "    sudo chown $(id -u):$(id -g) secrets/guard_key secrets/config"
+            exit 2
+        fi
+    fi
+
     local u
     for u in ntfy_url heartbeat_url; do
         [ -n "$(cfg "$u")" ] && check_url "$u" "$(cfg "$u")"

test/guard.bats

@@ -321,7 +321,12 @@ printf '%s\n' "$*" >> "$DEJIMA_GUARD_STATE/ssh.log"
 case "$*" in *"drift --state"*) echo "FILE aaa /etc/one" ;; esac
 EOF
     chmod +x "$STUB/ssh"
-    DEJIMA_GUARD_SSH_KEY=/var/lib/dejima-guard/id_ed25519 run_watch check
+    # A real file, because the key is checked for existence and readability
+    # before anything connects - an unreadable one is the expected first
+    # failure of a container deployment and is reported here rather than left
+    # to look like the devbox rejecting it.
+    : > "$BATS_TEST_TMPDIR/key"; chmod 600 "$BATS_TEST_TMPDIR/key"
+    DEJIMA_GUARD_SSH_KEY="$BATS_TEST_TMPDIR/key" run_watch check
     grep -q 'IdentitiesOnly=yes' "$DEJIMA_GUARD_STATE/ssh.log"
 }
 
@@ -569,3 +574,63 @@ keyline() { awk '{print $1, $2}' "$BATS_TEST_TMPDIR/hk_$1.pub"; }
     [ "$status" -ne 0 ]
     [ ! -s "$DEJIMA_GUARD_SSH_KNOWN_HOSTS" ]
 }
+
+# --- the key this machine holds ----------------------------------------------
+#
+# Containers make an unreadable key the expected first failure rather than an
+# unlikely one: the image runs as its own uid, compose mounts a file secret with
+# the host's ownership, and a private key is 0600 to somebody else. So a
+# correctly built deployment cannot read its own key until the ownership is
+# made to match, and the whole setup looks like the devbox refusing it.
+
+@test "an unreadable key is reported here, not left to look like a rejection" {
+    # ssh calls it "Permission denied (publickey,password)", which is what a
+    # devbox that rejected the key says too - so the operator debugs the far
+    # end over a file on this one.
+    key="$BATS_TEST_TMPDIR/unreadable_key"
+    : > "$key"; chmod 000 "$key"
+    export DEJIMA_GUARD_SSH_KEY="$key"
+    stub_ssh "FILE /etc/passwd aaaa" 0
+    run_watch check
+    [ "$status" -ne 0 ]
+    [[ "$output" == *"cannot read ssh_key"* ]]
+}
+
+@test "an unreadable key names the chown that fixes it" {
+    key="$BATS_TEST_TMPDIR/unreadable_key"
+    : > "$key"; chmod 000 "$key"
+    export DEJIMA_GUARD_SSH_KEY="$key"
+    stub_ssh "" 0
+    run_watch check
+    [[ "$output" == *"chown"* ]]
+}
+
+@test "a key that does not exist is named, rather than reported as a devbox fault" {
+    export DEJIMA_GUARD_SSH_KEY="$BATS_TEST_TMPDIR/no-such-key"
+    stub_ssh "" 0
+    run_watch check
+    [ "$status" -ne 0 ]
+    [[ "$output" == *"does not exist"* ]]
+}
+
+@test "a bad key stops the cycle before it can alert about the devbox" {
+    # A file on this machine is not a finding about that one. Reporting it as
+    # one sends the operator to the wrong host, and the silence that follows is
+    # what the dead-man's switch is for.
+    key="$BATS_TEST_TMPDIR/unreadable_key"
+    : > "$key"; chmod 000 "$key"
+    export DEJIMA_GUARD_SSH_KEY="$key"
+    stub_ssh "" 0
+    run_watch check
+    [ "$(alerts)" -eq 0 ]
+    [ "$(heartbeats)" -eq 0 ]
+}
+
+@test "the uid the README tells you to chown to is the one the image runs as" {
+    # Two files, one number. A uid that moved between them would produce
+    # exactly the failure this documents, with the documentation insisting it
+    # had been dealt with.
+    uid=$(grep -oE '\-\-uid [0-9]+' "$REPO/Dockerfile" | awk '{print $2}')
+    [ -n "$uid" ]
+    grep -q "chown $uid:$uid" "$REPO/README.md"
+}