diff --git a/cmd/minstrel/main.go b/cmd/minstrel/main.go index 7768650d..77771737 100644 --- a/cmd/minstrel/main.go +++ b/cmd/minstrel/main.go @@ -35,6 +35,7 @@ import ( "git.fabledsword.com/bvandeusen/minstrel/internal/requests" "git.fabledsword.com/bvandeusen/minstrel/internal/scrobble" "git.fabledsword.com/bvandeusen/minstrel/internal/scrobble/listenbrainz" + "git.fabledsword.com/bvandeusen/minstrel/internal/secret" "git.fabledsword.com/bvandeusen/minstrel/internal/server" "git.fabledsword.com/bvandeusen/minstrel/internal/similarity" "git.fabledsword.com/bvandeusen/minstrel/internal/subsonic" @@ -128,6 +129,18 @@ func run() error { } defer pool.Close() + // Stored credentials are sealed under a key derived from the stream + // secret. Install it before anything reads one, and seal any value still + // stored in the clear. + secret.Install(cfg.StreamSecret) + sealed, err := secret.SealStored(ctx, pool) + if err != nil { + return fmt.Errorf("seal stored credentials: %w", err) + } + if sealed > 0 { + logger.Info("sealed stored credentials", "count", sealed) + } + // Fingerprinting settings: one instance, shared by the scanner, // the fingerprint backfill, the duplicate sweep and the admin API, so a save // reaches all of them without a restart. A load failure is logged, not fatal: diff --git a/docs/security.md b/docs/security.md index 7ed8472a..1864981e 100644 --- a/docs/security.md +++ b/docs/security.md @@ -49,22 +49,42 @@ cross-origin preflight. Requests authenticated with a bearer token or the Subsonic query parameters carry nothing a browser attaches on its own, so they aren't checked. -## Subsonic sign-in, and the one password stored in plain text +## Credentials Minstrel has to read back + +Some credentials can't be hashed, because Minstrel has to send or compare the +value itself: +- the API keys and passwords for indexers, Prowlarr and download clients; +- the AcoustID key, and the cover-art and tag provider keys; +- the SMTP password; +- each user's ListenBrainz token and Subsonic password. + +These are encrypted in the database (AES-256-GCM). Each column has its own key, +derived from the stream secret (`MINSTREL_STREAM_SECRET`, or the +`stream_secret` file in the data directory), and each value is bound to its +row. The stream secret is never stored in the database, so a copy of the +database alone doesn't reveal any of them. None of them is ever sent back to a +browser: Settings only shows whether one is set. + +**Keep the stream secret with the data directory, and don't change it +casually.** If it changes, every stored credential reads as not set, and each +one has to be entered again in Settings (Subsonic passwords generated again). +Minstrel logs a warning for each one that no longer opens. + +## Subsonic sign-in 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, and it is the only credential -Minstrel keeps unhashed. +can read: the Subsonic password, encrypted as described above. - **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. - **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. + or turn it off. Because it is random, even someone holding both the database + and the stream secret gains 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. diff --git a/internal/acquire/build.go b/internal/acquire/build.go index 882d528c..15760b53 100644 --- a/internal/acquire/build.go +++ b/internal/acquire/build.go @@ -24,7 +24,7 @@ func Build(ctx context.Context, pool *pgxpool.Pool) (*Service, dbq.AcquisitionSe if err != nil { return nil, settings, fmt.Errorf("acquire: read search sources: %w", err) } - clients, err := q.ListUsableAcquisitionServices(ctx, JobDownload) + clients, err := usableServices(ctx, q, JobDownload) if err != nil { return nil, settings, fmt.Errorf("acquire: read download clients: %w", err) } diff --git a/internal/acquire/settings.go b/internal/acquire/settings.go index e71284a3..37726d61 100644 --- a/internal/acquire/settings.go +++ b/internal/acquire/settings.go @@ -23,6 +23,7 @@ import ( "git.fabledsword.com/bvandeusen/minstrel/internal/exchange" "git.fabledsword.com/bvandeusen/minstrel/internal/indexer" "git.fabledsword.com/bvandeusen/minstrel/internal/prowlarr" + "git.fabledsword.com/bvandeusen/minstrel/internal/secret" ) // Kinds: which list a row appears in. @@ -159,7 +160,7 @@ func DownloadClient(row dbq.AcquisitionService) (downloader.Driver, error) { // DownloadClientFor builds the download client a grab went to, by the code // of its acquisition_services row. func DownloadClientFor(ctx context.Context, pool *pgxpool.Pool, code string) (downloader.Driver, error) { - row, err := dbq.New(pool).GetAcquisitionService(ctx, code) + row, err := getService(ctx, dbq.New(pool), code) if err != nil { if errors.Is(err, pgx.ErrNoRows) { return nil, fmt.Errorf("the download client %q is no longer set up", code) @@ -171,7 +172,7 @@ func DownloadClientFor(ctx context.Context, pool *pgxpool.Pool, code string) (do // Sources builds every enabled search source. func Sources(ctx context.Context, pool *pgxpool.Pool) ([]Source, error) { - rows, err := dbq.New(pool).ListUsableAcquisitionServices(ctx, JobSearch) + rows, err := usableServices(ctx, dbq.New(pool), JobSearch) if err != nil { return nil, err } @@ -182,4 +183,33 @@ func Sources(ctx context.Context, pool *pgxpool.Pool) ([]Source, error) { return out, nil } +// getService and usableServices are the only reads of a full +// acquisition_services row. They hand back the credentials opened, so the +// clients built from a row are given the key and password themselves. +func getService(ctx context.Context, q *dbq.Queries, code string) (dbq.AcquisitionService, error) { + row, err := q.GetAcquisitionService(ctx, code) + if err != nil { + return row, err + } + return revealed(row), nil +} + +func usableServices(ctx context.Context, q *dbq.Queries, job string) ([]dbq.AcquisitionService, error) { + rows, err := q.ListUsableAcquisitionServices(ctx, job) + if err != nil { + return nil, err + } + for i := range rows { + rows[i] = revealed(rows[i]) + } + return rows, nil +} + +// revealed is row with its sealed credentials opened. +func revealed(row dbq.AcquisitionService) dbq.AcquisitionService { + row.ApiKey = secret.Reveal(secret.AcquisitionAPIKey.Purpose(), row.Code, row.ApiKey) + row.Password = secret.Reveal(secret.AcquisitionPassword.Purpose(), row.Code, row.Password) + return row +} + func msDuration(ms int32) time.Duration { return time.Duration(ms) * time.Millisecond } diff --git a/internal/acquire/store.go b/internal/acquire/store.go index 0245be01..b7141fda 100644 --- a/internal/acquire/store.go +++ b/internal/acquire/store.go @@ -11,6 +11,7 @@ import ( "github.com/jackc/pgx/v5/pgxpool" "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" + "git.fabledsword.com/bvandeusen/minstrel/internal/secret" ) // ErrNotFound is a code with no row. @@ -49,7 +50,7 @@ func (s *Store) List(ctx context.Context) ([]dbq.ListAcquisitionServicesRow, err // Get returns one row, secrets included: for building a client only. func (s *Store) Get(ctx context.Context, code string) (dbq.AcquisitionService, error) { - row, err := dbq.New(s.pool).GetAcquisitionService(ctx, code) + row, err := getService(ctx, dbq.New(s.pool), code) if errors.Is(err, pgx.ErrNoRows) { return row, ErrNotFound } @@ -163,19 +164,27 @@ func (s *Store) Delete(ctx context.Context, code string) error { return nil } -// SetAPIKey stores a key, trimmed; empty clears it. +// SetAPIKey stores a key, trimmed and sealed; empty clears it. func (s *Store) SetAPIKey(ctx context.Context, code, key string) error { + sealed, err := secret.Seal(secret.AcquisitionAPIKey.Purpose(), code, strings.TrimSpace(key)) + if err != nil { + return err + } n, err := dbq.New(s.pool).SetAcquisitionServiceAPIKey(ctx, dbq.SetAcquisitionServiceAPIKeyParams{ - Code: code, ApiKey: strings.TrimSpace(key), + Code: code, ApiKey: sealed, }) return affected(n, err) } -// SetPassword stores a password as typed (spaces are part of a password); -// empty clears it. +// SetPassword stores a password as typed (spaces are part of a password), +// sealed; empty clears it. func (s *Store) SetPassword(ctx context.Context, code, password string) error { + sealed, err := secret.Seal(secret.AcquisitionPassword.Purpose(), code, password) + if err != nil { + return err + } n, err := dbq.New(s.pool).SetAcquisitionServicePassword(ctx, dbq.SetAcquisitionServicePasswordParams{ - Code: code, Password: password, + Code: code, Password: sealed, }) return affected(n, err) } diff --git a/internal/acquire/store_test.go b/internal/acquire/store_test.go index 37c369da..420a38b0 100644 --- a/internal/acquire/store_test.go +++ b/internal/acquire/store_test.go @@ -12,6 +12,7 @@ import ( "git.fabledsword.com/bvandeusen/minstrel/internal/db" "git.fabledsword.com/bvandeusen/minstrel/internal/dbtest" + "git.fabledsword.com/bvandeusen/minstrel/internal/secret" ) func newPool(t *testing.T) *pgxpool.Pool { @@ -83,6 +84,15 @@ func TestStore_AddsConfiguresAndRemoves_Integration(t *testing.T) { if err != nil || full.ApiKey != "key" { t.Errorf("stored key = %q (err %v), want it trimmed", full.ApiKey, err) } + // The column holds the key sealed, never the key. + var raw string + if err := s.pool.QueryRow(ctx, + "SELECT api_key FROM acquisition_services WHERE code = $1", first.Code).Scan(&raw); err != nil { + t.Fatalf("read api_key: %v", err) + } + if raw == "key" || !secret.IsSealed(raw) { + t.Errorf("api_key column = %q, want it sealed", raw) + } rows, err := s.List(ctx) if err != nil || len(rows) != 2 { t.Fatalf("list: %d rows, err %v", len(rows), err) diff --git a/internal/api/admin_smtp.go b/internal/api/admin_smtp.go index fd2c2378..7a04b17b 100644 --- a/internal/api/admin_smtp.go +++ b/internal/api/admin_smtp.go @@ -10,6 +10,7 @@ import ( "git.fabledsword.com/bvandeusen/minstrel/internal/apierror" "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" "git.fabledsword.com/bvandeusen/minstrel/internal/mailer" + "git.fabledsword.com/bvandeusen/minstrel/internal/secret" ) type smtpConfigResp struct { @@ -76,11 +77,15 @@ func (h *handlers) handleUpdateSMTPConfig(w http.ResponseWriter, r *http.Request writeErrWithLog(w, h.logger, "admin smtp: get failed", apierror.Internal(err)) return } - // Preserve existing password when the request sends "" (so the - // admin doesn't have to re-enter it on every save). - password := req.Password - if password == "" { - password = current.Password + // Preserve the stored (sealed) password when the request sends "", so + // the admin doesn't have to re-enter it on every save. + password := current.Password + if req.Password != "" { + password, err = secret.Seal(secret.SMTPPassword.Purpose(), "", req.Password) + if err != nil { + writeErrWithLog(w, h.logger, "admin smtp: seal password", apierror.Internal(err)) + return + } } if err := q.UpdateSMTPConfig(r.Context(), dbq.UpdateSMTPConfigParams{ Enabled: req.Enabled, diff --git a/internal/api/listenbrainz.go b/internal/api/listenbrainz.go index 571b5a71..3a53c4b9 100644 --- a/internal/api/listenbrainz.go +++ b/internal/api/listenbrainz.go @@ -11,6 +11,7 @@ import ( "git.fabledsword.com/bvandeusen/minstrel/internal/apierror" "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" + "git.fabledsword.com/bvandeusen/minstrel/internal/secret" ) // LBStatus is the GET response body and the PUT response body. Token is @@ -53,10 +54,11 @@ func (h *handlers) handlePutListenBrainz(w http.ResponseWriter, r *http.Request) q := dbq.New(h.pool) if body.Token != nil { - t := *body.Token - var tokenPtr *string - if t != "" { - tokenPtr = &t + tokenPtr, err := secret.SealPtr(secret.ListenBrainzToken.Purpose(), user.ID.String(), body.Token) + if err != nil { + h.logger.Error("api: seal listenbrainz token", "err", err) + writeErr(w, apierror.InternalMsg("update failed", err)) + return } if err := q.SetListenBrainzToken(r.Context(), dbq.SetListenBrainzTokenParams{ ID: user.ID, ListenbrainzToken: tokenPtr, diff --git a/internal/api/me_subsonic_password.go b/internal/api/me_subsonic_password.go index 6a0009fe..9f3e1778 100644 --- a/internal/api/me_subsonic_password.go +++ b/internal/api/me_subsonic_password.go @@ -8,6 +8,7 @@ import ( "git.fabledsword.com/bvandeusen/minstrel/internal/apierror" "git.fabledsword.com/bvandeusen/minstrel/internal/audit" "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" + "git.fabledsword.com/bvandeusen/minstrel/internal/secret" ) // The Subsonic password is for clients that sign in with Subsonic's t/s @@ -51,9 +52,15 @@ func (h *handlers) handleGenerateMySubsonicPassword(w http.ResponseWriter, r *ht return } pw := base64.RawURLEncoding.EncodeToString(b) + sealed, err := secret.Seal(secret.SubsonicPassword.Purpose(), user.ID.String(), pw) + if err != nil { + h.logger.Error("generate subsonic password: seal failed", "err", err) + writeErr(w, apierror.Internal(err)) + return + } if err := dbq.New(h.pool).SetSubsonicPassword(r.Context(), dbq.SetSubsonicPasswordParams{ ID: user.ID, - SubsonicPassword: &pw, + SubsonicPassword: &sealed, }); err != nil { h.logger.Error("generate subsonic password: update failed", "err", err) writeErr(w, apierror.Internal(err)) diff --git a/internal/api/me_subsonic_password_test.go b/internal/api/me_subsonic_password_test.go index f908cab1..46a842c3 100644 --- a/internal/api/me_subsonic_password_test.go +++ b/internal/api/me_subsonic_password_test.go @@ -11,6 +11,7 @@ import ( "github.com/go-chi/chi/v5" "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" + "git.fabledsword.com/bvandeusen/minstrel/internal/secret" ) func newMeSubsonicPasswordRouter(h *handlers) chi.Router { @@ -21,6 +22,8 @@ func newMeSubsonicPasswordRouter(h *handlers) chi.Router { return r } +// readSubsonicPassword returns the stored Subsonic password opened. It fails +// the test if the column holds the password in the clear. func readSubsonicPassword(t *testing.T, h *handlers, user dbq.User) *string { t.Helper() var pw *string @@ -28,7 +31,17 @@ func readSubsonicPassword(t *testing.T, h *handlers, user dbq.User) *string { "SELECT subsonic_password FROM users WHERE id = $1", user.ID).Scan(&pw); err != nil { t.Fatalf("read subsonic_password: %v", err) } - return pw + if pw == nil { + return nil + } + if !secret.IsSealed(*pw) { + t.Fatalf("subsonic_password is stored in the clear") + } + plain, err := secret.Open(secret.SubsonicPassword.Purpose(), user.ID.String(), *pw) + if err != nil { + t.Fatalf("open subsonic_password: %v", err) + } + return &plain } func TestSubsonicPassword_GenerateIsRandomAndNotTheLoginPassword(t *testing.T) { diff --git a/internal/coverart/settings.go b/internal/coverart/settings.go index d2e51b14..b4069aa1 100644 --- a/internal/coverart/settings.go +++ b/internal/coverart/settings.go @@ -13,6 +13,7 @@ import ( "github.com/jackc/pgx/v5/pgxpool" "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" + "git.fabledsword.com/bvandeusen/minstrel/internal/secret" ) // SettingsService reconciles the registry of compiled-in providers @@ -112,7 +113,7 @@ func (s *SettingsService) reconcile(ctx context.Context) error { } key := "" if row.ApiKey != nil { - key = *row.ApiKey + key = secret.Reveal(secret.CoverArtAPIKey.Purpose(), row.ProviderID, *row.ApiKey) } s.enabledIDs[p.ID()] = row.Enabled s.apiKeys[p.ID()] = key @@ -204,11 +205,15 @@ func (s *SettingsService) UpdateProvider(ctx context.Context, providerID string, // (apiKeyChanged flag) and $4 (api_key value). The actual generated // field for $4 is ApiKey *string. apiKeyChanged := patch.APIKey != nil + sealedKey, err := secret.SealPtr(secret.CoverArtAPIKey.Purpose(), providerID, patch.APIKey) + if err != nil { + return false, fmt.Errorf("seal provider api key: %w", err) + } if err := q.UpdateProviderSettings(ctx, dbq.UpdateProviderSettingsParams{ ProviderID: providerID, Enabled: enabledVal, Column3: apiKeyChanged, - ApiKey: patch.APIKey, + ApiKey: sealedKey, }); err != nil { return false, fmt.Errorf("update provider settings: %w", err) } @@ -225,7 +230,7 @@ func (s *SettingsService) UpdateProvider(ctx context.Context, providerID string, for _, r := range rows { key := "" if r.ApiKey != nil { - key = *r.ApiKey + key = secret.Reveal(secret.CoverArtAPIKey.Purpose(), r.ProviderID, *r.ApiKey) } s.enabledIDs[r.ProviderID] = r.Enabled s.apiKeys[r.ProviderID] = key diff --git a/internal/dbtest/secret.go b/internal/dbtest/secret.go new file mode 100644 index 00000000..49e68fc1 --- /dev/null +++ b/internal/dbtest/secret.go @@ -0,0 +1,10 @@ +package dbtest + +import "git.fabledsword.com/bvandeusen/minstrel/internal/secret" + +// Integration tests run with a master key installed, as the server runs +// after startup, so code that seals or opens a stored credential behaves as +// it does in production. +func init() { + secret.Install([]byte("minstrel integration tests: not a deployment key")) +} diff --git a/internal/library/acoustid_settings.go b/internal/library/acoustid_settings.go index f504219d..27e8d963 100644 --- a/internal/library/acoustid_settings.go +++ b/internal/library/acoustid_settings.go @@ -10,6 +10,7 @@ import ( "github.com/jackc/pgx/v5/pgxpool" "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" + "git.fabledsword.com/bvandeusen/minstrel/internal/secret" ) // AcoustID lookup settings: a database row, changed without a restart, shared @@ -97,8 +98,12 @@ func (s *AcoustIDSettingsService) Set(ctx context.Context, in AcoustIDSettingsUp } params := dbq.UpdateAcoustIDSettingsParams{Enabled: in.Enabled, MinScore: in.MinScore} if in.APIKey != nil { + sealed, err := secret.Seal(secret.AcoustIDAPIKey.Purpose(), "", *in.APIKey) + if err != nil { + return AcoustIDSettings{}, fmt.Errorf("acoustid settings: seal key: %w", err) + } params.SetApiKey = true - params.ApiKey = *in.APIKey + params.ApiKey = sealed } row, err := dbq.New(s.pool).UpdateAcoustIDSettings(ctx, params) if err != nil { @@ -126,7 +131,7 @@ func (s *AcoustIDSettingsService) Changed() <-chan struct{} { func acoustIDSettingsFromRow(row dbq.AcoustidSetting) AcoustIDSettings { out := AcoustIDSettings{Enabled: row.Enabled, MinScore: row.MinScore, UpdatedAt: row.UpdatedAt.Time} if row.ApiKey != nil { - out.APIKey = *row.ApiKey + out.APIKey = secret.Reveal(secret.AcoustIDAPIKey.Purpose(), "", *row.ApiKey) } return out } diff --git a/internal/mailer/mailer.go b/internal/mailer/mailer.go index 090017d5..43c782ae 100644 --- a/internal/mailer/mailer.go +++ b/internal/mailer/mailer.go @@ -23,6 +23,7 @@ import ( "github.com/jackc/pgx/v5/pgxpool" "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" + "git.fabledsword.com/bvandeusen/minstrel/internal/secret" ) // ErrNotConfigured is returned when smtp_config.enabled is false or @@ -68,7 +69,8 @@ func (s *SMTPSender) Send(ctx context.Context, to, subject, textBody, htmlBody s var auth smtp.Auth if cfg.Username != "" { - auth = smtp.PlainAuth("", cfg.Username, cfg.Password, cfg.Host) + password := secret.Reveal(secret.SMTPPassword.Purpose(), "", cfg.Password) + auth = smtp.PlainAuth("", cfg.Username, password, cfg.Host) } // We respect the use_tls flag: if true, we require STARTTLS via diff --git a/internal/scrobble/queue_test.go b/internal/scrobble/queue_test.go index e3a22c94..69dd33a2 100644 --- a/internal/scrobble/queue_test.go +++ b/internal/scrobble/queue_test.go @@ -13,6 +13,7 @@ import ( "git.fabledsword.com/bvandeusen/minstrel/internal/db" "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" "git.fabledsword.com/bvandeusen/minstrel/internal/dbtest" + "git.fabledsword.com/bvandeusen/minstrel/internal/secret" ) func testPool(t *testing.T) (*pgxpool.Pool, *dbq.Queries) { @@ -63,8 +64,13 @@ func seed(t *testing.T, opts seedOpts) setup { t.Fatalf("user: %v", err) } if opts.lbToken != "" { + // Stored sealed, as the settings route stores it. + sealed, err := secret.Seal(secret.ListenBrainzToken.Purpose(), u.ID.String(), opts.lbToken) + if err != nil { + t.Fatalf("seal token: %v", err) + } if err := q.SetListenBrainzToken(ctx, dbq.SetListenBrainzTokenParams{ - ID: u.ID, ListenbrainzToken: &opts.lbToken, + ID: u.ID, ListenbrainzToken: &sealed, }); err != nil { t.Fatalf("token: %v", err) } diff --git a/internal/scrobble/worker.go b/internal/scrobble/worker.go index 05c2ad27..53fcfe8b 100644 --- a/internal/scrobble/worker.go +++ b/internal/scrobble/worker.go @@ -11,6 +11,7 @@ import ( "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" "git.fabledsword.com/bvandeusen/minstrel/internal/scrobble/listenbrainz" + "git.fabledsword.com/bvandeusen/minstrel/internal/secret" ) // backoffSchedule maps the failure count (1-indexed) to the delay before @@ -98,7 +99,11 @@ func (w *Worker) tickOnce(ctx context.Context) error { key := r.UserID.String() ub := batches[key] if ub == nil { - ub = &userBatch{token: *r.LbToken, userID: r.UserID} + token := secret.Reveal(secret.ListenBrainzToken.Purpose(), key, *r.LbToken) + if token == "" { + continue + } + ub = &userBatch{token: token, userID: r.UserID} batches[key] = ub } ub.queueIDs = append(ub.queueIDs, r.QueueID) diff --git a/internal/secret/secret.go b/internal/secret/secret.go new file mode 100644 index 00000000..cda7fc45 --- /dev/null +++ b/internal/secret/secret.go @@ -0,0 +1,179 @@ +// Package secret seals the credentials Minstrel keeps for outside systems +// (indexer and download-client keys, the SMTP password, provider API keys, +// ListenBrainz tokens, Subsonic passwords) so the database holds ciphertext, +// not the credential. +// +// Each column has its own key, derived from the install's master key with +// HKDF-SHA256 and the column as the info string, so a value sealed for one +// column cannot be opened as another, and the master key never touches a +// cipher. Each value is also bound to the row it belongs to (its owner), so a +// sealed value copied onto another row does not open there. +// +// The master key is the stream secret (MINSTREL_STREAM_SECRET, or the +// stream_secret file in the data directory). It lives outside the database, +// so a copy of the database alone opens nothing. Changing it makes every +// sealed value unreadable: each one reads as unset and has to be entered +// again in Settings. +package secret + +import ( + "crypto/aes" + "crypto/cipher" + "crypto/hkdf" + "crypto/rand" + "crypto/sha256" + "encoding/base64" + "errors" + "fmt" + "log/slog" + "strings" + "sync" +) + +// ErrNoKey is a seal or open before Install has given the package a key. +var ErrNoKey = errors.New("secret: no master key installed") + +// ErrCorrupt is a stored value that does not open: the master key changed, +// the value was edited by hand, or it belongs to a different row. +var ErrCorrupt = errors.New("secret: the stored value does not open") + +// sealedPrefix versions the format, so a future change of cipher can still +// read what this one wrote. Nothing a person types starts with it by chance +// often enough to matter, and Open rejects anything without it. +const sealedPrefix = "v1." + +var b64 = base64.RawURLEncoding + +var ( + mu sync.RWMutex + master []byte + boxes map[string]cipher.AEAD +) + +// Install sets the master key every Seal and Open derives from. The server +// calls it once at startup, before anything reads a credential. +func Install(key []byte) { + mu.Lock() + defer mu.Unlock() + master = append([]byte(nil), key...) + boxes = map[string]cipher.AEAD{} +} + +// aead returns purpose's cipher, deriving it on first use. +func aead(purpose string) (cipher.AEAD, error) { + mu.RLock() + a, ok := boxes[purpose] + key := master + mu.RUnlock() + if ok { + return a, nil + } + if len(key) == 0 { + return nil, ErrNoKey + } + derived, err := hkdf.Key(sha256.New, key, nil, "minstrel/"+purpose, 32) + if err != nil { + return nil, fmt.Errorf("secret: derive: %w", err) + } + block, err := aes.NewCipher(derived) + if err != nil { + return nil, fmt.Errorf("secret: %w", err) + } + a, err = cipher.NewGCM(block) + if err != nil { + return nil, fmt.Errorf("secret: %w", err) + } + mu.Lock() + if boxes != nil { + boxes[purpose] = a + } + mu.Unlock() + return a, nil +} + +// IsSealed reports whether stored is in the sealed format. +func IsSealed(stored string) bool { return strings.HasPrefix(stored, sealedPrefix) } + +// Seal encrypts plain for purpose (the column it is stored in) and owner (the +// row it belongs to). The empty string stays empty, so "is one set?" stays a +// test for the empty string. +func Seal(purpose, owner, plain string) (string, error) { + if plain == "" { + return "", nil + } + a, err := aead(purpose) + if err != nil { + return "", err + } + nonce := make([]byte, a.NonceSize()) + if _, err := rand.Read(nonce); err != nil { + return "", fmt.Errorf("secret: nonce: %w", err) + } + out := a.Seal(nonce, nonce, []byte(plain), []byte(owner)) + return sealedPrefix + b64.EncodeToString(out), nil +} + +// Open decrypts what Seal stored for the same purpose and owner. The empty +// string opens to itself. +func Open(purpose, owner, stored string) (string, error) { + if stored == "" { + return "", nil + } + a, err := aead(purpose) + if err != nil { + return "", err + } + raw, ok := strings.CutPrefix(stored, sealedPrefix) + if !ok { + return "", ErrCorrupt + } + data, err := b64.DecodeString(raw) + n := a.NonceSize() + if err != nil || len(data) < n { + return "", ErrCorrupt + } + plain, err := a.Open(nil, data[:n], data[n:], []byte(owner)) + if err != nil { + return "", ErrCorrupt + } + return string(plain), nil +} + +// Reveal is Open for a caller about to use the credential: a value that does +// not open is logged and read as unset, which is what it has become. The +// admin sees "not set" in Settings and enters it again. +func Reveal(purpose, owner, stored string) string { + plain, err := Open(purpose, owner, stored) + if err != nil { + slog.Warn("secret: a stored credential does not open; it reads as unset until it is entered again", + "column", purpose, "err", err) + return "" + } + return plain +} + +// RevealPtr is Reveal for a nullable column: nil stays nil, and a value that +// does not open reads as nil. +func RevealPtr(purpose, owner string, stored *string) *string { + if stored == nil { + return nil + } + plain := Reveal(purpose, owner, *stored) + if plain == "" { + return nil + } + return &plain +} + +// SealPtr is Seal for a nullable column: nil and the empty string both store +// NULL. +func SealPtr(purpose, owner string, plain *string) (*string, error) { + if plain == nil || *plain == "" { + return nil, nil + } + sealed, err := Seal(purpose, owner, *plain) + if err != nil { + return nil, err + } + return &sealed, nil +} diff --git a/internal/secret/secret_test.go b/internal/secret/secret_test.go new file mode 100644 index 00000000..70982e2d --- /dev/null +++ b/internal/secret/secret_test.go @@ -0,0 +1,110 @@ +package secret + +import ( + "errors" + "strings" + "testing" +) + +func withKey(t *testing.T, key string) { + t.Helper() + mu.RLock() + prev := master + mu.RUnlock() + Install([]byte(key)) + t.Cleanup(func() { Install(prev) }) +} + +func TestSealOpensForTheSamePurposeAndOwner(t *testing.T) { + withKey(t, "test master key") + sealed, err := Seal("t.col", "row-1", "hunter2") + if err != nil { + t.Fatal(err) + } + if !IsSealed(sealed) || strings.Contains(sealed, "hunter2") { + t.Fatalf("sealed = %q, want an opaque v1. value", sealed) + } + plain, err := Open("t.col", "row-1", sealed) + if err != nil || plain != "hunter2" { + t.Fatalf("Open = %q, %v; want hunter2", plain, err) + } + again, _ := Seal("t.col", "row-1", "hunter2") + if again == sealed { + t.Error("two seals of one value are identical; the nonce is not random") + } +} + +func TestOpenRefusesAnythingElse(t *testing.T) { + withKey(t, "test master key") + sealed, _ := Seal("t.col", "row-1", "hunter2") + + cases := map[string]func() (string, error){ + "another owner": func() (string, error) { return Open("t.col", "row-2", sealed) }, + "another purpose": func() (string, error) { return Open("t.other", "row-1", sealed) }, + "plain text": func() (string, error) { return Open("t.col", "row-1", "hunter2") }, + "truncated": func() (string, error) { return Open("t.col", "row-1", sealed[:len(sealed)-4]) }, + "not base64": func() (string, error) { return Open("t.col", "row-1", "v1.!!!") }, + "too short": func() (string, error) { return Open("t.col", "row-1", "v1.AAAA") }, + } + for name, open := range cases { + if _, err := open(); !errors.Is(err, ErrCorrupt) { + t.Errorf("%s: err = %v, want ErrCorrupt", name, err) + } + } + + Install([]byte("a different key")) + if _, err := Open("t.col", "row-1", sealed); !errors.Is(err, ErrCorrupt) { + t.Errorf("another master key: err = %v, want ErrCorrupt", err) + } +} + +func TestEmptyStaysEmpty(t *testing.T) { + withKey(t, "test master key") + if s, err := Seal("t.col", "o", ""); s != "" || err != nil { + t.Errorf("Seal(\"\") = %q, %v; want the empty string", s, err) + } + if s, err := Open("t.col", "o", ""); s != "" || err != nil { + t.Errorf("Open(\"\") = %q, %v; want the empty string", s, err) + } + empty := "" + if p, err := SealPtr("t.col", "o", &empty); p != nil || err != nil { + t.Errorf("SealPtr(\"\") = %v, %v; want nil", p, err) + } + if p := RevealPtr("t.col", "o", nil); p != nil { + t.Errorf("RevealPtr(nil) = %v, want nil", p) + } +} + +func TestNoKeyIsAnErrorNotAPanic(t *testing.T) { + withKey(t, "") + if _, err := Seal("t.col", "o", "x"); !errors.Is(err, ErrNoKey) { + t.Errorf("Seal: err = %v, want ErrNoKey", err) + } + if _, err := Open("t.col", "o", "v1.AAAA"); !errors.Is(err, ErrNoKey) { + t.Errorf("Open: err = %v, want ErrNoKey", err) + } +} + +func TestRevealReadsAnUnopenableValueAsUnset(t *testing.T) { + withKey(t, "test master key") + if got := Reveal("t.col", "o", "not sealed"); got != "" { + t.Errorf("Reveal = %q, want the empty string", got) + } + bad := "not sealed" + if got := RevealPtr("t.col", "o", &bad); got != nil { + t.Errorf("RevealPtr = %q, want nil", *got) + } +} + +func TestEveryColumnHasADistinctPurpose(t *testing.T) { + seen := map[string]bool{} + for _, c := range Columns { + if seen[c.Purpose()] { + t.Errorf("%s is listed twice", c.Purpose()) + } + seen[c.Purpose()] = true + } + if !seen[AcquisitionAPIKey.Purpose()] || !seen[SubsonicPassword.Purpose()] { + t.Error("Columns is missing a declared column") + } +} diff --git a/internal/secret/stored.go b/internal/secret/stored.go new file mode 100644 index 00000000..cc68e0c7 --- /dev/null +++ b/internal/secret/stored.go @@ -0,0 +1,112 @@ +package secret + +import ( + "context" + "fmt" + + "github.com/jackc/pgx/v5" + "github.com/jackc/pgx/v5/pgxpool" +) + +// Column is one database column that holds a sealed credential. +// +// Its Purpose ("table.column") is the key-derivation label every Seal and +// Open of that column passes. The owner a value is bound to is the row's key +// as text (KeyColumn::text), or "" for a single-row settings table. +type Column struct { + Table string + Column string + KeyColumn string + Singleton bool +} + +// Purpose is the label Seal and Open take for this column. +func (c Column) Purpose() string { return c.Table + "." + c.Column } + +// The sealed columns, and the purpose strings their callers pass. Every +// credential Minstrel stores for an outside system, or that must be read back +// in the clear, is listed here. +var ( + AcquisitionAPIKey = Column{"acquisition_services", "api_key", "code", false} + AcquisitionPassword = Column{"acquisition_services", "password", "code", false} + AcoustIDAPIKey = Column{"acoustid_settings", "api_key", "id", true} + SMTPPassword = Column{"smtp_config", "password", "id", true} + ListenBrainzToken = Column{"users", "listenbrainz_token", "id", false} + SubsonicPassword = Column{"users", "subsonic_password", "id", false} + CoverArtAPIKey = Column{"cover_art_provider_settings", "api_key", "provider_id", false} + TagProviderAPIKey = Column{"tag_provider_settings", "api_key", "provider_id", false} +) + +// Columns is every sealed column, for SealStored and for anything that has to +// know which columns hold credentials (a backup leaves them out). +var Columns = []Column{ + AcquisitionAPIKey, AcquisitionPassword, AcoustIDAPIKey, SMTPPassword, + ListenBrainzToken, SubsonicPassword, CoverArtAPIKey, TagProviderAPIKey, +} + +// Owner is the owner string for a row whose key is key. A single-row table's +// rows are owned by "". +func (c Column) Owner(key string) string { + if c.Singleton { + return "" + } + return key +} + +// SealStored seals every value in Columns that is still stored in the clear, +// and returns how many it sealed. The server runs it at startup, after +// migrations and before anything reads a credential, so a database written +// before sealing existed is brought over in place. Each row is updated only +// if it still holds the value that was read, so a concurrent write wins. +func SealStored(ctx context.Context, pool *pgxpool.Pool) (int, error) { + total := 0 + for _, c := range Columns { + n, err := sealColumn(ctx, pool, c) + total += n + if err != nil { + return total, fmt.Errorf("seal %s: %w", c.Purpose(), err) + } + } + return total, nil +} + +func sealColumn(ctx context.Context, pool *pgxpool.Pool, c Column) (int, error) { + table := pgx.Identifier{c.Table}.Sanitize() + col := pgx.Identifier{c.Column}.Sanitize() + key := pgx.Identifier{c.KeyColumn}.Sanitize() + + rows, err := pool.Query(ctx, fmt.Sprintf( + `SELECT %s::text, %s FROM %s WHERE %s IS NOT NULL AND %s <> '' AND %s NOT LIKE 'v1.%%'`, + key, col, table, col, col, col)) + if err != nil { + return 0, err + } + type pending struct{ key, plain string } + var todo []pending + for rows.Next() { + var p pending + if err := rows.Scan(&p.key, &p.plain); err != nil { + rows.Close() + return 0, err + } + todo = append(todo, p) + } + rows.Close() + if err := rows.Err(); err != nil { + return 0, err + } + + update := fmt.Sprintf(`UPDATE %s SET %s = $1 WHERE %s::text = $2 AND %s = $3`, table, col, key, col) + sealed := 0 + for _, p := range todo { + v, err := Seal(c.Purpose(), c.Owner(p.key), p.plain) + if err != nil { + return sealed, err + } + if _, err := pool.Exec(ctx, update, v, p.key, p.plain); err != nil { + return sealed, err + } + sealed++ + } + return sealed, nil +} diff --git a/internal/secret/stored_test.go b/internal/secret/stored_test.go new file mode 100644 index 00000000..4db0c047 --- /dev/null +++ b/internal/secret/stored_test.go @@ -0,0 +1,154 @@ +package secret_test + +import ( + "context" + "fmt" + "io" + "log/slog" + "os" + "testing" + + "github.com/jackc/pgx/v5" + "github.com/jackc/pgx/v5/pgxpool" + + "git.fabledsword.com/bvandeusen/minstrel/internal/db" + "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" + "git.fabledsword.com/bvandeusen/minstrel/internal/dbtest" + "git.fabledsword.com/bvandeusen/minstrel/internal/secret" +) + +func newPool(t *testing.T) *pgxpool.Pool { + t.Helper() + if testing.Short() { + t.Skip("skipping integration test in -short mode") + } + dsn := os.Getenv("MINSTREL_TEST_DATABASE_URL") + if dsn == "" { + t.Skip("MINSTREL_TEST_DATABASE_URL not set") + } + if err := db.Migrate(dsn, slog.New(slog.NewTextHandler(io.Discard, nil))); err != nil { + t.Fatalf("migrate: %v", err) + } + pool, err := pgxpool.New(context.Background(), dsn) + if err != nil { + t.Fatalf("pool: %v", err) + } + t.Cleanup(pool.Close) + dbtest.ResetDB(t, pool) + return pool +} + +// Every listed column exists, so a renamed or dropped column fails here +// rather than leaving a credential unsealed. +func TestColumnsExistInTheSchema_Integration(t *testing.T) { + pool := newPool(t) + for _, c := range secret.Columns { + var n int + err := pool.QueryRow(context.Background(), + `SELECT count(*) FROM information_schema.columns + WHERE table_schema = 'public' AND table_name = $1 AND column_name IN ($2, $3)`, + c.Table, c.Column, c.KeyColumn).Scan(&n) + if err != nil || n != 2 { + t.Errorf("%s (key %s): found %d of 2 columns, err %v", c.Purpose(), c.KeyColumn, n, err) + } + } +} + +// A database written before sealing existed is sealed in place at startup, +// every value opens to what it held, and a second pass has nothing to do. +func TestSealStoredSealsWhatIsInTheClear_Integration(t *testing.T) { + pool := newPool(t) + ctx := context.Background() + q := dbq.New(pool) + + u, err := q.CreateUser(ctx, dbq.CreateUserParams{ + Username: dbtest.TestUserPrefix + "sealer", PasswordHash: "x", ApiTokenHash: "sealer", IsAdmin: false, + }) + if err != nil { + t.Fatalf("user: %v", err) + } + if _, err := q.CreateAcquisitionService(ctx, dbq.CreateAcquisitionServiceParams{ + Code: "seal-test", Kind: "search", Protocol: "prowlarr", Label: "Seal test", + Category: "", TimeoutMs: 1000, SearchesPerHour: 0, + }); err != nil { + t.Fatalf("acquisition row: %v", err) + } + seed := []string{ + `UPDATE acquisition_services SET api_key = 'acq-key', password = 'acq-pw' WHERE code = 'seal-test'`, + `UPDATE acoustid_settings SET api_key = 'acoustid-key' WHERE id = true`, + `UPDATE smtp_config SET password = 'smtp-pw' WHERE id = true`, + fmt.Sprintf(`UPDATE users SET listenbrainz_token = 'lb-token', subsonic_password = 'sub-pw' WHERE id = '%s'`, u.ID.String()), + `INSERT INTO cover_art_provider_settings (provider_id, enabled, display_order, api_key) + VALUES ('seal-test', false, 99, 'cover-key') + ON CONFLICT (provider_id) DO UPDATE SET api_key = EXCLUDED.api_key`, + `INSERT INTO tag_provider_settings (provider_id, enabled, display_order, api_key) + VALUES ('seal-test', false, 99, 'tag-key') + ON CONFLICT (provider_id) DO UPDATE SET api_key = EXCLUDED.api_key`, + } + for _, s := range seed { + if _, err := pool.Exec(ctx, s); err != nil { + t.Fatalf("seed %q: %v", s, err) + } + } + t.Cleanup(func() { + for _, s := range []string{ + `DELETE FROM cover_art_provider_settings WHERE provider_id = 'seal-test'`, + `DELETE FROM tag_provider_settings WHERE provider_id = 'seal-test'`, + `DELETE FROM acquisition_services WHERE code = 'seal-test'`, + `UPDATE acoustid_settings SET api_key = NULL WHERE id = true`, + `UPDATE smtp_config SET password = '' WHERE id = true`, + } { + _, _ = pool.Exec(context.Background(), s) + } + }) + + want := []struct { + col secret.Column + key string + plain string + }{ + {secret.AcquisitionAPIKey, "seal-test", "acq-key"}, + {secret.AcquisitionPassword, "seal-test", "acq-pw"}, + {secret.AcoustIDAPIKey, "true", "acoustid-key"}, + {secret.SMTPPassword, "true", "smtp-pw"}, + {secret.ListenBrainzToken, u.ID.String(), "lb-token"}, + {secret.SubsonicPassword, u.ID.String(), "sub-pw"}, + {secret.CoverArtAPIKey, "seal-test", "cover-key"}, + {secret.TagProviderAPIKey, "seal-test", "tag-key"}, + } + if len(want) != len(secret.Columns) { + t.Fatalf("the test covers %d columns, secret.Columns lists %d", len(want), len(secret.Columns)) + } + + n, err := secret.SealStored(ctx, pool) + if err != nil { + t.Fatalf("SealStored: %v", err) + } + if n < len(want) { + t.Errorf("sealed %d values, want at least %d", n, len(want)) + } + + for _, w := range want { + var stored string + query := fmt.Sprintf(`SELECT %s FROM %s WHERE %s::text = $1`, + pgx.Identifier{w.col.Column}.Sanitize(), pgx.Identifier{w.col.Table}.Sanitize(), + pgx.Identifier{w.col.KeyColumn}.Sanitize()) + if err := pool.QueryRow(ctx, query, w.key).Scan(&stored); err != nil { + t.Errorf("%s: read: %v", w.col.Purpose(), err) + continue + } + if !secret.IsSealed(stored) { + t.Errorf("%s = %q, want it sealed", w.col.Purpose(), stored) + continue + } + got, err := secret.Open(w.col.Purpose(), w.col.Owner(w.key), stored) + if err != nil || got != w.plain { + t.Errorf("%s opens to %q (err %v), want %q", w.col.Purpose(), got, err, w.plain) + } + } + + again, err := secret.SealStored(ctx, pool) + if err != nil || again != 0 { + t.Errorf("second pass sealed %d (err %v), want 0", again, err) + } +} diff --git a/internal/subsonic/auth.go b/internal/subsonic/auth.go index 68f33fb7..89338dd4 100644 --- a/internal/subsonic/auth.go +++ b/internal/subsonic/auth.go @@ -17,6 +17,7 @@ import ( "git.fabledsword.com/bvandeusen/minstrel/internal/auth" "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" + "git.fabledsword.com/bvandeusen/minstrel/internal/secret" ) // Config controls wire-format concessions the server makes for Subsonic @@ -101,11 +102,15 @@ func authenticate(r *http.Request, pool *pgxpool.Pool, cfg Config) (dbq.User, in return dbq.User{}, ErrGeneric, "Auth lookup failed" } + // The stored Subsonic password is sealed; both t+s and p= compare against + // it opened. One that does not open reads as not configured. + subsonicPassword := secret.RevealPtr(secret.SubsonicPassword.Purpose(), user.ID.String(), user.SubsonicPassword) + if token, salt := params.Get("t"), params.Get("s"); token != "" && salt != "" { - if user.SubsonicPassword == nil { + if subsonicPassword == nil { return dbq.User{}, ErrTokenNotSupported, "Subsonic token auth not configured; use apiKey or set a Subsonic password" } - sum := md5.Sum([]byte(*user.SubsonicPassword + salt)) + sum := md5.Sum([]byte(*subsonicPassword + salt)) expected := hex.EncodeToString(sum[:]) if subtle.ConstantTimeCompare([]byte(expected), []byte(token)) != 1 { return dbq.User{}, ErrWrongCredentials, "Wrong username or password" @@ -117,14 +122,14 @@ func authenticate(r *http.Request, pool *pgxpool.Pool, cfg Config) (dbq.User, in if !cfg.AllowPlaintextPassword { return dbq.User{}, ErrTokenNotSupported, "Plaintext password auth is disabled; use apiKey or t+s" } - if user.SubsonicPassword == nil { + if subsonicPassword == nil { return dbq.User{}, ErrTokenNotSupported, "Subsonic password not configured for this user" } decoded, err := decodePassword(password) if err != nil { return dbq.User{}, ErrWrongCredentials, "Malformed password" } - if subtle.ConstantTimeCompare([]byte(*user.SubsonicPassword), []byte(decoded)) != 1 { + if subtle.ConstantTimeCompare([]byte(*subsonicPassword), []byte(decoded)) != 1 { return dbq.User{}, ErrWrongCredentials, "Wrong username or password" } return user, 0, "" diff --git a/internal/tags/settings.go b/internal/tags/settings.go index 3914bb3d..c3f68b8b 100644 --- a/internal/tags/settings.go +++ b/internal/tags/settings.go @@ -13,6 +13,7 @@ import ( "github.com/jackc/pgx/v5/pgxpool" "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" + "git.fabledsword.com/bvandeusen/minstrel/internal/secret" ) // SettingsService reconciles the compiled-in provider registry with the @@ -101,7 +102,7 @@ func (s *SettingsService) reconcile(ctx context.Context) error { } key := "" if row.ApiKey != nil { - key = *row.ApiKey + key = secret.Reveal(secret.TagProviderAPIKey.Purpose(), row.ProviderID, *row.ApiKey) } s.enabledIDs[p.ID()] = row.Enabled s.apiKeys[p.ID()] = key @@ -181,11 +182,15 @@ func (s *SettingsService) UpdateProvider(ctx context.Context, providerID string, if patch.Enabled != nil { enabledVal = *patch.Enabled } + sealedKey, err := secret.SealPtr(secret.TagProviderAPIKey.Purpose(), providerID, patch.APIKey) + if err != nil { + return false, fmt.Errorf("seal provider api key: %w", err) + } if err := q.UpdateTagProviderSettings(ctx, dbq.UpdateTagProviderSettingsParams{ ProviderID: providerID, Enabled: enabledVal, Column3: patch.APIKey != nil, - ApiKey: patch.APIKey, + ApiKey: sealedKey, }); err != nil { return false, fmt.Errorf("update provider settings: %w", err) } @@ -200,7 +205,7 @@ func (s *SettingsService) UpdateProvider(ctx context.Context, providerID string, for _, r := range rows { key := "" if r.ApiKey != nil { - key = *r.ApiKey + key = secret.Reveal(secret.TagProviderAPIKey.Purpose(), r.ProviderID, *r.ApiKey) } s.enabledIDs[r.ProviderID] = r.Enabled s.apiKeys[r.ProviderID] = key