feat: add SSL Bump support for HTTPS content inspection #131
Conversation
|
📰 BREAKING: Smoke Copilot is now investigating this pull request. Sources say the story is developing... |
|
💥 WHOOSH! Smoke Claude springs into action on this pull request! [Panel 1 begins...] |
Smoke Test Results (Run #20378121588)PR #111: docs: streamline README for first-time visitors ✅ GitHub MCP Test: PASS Overall: PASS
|
|
Smoke Test Results (Claude Engine) ✅ GitHub MCP: Last 2 merged PRs retrieved
✅ File Writing: Created test file successfully ✅ Bash Tool: File verification passed ❌ Playwright MCP: Page loaded but assets blocked (tunnel connection failures) Overall Status: PARTIAL PASS (3/4 tests passed; Playwright networking issues expected in firewall environment)
|
Test Coverage Report
Coverage ThresholdsThe project has the following coverage thresholds configured:
Coverage report generated by `npm run test:coverage` |
Mossaka
force-pushed
the
copilot/fix-tls-inspection-issue
branch
from
cfd060b to
34b0ec9
Compare
|
@copilot, please address the following issues:
and then because this's a power-user feature, can you add documentation, and make it optional (should not come out of the box and affects perf under default mode)? |
Fixed all three issues in commit af0bc2d:
Also added power-user warnings to documentation and increased SSL certificate cache from 4MB to 16MB per code review feedback. |
Co-authored-by: Mossaka <5447827+Mossaka@users.noreply.github.com>
Co-authored-by: Mossaka <5447827+Mossaka@users.noreply.github.com>
Co-authored-by: Mossaka <5447827+Mossaka@users.noreply.github.com>
Co-authored-by: Mossaka <5447827+Mossaka@users.noreply.github.com>
- Fix SSL database initialization: security_file_certgen requires the
directory to NOT exist, but Docker volume mounts create it. Now
initSslDb() creates the complete DB structure (certs/, index.txt,
size) directly on the host.
- Simplify entrypoint.sh: Since DB is pre-initialized on host, the
entrypoint only needs to fix permissions for the proxy user.
- Fix Squid config ordering: ACL definitions must appear before
ssl_bump rules that reference them. Moved ${aclSection} before
${sslBumpSection} in the config template.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Two issues fixed:
1. URL pattern deny rule was blocking CONNECT requests:
- The deny rule `http_access deny allowed_domains` was evaluated
for CONNECT requests, blocking SSL bump before the URL check
- Added `!CONNECT` to only deny actual HTTP requests after bump
- CONNECT requests now pass through for domain-allowed hosts
2. URL pattern regex escaping was corrupting .* wildcards:
- Input `https://api.github.com/users/.*` was becoming
`^https://api\.github\.com/users/\..*` (incorrect)
- Now preserves .* patterns using placeholder before escaping
- Output is correctly `^https://api\.github\.com/users/.*`
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Mossaka
force-pushed
the
copilot/fix-tls-inspection-issue
branch
from
4d05eb4 to
6bcee3e
Compare
|
📰 BREAKING: Smoke Copilot is now investigating this pull request. Sources say the story is developing... |
|
💥 WHOOSH! Smoke Claude springs into action on this pull request! [Panel 1 begins...] |
Smoke Test ResultsLast 2 Merged PRs:
Tests:
Status: PASS
|
🔐 Security Review: Critical Concerns IdentifiedThis PR introduces SSL Bump (HTTPS interception), which fundamentally changes the security model. While the feature is well-implemented with reasonable safeguards, there are critical security concerns that must be addressed before merging: 🚨 Critical Issue #1: CA Private Key Mounted to Squid ContainerFile: squidVolumes.push(`${sslConfig.caFiles.certPath}:${sslConfig.caFiles.certPath}:ro`);
squidVolumes.push(`${sslConfig.caFiles.keyPath}:${sslConfig.caFiles.keyPath}:ro`);Risk: The CA private key is mounted into the Squid container, which runs as the Why this is critical:
Suggested mitigation:
|
Smoke Test ResultsLast 2 merged PRs:
✅ GitHub MCP - Retrieved PR data Overall: PARTIAL PASS - 3/4 tests successful, Playwright has environment issues unrelated to Claude engine
|
Security improvements based on review feedback:
1. URL Pattern Validation (cli.ts):
- Reject patterns not starting with https://
- Block overly broad patterns like https://* or .*
- Require path component in URL patterns
- Add helpful error messages guiding users to correct usage
2. Container Hardening (docker-manager.ts):
- Drop unnecessary capabilities from Squid container:
NET_RAW, SYS_ADMIN, SYS_PTRACE, SYS_MODULE, MKNOD,
AUDIT_WRITE, SETFCAP
- Reduces attack surface if Squid process is compromised
3. Enhanced Security Documentation (docs/ssl-bump.md):
- Added prominent security warnings about threat model change
- Documented when NOT to use SSL Bump:
- Multi-tenant environments
- Untrusted workloads
- Multi-user systems
- Detailed CA private key exposure risks
- Listed all implemented mitigations
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
📰 BREAKING: Smoke Copilot is now investigating this pull request. Sources say the story is developing... |
|
💥 WHOOSH! Smoke Claude springs into action on this pull request! [Panel 1 begins...] |
Security Review: Critical Issues FoundI've completed a security review of this SSL Bump implementation and identified several critical security concerns that must be addressed: 🔴 CRITICAL: CA Private Key Stored in World-Readable /tmpFile: Issue: The CA private key is stored in
Code snippet: const sslDir = path.join(workDir, 'ssl');
if (!fs.existsSync(sslDir)) {
fs.mkdirSync(sslDir, { recursive: true, mode: 0o700 });
}Risk: An attacker who obtains the CA private key can impersonate any HTTPS site for the certificate's validity period (1 day). Recommendation:
🔴 CRITICAL: Missing Access Control Between Domain and URL PatternsFile: Issue: When URL patterns are used with SSL Bump, the access control logic has a flaw. The code generates: However, this appears AFTER the SSL Bump section (line 154), and I don't see where the normal domain Location: The interaction between SSL Bump URL access rules and the main access rules section needs verification. Risk: URL path filtering could be completely bypassed if domain-level rules take precedence. Recommendation:
|
🚀 Smoke Test ResultsLast 2 Merged PRs:
Test Results:
Overall Status: PASS ✅
|
|
Smoke Test Results
Status: FAIL
|
There was a problem hiding this comment.
Pull request overview
This pull request adds SSL Bump support to enable HTTPS content inspection and URL path filtering. The feature generates per-session CA certificates to intercept HTTPS traffic, allowing the firewall to filter based on URL paths rather than just domain names via SNI.
Key Changes:
- Implements SSL Bump with per-session CA certificate generation (1-day validity)
- Adds CLI flags
--ssl-bumpand--allow-urlsfor URL path filtering - Integrates SSL certificate management into Docker containers with automatic CA injection
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/ssl-bump.ts |
New module providing CA generation, SSL DB initialization, and URL pattern parsing utilities |
src/ssl-bump.test.ts |
Unit tests for URL pattern parsing with wildcard and regex escaping |
src/types.ts |
Adds SSL Bump configuration types for WrapperConfig and SquidConfig |
src/squid-config.ts |
Generates SSL Bump configuration section with TLS interception settings |
src/squid-config.test.ts |
Tests for SSL Bump configuration generation |
src/docker-manager.ts |
Handles SSL certificate mounting, CA generation, and URL pattern transformation |
src/cli.ts |
Adds --ssl-bump and --allow-urls flags with security validation |
containers/squid/Dockerfile |
Installs OpenSSL and squid-openssl for SSL Bump support |
containers/squid/entrypoint.sh |
Fixes SSL database permissions for Squid proxy user |
containers/agent/entrypoint.sh |
Injects CA certificate into container trust store |
docs/ssl-bump.md |
Comprehensive SSL Bump documentation with security warnings |
docs/usage.md |
Usage documentation for SSL Bump feature |
docs/troubleshooting.md |
SSL Bump troubleshooting section |
docs-site/src/content/docs/reference/ssl-bump.md |
Website documentation for SSL Bump |
docs-site/src/content/docs/reference/security-architecture.md |
Security architecture updates for SSL Bump |
docs-site/src/content/docs/reference/cli-reference.md |
CLI reference updates |
docs-site/src/content/docs/index.md |
Homepage update mentioning URL path filtering |
README.md |
Adds link to SSL Bump documentation |
Comments suppressed due to low confidence (1)
containers/squid/Dockerfile:22
- The comment states port 3129 is for "HTTPS with SSL Bump", but this is incorrect. With SSL Bump, both HTTP and HTTPS are handled on port 3128 using the ssl-bump option. The configuration does not use or expose port 3129 anywhere.
# Expose Squid port (3128 for HTTP, 3129 for HTTPS with SSL Bump)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Expose Squid port (3128 for HTTP, 3129 for HTTPS with SSL Bump) | ||
| EXPOSE 3128 | ||
| EXPOSE 3129 |
There was a problem hiding this comment.
Port 3129 is exposed in the Dockerfile comment and EXPOSE directive but is never used or configured in the Squid configuration. The SSL Bump configuration uses port 3128 with the ssl-bump option, not a separate port 3129. This EXPOSE directive should be removed as it's misleading.
This issue also appears in the following locations of the same file:
- line 22
| # Expose Squid port (3128 for HTTP, 3129 for HTTPS with SSL Bump) | |
| EXPOSE 3128 | |
| EXPOSE 3129 | |
| # Expose Squid port (3128 for HTTP and SSL Bump) | |
| EXPOSE 3128 |
| # Step 3 (SslBump3): Bump or splice based on policy | ||
| acl step1 at_step SslBump1 | ||
| acl step2 at_step SslBump2 | ||
| acl step3 at_step SslBump3 |
There was a problem hiding this comment.
The ACL 'step3' is defined at line 141 but is never referenced in any ssl_bump directives. Squid ACL definitions that are not used can cause confusion and should be removed. The SSL Bump configuration only uses step1 and step2 in the peek and stare directives.
| acl step3 at_step SslBump3 |
| export async function generateSessionCa(config: SslBumpConfig): Promise<CaFiles> { | ||
| const { workDir, commonName = 'AWF Session CA', validityDays = 1 } = config; | ||
|
|
||
| // Create ssl directory in workDir | ||
| const sslDir = path.join(workDir, 'ssl'); | ||
| if (!fs.existsSync(sslDir)) { | ||
| fs.mkdirSync(sslDir, { recursive: true, mode: 0o700 }); | ||
| } | ||
|
|
||
| const certPath = path.join(sslDir, 'ca-cert.pem'); | ||
| const keyPath = path.join(sslDir, 'ca-key.pem'); | ||
| const derPath = path.join(sslDir, 'ca-cert.der'); | ||
|
|
||
| logger.debug(`Generating SSL Bump CA certificate in ${sslDir}`); | ||
|
|
||
| try { | ||
| // Generate RSA private key and self-signed certificate in one command | ||
| // Using -batch to avoid interactive prompts | ||
| await execa('openssl', [ | ||
| 'req', | ||
| '-new', | ||
| '-newkey', 'rsa:2048', | ||
| '-days', validityDays.toString(), | ||
| '-nodes', // No password on private key | ||
| '-x509', | ||
| '-subj', `/CN=${commonName}`, | ||
| '-keyout', keyPath, | ||
| '-out', certPath, | ||
| '-batch', | ||
| ]); | ||
|
|
||
| // Set restrictive permissions on private key | ||
| fs.chmodSync(keyPath, 0o600); | ||
| fs.chmodSync(certPath, 0o644); | ||
|
|
||
| logger.debug(`CA certificate generated: ${certPath}`); | ||
| logger.debug(`CA private key generated: ${keyPath}`); | ||
|
|
||
| // Generate DER format for easier import into trust stores | ||
| await execa('openssl', [ | ||
| 'x509', | ||
| '-in', certPath, | ||
| '-outform', 'DER', | ||
| '-out', derPath, | ||
| ]); | ||
|
|
||
| fs.chmodSync(derPath, 0o644); | ||
| logger.debug(`CA certificate (DER) generated: ${derPath}`); | ||
|
|
||
| return { certPath, keyPath, derPath }; | ||
| } catch (error) { | ||
| const message = error instanceof Error ? error.message : String(error); | ||
| throw new Error(`Failed to generate SSL Bump CA: ${message}`); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Initializes Squid's SSL certificate database | ||
| * | ||
| * Squid requires a certificate database to store dynamically generated | ||
| * certificates for SSL Bump mode. The database structure expected by Squid is: | ||
| * - ssl_db/certs/ - Directory for storing generated certificates | ||
| * - ssl_db/index.txt - Index file for certificate lookups | ||
| * - ssl_db/size - File tracking current database size | ||
| * | ||
| * NOTE: We create this structure on the host because security_file_certgen | ||
| * (Squid's DB initialization tool) requires the directory to NOT exist when | ||
| * it runs. Since Docker volume mounts create the directory, we need to | ||
| * pre-populate the structure ourselves. | ||
| * | ||
| * @param workDir - Working directory | ||
| * @returns Path to the SSL database directory | ||
| */ | ||
| export async function initSslDb(workDir: string): Promise<string> { | ||
| const sslDbPath = path.join(workDir, 'ssl_db'); | ||
| const certsPath = path.join(sslDbPath, 'certs'); | ||
| const indexPath = path.join(sslDbPath, 'index.txt'); | ||
| const sizePath = path.join(sslDbPath, 'size'); | ||
|
|
||
| // Create the database structure | ||
| if (!fs.existsSync(sslDbPath)) { | ||
| fs.mkdirSync(sslDbPath, { recursive: true, mode: 0o700 }); | ||
| } | ||
|
|
||
| // Create certs subdirectory | ||
| if (!fs.existsSync(certsPath)) { | ||
| fs.mkdirSync(certsPath, { mode: 0o700 }); | ||
| } | ||
|
|
||
| // Create index.txt (empty file for certificate index) | ||
| if (!fs.existsSync(indexPath)) { | ||
| fs.writeFileSync(indexPath, '', { mode: 0o600 }); | ||
| } | ||
|
|
||
| // Create size file (tracks current DB size, starts at 0) | ||
| if (!fs.existsSync(sizePath)) { | ||
| fs.writeFileSync(sizePath, '0\n', { mode: 0o600 }); | ||
| } | ||
|
|
||
| logger.debug(`SSL certificate database initialized at: ${sslDbPath}`); | ||
| return sslDbPath; | ||
| } |
There was a problem hiding this comment.
The core SSL Bump functions generateSessionCa and initSslDb lack test coverage. These functions handle critical security functionality including CA certificate generation and SSL database initialization. Consider adding tests to verify certificate generation, file permissions, error handling, and database structure creation.
| * Validates that OpenSSL is available | ||
| * | ||
| * @returns true if OpenSSL is available, false otherwise | ||
| */ | ||
| export async function isOpenSslAvailable(): Promise<boolean> { | ||
| try { | ||
| await execa('openssl', ['version']); | ||
| return true; | ||
| } catch { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
The function isOpenSslAvailable is exported but never used anywhere in the codebase. Consider either using it to validate OpenSSL availability before calling generateSessionCa, or removing it if it's not needed.
| * Validates that OpenSSL is available | |
| * | |
| * @returns true if OpenSSL is available, false otherwise | |
| */ | |
| export async function isOpenSslAvailable(): Promise<boolean> { | |
| try { | |
| await execa('openssl', ['version']); | |
| return true; | |
| } catch { | |
| return false; | |
| } | |
| } | |
| /** |
src/ssl-bump.ts- CA generation utilitiessrc/types.ts- Add SSL Bump configuration typessrc/squid-config.ts- Add SSL Bump configurationsrc/docker-manager.ts- Handle CA certificate mountingsrc/cli.ts- Add CLI flags--ssl-bumpand--allow-urlscontainers/squid/Dockerfile- Add OpenSSL toolscontainers/agent/entrypoint.sh- Add CA certificate injectionparseUrlPatterns()to transform user URLs to regex patternshttp_port 3128 ssl-bumpfor explicit proxy)Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.