diff --git a/cmd/minstrel/admin.go b/cmd/minstrel/admin.go index 72248b40..c2dcd3eb 100644 --- a/cmd/minstrel/admin.go +++ b/cmd/minstrel/admin.go @@ -55,12 +55,14 @@ func runAdmin(args []string) error { } } -// adminResetPassword resets a user's credentials. It updates BOTH -// password_hash (bcrypt, for /api/auth/login) and subsonic_password -// (plaintext, required for Subsonic t+s token verification) so neither -// auth path is left stale. Recovers a locked-out operator when the -// bootstrap password was missed or the DB volume was recreated (Fable -// #321) without DB surgery. +// adminResetPassword resets a user's login password (password_hash). It +// recovers a locked-out operator when the bootstrap password was missed or +// the DB volume was recreated (Fable #321) without DB surgery. +// +// It deliberately leaves subsonic_password alone. That column is stored in +// plain text, and it used to receive the new login password here, so any +// account recovered this way had its login password readable in the database +// (#5026). The Subsonic password is generated separately in Settings. func adminResetPassword(args []string) error { fs := flag.NewFlagSet("admin reset-password", flag.ContinueOnError) configPath := fs.String("config", os.Getenv("MINSTREL_CONFIG"), "path to YAML config file") @@ -112,13 +114,6 @@ func adminResetPassword(args []string) error { }); err != nil { return fmt.Errorf("update password_hash: %w", err) } - sp := pw - if err := q.SetSubsonicPassword(ctx, dbq.SetSubsonicPasswordParams{ - ID: user.ID, - SubsonicPassword: &sp, - }); err != nil { - return fmt.Errorf("update subsonic_password: %w", err) - } if generated { fmt.Printf("minstrel: password for %q reset.\nNew password: %s\n", *username, pw) diff --git a/docs/security.md b/docs/security.md index fc6b030f..7452b8df 100644 --- a/docs/security.md +++ b/docs/security.md @@ -54,20 +54,24 @@ they aren't checked. Classic Subsonic clients sign in with `t` and `s`: the MD5 of the password followed by a random salt. To check that, the server has to know the password itself, so supporting this sign-in method means storing a password Minstrel -can read. That is the `subsonic_password` column, and it is the only -credential Minstrel keeps unhashed. +can read. That is the Subsonic password, and it is the only credential +Minstrel keeps unhashed. - **The recommended way in is the API key.** Clients that support the OpenSubsonic `apiKey` should use it. The key is stored hashed and can be replaced at any time in Settings. -- **`t`/`s` and `p=` sign-in are off for an account until its - `subsonic_password` is set**, and nothing in the app sets it. Plain `p=` - sign-in is additionally off server-wide unless +- **The Subsonic password is never your login password.** Minstrel generates + it (**Settings → Subsonic password**), shows it once, and lets you replace + or turn it off. Because it is random, a copy of the database exposes access + to this server's Subsonic API and nothing else: it can't be a password you + also use somewhere else. +- **`t`/`s` and `p=` sign-in are off for an account until it has a Subsonic + password.** Plain `p=` sign-in is additionally off server-wide unless `subsonic.allow_plaintext_password` is enabled. -- **Known issue:** `minstrel admin reset-password` writes the new login - password into `subsonic_password` as well, so `t`/`s` clients keep working - after a recovery. For an account reset that way, the login password is - stored in plain text until the column is cleared. +- `minstrel admin reset-password` changes only the login password. Older + versions also copied it into the Subsonic password; upgrading clears every + Subsonic password once, so those copies are gone. An account that used + `t`/`s` sign-in needs a new Subsonic password generated in Settings. ## Android allows plain HTTP diff --git a/internal/api/api.go b/internal/api/api.go index 808e50ea..cdbff7db 100644 --- a/internal/api/api.go +++ b/internal/api/api.go @@ -109,6 +109,9 @@ func Mount(r chi.Router, pool *pgxpool.Pool, logger *slog.Logger, events *playev authed.Put("/me/profile", h.handleUpdateMyProfile) authed.Put("/me/timezone", h.handlePutTimezone) authed.Post("/me/api-token", h.handleRegenerateMyAPIToken) + authed.Get("/me/subsonic-password", h.handleGetMySubsonicPassword) + authed.Post("/me/subsonic-password", h.handleGenerateMySubsonicPassword) + authed.Delete("/me/subsonic-password", h.handleClearMySubsonicPassword) authed.Get("/me/sessions", h.handleListMySessions) authed.Delete("/me/sessions/{id}", h.handleRevokeMySession) authed.Post("/me/sessions/logout-others", h.handleRevokeMyOtherSessions) diff --git a/internal/api/me_subsonic_password.go b/internal/api/me_subsonic_password.go new file mode 100644 index 00000000..62143d33 --- /dev/null +++ b/internal/api/me_subsonic_password.go @@ -0,0 +1,84 @@ +package api + +import ( + "crypto/rand" + "encoding/base64" + "net/http" + + "git.fabledsword.com/bvandeusen/minstrel/internal/apierror" + "git.fabledsword.com/bvandeusen/minstrel/internal/audit" + "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" +) + +// The Subsonic password is for clients that sign in with Subsonic's t/s +// scheme (md5 of the password plus a salt). Checking that needs the password +// itself, so it is stored readable (migration 0003). That is why Minstrel +// generates it rather than letting the user choose one: a generated value +// can never be a login password reused from somewhere else, so a leaked +// users table gives up access to this server's /rest API and nothing more +// (M462 #5026). + +const subsonicPasswordBytes = 18 // 24 base64url characters + +type subsonicPasswordStatusResp struct { + Enabled bool `json:"enabled"` +} + +type subsonicPasswordResp struct { + Password string `json:"password"` +} + +// handleGetMySubsonicPassword implements GET /api/me/subsonic-password. It +// reports only whether one is set; the value is shown once, when generated. +func (h *handlers) handleGetMySubsonicPassword(w http.ResponseWriter, r *http.Request) { + user, ok := requireUser(w, r) + if !ok { + return + } + writeJSON(w, http.StatusOK, subsonicPasswordStatusResp{Enabled: user.SubsonicPassword != nil}) +} + +// handleGenerateMySubsonicPassword implements POST /api/me/subsonic-password: +// replaces any existing Subsonic password with a new random one and returns it. +func (h *handlers) handleGenerateMySubsonicPassword(w http.ResponseWriter, r *http.Request) { + user, ok := requireUser(w, r) + if !ok { + return + } + b := make([]byte, subsonicPasswordBytes) + if _, err := rand.Read(b); err != nil { + h.logger.Error("generate subsonic password: rand failed", "err", err) + writeErr(w, apierror.Internal(err)) + return + } + pw := base64.RawURLEncoding.EncodeToString(b) + if err := dbq.New(h.pool).SetSubsonicPassword(r.Context(), dbq.SetSubsonicPasswordParams{ + ID: user.ID, + SubsonicPassword: &pw, + }); err != nil { + h.logger.Error("generate subsonic password: update failed", "err", err) + writeErr(w, apierror.Internal(err)) + return + } + audit.WriteOrLog(r.Context(), h.pool, h.logger, user.ID, user.ID, audit.ActionSubsonicPasswordSet, nil) + writeJSON(w, http.StatusOK, subsonicPasswordResp{Password: pw}) +} + +// handleClearMySubsonicPassword implements DELETE /api/me/subsonic-password, +// which turns t/s and p= sign-in off for the account. +func (h *handlers) handleClearMySubsonicPassword(w http.ResponseWriter, r *http.Request) { + user, ok := requireUser(w, r) + if !ok { + return + } + if err := dbq.New(h.pool).SetSubsonicPassword(r.Context(), dbq.SetSubsonicPasswordParams{ + ID: user.ID, + SubsonicPassword: nil, + }); err != nil { + h.logger.Error("clear subsonic password: update failed", "err", err) + writeErr(w, apierror.Internal(err)) + return + } + audit.WriteOrLog(r.Context(), h.pool, h.logger, user.ID, user.ID, audit.ActionSubsonicPasswordClear, nil) + w.WriteHeader(http.StatusNoContent) +} diff --git a/internal/api/me_subsonic_password_test.go b/internal/api/me_subsonic_password_test.go new file mode 100644 index 00000000..f908cab1 --- /dev/null +++ b/internal/api/me_subsonic_password_test.go @@ -0,0 +1,132 @@ +package api + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "os" + "testing" + + "github.com/go-chi/chi/v5" + + "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" +) + +func newMeSubsonicPasswordRouter(h *handlers) chi.Router { + r := chi.NewRouter() + r.Get("/api/me/subsonic-password", h.handleGetMySubsonicPassword) + r.Post("/api/me/subsonic-password", h.handleGenerateMySubsonicPassword) + r.Delete("/api/me/subsonic-password", h.handleClearMySubsonicPassword) + return r +} + +func readSubsonicPassword(t *testing.T, h *handlers, user dbq.User) *string { + t.Helper() + var pw *string + if err := h.pool.QueryRow(context.Background(), + "SELECT subsonic_password FROM users WHERE id = $1", user.ID).Scan(&pw); err != nil { + t.Fatalf("read subsonic_password: %v", err) + } + return pw +} + +func TestSubsonicPassword_GenerateIsRandomAndNotTheLoginPassword(t *testing.T) { + if os.Getenv("MINSTREL_TEST_DATABASE_URL") == "" { + t.Skip("MINSTREL_TEST_DATABASE_URL not set") + } + h, pool := testHandlers(t) + user := seedUser(t, pool, "sspw1", "login-pw", false) + router := newMeSubsonicPasswordRouter(h) + + generate := func() string { + req := withUser(httptest.NewRequest(http.MethodPost, "/api/me/subsonic-password", nil), user) + rec := httptest.NewRecorder() + router.ServeHTTP(rec, req) + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200; body=%s", rec.Code, rec.Body.String()) + } + var resp subsonicPasswordResp + if err := json.Unmarshal(rec.Body.Bytes(), &resp); err != nil { + t.Fatalf("decode: %v", err) + } + return resp.Password + } + + first := generate() + if len(first) != 24 { + t.Errorf("password length = %d, want 24", len(first)) + } + if first == "login-pw" { + t.Errorf("generated password equals the login password") + } + if got := readSubsonicPassword(t, h, user); got == nil || *got != first { + t.Errorf("stored = %v, want the returned password", got) + } + + second := generate() + if second == first { + t.Errorf("regenerate returned the same password") + } + if got := readSubsonicPassword(t, h, user); got == nil || *got != second { + t.Errorf("stored after regenerate = %v, want the new password", got) + } +} + +func TestSubsonicPassword_StatusAndClear(t *testing.T) { + if os.Getenv("MINSTREL_TEST_DATABASE_URL") == "" { + t.Skip("MINSTREL_TEST_DATABASE_URL not set") + } + h, pool := testHandlers(t) + user := seedUser(t, pool, "sspw2", "login-pw", false) + router := newMeSubsonicPasswordRouter(h) + + status := func(u dbq.User) bool { + req := withUser(httptest.NewRequest(http.MethodGet, "/api/me/subsonic-password", nil), u) + rec := httptest.NewRecorder() + router.ServeHTTP(rec, req) + if rec.Code != http.StatusOK { + t.Fatalf("GET status = %d, want 200; body=%s", rec.Code, rec.Body.String()) + } + var resp subsonicPasswordStatusResp + if err := json.Unmarshal(rec.Body.Bytes(), &resp); err != nil { + t.Fatalf("decode: %v", err) + } + return resp.Enabled + } + + if status(user) { + t.Errorf("enabled = true for a new account, want false") + } + pw := "set-by-test" + user.SubsonicPassword = &pw + if !status(user) { + t.Errorf("enabled = false with a password set, want true") + } + // The status response never carries the value itself. + req := withUser(httptest.NewRequest(http.MethodGet, "/api/me/subsonic-password", nil), user) + rec := httptest.NewRecorder() + router.ServeHTTP(rec, req) + var raw map[string]any + if err := json.Unmarshal(rec.Body.Bytes(), &raw); err != nil { + t.Fatalf("decode: %v", err) + } + if _, has := raw["password"]; has { + t.Errorf("GET response includes the password: %s", rec.Body.String()) + } + + if err := dbq.New(pool).SetSubsonicPassword(context.Background(), dbq.SetSubsonicPasswordParams{ + ID: user.ID, SubsonicPassword: &pw, + }); err != nil { + t.Fatalf("seed subsonic_password: %v", err) + } + req = withUser(httptest.NewRequest(http.MethodDelete, "/api/me/subsonic-password", nil), user) + rec = httptest.NewRecorder() + router.ServeHTTP(rec, req) + if rec.Code != http.StatusNoContent { + t.Fatalf("DELETE status = %d, want 204; body=%s", rec.Code, rec.Body.String()) + } + if got := readSubsonicPassword(t, h, user); got != nil { + t.Errorf("stored after clear = %q, want NULL", *got) + } +} diff --git a/internal/audit/audit.go b/internal/audit/audit.go index ef2c5f60..b9e92c5d 100644 --- a/internal/audit/audit.go +++ b/internal/audit/audit.go @@ -60,6 +60,10 @@ const ( // and its history moved onto the copy kept. The metadata names both, so the // log can answer "where did that file go" long after the report is gone. ActionDuplicateMerge Action = "duplicate_merge" + + // Subsonic password (#5026): generated in Settings for t/s-only clients. + ActionSubsonicPasswordSet Action = "subsonic_password_set" + ActionSubsonicPasswordClear Action = "subsonic_password_clear" ) // Write inserts one audit_log row. metadata is marshaled as JSON; diff --git a/internal/audit/audit_test.go b/internal/audit/audit_test.go index 84faacd7..46998384 100644 --- a/internal/audit/audit_test.go +++ b/internal/audit/audit_test.go @@ -169,6 +169,8 @@ func TestWrite_AllActionConstantsArePersisted(t *testing.T) { audit.ActionForgotPasswordInit, audit.ActionPasswordResetByEmail, audit.ActionDuplicateMerge, + audit.ActionSubsonicPasswordSet, + audit.ActionSubsonicPasswordClear, } for _, a := range actions { if err := audit.Write(context.Background(), pool, nilUUID, nilUUID, a, nil); err != nil { diff --git a/internal/db/dbq/users.sql.go b/internal/db/dbq/users.sql.go index 96000206..0c1173c5 100644 --- a/internal/db/dbq/users.sql.go +++ b/internal/db/dbq/users.sql.go @@ -589,7 +589,8 @@ type SetSubsonicPasswordParams struct { } // Stores (or clears with NULL) the per-user Subsonic legacy credential used -// for t/s and p auth on /rest/*. Must be plaintext; see migration 0003. +// for t/s and p auth on /rest/*. Must be plaintext; see migration 0003. Only +// ever a server-generated value, never the login password (#5026). func (q *Queries) SetSubsonicPassword(ctx context.Context, arg SetSubsonicPasswordParams) error { _, err := q.db.Exec(ctx, setSubsonicPassword, arg.ID, arg.SubsonicPassword) return err diff --git a/internal/db/migrations/0064_clear_subsonic_password.down.sql b/internal/db/migrations/0064_clear_subsonic_password.down.sql new file mode 100644 index 00000000..98a094aa --- /dev/null +++ b/internal/db/migrations/0064_clear_subsonic_password.down.sql @@ -0,0 +1,2 @@ +-- The cleared values are gone and were never meant to be kept; nothing to undo. +SELECT 1; diff --git a/internal/db/migrations/0064_clear_subsonic_password.up.sql b/internal/db/migrations/0064_clear_subsonic_password.up.sql new file mode 100644 index 00000000..ab83c2b4 --- /dev/null +++ b/internal/db/migrations/0064_clear_subsonic_password.up.sql @@ -0,0 +1,10 @@ +-- `minstrel admin reset-password` used to copy the new login password into +-- subsonic_password, so every account recovered through the CLI had its login +-- password stored in plain text, and a later password change in Settings left +-- that copy behind (M462 #5026). The CLI no longer writes this column; a +-- Subsonic password is now generated separately in Settings and is never the +-- login password. Clearing every value here removes the copies already made. +-- +-- Accounts whose Subsonic client signs in with t/s stop working until the user +-- generates a Subsonic password (or switches the client to an API key). +UPDATE users SET subsonic_password = NULL WHERE subsonic_password IS NOT NULL; diff --git a/internal/db/queries/users.sql b/internal/db/queries/users.sql index 9014e631..22f0ef88 100644 --- a/internal/db/queries/users.sql +++ b/internal/db/queries/users.sql @@ -36,7 +36,8 @@ SELECT count(*) FROM users; -- name: SetSubsonicPassword :exec -- Stores (or clears with NULL) the per-user Subsonic legacy credential used --- for t/s and p auth on /rest/*. Must be plaintext; see migration 0003. +-- for t/s and p auth on /rest/*. Must be plaintext; see migration 0003. Only +-- ever a server-generated value, never the login password (#5026). UPDATE users SET subsonic_password = $2 WHERE id = $1; -- name: GetUserByID :one diff --git a/web/src/lib/api/me.ts b/web/src/lib/api/me.ts index 831d2dd5..1bda18a9 100644 --- a/web/src/lib/api/me.ts +++ b/web/src/lib/api/me.ts @@ -55,6 +55,25 @@ export async function regenerateAPIToken(): Promise { return api.post('/api/me/api-token', {}); } +// Subsonic password (#5026) ------------------------------------------------ +// For Subsonic clients that only sign in with a username and password (the +// t/s scheme). The server generates it, so it is never the login password; +// like the API key it is shown once, when generated. + +export type SubsonicPasswordStatus = { enabled: boolean }; + +export async function getSubsonicPasswordStatus(): Promise { + return api.get('/api/me/subsonic-password'); +} + +export async function generateSubsonicPassword(): Promise<{ password: string }> { + return api.post<{ password: string }>('/api/me/subsonic-password', {}); +} + +export async function clearSubsonicPassword(): Promise { + await api.del('/api/me/subsonic-password'); +} + // Submits the browser's current IANA timezone for the authenticated // user. Called from the auth store on login + bootstrap + once weekly // (cadence tracked client-side in localStorage). Failures are diff --git a/web/src/routes/settings/+page.svelte b/web/src/routes/settings/+page.svelte index 4a804787..d4abe4e3 100644 --- a/web/src/routes/settings/+page.svelte +++ b/web/src/routes/settings/+page.svelte @@ -1,4 +1,5 @@ {pageTitle('Settings')} @@ -553,9 +628,51 @@ - + +
+

Subsonic password

+

+ For Subsonic apps that can't use an API token and ask for a username and password instead. + Sign those apps in with your username and this password, not your login password. + Minstrel generates it, and has to store it readable for these apps to work, so it is never + your login password. Use the API token where the app supports it. +

+ {#if subsonicEnabled !== null} +

+ {subsonicEnabled ? 'A Subsonic password is set.' : "No Subsonic password is set; these apps can't sign in."} +

+ {/if} + {#if subsonicPassword} + + {subsonicPassword} + +

Copy this now. It won't be shown again.

+ {/if} +
+ {#if subsonicPassword} + + {/if} + + {#if subsonicEnabled} + + {/if} +
+
+ +
diff --git a/web/src/routes/settings/settings.test.ts b/web/src/routes/settings/settings.test.ts index 9e2d09c6..5ffd2b16 100644 --- a/web/src/routes/settings/settings.test.ts +++ b/web/src/routes/settings/settings.test.ts @@ -1,5 +1,5 @@ import { afterEach, describe, expect, test, vi } from 'vitest'; -import { render, screen, fireEvent, waitFor } from '@testing-library/svelte'; +import { render, screen, fireEvent, waitFor, within } from '@testing-library/svelte'; import { readable, writable } from 'svelte/store'; import type { LBStatus } from '$lib/api/listenbrainz'; @@ -16,7 +16,10 @@ vi.mock('$lib/api/me', () => ({ changePassword: vi.fn(), // Default to a resolved value so the page's $effect doesn't crash // on `.then()` of undefined when individual tests don't override. - regenerateAPIToken: vi.fn() + regenerateAPIToken: vi.fn(), + getSubsonicPasswordStatus: vi.fn(), + generateSubsonicPassword: vi.fn(), + clearSubsonicPassword: vi.fn() })); // Mutable holder so individual tests can inject populated metrics; @@ -45,7 +48,10 @@ import { import { updateProfile, changePassword, - regenerateAPIToken + regenerateAPIToken, + getSubsonicPasswordStatus, + generateSubsonicPassword, + clearSubsonicPassword } from '$lib/api/me'; function mockStatusStore(data: LBStatus) { @@ -361,3 +367,48 @@ describe('Settings page — API Token card', () => { expect(screen.queryByRole('button', { name: /^copy$/i })).toBeNull(); }); }); + +describe('Settings page — Subsonic password card', () => { + function mockLB() { + (createLBStatusQuery as ReturnType).mockReturnValue( + mockStatusStore({ enabled: false, token_set: false, last_scrobbled_at: null }) + ); + (createTokenMutation as ReturnType).mockReturnValue(mockMutationStore()); + (createEnabledMutation as ReturnType).mockReturnValue(mockMutationStore()); + } + + test('with none set, Generate creates one on the first click and shows it once', async () => { + mockLB(); + (getSubsonicPasswordStatus as ReturnType).mockResolvedValue({ enabled: false }); + (generateSubsonicPassword as ReturnType).mockResolvedValue({ password: 'gen_pw_123' }); + render(SettingsPage); + await waitFor(() => expect(screen.getByText(/no subsonic password is set/i)).toBeInTheDocument()); + expect(screen.queryByRole('button', { name: /turn off/i })).not.toBeInTheDocument(); + + await fireEvent.click(screen.getByRole('button', { name: /^generate$/i })); + await waitFor(() => expect(generateSubsonicPassword).toHaveBeenCalledTimes(1)); + await waitFor(() => expect(screen.getByText('gen_pw_123')).toBeInTheDocument()); + expect(screen.getByText(/a subsonic password is set/i)).toBeInTheDocument(); + }); + + test('with one set, Regenerate and Turn off each need a second click', async () => { + mockLB(); + (getSubsonicPasswordStatus as ReturnType).mockResolvedValue({ enabled: true }); + (clearSubsonicPassword as ReturnType).mockResolvedValue(undefined); + render(SettingsPage); + // The API token card has a Regenerate button too; the Subsonic card's is + // the one beside Turn off. + const turnOff = await screen.findByRole('button', { name: /turn off/i }); + const subsonicRegen = within(turnOff.parentElement!).getByRole('button', { name: /^regenerate$/i }); + + await fireEvent.click(subsonicRegen); + expect(generateSubsonicPassword).not.toHaveBeenCalled(); + expect(subsonicRegen).toHaveTextContent(/click again to confirm/i); + + await fireEvent.click(turnOff); + expect(clearSubsonicPassword).not.toHaveBeenCalled(); + await fireEvent.click(turnOff); + await waitFor(() => expect(clearSubsonicPassword).toHaveBeenCalledTimes(1)); + await waitFor(() => expect(screen.getByText(/no subsonic password is set/i)).toBeInTheDocument()); + }); +});