From 0f43f6c1f2f5523f16dab6858a3e276327376ac7 Mon Sep 17 00:00:00 2001 From: Greyson Parrelli Date: Thu, 6 Aug 2026 15:55:56 -0400 Subject: [PATCH] Hopefully fix some bugs. --- README.md | 4 ++- functions | 21 ++++++++---- install | 48 ++++++++++++++++------------ internal/authproxy/appconfig.go | 44 +++++++++++++++++++------ internal/authproxy/appconfig_test.go | 31 ++++++++++++++++++ internal/authproxy/server.go | 9 +++++- subcommands/report | 9 ++++-- test/access-list-test.sh | 35 ++++++++++++++++++++ 8 files changed, 159 insertions(+), 42 deletions(-) diff --git a/README.md b/README.md index 0f4df15..d932d70 100644 --- a/README.md +++ b/README.md @@ -313,7 +313,9 @@ and sign out at `https:///_google-auth/logout`. - **Websockets** work — upgrade headers are forwarded exactly like dokku's stock nginx template. - **Plugin updates:** `sudo dokku plugin:update google-auth` rebuilds the - service image and restarts the container automatically. + service image and recreates the container with freshly written settings. + `dokku google-auth:report` names the running service's state; 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 the config whenever you change an app's lists (and on every deploy), so this diff --git a/functions b/functions index d18d816..7102eb2 100644 --- a/functions +++ b/functions @@ -209,7 +209,7 @@ fn-ga-apply-list-change() { dokku_log_verbose "auth service is not running; changes apply when it starts" return 0 fi - if ! fn-ga-service-has-app-mount; then + if ! fn-ga-service-reads-app-lists; then dokku_log_info1 "recreating the auth service so it can read per-app lists" fn-ga-service-start return 0 @@ -615,12 +615,19 @@ fn-ga-service-running() { [[ "$(docker container inspect -f '{{.State.Running}}' "$GOOGLE_AUTH_SERVICE_NAME" 2>/dev/null)" == "true" ]] } -# True when the running container has the per-app config mount. A container -# started by an older version of this plugin will not, and would silently -# ignore per-app lists. -fn-ga-service-has-app-mount() { - docker container inspect -f '{{range .Mounts}}{{println .Destination}}{{end}}' \ - "$GOOGLE_AUTH_SERVICE_NAME" 2>/dev/null | grep -qxF "$GOOGLE_AUTH_APP_CONFIG_MOUNT" +# True when the running container can actually read per-app lists. That takes +# both halves: the bind mount, and the environment variable pointing at it. +# Either one alone makes the service ignore every per-app list and fall back to +# the global one — silently, and in the permissive direction — so both are +# checked. A container started by an older version of this plugin has neither; +# one recreated from a stale service.env has the mount without the variable. +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" } fn-ga-write-env-file() { diff --git a/install b/install index 3141f2c..8cfb3d5 100755 --- a/install +++ b/install @@ -8,30 +8,36 @@ DOKKU_LIB_ROOT=${DOKKU_LIB_ROOT:-/var/lib/dokku} DATA_ROOT="$DOKKU_LIB_ROOT/data/google-auth" mkdir -p "$DATA_ROOT/global" "$DATA_ROOT/apps" + +if command -v docker >/dev/null 2>&1; then + echo "-----> Building dokku-google-auth service image (first build can take a few minutes)" + docker image build -t dokku-google-auth:latest "$PLUGIN_DIR" + # Pick up the new image if the service is already running. This goes through + # fn-ga-service-start rather than its own `docker run` so an upgrade always + # rewrites service.env first: recreating the container from the file an older + # version left behind gives it stale settings — most damagingly a missing + # GOOGLE_AUTH_APP_CONFIG_DIR, which makes the service ignore every per-app + # access list and fall back to the global one. + if [[ "$(docker container inspect -f '{{.State.Running}}' dokku-google-auth 2>/dev/null)" == "true" ]]; then + echo "-----> Restarting google-auth service with the new image" + # shellcheck disable=SC1091 + source "$PLUGIN_DIR/functions" + if fn-ga-configured; then + fn-ga-service-start + else + echo " ! google-auth is not fully configured; the running service keeps the old image" 1>&2 + echo " ! run 'dokku google-auth:configure' then 'dokku google-auth:restart'" 1>&2 + fi + fi +else + echo " ! docker not found; the google-auth service image was not built" 1>&2 +fi + +# Ownership and modes last: the steps above can rewrite service.env as root, +# and the dokku user has to be able to rewrite it from then on. chown -R dokku:dokku "$DATA_ROOT" 2>/dev/null || true chmod 700 "$DATA_ROOT" # apps/ is bind-mounted into the service container, which runs as an # unprivileged uid and must be able to traverse it to read per-app access # lists. The 0700 on DATA_ROOT still keeps other host users out. chmod 711 "$DATA_ROOT/apps" - -if command -v docker >/dev/null 2>&1; then - echo "-----> Building dokku-google-auth service image (first build can take a few minutes)" - docker image build -t dokku-google-auth:latest "$PLUGIN_DIR" - # Pick up the new image if the service is already running. - if [[ "$(docker container inspect -f '{{.State.Running}}' dokku-google-auth 2>/dev/null)" == "true" ]]; then - echo "-----> Restarting google-auth service with the new image" - port="$(head -n1 "$DATA_ROOT/global/port" 2>/dev/null || true)" - port="${port:-2999}" - docker container rm -f dokku-google-auth >/dev/null 2>&1 || true - docker container run -d \ - --name dokku-google-auth \ - --restart=unless-stopped \ - -p "127.0.0.1:${port}:2999" \ - --env-file "$DATA_ROOT/service.env" \ - -v "$DATA_ROOT/apps:/data/apps:ro" \ - dokku-google-auth:latest >/dev/null - fi -else - echo " ! docker not found; the google-auth service image was not built" 1>&2 -fi diff --git a/internal/authproxy/appconfig.go b/internal/authproxy/appconfig.go index c2d4bfa..83bcee0 100644 --- a/internal/authproxy/appconfig.go +++ b/internal/authproxy/appconfig.go @@ -1,6 +1,9 @@ package authproxy import ( + "errors" + "io/fs" + "log" "os" "path/filepath" "strings" @@ -14,6 +17,13 @@ type accessLists struct { AllowedDomains []string AllowedEmails []string DeniedEmails []string + + // Unreadable records that one of the app's list files exists but could not + // be read, leaving its real rules unknown. Falling back to the global list + // would then silently widen access — the app is likely narrower than global, + // which is why it has a list at all — so an app in this state denies + // everyone until the file is readable again. + Unreadable bool } func (a accessLists) hasAllowRules() bool { @@ -64,26 +74,40 @@ func (s *appStore) lists(app string) accessLists { return hit.lists } - lists := accessLists{ - AllowedDomains: readDomainFile(filepath.Join(s.dir, app, "allowed-domains")), - AllowedEmails: readListFile(filepath.Join(s.dir, app, "allowed-emails")), - DeniedEmails: readListFile(filepath.Join(s.dir, app, "denied-emails")), + var lists accessLists + read := func(name string) []string { + entries, err := readListFile(filepath.Join(s.dir, app, name)) + if err != nil { + log.Printf("appconfig: cannot read %s for app %q: %v — denying every account "+ + "for that app until it is readable", name, app, err) + lists.Unreadable = true + } + return entries } + lists.AllowedDomains = trimAts(read("allowed-domains")) + lists.AllowedEmails = read("allowed-emails") + lists.DeniedEmails = read("denied-emails") + s.cache[app] = cachedLists{lists: lists, loadedAt: s.now()} return lists } -func readListFile(path string) []string { - // A missing or unreadable file is normal: most apps have no overrides. +// readListFile returns the entries in a list file. A missing file is normal — +// most apps have no overrides — and reads as an empty list; any other error is +// reported, because it means the app's rules are unknown rather than absent. +func readListFile(path string) ([]string, error) { raw, err := os.ReadFile(path) if err != nil { - return nil + if errors.Is(err, fs.ErrNotExist) { + return nil, nil + } + return nil, err } - return splitList(string(raw)) + return splitList(string(raw)), nil } -func readDomainFile(path string) []string { - entries := readListFile(path) +// trimAts accepts domains written either as "signal.org" or "@signal.org". +func trimAts(entries []string) []string { for i, d := range entries { entries[i] = strings.TrimPrefix(d, "@") } diff --git a/internal/authproxy/appconfig_test.go b/internal/authproxy/appconfig_test.go index a032cc6..3158912 100644 --- a/internal/authproxy/appconfig_test.go +++ b/internal/authproxy/appconfig_test.go @@ -53,6 +53,37 @@ func TestAppStoreReadsLists(t *testing.T) { } } +// A list file the service cannot read leaves the app's real rules unknown. +// Inheriting the global list there would hand out access the app's own list was +// written to withhold, so the app denies instead. +func TestUnreadableListDeniesInsteadOfInheriting(t *testing.T) { + if os.Geteuid() == 0 { + t.Skip("root reads files regardless of mode") + } + dir := t.TempDir() + writeAppLists(t, dir, "locked", map[string]string{"allowed-emails": "ceo@signal.org\n"}) + if err := os.Chmod(filepath.Join(dir, "locked", "allowed-emails"), 0o000); err != nil { + t.Fatal(err) + } + + s := newTestServer(t, "http://unused.invalid") // global list allows all of signal.org + s.cfg.AppConfigDir = dir + s.apps = newAppStore(dir) + + if !s.apps.lists("locked").Unreadable { + t.Error("an unreadable list file should mark the app's lists unreadable") + } + for _, email := range []string{"ceo@signal.org", "anyone@signal.org"} { + if s.emailAllowedFor("locked", email) { + t.Errorf("emailAllowedFor(locked, %q) = true; an app whose list cannot be read must deny", email) + } + } + // Only that app is affected: everyone else still uses the global list. + if !s.emailAllowedFor("inherits", "anyone@signal.org") { + t.Error("an app with no config of its own should still inherit the global list") + } +} + // The app name reaches the service in a header, so it must never be able to // walk out of the config directory. func TestAppStoreRejectsUnusableNames(t *testing.T) { diff --git a/internal/authproxy/server.go b/internal/authproxy/server.go index b37d58c..ca6bc24 100644 --- a/internal/authproxy/server.go +++ b/internal/authproxy/server.go @@ -313,6 +313,7 @@ func (s *Server) effectiveLists(app string) accessLists { AllowedDomains: s.cfg.AllowedDomains, AllowedEmails: s.cfg.AllowedEmails, DeniedEmails: s.cfg.DeniedEmails, + Unreadable: own.Unreadable, } if own.hasAllowRules() { out.AllowedDomains = own.AllowedDomains @@ -326,10 +327,16 @@ func (s *Server) effectiveLists(app string) accessLists { // emailAllowedFor applies app's rules: the deny list first (it always wins), // then the allow list, where an address must match an allowed email or an -// allowed domain. An empty app means "global rules only". +// allowed domain. An empty app means "global rules only"; an app whose own +// lists could not be read is denied outright rather than quietly handed the +// broader global list. func (s *Server) emailAllowedFor(app, email string) bool { email = strings.ToLower(email) lists := s.effectiveLists(app) + if lists.Unreadable { + // The app has rules we could not read; see accessLists.Unreadable. + return false + } if slices.Contains(lists.DeniedEmails, email) { return false } diff --git a/subcommands/report b/subcommands/report index 8e3ac67..b7a15f7 100755 --- a/subcommands/report +++ b/subcommands/report @@ -94,10 +94,15 @@ cmd-google-auth-report() { dokku_log_verbose "Denied emails: $(fn-ga-global-get-list denied-emails | paste -sd' ' -) (applies to every app)" dokku_log_verbose "Session TTL: $(fn-ga-global-get session-ttl 24h)" dokku_log_verbose "Service port: 127.0.0.1:$(fn-ga-global-get port "$GOOGLE_AUTH_DEFAULT_PORT")" - if fn-ga-service-running; then + if ! fn-ga-service-running; then + dokku_log_verbose "Service: not running" + elif fn-ga-service-reads-app-lists; then dokku_log_verbose "Service: running" else - dokku_log_verbose "Service: not running" + # 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)" fi local app diff --git a/test/access-list-test.sh b/test/access-list-test.sh index 7242edd..22a9a77 100755 --- a/test/access-list-test.sh +++ b/test/access-list-test.sh @@ -215,6 +215,41 @@ grep -q 'proxy_set_header X-Google-Auth-App "my-app";' "$APP_CONF" || fail "changing an app's list should rewrite its nginx config to stamp the app name" 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 +# from a service.env written before per-app lists existed has the mount but not +# the variable, ignores every per-app list, and still looks healthy — so the +# check must not be satisfied by the mount alone. +mkdir -p "$WORK/bin-docker" +cat >"$WORK/bin-docker/docker" <<'EOF' +#!/bin/sh +# Stands in for `docker container inspect`, replaying a canned inspection. +if [ "$1" = "container" ] && [ "$2" = "inspect" ]; then + cat "$DOCKER_INSPECT_FIXTURE" + exit 0 +fi +exit 0 +EOF +chmod +x "$WORK/bin-docker/docker" + +# Runs the check against one canned inspection, in a subshell so the stub and +# its fixture do not leak into the rest of the file. +reads_app_lists() ( + export DOCKER_INSPECT_FIXTURE="$WORK/inspect-fixture" + printf '%s\n' "$@" >"$DOCKER_INSPECT_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" || + fail "a container with both the mount and the env var should read per-app lists" +reads_app_lists "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" && + 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" +echo "ok: per-app list readiness check" + # --- 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"