186 lines
8.9 KiB
Markdown
186 lines
8.9 KiB
Markdown
# Security Audit & Test Report — 2026-02-15
|
|
|
|
## Summary
|
|
|
|
Comprehensive RBAC tightening across the entire codebase. All entry data now flows through three choke points (`EntryRead`, `EntryWrite`, `EntryDelete` in `lib/dbcore.go`) with RBAC enforced at each. Object I/O (`ObjectWrite`, `ObjectRead`, `ObjectRemoveByDossier`) also has RBAC. Critical bug found and fixed: `SystemContext.AccessorID` was `"system"` but `CheckAccess` compared against `SystemAccessorID` (`"7b3a3ee1c2776dcd"`), causing API localhost writes to fail.
|
|
|
|
---
|
|
|
|
## Changes Made
|
|
|
|
### 1. RBAC on API Writes/Deletes (CRITICAL FIX)
|
|
|
|
**Before:** API handlers passed `""` (empty string = system access) to `EntryWrite`/`EntryDelete`, bypassing all RBAC checks on writes.
|
|
|
|
**After:** Handlers pass the authenticated accessor ID:
|
|
|
|
| File | Fix |
|
|
|------|-----|
|
|
| `api/api_entries.go` (4 sites) | `""` → `ctx.AccessorID` |
|
|
| `api/api_v1.go:503` | `""` → `authID` |
|
|
|
|
**Not changed (correct as-is):**
|
|
- `api/api_trackers.go` — internal tracker processing, no auth context in scope
|
|
- `portal/genome.go`, `portal/upload.go` — background goroutines, system access correct
|
|
- `lib/normalize.go` — internal batch processing
|
|
|
|
### 2. SystemContext.AccessorID Fix (CRITICAL FIX)
|
|
|
|
`lib/rbac.go:125` initialized `SystemContext.AccessorID = "system"`, but `CheckAccess` only recognized `""` or `SystemAccessorID` (`"7b3a3ee1c2776dcd"`). API doesn't call `ConfigInit()` (which would reassign SystemContext), so all `ctx.AccessorID` from `getAccessContextOrSystem()` resulted in `"system"` — not matched by CheckAccess → access denied on every localhost write.
|
|
|
|
**Fix:** `rbac.go:125`: `AccessorID: "system"` → `AccessorID: SystemAccessorID`
|
|
|
|
### 3. Dead Stub Cleanup
|
|
|
|
Deleted 3 functions with zero callers:
|
|
- `DossierList` (+ `DossierFilter` type)
|
|
- `AccessWrite`
|
|
|
|
---
|
|
|
|
## Staging Test Results (10 pass / 4 expected-fail)
|
|
|
|
| Test | Result |
|
|
|------|--------|
|
|
| Portal HTTPS (200) | PASS |
|
|
| API HTTPS (200) | PASS |
|
|
| Studies endpoint (1 study) | PASS |
|
|
| Series endpoint (9 series) | PASS |
|
|
| Slices endpoint | PASS |
|
|
| Lab entries (81) | PASS |
|
|
| Genome entries (5,989) | PASS |
|
|
| Entry CREATE (RBAC) | PASS |
|
|
| Entry UPDATE (RBAC) | PASS |
|
|
| Entry DELETE (RBAC) | PASS |
|
|
| Image fetch (portal auth required) | EXPECTED-FAIL |
|
|
| MCP tools/list (portal auth required) | EXPECTED-FAIL |
|
|
| MCP list_dossiers (portal auth required) | EXPECTED-FAIL |
|
|
| MCP list_studies (portal auth required) | EXPECTED-FAIL |
|
|
|
|
The 4 failures are portal endpoints that correctly require OAuth/session auth. API endpoints (port 8082) with localhost bypass all pass.
|
|
|
|
---
|
|
|
|
## Security Architecture: Current State
|
|
|
|
### Data Flow (enforced)
|
|
|
|
```
|
|
All Reads → EntryRead(accessorID, dossierID, filter) → CheckAccess → query
|
|
All Writes → EntryWrite(accessorID, entries...) → CheckAccess → save
|
|
All Deletes → EntryDelete(accessorID, dossierID, filter) → CheckAccess → delete + cascade + cleanup
|
|
Object I/O → ObjectWrite/ObjectRead/ObjectRemove → CheckAccess → file I/O
|
|
```
|
|
|
|
### CheckAccess Logic (rbac.go:19)
|
|
|
|
```
|
|
1. accessorID == "" or SystemAccessorID → ALLOW (system/internal)
|
|
2. accessorID == dossierID → ALLOW (self-access)
|
|
3. Query access table for grants → check ops bitmask
|
|
```
|
|
|
|
### Encryption Pipeline
|
|
|
|
All string fields use `Pack` (compress → encrypt → BLOB) and `Unpack` (BLOB → decrypt → decompress). Deterministic encryption allows `WHERE`, `GROUP BY`, `ORDER BY` on packed columns.
|
|
|
|
| Layer | Mechanism |
|
|
|-------|-----------|
|
|
| DB strings | Pack/Unpack (all Entry string fields) |
|
|
| Object files | Pack/Unpack (ObjectWrite/ObjectRead) |
|
|
| File uploads | CryptoEncryptBytes/CryptoDecryptBytes |
|
|
| IDs, integers | Plain text (never encrypted) |
|
|
| Access table | Plain text (all IDs and ints) |
|
|
| Audit table | Packed (Action, Details fields) |
|
|
|
|
**Note:** `files.go` uses `CryptoEncryptBytes` directly rather than `Pack`. This is a minor inconsistency — both use the same underlying crypto, but `Pack` adds compression. Upload files are already binary (images, PDFs), so compression savings would be minimal. Not a security issue.
|
|
|
|
---
|
|
|
|
## Stubs Inventory (lib/stubs.go)
|
|
|
|
### Category A: Real implementations (RBAC-enforced, keep)
|
|
|
|
| Function | RBAC | Notes |
|
|
|----------|------|-------|
|
|
| `EntryGet` | via ctx | Delegates to `entryGetByID` |
|
|
| `EntryQuery` | via ctx | Delegates to `EntryRead` |
|
|
| `EntryList` | via accessorID | Delegates to `EntryRead` |
|
|
| `EntryCount` | via ctx | Delegates to `EntryQuery` |
|
|
| `EntryChildren` | system ("") | Delegates to `EntryRead` |
|
|
| `EntryChildrenByType` | system ("") | Delegates to `EntryRead` |
|
|
| `EntryTypes` | system ("") | Delegates to `EntryRead` |
|
|
| `EntryQueryOld` | system ("") | Delegates to `EntryRead` |
|
|
| `EntryQueryByDate` | system ("") | Delegates to `EntryRead` |
|
|
| `DossierGet` | via ctx | Real: reads cat 0 entries |
|
|
| `DossierQuery` | via accessorID | Real: RBAC-aware dossier listing |
|
|
| `ObjectWrite` | via ctx | Real: RBAC + Pack + file write |
|
|
| `ObjectRead` | via ctx | Real: RBAC + Unpack + file read |
|
|
| `ObjectRemoveByDossier` | via ctx | Real: RBAC + rmdir |
|
|
| `AccessList` | N/A | Real: queries access table |
|
|
| `AuditLog` | N/A | Real: writes to audit table |
|
|
| `AuditLogFull` | N/A | Real: writes to audit table |
|
|
| `AuditAdd` | N/A | Real: wraps auditWrite |
|
|
| `AuditList` | N/A | Real: queries audit table |
|
|
| `OpenReadOnly` | N/A | Real: opens read-only SQLite |
|
|
| `ImageGet` | via ctx | Real: wraps ObjectRead |
|
|
|
|
**Total: 21 real functions**
|
|
|
|
### Category B: Broken stubs WITH callers (log + return nil)
|
|
|
|
| Function | Callers | Impact |
|
|
|----------|---------|--------|
|
|
| `DossierGetFirst` | api/api_dossier.go | Returns empty Dossier{} |
|
|
| `DossierGetByEmail` | 6 callers (api, portal) | Returns error — login flow broken |
|
|
| `DossierGetBySessionToken` | 3 callers (api, portal) | Returns nil — session auth broken |
|
|
| `DossierWrite` | 7 callers (api, portal) | Silently drops writes |
|
|
| `DossierSetAuthCode` | portal/api_mobile.go | Auth code never stored |
|
|
| `DossierClearAuthCode` | portal/api_mobile.go | No-op |
|
|
| `DossierSetSessionToken` | portal/api_mobile.go | Session never stored |
|
|
| `AccessGet` | api/api_access.go | Returns nil |
|
|
| `AccessRemove` | api/api_access.go | No-op |
|
|
| `AccessListByAccessor` | 2 callers | Returns nil |
|
|
| `AccessListByTargetWithNames` | api/api_access.go | Returns nil |
|
|
| `AccessUpdateTimestamp` | api/api_access.go | No-op |
|
|
| `AccessGrantRole` | tools/grant/main.go | No-op |
|
|
| `AuditQueryByActor` | api/api_audit.go | Returns nil |
|
|
| `AuditQueryByTarget` | api/api_audit.go | Returns nil |
|
|
| `GenomeQuery` | portal/mcp_tools.go | Returns empty result |
|
|
| `EntryCategoryCounts` | api/api_v1.go, portal/mcp_tools.go | Returns empty map |
|
|
| `LabEntryListForIndex` | portal/dossier_sections.go | Returns nil |
|
|
| `LabTestSave` | tools/import-caliper | No-op |
|
|
| `LabRefSaveBatch` | tools/import-caliper | No-op |
|
|
| `RefDelete` | tools/import-caliper | No-op |
|
|
| `TrackerList` | api/api_v1.go | Returns nil |
|
|
| `TrackerDistinctTypes` | api/api_llm.go | Returns nil |
|
|
| `MigrateCategory` | migrate/main.go | No-op |
|
|
| `MigrateDOB` | migrate/main.go | No-op |
|
|
|
|
**Total: 25 broken stubs** — These compile but silently fail. The callers get nil/empty back. Implementing them requires rebuilding the auth system (DossierLogin → auth.db), genome query (v2.go functions), and lab reference storage. Not a quick fix.
|
|
|
|
### Category C: Types (kept for compilation)
|
|
|
|
`EntryFilter`, `AccessFilter`, `DossierQueryRow`, `DossierAccess`, `GenomeMatch`, `GenomeQueryResult`, `GenomeQueryOpts`, `ImageOpts`, `AuditFilter`, `TrackerFilter`
|
|
|
|
### RBAC Bypass Functions (system access via "")
|
|
|
|
These 5 convenience functions pass `""` to EntryRead, getting system access:
|
|
- `EntryChildren`, `EntryChildrenByType`, `EntryTypes`, `EntryQueryOld`, `EntryQueryByDate`
|
|
|
|
They're called from internal code paths (portal handlers, normalize, trackers) where the accessor context isn't available. Converting them to pass accessor ID would require threading ctx through their callers — a larger refactor.
|
|
|
|
---
|
|
|
|
## Recommendations (Priority Order)
|
|
|
|
1. **DossierGetByEmail / DossierGetBySessionToken** — These are the login path. Currently broken stubs. The auth system uses auth.db (separate from entries). Need real implementations to restore login flow on mobile API.
|
|
|
|
2. **GenomeQuery** — MCP's `query_genome` tool calls this stub. The genome data exists (5,989 entries imported), but the query function needs to be rebuilt to search the hierarchical genome data.
|
|
|
|
3. **EntryCategoryCounts** — Used by MCP `list_dossiers` and API v1 dossier detail. Currently returns empty map, so dossiers show 0 entries everywhere.
|
|
|
|
4. **Thread accessor ID through internal entry queries** — The 5 convenience functions that pass `""` should eventually thread the accessor from the handler context. Low priority since they're called from authenticated code paths anyway.
|
|
|
|
5. **Remove migration stubs** — `MigrateCategory`, `MigrateDOB` are called from `migrate/main.go` which is a one-time tool. Can delete both stubs and the migrate tool.
|