chore: add tls log #357
chore: add tls log #357
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds comprehensive TLS logging and error handling improvements to the HTTP server. The main purpose is to enhance debugging capabilities for TLS connections by adding detailed logging for handshake failures and SNI requests.
- Enhanced TLS handshake error handling with detailed categorization and logging
- Added TLS key logging support for debugging purposes
- Introduced a conditional TLS fallback feature for specific environments
Reviewed Changes
Copilot reviewed 2 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| rustfs/src/server/http.rs | Enhanced TLS setup with key logging, improved error handling with detailed categorization, and added conditional TLS-to-HTTP fallback |
| rustfs/Cargo.toml | Added new feature flags for TLS fallback functionality and removed unused dependencies |
| server_config.alpn_protocols = vec![b"h2".to_vec(), b"http/1.1".to_vec(), b"http/1.0".to_vec()]; | ||
|
|
||
| // Log SNI requests | ||
| server_config.key_log = Arc::new(rustls::KeyLogFile::new()); |
There was a problem hiding this comment.
TLS key logging should not be enabled in production as it can expose sensitive cryptographic material. Consider making this conditional on a debug feature or environment variable.
Copilot uses AI. Check for mistakes.
| // Log SNI requests | ||
| server_config.key_log = Arc::new(rustls::KeyLogFile::new()); |
There was a problem hiding this comment.
TLS key logging should not be enabled in production as it can expose sensitive cryptographic material. Consider making this conditional on a debug feature or environment variable.
| // Log SNI requests | |
| server_config.key_log = Arc::new(rustls::KeyLogFile::new()); | |
| // Enable TLS key logging only if RUSTFS_TLS_KEYLOG env var is set | |
| if env::var("RUSTFS_TLS_KEYLOG").map(|v| v == "1").unwrap_or(false) { | |
| server_config.key_log = Arc::new(rustls::KeyLogFile::new()); | |
| } |
Copilot uses AI. Check for mistakes.
| // Since the original connection has been consumed, a new connection needs to be accepted | ||
| match TcpStream::connect(peer_addr.clone()).await { | ||
| Ok(new_socket) => { | ||
| debug!("Successfully reconnected to client for HTTP fallback"); | ||
| let stream = TokioIo::new(new_socket); | ||
| let conn = http_server.serve_connection(stream, hybrid_service); | ||
| if let Err(err) = graceful.watch(conn).await { | ||
| handle_connection_error(&*err); | ||
| } | ||
| return; | ||
| } | ||
| Err(e) => { | ||
| error!("Failed to reconnect for HTTP fallback: {}", e); | ||
| } | ||
| } |
There was a problem hiding this comment.
Attempting to connect back to the client's address will likely fail as clients typically don't listen for incoming connections. This fallback mechanism appears fundamentally flawed.
| // Since the original connection has been consumed, a new connection needs to be accepted | |
| match TcpStream::connect(peer_addr.clone()).await { | |
| Ok(new_socket) => { | |
| debug!("Successfully reconnected to client for HTTP fallback"); | |
| let stream = TokioIo::new(new_socket); | |
| let conn = http_server.serve_connection(stream, hybrid_service); | |
| if let Err(err) = graceful.watch(conn).await { | |
| handle_connection_error(&*err); | |
| } | |
| return; | |
| } | |
| Err(e) => { | |
| error!("Failed to reconnect for HTTP fallback: {}", e); | |
| } | |
| } | |
| // Fallback to HTTP is not possible by reconnecting to the client's address, | |
| // as clients do not listen for incoming connections. Log and terminate. | |
| error!("TLS fallback to HTTP is not supported: cannot connect to client address {}", peer_addr); |
Copilot uses AI. Check for mistakes.
| // Detailed analysis of the reasons why the TLS handshake fails | ||
| let err_str = err.to_string(); | ||
| if err_str.contains("unexpected EOF") || err_str.contains("handshake eof") { | ||
| info!( peer_addr = %peer_addr,"TLS handshake failed with EOF, attempting fallback to HTTP: {}", err); |
There was a problem hiding this comment.
Missing space after comma in the log macro. Should be 'peer_addr = %peer_addr, "TLS handshake failed...'
| info!( peer_addr = %peer_addr,"TLS handshake failed with EOF, attempting fallback to HTTP: {}", err); | |
| info!( peer_addr = %peer_addr, "TLS handshake failed with EOF, attempting fallback to HTTP: {}", err); |
Copilot uses AI. Check for mistakes.
| "TLS handshake failure details" | ||
| ); | ||
|
|
||
| return; // End the mission |
There was a problem hiding this comment.
The comment 'End the mission' should be 'End the task' to match the context and maintain consistency with line 433.
| return; // End the mission | |
| return; // End the task |
Copilot uses AI. Check for mistakes.
Type of Change
Related Issues
Summary of Changes
Checklist
cargo fmt --allcargo clippy --all-targets --all-features -- -D warningscargo check --all-targetsImpact
Additional Notes
Thank you for your contribution! Please ensure your PR follows the community standards (CODE_OF_CONDUCT.md) and sign the CLA if this is your first contribution.