TASK-031: Enforce bcrypt 72-byte password limit
Bcrypt silently truncates passwords at 72 bytes, so passwords longer than 72 characters provide no additional security. This commit enforces the 72-byte maximum across the authentication and setup flows. Changes: - Add max_length=72 to LoginRequest.password and SetupRequest.master_password - Update field validator in SetupRequest to explicitly check max_length - Add comprehensive tests for password length validation (6 new test cases) - Document the 72-byte limitation in Features.md (master password options) - Add new section 12 'Password Hashing' in Backend-Development.md explaining: - The bcrypt truncation behavior - Why the limit is enforced - The validation flow from frontend to backend - What happens when passwords exceed the limit All existing tests pass, no regressions introduced. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This commit is contained in:
@@ -1085,7 +1085,39 @@ The login endpoint (`POST /api/auth/login`) is protected against brute-force att
|
|||||||
|
|
||||||
---
|
---
|
||||||
|
|
||||||
## 13. File I/O Conventions
|
## 12. Password Hashing
|
||||||
|
|
||||||
|
The master password is hashed using **bcrypt** with an auto-generated salt. All password validation uses the models in `app.models.auth` and `app.models.setup`.
|
||||||
|
|
||||||
|
### The 72-Byte Bcrypt Limitation
|
||||||
|
|
||||||
|
**Important:** bcrypt silently truncates all input at **72 bytes** before hashing. This means:
|
||||||
|
- A user who sets a 100-character password is actually authenticated by only the first 72 bytes
|
||||||
|
- Extra characters beyond 72 bytes provide **zero additional security**
|
||||||
|
- An attacker who has reduced their search space to 72 bytes can brute-force the password more efficiently than intended
|
||||||
|
|
||||||
|
**Solution:** Both password fields enforce a **maximum length of 72 bytes**:
|
||||||
|
- `LoginRequest.password` — max 72 characters (enforced via Pydantic `Field(max_length=72)`)
|
||||||
|
- `SetupRequest.master_password` — max 72 characters (enforced via Pydantic `Field(max_length=72)`)
|
||||||
|
|
||||||
|
**Validation flow:**
|
||||||
|
1. Frontend → hashes password with SHA256 using `SubtleCrypto` before transmission
|
||||||
|
2. Backend receives SHA256 hash, validates length (≤ 72 bytes)
|
||||||
|
3. Backend → hashes with bcrypt using `run_blocking(bcrypt.hashpw)` to avoid event loop stall
|
||||||
|
4. Hash stored in SQLite `settings` table
|
||||||
|
|
||||||
|
**If a password exceeds 72 bytes:**
|
||||||
|
- Pydantic raises `ValidationError` with error code `string_too_long`
|
||||||
|
- The router returns **HTTP 422 Unprocessable Entity**
|
||||||
|
- The frontend should inform the user to choose a shorter password
|
||||||
|
|
||||||
|
**Implementation:**
|
||||||
|
- Models: `app.models.auth.LoginRequest`, `app.models.setup.SetupRequest`
|
||||||
|
- Service layer: `app.services.auth_service._check_password()`, `app.services.setup_service.run_setup()`
|
||||||
|
|
||||||
|
---
|
||||||
|
|
||||||
|
## 14. File I/O Conventions
|
||||||
|
|
||||||
All file write operations to critical configuration files must be **atomic** to prevent corruption if the process is killed mid-write.
|
All file write operations to critical configuration files must be **atomic** to prevent corruption if the process is killed mid-write.
|
||||||
|
|
||||||
@@ -1163,7 +1195,7 @@ atomic_write(path, updated_content) # Atomic write, auto-cleanup on error
|
|||||||
|
|
||||||
---
|
---
|
||||||
|
|
||||||
## 14. Git & Workflow
|
## 15. Git & Workflow
|
||||||
|
|
||||||
- **Branch naming:** `feature/<short-description>`, `fix/<short-description>`, `chore/<short-description>`.
|
- **Branch naming:** `feature/<short-description>`, `fix/<short-description>`, `chore/<short-description>`.
|
||||||
- **Commit messages:** imperative tense, max 72 chars first line (`Add jail reload endpoint`, `Fix ban history query`).
|
- **Commit messages:** imperative tense, max 72 chars first line (`Add jail reload endpoint`, `Fix ban history query`).
|
||||||
@@ -1173,7 +1205,7 @@ atomic_write(path, updated_content) # Atomic write, auto-cleanup on error
|
|||||||
|
|
||||||
---
|
---
|
||||||
|
|
||||||
## 15. Coding Principles
|
## 16. Coding Principles
|
||||||
|
|
||||||
These principles are **non-negotiable**. Every backend contributor must internalise and apply them daily.
|
These principles are **non-negotiable**. Every backend contributor must internalise and apply them daily.
|
||||||
|
|
||||||
@@ -1560,7 +1592,7 @@ When user-supplied URLs are fetched by the backend, validate them before making
|
|||||||
|
|
||||||
---
|
---
|
||||||
|
|
||||||
## 16. Quick Reference — Do / Don't
|
## 17. Quick Reference — Do / Don't
|
||||||
|
|
||||||
| Do | Don't |
|
| Do | Don't |
|
||||||
|---|---|
|
|---|---|
|
||||||
|
|||||||
@@ -14,7 +14,7 @@ A web application to monitor, manage, and configure fail2ban from a clean, acces
|
|||||||
|
|
||||||
### Options
|
### Options
|
||||||
|
|
||||||
- **Master Password** — Set a single global password that protects the entire web interface.
|
- **Master Password** — Set a single global password that protects the entire web interface. Must be between 8 and 72 characters long (72-byte limit is due to bcrypt truncation) and include one uppercase letter, one number, and one special character from `!@#$%^&*()`.
|
||||||
- **Database Path** — Define where the application stores its own SQLite database.
|
- **Database Path** — Define where the application stores its own SQLite database.
|
||||||
- **fail2ban Connection** — Specify how the application connects to the running fail2ban instance (socket path or related settings).
|
- **fail2ban Connection** — Specify how the application connects to the running fail2ban instance (socket path or related settings).
|
||||||
- **General Preferences** — Any additional application-level settings such as default time zone, date format, or session duration.
|
- **General Preferences** — Any additional application-level settings such as default time zone, date format, or session duration.
|
||||||
|
|||||||
@@ -11,7 +11,11 @@ class LoginRequest(BaseModel):
|
|||||||
|
|
||||||
model_config = ConfigDict(strict=True)
|
model_config = ConfigDict(strict=True)
|
||||||
|
|
||||||
password: str = Field(..., description="Master password to authenticate with.")
|
password: str = Field(
|
||||||
|
...,
|
||||||
|
max_length=72,
|
||||||
|
description="Master password to authenticate with (max 72 bytes due to bcrypt truncation).",
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
class LoginResponse(BaseModel):
|
class LoginResponse(BaseModel):
|
||||||
|
|||||||
@@ -14,7 +14,8 @@ class SetupRequest(BaseModel):
|
|||||||
master_password: str = Field(
|
master_password: str = Field(
|
||||||
...,
|
...,
|
||||||
min_length=8,
|
min_length=8,
|
||||||
description="Master password that protects the BanGUI interface.",
|
max_length=72,
|
||||||
|
description="Master password that protects the BanGUI interface (max 72 bytes due to bcrypt truncation).",
|
||||||
)
|
)
|
||||||
|
|
||||||
@field_validator("master_password")
|
@field_validator("master_password")
|
||||||
@@ -22,6 +23,8 @@ class SetupRequest(BaseModel):
|
|||||||
def validate_master_password(cls, value: str) -> str:
|
def validate_master_password(cls, value: str) -> str:
|
||||||
if len(value) < 8:
|
if len(value) < 8:
|
||||||
raise ValueError("Password must be at least 8 characters long.")
|
raise ValueError("Password must be at least 8 characters long.")
|
||||||
|
if len(value) > 72:
|
||||||
|
raise ValueError("Password must not exceed 72 bytes (bcrypt limitation).")
|
||||||
if not any(char.isupper() for char in value):
|
if not any(char.isupper() for char in value):
|
||||||
raise ValueError("Password must include at least one uppercase letter.")
|
raise ValueError("Password must include at least one uppercase letter.")
|
||||||
if not any(char.isdigit() for char in value):
|
if not any(char.isdigit() for char in value):
|
||||||
|
|||||||
@@ -289,3 +289,78 @@ def test_global_config_response_log_target_invalid_path(_mock_allowed_dirs: None
|
|||||||
)
|
)
|
||||||
error_msg = str(exc_info.value)
|
error_msg = str(exc_info.value)
|
||||||
assert "outside allowed directories" in error_msg
|
assert "outside allowed directories" in error_msg
|
||||||
|
|
||||||
|
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
|
# LoginRequest and SetupRequest password validation
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
|
|
||||||
|
|
||||||
|
def test_login_request_password_accepted_at_72_bytes() -> None:
|
||||||
|
"""LoginRequest accepts passwords exactly 72 bytes long."""
|
||||||
|
from app.models.auth import LoginRequest
|
||||||
|
|
||||||
|
password_72 = "a" * 72
|
||||||
|
req = LoginRequest(password=password_72)
|
||||||
|
assert req.password == password_72
|
||||||
|
|
||||||
|
|
||||||
|
def test_login_request_password_rejected_over_72_bytes() -> None:
|
||||||
|
"""LoginRequest rejects passwords exceeding 72 bytes."""
|
||||||
|
from app.models.auth import LoginRequest
|
||||||
|
|
||||||
|
password_73 = "a" * 73
|
||||||
|
with pytest.raises(ValidationError) as exc_info:
|
||||||
|
LoginRequest(password=password_73)
|
||||||
|
error_msg = str(exc_info.value)
|
||||||
|
assert "at most 72 characters" in error_msg or "max_length" in error_msg
|
||||||
|
|
||||||
|
|
||||||
|
def test_setup_request_master_password_accepted_at_72_bytes() -> None:
|
||||||
|
"""SetupRequest accepts master_password exactly 72 bytes long."""
|
||||||
|
from app.models.setup import SetupRequest
|
||||||
|
|
||||||
|
password_72 = "Password1!" + "a" * 61 # 72 chars total with required complexity
|
||||||
|
req = SetupRequest(master_password=password_72)
|
||||||
|
assert req.master_password == password_72
|
||||||
|
|
||||||
|
|
||||||
|
def test_setup_request_master_password_rejected_over_72_bytes() -> None:
|
||||||
|
"""SetupRequest rejects master_password exceeding 72 bytes."""
|
||||||
|
from app.models.setup import SetupRequest
|
||||||
|
|
||||||
|
password_73 = "Password1!" + "a" * 63 # 73 chars total
|
||||||
|
with pytest.raises(ValidationError) as exc_info:
|
||||||
|
SetupRequest(master_password=password_73)
|
||||||
|
error_msg = str(exc_info.value)
|
||||||
|
assert "at most 72 characters" in error_msg or "string_too_long" in error_msg
|
||||||
|
|
||||||
|
|
||||||
|
def test_setup_request_master_password_min_length_still_enforced() -> None:
|
||||||
|
"""SetupRequest still enforces minimum password length (8 characters)."""
|
||||||
|
from app.models.setup import SetupRequest
|
||||||
|
|
||||||
|
with pytest.raises(ValidationError) as exc_info:
|
||||||
|
SetupRequest(master_password="Short1!")
|
||||||
|
error_msg = str(exc_info.value)
|
||||||
|
assert "8 characters" in error_msg
|
||||||
|
|
||||||
|
|
||||||
|
def test_setup_request_master_password_complexity_still_enforced() -> None:
|
||||||
|
"""SetupRequest still enforces password complexity requirements."""
|
||||||
|
from app.models.setup import SetupRequest
|
||||||
|
|
||||||
|
# Missing uppercase
|
||||||
|
with pytest.raises(ValidationError) as exc_info:
|
||||||
|
SetupRequest(master_password="password123!")
|
||||||
|
assert "uppercase" in str(exc_info.value)
|
||||||
|
|
||||||
|
# Missing number
|
||||||
|
with pytest.raises(ValidationError) as exc_info:
|
||||||
|
SetupRequest(master_password="Password!")
|
||||||
|
assert "number" in str(exc_info.value)
|
||||||
|
|
||||||
|
# Missing special character
|
||||||
|
with pytest.raises(ValidationError) as exc_info:
|
||||||
|
SetupRequest(master_password="Password1")
|
||||||
|
assert "special character" in str(exc_info.value)
|
||||||
|
|||||||
Reference in New Issue
Block a user