feat(ingress): skip verifying system API signatures

[mempirate]

Description

Instead of verifying signatures on the system API, it just extracts the signing peer address from the header.

[mempirate]

@claude review sir

[claude]

Claude finished @mempirate's task —— View job

---

Code Review Complete ✓

Tasks

• Read the modified file and understand changes
• Analyze security implications
• Review code quality and best practices
• Check for test coverage
• Provide detailed review feedback

---

Summary

This PR removes cryptographic signature verification from the system API handler, replacing it with simple address extraction from the header. While the code changes are clean and well-implemented, this introduces a critical security vulnerability that needs to be addressed before merging.

🔴 Critical Issues

1. Authentication Bypass Vulnerability (src/ingress/mod.rs:280-282)

The change from maybe_verify_signature() to maybe_extract_signer() removes cryptographic proof of identity. This allows any client with HTTPS access to impersonate any peer address by simply setting the header.

Impact: An attacker can:

• Send malicious bundles/transactions claiming to be from trusted peers
• Bypass rate limiting and priority mechanisms tied to specific entities
• Potentially disrupt the orderflow routing system

The comment states: "we can assume the channel (HTTPs session) is authenticated"

• ⚠️ Standard HTTPS only authenticates the server to the client, NOT the client to the server
• If this relies on mutual TLS (mTLS), this assumption must be validated and documented
• If behind an authenticated proxy, this should be explicitly configured and validated

⚠️ Major Concerns

2. Missing Test Coverage

• No tests exist for maybe_extract_signer() function
• No tests validate the system_handler's authentication behavior
• The existing test file (tests/ingress.rs) only tests the user handler flow
• Risk: Security-critical code changes without test coverage

3. Insufficient Documentation

• The security model and assumptions are not documented
• No explanation of what "authenticated channel" means in practice
• The maybe_extract_signer() function needs a security warning in its docs

✅ Positive Aspects

1. Clean Implementation: The code changes are minimal and well-structured
2. Proper Error Handling: Error messages updated appropriately
3. Good Documentation: The new function has inline documentation (though needs security warnings)

📋 Recommendations

High Priority:

1. Add Authentication Validation:

   // Option A: Validate mTLS client certificate
   if let Some(cert) = request.peer_cert() {
       let cert_address = extract_address_from_cert(cert)?;
       ensure!(cert_address == claimed_address, "Certificate mismatch");
   }
   
   // Option B: Add feature flag
   if config.system_api_requires_signature_verification {
       maybe_verify_signature(&headers, &body, USE_LEGACY_SIGNATURE)
   } else {
       maybe_extract_signer(&headers)
   }

2. Add Comprehensive Tests:

   • Test that system_handler rejects requests with invalid/missing peer addresses
   • Test that system_handler correctly identifies known peers
   • Test behavior with unknown peer addresses
   • Add integration tests for the authentication flow

3. Document Security Model:

   • Create/update documentation explaining the trust model
   • Document deployment requirements (mTLS configuration, network topology, etc.)
   • Add security warning to maybe_extract_signer() function

Medium Priority:

4. Consider Feature Flag: Instead of removing signature verification entirely, make it configurable
5. Add Metrics: Track authentication failures on system API for monitoring
6. Audit Peer Registry: Ensure ingress.forwarders.find_peer() has proper access controls

🤔 Questions for Author

1. What mechanism ensures the HTTPS session is authenticated? Is mTLS configured?
2. Is this deployment behind a trusted proxy that handles authentication?
3. Have you considered the attack surface if the peer registry is compromised?
4. Why was signature verification removed rather than making it optional?

Verdict

❌ I recommend not merging this PR until the security concerns are addressed. The removal of cryptographic verification needs either:

• Strong compensating controls (e.g., validated mTLS)
• Clear documentation of the security assumptions
• Test coverage proving the security model works as intended

The code quality is good, but the security implications are too significant to merge without additional safeguards.

---

Review Focus Areas: Security ⚠️ | Code Quality ✓ | Testing ⚠️ | Documentation ⚠️ | Performance ✓

---

[Job Run](https://github.com/flashbots/buildernet-orderflow-proxy-v2/actions/runs/${{ github.run_id }}) | Branch

[claude]

🔴 Critical Security Issue: Bypassing Signature Verification

This change removes cryptographic signature verification for system API requests, replacing it with simple address extraction. This creates a significant security vulnerability:

Issues:

1. Authentication Bypass: An attacker can claim to be any address by simply setting the header without proving ownership of the private key
2. Trust Assumption: The comment states "we can assume the channel (HTTPs session) is authenticated" but:
   • HTTPS only authenticates the server, not the client (unless using mTLS)
   • If this relies on mTLS, that should be explicitly documented and validated
   • The header can be set arbitrarily by any client with HTTPS access

Attack Scenario:

// Attacker can impersonate any peer by setting:
// X-Flashbots-Signature: <any_valid_peer_address>:fake_signature
// Since we only extract the address and don't verify the signature

Recommendations:

1. If mTLS is used, verify the client certificate's subject matches the claimed address
2. If this is intentional for a specific deployment (e.g., behind authenticated proxy), add explicit configuration flag
3. Document the security model and assumptions clearly
4. Consider adding a feature flag to enable/disable signature verification rather than removing it entirely

[claude]

⚠️ Documentation Needed

While this helper function is well-documented, the security implications of using it instead of maybe_verify_signature should be clearly stated:

Suggested change

	pub fn maybe_extract_signer(headers: &HeaderMap) -> Option<Address> {
	/// Parse [`FLASHBOTS_SIGNATURE_HEADER`] header and extract the signer of the request (without
	/// verifying the signature). For signature verification, use [`maybe_verify_signature`].
	///
	/// # Security Warning
	///
	/// This function does NOT verify the signature and should only be used in contexts where
	/// authentication is handled by other means (e.g., mTLS). Using this function without
	/// additional authentication allows any client to impersonate any address.
	pub fn maybe_extract_signer(headers: &HeaderMap) -> Option<Address> {

[mempirate]

Converted to draft because even though with TLS, the client can verify the server, it doesn't work the other way around by default. So we need the signature verification to authenticate the client to the server, until we maybe do something like mTLS.