Fix plugin updates.

README.md

@@ -314,7 +314,15 @@ and sign out at `https://<any-app>/_google-auth/logout`.
   stock nginx template.
 - **Plugin updates:** `sudo dokku plugin:update google-auth` rebuilds the
   service image and recreates the container with freshly written settings.
-  `dokku google-auth:report` names the running service's state; if it ever says
+  Editing the plugin directory in place works too: the image is tagged with a
+  fingerprint of its source, so `google-auth:restart` (and anything else that
+  starts the service) rebuilds when the Go code has moved rather than reusing
+  the binary that happens to be tagged. This matters because the shell half of
+  the plugin takes effect the moment the files change while the binary does
+  not, and a container running an older build accepts the mount, accepts
+  `GOOGLE_AUTH_APP_CONFIG_DIR`, and ignores both.
+  `dokku google-auth:report` names the running service's state — it asks the
+  service itself rather than trusting Docker's metadata — and if it ever says
   it is ignoring per-app lists, `dokku google-auth:restart` recreates it.
 - **An app's own lists need its nginx config to be current**, since that is
   what tells the service which app a request belongs to. The plugin rewrites

functions

@@ -210,7 +210,7 @@ fn-ga-apply-list-change() {
     return 0
   fi
   if ! fn-ga-service-reads-app-lists; then
-    dokku_log_info1 "recreating the auth service so it can read per-app lists"
+    dokku_log_info1 "recreating the auth service so it can read per-app lists (this rebuilds the image if the plugin was updated)"
     fn-ga-service-start
     return 0
   fi
@@ -275,7 +275,7 @@ fn-ga-app-set-enabled() {
   local dir
   dir="$(fn-ga-app-dir "$APP")"
   if [[ "$ENABLED" == "true" ]]; then
-    mkdir -p "$dir"
+    dir="$(fn-ga-app-dir-ensure "$APP")"
     touch "$dir/enabled"
   else
     rm -f "$dir/enabled"
@@ -326,9 +326,8 @@ fn-ga-validate-pattern() {
 fn-ga-exclude-add() {
   declare APP="$1" PATTERN="$2"
   local dir file
-  dir="$(fn-ga-app-dir "$APP")"
+  dir="$(fn-ga-app-dir-ensure "$APP")"
   file="$dir/excludes"
-  mkdir -p "$dir"
   touch "$file"
   grep -qxF "$PATTERN" "$file" || printf '%s\n' "$PATTERN" >>"$file"
 }
@@ -605,29 +604,103 @@ fn-ga-image-exists() {
   docker image inspect "$GOOGLE_AUTH_IMAGE" >/dev/null 2>&1
 }
 
+# Fingerprint of everything that ends up in the service image. The image is
+# labelled with it so an upgraded plugin rebuilds instead of reusing a binary
+# built from older source. Without this the shell half of the plugin upgrades
+# the moment the files change — new commands, new nginx config, new env var —
+# while the binary enforcing them stays whatever was built first, and the
+# mismatch is invisible from the outside.
+fn-ga-source-hash() {
+  local dir="$GOOGLE_AUTH_PLUGIN_DIR" file
+  {
+    while IFS= read -r file; do
+      printf '%s ' "${file#"$dir/"}"
+      sha256sum "$file" | awk '{print $1}'
+    done < <(
+      {
+        printf '%s\n' "$dir/Dockerfile" "$dir/go.mod"
+        find "$dir/cmd" "$dir/internal" -type f 2>/dev/null
+      } | LC_ALL=C sort
+    )
+  } | sha256sum | awk '{print $1}'
+}
+
+# True when the built image matches the plugin's current source.
+fn-ga-image-current() {
+  fn-ga-image-exists || return 1
+  local labelled
+  labelled="$(docker image inspect -f '{{index .Config.Labels "google-auth.source-hash"}}' "$GOOGLE_AUTH_IMAGE" 2>/dev/null)"
+  # Images built before this label existed report an empty value or "<no value>",
+  # and are stale by definition.
+  [[ -n "$labelled" && "$labelled" != "<no value>" ]] || return 1
+  [[ "$labelled" == "$(fn-ga-source-hash)" ]]
+}
+
 fn-ga-build-image() {
   command -v docker >/dev/null 2>&1 || dokku_log_fail "docker is required to build the google-auth service image"
   dokku_log_info1 "Building $GOOGLE_AUTH_IMAGE (first build downloads the Go toolchain image; this can take a few minutes)"
-  docker image build -t "$GOOGLE_AUTH_IMAGE" "$GOOGLE_AUTH_PLUGIN_DIR"
+  docker image build \
+    --label "google-auth.source-hash=$(fn-ga-source-hash)" \
+    -t "$GOOGLE_AUTH_IMAGE" "$GOOGLE_AUTH_PLUGIN_DIR"
 }
 
 fn-ga-service-running() {
   [[ "$(docker container inspect -f '{{.State.Running}}' "$GOOGLE_AUTH_SERVICE_NAME" 2>/dev/null)" == "true" ]]
 }
 
-# True when the running container can actually read per-app lists. That takes
+# Asks the running service which per-app config directory it is using, and
-# both halves: the bind mount, and the environment variable pointing at it.
+# prints it. This is the only check that sees the binary rather than the
-# Either one alone makes the service ignore every per-app list and fall back to
+# container around it; a build from before per-app lists existed answers with a
-# the global one — silently, and in the permissive direction — so both are
+# bare "ok" and names no directory. Fails if the service is unreachable or too
-# checked. A container started by an older version of this plugin has neither;
+# old to answer.
-# one recreated from a stale service.env has the mount without the variable.
+fn-ga-service-app-config-dir() {
+  command -v curl >/dev/null 2>&1 || return 1
+  local port body dir
+  port="$(fn-ga-global-get port "$GOOGLE_AUTH_DEFAULT_PORT")"
+  body="$(curl -fsS --max-time 3 "http://127.0.0.1:${port}${GOOGLE_AUTH_ROUTE_PREFIX}/healthz" 2>/dev/null)" || return 1
+  dir="$(sed -n 's/.*"app_config_dir":"\([^"]*\)".*/\1/p' <<<"$body")"
+  [[ -n "$dir" ]] || return 1
+  printf '%s' "$dir"
+}
+
+# True when the running service can actually read per-app lists. That takes
+# three things, and missing any of them makes the service ignore every per-app
+# list and fall back to the global one — silently, and in the permissive
+# direction — so all three are checked:
+#
+#   the bind mount             — a container from an older plugin has none;
+#   the env var naming it      — one recreated from a stale service.env has the
+#                                mount without the variable;
+#   a binary that reads both   — one recreated from a stale *image* has the
+#                                mount and the variable and ignores them, which
+#                                no amount of docker metadata can reveal. Only
+#                                the service's own answer distinguishes it.
 fn-ga-service-reads-app-lists() {
   local inspected
   inspected="$(docker container inspect \
     -f '{{range .Mounts}}mount={{println .Destination}}{{end}}{{range .Config.Env}}env={{println .}}{{end}}' \
     "$GOOGLE_AUTH_SERVICE_NAME" 2>/dev/null)" || return 1
   grep -qxF "mount=$GOOGLE_AUTH_APP_CONFIG_MOUNT" <<<"$inspected" || return 1
-  grep -qxF "env=GOOGLE_AUTH_APP_CONFIG_DIR=$GOOGLE_AUTH_APP_CONFIG_MOUNT" <<<"$inspected"
+  grep -qxF "env=GOOGLE_AUTH_APP_CONFIG_DIR=$GOOGLE_AUTH_APP_CONFIG_MOUNT" <<<"$inspected" || return 1
+
+  local reported
+  if reported="$(fn-ga-service-app-config-dir)"; then
+    [[ "$reported" == "$GOOGLE_AUTH_APP_CONFIG_MOUNT" ]]
+    return $?
+  fi
+  # No answer (curl missing, or the service is not reachable on the loopback
+  # port): fall back to checking that the image was built from this plugin's
+  # current source and that the container is running that image.
+  fn-ga-image-current && fn-ga-container-runs-current-image
+}
+
+# True when the running container was created from the current service image,
+# rather than from an earlier build still tagged over.
+fn-ga-container-runs-current-image() {
+  local running current
+  running="$(docker container inspect -f '{{.Image}}' "$GOOGLE_AUTH_SERVICE_NAME" 2>/dev/null)" || return 1
+  current="$(docker image inspect -f '{{.Id}}' "$GOOGLE_AUTH_IMAGE" 2>/dev/null)" || return 1
+  [[ -n "$running" && "$running" == "$current" ]]
 }
 
 fn-ga-write-env-file() {
@@ -637,8 +710,13 @@ fn-ga-write-env-file() {
   emails="$(fn-ga-global-get-list allowed-emails | paste -sd, -)"
   denied="$(fn-ga-global-get-list denied-emails | paste -sd, -)"
   mkdir -p "$GOOGLE_AUTH_DATA_ROOT"
-  umask 077
+  # The umask stays inside this subshell. Leaking it left every directory
-  cat >"$envfile" <<EOF
+  # created later in the same command at 0700 — including an app's config
+  # directory during google-auth:enable, which the service then could not
+  # traverse to read that app's lists.
+  (
+    umask 077
+    cat >"$envfile" <<EOF
 GOOGLE_AUTH_CLIENT_ID=$(fn-ga-global-get client-id)
 GOOGLE_AUTH_CLIENT_SECRET=$(fn-ga-global-get client-secret)
 GOOGLE_AUTH_COOKIE_SECRET=$(fn-ga-global-get cookie-secret)
@@ -652,11 +730,16 @@ GOOGLE_AUTH_SESSION_TTL=$(fn-ga-global-get session-ttl 24h)
 GOOGLE_AUTH_ALLOW_INSECURE=$(fn-ga-global-get allow-insecure false)
 GOOGLE_AUTH_LISTEN=:2999
 EOF
+  )
 }
 
 fn-ga-service-start() {
   fn-ga-configured || dokku_log_fail "google-auth is not fully configured; run: dokku google-auth:configure"
-  fn-ga-image-exists || fn-ga-build-image
+  # Rebuild whenever the source moved, not merely when the image is missing.
+  # Starting the service is how an operator expects to pick up an upgraded
+  # plugin, and reusing the old binary there is what makes a new feature look
+  # broken rather than absent.
+  fn-ga-image-current || fn-ga-build-image
   fn-ga-write-env-file
   local port
   port="$(fn-ga-global-get port "$GOOGLE_AUTH_DEFAULT_PORT")"

internal/authproxy/appconfig_test.go

@@ -203,6 +203,30 @@ func TestPerAppDenyCombinesWithGlobal(t *testing.T) {
 	}
 }
 
+// The plugin reads healthz to find out whether the running binary honours
+// per-app lists at all — docker metadata cannot tell a current container from
+// one recreated from a stale image. Changing this shape breaks that check.
+func TestHealthzReportsAppConfigDir(t *testing.T) {
+	s := newPerAppServer(t, nil)
+	s.cfg.AppConfigDir = "/data/apps"
+
+	r := httptest.NewRequest("GET", "http://"+appHost+RoutePrefix+"/healthz", nil)
+	w := do(s.Routes(), r)
+	if w.Code != http.StatusOK {
+		t.Fatalf("healthz = %d, want 200", w.Code)
+	}
+	if got, want := w.Body.String(), `{"ok":true,"app_config_dir":"/data/apps"}`; got != want {
+		t.Errorf("healthz body = %s, want %s", got, want)
+	}
+
+	// Per-app config off: the field is present but empty, so the plugin can
+	// tell "configured with no directory" from "too old to answer".
+	s.cfg.AppConfigDir = ""
+	if got, want := do(s.Routes(), r).Body.String(), `{"ok":true,"app_config_dir":""}`; got != want {
+		t.Errorf("healthz body with per-app config off = %s, want %s", got, want)
+	}
+}
+
 // The app name is read from a header, so verify must use the value nginx set
 // and a session must not be portable to an app with stricter rules.
 func TestVerifyUsesAppHeader(t *testing.T) {

internal/authproxy/server.go

@@ -255,9 +255,19 @@ func (s *Server) handleLogout(w http.ResponseWriter, r *http.Request) {
 		<p><a href="/">Sign in again</a></p>`)
 }
 
+// handleHealthz is both the container health probe and the plugin's way of
+// asking the running binary what it is doing. It reports the per-app config
+// directory because nothing outside the process can: a container recreated
+// from a stale image has the bind mount and GOOGLE_AUTH_APP_CONFIG_DIR in its
+// environment while running a binary from before per-app lists existed, which
+// ignores both and quietly falls back to the global lists. That build answers
+// this endpoint with a bare "ok" and no directory, which is what tells the two
+// apart.
 func (s *Server) handleHealthz(w http.ResponseWriter, _ *http.Request) {
+	w.Header().Set("Content-Type", "application/json")
+	w.Header().Set("Cache-Control", "no-store")
 	w.WriteHeader(http.StatusOK)
-	fmt.Fprint(w, "ok")
+	fmt.Fprintf(w, `{"ok":true,"app_config_dir":%q}`, s.cfg.AppConfigDir)
 }
 
 // handleStatus is a small human-readable page for debugging.

subcommands/report

@@ -102,7 +102,11 @@ cmd-google-auth-report() {
     # Worth calling out: every per-app list below is being ignored, and the
     # apps that have one are falling back to the broader global list.
     dokku_log_verbose "Service:         running, but IGNORING all per-app lists"
-    dokku_log_verbose "                 (started without the per-app config; fix with: dokku google-auth:restart)"
+    if ! fn-ga-image-current; then
+      dokku_log_verbose "                 (the service image is older than the plugin; fix with: dokku google-auth:restart)"
+    else
+      dokku_log_verbose "                 (started without the per-app config; fix with: dokku google-auth:restart)"
+    fi
   fi
 
   local app

test/access-list-test.sh

@@ -187,6 +187,17 @@ grep -qx "GOOGLE_AUTH_ALLOWED_EMAILS=guest@partner.com" "$ENV_FILE" || fail "env
 grep -qx "GOOGLE_AUTH_DENIED_EMAILS=former@signal.org" "$ENV_FILE" || fail "env file missing global denied emails"
 grep -qx "GOOGLE_AUTH_APP_CONFIG_DIR=/data/apps" "$ENV_FILE" || fail "env file must point the service at the mount"
 grep -q "ceo@signal.org" "$ENV_FILE" && fail "per-app entries must not be baked into the env file"
+[[ "$(stat -c '%a' "$ENV_FILE")" == "600" ]] || fail "the env file holds the client secret and must stay 0600"
+# Writing the env file tightens the umask to protect that secret. It must not
+# tighten anything else: google-auth:enable writes the env file (starting the
+# service) and then creates the app's config directory, which the unprivileged
+# service has to be able to traverse.
+fn-ga-app-set-enabled umask-app true
+[[ "$(stat -c '%a' "$DATA/apps/umask-app")" == "711" ]] ||
+  fail "a directory created after the env file must still be traversable by the container uid"
+fn-ga-exclude-add umask-app /api/hook
+[[ "$(stat -c '%a' "$DATA/apps/umask-app")" == "711" ]] ||
+  fail "adding an exclusion must leave the app directory traversable"
 echo "ok: service env file"
 
 # --- report shows both scopes, enabled or not ---
@@ -216,40 +227,89 @@ grep -q 'proxy_set_header X-Google-Auth-App "my-app";' "$APP_CONF" ||
 echo "ok: per-app list change refreshes the nginx config"
 
 # --- a running service only counts if it can really read per-app lists ---
-# It takes both the bind mount and the env var naming it. A container recreated
+# It takes the bind mount, the env var naming it, and a binary that reads both.
-# from a service.env written before per-app lists existed has the mount but not
+# Every combination that is missing one still looks healthy from outside while
-# the variable, ignores every per-app list, and still looks healthy — so the
+# quietly falling back to the global lists, so none of them may satisfy the
-# check must not be satisfied by the mount alone.
+# check.
 mkdir -p "$WORK/bin-docker"
 cat >"$WORK/bin-docker/docker" <<'EOF'
 #!/bin/sh
-# Stands in for `docker container inspect`, replaying a canned inspection.
+# Stands in for the docker CLI, replaying canned inspections.
-if [ "$1" = "container" ] && [ "$2" = "inspect" ]; then
+case "$1 $2" in
-  cat "$DOCKER_INSPECT_FIXTURE"
+  "container inspect") cat "$DOCKER_INSPECT_FIXTURE" ;;
-  exit 0
+  "image inspect") printf '%s\n' "$DOCKER_IMAGE_FIXTURE" ;;
-fi
+esac
 exit 0
 EOF
-chmod +x "$WORK/bin-docker/docker"
+cat >"$WORK/bin-docker/curl" <<'EOF'
+#!/bin/sh
+# Stands in for the healthz probe against the running service. An empty
+# fixture means the service did not answer.
+[ -s "$CURL_BODY_FIXTURE" ] || exit 7
+cat "$CURL_BODY_FIXTURE"
+EOF
+chmod +x "$WORK/bin-docker/docker" "$WORK/bin-docker/curl"
 
-# Runs the check against one canned inspection, in a subshell so the stub and
+# Runs the check against one canned healthz body and one canned container
-# its fixture do not leak into the rest of the file.
+# inspection, in a subshell so the stubs and fixtures do not leak into the rest
+# of the file.
+# shellcheck disable=SC2030,SC2031 # the subshell is what keeps the stubs local
 reads_app_lists() (
+  local healthz="$1"
+  shift
   export DOCKER_INSPECT_FIXTURE="$WORK/inspect-fixture"
+  export CURL_BODY_FIXTURE="$WORK/healthz-fixture"
+  export DOCKER_IMAGE_FIXTURE=""
   printf '%s\n' "$@" >"$DOCKER_INSPECT_FIXTURE"
+  printf '%s' "$healthz" >"$CURL_BODY_FIXTURE"
   PATH="$WORK/bin-docker:$PATH"
   fn-ga-service-reads-app-lists
 )
 
-reads_app_lists "mount=/data/apps" "env=GOOGLE_AUTH_APP_CONFIG_DIR=/data/apps" "env=GOOGLE_AUTH_CLIENT_ID=x" ||
+CURRENT_HEALTHZ='{"ok":true,"app_config_dir":"/data/apps"}'
-  fail "a container with both the mount and the env var should read per-app lists"
+# The reply from a build made before per-app lists existed.
-reads_app_lists "mount=/data/apps" "env=GOOGLE_AUTH_CLIENT_ID=x" &&
+LEGACY_HEALTHZ='ok'
+
+reads_app_lists "$CURRENT_HEALTHZ" "mount=/data/apps" "env=GOOGLE_AUTH_APP_CONFIG_DIR=/data/apps" "env=GOOGLE_AUTH_CLIENT_ID=x" ||
+  fail "a container with the mount, the env var, and a current binary should read per-app lists"
+# The case the plugin used to miss entirely: docker metadata is perfect because
+# the current shell created the container, but the image it started is old, so
+# the binary inside ignores the mount and every per-app list with it.
+reads_app_lists "$LEGACY_HEALTHZ" "mount=/data/apps" "env=GOOGLE_AUTH_APP_CONFIG_DIR=/data/apps" "env=GOOGLE_AUTH_CLIENT_ID=x" &&
+  fail "a container running a pre-per-app-lists binary must not count, however well configured"
+reads_app_lists '{"ok":true,"app_config_dir":"/somewhere/else"}' "mount=/data/apps" "env=GOOGLE_AUTH_APP_CONFIG_DIR=/data/apps" &&
+  fail "a service reading some other directory is not reading the mounted one"
+reads_app_lists "$CURRENT_HEALTHZ" "mount=/data/apps" "env=GOOGLE_AUTH_CLIENT_ID=x" &&
   fail "a container with the mount but no GOOGLE_AUTH_APP_CONFIG_DIR ignores per-app lists"
-reads_app_lists "env=GOOGLE_AUTH_APP_CONFIG_DIR=/data/apps" &&
+reads_app_lists "$CURRENT_HEALTHZ" "env=GOOGLE_AUTH_APP_CONFIG_DIR=/data/apps" &&
   fail "a container with the env var but no mount has nothing to read"
-reads_app_lists "" && fail "a container with neither should not count"
+reads_app_lists "$CURRENT_HEALTHZ" "" && fail "a container with neither should not count"
+# Unreachable service: with no answer to go on, an image that cannot be shown
+# to match the plugin's source is assumed stale rather than assumed good.
+reads_app_lists "" "mount=/data/apps" "env=GOOGLE_AUTH_APP_CONFIG_DIR=/data/apps" &&
+  fail "an unanswering service with an unverifiable image should not count"
 echo "ok: per-app list readiness check"
 
+# --- the built image has to match the source the plugin is running from ---
+# The shell half of the plugin upgrades as soon as the files change; the binary
+# only upgrades when the image is rebuilt. Reusing an image just because one
+# exists is what lets a per-app list be configured, reported, and ignored.
+# shellcheck disable=SC2030,SC2031 # the subshell is what keeps the stubs local
+image_current() (
+  export DOCKER_IMAGE_FIXTURE="$1"
+  PATH="$WORK/bin-docker:$PATH"
+  fn-ga-image-current
+)
+
+SOURCE_HASH="$(fn-ga-source-hash)"
+[[ -n "$SOURCE_HASH" ]] || fail "source hash should not be empty"
+[[ "$SOURCE_HASH" == "$(fn-ga-source-hash)" ]] || fail "source hash should be stable across calls"
+image_current "$SOURCE_HASH" || fail "an image labelled with the current source hash is current"
+image_current "0000000000000000" && fail "an image built from other source is not current"
+image_current "<no value>" && fail "an unlabelled image (built before this check) is not current"
+image_current "" && fail "an image with no label value is not current"
+echo "ok: service image tracks the plugin source"
+
 # --- lifecycle triggers carry per-app lists ---
 "$ROOT/post-app-rename" my-app renamed-app
 [[ ! -d "$DATA/apps/my-app" ]] || fail "rename should move the app's directory"