Merge pull request 'Make removing a name actually revoke it' (#25) from revoke-takes-effect into main

Reviewed-on: #25 Reviewed-by: branderson <brad@branderson.io>
2026-09-01 02:51:16 -07:00
parent 3489b5f167 f8366eccb5
commit 7df4f6ef35
3 changed files with 179 additions and 0 deletions
@@ -50,6 +50,9 @@ EOF
 die() { echo "$PROG: $*" >&2; exit 2; }
 say() { printf '%s\n' "$*" >&2; }

+# CFG holds every known key, so an unset one is empty rather than absent.
+cfg_or_default() { [ -n "${CFG[$1]:-}" ] && printf '%s\n' "${CFG[$1]}" || printf '%s\n' "$2"; }
+
 # --- config -----------------------------------------------------------------
 # Parsed, never sourced. This file sits on a CI host and names a token path; a
 # config format that executes is a config format that runs whatever an
@@ -477,7 +480,50 @@ case "$cmd" in
        if [ -f "${CFG[out]}" ] && cmp -s "$new" "${CFG[out]}"; then
            exit 0
        fi
+        # REMOVING A NAME DID NOT REVOKE ANYTHING. dnsmasq puts the addresses
+        # it answers with into the firewall's set, and nothing ever took them
+        # out: after a re-render the name stopped resolving, and every address
+        # it had ever resolved to stayed reachable until the host rebooted. A
+        # job that had already looked the name up, or that reads the address
+        # out of the declaration's history, connected to it as before.
+        #
+        # So a name being dropped means the set no longer describes the policy,
+        # and the only fix ipset offers is to empty it: the set holds addresses
+        # with no memory of which name put them there.
+        removed=""
+        if [ -f "${CFG[out]}" ]; then
+            removed=$(comm -23 \
+                <(sed -n 's|^server=/\([^/]*\)/.*|\1|p' "${CFG[out]}" | sort -u) \
+                <(sed -n 's|^server=/\([^/]*\)/.*|\1|p' "$new"          | sort -u))
+        fi
+
        install -m 0644 "$new" "${CFG[out]}"
+
+        # Order matters. Flush first, then reload: the reload is a SIGHUP,
+        # which clears dnsmasq's cache, so the next query for a name that is
+        # still allowed goes upstream and puts its address back. Reloading
+        # first would leave a cached answer that repopulates nothing, and names
+        # that are still on the list would be unreachable until their TTL ran
+        # out.
+        #
+        # Only on a removal. Flushing whenever anything changed would empty the
+        # set every time a name was added, and a job mid-run would find a
+        # destination it had already resolved refused until it looked it up
+        # again. Established connections survive either way - the firewall
+        # accepts ESTABLISHED,RELATED before it consults the set.
+        if [ -n "$removed" ]; then
+            set_name=$(cfg_or_default ipset ci-allow)
+            if command -v ipset >/dev/null 2>&1 && ipset list -n 2>/dev/null | grep -qx "$set_name"; then
+                ipset flush "$set_name" \
+                    && say "flushed $set_name: $(printf '%s\n' "$removed" | grep -c .) name(s) revoked" \
+                    || say "WARNING: could not flush $set_name - the revoked names' addresses stay reachable until it is emptied or the host reboots"
+            else
+                say "WARNING: $set_name does not exist here, so nothing was revoked."
+                say "  The names are gone from the resolver, but any address already"
+                say "  in the firewall's set stays reachable. Run dejima-ci-firewall."
+            fi
+        fi
+
        ${CFG[reload]}
        echo "allowlist updated: $(( $(wc -l < "${CFG[out]}") - 1 )) names"
        ;;
@@ -170,6 +170,15 @@ there is one.
 - **Exfiltration, and command-and-control.** Egress to the internet is unbounded
  from the VM, deliberately. The confined tier's allowlist bounds it for a job,
  and an escape to root on the VM is precisely what leaves that behind.
+- **One trusted project widening another's job network.** The allowlist is
+  rendered as the union of every declaration in `repos_file`, so a name added to
+  any of them is reachable from every project's jobs. It is bounded by who can
+  do it: the declaration is read from `main` of a repository you listed, and a
+  repository on that list is already trusted to run code on this host. So the
+  exposure is between trusted-but-separate projects, and to a dependency that
+  reaches one project's `main`, rather than to anyone who can open a pull
+  request. [Per-project allowlists](#rejected-with-reasons) says what isolating
+  them would take.
 - **The forge and the resolver.** Both are reachable from this VM on the ports
  listed, by anything on it. "No LAN access" is not literally true.
 - **The forge credential the runner registers with.** It is on the box, and
@@ -215,6 +224,31 @@ What to check, rather than re-argue:

 ## Rejected, with reasons

+### Per-project allowlists, for now
+
+Every job on the confined tier resolves against one list and is filtered by one
+ipset, so every project's declaration is reachable from every project's jobs.
+Isolating them is possible and is not yet done, which is a scheduling answer
+rather than a design one.
+
+What it takes, in the order the pieces constrain each other:
+
+- **A set per project.** The current set holds addresses with no memory of which
+  name put them there, and every bridge's rule matches the same one. So a
+  per-project *resolver* alone buys almost nothing: a project that cannot
+  resolve a name can still connect to the address another project's lookup put
+  in the set, and declarations are public. dnsmasq will populate several sets
+  per name (`ipset=/name/set-a,set-b`), so this part needs one resolver, not
+  one per project.
+- **A bridge per project**, because the only thing the firewall can tell about a
+  job is which network it came in on. That is the handle, and there is no other.
+- **A runner pool per project**, registered to that repository, so a pool only
+  ever receives jobs it has the network for.
+
+The cost is a network, a set and a runner registration per isolated project,
+which is why the likely shape is one pool per repository that actually declares
+domains and one shared pool for those that declare none.
+
 Recorded so they are not re-proposed without new information.

 **An approval gate before CI runs.** Not rejected as a weak control. There is
@@ -534,3 +534,102 @@ run_tool() { run "$BIN" "$@" --config "$T/config"; }
    run_tool render
    [[ "$output" == *"owner/repo: 2 name(s) declared"* ]]
 }
+# --- revoking a name --------------------------------------------------------
+#
+# dnsmasq puts the addresses it answers with into the firewall's set, and
+# nothing took them out again. After a re-render the name stopped resolving and
+# every address it had ever resolved to stayed reachable until the host
+# rebooted, so removing a domain from a declaration revoked nothing.
+
+stub_ipset() {
+    cat > "$T/stub/ipset" <<'IPSETEOF'
+#!/usr/bin/env bash
+case "$1" in
+    list) printf '%s\n' "${STUB_IPSET_NAMES:-ci-allow}" ;;
+    flush) printf '%s\n' "$2" >> "$STUB_DIR/flushed" ;;
+esac
+exit 0
+IPSETEOF
+    chmod +x "$T/stub/ipset"
+}
+
+@test "removing a name flushes the set, or it stays reachable by address" {
+    stub_ipset
+    mkdir -p "$STUB_DIR/owner"
+    printf 'keep.example.com\ndrop.example.com\n' > "$STUB_DIR/owner/repo.body"
+    run_tool render
+    [ "$status" -eq 0 ]
+
+    printf 'keep.example.com\n' > "$STUB_DIR/owner/repo.body"
+    run_tool render
+    [ "$status" -eq 0 ]
+    grep -qx ci-allow "$STUB_DIR/flushed"
+    [[ "$output" == *"1 name(s) revoked"* ]]
+}
+
+@test "adding a name does not flush, since nothing was revoked" {
+    # A flush on every change would empty the set whenever a name was added,
+    # and a job mid-run would find a destination it had already resolved
+    # refused until it looked it up again.
+    stub_ipset
+    mkdir -p "$STUB_DIR/owner"
+    printf 'one.example.com\n' > "$STUB_DIR/owner/repo.body"
+    run_tool render
+    printf 'one.example.com\ntwo.example.com\n' > "$STUB_DIR/owner/repo.body"
+    run_tool render
+    [ "$status" -eq 0 ]
+    [ ! -f "$STUB_DIR/flushed" ]
+}
+
+@test "an unchanged render flushes nothing and reloads nothing" {
+    stub_ipset
+    mkdir -p "$STUB_DIR/owner"
+    printf 'one.example.com\n' > "$STUB_DIR/owner/repo.body"
+    run_tool render
+    run_tool render
+    [ "$status" -eq 0 ]
+    [ ! -f "$STUB_DIR/flushed" ]
+}
+
+@test "a missing set is reported rather than passed over" {
+    # The names go out of the resolver either way, so this half-succeeds. An
+    # operator who is not told will believe a revocation took effect.
+    stub_ipset
+    mkdir -p "$STUB_DIR/owner"
+    printf 'keep.example.com\ndrop.example.com\n' > "$STUB_DIR/owner/repo.body"
+    run_tool render
+    printf 'keep.example.com\n' > "$STUB_DIR/owner/repo.body"
+    STUB_IPSET_NAMES="something-else" run_tool render
+    [[ "$output" == *"stays reachable"* ]]
+}
+
+@test "the flush happens before the reload, not after" {
+    # The reload is a SIGHUP, which clears dnsmasq's cache. Flushing after it
+    # would leave cached answers that repopulate nothing, so names still on the
+    # list would be unreachable until their TTL ran out.
+    stub_ipset
+    cat > "$T/stub/reloader" <<'RELEOF'
+#!/usr/bin/env bash
+printf 'reload\n' >> "$STUB_DIR/order"
+RELEOF
+    chmod +x "$T/stub/reloader"
+    cat > "$T/stub/ipset" <<'IPSETEOF'
+#!/usr/bin/env bash
+case "$1" in
+    list) printf 'ci-allow\n' ;;
+    flush) printf 'flush\n' >> "$STUB_DIR/order" ;;
+esac
+exit 0
+IPSETEOF
+    chmod +x "$T/stub/ipset"
+    sed -i "s|^reload = .*|reload = $T/stub/reloader|" "$T/config"
+
+    mkdir -p "$STUB_DIR/owner"
+    printf 'keep.example.com\ndrop.example.com\n' > "$STUB_DIR/owner/repo.body"
+    run_tool render
+    : > "$STUB_DIR/order"          # the first render installs and reloads too
+    printf 'keep.example.com\n' > "$STUB_DIR/owner/repo.body"
+    run_tool render
+    [ "$(head -1 "$STUB_DIR/order")" = flush ]
+    [ "$(tail -1 "$STUB_DIR/order")" = reload ]
+}