Keep server data in an encrypted database, and harden every part

Storage. The server keeps its data in SQLite (a file in the data
directory, the default) or PostgreSQL (database, KEEPORG_DATABASE_URL or
-database). Every row, the KDBX versions included, is sealed with
AES-256-GCM under a key derived from the master key and named by an HMAC.
The master key is required to start and a different one is refused. A data
directory from 0.10 or earlier is imported on first start. Several servers
may share one PostgreSQL database. Go goes to 1.26 for the pure-Go
drivers (modernc.org/sqlite, jackc/pgx).

Hardening, after an audit of the server, both clients and the deployment:
- Single sign-on approves a device only after an explicit Approve in the
  browser that started it, bound by a cookie and PKCE; identities are never
  linked by username, by verified email only when allowed, never to admins.
- Sign-in attempts are limited per address and per account, passwords are
  checked outside the store lock with uniform timing, one-time codes work
  once, and starting two-factor needs the password.
- Merge vaults bound KDF parameters, payload size and XML shape, check
  pushes before storing them, and run at most two merges at once. A version
  is written in the same transaction as its metadata.
- Tokens in URLs only on the event stream, which ends on revocation; CSP,
  frame-ancestors, no-store and HSTS; sign-in CSRF; 404 for databases
  without access; new trusted_proxies.
- Clients open only the server's approval page, keep case-variant and
  retired file names apart, never overwrite a save made during a pull,
  require https except locally, send the stream token in a header, bound
  their reads and escape notifications and terminal output.
- The .deb creates the master key, systemd is sandboxed further, nginx sets
  modern TLS and HSTS, and CI deploys from main only.

Server 0.12.0, desktop 0.6.0, Android 0.4.0 (versionCode 4).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

internal/client/admin.go

@@ -153,13 +153,15 @@ func (a *API) ChangePassword(ctx context.Context, current, next, totp string) er
 }
 
 // EnrollTOTP starts second-factor enrolment and returns the shared secret
-// and its otpauth URI.
-func (a *API) EnrollTOTP(ctx context.Context) (secret, uri string, err error) {
+// and its otpauth URI. It needs the account password, and the current code
+// when a second factor is already on and is being replaced.
+func (a *API) EnrollTOTP(ctx context.Context, password, code string) (secret, uri string, err error) {
 	var out struct {
 		Secret string `json:"secret"`
 		URI    string `json:"uri"`
 	}
-	err = a.do(ctx, http.MethodPost, "/api/v1/auth/totp/enroll", nil, &out)
+	err = a.do(ctx, http.MethodPost, "/api/v1/auth/totp/enroll",
+		map[string]string{"password": password, "totp": code}, &out)
 	return out.Secret, out.URI, err
 }
 

internal/client/api.go

@@ -16,6 +16,9 @@ import (
 	"time"
 )
 
+// maxPayload bounds one database download.
+const maxPayload = 256 << 20
+
 // API talks to one KeepOrg server as one user.
 type API struct {
 	BaseURL string
@@ -185,10 +188,15 @@ func (a *API) Pull(ctx context.Context, dbID string) ([]byte, int64, error) {
 	if resp.StatusCode != http.StatusOK {
 		return nil, 0, decodeError(resp)
 	}
-	data, err := io.ReadAll(resp.Body)
+	// The server's own limit is tens of megabytes; a server that sends more
+	// is not one to trust with memory.
+	data, err := io.ReadAll(io.LimitReader(resp.Body, maxPayload+1))
 	if err != nil {
 		return nil, 0, err
 	}
+	if len(data) > maxPayload {
+		return nil, 0, fmt.Errorf("the server sent more than %d MiB for one database", maxPayload>>20)
+	}
 	v, _ := strconv.ParseInt(strings.Trim(resp.Header.Get("ETag"), `"`), 10, 64)
 	return data, v, nil
 }
@@ -216,7 +224,7 @@ func (a *API) Push(ctx context.Context, dbID string, baseVersion int64, data []b
 		return nil, decodeError(resp)
 	}
 	var out PushResult
-	if err := json.NewDecoder(resp.Body).Decode(&out); err != nil {
+	if err := json.NewDecoder(io.LimitReader(resp.Body, 1<<20)).Decode(&out); err != nil {
 		return nil, err
 	}
 	return &out, nil
@@ -236,10 +244,10 @@ type Event struct {
 // The caller reconnects; losing events is safe because every notification
 // only ever triggers a full reconcile.
 func (a *API) Events(ctx context.Context, onEvent func(Event)) error {
-	// The token travels as a query parameter because an event stream has no
-	// place to put a header on reconnect in every client runtime.
-	u := a.BaseURL + "/api/v1/events?access_token=" + url.QueryEscape(a.Token)
-	req, err := http.NewRequestWithContext(ctx, http.MethodGet, u, nil)
+	// The token goes in the header like everywhere else. In the URL it
+	// would land in proxy logs, and in every error message Go makes about
+	// a failed request, which this client logs.
+	req, err := a.request(ctx, http.MethodGet, "/api/v1/events", nil)
 	if err != nil {
 		return err
 	}

internal/client/config.go

@@ -6,6 +6,7 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
+	"net"
 	"net/http"
 	"net/url"
 	"os"
@@ -43,6 +44,10 @@ type Config struct {
 	LogFile string `json:"log_file"`
 	// Notifications turns the tray's desktop notifications on or off.
 	Notifications bool `json:"notifications"`
+	// AllowInsecureHTTP permits an http:// server_url that is not this
+	// machine. Without TLS the password, the session token and every vault
+	// cross the network readable and alterable by anyone on the path.
+	AllowInsecureHTTP bool `json:"allow_insecure_http"`
 }
 
 // ErrNoConfig means the client has not been logged in yet.
@@ -134,6 +139,10 @@ func (c *Config) Validate() error {
 		if err != nil || (u.Scheme != "http" && u.Scheme != "https") || u.Host == "" {
 			return fmt.Errorf("server_url %q must be an http:// or https:// URL", c.ServerURL)
 		}
+		if u.Scheme == "http" && !isLoopback(u.Hostname()) && !c.AllowInsecureHTTP {
+			return fmt.Errorf("server_url %q is not encrypted; use https://, "+
+				"or set allow_insecure_http if you really mean it", c.ServerURL)
+		}
 	}
 	if c.CAFile != "" {
 		if _, err := os.Stat(c.CAFile); err != nil {
@@ -143,6 +152,31 @@ func (c *Config) Validate() error {
 	return nil
 }
 
+func isLoopback(host string) bool {
+	if host == "localhost" {
+		return true
+	}
+	ip := net.ParseIP(host)
+	return ip != nil && ip.IsLoopback()
+}
+
+// CheckVerificationURL makes sure a page the server asks the client to open
+// in a browser is on that server. A compromised server must not be able to
+// make the desktop open whatever it likes — a file: URL, a network share
+// that runs a program, another application's URL scheme.
+func CheckVerificationURL(serverURL, raw string) error {
+	srv, err := url.Parse(serverURL)
+	if err != nil {
+		return err
+	}
+	u, err := url.Parse(raw)
+	if err != nil || u.Scheme != srv.Scheme || !strings.EqualFold(u.Host, srv.Host) ||
+		u.User != nil || u.Path != strings.TrimRight(srv.Path, "/")+"/device" {
+		return fmt.Errorf("the server asked to open %q, which is not its own approval page", raw)
+	}
+	return nil
+}
+
 // API returns a client for the configured server, signed in with the stored
 // token when there is one.
 func (c *Config) API() (*API, error) {

internal/client/config_test.go

@@ -119,3 +119,45 @@ func TestLockDirAdmitsOneAgent(t *testing.T) {
 	}
 	again()
 }
+
+func TestCheckVerificationURLStaysOnTheServer(t *testing.T) {
+	ok := []string{"https://k.example.com/device?code=ABCD-EFGH"}
+	bad := []string{
+		"file:///Applications/Calculator.app",
+		`\\attacker\share\x.exe`,
+		"https://evil.example/device?code=X",
+		"http://k.example.com/device?code=X",
+		"https://user@k.example.com/device",
+		"https://k.example.com/somewhere-else",
+		"keeporg://device-approved",
+	}
+	for _, u := range ok {
+		if err := CheckVerificationURL("https://k.example.com", u); err != nil {
+			t.Errorf("%s refused: %v", u, err)
+		}
+	}
+	for _, u := range bad {
+		if err := CheckVerificationURL("https://k.example.com", u); err == nil {
+			t.Errorf("%s accepted", u)
+		}
+	}
+}
+
+func TestPlainHTTPIsRefusedExceptLocally(t *testing.T) {
+	c := DefaultConfig()
+	for url, wantErr := range map[string]bool{
+		"http://keeporg.example.com":  true,
+		"http://127.0.0.1:8765":       false,
+		"http://localhost:8765":       false,
+		"https://keeporg.example.com": false,
+	} {
+		c.ServerURL = url
+		if err := c.Validate(); (err != nil) != wantErr {
+			t.Errorf("%s: %v", url, err)
+		}
+	}
+	c.ServerURL, c.AllowInsecureHTTP = "http://keeporg.example.com", true
+	if err := c.Validate(); err != nil {
+		t.Errorf("allow_insecure_http ignored: %v", err)
+	}
+}

internal/client/mirror.go

@@ -30,6 +30,11 @@ type fileState struct {
 	Version        int64  `json:"version"`
 	SHA256         string `json:"sha256"`
 	ConflictMarker string `json:"conflict_marker,omitempty"`
+	// Retired marks a database this device no longer has access to. Its
+	// file stays where it is, and its name stays taken: a database created
+	// later under the same name must not adopt — and upload — a file that
+	// belonged to another.
+	Retired bool `json:"retired,omitempty"`
 }
 
 // Status is a human-readable account of one mirrored database.
@@ -192,12 +197,12 @@ func (m *Mirror) SyncAll(ctx context.Context) error {
 			}
 		}
 	}
-	// A database that is no longer assigned stops being tracked. Its file is
-	// left alone: deleting a user's local copy of a password database is not
-	// this program's decision.
-	for id := range m.st {
+	// A database that is no longer assigned stops being synchronized. Its
+	// file is left alone — deleting a user's local copy of a password
+	// database is not this program's decision — and its name stays retired.
+	for id, st := range m.st {
 		if !seen[id] {
-			delete(m.st, id)
+			st.Retired = true
 			delete(m.status, id)
 		}
 	}
@@ -263,7 +268,7 @@ func (m *Mirror) reconcile(ctx context.Context, db Database) error {
 		return nil
 
 	case actPull:
-		return m.pull(ctx, db, st, path)
+		return m.pull(ctx, db, st, path, localSHA)
 
 	case actPush, actPushForMerge:
 		return m.push(ctx, db, st, path, local, false)
@@ -276,7 +281,7 @@ func (m *Mirror) reconcile(ctx context.Context, db Database) error {
 		if err != nil {
 			return err
 		}
-		if err := m.pull(ctx, db, st, path); err != nil {
+		if err := m.pull(ctx, db, st, path, localSHA); err != nil {
 			return err
 		}
 		why := "the server copy was newer, your save was kept as " + filepath.Base(copyPath)
@@ -306,7 +311,15 @@ func (m *Mirror) reconcile(ctx context.Context, db Database) error {
 	return nil
 }
 
-func (m *Mirror) pull(ctx context.Context, db Database, st *fileState, path string) error {
+// errChangedDuringSync means the local file was saved while a pass was
+// deciding about it. Nothing is overwritten; the next pass sees the save.
+var errChangedDuringSync = errors.New("the file was saved during synchronization; trying again")
+
+// pull replaces the local file with the server copy. expectSHA is the digest
+// of the local file the decision was made on ("" for none): if the file is
+// different now, a KeePass application saved it in the meantime, and
+// replacing it would lose that save without a trace.
+func (m *Mirror) pull(ctx context.Context, db Database, st *fileState, path, expectSHA string) error {
 	data, version, err := m.api.Pull(ctx, db.ID)
 	if err != nil {
 		return err
@@ -314,6 +327,13 @@ func (m *Mirror) pull(ctx context.Context, db Database, st *fileState, path stri
 	if version == 0 || len(data) == 0 {
 		return nil // nothing stored yet
 	}
+	now, exists, err := readFileIfExists(path)
+	if err != nil {
+		return err
+	}
+	if (exists && digest(now) != expectSHA) || (!exists && expectSHA != "") {
+		return errChangedDuringSync
+	}
 	if err := writeFileAtomic(path, data); err != nil {
 		return err
 	}
@@ -332,7 +352,7 @@ func (m *Mirror) push(ctx context.Context, db Database, st *fileState, path stri
 			// The server folded our save into someone else's. What it holds
 			// now is neither copy, so take it.
 			m.log.Printf("%s: server merged version %d (%s)", db.Name, res.Version, res.MergeSummary)
-			if err := m.pull(ctx, db, st, path); err != nil {
+			if err := m.pull(ctx, db, st, path, digest(data)); err != nil {
 				return err
 			}
 			m.setStatus(db, "merged", res.MergeSummary)
@@ -341,7 +361,7 @@ func (m *Mirror) push(ctx context.Context, db Database, st *fileState, path stri
 		if res.SHA256 != digest(data) {
 			// Whatever the server stored differs from what we sent, so the
 			// local file is no longer what the vault holds.
-			return m.pull(ctx, db, st, path)
+			return m.pull(ctx, db, st, path, digest(data))
 		}
 		if !res.Unchanged {
 			m.log.Printf("%s: pushed version %d", db.Name, res.Version)
@@ -371,7 +391,7 @@ func (m *Mirror) push(ctx context.Context, db Database, st *fileState, path stri
 		if cerr != nil {
 			return cerr
 		}
-		if perr := m.pull(ctx, db, st, path); perr != nil {
+		if perr := m.pull(ctx, db, st, path, digest(data)); perr != nil {
 			return perr
 		}
 		why := "the server copy was newer, your save was kept as " + filepath.Base(copyPath)
@@ -386,7 +406,7 @@ func (m *Mirror) push(ctx context.Context, db Database, st *fileState, path stri
 			if cerr != nil {
 				return cerr
 			}
-			if perr := m.pull(ctx, db, st, path); perr != nil {
+			if perr := m.pull(ctx, db, st, path, digest(data)); perr != nil {
 				return perr
 			}
 			why := "you have read-only access, your save was kept as " + filepath.Base(copyPath)
@@ -406,6 +426,8 @@ func (m *Mirror) trackedState(db Database) *fileState {
 		m.st[db.ID] = st
 		return st
 	}
+	// Access came back: the file and what was last agreed are still valid.
+	st.Retired = false
 	if st.File == "" {
 		st.File = m.fileNameFor(db)
 	}
@@ -413,6 +435,13 @@ func (m *Mirror) trackedState(db Database) *fileState {
 		want := m.fileNameFor(db)
 		old := filepath.Join(m.dir, st.File)
 		if _, err := os.Stat(old); err == nil && want != st.File {
+			// Never onto a file that is already there: os.Rename would
+			// replace it, and it may be someone else's vault.
+			if _, err := os.Lstat(filepath.Join(m.dir, want)); err == nil {
+				m.log.Printf("%s: not renaming %s to %s, which already exists", db.Name, st.File, want)
+				st.Name = db.Name
+				return st
+			}
 			if err := os.Rename(old, filepath.Join(m.dir, want)); err != nil {
 				m.log.Printf("%s: rename %s to %s: %v", db.Name, st.File, want, err)
 				return st
@@ -425,7 +454,9 @@ func (m *Mirror) trackedState(db Database) *fileState {
 }
 
 // fileNameFor turns a database name into a filename, keeping it unique
-// within the mirror directory.
+// within the mirror directory. Names compare without regard to case, since
+// macOS and Windows file systems do: "work" and "Work" are one file there,
+// and two databases sharing it would upload each other.
 func (m *Mirror) fileNameFor(db Database) string {
 	base := sanitize(db.Name)
 	if base == "" {
@@ -433,7 +464,7 @@ func (m *Mirror) fileNameFor(db Database) string {
 	}
 	name := base + ".kdbx"
 	for id, st := range m.st {
-		if id != db.ID && st.File == name {
+		if id != db.ID && strings.EqualFold(st.File, name) {
 			return fmt.Sprintf("%s-%s.kdbx", base, shortID(db.ID))
 		}
 	}
@@ -521,7 +552,8 @@ func writeFileAtomic(path string, data []byte) error {
 	return os.Rename(tmp, path)
 }
 
-// sanitize keeps a database name usable as a filename on every platform.
+// sanitize keeps a database name usable as a filename on every platform,
+// and out of the names this client makes for itself.
 func sanitize(name string) string {
 	var b strings.Builder
 	for _, r := range strings.TrimSpace(name) {
@@ -533,7 +565,45 @@ func sanitize(name string) string {
 			b.WriteRune('-')
 		}
 	}
-	return strings.Trim(strings.TrimSuffix(b.String(), ".kdbx"), " .-")
+	out := b.String()
+	for strings.Contains(out, "..") {
+		out = strings.ReplaceAll(out, "..", ".")
+	}
+	out = strings.Trim(strings.TrimSuffix(out, ".kdbx"), " .-")
+	// A database called "Work.server-v7" or "Work.conflict-…" would collide
+	// with the side copies kept for "Work".
+	for _, reserved := range []string{".server-v", ".conflict-", ".keeporg"} {
+		out = undotFold(out, reserved)
+	}
+	// Windows maps these to devices, whatever the extension.
+	stem, _, _ := strings.Cut(out, ".")
+	if windowsDevice[strings.ToUpper(stem)] {
+		out += "-db"
+	}
+	return out
+}
+
+var windowsDevice = map[string]bool{
+	"CON": true, "PRN": true, "AUX": true, "NUL": true,
+	"COM1": true, "COM2": true, "COM3": true, "COM4": true, "COM5": true,
+	"COM6": true, "COM7": true, "COM8": true, "COM9": true,
+	"LPT1": true, "LPT2": true, "LPT3": true, "LPT4": true, "LPT5": true,
+	"LPT6": true, "LPT7": true, "LPT8": true, "LPT9": true,
+}
+
+// undotFold turns the leading dot of every case-insensitive occurrence of
+// marker into a dash, keeping the name's own spelling.
+func undotFold(s, marker string) string {
+	lower, low := []byte(strings.ToLower(s)), strings.ToLower(marker)
+	out := []byte(s)
+	for i := 0; ; {
+		j := strings.Index(string(lower[i:]), low)
+		if j < 0 {
+			return string(out)
+		}
+		out[i+j] = '-'
+		i += j + len(marker)
+	}
 }
 
 func shortID(id string) string {

internal/client/mirror_names_test.go

@@ -0,0 +1,90 @@
+package client
+
+import (
+	"context"
+	"io"
+	"log"
+	"os"
+	"path/filepath"
+	"testing"
+	"time"
+)
+
+func TestSanitizeKeepsClearOfTheClientsOwnNames(t *testing.T) {
+	for in, want := range map[string]string{
+		"Team Vault":       "Team Vault",
+		"../../etc/passwd": "etc-passwd",
+		"Work.server-v7":   "Work-server-v7",
+		"Work.CONFLICT-1":  "Work-CONFLICT-1",
+		"nul":              "nul-db",
+		"COM3.backup":      "COM3.backup-db",
+		".keeporg-state":   "keeporg-state",
+	} {
+		if got := sanitize(in); got != want {
+			t.Errorf("sanitize(%q) = %q, want %q", in, got, want)
+		}
+	}
+}
+
+func TestFileNamesDifferingOnlyInCaseDoNotShareAFile(t *testing.T) {
+	m := &Mirror{st: map[string]*fileState{"a": {Name: "Work", File: "Work.kdbx"}}}
+	if got := m.fileNameFor(Database{ID: "bbbbbbbb", Name: "work"}); got == "work.kdbx" {
+		t.Fatalf("a case variant got the same file on a case-insensitive disk: %s", got)
+	}
+}
+
+// The attack: a database the user lost access to leaves its file behind; an
+// administrator elsewhere creates an empty database of the same name and
+// grants it to the user. The leftover must not be adopted and uploaded.
+func TestARetiredFileIsNotAdoptedByANewDatabase(t *testing.T) {
+	s := newStack(t)
+	s.sync()
+	v, _ := s.remote()
+	if v == 0 {
+		s.write("Team Vault.kdbx", "the original vault")
+		s.sync()
+	}
+	ctx := context.Background()
+	if err := s.admin.DeleteDatabase(ctx, s.dbID, password, ""); err != nil {
+		t.Fatal(err)
+	}
+	s.sync()
+	impostor, err := s.admin.CreateDatabase(ctx, "Team Vault", "everyone", "")
+	if err != nil {
+		t.Fatal(err)
+	}
+	s.sync()
+	if _, v, err := s.admin.Pull(ctx, impostor.ID); err != nil || v != 0 {
+		t.Fatalf("the leftover file was uploaded into the new database: version %d, %v", v, err)
+	}
+	if got := s.read("Team Vault.kdbx"); got != "the original vault" {
+		t.Fatalf("the leftover file changed: %q", got)
+	}
+}
+
+func TestPullDoesNotOverwriteASaveMadeDuringSync(t *testing.T) {
+	dir := t.TempDir()
+	m := &Mirror{dir: dir, log: log.New(io.Discard, "", 0)}
+	path := filepath.Join(dir, "v.kdbx")
+	os.WriteFile(path, []byte("saved just now"), 0o600)
+	// The decision was made on the earlier content.
+	now, _, _ := readFileIfExists(path)
+	if digest(now) == digest([]byte("earlier")) {
+		t.Fatal("test setup")
+	}
+	s := newStack(t)
+	m.api = s.admin
+	if _, err := s.admin.Push(context.Background(), s.dbID, 0, []byte("server copy"), false); err != nil {
+		t.Fatal(err)
+	}
+	m.st, m.status = map[string]*fileState{}, map[string]*Status{}
+	st := &fileState{File: "v.kdbx"}
+	err := m.pull(context.Background(), Database{ID: s.dbID, Name: "v"}, st, path, digest([]byte("earlier")))
+	if err != errChangedDuringSync {
+		t.Fatalf("pull over a fresh save: %v", err)
+	}
+	if b, _ := os.ReadFile(path); string(b) != "saved just now" {
+		t.Fatalf("the save was overwritten: %q", b)
+	}
+	_ = time.Second
+}

internal/client/sse.go

@@ -3,10 +3,14 @@ package client
 import (
 	"bufio"
 	"encoding/json"
+	"errors"
 	"io"
 	"strings"
 )
 
+// maxEventBytes bounds one event's data.
+const maxEventBytes = 1 << 20
+
 // sseDecoder reads the subset of the Server-Sent Events format the server
 // emits: "event:" and "data:" lines, one JSON payload per event.
 type sseDecoder struct {
@@ -47,6 +51,11 @@ func (d *sseDecoder) next() (*Event, error) {
 			name = strings.TrimSpace(strings.TrimPrefix(line, "event:"))
 		case strings.HasPrefix(line, "data:"):
 			data += strings.TrimSpace(strings.TrimPrefix(line, "data:"))
+			// Events are a few hundred bytes; a stream that keeps adding
+			// to one is not a KeepOrg server.
+			if len(data) > maxEventBytes {
+				return nil, errors.New("event stream: an event exceeds 1 MiB")
+			}
 		}
 	}
 	if err := d.sc.Err(); err != nil {