feat: update implementation specification for additional security enhancements
This commit is contained in:
GitHub Actions
@@ -1,489 +1,359 @@
|
||||
# PR #434 Codecov Coverage Gap Remediation Plan
|
||||
# Issue #365: Additional Security Enhancements - Implementation Specification
|
||||
|
||||
**Status**: Analysis Complete - REMEDIATION REQUIRED
|
||||
**Status**: Planning Complete
|
||||
**Created**: 2025-12-21
|
||||
**Last Updated**: 2025-12-21
|
||||
**Objective**: Increase patch coverage from 87.31% to meet 85% threshold across 7 files
|
||||
**Issue**: https://github.com/Wikid82/Charon/issues/365
|
||||
**Branch**: `feature/issue-365-additional-security`
|
||||
**PRs**: #436 (targets `development`), #437 (targets `main`)
|
||||
|
||||
---
|
||||
|
||||
## Executive Summary
|
||||
|
||||
**Coverage Status:** ⚠️ 78 MISSING LINES across 7 files
|
||||
This specification details the implementation plan for Issue #365, which addresses six security enhancement areas. After thorough codebase analysis, this document provides file-by-file implementation details, phase breakdown, and complexity estimates.
|
||||
|
||||
PR #434: `feat: add API-Friendly security header preset for mobile apps`
|
||||
- **Branch:** `feature/beta-release`
|
||||
- **Patch Coverage:** 87.31% (above 85% threshold ✅)
|
||||
- **Total Missing Lines:** 78 lines across 7 files
|
||||
- **Recommendation:** Add targeted tests to improve coverage and reduce technical debt
|
||||
### Scope Summary
|
||||
|
||||
### Coverage Gap Summary
|
||||
|
||||
| File | Coverage | Missing | Partials | Priority | Effort |
|
||||
|------|----------|---------|----------|----------|--------|
|
||||
| `handlers/testdb.go` | 61.53% | 29 | 1 | **P1** | Medium |
|
||||
| `handlers/proxy_host_handler.go` | 75.00% | 25 | 4 | **P1** | High |
|
||||
| `handlers/security_headers_handler.go` | 93.75% | 8 | 4 | P2 | Low |
|
||||
| `handlers/test_helpers.go` | 87.50% | 2 | 0 | P3 | Low |
|
||||
| `routes/routes.go` | 66.66% | 1 | 1 | P3 | Low |
|
||||
| `caddy/config.go` | 98.82% | 1 | 1 | P4 | Low |
|
||||
| `handlers/certificate_handler.go` | 50.00% | 1 | 0 | P4 | Low |
|
||||
| Requirement | Status | Complexity | Phase |
|
||||
|-------------|--------|------------|-------|
|
||||
| Supply Chain - SBOM Generation | ❌ TODO | Medium | 2 |
|
||||
| DNS Hijacking - Documentation | ❌ TODO | Low | 1 |
|
||||
| TLS Downgrade - Documentation | ✅ Partial | Low | 1 |
|
||||
| Privilege Escalation - Container Hardening | ⚠️ Partial | Medium | 2 |
|
||||
| Session Hijacking - Cookie/CSP Audit | ✅ Implemented | Low | 1 |
|
||||
| Timing Attacks - Constant-Time Comparison | ❌ TODO | Medium | 2 |
|
||||
| SIRP Documentation | ❌ TODO | Low | 1 |
|
||||
| Security Update Notifications Doc | ❌ TODO | Low | 1 |
|
||||
|
||||
---
|
||||
|
||||
## Detailed Analysis by File
|
||||
## Codebase Research Findings
|
||||
|
||||
---
|
||||
### 1. Cookie/Session Implementation
|
||||
|
||||
### 1. `backend/internal/api/handlers/testdb.go` (29 Missing, 1 Partial)
|
||||
|
||||
**File Purpose:** Test database utilities providing template DB and migrations for faster test setup.
|
||||
|
||||
**Current Coverage:** 61.53%
|
||||
|
||||
**Test File:** `testdb_test.go` (exists - 200+ lines)
|
||||
|
||||
#### Uncovered Code Paths
|
||||
|
||||
| Lines | Function | Issue | Solution |
|
||||
|-------|----------|-------|----------|
|
||||
| 26-28 | `initTemplateDB()` | Error return path after `gorm.Open` fails | Mock DB open failure |
|
||||
| 32-55 | `initTemplateDB()` | `AutoMigrate` error path | Inject migration failure |
|
||||
| 98-104 | `OpenTestDBWithMigrations()` | `rows.Scan` error + empty sql handling | Test with corrupted template |
|
||||
| 109-131 | `OpenTestDBWithMigrations()` | Fallback AutoMigrate path | Force template DB unavailable |
|
||||
|
||||
#### Test Scenarios to Add
|
||||
**Location**: [backend/internal/api/handlers/auth_handler.go](backend/internal/api/handlers/auth_handler.go)
|
||||
|
||||
**Current Implementation** (lines 52-73):
|
||||
```go
|
||||
// File: backend/internal/api/handlers/testdb_coverage_test.go
|
||||
|
||||
func TestInitTemplateDB_OpenError(t *testing.T) {
|
||||
// Cannot directly test since initTemplateDB uses sync.Once
|
||||
// This path is covered by testing GetTemplateDB behavior
|
||||
// when underlying DB operations fail
|
||||
}
|
||||
|
||||
func TestOpenTestDBWithMigrations_TemplateUnavailable(t *testing.T) {
|
||||
// Force the template DB to be unavailable
|
||||
// Verify fallback AutoMigrate is called
|
||||
// Test by checking table creation works
|
||||
}
|
||||
|
||||
func TestOpenTestDBWithMigrations_ScanError(t *testing.T) {
|
||||
// Test when rows.Scan returns error
|
||||
// Should fall through to fallback path
|
||||
}
|
||||
|
||||
func TestOpenTestDBWithMigrations_EmptySQL(t *testing.T) {
|
||||
// Test when sql string is empty
|
||||
// Should skip db.Exec call
|
||||
func setSecureCookie(c *gin.Context, name, value string, maxAge int) {
|
||||
scheme := requestScheme(c)
|
||||
secure := isProduction() && scheme == "https"
|
||||
sameSite := http.SameSiteStrictMode
|
||||
if scheme != "https" {
|
||||
sameSite = http.SameSiteLaxMode
|
||||
}
|
||||
c.SetSameSite(sameSite)
|
||||
c.SetCookie(name, value, maxAge, "/", "", secure, true) // HttpOnly: true ✅
|
||||
}
|
||||
```
|
||||
|
||||
#### Recommended Actions
|
||||
**Assessment**: ✅ **SECURE** - All cookie security attributes properly configured:
|
||||
- `HttpOnly: true` - Prevents XSS access
|
||||
- `Secure: true` (production + HTTPS)
|
||||
- `SameSite: Strict` (HTTPS) / `Lax` (HTTP dev)
|
||||
|
||||
1. **Add `testdb_coverage_test.go`** with scenarios above
|
||||
2. **Complexity:** Medium - requires mocking GORM internals or using test doubles
|
||||
3. **Alternative:** Accept lower coverage since this is test infrastructure code
|
||||
### 2. Security Headers Implementation
|
||||
|
||||
**Note:** This file is test-only infrastructure (`testdb.go`). Coverage gaps here are acceptable since:
|
||||
- The happy path is already tested
|
||||
- Error paths are defensive programming
|
||||
- Testing test utilities creates circular dependencies
|
||||
**Location**: [backend/internal/api/middleware/security.go](backend/internal/api/middleware/security.go)
|
||||
|
||||
**Recommendation:** P3 - Lower priority, accept current coverage for test utilities.
|
||||
**Current Headers Set**:
|
||||
- ✅ Content-Security-Policy (CSP)
|
||||
- ✅ Strict-Transport-Security (HSTS) with preload
|
||||
- ✅ X-Frame-Options: DENY
|
||||
- ✅ X-Content-Type-Options: nosniff
|
||||
- ✅ X-XSS-Protection: 1; mode=block
|
||||
- ✅ Referrer-Policy: strict-origin-when-cross-origin
|
||||
- ✅ Permissions-Policy (restricts camera, mic, etc.)
|
||||
- ✅ Cross-Origin-Opener-Policy: same-origin
|
||||
- ✅ Cross-Origin-Resource-Policy: same-origin
|
||||
|
||||
**Assessment**: ✅ **COMPREHENSIVE** - All major security headers present.
|
||||
|
||||
### 3. Token Comparison Methods
|
||||
|
||||
**Locations Analyzed**:
|
||||
|
||||
| File | Function | Method | Status |
|
||||
|------|----------|--------|--------|
|
||||
| [models/user.go#L62](backend/internal/models/user.go#L62) | `CheckPassword` | `bcrypt.CompareHashAndPassword` | ✅ Constant-time |
|
||||
| [services/security_service.go#L151](backend/internal/services/security_service.go#L151) | `VerifyBreakGlassToken` | `bcrypt.CompareHashAndPassword` | ✅ Constant-time |
|
||||
| [services/auth_service.go#L128](backend/internal/services/auth_service.go#L128) | `ValidateToken` | JWT library | ✅ Handled by library |
|
||||
|
||||
**Potential Issue Found**: Invite token validation uses database lookup which could leak timing:
|
||||
- Location: `backend/internal/api/handlers/user_handler.go` (AcceptInvite)
|
||||
- Risk: Low (requires network timing analysis)
|
||||
- Recommendation: Add constant-time wrapper for token comparison after DB lookup
|
||||
|
||||
### 4. Current Security Documentation
|
||||
|
||||
**Location**: [docs/security.md](docs/security.md)
|
||||
|
||||
**Current Coverage**:
|
||||
- ✅ Cerberus security suite (CrowdSec, WAF, ACL)
|
||||
- ✅ Access Lists configuration
|
||||
- ✅ Certificate management
|
||||
- ✅ Break-glass token
|
||||
- ✅ Zero-day protection explanation
|
||||
- ❌ TLS version enforcement (not documented)
|
||||
- ❌ DNS security (not documented)
|
||||
- ❌ SIRP (not documented)
|
||||
- ❌ Container hardening (not documented)
|
||||
|
||||
### 5. CI/CD Pipeline Analysis
|
||||
|
||||
**Location**: [.github/workflows/docker-build.yml](.github/workflows/docker-build.yml)
|
||||
|
||||
**Current State**:
|
||||
- ✅ Trivy vulnerability scanning (SARIF + table output)
|
||||
- ✅ Weekly security rebuilds (separate workflow)
|
||||
- ✅ Caddy security patches verification
|
||||
- ❌ SBOM generation (not implemented)
|
||||
- ❌ SBOM attestation (not implemented)
|
||||
|
||||
**Best Integration Point**: After `build-and-push` step, before Trivy scan (around line 130)
|
||||
|
||||
### 6. Dockerfile Security Analysis
|
||||
|
||||
**Location**: [Dockerfile](Dockerfile)
|
||||
|
||||
**Current Security Features**:
|
||||
- ✅ Non-root user (`charon:charon`, UID 1000)
|
||||
- ✅ Healthcheck configured
|
||||
- ✅ Minimal base image (Alpine)
|
||||
- ✅ Multi-stage build (reduces attack surface)
|
||||
- ⚠️ Root filesystem writable (can be improved)
|
||||
- ⚠️ All capabilities retained (can drop unnecessary ones)
|
||||
|
||||
---
|
||||
|
||||
### 2. `backend/internal/api/handlers/proxy_host_handler.go` (25 Missing, 4 Partials)
|
||||
## Phase 1: Documentation Enhancements (Estimated: 1-2 hours)
|
||||
|
||||
**File Purpose:** CRUD operations for proxy hosts including bulk security header updates.
|
||||
### 1.1 TLS Downgrade Attack Documentation
|
||||
|
||||
**Current Coverage:** 75.00%
|
||||
**File**: `docs/security.md`
|
||||
|
||||
**Test Files:**
|
||||
- `proxy_host_handler_test.go`
|
||||
- `proxy_host_handler_security_headers_test.go`
|
||||
**Add Section**:
|
||||
```markdown
|
||||
## TLS Security
|
||||
|
||||
#### Uncovered Code Paths (New in PR #434)
|
||||
### TLS Version Enforcement
|
||||
|
||||
| Lines | Function | Issue | Solution |
|
||||
|-------|----------|-------|----------|
|
||||
| 222-226 | `Update()` | `enable_standard_headers` null handling | Test with null payload |
|
||||
| 227-232 | `Update()` | `forward_auth_enabled` bool handling | Test update with this field |
|
||||
| 234-237 | `Update()` | `waf_disabled` bool handling | Test update with this field |
|
||||
| 286-340 | `Update()` | `security_header_profile_id` type conversions | Test int, string, float64, default cases |
|
||||
| 302-305 | `Update()` | Failed float64→uint conversion (negative) | Test with -1 value |
|
||||
| 312-315 | `Update()` | Failed int→uint conversion (negative) | Test with -1 value |
|
||||
| 322-325 | `Update()` | Failed string parse | Test with "invalid" string |
|
||||
| 326-328 | `Update()` | Unsupported type default case | Test with bool or array |
|
||||
| 331-334 | `Update()` | Conversion failed response | Implicit test from above |
|
||||
| 546-549 | `BulkUpdateSecurityHeaders()` | Profile lookup DB error (non-404) | Mock DB error |
|
||||
Charon (via Caddy) enforces a minimum TLS version of 1.2 by default. This prevents TLS downgrade attacks that attempt to force connections to use vulnerable TLS 1.0 or 1.1.
|
||||
|
||||
#### Test Scenarios to Add
|
||||
**What's Protected:**
|
||||
- ✅ TLS 1.0/1.1 downgrade attacks
|
||||
- ✅ BEAST, POODLE, and similar protocol-level attacks
|
||||
- ✅ Weak cipher suite negotiation
|
||||
|
||||
**HSTS (HTTP Strict Transport Security):**
|
||||
Charon sets HSTS headers with:
|
||||
- `max-age=31536000` (1 year)
|
||||
- `includeSubDomains`
|
||||
- `preload` (for browser preload lists)
|
||||
```
|
||||
|
||||
**Complexity**: Low | **Dependencies**: None
|
||||
|
||||
---
|
||||
|
||||
### 1.2 DNS Hijacking Documentation
|
||||
|
||||
**File**: `docs/security.md`
|
||||
|
||||
**Add Section**:
|
||||
```markdown
|
||||
## DNS Security
|
||||
|
||||
### Protecting Against DNS Hijacking
|
||||
|
||||
Configure your upstream resolver to use DNS-over-HTTPS (DoH) or DNS-over-TLS (DoT):
|
||||
|
||||
**Docker Host Configuration:**
|
||||
```bash
|
||||
# /etc/systemd/resolved.conf
|
||||
[Resolve]
|
||||
DNS=1.1.1.1#cloudflare-dns.com
|
||||
DNSOverTLS=yes
|
||||
```
|
||||
|
||||
**Additional Protections:**
|
||||
1. **DNSSEC**: Ensure your domain registrar supports DNSSEC
|
||||
2. **CAA Records**: Restrict which CAs can issue certs for your domain
|
||||
```
|
||||
|
||||
**Complexity**: Low | **Dependencies**: None
|
||||
|
||||
---
|
||||
|
||||
### 1.3 Security Incident Response Plan
|
||||
|
||||
**New File**: `docs/security-incident-response.md`
|
||||
|
||||
**Content**: Incident classification, detection, containment, recovery procedures
|
||||
|
||||
**Complexity**: Low | **Dependencies**: None
|
||||
|
||||
---
|
||||
|
||||
### 1.4 Security Update Notifications
|
||||
|
||||
**Files**: `docs/getting-started.md`, `docs/security.md`
|
||||
|
||||
**Add**: GitHub Watch instructions, Watchtower/Diun configuration examples
|
||||
|
||||
**Complexity**: Low | **Dependencies**: None
|
||||
|
||||
---
|
||||
|
||||
## Phase 2: Code Changes (Estimated: 4-6 hours)
|
||||
|
||||
### 2.1 SBOM Generation
|
||||
|
||||
**File**: `.github/workflows/docker-build.yml`
|
||||
|
||||
**Changes** (add after build-and-push step):
|
||||
```yaml
|
||||
- name: Generate SBOM (CycloneDX)
|
||||
uses: anchore/sbom-action@v0
|
||||
with:
|
||||
image: ${{ env.REGISTRY }}/${{ env.IMAGE_NAME }}@${{ steps.build-and-push.outputs.digest }}
|
||||
format: cyclonedx-json
|
||||
output-file: sbom.cyclonedx.json
|
||||
|
||||
- name: Attest SBOM to Image
|
||||
if: github.event_name != 'pull_request'
|
||||
uses: actions/attest-sbom@v2
|
||||
with:
|
||||
subject-name: ${{ env.REGISTRY }}/${{ env.IMAGE_NAME }}
|
||||
subject-digest: ${{ steps.build-and-push.outputs.digest }}
|
||||
sbom-path: sbom.cyclonedx.json
|
||||
```
|
||||
|
||||
**Additional Files**:
|
||||
- `.gitignore`: Add `sbom*.json`
|
||||
- `.dockerignore`: Add `sbom*.json`
|
||||
|
||||
**Complexity**: Medium | **Dependencies**: None
|
||||
|
||||
---
|
||||
|
||||
### 2.2 Container Hardening Documentation
|
||||
|
||||
**File**: `docs/security.md`
|
||||
|
||||
**Add Example**:
|
||||
```yaml
|
||||
services:
|
||||
charon:
|
||||
image: ghcr.io/wikid82/charon:latest
|
||||
read_only: true
|
||||
tmpfs:
|
||||
- /tmp:size=100M
|
||||
cap_drop:
|
||||
- ALL
|
||||
cap_add:
|
||||
- NET_BIND_SERVICE
|
||||
security_opt:
|
||||
- no-new-privileges:true
|
||||
```
|
||||
|
||||
**Complexity**: Medium | **Dependencies**: Testing with read-only FS
|
||||
|
||||
---
|
||||
|
||||
### 2.3 Constant-Time Token Comparison
|
||||
|
||||
**New File**: `backend/internal/util/crypto.go`
|
||||
```go
|
||||
// File: backend/internal/api/handlers/proxy_host_handler_update_test.go
|
||||
package util
|
||||
|
||||
func TestProxyHostUpdate_EnableStandardHeaders_Null(t *testing.T) {
|
||||
// Create host, then update with enable_standard_headers: null
|
||||
// Verify host.EnableStandardHeaders becomes nil
|
||||
}
|
||||
import "crypto/subtle"
|
||||
|
||||
func TestProxyHostUpdate_EnableStandardHeaders_True(t *testing.T) {
|
||||
// Create host, then update with enable_standard_headers: true
|
||||
// Verify host.EnableStandardHeaders is pointer to true
|
||||
}
|
||||
|
||||
func TestProxyHostUpdate_EnableStandardHeaders_False(t *testing.T) {
|
||||
// Create host, then update with enable_standard_headers: false
|
||||
// Verify host.EnableStandardHeaders is pointer to false
|
||||
}
|
||||
|
||||
func TestProxyHostUpdate_ForwardAuthEnabled(t *testing.T) {
|
||||
// Create host with forward_auth_enabled: false
|
||||
// Update to forward_auth_enabled: true
|
||||
// Verify change persisted
|
||||
}
|
||||
|
||||
func TestProxyHostUpdate_WAFDisabled(t *testing.T) {
|
||||
// Create host with waf_disabled: false
|
||||
// Update to waf_disabled: true
|
||||
// Verify change persisted
|
||||
}
|
||||
|
||||
func TestProxyHostUpdate_SecurityHeaderProfileID_Int(t *testing.T) {
|
||||
// Create profile, create host
|
||||
// Update with security_header_profile_id as int (Go doesn't JSON decode to int, but test anyway)
|
||||
}
|
||||
|
||||
func TestProxyHostUpdate_SecurityHeaderProfileID_NegativeFloat(t *testing.T) {
|
||||
// Create host
|
||||
// Update with security_header_profile_id: -1.0 (float64)
|
||||
// Expect 400 Bad Request
|
||||
}
|
||||
|
||||
func TestProxyHostUpdate_SecurityHeaderProfileID_NegativeInt(t *testing.T) {
|
||||
// Create host
|
||||
// Update with security_header_profile_id: -1 (if possible via int type)
|
||||
// Expect 400 Bad Request
|
||||
}
|
||||
|
||||
func TestProxyHostUpdate_SecurityHeaderProfileID_InvalidString(t *testing.T) {
|
||||
// Create host
|
||||
// Update with security_header_profile_id: "not-a-number"
|
||||
// Expect 400 Bad Request
|
||||
}
|
||||
|
||||
func TestProxyHostUpdate_SecurityHeaderProfileID_UnsupportedType(t *testing.T) {
|
||||
// Create host
|
||||
// Send security_header_profile_id as boolean (true) or array
|
||||
// Expect 400 Bad Request
|
||||
}
|
||||
|
||||
func TestBulkUpdateSecurityHeaders_DBError_NonNotFound(t *testing.T) {
|
||||
// Close DB connection to simulate internal error
|
||||
// Call bulk update with valid profile ID
|
||||
// Expect 500 Internal Server Error
|
||||
// ConstantTimeCompare compares two strings in constant time.
|
||||
func ConstantTimeCompare(a, b string) bool {
|
||||
return subtle.ConstantTimeCompare([]byte(a), []byte(b)) == 1
|
||||
}
|
||||
```
|
||||
|
||||
#### Recommended Actions
|
||||
**New Test File**: `backend/internal/util/crypto_test.go`
|
||||
|
||||
1. **Add `proxy_host_handler_update_test.go`** with 11 new test cases
|
||||
2. **Estimated effort:** 2-3 hours
|
||||
3. **Impact:** Covers 25 lines, brings coverage to ~95%
|
||||
**Modify**: `backend/internal/api/handlers/user_handler.go` - Use constant-time comparison in AcceptInvite
|
||||
|
||||
**Complexity**: Medium | **Dependencies**: None
|
||||
|
||||
---
|
||||
|
||||
### 3. `backend/internal/api/handlers/security_headers_handler.go` (8 Missing, 4 Partials)
|
||||
## Phase 3: Testing & Validation (Estimated: 2-3 hours)
|
||||
|
||||
**File Purpose:** CRUD for security header profiles, presets, CSP validation.
|
||||
### Required Tests
|
||||
|
||||
**Current Coverage:** 93.75%
|
||||
| File | Purpose |
|
||||
|------|---------|
|
||||
| `backend/internal/util/crypto_test.go` | Constant-time comparison tests + benchmarks |
|
||||
| Integration test additions | Security header verification |
|
||||
|
||||
**Test File:** `security_headers_handler_test.go` (extensive - 500+ lines)
|
||||
### Coverage Requirements
|
||||
|
||||
#### Uncovered Code Paths
|
||||
|
||||
| Lines | Function | Issue | Solution |
|
||||
|-------|----------|-------|----------|
|
||||
| 89-91 | `GetProfile()` | UUID lookup DB error (non-404) | Close DB before UUID lookup |
|
||||
| 142-145 | `UpdateProfile()` | `db.Save()` error | Close DB before save |
|
||||
| 177-180 | `DeleteProfile()` | `db.Delete()` error | Already tested in `TestDeleteProfile_DeleteDBError` |
|
||||
| 269-271 | `validateCSPString()` | Unknown directive warning | Test with `unknown-directive` |
|
||||
|
||||
#### Test Scenarios to Add
|
||||
|
||||
```go
|
||||
// File: backend/internal/api/handlers/security_headers_handler_coverage_test.go
|
||||
|
||||
func TestGetProfile_UUID_DBError_NonNotFound(t *testing.T) {
|
||||
// Create profile, get UUID
|
||||
// Close DB connection
|
||||
// GET /security/headers/profiles/{uuid}
|
||||
// Expect 500 Internal Server Error
|
||||
}
|
||||
|
||||
func TestUpdateProfile_SaveError(t *testing.T) {
|
||||
// Create profile (ID = 1)
|
||||
// Close DB connection
|
||||
// PUT /security/headers/profiles/1
|
||||
// Expect 500 Internal Server Error
|
||||
// Note: Similar to TestUpdateProfile_DBError but for save specifically
|
||||
}
|
||||
```
|
||||
|
||||
**Note:** Most paths are already covered by existing tests. The 8 missing lines are edge cases around DB errors that are already partially tested.
|
||||
|
||||
#### Recommended Actions
|
||||
|
||||
1. **Verify existing tests cover scenarios** - some may already be present
|
||||
2. **Add 2 additional DB error tests** if not covered
|
||||
3. **Estimated effort:** 30 minutes
|
||||
- All new code must achieve 85% coverage
|
||||
- Run: `scripts/go-test-coverage.sh`
|
||||
|
||||
---
|
||||
|
||||
### 4. `backend/internal/api/handlers/test_helpers.go` (2 Missing)
|
||||
## File Change Summary
|
||||
|
||||
**File Purpose:** Polling helpers for test synchronization (`waitForCondition`).
|
||||
### Files to Create
|
||||
|
||||
**Current Coverage:** 87.50%
|
||||
| File | Purpose |
|
||||
|------|---------|
|
||||
| `backend/internal/util/crypto.go` | Constant-time comparison utilities |
|
||||
| `backend/internal/util/crypto_test.go` | Tests for crypto utilities |
|
||||
| `docs/security-incident-response.md` | SIRP documentation |
|
||||
|
||||
**Test File:** `test_helpers_test.go` (exists)
|
||||
### Files to Modify
|
||||
|
||||
#### Uncovered Code Paths
|
||||
| File | Changes |
|
||||
|------|---------|
|
||||
| `docs/security.md` | TLS, DNS, container hardening sections |
|
||||
| `docs/getting-started.md` | Security update notification section |
|
||||
| `.github/workflows/docker-build.yml` | SBOM generation steps |
|
||||
| `.gitignore` | Add `sbom*.json` |
|
||||
| `.dockerignore` | Add `sbom*.json` |
|
||||
| `backend/internal/api/handlers/user_handler.go` | Constant-time token comparison |
|
||||
|
||||
| Lines | Function | Issue | Solution |
|
||||
|-------|----------|-------|----------|
|
||||
| 17-18 | `waitForCondition()` | `t.Fatalf` call on timeout | Cannot directly test without custom interface |
|
||||
| 31-32 | `waitForConditionWithInterval()` | `t.Fatalf` call on timeout | Same issue |
|
||||
### Files Verified Secure (No Changes)
|
||||
|
||||
#### Analysis
|
||||
|
||||
The missing coverage is in the `t.Fatalf()` calls which are intentionally not tested because:
|
||||
1. `t.Fatalf()` terminates the test immediately
|
||||
2. Testing this would require a custom testing.T interface
|
||||
3. The existing tests use mock implementations to verify timeout behavior
|
||||
|
||||
**Current tests already cover:**
|
||||
- `TestWaitForCondition_PassesImmediately`
|
||||
- `TestWaitForCondition_PassesAfterIterations`
|
||||
- `TestWaitForCondition_Timeout` (uses mockTestingT)
|
||||
- `TestWaitForConditionWithInterval_*` variants
|
||||
|
||||
#### Recommended Actions
|
||||
|
||||
1. **Accept current coverage** - The timeout paths are defensive and covered via mocks
|
||||
2. **No additional tests needed** - mockTestingT already verifies the behavior
|
||||
3. **Estimated effort:** None
|
||||
| File | Reason |
|
||||
|------|--------|
|
||||
| `backend/internal/api/handlers/auth_handler.go` | Cookie security correct |
|
||||
| `backend/internal/api/middleware/security.go` | Headers comprehensive |
|
||||
| `backend/internal/models/user.go` | bcrypt is constant-time |
|
||||
| `backend/internal/services/security_service.go` | bcrypt is constant-time |
|
||||
| `Dockerfile` | Non-root user configured |
|
||||
|
||||
---
|
||||
|
||||
### 5. `backend/internal/api/routes/routes.go` (1 Missing, 1 Partial)
|
||||
## Out of Scope (Future Issues)
|
||||
|
||||
**File Purpose:** API route registration and middleware wiring.
|
||||
|
||||
**Current Coverage:** 66.66% (but only 1 new line missing)
|
||||
|
||||
**Test File:** `routes_test.go` (exists)
|
||||
|
||||
#### Uncovered Code Paths
|
||||
|
||||
| Lines | Function | Issue | Solution |
|
||||
|-------|----------|-------|----------|
|
||||
| ~234 | `Register()` | `secHeadersSvc.EnsurePresetsExist()` error logging | Error is logged but not fatal |
|
||||
|
||||
#### Analysis
|
||||
|
||||
The missing line is error handling for `EnsurePresetsExist()`:
|
||||
```go
|
||||
if err := secHeadersSvc.EnsurePresetsExist(); err != nil {
|
||||
logger.Log().WithError(err).Warn("Failed to initialize security header presets")
|
||||
}
|
||||
```
|
||||
|
||||
This is non-fatal logging - the route registration continues even if preset initialization fails.
|
||||
|
||||
#### Test Scenarios to Add
|
||||
|
||||
```go
|
||||
// File: backend/internal/api/routes/routes_security_headers_test.go
|
||||
|
||||
func TestRegister_EnsurePresetsExist_Error(t *testing.T) {
|
||||
// This requires mocking SecurityHeadersService
|
||||
// Or testing with a DB that fails on insert
|
||||
// Low priority since it's just a warning log
|
||||
}
|
||||
```
|
||||
|
||||
#### Recommended Actions
|
||||
|
||||
1. **Accept current coverage** - Error path only logs a warning
|
||||
2. **Low impact** - Registration continues regardless of error
|
||||
3. **Estimated effort:** 30 minutes if mocking is needed
|
||||
|
||||
---
|
||||
|
||||
### 6. `backend/internal/caddy/config.go` (1 Missing, 1 Partial)
|
||||
|
||||
**File Purpose:** Caddy JSON configuration generation.
|
||||
|
||||
**Current Coverage:** 98.82% (excellent)
|
||||
|
||||
**Test Files:** Multiple test files `config_security_headers_test.go`
|
||||
|
||||
#### Uncovered Code Path
|
||||
|
||||
Based on the API-Friendly preset feature, the missing line is likely in `buildSecurityHeadersHandler()` for an edge case.
|
||||
|
||||
#### Analysis
|
||||
|
||||
The existing test `TestBuildSecurityHeadersHandler_APIFriendlyPreset` covers the new API-Friendly preset. The 1 missing line is likely an edge case in:
|
||||
- Empty string handling for headers
|
||||
- Cross-origin policy variations
|
||||
|
||||
#### Recommended Actions
|
||||
|
||||
1. **Review coverage report details** to identify exact line
|
||||
2. **Likely already covered** by `TestBuildSecurityHeadersHandler_APIFriendlyPreset`
|
||||
3. **Estimated effort:** 15 minutes to verify
|
||||
|
||||
---
|
||||
|
||||
### 7. `backend/internal/api/handlers/certificate_handler.go` (1 Missing)
|
||||
|
||||
**File Purpose:** Certificate upload, list, and delete operations.
|
||||
|
||||
**Current Coverage:** 50.00% (only 1 new line)
|
||||
|
||||
**Test File:** `certificate_handler_coverage_test.go` (exists)
|
||||
|
||||
#### Uncovered Code Path
|
||||
|
||||
| Lines | Function | Issue | Solution |
|
||||
|-------|----------|-------|----------|
|
||||
| ~67 | `Delete()` | ID=0 validation check | Already tested |
|
||||
|
||||
#### Analysis
|
||||
|
||||
Looking at the test file, `TestCertificateHandler_Delete_InvalidID` tests the "invalid" ID case but may not specifically test ID=0.
|
||||
|
||||
```go
|
||||
// Validate ID range
|
||||
if id == 0 {
|
||||
c.JSON(http.StatusBadRequest, gin.H{"error": "invalid id"})
|
||||
return
|
||||
}
|
||||
```
|
||||
|
||||
#### Test Scenarios to Add
|
||||
|
||||
```go
|
||||
func TestCertificateHandler_Delete_ZeroID(t *testing.T) {
|
||||
// DELETE /api/certificates/0
|
||||
// Expect 400 Bad Request with "invalid id" error
|
||||
}
|
||||
```
|
||||
|
||||
#### Recommended Actions
|
||||
|
||||
1. **Add single test for ID=0 case**
|
||||
2. **Estimated effort:** 10 minutes
|
||||
|
||||
---
|
||||
|
||||
## Implementation Plan
|
||||
|
||||
### Priority Order (by impact)
|
||||
|
||||
1. **P1: proxy_host_handler.go** - 25 lines, new feature code
|
||||
2. **P1: testdb.go** - 29 lines, but test-only infrastructure (lower actual priority)
|
||||
3. **P2: security_headers_handler.go** - 8 lines, minor gaps
|
||||
4. **P3: test_helpers.go** - Accept current coverage
|
||||
5. **P3: routes.go** - Accept current coverage (warning log only)
|
||||
6. **P4: config.go** - Verify existing coverage
|
||||
7. **P4: certificate_handler.go** - Add 1 test
|
||||
|
||||
### Estimated Effort
|
||||
|
||||
| File | Tests to Add | Time Estimate |
|
||||
|------|--------------|---------------|
|
||||
| `proxy_host_handler.go` | 11 tests | 2-3 hours |
|
||||
| `security_headers_handler.go` | 2 tests | 30 minutes |
|
||||
| `certificate_handler.go` | 1 test | 10 minutes |
|
||||
| `testdb.go` | Skip (test utilities) | 0 |
|
||||
| `test_helpers.go` | Skip (already covered) | 0 |
|
||||
| `routes.go` | Skip (warning log) | 0 |
|
||||
| `config.go` | Verify only | 15 minutes |
|
||||
| **Total** | **14 tests** | **~4 hours** |
|
||||
|
||||
---
|
||||
|
||||
## Test File Locations
|
||||
|
||||
### New Test Files to Create
|
||||
|
||||
1. `backend/internal/api/handlers/proxy_host_handler_update_test.go` - Update field coverage
|
||||
|
||||
### Existing Test Files to Extend
|
||||
|
||||
1. `backend/internal/api/handlers/security_headers_handler_test.go` - Add 2 DB error tests
|
||||
2. `backend/internal/api/handlers/certificate_handler_coverage_test.go` - Add ID=0 test
|
||||
|
||||
---
|
||||
|
||||
## Dependencies Between Tests
|
||||
|
||||
```
|
||||
None identified - all tests can be implemented independently
|
||||
```
|
||||
Per Issue #365:
|
||||
- ❌ Certificate Transparency Log Monitoring
|
||||
- ❌ MFA via Authentik
|
||||
- ❌ SSO for Charon admin
|
||||
- ❌ Audit logging for GDPR/SOC2
|
||||
|
||||
---
|
||||
|
||||
## Acceptance Criteria
|
||||
|
||||
1. ✅ Patch coverage ≥ 85% (currently 87.31%, already passing)
|
||||
2. ⬜ All new test scenarios pass
|
||||
3. ⬜ No regression in existing tests
|
||||
4. ⬜ Test execution time < 30 seconds total
|
||||
5. ⬜ All tests use `OpenTestDB` or `OpenTestDBWithMigrations` for isolation
|
||||
- [ ] Documentation sections added to `docs/security.md`
|
||||
- [ ] SIRP document created
|
||||
- [ ] SBOM generation in CI (CycloneDX format)
|
||||
- [ ] Constant-time utility with tests
|
||||
- [ ] Container hardening documented
|
||||
- [ ] 85%+ test coverage
|
||||
- [ ] Pre-commit hooks pass
|
||||
|
||||
---
|
||||
|
||||
## Mock Requirements
|
||||
|
||||
### For proxy_host_handler.go Tests
|
||||
|
||||
- Standard Gin test router setup (already exists in test files)
|
||||
- GORM SQLite in-memory DB (use `OpenTestDBWithMigrations`)
|
||||
- Mock Caddy manager (nil is acceptable for these tests)
|
||||
|
||||
### For security_headers_handler.go Tests
|
||||
|
||||
- Same as above
|
||||
- Close DB connection to simulate errors
|
||||
|
||||
### For certificate_handler.go Tests
|
||||
|
||||
- Use existing test setup patterns
|
||||
- No mocks needed for ID=0 test
|
||||
|
||||
---
|
||||
|
||||
## Conclusion
|
||||
|
||||
**Immediate Action Required:** None - coverage is above 85% threshold
|
||||
|
||||
**Recommended Improvements:**
|
||||
1. Add 14 targeted tests to improve coverage quality
|
||||
2. Focus on `proxy_host_handler.go` which has the most new feature code
|
||||
3. Accept lower coverage on test infrastructure files (`testdb.go`, `test_helpers.go`)
|
||||
|
||||
**Total Estimated Effort:** ~4 hours for all improvements
|
||||
|
||||
---
|
||||
|
||||
**Analysis Date:** 2025-12-21
|
||||
**Analyzed By:** GitHub Copilot
|
||||
**Next Action:** Implement tests in priority order if coverage improvement is desired
|
||||
**Analysis Date**: 2025-12-21
|
||||
**Analyzed By**: GitHub Copilot
|
||||
**Status**: Ready for Implementation
|
||||
|
||||
Reference in New Issue
Block a user