00a18704e8
HTTP/HTTPS uptime monitors targeting LAN addresses (192.168.x.x, 10.x.x.x, 172.16.x.x) permanently reported 'down' on fresh installs because SSRF protection rejects RFC 1918 ranges at two independent checkpoints: the URL validator (DNS-resolution layer) and the safe dialer (TCP-connect layer). Fixing only one layer leaves the monitor broken in practice. - Add IsRFC1918() predicate to the network package covering only the three RFC 1918 CIDRs; 169.254.x.x (link-local / cloud metadata) and loopback are intentionally excluded - Add WithAllowRFC1918() functional option to both SafeHTTPClient and ValidationConfig; option defaults to false so existing behaviour is unchanged for every call site except uptime monitors - In uptime_service.go, pass WithAllowRFC1918() to both ValidateExternalURL and NewSafeHTTPClient together; a coordinating comment documents that both layers must be relaxed as a unit - 169.254.169.254 and the full 169.254.0.0/16 link-local range remain unconditionally blocked; the cloud-metadata error path is preserved - 21 new tests across three packages, including an explicit regression guard that confirms RFC 1918 blocks are still applied without the option set (TestValidateExternalURL_RFC1918BlockedByDefault) Fixes issues 6 and 7 from the fresh-install bug report.
215 lines
7.8 KiB
Markdown
215 lines
7.8 KiB
Markdown
# QA Security Report — PR-3: RFC 1918 Bypass for Uptime Monitor
|
||
|
||
**Date:** 2026-03-17
|
||
**QA Agent:** Security QA
|
||
**Status at Review Start:** Implementation complete, Supervisor-approved
|
||
**Final Verdict:** ✅ APPROVED FOR COMMIT
|
||
|
||
---
|
||
|
||
## Summary
|
||
|
||
PR-3 adds RFC 1918 private-address bypass capability to the uptime monitor system, allowing users to monitor hosts on private network ranges (10.x.x.x, 172.16–31.x.x, 192.168.x.x) without disabling SSRF protections globally. The implementation spans three production files and three test files.
|
||
|
||
---
|
||
|
||
## Pre-Audit Fix
|
||
|
||
**Issue:** The Supervisor identified that `TestValidateExternalURL_WithAllowRFC1918_BlocksMetadata` in `url_validator_test.go` should include `WithAllowHTTP()` to exercise the SSRF IP-level check rather than failing at the scheme check.
|
||
|
||
**Finding:** `WithAllowHTTP()` was already present in the test at audit start. No change required.
|
||
|
||
---
|
||
|
||
## Audit Results
|
||
|
||
### 1. Build Verification
|
||
|
||
```
|
||
cd /projects/Charon/backend && go build ./...
|
||
```
|
||
|
||
**Result: ✅ PASS** — Clean build, zero errors.
|
||
|
||
---
|
||
|
||
### 2. Targeted Package Tests (PR-3 Files)
|
||
|
||
#### `internal/network` — RFC 1918 tests
|
||
|
||
| Test | Result |
|
||
|---|---|
|
||
| `TestIsRFC1918_RFC1918Addresses` (11 subtests) | ✅ PASS |
|
||
| `TestIsRFC1918_NonRFC1918Addresses` (9 subtests) | ✅ PASS |
|
||
| `TestIsRFC1918_NilIP` | ✅ PASS |
|
||
| `TestIsRFC1918_BoundaryAddresses` (5 subtests) | ✅ PASS |
|
||
| `TestIsRFC1918_IPv4MappedAddresses` (5 subtests) | ✅ PASS |
|
||
| `TestSafeDialer_AllowRFC1918_ValidationLoopSkipsRFC1918` | ✅ PASS |
|
||
| `TestSafeDialer_AllowRFC1918_BlocksLinkLocal` | ✅ PASS |
|
||
| `TestSafeDialer_AllowRFC1918_BlocksLoopbackWithoutAllowLocalhost` | ✅ PASS |
|
||
| `TestNewSafeHTTPClient_AllowRFC1918_BlocksSSRFMetadata` | ✅ PASS |
|
||
| `TestNewSafeHTTPClient_WithAllowRFC1918_OptionApplied` | ✅ PASS |
|
||
|
||
**Package result:** `ok github.com/Wikid82/charon/backend/internal/network 0.208s`
|
||
|
||
#### `internal/security` — RFC 1918 tests
|
||
|
||
| Test | Result |
|
||
|---|---|
|
||
| `TestValidateExternalURL_WithAllowRFC1918_Permits10x` | ✅ PASS |
|
||
| `TestValidateExternalURL_WithAllowRFC1918_Permits172_16x` | ✅ PASS |
|
||
| `TestValidateExternalURL_WithAllowRFC1918_Permits192_168x` | ✅ PASS |
|
||
| `TestValidateExternalURL_WithAllowRFC1918_BlocksMetadata` | ✅ PASS |
|
||
| `TestValidateExternalURL_WithAllowRFC1918_BlocksLinkLocal` | ✅ PASS |
|
||
| `TestValidateExternalURL_WithAllowRFC1918_BlocksLoopback` | ✅ PASS |
|
||
| `TestValidateExternalURL_RFC1918BlockedByDefault` | ✅ PASS |
|
||
| `TestValidateExternalURL_WithAllowRFC1918_IPv4MappedIPv6Allowed` | ✅ PASS |
|
||
| `TestValidateExternalURL_WithAllowRFC1918_IPv4MappedMetadataBlocked` | ✅ PASS |
|
||
|
||
**Package result:** `ok github.com/Wikid82/charon/backend/internal/security 0.007s`
|
||
|
||
#### `internal/services` — RFC 1918 / Private IP tests
|
||
|
||
| Test | Result |
|
||
|---|---|
|
||
| `TestCheckMonitor_HTTP_LocalhostSucceedsWithPrivateIPBypass` | ✅ PASS |
|
||
| `TestCheckMonitor_TCP_AcceptsRFC1918Address` | ✅ PASS |
|
||
|
||
**Package result:** `ok github.com/Wikid82/charon/backend/internal/services 4.256s`
|
||
|
||
**Targeted total: 21/21 tests pass.**
|
||
|
||
---
|
||
|
||
### 3. Full Backend Coverage Suite
|
||
|
||
All 30 packages pass. No regressions introduced.
|
||
|
||
| Package | Coverage | Result |
|
||
|---|---|---|
|
||
| `internal/network` | 92.1% | ✅ PASS |
|
||
| `internal/security` | 94.1% | ✅ PASS |
|
||
| `internal/services` | 86.0% | ✅ PASS |
|
||
| `internal/api/handlers` | 86.3% | ✅ PASS |
|
||
| `internal/api/middleware` | 97.2% | ✅ PASS |
|
||
| `internal/caddy` | 96.8% | ✅ PASS |
|
||
| `internal/cerberus` | 93.8% | ✅ PASS |
|
||
| `internal/crowdsec` | 86.2% | ✅ PASS |
|
||
| `internal/models` | 97.5% | ✅ PASS |
|
||
| `internal/server` | 92.0% | ✅ PASS |
|
||
| *(all other packages)* | ≥78% | ✅ PASS |
|
||
|
||
**No packages failed. No regressions.**
|
||
|
||
All three PR-3 packages are above the 85% project threshold.
|
||
|
||
---
|
||
|
||
### 4. Linting
|
||
|
||
Initial run on the three modified packages revealed **one new issue introduced by PR-3** and **17 pre-existing issues** in unrelated service files.
|
||
|
||
#### New issue in PR-3 code
|
||
|
||
| File | Line | Issue | Action |
|
||
|---|---|---|---|
|
||
| `internal/network/safeclient_test.go:1130` | `bodyclose` — response body not closed | ✅ **Fixed** |
|
||
|
||
**Fix applied:** `TestNewSafeHTTPClient_AllowRFC1918_BlocksSSRFMetadata` was updated to assign the response and conditionally close the body:
|
||
|
||
```go
|
||
resp, err := client.Get("http://169.254.169.254/latest/meta-data/")
|
||
if resp != nil {
|
||
_ = resp.Body.Close()
|
||
}
|
||
```
|
||
|
||
A secondary `gosec G104` (unhandled error on `Body.Close()`) was also resolved by the explicit `_ =` assignment.
|
||
|
||
#### Pre-existing issues (not introduced by PR-3)
|
||
|
||
17 issues exist in `internal/services/` files unrelated to PR-3 (`backup_service.go`, `crowdsec_startup.go`, `dns_detection_service.go`, `emergency_token_service.go`, `mail_service.go`, `plugin_loader.go`, `docker_service_test.go`, etc.). These are pre-existing and out of scope for this PR.
|
||
|
||
#### Final lint state — PR-3 packages
|
||
|
||
```
|
||
golangci-lint run ./internal/network/... ./internal/security/...
|
||
0 issues.
|
||
```
|
||
|
||
**Result: ✅ PASS** for all PR-3 code.
|
||
|
||
---
|
||
|
||
### 5. Security Manual Check — Call Site Isolation
|
||
|
||
```
|
||
grep -rn "WithAllowRFC1918" --include="*.go" .
|
||
```
|
||
|
||
**Expected:** `WithAllowRFC1918` used only in its definition files and `uptime_service.go` (2 call sites).
|
||
|
||
**Actual findings:**
|
||
|
||
| File | Context |
|
||
|---|---|
|
||
| `internal/network/safeclient.go:259` | Definition of `WithAllowRFC1918()` (network layer option) |
|
||
| `internal/security/url_validator.go:161` | Definition of `WithAllowRFC1918()` (security layer option) |
|
||
| `internal/services/uptime_service.go:748` | Call site 1 — `security.WithAllowRFC1918()` (URL pre-validation) |
|
||
| `internal/services/uptime_service.go:767` | Call site 2 — `network.WithAllowRFC1918()` (dial-time SSRF guard, mirrors line 748) |
|
||
| `internal/network/safeclient_test.go` | Test uses only |
|
||
| `internal/security/url_validator_test.go` | Test uses only |
|
||
|
||
**Security assessment:**
|
||
|
||
- `WithAllowRFC1918` is **not present** in any notification, webhook, DNS, or other service call paths.
|
||
- The two `uptime_service.go` call sites are correctly paired: the security layer pre-validates the URL hostname, and the network layer enforces the same policy at dial time. This dual-layer approach is the correct defense-in-depth pattern.
|
||
- 169.254.x.x (link-local/cloud metadata), 127.x.x.x (loopback), and IPv4-mapped IPv6 equivalents remain blocked even with `AllowRFC1918=true`. Confirmed by test coverage.
|
||
|
||
**Result: ✅ PASS — Call site isolation confirmed. No scope creep.**
|
||
|
||
---
|
||
|
||
### 6. GORM Security Scan
|
||
|
||
**Skipped** per `testing.instructions.md` gate criteria: PR-3 does not touch `backend/internal/models/**` or any database/GORM query logic. Trigger condition not met.
|
||
|
||
---
|
||
|
||
### 7. Pre-Commit Hooks (lefthook)
|
||
|
||
```
|
||
lefthook run pre-commit
|
||
```
|
||
|
||
| Hook | Result |
|
||
|---|---|
|
||
| `check-yaml` | ✅ PASS |
|
||
| `actionlint` | ✅ PASS |
|
||
| `end-of-file-fixer` | ✅ PASS |
|
||
| `trailing-whitespace` | ✅ PASS |
|
||
| `dockerfile-check` | ✅ PASS |
|
||
| `shellcheck` | ✅ PASS |
|
||
| File-specific hooks (golangci-lint-fast, go-vet, etc.) | Skipped — no staged files (expected behavior) |
|
||
|
||
**Result: ✅ PASS** — All active hooks passed in 7.45s.
|
||
|
||
---
|
||
|
||
## Issues Found and Resolved
|
||
|
||
| # | Severity | File | Issue | Resolution |
|
||
|---|---|---|---|---|
|
||
| 1 | Low | `safeclient_test.go:1130` | `bodyclose`: response body not closed in test | Fixed — added conditional `resp.Body.Close()` |
|
||
| 2 | Low | `safeclient_test.go:1132` | `gosec G104`: unhandled error on `Body.Close()` | Fixed — added `_ =` explicit ignore |
|
||
|
||
No security vulnerabilities. No logic defects. No regressions.
|
||
|
||
---
|
||
|
||
## Final Verdict
|
||
|
||
**✅ APPROVED FOR COMMIT**
|
||
|
||
All audit steps passed. The two minor lint issues introduced by the new test code have been fixed. The implementation correctly scopes `WithAllowRFC1918` to the uptime service only, maintains dual-layer SSRF protection, and does not weaken any other security boundary. All 21 new tests pass. All 30 backend packages pass with zero regressions.
|