From 3922aae9a7d5cf911a2f9d75a1ca6879c72eaf06 Mon Sep 17 00:00:00 2001 From: Minseong Choi Date: Tue, 29 Sep 2026 13:39:35 +0900 Subject: [PATCH] fix(api): require reauthentication for changes to own email and passkeys; update OpenAPI descriptions --- deploy/bootstrap.sh | 18 +++---- deploy/bootstrap_test.sh | 43 +++++++++++++++-- docs/openapi.yaml | 8 +++- internal/api/handlers_users.go | 10 ++++ internal/api/handlers_users_test.go | 73 +++++++++++++++++++++++++++++ panel/src/lib/openapi.gen.ts | 12 ++++- 6 files changed, 149 insertions(+), 15 deletions(-) diff --git a/deploy/bootstrap.sh b/deploy/bootstrap.sh index f80b717..01cf110 100644 --- a/deploy/bootstrap.sh +++ b/deploy/bootstrap.sh @@ -3438,16 +3438,18 @@ atomic_install_file() { mv -fT "$staged" "$target" } -# install_if_changed is atomic_install_file that leaves a target with the same bytes in -# place, fixing only its mode and owner, so the target's mtime keeps meaning "the content -# changed". felis domain check reads a proxy started before felis-link.properties' mtime -# as one still on the old names, and a re-run that rewrote the same bytes made every -# install look behind (and `felis domain set` restart the proxy for nothing). +# install_if_changed is atomic_install_file that leaves the target alone when it already +# has the same bytes, owner and mode, so its mtime keeps meaning "the content changed". +# felis domain check reads a proxy started before felis-link.properties' mtime as one +# still on the old names, and a re-run that rewrote the same bytes made every install look +# behind (and `felis domain set` restart the proxy for nothing). +# It never fixes a target in place: the proxy's account owns these directories and can +# swap the file for a symlink after the checks, and a chown or chmod by path would follow +# it to, say, k3s.yaml. Owner and group compare by name, as the callers pass them. install_if_changed() { local source="$1" target="$2" mode="$3" owner="$4" group="$5" - if [ -f "$target" ] && [ ! -L "$target" ] && cmp -s "$source" "$target"; then - chown "${owner}:${group}" "$target" - chmod "$mode" "$target" + if [ -f "$target" ] && [ ! -L "$target" ] && cmp -s "$source" "$target" \ + && [ "$(stat -c '%U:%G %a' "$target")" = "${owner}:${group} ${mode#0}" ]; then return 0 fi atomic_install_file "$@" diff --git a/deploy/bootstrap_test.sh b/deploy/bootstrap_test.sh index bae45ec..9ff6471 100644 --- a/deploy/bootstrap_test.sh +++ b/deploy/bootstrap_test.sh @@ -2064,7 +2064,8 @@ else fi # A re-run that writes the same felis-link.properties leaves the file alone: felis domain -# check reads a proxy started before the file's mtime as still on the old names. +# check reads a proxy started before the file's mtime as still on the old names. stat answers +# as for the file the first install left, root:v 0640, which the test's user cannot make. run_link() { # velocity-dir [root-domain] VD="$1" RD="${2:-r.example.com}" TMPDIR="$1" FNFILE="$fnfile" bash -c ' set -Eeuo pipefail @@ -2073,6 +2074,7 @@ run_link() { # velocity-dir [root-domain] prepare_velocity_layout() { :; } atomic_install_file() { echo "REPLACED $(basename "$2")"; cp "$1" "$2"; } chown() { echo "CHOWN $*"; }; chmod() { echo "CHMOD $*"; } + stat() { echo "root:v 640"; } . "$FNFILE" STATE_DIR="$VD" FELIS_ROOT_DOMAIN="$RD" FORWARDING_SECRET=f SERVICE_TOKEN=t LOGIN_SERVER=login \ LOBBY_SERVER=lobby FELIS_GAME_PORT=25565 VELOCITY_DIR="$VD" VELOCITY_USER=v NODE_IP=10.0.0.5 @@ -2089,8 +2091,10 @@ case "$out" in *"REPLACED felis-link.properties"*) echo "FAIL: a re-run with the same names replaced felis-link.properties"; fails=$((fails + 1)) ;; *) echo "PASS a re-run with the same names leaves felis-link.properties in place" ;; esac -expect "the re-run still fixes the owner" "CHOWN root:v $lprops" "$out" -expect "the re-run still fixes the mode" "CHMOD 0640 $lprops" "$out" +case "$out" in + *CHOWN*|*CHMOD*) echo "FAIL: the re-run changed felis-link.properties by path:"; printf '%s\n' "$out"; fails=$((fails + 1)) ;; + *) echo "PASS the re-run changes nothing by path" ;; +esac expect "the kept file keeps its mtime" "$before" "$(ls -l --time-style=+%s "$lprops" 2>/dev/null || stat -f '%m' "$lprops")" expect "a re-run on other names replaces felis-link.properties" "REPLACED felis-link.properties" "$(run_link "$ldir2" other.example.net)" expect "the replaced file has the new root domain" "root-domain=other.example.net" "$(grep '^root-domain=' "$lprops")" @@ -4493,6 +4497,39 @@ expect "from FELIS_ARTIFACT_DIR it stops the install" "DIE: FELIS_ARTIFACT_DIR: expect "a source build builds the plugin" "ENSURE BUILD" "$(run_plugin "" 0)" +# --- install_if_changed never fixes a target in place ----------------------------------------- +# The proxy's account owns the directories these files land in and can swap one for a symlink +# after the checks, so a chown or chmod by path would land on whatever the link names. The cmp +# stub makes that swap right after the content check; chown and chmod report every call. +iicblock="$(bsfn install_if_changed)" +[ -n "$iicblock" ] || { echo "FAIL: no install_if_changed in $BS"; exit 1; } +mkdir "$adir/iic" +printf 'plugin\n' > "$adir/iic/src" +printf 'not the plugin\n' > "$adir/iic/decoy" +chmod 600 "$adir/iic/decoy" +mine="$(stat -c '%U:%G' "$adir/iic/src")" +run_iic() { # target's mode, the owner:group asked for, then "edited" or "swapped" + rm -f "$adir/iic/dst" + if [ "${3-}" = edited ]; then printf 'an older plugin\n' > "$adir/iic/dst"; else cp "$adir/iic/src" "$adir/iic/dst"; fi + chmod "$1" "$adir/iic/dst" + D="$adir/iic" OG="$2" HOW="${3-}" bash -c ' + atomic_install_file() { echo "ATOMIC $2"; } + chown() { echo "CHOWN $*"; command chown "$@"; } + chmod() { echo "CHMOD $*"; command chmod "$@"; } + cmp() { command cmp "$@" || return; [ "$HOW" != swapped ] || ln -sfn "$D/decoy" "$3"; } + '"$iicblock"' + install_if_changed "$D/src" "$D/dst" 0644 "${OG%%:*}" "${OG#*:}"' 2>&1 +} +iic_is() { # label want got + [ "$3" = "$2" ] && echo "PASS $1" || { printf 'FAIL %s: got\n%s\nwant\n%s\n' "$1" "$3" "$2"; fails=$((fails + 1)); } +} +iic_is "the same bytes, owner and mode are left alone" "" "$(run_iic 644 "$mine")" +iic_is "the same bytes with the wrong mode are reinstalled" "ATOMIC $adir/iic/dst" "$(run_iic 600 "$mine")" +iic_is "the same bytes with the wrong owner are reinstalled" "ATOMIC $adir/iic/dst" "$(run_iic 644 "felis-nobody:${mine#*:}")" +iic_is "new bytes are installed" "ATOMIC $adir/iic/dst" "$(run_iic 644 "$mine" edited)" +iic_is "a target swapped for a symlink after the checks is reinstalled" "ATOMIC $adir/iic/dst" "$(run_iic 644 "$mine" swapped)" +iic_is "and the file the link named keeps its mode" 600 "$(stat -c %a "$adir/iic/decoy")" + # --- k3s's own images from its GitHub release ---------------------------------------------------- kablock="$(bsfn stage_k3s_airgap_images)" mkdir -p "$adir/k3simg" "$adir/k3srel" diff --git a/docs/openapi.yaml b/docs/openapi.yaml index 9c84e11..3241043 100644 --- a/docs/openapi.yaml +++ b/docs/openapi.yaml @@ -5864,7 +5864,7 @@ paths: $ref: '#/components/responses/Unauthorized' '403': description: >- - Not an owner (forbidden); a change to the caller's own role (self_protected); or a role change on the owner account (owner_protected), which only the host's break-glass console (sudo felis breakGlass) may make. + Not an owner (forbidden); a change to the caller's own role (self_protected); a role change on the owner account (owner_protected), which only the host's break-glass console (sudo felis breakGlass) may make; or a change to the caller's own email without a reauth in the last 5 minutes (reauth_required). content: application/json: schema: { $ref: '#/components/schemas/Error' } @@ -6136,7 +6136,11 @@ paths: '401': $ref: '#/components/responses/Unauthorized' '403': - $ref: '#/components/responses/Forbidden' + description: >- + Not an owner (forbidden); or unbinding the caller's own passkeys without a reauth in the last 5 minutes (reauth_required). + content: + application/json: + schema: { $ref: '#/components/schemas/Error' } /api/v1/users/{id}/links: post: diff --git a/internal/api/handlers_users.go b/internal/api/handlers_users.go index 6077356..3a4ac6f 100644 --- a/internal/api/handlers_users.go +++ b/internal/api/handlers_users.go @@ -175,6 +175,11 @@ func (a *API) handlePatchUser(w http.ResponseWriter, r *http.Request) { "role must be 'admin' or 'user', got %q", *body.Role)) return } + // A new address unverifies the old one, and with it the email factor that + // guards adding a passkey: your own takes the same reauth as /account/email. + if body.Email != nil && id == p.UserID && !a.requireReauth(w, r, p) { + return + } u, err := a.Repo.UpdateUser(r.Context(), id, UpdateUserInput(body), p.Email) if err != nil { @@ -431,6 +436,11 @@ func (a *API) handleUnbindUserPasskeys(w http.ResponseWriter, r *http.Request) { writeError(w, r, errBadRequest) return } + // Your own passkeys are a factor that guards adding one: severing them takes + // the same reauth as removing one under /account/passkey. + if p := principalFromContext(r.Context()); id == p.UserID && !a.requireReauth(w, r, p) { + return + } if err := a.Repo.DeleteAllPasskeyCredentialsForUser(r.Context(), id); err != nil { writeError(w, r, err) diff --git a/internal/api/handlers_users_test.go b/internal/api/handlers_users_test.go index 7a35942..7c04b36 100644 --- a/internal/api/handlers_users_test.go +++ b/internal/api/handlers_users_test.go @@ -1,7 +1,9 @@ package api import ( + "context" "net/http" + "net/http/httptest" "strings" "testing" ) @@ -83,6 +85,77 @@ func TestOwnerAccountProtectedFromPanelMutations(t *testing.T) { }) } +// The caller's own email and passkeys are ways into the caller's account, so +// changing them through /users/{id} takes the same recent reauth as through +// /account. Without it a stolen owner session could strip both here, and with no +// factor left to guard, /account/passkey/register/begin would let it plant its own. +func TestOwnSignInFactorsNeedReauthUnderUsers(t *testing.T) { + owner := &Principal{UserID: "usr-root", Role: "owner", Email: "root@example.net", + ViaAdminAccess: true, ViaSession: true, EmailVerified: true} + repo := newFakeRepo() + repo.seedUser(UserView{ID: "usr-root", Username: "root", Email: "root@example.net", Role: "owner", EmailVerified: true}) + repo.seedUser(UserView{ID: "u2", Username: "alice", Email: "alice@example.net", Role: "user"}) + repo.passkeyCreds["pk-root"] = PasskeyCredential{ID: "pk-root", UserID: "usr-root", CredentialID: "c-root", UserVerified: true, CreatedAt: frozenNow} + api := newTestAPI(repo, newFakeCluster()) + api.Mailer = &captureMailer{} + api.External = staticExternal{p: owner} + eh := api.ExternalHandler() + send := func(method, path, body string) *httptest.ResponseRecorder { + if body == "" { + return do(eh, method, path, "", nil) + } + return do(eh, method, path, body, jsonHeader) + } + + own := []struct { + name, method, path, body string + untouched func() bool + }{ + {"own email", "PATCH", "/api/v1/users/usr-root", `{"email":"thief@example.net"}`, func() bool { + d, err := repo.UserDetail(context.Background(), "usr-root") + return err == nil && d.Email == "root@example.net" + }}, + {"own passkeys", "DELETE", "/api/v1/users/usr-root/passkeys", "", func() bool { + _, ok := repo.passkeyCreds["pk-root"] + return ok + }}, + } + for _, tc := range own { + t.Run(tc.name+" refused without a recent reauth", func(t *testing.T) { + w := send(tc.method, tc.path, tc.body) + if w.Code != http.StatusForbidden || decodeErr(t, w) != "reauth_required" { + t.Fatalf("code = %d body %s, want 403 reauth_required", w.Code, w.Body.String()) + } + if !tc.untouched() { + t.Fatal("the change went through despite the refusal") + } + }) + } + // Another account's factors are the owner's to manage, and a username is no way in. + for _, tc := range []struct{ name, method, path, body string }{ + {"another user's email", "PATCH", "/api/v1/users/u2", `{"email":"alice@new.example"}`}, + {"another user's passkeys", "DELETE", "/api/v1/users/u2/passkeys", ""}, + {"own username", "PATCH", "/api/v1/users/usr-root", `{"username":"root2"}`}, + } { + t.Run("control: "+tc.name+" needs no reauth", func(t *testing.T) { + if w := send(tc.method, tc.path, tc.body); w.Code != http.StatusOK { + t.Fatalf("code = %d body %s, want 200", w.Code, w.Body.String()) + } + }) + } + owner.ReauthAt = api.now() + for _, tc := range own { + t.Run(tc.name+" allowed after a reauth", func(t *testing.T) { + if w := send(tc.method, tc.path, tc.body); w.Code != http.StatusOK { + t.Fatalf("code = %d body %s, want 200", w.Code, w.Body.String()) + } + if tc.untouched() { + t.Fatal("the change did not go through") + } + }) + } +} + // The user-scoped admin sub-resources (quotas, account links) must answer 404 // for an unknown user id. Before the requireLiveUser guard the quota upsert and // the link insert reached the users(id) foreign key and surfaced as an opaque diff --git a/panel/src/lib/openapi.gen.ts b/panel/src/lib/openapi.gen.ts index 345004d..c5fffc3 100644 --- a/panel/src/lib/openapi.gen.ts +++ b/panel/src/lib/openapi.gen.ts @@ -8280,7 +8280,7 @@ export interface operations { }; 400: components["responses"]["BadRequest"]; 401: components["responses"]["Unauthorized"]; - /** @description Not an owner (forbidden); a change to the caller's own role (self_protected); or a role change on the owner account (owner_protected), which only the host's break-glass console (sudo felis breakGlass) may make. */ + /** @description Not an owner (forbidden); a change to the caller's own role (self_protected); a role change on the owner account (owner_protected), which only the host's break-glass console (sudo felis breakGlass) may make; or a change to the caller's own email without a reauth in the last 5 minutes (reauth_required). */ 403: { headers: { [name: string]: unknown; @@ -8541,7 +8541,15 @@ export interface operations { }; }; 401: components["responses"]["Unauthorized"]; - 403: components["responses"]["Forbidden"]; + /** @description Not an owner (forbidden); or unbinding the caller's own passkeys without a reauth in the last 5 minutes (reauth_required). */ + 403: { + headers: { + [name: string]: unknown; + }; + content: { + "application/json": components["schemas"]["Error"]; + }; + }; }; }; linkAccount: {