diff --git a/.dockerignore b/.dockerignore index 280c7d0..3c378fc 100644 --- a/.dockerignore +++ b/.dockerignore @@ -5,6 +5,7 @@ web/out server/webui/dist /artex data +keys *.db transcripts bench/kv diff --git a/.gitignore b/.gitignore index 58dbb87..8cca111 100644 --- a/.gitignore +++ b/.gitignore @@ -47,6 +47,7 @@ Thumbs.db # JWT signing key (generated at runtime, stored in the project root, never committed) /jwt.key +/keys/ # local environment generated by Docker deployment (contains passwords/keys, not committed) .env diff --git a/CHANGELOG.md b/CHANGELOG.md index aa3782a..c9d6bec 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,15 @@ All notable changes to this project are documented in this file. The format is b ## [Unreleased] +### Fixed + +- **The JWT signing key survives container rebuilds**: the server wrote `jwt.key` to the executable's directory, `/app` in the image, so the key lived in the container's writable layer and recreating the container (for example `docker compose up -d --build`) generated a new key and signed every user out. The key directory is now set by `ARTEX_JWT_KEY_DIR` (default: the executable's directory, unchanged for non-Docker installs). `docker-compose.yml` sets it to `/app/keys` and bind-mounts `./keys` there, outside `./data`, which the file manager browses. On start, when `/jwt.key` is missing, a key at `data/jwt.key`, then at the executable's directory, is copied there with mode 0600 and the old file is removed; otherwise a new key is generated. An existing key is never overwritten. Upgrading a Docker install signs every user out once, because the old key is in the container being replaced; to keep sessions, copy it out before recreating the container: + + ```bash + mkdir -p keys + docker cp artex-artex-1:/app/jwt.key ./keys/jwt.key + ``` + ## [0.5.0] - 2026-11-04 Ollama support, a setup token for the first-run claim, secret scrubbing for child processes, a Docker image that builds from a fresh clone, Go 1.26.10 and Node 22, and race and shellcheck runs in CI. Entries marked "direct commit" landed on `main` without a pull request. @@ -43,7 +52,6 @@ Ollama support, a setup token for the first-run claim, secret scrubbing for chil ### Known issues -- **The JWT signing key is not in the Docker volume**: the server writes `jwt.key` to the directory of the executable (`config.BaseDir()`), which is `/app` in the image, while the Dockerfile and `docker-compose.yml` comments say it lives in `/app/data`. The key therefore sits in the container's writable layer: recreating the container (for example `docker compose up -d --build` after an update) generates a new key and signs every user out. A key found at `data/jwt.key` is moved to `/app/jwt.key` on start and removed from the volume. - **The secret scrub has limits**: `os.Unsetenv` changes only the Go runtime's copy, so a process running as the same user can still read `/proc//environ` of the server (and, in Docker, of PID 1, `start.sh`). Other variables that may carry credentials, such as `ARTEX_LLM_PROXY`, are not scrubbed. - **The LLM test and model-list endpoints still take `proxy` from the request**, so a saved key can travel through a request-supplied proxy when the base URL is plain HTTP. - **Postgres passwords with URL-special characters** (`@`, `/`, `#`, `?`) break the `ARTEX_PG_DSN` that `docker-compose.yml` builds by interpolation, and Compose interpolates `$` in `.env`. diff --git a/Dockerfile b/Dockerfile index 407f8a7..df949f7 100644 --- a/Dockerfile +++ b/Dockerfile @@ -123,10 +123,10 @@ COPY --from=build /out/artex /app/artex # to artex — docker stop only signals PID 1; without forwarding, artex never receives it, # cannot shut down gracefully, and is hard-killed by SIGKILL after 10 seconds. COPY start.sh /app/start.sh -RUN chmod +x /app/artex /app/start.sh +RUN chmod +x /app/artex /app/start.sh && mkdir -p /app/keys COPY skills/ /app/skills/ -# persistence point for data/ (SQLite + jwt.key) -VOLUME ["/app/data"] +# persistence points: data/ (SQLite, task files) and keys/ (jwt.key, via ARTEX_JWT_KEY_DIR) +VOLUME ["/app/data", "/app/keys"] EXPOSE 8787 8788 ENTRYPOINT ["/app/start.sh"] CMD ["-addr", ":8787", "-proxy", ":8788"] diff --git a/README.md b/README.md index b8fe7c1..f2d5a61 100644 --- a/README.md +++ b/README.md @@ -180,7 +180,7 @@ Daily cron example (03:20, operator crontab): ## Updating and upgrading -> Upgrading only swaps the program and never touches data: the Postgres volume `pgdata`, `./data` (jwt.key / SQLite, etc.), and `./skills` are all preserved. **No manual database migration is needed**: `artex` idempotently re-runs `schema.sql` on every start (with `ADD COLUMN` / `CREATE INDEX IF NOT EXISTS`), i.e. "restart equals migrate". It is still recommended to back up `./data` and the database before upgrading. +> Upgrading only swaps the program and never touches data: the Postgres volume `pgdata`, `./data` (SQLite, task files, etc.), `./keys` (jwt.key), and `./skills` are all preserved. **No manual database migration is needed**: `artex` idempotently re-runs `schema.sql` on every start (with `ADD COLUMN` / `CREATE INDEX IF NOT EXISTS`), i.e. "restart equals migrate". It is still recommended to back up `./data` and the database before upgrading. ### Upgrading from a clone made before 2026-10-23 diff --git a/cmd/artex/main.go b/cmd/artex/main.go index a09553e..028db15 100644 --- a/cmd/artex/main.go +++ b/cmd/artex/main.go @@ -13,6 +13,7 @@ import ( "os/signal" "path/filepath" "runtime" + "strings" "syscall" "time" @@ -97,7 +98,17 @@ func run() int { skillDir = abs } log.Printf("[config] skill directory: %s", skillDir) - srv := server.New(ctx, mgr, skillDir, *dataDir, config.BaseDir(), server.WithSecretScrub()) + // JWT signing key directory: ARTEX_JWT_KEY_DIR (the Docker image points it at + // the /app/keys volume) or the base dir. Never the browsable data dir. + keyDir := strings.TrimSpace(os.Getenv("ARTEX_JWT_KEY_DIR")) + if keyDir == "" { + keyDir = config.BaseDir() + } + if abs, err := filepath.Abs(keyDir); err == nil { + keyDir = abs + } + log.Printf("[config] JWT key directory: %s", keyDir) + srv := server.New(ctx, mgr, skillDir, *dataDir, keyDir, server.WithSecretScrub()) httpSrv := &http.Server{ Addr: *addr, Handler: srv.Handler(), diff --git a/docker-compose.yml b/docker-compose.yml index b041f28..682cb85 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -34,13 +34,17 @@ services: ARTEX_LLM_MODEL: ${ARTEX_LLM_MODEL:-} ARTEX_LLM_BASE_URL: ${ARTEX_LLM_BASE_URL:-} ARTEX_LLM_PROXY: ${ARTEX_LLM_PROXY:-} + ARTEX_JWT_KEY_DIR: /app/keys ports: - "${ARTEX_BIND_ADDR:-127.0.0.1}:8787:8787" - "127.0.0.1:8788:8788" volumes: - # Bind the data directory (jwt.key, SQLite, etc.) to data/ in the project root, + # Bind the data directory (SQLite, task files, etc.) to data/ in the project root, # so it is easy to inspect/back up on the host and survives container rebuilds. - ./data:/app/data + # JWT signing key (jwt.key), kept out of data/, which the file manager browses. + # Persisting it keeps users signed in across container rebuilds. + - ./keys:/app/keys # Mount skills/ from the project root into the container (overriding the image's built-in /app/skills); # skill changes require no image rebuild — the container reads the host copy. - ./skills:/app/skills diff --git a/scripts/benchmark/README.md b/scripts/benchmark/README.md index 6604318..a3b7fa1 100755 --- a/scripts/benchmark/README.md +++ b/scripts/benchmark/README.md @@ -58,7 +58,7 @@ Standing rules, enforced by the runner: cd scripts/benchmark # against a locally deployed ARTEX, with an independent judge endpoint -ARTEX_TOKEN= \ +ARTEX_TOKEN= \ JUDGE_BASE_URL=https://api.example.com JUDGE_MODEL= JUDGE_API_KEY=... \ python3 run_bench.py --corpus juice-shop --subject-model glm-5.3 ``` diff --git a/server/auth.go b/server/auth.go index a94971e..4242d22 100644 --- a/server/auth.go +++ b/server/auth.go @@ -16,6 +16,8 @@ import ( "github.com/golang-jwt/jwt/v5" "golang.org/x/crypto/bcrypt" + + "github.com/Autumn-27/artex/config" ) const ( @@ -47,28 +49,38 @@ func validatePassword(pw string) string { return "" } +// jwtLegacyBaseDir is the second legacy key location (the executable's directory, +// where releases before ARTEX_JWT_KEY_DIR wrote the key). A var so tests can point it +// at a temp dir. +var jwtLegacyBaseDir = config.BaseDir + // loadOrCreateJWTKey reads the 32-byte signing key from keyDir/jwt.key. keyDir is -// the project base dir (next to the executable), NOT the browsable workspace root -// (dataDir) — the signing key must never be listable/downloadable via the file -// manager. Legacy installs kept it at dataDir/jwt.key; if present there and not yet -// at the new location, it is migrated (key preserved, so sessions stay valid) and -// the old file removed so it disappears from the workspace. On first run a random -// key is generated and persisted. +// ARTEX_JWT_KEY_DIR (the /app/keys volume in Docker) or the project base dir, NOT +// the browsable workspace root (dataDir) — the signing key must never be +// listable/downloadable via the file manager. When keyDir/jwt.key is missing, a key +// at dataDir/jwt.key, then at BaseDir()/jwt.key, is migrated (key preserved, so +// sessions stay valid) and the old file removed. Otherwise a random key is +// generated. An existing key file is never overwritten. func loadOrCreateJWTKey(keyDir, dataDir string) ([]byte, error) { + if err := os.MkdirAll(keyDir, 0o700); err != nil { + return nil, fmt.Errorf("create jwt key dir: %w", err) + } path := filepath.Join(keyDir, jwtKeyFilename) - // one-time migration out of the old in-workspace location. - if legacy := filepath.Join(dataDir, jwtKeyFilename); legacy != path { - if _, err := os.Stat(path); os.IsNotExist(err) { - if data, rerr := os.ReadFile(legacy); rerr == nil { - if werr := os.WriteFile(path, data, 0o600); werr == nil { - _ = os.Remove(legacy) - log.Printf("[auth] JWT key migrated from %s to %s (moved out of the browsable workspace)", legacy, path) - } - } + if _, err := os.Stat(path); os.IsNotExist(err) { + if err := migrateJWTKey(path, dataDir); err != nil { + return nil, err } } - if data, err := os.ReadFile(path); err == nil && len(strings.TrimSpace(string(data))) >= 32 { - return []byte(strings.TrimSpace(string(data))), nil + data, err := os.ReadFile(path) + if err == nil { + key := strings.TrimSpace(string(data)) + if len(key) < 32 { + return nil, fmt.Errorf("jwt key %s is shorter than 32 bytes; fix or remove it (it is never overwritten)", path) + } + return []byte(key), nil + } + if !os.IsNotExist(err) { + return nil, fmt.Errorf("read jwt key: %w", err) } buf := make([]byte, 32) for i := range buf { @@ -78,13 +90,71 @@ func loadOrCreateJWTKey(keyDir, dataDir string) ([]byte, error) { } buf[i] = keyChars[n.Int64()] } - if err := os.WriteFile(path, buf, 0600); err != nil { + if err := writeNewJWTKey(path, buf); err != nil { return nil, fmt.Errorf("write jwt key: %w", err) } log.Printf("[auth] new JWT key written to %s", path) return buf, nil } +// migrateJWTKey copies the first legacy key found (dataDir, then BaseDir) to dst +// and removes the old file. Candidates equal to dst are skipped. +func migrateJWTKey(dst, dataDir string) error { + for _, src := range []string{ + filepath.Join(dataDir, jwtKeyFilename), + filepath.Join(jwtLegacyBaseDir(), jwtKeyFilename), + } { + if samePath(src, dst) { + continue + } + data, err := os.ReadFile(src) + if os.IsNotExist(err) { + continue + } + if err != nil { + log.Printf("[auth] cannot read legacy JWT key %s: %v", src, err) + continue + } + if err := writeNewJWTKey(dst, data); err != nil { + return fmt.Errorf("migrate jwt key from %s: %w", src, err) + } + if err := os.Remove(src); err != nil { + log.Printf("[auth] JWT key copied to %s but the old file %s could not be removed: %v", dst, src, err) + } + log.Printf("[auth] JWT key migrated from %s to %s", src, dst) + return nil + } + return nil +} + +// writeNewJWTKey creates path with mode 0600, failing if it already exists. +func writeNewJWTKey(path string, data []byte) error { + f, err := os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0o600) + if err != nil { + return err + } + if _, err := f.Write(data); err != nil { + f.Close() + _ = os.Remove(path) + return err + } + if err := f.Close(); err != nil { + _ = os.Remove(path) + return err + } + return nil +} + +func samePath(a, b string) bool { + if absA, err := filepath.Abs(a); err == nil { + a = absA + } + if absB, err := filepath.Abs(b); err == nil { + b = absB + } + return a == b +} + // signJWT issues a 7-day HS256 token for user ARTEX. func signJWT(key []byte) (string, error) { return jwt.NewWithClaims(jwt.SigningMethodHS256, jwt.RegisteredClaims{ diff --git a/server/auth_test.go b/server/auth_test.go index 752b5de..7067992 100644 --- a/server/auth_test.go +++ b/server/auth_test.go @@ -3,10 +3,188 @@ package server import ( "encoding/json" "net/http/httptest" + "os" + "path/filepath" + "runtime" "strings" "testing" ) +// jwtKeyDirs returns fresh key, data and legacy base dirs and points +// jwtLegacyBaseDir at the latter for the duration of the test. keyDir does not +// exist yet, so loadOrCreateJWTKey has to create it. +func jwtKeyDirs(t *testing.T) (keyDir, dataDir, baseDir string) { + t.Helper() + root := t.TempDir() + keyDir = filepath.Join(root, "keys") + dataDir = filepath.Join(root, "data") + baseDir = filepath.Join(root, "base") + for _, d := range []string{dataDir, baseDir} { + if err := os.MkdirAll(d, 0o755); err != nil { + t.Fatal(err) + } + } + old := jwtLegacyBaseDir + jwtLegacyBaseDir = func() string { return baseDir } + t.Cleanup(func() { jwtLegacyBaseDir = old }) + return keyDir, dataDir, baseDir +} + +// assertKeyFileMode checks for 0600. Windows has no Unix permission bits (Go +// reports 0666 or 0444), so the check is skipped there. +func assertKeyFileMode(t *testing.T, path string) { + t.Helper() + fi, err := os.Stat(path) + if err != nil { + t.Fatal(err) + } + if runtime.GOOS == "windows" { + t.Logf("mode check skipped on windows (got %v)", fi.Mode().Perm()) + return + } + if got := fi.Mode().Perm(); got != 0o600 { + t.Fatalf("%s mode=%v want 0600", path, got) + } +} + +func assertMissing(t *testing.T, path string) { + t.Helper() + if _, err := os.Stat(path); !os.IsNotExist(err) { + t.Fatalf("%s should not exist (stat err=%v)", path, err) + } +} + +func TestLoadOrCreateJWTKeyMigrates(t *testing.T) { + const legacyKey = "legacy-key-0123456789abcdefghijkl" + for _, src := range []string{"data", "base"} { + t.Run(src, func(t *testing.T) { + keyDir, dataDir, baseDir := jwtKeyDirs(t) + legacy := filepath.Join(dataDir, jwtKeyFilename) + if src == "base" { + legacy = filepath.Join(baseDir, jwtKeyFilename) + } + if err := os.WriteFile(legacy, []byte(legacyKey+"\n"), 0o644); err != nil { + t.Fatal(err) + } + + key, err := loadOrCreateJWTKey(keyDir, dataDir) + if err != nil { + t.Fatal(err) + } + if string(key) != legacyKey { + t.Fatalf("key=%q want the migrated %q", key, legacyKey) + } + path := filepath.Join(keyDir, jwtKeyFilename) + data, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + if string(data) != legacyKey+"\n" { + t.Fatalf("migrated file content=%q, want the legacy bytes unchanged", data) + } + assertKeyFileMode(t, path) + assertMissing(t, legacy) + }) + } +} + +func TestLoadOrCreateJWTKeyMigratesDataDirFirst(t *testing.T) { + keyDir, dataDir, baseDir := jwtKeyDirs(t) + dataKey := "data-key-0123456789abcdefghijklmn" + baseKey := "base-key-0123456789abcdefghijklmn" + if err := os.WriteFile(filepath.Join(dataDir, jwtKeyFilename), []byte(dataKey), 0o600); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(baseDir, jwtKeyFilename), []byte(baseKey), 0o600); err != nil { + t.Fatal(err) + } + key, err := loadOrCreateJWTKey(keyDir, dataDir) + if err != nil { + t.Fatal(err) + } + if string(key) != dataKey { + t.Fatalf("key=%q want the dataDir key", key) + } + assertMissing(t, filepath.Join(dataDir, jwtKeyFilename)) +} + +func TestLoadOrCreateJWTKeyGenerates(t *testing.T) { + keyDir, dataDir, _ := jwtKeyDirs(t) + key, err := loadOrCreateJWTKey(keyDir, dataDir) + if err != nil { + t.Fatal(err) + } + if len(key) != 32 { + t.Fatalf("len(key)=%d want 32", len(key)) + } + path := filepath.Join(keyDir, jwtKeyFilename) + data, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + if len(data) != 32 || string(data) != string(key) { + t.Fatalf("file holds %d bytes, want the 32 returned", len(data)) + } + assertKeyFileMode(t, path) + assertMissing(t, filepath.Join(dataDir, jwtKeyFilename)) + + again, err := loadOrCreateJWTKey(keyDir, dataDir) + if err != nil { + t.Fatal(err) + } + if string(again) != string(key) { + t.Fatal("second load returned a different key") + } +} + +func TestLoadOrCreateJWTKeyNeverOverwrites(t *testing.T) { + keyDir, dataDir, baseDir := jwtKeyDirs(t) + if err := os.MkdirAll(keyDir, 0o700); err != nil { + t.Fatal(err) + } + path := filepath.Join(keyDir, jwtKeyFilename) + current := "current-key-0123456789abcdefghijk" + if err := os.WriteFile(path, []byte(current), 0o600); err != nil { + t.Fatal(err) + } + // legacy keys must not replace the current one, and are left alone. + legacyData := filepath.Join(dataDir, jwtKeyFilename) + legacyBase := filepath.Join(baseDir, jwtKeyFilename) + for _, p := range []string{legacyData, legacyBase} { + if err := os.WriteFile(p, []byte("legacy-key-0123456789abcdefghijkl"), 0o600); err != nil { + t.Fatal(err) + } + } + key, err := loadOrCreateJWTKey(keyDir, dataDir) + if err != nil { + t.Fatal(err) + } + if string(key) != current { + t.Fatalf("key=%q want the existing %q", key, current) + } + for _, p := range []string{legacyData, legacyBase} { + if _, err := os.Stat(p); err != nil { + t.Fatalf("legacy key %s should be untouched: %v", p, err) + } + } + + // a short (invalid) key is an error, not a reason to replace it. + short := "too-short" + if err := os.WriteFile(path, []byte(short), 0o600); err != nil { + t.Fatal(err) + } + if _, err := loadOrCreateJWTKey(keyDir, dataDir); err == nil { + t.Fatal("want an error for a short key") + } + data, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + if string(data) != short { + t.Fatalf("short key was overwritten with %d bytes", len(data)) + } +} + // GetSetting distinguishes "key absent" from "read failed" only through its error: // both return an empty value. If a password handler mistakes the error for "no // password set yet", a database blip opens the init door — authInit would let an diff --git a/server/server.go b/server/server.go index eddea90..c74ecba 100644 --- a/server/server.go +++ b/server/server.go @@ -44,7 +44,7 @@ type Server struct { ctx context.Context skillDir string // root directory for skill subdirectories - jwtKey []byte // HS256 signing key loaded from / generated into dataDir/jwt.key + jwtKey []byte // HS256 signing key loaded from / generated into keyDir/jwt.key scrubSecrets bool // drop ARTEX_PG_DSN and the API keys from the environment once read (binary only)