Fix + Optimizing TLS detection via half-handshake#1575
Conversation
WalkthroughAdds an optional, environment-controlled TLS detection step to TCP connect scans: when ENABLE_TLS_DETECTION is true, the scanner invokes netutil.DetectTLS after connection and records the result in the port's TLS field. Also updates go module dependency versions. Changes
Sequence Diagram(s)sequenceDiagram
participant Scanner
participant Connector
participant Target
participant TLSDetector
Scanner->>Connector: ConnectPort(host, port, protocol)
Connector->>Target: TCP connect (establish socket)
Target-->>Connector: TCP connection established
alt ENABLE_TLS_DETECTION = true and protocol == tcp
Connector->>TLSDetector: netutil.DetectTLS(conn)
TLSDetector->>Target: send ClientHello (non-blocking/minimal)
alt TLS supported
Target-->>TLSDetector: ServerHello/response
TLSDetector->>Connector: return true
Connector->>Scanner: Port.TLS = true
else TLS not supported
Target-->>TLSDetector: reset/no TLS response
TLSDetector->>Connector: return false
Connector->>Scanner: Port.TLS = false
end
else TLS detection disabled or non-TCP
Connector->>Scanner: Port.TLS = false (unchanged)
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
🔇 Additional comments (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/scan/scan.go(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/scan/scan.go (1)
pkg/protocol/protocol.go (1)
TCP(10-10)
| if EnableTLSDetection { | ||
| //nolint | ||
| p.TLS = detectTLS(conn, host, timeout) | ||
| } |
There was a problem hiding this comment.
Don’t mutate the shared *port.Port when tagging TLS.
Scanner.Ports stores canonical *port.Port values that are reused for every host (see Line 86 and the reuse in the enqueue loops). Multiple goroutines run ConnectPort concurrently, so writing p.TLS = … here introduces a data race. Even worse, ScanResults.AddPort later keeps the same pointer, so a TLS value computed for host A will be overwritten by host B and host A’s result flips retroactively. Please keep the TLS bit host-scoped instead (e.g., copy the struct before tagging or pass the bool alongside the result) and avoid mutating the shared definition.
🤖 Prompt for AI Agents
In pkg/scan/scan.go around lines 585 to 588, the code mutates the shared
*port.Port by setting p.TLS = detectTLS(...), which causes data races and
incorrect cross-host TLS tagging because Scanner.Ports holds canonical pointers
reused across goroutines; instead, do not modify the shared pointer — either
make a local copy of the port struct (value copy) and set the TLS field on that
copy before adding it to host-scoped results, or change the result path to carry
the TLS bool alongside the existing *port.Port (e.g., AddPort(port *port.Port,
tls bool) or enqueue a portValue struct), ensure DetectTLS is called per-host
and its bool kept host-scoped, and update ScanResults.AddPort and any callers to
accept the copy or the additional bool so the shared Scanner.Ports entries
remain immutable.
|
and I think it makes sense to move this into |
Closes #1572
Summary by CodeRabbit
New Features
Chores