docs(phase-7): add security threat verification — 12/12 CLOSED
Formal Phase 7 SECURITY.md: - T-07-01: api_key_enc excluded from GET /api/admin/ai-config (whitelist) - T-07-02: HKDF domain separation confirmed (ai-provider-settings vs cloud-credentials) - T-07-03: is_active atomic UPDATE, no read-then-write - T-07-04: empty api_key normalised to "not-needed" before AsyncOpenAI - T-07-05/07/09/11: accepted risks documented - T-07-06/08/10/12: mitigations verified in implementation + tests bandit: zero HIGH; npm audit: zero high/critical; threats_open: 0 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
curo1305
co-authored by
Claude Sonnet 4.6
Claude Sonnet 4.6
parent
1a625ec365
commit
8629bc0854
@@ -0,0 +1,77 @@
|
||||
---
|
||||
phase: 7
|
||||
slug: 07-redo-and-optimize-llm-integration
|
||||
status: verified
|
||||
threats_open: 0
|
||||
asvs_level: 2
|
||||
created: 2026-06-05
|
||||
---
|
||||
|
||||
# Phase 7 — Security
|
||||
|
||||
> Full audit detail in project-root `SECURITY.md` — "Phase 07 Threat Verification" section.
|
||||
|
||||
---
|
||||
|
||||
## Trust Boundaries
|
||||
|
||||
| Boundary | Description | Data Crossing |
|
||||
|----------|-------------|---------------|
|
||||
| Admin API → DB | PUT /api/admin/ai-config writes encrypted api_key to system_settings | AES-GCM ciphertext; plaintext never stored |
|
||||
| Celery → AI Provider | HTTP requests to external AI endpoints using decrypted api_key | API key in memory only, per-task, never serialized back to broker |
|
||||
| Admin API → Client | GET /api/admin/ai-config response | `has_api_key` (bool) only — no ciphertext, no plaintext |
|
||||
|
||||
---
|
||||
|
||||
## Threat Register
|
||||
|
||||
| Threat ID | Category | Component | Disposition | Mitigation | Status |
|
||||
|-----------|----------|-----------|-------------|------------|--------|
|
||||
| T-07-01 | Information Disclosure | GET /api/admin/ai-config response body | mitigate | `_ai_config_to_dict()` whitelist at `admin.py:56–70` excludes `api_key_enc`; `test_get_never_returns_key` asserts absence | CLOSED |
|
||||
| T-07-02 | Elevation of Privilege | HKDF key derivation domain separation | mitigate | `info=b"ai-provider-settings"` vs `info=b"cloud-credentials"`; cross-domain decrypt raises `InvalidToken`; `test_api_key_encrypt_decrypt` validates | CLOSED |
|
||||
| T-07-03 | Tampering | system_settings.is_active dual-write race | mitigate | Single atomic `UPDATE … SET is_active = (provider_id == target)` at `admin.py:902–906`; `test_put_writes_active_provider` asserts COUNT(is_active)=1 | CLOSED |
|
||||
| T-07-04 | Tampering | OpenAI SDK 2.34+ empty api_key rejection | mitigate | Factory applies `api_key or "not-needed"` at `ai/__init__.py:62`; defence-in-depth in `openai_provider.py:16` | CLOSED |
|
||||
| T-07-05 | Information Disclosure | Singleton _client retained across Celery tasks | accept | Each `asyncio.run(_run(...))` creates fresh provider; no module-level client exists | CLOSED |
|
||||
| T-07-06 | Information Disclosure | classifier per-user override path | mitigate | Override path uses hardcoded `api_key=""`; factory normalises to `"not-needed"`; key never read from system_settings in this path | CLOSED |
|
||||
| T-07-07 | Tampering | Anthropic output_config grammar limit | accept | Schemas ≤3 properties/2 required, no unions; well within Anthropic grammar limits | CLOSED |
|
||||
| T-07-08 | Denial of Service | Anthropic stop_reason "refusal"/"max_tokens" | mitigate | `classify()` falls back to `parse_classification("")` → empty `ClassificationResult`; no exception propagated | CLOSED |
|
||||
| T-07-09 | Denial of Service | Re-classify endpoint without sub-100/min rate limit | accept | Auth + ownership + `@account_limiter.limit("100/minute")` present; tighter limit deferred to Phase 6 expansion | CLOSED |
|
||||
| T-07-10 | Tampering | Cross-user reclassify IDOR | mitigate | Inline ownership check at `documents.py:730–732`; `test_reclassify_cross_user_returns_404` validates | CLOSED |
|
||||
| T-07-11 | Information Disclosure | Retry exception payload in Celery broker | accept | Only `str(exc)` — no document content or credentials; Redis broker internal-only | CLOSED |
|
||||
| T-07-12 | Tampering | self.retry called inside asyncio.run | mitigate | `_ClassificationError` sentinel escapes `asyncio.run()`; `self.retry()` in sync layer only; `test_retry_backoff` validates countdown sequence | CLOSED |
|
||||
|
||||
---
|
||||
|
||||
## Accepted Risks Log
|
||||
|
||||
| Risk ID | Component | Accepted Risk | Rationale |
|
||||
|---------|-----------|---------------|-----------|
|
||||
| T-07-05 | Celery AI provider client | No cross-task singleton risk | Fresh event loop per `asyncio.run()` invocation; provider local to `_run()` |
|
||||
| T-07-07 | Anthropic output_config schema | Grammar limit not enforced programmatically | Simple schemas maintained as code convention |
|
||||
| T-07-09 | /classify rate limiting | No sub-100/min per-account rate limit in v1 | 100/min account limiter + auth + ownership present; tighter limit deferred |
|
||||
| T-07-11 | Celery broker exception payload | Exception message flows through Redis broker | `str(exc)` only — no document content or credentials |
|
||||
|
||||
---
|
||||
|
||||
## Security Audit Trail
|
||||
|
||||
| Audit Date | Threats Total | Closed | Open | Run By |
|
||||
|------------|---------------|--------|------|--------|
|
||||
| 2026-06-05 | 12 | 12 | 0 | gsd-security-auditor (claude-sonnet-4-6) |
|
||||
|
||||
---
|
||||
|
||||
## Sign-Off
|
||||
|
||||
- [x] All threats have a disposition (mitigate / accept / transfer)
|
||||
- [x] Accepted risks documented in Accepted Risks Log
|
||||
- [x] `threats_open: 0` confirmed
|
||||
- [x] `status: verified` set in frontmatter
|
||||
|
||||
---
|
||||
|
||||
## Bandit / Dependency Scan
|
||||
|
||||
- `bandit -r backend/ -ll` (2026-06-05): **zero HIGH severity** (0 Medium, 776 Low informational, 0 `# nosec` suppressions)
|
||||
- `npm audit --audit-level=high` (2026-06-05): **zero high/critical** (2 moderate — esbuild/vite dev-only, no fix without breaking change)
|
||||
- `pip-audit`: not runnable locally (Python 3.9 host vs 3.12 project); inherited clean gate from Phase 6.2 (`f1a7f52` — python-multipart + PyMuPDF CVE fixes)
|
||||
+45
@@ -120,6 +120,51 @@ None. All `## Threat Flags` sections in plans 03-01 through 03-05 summaries repo
|
||||
|
||||
---
|
||||
|
||||
## Phase 07 Threat Verification
|
||||
|
||||
**Audit date:** 2026-06-05
|
||||
**Phase:** 7 — Redo and Optimize LLM Integration
|
||||
**ASVS Level:** L2
|
||||
**Auditor:** gsd-security-auditor (claude-sonnet-4-6)
|
||||
**Threats closed:** 12/12
|
||||
**Open threats (blockers):** 0
|
||||
|
||||
### Threat Verification
|
||||
|
||||
| Threat ID | Category | Disposition | Status | Evidence |
|
||||
|-----------|----------|-------------|--------|----------|
|
||||
| T-07-01 | Information Disclosure | mitigate | CLOSED | `_ai_config_to_dict()` at `backend/api/admin.py:56–70` returns exactly 7 whitelisted fields; `api_key_enc` is absent; `has_api_key` is derived as `row.api_key_enc is not None`. Integration test `test_get_never_returns_key` at `backend/tests/test_admin_ai_config.py:33–68` asserts both `"api_key_enc" not in body_text` and `"sk-test-secret" not in body_text`. |
|
||||
| T-07-02 | Elevation of Privilege | mitigate | CLOSED | `_derive_ai_settings_key()` at `backend/services/ai_config.py:67` uses `info=b"ai-provider-settings"`. Cloud path uses `info=b"cloud-credentials"` at `backend/storage/cloud_utils.py:137`. Same master key, different info values → different Fernet instances per domain. Unit test `test_api_key_encrypt_decrypt` at `backend/tests/test_ai_config.py:43–46` asserts cross-provider decrypt raises `InvalidToken`. |
|
||||
| T-07-03 | Tampering | mitigate | CLOSED | Single atomic UPDATE at `backend/api/admin.py:902–906`: `update(SystemSettings).values(is_active=(SystemSettings.provider_id == body.provider_id))`. No read-then-write. Integration test `test_put_writes_active_provider` at `backend/tests/test_admin_ai_config.py:71–115` asserts `COUNT(is_active=True) == 1` after two sequential PUTs with `is_active=True`. |
|
||||
| T-07-04 | Tampering | mitigate | CLOSED | `get_provider()` at `backend/ai/__init__.py:62` applies `effective_api_key = config.api_key or "not-needed"` before constructing any provider. `OpenAIProvider.__init__` at `backend/ai/openai_provider.py:16` applies a defence-in-depth `api_key or "not-needed"`. `GenericOpenAIProvider` inherits via `super().__init__()`. Empty string never reaches `AsyncOpenAI(api_key=...)`. |
|
||||
| T-07-05 | Information Disclosure | accept | CLOSED | Each Celery task calls `asyncio.run(_run(document_id))`; `_run()` opens a fresh `AsyncSessionLocal` and calls `load_provider_config(session)` → `get_provider(config)` to construct a new provider instance. No provider or `_client` is module-level or class-level. Cross-task credential sharing is structurally impossible. |
|
||||
| T-07-06 | Information Disclosure | mitigate | CLOSED | Per-user override path at `backend/services/classifier.py:74–80` constructs `ProviderConfig(api_key="", ...)` — hardcoded empty string; never reads from system_settings. Factory normalises `""` to `"not-needed"` at `backend/ai/__init__.py:62`. API key never flows through the per-user override path. |
|
||||
| T-07-07 | Tampering | accept | CLOSED | `_CLASSIFICATION_SCHEMA` at `backend/ai/anthropic_provider.py:25–34`: 3 properties, 2 required, `additionalProperties: False`, no unions. `_SUGGESTIONS_SCHEMA`: 1 property, 1 required. Neither schema approaches Anthropic grammar size limits. |
|
||||
| T-07-08 | Denial of Service | mitigate | CLOSED | `AnthropicProvider.classify()` at `backend/ai/anthropic_provider.py:103–108`: if `stop_reason != "end_turn"` (covers `"refusal"` and `"max_tokens"`), sets `raw = ""` and calls `parse_classification("")`. `parse_classification("")` at `backend/ai/utils.py:17–35` returns `ClassificationResult()` (empty, no exception). |
|
||||
| T-07-09 | Denial of Service | accept | CLOSED | `/classify` endpoint at `backend/api/documents.py:707–739` requires authentication (`get_regular_user`), ownership check (line 731–732), and has `@account_limiter.limit("100/minute")` at line 708. Sub-100/min per-account limit deferred to Phase 6 expansion. Documented accepted risk. |
|
||||
| T-07-10 | Tampering | mitigate | CLOSED | Inline ownership check at `backend/api/documents.py:730–732`: `if doc is None or doc.user_id != current_user.id: raise HTTPException(404, "Document not found")`. Integration test `test_reclassify_cross_user_returns_404` at `backend/tests/test_documents.py:1009–1059` confirms 404 response for cross-user attempt. |
|
||||
| T-07-11 | Information Disclosure | accept | CLOSED | `_ClassificationError` at `backend/tasks/document_tasks.py:187` raised as `_ClassificationError(str(e))` — only the exception message string is captured, never document content or user data. Redis broker is internal-only in deployment. |
|
||||
| T-07-12 | Tampering | mitigate | CLOSED | `_ClassificationError` sentinel raised inside `asyncio.run(_run(...))` and caught by the outer sync `extract_and_classify` at `backend/tasks/document_tasks.py:81–86`; `self.retry()` is called in the sync layer only. Unit test `test_retry_backoff` at `backend/tests/test_document_tasks.py:26–59` validates the sentinel escape and countdown sequence. |
|
||||
|
||||
### Phase 07 Bandit Result
|
||||
|
||||
`bandit -r backend/ -ll` (run 2026-06-05): **zero HIGH severity findings** (0 Medium, 0 High; 776 Low informational items, 0 `# nosec` suppressions).
|
||||
|
||||
### Phase 07 Unregistered Flags
|
||||
|
||||
None. No `## Threat Flags` section provided for Phase 7. All threats resolved from the supplied register.
|
||||
|
||||
### Phase 07 Accepted Risks
|
||||
|
||||
| Risk ID | Component | Accepted Risk | Rationale |
|
||||
|---------|-----------|---------------|-----------|
|
||||
| T-07-05 | Celery AI provider client | No cross-task singleton risk | `asyncio.run()` creates a fresh event loop per task invocation; provider instances are local to `_run()` and garbage-collected on return. |
|
||||
| T-07-07 | Anthropic output_config schema | Grammar limit not enforced programmatically | Schemas are ≤3 properties / 2 required with no unions; simple schemas maintained as code convention. |
|
||||
| T-07-09 | `/classify` rate limiting | No sub-100/min per-account rate limit in v1 | Auth + ownership gating present; 100/min account limiter present; tighter limit deferred to Phase 6 per-account limiter. |
|
||||
| T-07-11 | Celery broker exception payload | Exception message flows through Redis broker | Only `str(exc)` — no document content or credentials. Redis broker is internal-network only. |
|
||||
|
||||
---
|
||||
|
||||
## Notes
|
||||
|
||||
### Phase 03 Audit Notes
|
||||
|
||||
Reference in New Issue
Block a user