1
0
Эх сурвалжийг харах

Fix R01-N01: hash-only LOCAL_ADMIN_PASSWORD_HASH (no plaintext fallback)

The local-admin path previously read LOCAL_ADMIN_PASSWORD verbatim from
.env and compared it with hash_equals(). Anyone with read access to .env
(leaked image layer, stray backup, container exec) had immediate admin.
The OIDC path was the strong link; this was the weak one.

Switches LocalAdmin to verify with PHP's password_verify() against a
bcrypt hash stored in LOCAL_ADMIN_PASSWORD_HASH. The plaintext password
never lands on disk again. Per the user's explicit choice this is a
clean break — there is NO LOCAL_ADMIN_PASSWORD fallback. Operators with
existing plaintext .env files must regenerate as a hash on the next
deploy; LocalAdmin::isEnabled() returns false until they do, so
/auth/local 404s instead of silently authenticating with stale config.

Surface changes:
- src/Auth/LocalAdmin.php — passwordHash() reads
  LOCAL_ADMIN_PASSWORD_HASH; verify() uses password_verify(); isEnabled()
  requires email + hash; email comparison stays trim()ed and
  hash_equals().
- .env.example — LOCAL_ADMIN_PASSWORD → LOCAL_ADMIN_PASSWORD_HASH; comment
  block now shows the docker run --rm php:8.3-cli generation one-liner.
- README.md Quick setup — step 3 expanded with a host-PHP and
  no-host-PHP password_hash() recipe, plus the single-quotes-in-.env
  shell-expansion warning (the `$` segments in `$2y$...` get eaten by
  bash on `docker compose up` otherwise).
- SPEC §8 + §11 — env block + quick-start updated to the new var name.
- doc/admin-manual.md §3.5 — full rewrite: lifts the hash recipe up,
  adds the rotate-by-regenerating note, explains why the salt makes
  every invocation produce a different `$2y$...` string. §3.1 + §6
  troubleshooting cross-references updated.
- ACCEPTANCE.md — setup preamble references the hash + a pointer to
  the README recipe.

New tests/Auth/LocalAdminTest.php (9 cases) locks the contract in:
- isEnabled() requires both env vars
- verify() accepts correct password against a real password_hash() output
- verify() rejects wrong password / wrong email
- verify() with only the LEGACY LOCAL_ADMIN_PASSWORD set returns false
  (regression guard against "operator forgot to migrate")
- verify() returns false when disabled regardless of input
- email input is trim()ed before comparison
- displayName() default + override

Suite: 159 / 443 (was 150 / 430).

Closes R01-N01 from doc/REVIEW_01.md.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
chiappa 2 өдөр өмнө
parent
commit
857df15c93

+ 10 - 4
.env.example

@@ -20,11 +20,17 @@ APP_ENV=production
 
 # ---------------------------------------------------------------------------
 # Local admin (optional) — lets you sign in without Entra, e.g. during initial
-# setup or for a fully on-prem deployment. Set BOTH email and password to
-# enable; leave blank to disable. The password is compared in plain text
-# against this env value — so .env must be readable only by the app user.
+# setup or for a fully on-prem deployment. Set BOTH email and the password
+# hash to enable; leave blank to disable. The password is verified with PHP's
+# password_verify() against LOCAL_ADMIN_PASSWORD_HASH, so .env never contains
+# the password itself. Generate the hash with:
+#     docker run --rm php:8.3-cli php -r \
+#       'echo password_hash(readline("Password: "), PASSWORD_DEFAULT), PHP_EOL;'
+# (Or `php -r '...'` directly if you have PHP 8 on the host.) Paste the
+# resulting `$2y$...` string verbatim. Single quotes recommended in .env so
+# the `$` in the hash isn't interpreted by the shell.
 # The resulting user is stored with entra_oid = "local:<email>" and is_admin=1.
 # ---------------------------------------------------------------------------
 LOCAL_ADMIN_EMAIL=
-LOCAL_ADMIN_PASSWORD=
+LOCAL_ADMIN_PASSWORD_HASH=
 LOCAL_ADMIN_NAME=Local Admin

+ 3 - 2
ACCEPTANCE.md

@@ -12,8 +12,9 @@ docker compose up --build
 ```
 
 `.env` should have EITHER valid `ENTRA_*` values OR `LOCAL_ADMIN_EMAIL` +
-`LOCAL_ADMIN_PASSWORD` set. Below, "sign in" means whichever flow you
-configured.
+`LOCAL_ADMIN_PASSWORD_HASH` set (the hash is the output of PHP's
+`password_hash()` — see README's Quick setup). Below, "sign in" means
+whichever flow you configured.
 
 ## Steps
 

+ 19 - 3
README.md

@@ -28,9 +28,25 @@ cd sprint_planer_web
 # 2. Create the .env file from the template
 cp .env.example .env
 
-# 3. Edit .env. The minimum to sign in without Entra is:
-#      LOCAL_ADMIN_EMAIL=you@example.com
-#      LOCAL_ADMIN_PASSWORD=<a long passphrase>
+# 3. Generate a hash for the local-admin password. The app verifies sign-ins
+#    with password_verify() against LOCAL_ADMIN_PASSWORD_HASH, so the
+#    plaintext password never lands on disk. Pick the snippet that matches
+#    what you have installed:
+#
+#    a) Host PHP 8 (any minor):
+#       php -r 'echo password_hash(readline("Password: "), PASSWORD_DEFAULT), PHP_EOL;'
+#
+#    b) No host PHP — use the Docker image you already build with:
+#       docker run --rm -it php:8.3-cli php -r \
+#         'echo password_hash(readline("Password: "), PASSWORD_DEFAULT), PHP_EOL;'
+#
+#    Both prompt for the password (no echo on most TTYs) and print a
+#    bcrypt string starting with `$2y$`. Paste it into .env as:
+#       LOCAL_ADMIN_EMAIL='you@example.com'
+#       LOCAL_ADMIN_PASSWORD_HASH='$2y$...'
+#    Use single quotes — the `$` in the hash will otherwise be eaten by the
+#    shell on `docker compose up`.
+#
 #    For Entra-based sign-in, fill ENTRA_TENANT_ID / ENTRA_CLIENT_ID /
 #    ENTRA_CLIENT_SECRET instead (or in addition).
 chmod 600 .env

+ 8 - 4
SPEC.md

@@ -268,10 +268,13 @@ SESSION_PATH=/var/www/data/sessions
 APP_ENV=production
 
 # Optional local admin fallback (disables when blank).
-# Password is compared verbatim (not hashed) — .env must be file-permissions
-# protected. The resulting user is entra_oid="local:<email>", is_admin=1.
+# Password is verified with PHP's password_verify() against the bcrypt hash
+# stored in LOCAL_ADMIN_PASSWORD_HASH; the plaintext password never lands on
+# disk. Generate the hash via `password_hash($pw, PASSWORD_DEFAULT)` (see
+# README's Quick setup). The resulting user row has
+# entra_oid="local:<email>", is_admin=1.
 LOCAL_ADMIN_EMAIL=
-LOCAL_ADMIN_PASSWORD=
+LOCAL_ADMIN_PASSWORD_HASH=
 LOCAL_ADMIN_NAME=Local Admin
 ```
 
@@ -1117,7 +1120,8 @@ Nothing scheduled.
 
 ```bash
 cp .env.example .env
-# Fill Entra vars, OR set LOCAL_ADMIN_EMAIL + LOCAL_ADMIN_PASSWORD
+# Fill Entra vars, OR set LOCAL_ADMIN_EMAIL + LOCAL_ADMIN_PASSWORD_HASH
+# (see README's Quick setup for the password_hash() one-liner)
 docker compose up --build
 # open http://localhost:8080
 ```

+ 36 - 7
doc/admin-manual.md

@@ -72,7 +72,7 @@ In the Entra app registration, configure:
 Leave Entra fields blank if you only intend to use the local-admin
 fallback. The OIDC sign-in button still appears, but clicking it will fail
 until the variables are populated — keep `LOCAL_ADMIN_EMAIL` /
-`LOCAL_ADMIN_PASSWORD` filled if you skip Entra entirely.
+`LOCAL_ADMIN_PASSWORD_HASH` filled if you skip Entra entirely.
 
 ### 3.2 Application URL
 
@@ -111,17 +111,46 @@ turns them on — useful when troubleshooting in a non-public install.
 
 ```
 LOCAL_ADMIN_EMAIL=admin@example.com
-LOCAL_ADMIN_PASSWORD=<a long passphrase>
+LOCAL_ADMIN_PASSWORD_HASH='$2y$10$...'      # output of password_hash()
 LOCAL_ADMIN_NAME=Local Admin
 ```
 
-If both `LOCAL_ADMIN_EMAIL` and `LOCAL_ADMIN_PASSWORD` are set, a second
-sign-in form appears at `/auth/local`. The password is **compared in
-plain text** against the env value — so:
+If both `LOCAL_ADMIN_EMAIL` and `LOCAL_ADMIN_PASSWORD_HASH` are set, a
+second sign-in form appears at `/auth/local`. The submitted password is
+verified with PHP's `password_verify()` against the stored bcrypt hash —
+the plaintext password is never written to disk.
+
+**Generating the hash.** Pick whichever one-liner matches what you have
+installed; both prompt interactively so the password isn't kept in shell
+history:
+
+```bash
+# Host PHP 8 (any minor):
+php -r 'echo password_hash(readline("Password: "), PASSWORD_DEFAULT), PHP_EOL;'
+
+# No host PHP — borrow the runtime image:
+docker run --rm -it php:8.3-cli php -r \
+  'echo password_hash(readline("Password: "), PASSWORD_DEFAULT), PHP_EOL;'
+```
+
+Each invocation prints a different `$2y$10$...` string for the same
+input — that's the per-hash random salt. Either one verifies.
+
+Paste the result into `.env` between **single quotes** so the shell that
+launches `docker compose` doesn't try to expand the `$` segments inside
+the hash:
+
+```
+LOCAL_ADMIN_PASSWORD_HASH='$2y$10$abcdefghijklmnopqrstuvwxyz0123456789abcdefghijklmnop'
+```
+
+Then:
 
 - Pick a long, unique passphrase you do not reuse anywhere else.
 - Make sure `.env` is `chmod 600` and owned by a single trusted user.
-- Rotate the passphrase by editing `.env` and restarting the container.
+- Rotate the passphrase by regenerating the hash, replacing the line in
+  `.env`, and restarting the container — active sessions stay valid
+  until they expire (the hash is only consulted at sign-in time).
 
 The local-admin user is recorded in the database as
 `entra_oid = "local:<email>"` with `is_admin = 1`. If the `users` table
@@ -304,7 +333,7 @@ before pulling.
 | Symptom | Likely cause | Fix |
 |---|---|---|
 | `/auth/login` returns "redirect URI mismatch" | The redirect URI registered in Entra does not equal `{APP_BASE_URL}/auth/callback`. | Update either `APP_BASE_URL` in `.env` or the Entra app registration. |
-| `/auth/local` returns 404 | `LOCAL_ADMIN_EMAIL` or `LOCAL_ADMIN_PASSWORD` is blank. | Set both, then `docker compose restart`. |
+| `/auth/local` returns 404 | `LOCAL_ADMIN_EMAIL` or `LOCAL_ADMIN_PASSWORD_HASH` is blank. | Set both (see §3.5 for the hash recipe), then `docker compose restart`. |
 | Sign-in succeeds but every page shows "not authorised" | The user has no `is_admin` flag and is trying to access an admin-only page. | Promote them via Users (logged in as another admin). |
 | Container restarts in a loop | Most often a malformed `.env` line or a permission problem on `./data`. | `docker compose logs` will show the PHP fatal. Check that `./data` is writable by the `www-data` user inside the container (uid 33 on the Apache image). |
 | CSS looks broken / unstyled | The Node build stage was skipped or used a stale layer. | `docker compose build --no-cache` and restart. |

+ 13 - 11
src/Auth/LocalAdmin.php

@@ -10,15 +10,17 @@ namespace App\Auth;
  *
  * Enabled iff BOTH env vars are set:
  *     LOCAL_ADMIN_EMAIL
- *     LOCAL_ADMIN_PASSWORD
+ *     LOCAL_ADMIN_PASSWORD_HASH    (PHP password_hash() output, e.g. bcrypt)
  *
  * Optional:
- *     LOCAL_ADMIN_NAME       display name (default "Local Admin")
+ *     LOCAL_ADMIN_NAME             display name (default "Local Admin")
  *
- * Password is compared verbatim against the env value using a timing-safe
- * comparison. The corresponding user row is stored with
- *     entra_oid    = "local:<email>"
- *     is_admin     = 1
+ * The hash is generated once at install time with PHP's `password_hash()`
+ * (see README's Quick setup) and verified at sign-in with `password_verify()`.
+ * Storing only the hash means a leaked `.env` no longer hands an attacker a
+ * usable password directly. The corresponding user row is stored with
+ *     entra_oid = "local:<email>"
+ *     is_admin  = 1
  * so it will not collide with a real Entra user.
  */
 final class LocalAdmin
@@ -27,7 +29,7 @@ final class LocalAdmin
 
     public static function isEnabled(): bool
     {
-        return self::email() !== '' && self::password() !== '';
+        return self::email() !== '' && self::passwordHash() !== '';
     }
 
     public static function email(): string
@@ -55,13 +57,13 @@ final class LocalAdmin
             return false;
         }
         $emailMatch = hash_equals(self::email(), trim($email));
-        $pwMatch    = hash_equals(self::password(), $password);
+        $pwMatch    = password_verify($password, self::passwordHash());
         return $emailMatch && $pwMatch;
     }
 
-    private static function password(): string
+    private static function passwordHash(): string
     {
-        $v = getenv('LOCAL_ADMIN_PASSWORD');
-        return is_string($v) ? $v : '';
+        $v = getenv('LOCAL_ADMIN_PASSWORD_HASH');
+        return is_string($v) ? trim($v) : '';
     }
 }

+ 126 - 0
tests/Auth/LocalAdminTest.php

@@ -0,0 +1,126 @@
+<?php
+
+declare(strict_types=1);
+
+namespace App\Tests\Auth;
+
+use App\Auth\LocalAdmin;
+use App\Tests\TestCase;
+
+/**
+ * Lock-in tests for the hash-only LocalAdmin path (R01-N01). The fallback
+ * accepts ONLY a password_hash()-produced string in LOCAL_ADMIN_PASSWORD_HASH;
+ * the legacy plaintext LOCAL_ADMIN_PASSWORD env var is no longer consulted.
+ */
+final class LocalAdminTest extends TestCase
+{
+    /** @var array<string, string|false> */
+    private array $envBackup = [];
+
+    /** @var string[] */
+    private array $envKeys = [
+        'LOCAL_ADMIN_EMAIL',
+        'LOCAL_ADMIN_PASSWORD_HASH',
+        'LOCAL_ADMIN_PASSWORD',
+        'LOCAL_ADMIN_NAME',
+    ];
+
+    protected function setUp(): void
+    {
+        parent::setUp();
+        foreach ($this->envKeys as $k) {
+            $this->envBackup[$k] = getenv($k);
+            putenv($k);  // unset
+        }
+    }
+
+    protected function tearDown(): void
+    {
+        foreach ($this->envKeys as $k) {
+            $prev = $this->envBackup[$k] ?? false;
+            if ($prev === false) {
+                putenv($k);
+            } else {
+                putenv("{$k}={$prev}");
+            }
+        }
+        parent::tearDown();
+    }
+
+    public function testDisabledWhenEmailOrHashMissing(): void
+    {
+        self::assertFalse(LocalAdmin::isEnabled(), 'no env at all');
+
+        putenv('LOCAL_ADMIN_EMAIL=admin@example.com');
+        self::assertFalse(LocalAdmin::isEnabled(), 'email only');
+
+        putenv('LOCAL_ADMIN_EMAIL');
+        putenv('LOCAL_ADMIN_PASSWORD_HASH=' . password_hash('hunter2', PASSWORD_DEFAULT));
+        self::assertFalse(LocalAdmin::isEnabled(), 'hash only');
+    }
+
+    public function testEnabledWhenBothSet(): void
+    {
+        putenv('LOCAL_ADMIN_EMAIL=admin@example.com');
+        putenv('LOCAL_ADMIN_PASSWORD_HASH=' . password_hash('hunter2', PASSWORD_DEFAULT));
+        self::assertTrue(LocalAdmin::isEnabled());
+    }
+
+    public function testVerifyAcceptsCorrectPasswordAgainstStoredHash(): void
+    {
+        $hash = password_hash('correct horse battery staple', PASSWORD_DEFAULT);
+        putenv('LOCAL_ADMIN_EMAIL=admin@example.com');
+        putenv('LOCAL_ADMIN_PASSWORD_HASH=' . $hash);
+
+        self::assertTrue(LocalAdmin::verify('admin@example.com', 'correct horse battery staple'));
+    }
+
+    public function testVerifyRejectsWrongPassword(): void
+    {
+        putenv('LOCAL_ADMIN_EMAIL=admin@example.com');
+        putenv('LOCAL_ADMIN_PASSWORD_HASH=' . password_hash('hunter2', PASSWORD_DEFAULT));
+
+        self::assertFalse(LocalAdmin::verify('admin@example.com', 'hunter1'));
+    }
+
+    public function testVerifyRejectsWrongEmail(): void
+    {
+        putenv('LOCAL_ADMIN_EMAIL=admin@example.com');
+        putenv('LOCAL_ADMIN_PASSWORD_HASH=' . password_hash('hunter2', PASSWORD_DEFAULT));
+
+        self::assertFalse(LocalAdmin::verify('attacker@example.com', 'hunter2'));
+    }
+
+    public function testVerifyRejectsLegacyPlaintextEnvVar(): void
+    {
+        // Prior versions read LOCAL_ADMIN_PASSWORD verbatim. After R01-N01 the
+        // hash-only contract means a plaintext env var alone must NOT enable
+        // the fallback or authenticate anyone. (Belt-and-braces: if an
+        // operator forgot to migrate, /auth/local should 404, not silently
+        // accept the old value.)
+        putenv('LOCAL_ADMIN_EMAIL=admin@example.com');
+        putenv('LOCAL_ADMIN_PASSWORD=hunter2');
+        self::assertFalse(LocalAdmin::isEnabled());
+        self::assertFalse(LocalAdmin::verify('admin@example.com', 'hunter2'));
+    }
+
+    public function testVerifyReturnsFalseWhenDisabledRegardlessOfInput(): void
+    {
+        self::assertFalse(LocalAdmin::verify('admin@example.com', 'anything'));
+    }
+
+    public function testEmailIsTrimmedOnComparison(): void
+    {
+        putenv('LOCAL_ADMIN_EMAIL=admin@example.com');
+        putenv('LOCAL_ADMIN_PASSWORD_HASH=' . password_hash('pw', PASSWORD_DEFAULT));
+        self::assertTrue(LocalAdmin::verify('  admin@example.com  ', 'pw'));
+    }
+
+    public function testDisplayNameDefaultsAndOverride(): void
+    {
+        self::assertSame('Local Admin', LocalAdmin::displayName());
+
+        putenv('LOCAL_ADMIN_NAME=Site Operator');
+        self::assertSame('Site Operator', LocalAdmin::displayName());
+    }
+}