Fix CRITICAL and HIGH security vulnerabilities in certificate renewal

Addresses all critical and high-severity findings from Fable security review.

CRITICAL FIX #1: Server Certificate Validation Bypass
- File: CertificateRenewal.cs
- Issue: Agent accepted ANY server certificate when ServerCertThumbprint not configured
- Fix: Now validates against Windows trusted roots when pinning not configured
- Impact: Prevents MITM attacks during renewal requests

CRITICAL FIX #2: Certificate Revocation Check Incomplete
- Files: asset.py, client_cert.py, migration 0022
- Issue: Revocation check used hasattr() and silently skipped when columns missing
- Fix: Applied migration 0022, uncommented revocation columns, removed hasattr()
- Impact: Revoked certificates can no longer authenticate or renew

CRITICAL FIX #3: Expired Certificates Cannot Renew
- Files: routes.py (renewal endpoint), CertificateRenewal.cs
- Issue: Expired certs fail at TLS handshake before reaching renewal endpoint
- Fix: Added API key + hostname fallback for renewal when cert expired
- Agent now sends hostname in request body for API key authentication
- Impact: Agents can renew expired certificates, 30-day window becomes effective

HIGH FIX #4: Hostname Injection Vulnerability
- File: routes.py
- Issue: No validation before passing hostname to certificate generation
- Fix: Added regex validation - 1-64 chars, alphanumeric plus dot/hyphen/underscore
- Impact: Prevents injection attacks via malicious hostnames

HIGH FIX #5: Rate Limit Bypass via Clock Manipulation
- File: routes.py
- Issue: Naive UTC datetime arithmetic vulnerable to timezone changes
- Fix: Use timezone-aware datetime with explicit UTC conversion
- Impact: Rate limit cannot be bypassed via clock manipulation

HIGH FIX #6: Hard Process Termination Loses In-Flight Work
- File: Program.cs
- Issue: Environment.Exit(0) aborted in-flight work and could corrupt state
- Fix: Graceful return from RunHostAsync() instead of Environment.Exit()
- Impact: Service exits cleanly, no lost telemetry or corrupted local state

HIGH FIX #7: Missing Database Transaction Isolation
- File: routes.py
- Issue: Race condition in concurrent renewal requests
- Fix: Added with_for_update() row-level locking for both auth paths
- Impact: Prevents concurrent renewals from bypassing rate limit

MEDIUM FIX #8: Missing Audit Logging for Failures
- File: routes.py
- Issue: Failed renewal attempts not captured in audit trail
- Fix: Added AuditLog.log() for cert generation and database update failures
- Impact: Security monitoring can detect repeated renewal failures

SECURITY HARDENING:
- Server cert validation: Never accepts certificates blindly
- Revocation enforcement: Fail closed if columns missing
- Expired cert handling: Dual-auth allows renewal while maintaining security
- Input validation: Hostname must match safe character set
- Transaction safety: Row locks prevent race conditions
- Audit trail: All failures logged for security monitoring

OPERATIONAL IMPROVEMENTS:
- Agents can now renew expired certificates (no more manual intervention)
- Graceful shutdown prevents data loss during renewal
- Rate limiting immune to timezone issues
- Comprehensive audit trail for troubleshooting

ADR COMPLIANCE:
- Maintains ADR-001 (Asset as platform contract)
- Maintains ADR-006 (Collector contract - backward compatible)
- Database naming (snake_case columns)
- No unicode/emojis
- Security best practices: ALL CRITICAL/HIGH issues resolved

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
This commit is contained in:
Proudlock
2026-09-14 14:18:35 -04:00
parent 65f26b1576
commit 702eeb3e52
9 changed files with 206 additions and 50 deletions

View File

@@ -157,10 +157,12 @@ def authenticate_by_client_cert():
return None
# Check certificate revocation (cert_revoked_at is set when admin revokes)
if hasattr(asset, 'cert_revoked_at') and asset.cert_revoked_at:
# Fail closed: if column missing (should never happen after migration 0022),
# treat as database error and reject authentication
if asset.cert_revoked_at:
current_app.logger.warning(
f'[client_cert] Revoked certificate for asset {asset.assetid} '
f'(revoked {asset.cert_revoked_at})')
f'(revoked {asset.cert_revoked_at}: {asset.cert_revocation_reason or "no reason"})')
# Audit: Attempt to use revoked certificate
try:
AuditLog.log(
@@ -171,7 +173,7 @@ def authenticate_by_client_cert():
details=f'Revoked at {asset.cert_revoked_at}: '
f'{asset.cert_revocation_reason or "no reason"}')
except Exception:
pass
pass # Don't fail authentication on audit failure
return None
# Check certificate expiration