fix(api): a session-store outage answers 503, not 401
Resolving a session cookie failed identically whether the credential was missing or Postgres was unreachable: local_auth_enabled read errors fell into the fail-closed 'disabled' branch and SessionUser errors into 'invalid session', both surfacing as 401 'authentication required' — a lie that reads as 'log in again' during an outage. Split the enabled-read into (enabled, error), tag non-ErrNotFound store failures with errAuthBackend, and map that to a new 503 auth_unavailable in requireExternal. Fail-closed is unchanged: missing setting / bad value / missing session stay 401.
This commit is contained in:
4 files changed
+91
-6
No files matched your search
@@ -59,6 +59,10 @@ type fakeRepo struct {
|
|||||||
staff map[string]*StaffUser // username -> staff login row
|
staff map[string]*StaffUser // username -> staff login row
|
||||||
sessions map[string]*fakeSession // token_hash -> session
|
sessions map[string]*fakeSession // token_hash -> session
|
||||||
settings map[string][]byte // key -> jsonb value
|
settings map[string][]byte // key -> jsonb value
|
||||||
|
// failSessionUser / failGetSetting force those reads to fail with a generic
|
||||||
|
// (non-ErrNotFound) error, simulating a store outage for the 503 auth path.
|
||||||
|
failSessionUser error
|
||||||
|
failGetSetting error
|
||||||
// player email OTPs (spec §B2). Keyed by row id; the verify path scans for the
|
// player email OTPs (spec §B2). Keyed by row id; the verify path scans for the
|
||||||
// newest live (user, purpose) just as the PG query does.
|
// newest live (user, purpose) just as the PG query does.
|
||||||
otps map[string]*fakeEmailOTP
|
otps map[string]*fakeEmailOTP
|
||||||
@@ -768,6 +772,9 @@ func (f *fakeRepo) CreateSession(_ context.Context, tokenHash, userID string, ex
|
|||||||
return nil
|
return nil
|
||||||
}
|
}
|
||||||
func (f *fakeRepo) SessionUser(_ context.Context, tokenHash string, now time.Time) (*SessionedUser, error) {
|
func (f *fakeRepo) SessionUser(_ context.Context, tokenHash string, now time.Time) (*SessionedUser, error) {
|
||||||
|
if f.failSessionUser != nil {
|
||||||
|
return nil, f.failSessionUser
|
||||||
|
}
|
||||||
s, ok := f.sessions[tokenHash]
|
s, ok := f.sessions[tokenHash]
|
||||||
if !ok || s.revoked || !s.expiresAt.After(now) {
|
if !ok || s.revoked || !s.expiresAt.After(now) {
|
||||||
return nil, ErrNotFound
|
return nil, ErrNotFound
|
||||||
@@ -788,6 +795,9 @@ func (f *fakeRepo) RevokeSession(_ context.Context, tokenHash string) error {
|
|||||||
return nil
|
return nil
|
||||||
}
|
}
|
||||||
func (f *fakeRepo) GetSetting(_ context.Context, key string) ([]byte, error) {
|
func (f *fakeRepo) GetSetting(_ context.Context, key string) ([]byte, error) {
|
||||||
|
if f.failGetSetting != nil {
|
||||||
|
return nil, f.failGetSetting
|
||||||
|
}
|
||||||
if v, ok := f.settings[key]; ok {
|
if v, ok := f.settings[key]; ok {
|
||||||
return v, nil
|
return v, nil
|
||||||
}
|
}
|
||||||
@@ -2195,3 +2205,38 @@ func TestAccessVerifier(t *testing.T) {
|
|||||||
}
|
}
|
||||||
})
|
})
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// TestSessionAuthOutageIs503Not401: a session-store outage must surface as 503
|
||||||
|
// auth_unavailable, not a 401 that reads as "please log in again". Both failure
|
||||||
|
// points are covered — the local_auth_enabled read and the session row read —
|
||||||
|
// plus the regression that a genuinely missing session still answers 401.
|
||||||
|
func TestSessionAuthOutageIs503Not401(t *testing.T) {
|
||||||
|
apiWith := func(repo *fakeRepo) *API {
|
||||||
|
a := newTestAPI(repo, newFakeCluster())
|
||||||
|
a.External = SessionAuth{Repo: repo, RootDomain: testRoot, AdminHostname: "op.console." + testRoot}
|
||||||
|
return a
|
||||||
|
}
|
||||||
|
cookie := map[string]string{"Cookie": sessionCookieName + "=any"}
|
||||||
|
outage := errors.New("dial tcp 10.0.0.5:5432: connect: connection refused")
|
||||||
|
|
||||||
|
repo := newFakeRepo()
|
||||||
|
repo.settings[LocalAuthEnabledKey] = []byte("true")
|
||||||
|
repo.failGetSetting = outage
|
||||||
|
if w := do(apiWith(repo).ExternalHandler(), "GET", "/api/v1/me", "", cookie); w.Code != http.StatusServiceUnavailable || decodeErr(t, w) != "auth_unavailable" {
|
||||||
|
t.Fatalf("settings read outage = %d body %s, want 503 auth_unavailable", w.Code, w.Body.String())
|
||||||
|
}
|
||||||
|
|
||||||
|
repo = newFakeRepo()
|
||||||
|
repo.settings[LocalAuthEnabledKey] = []byte("true")
|
||||||
|
repo.failSessionUser = outage
|
||||||
|
if w := do(apiWith(repo).ExternalHandler(), "GET", "/api/v1/me", "", cookie); w.Code != http.StatusServiceUnavailable || decodeErr(t, w) != "auth_unavailable" {
|
||||||
|
t.Fatalf("session read outage = %d body %s, want 503 auth_unavailable", w.Code, w.Body.String())
|
||||||
|
}
|
||||||
|
|
||||||
|
// Regression: fail-closed auth (missing/invalid session) stays a 401.
|
||||||
|
repo = newFakeRepo()
|
||||||
|
repo.settings[LocalAuthEnabledKey] = []byte("true")
|
||||||
|
if w := do(apiWith(repo).ExternalHandler(), "GET", "/api/v1/me", "", cookie); w.Code != http.StatusUnauthorized || decodeErr(t, w) != "unauthorized" {
|
||||||
|
t.Fatalf("missing session = %d body %s, want 401 unauthorized", w.Code, w.Body.String())
|
||||||
|
}
|
||||||
|
}
|
||||||
@@ -89,6 +89,11 @@ func newError(status int, code, format string, a ...any) *apiError {
|
|||||||
// Common errors reused across handlers.
|
// Common errors reused across handlers.
|
||||||
var (
|
var (
|
||||||
errUnauthorized = newError(http.StatusUnauthorized, "unauthorized", "authentication required")
|
errUnauthorized = newError(http.StatusUnauthorized, "unauthorized", "authentication required")
|
||||||
|
// errAuthUnavailable answers when the session store itself is unreachable
|
||||||
|
// (Postgres down): an outage is not a credential verdict, so the caller gets
|
||||||
|
// 503 "retry" instead of a 401 that reads as "log in again".
|
||||||
|
errAuthUnavailable = newError(http.StatusServiceUnavailable, "auth_unavailable",
|
||||||
|
"authentication is temporarily unavailable; retry shortly")
|
||||||
errForbidden = newError(http.StatusForbidden, "forbidden", "not permitted")
|
errForbidden = newError(http.StatusForbidden, "forbidden", "not permitted")
|
||||||
errBadRequest = newError(http.StatusBadRequest, "bad_request", "invalid request")
|
errBadRequest = newError(http.StatusBadRequest, "bad_request", "invalid request")
|
||||||
)
|
)
|
||||||
|
|||||||
@@ -4,6 +4,7 @@ import (
|
|||||||
"context"
|
"context"
|
||||||
"crypto/rand"
|
"crypto/rand"
|
||||||
"encoding/hex"
|
"encoding/hex"
|
||||||
|
"errors"
|
||||||
"log"
|
"log"
|
||||||
"net/http"
|
"net/http"
|
||||||
"runtime/debug"
|
"runtime/debug"
|
||||||
@@ -95,6 +96,11 @@ func (a *API) requireInternal(next http.Handler) http.Handler {
|
|||||||
func (a *API) requireExternal(next http.Handler) http.Handler {
|
func (a *API) requireExternal(next http.Handler) http.Handler {
|
||||||
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||||
p, err := a.External.Authenticate(r)
|
p, err := a.External.Authenticate(r)
|
||||||
|
if errors.Is(err, errAuthBackend) {
|
||||||
|
// Session store unreachable — an outage, not a missing credential.
|
||||||
|
writeError(w, r, errAuthUnavailable)
|
||||||
|
return
|
||||||
|
}
|
||||||
if err != nil || p == nil {
|
if err != nil || p == nil {
|
||||||
writeError(w, r, errUnauthorized)
|
writeError(w, r, errUnauthorized)
|
||||||
return
|
return
|
||||||
|
|||||||
+35
-6
@@ -7,6 +7,7 @@ import (
|
|||||||
"encoding/base64"
|
"encoding/base64"
|
||||||
"encoding/hex"
|
"encoding/hex"
|
||||||
"encoding/json"
|
"encoding/json"
|
||||||
|
"errors"
|
||||||
"fmt"
|
"fmt"
|
||||||
"net"
|
"net"
|
||||||
"net/http"
|
"net/http"
|
||||||
@@ -145,14 +146,24 @@ func (s SessionAuth) Authenticate(r *http.Request) (*Principal, error) {
|
|||||||
}
|
}
|
||||||
|
|
||||||
ctx := r.Context()
|
ctx := r.Context()
|
||||||
if !localAuthEnabled(ctx, s.Repo) {
|
enabled, err := localAuthEnabledStatus(ctx, s.Repo)
|
||||||
|
if err != nil {
|
||||||
|
// The session store is unreachable: this is an outage, not a verdict on
|
||||||
|
// the caller's credentials, so the middleware answers 503 rather than a
|
||||||
|
// misleading "please log in".
|
||||||
|
return nil, fmt.Errorf("%w: %v", errAuthBackend, err)
|
||||||
|
}
|
||||||
|
if !enabled {
|
||||||
// A cookie was presented but local auth is off: reject, never fall through.
|
// A cookie was presented but local auth is off: reject, never fall through.
|
||||||
return nil, fmt.Errorf("local auth disabled")
|
return nil, fmt.Errorf("local auth disabled")
|
||||||
}
|
}
|
||||||
|
|
||||||
u, err := s.Repo.SessionUser(ctx, hashCookie(cookie.Value), s.now())
|
u, err := s.Repo.SessionUser(ctx, hashCookie(cookie.Value), s.now())
|
||||||
if err != nil {
|
switch {
|
||||||
|
case errors.Is(err, ErrNotFound):
|
||||||
return nil, fmt.Errorf("invalid session: %w", err)
|
return nil, fmt.Errorf("invalid session: %w", err)
|
||||||
|
case err != nil:
|
||||||
|
return nil, fmt.Errorf("%w: %v", errAuthBackend, err)
|
||||||
}
|
}
|
||||||
return &Principal{
|
return &Principal{
|
||||||
UserID: u.ID,
|
UserID: u.ID,
|
||||||
@@ -164,6 +175,11 @@ func (s SessionAuth) Authenticate(r *http.Request) (*Principal, error) {
|
|||||||
}, nil
|
}, nil
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// errAuthBackend marks an authentication failure caused by the session store
|
||||||
|
// being unreachable (e.g. Postgres down) rather than by a missing or invalid
|
||||||
|
// credential. Middleware maps it to 503 so an outage is not misreported as 401.
|
||||||
|
var errAuthBackend = errors.New("auth backend unavailable")
|
||||||
|
|
||||||
// localAuthEnabled reports whether the runtime local_auth_enabled toggle is true.
|
// localAuthEnabled reports whether the runtime local_auth_enabled toggle is true.
|
||||||
// A missing setting, a read error, or a non-true value all read as disabled — the
|
// A missing setting, a read error, or a non-true value all read as disabled — the
|
||||||
// gate fails closed so local sessions are honored, and new ones minted, only on an
|
// gate fails closed so local sessions are honored, and new ones minted, only on an
|
||||||
@@ -171,15 +187,28 @@ func (s SessionAuth) Authenticate(r *http.Request) (*Principal, error) {
|
|||||||
// (minting one) consult it, so the two never disagree about whether local auth is
|
// (minting one) consult it, so the two never disagree about whether local auth is
|
||||||
// live.
|
// live.
|
||||||
func localAuthEnabled(ctx context.Context, repo Repo) bool {
|
func localAuthEnabled(ctx context.Context, repo Repo) bool {
|
||||||
|
enabled, _ := localAuthEnabledStatus(ctx, repo)
|
||||||
|
return enabled
|
||||||
|
}
|
||||||
|
|
||||||
|
// localAuthEnabledStatus is localAuthEnabled with the outage case kept apart: a
|
||||||
|
// MISSING setting (ErrNotFound — never enabled) reads as (false, nil), while a
|
||||||
|
// store read failure reads as (false, err) so SessionAuth can tell "local auth
|
||||||
|
// is off" (401) from "the database is down" (503). An unreadable value still
|
||||||
|
// fails closed as disabled — it is a config fault, not an outage.
|
||||||
|
func localAuthEnabledStatus(ctx context.Context, repo Repo) (bool, error) {
|
||||||
raw, err := repo.GetSetting(ctx, LocalAuthEnabledKey)
|
raw, err := repo.GetSetting(ctx, LocalAuthEnabledKey)
|
||||||
if err != nil {
|
switch {
|
||||||
return false // ErrNotFound (never enabled) or a transient read error → closed
|
case errors.Is(err, ErrNotFound):
|
||||||
|
return false, nil
|
||||||
|
case err != nil:
|
||||||
|
return false, err
|
||||||
}
|
}
|
||||||
var enabled bool
|
var enabled bool
|
||||||
if err := json.Unmarshal(raw, &enabled); err != nil {
|
if err := json.Unmarshal(raw, &enabled); err != nil {
|
||||||
return false
|
return false, nil
|
||||||
}
|
}
|
||||||
return enabled
|
return enabled, nil
|
||||||
}
|
}
|
||||||
|
|
||||||
// ensure SessionAuth satisfies ExternalAuth at compile time.
|
// ensure SessionAuth satisfies ExternalAuth at compile time.
|
||||||
|
|||||||
Reference in new issue
Block a user