fix(auth): the Subsonic password is generated, never the login password (M462 #5026)
release / govulncheck (push) Successful in 39s
release / web (push) Successful in 1m8s
release / go (push) Successful in 1m30s
release / integration (push) Successful in 4m37s
release / android (push) Successful in 5m56s
release / Build signed APK (releases and dev) (push) Successful in 5m55s
release / Attach APK to the Release (tag releases only) (push) Skipped
release / Build + push container image (push) Successful in 1m23s
release / Verify release artifacts (tag releases only) (push) Skipped
release / govulncheck (push) Successful in 39s
release / web (push) Successful in 1m8s
release / go (push) Successful in 1m30s
release / integration (push) Successful in 4m37s
release / android (push) Successful in 5m56s
release / Build signed APK (releases and dev) (push) Successful in 5m55s
release / Attach APK to the Release (tag releases only) (push) Skipped
release / Build + push container image (push) Successful in 1m23s
release / Verify release artifacts (tag releases only) (push) Skipped
`minstrel admin reset-password` copied the new login password into subsonic_password, which is stored in plain text because Subsonic t/s sign-in needs it. Every account recovered through the CLI had its login password readable in the database, and changing the password later left the copy behind. - reset-password now changes only password_hash. - Migration 0064 clears every subsonic_password, removing the copies. - Settings gets a Subsonic password card: the server generates a random password, shows it once, and it can be regenerated or turned off (GET/POST/DELETE /api/me/subsonic-password, audited). Generated rather than user-chosen so it can never be a reused password. - docs/security.md describes the separate password instead of the known issue. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
@@ -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)
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
@@ -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)
|
||||
}
|
||||
}
|
||||
@@ -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;
|
||||
|
||||
@@ -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 {
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -0,0 +1,2 @@
|
||||
-- The cleared values are gone and were never meant to be kept; nothing to undo.
|
||||
SELECT 1;
|
||||
@@ -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;
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user