feat(probe): add transport probing and STARTTLS support#131
feat(probe): add transport probing and STARTTLS support#131danielewood merged 45 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds raw SSH transport probing, STARTTLS support for SMTP/IMAP/POP3/LDAP in ConnectTLS and ScanCipherSuites, a new SecurityPolicy type with FIPS 140-2/140-3 heuristics, and non-TLS service detection improvements. It also renames probe implementation files from tls13probe.go/legacyprobe.go/quicprobe.go to the probe_*.go convention and adds comprehensive test coverage.
Changes:
- New
probe_ssh.go+ProbeSSHAPI: reads SSH banner, parses KEXINIT, rates algorithms, supports FIPS heuristics. - New
connect.goadditions: STARTTLS negotiation (SMTP/IMAP/POP3/LDAP), non-TLS detection,SecurityPolicyintegration forConnectTLS/ScanCipherSuites, anddialProbeConnSTARTTLS plumbing. - New
security_policy.go,connect_policy.go:SecurityPolicytype and FIPS allowlist functions; newcmd/certkit/probesubcommand grouping.
Reviewed changes
Copilot reviewed 18 out of 22 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| probe_ssh.go | Core SSH probing: banner, KEXINIT parsing, diagnostics, FIPS policy |
| probe_ssh_test.go | Full unit test coverage for SSH probe, format, diagnostics, FIPS policy |
| probe_tls13.go | Removes net.Dialer in favor of dialProbeConn; adds startTLS to cipherProbeInput |
| probe_legacy.go | Same dialProbeConn refactor as TLS 1.3 prober |
| probe_quic.go | Comment update for renamed probe file |
| probe_protocol_helpers.go | Extracted helper: length encoding utilities (new file) |
| security_policy.go | New SecurityPolicy type + DisplayName/Enabled methods |
| connect.go | STARTTLS negotiation, non-TLS error inference, prefixCapturingConn, bufferedPrefixConn, dialProbeConn |
| connect_policy.go | FIPS allowlist functions for TLS ciphers, protocols, certificate keys/signatures |
| connect_test.go | New STARTTLS, policy, and non-TLS detection tests |
| cmd/certkit/probe.go | New probe parent command |
| cmd/certkit/probe_ssh.go | CLI subcommand for SSH probing |
| cmd/certkit/policy.go | selectedPolicy helper shared by connect and probe |
| cmd/certkit/connect.go | FIPS flag plumbing for connect command |
| README.md | Documents new --fips-140-2 / --fips-140-3 flags |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8f548d1cca
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4e44313179
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0727948864
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d1956ccda2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
💡 Codex Review
Lines 533 to 536 in 897dec4
Omit SNI when probing IP endpoints
appendSNIExtension currently emits a server_name extension for any non-empty serverName, and ScanCipherSuites defaults that value to input.Host, so probing an IP target sends an IP literal as SNI. Many TLS frontends reject that with unrecognized_name, which makes raw TLS 1.3/QUIC probe paths look unsupported even when the endpoint actually supports those ciphers/groups. Please skip SNI when the host is an IP literal so IP-based scans don't produce false negatives.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 32 out of 37 changed files in this pull request and generated 8 comments.
Comments suppressed due to low confidence (1)
probe_tls13.go:488
- In
probe_tls13.go, theisIPLiteralServerNamefunction handles zone IDs for IPv6 addresses (e.g.,fe80::1%en0) by stripping the%suffix before callingnet.ParseIP. However, the function is called inappendSNIExtensionwhere the serverName originates fromcipherProbeInput.addr. An IPv6 address obtained fromnet.SplitHostPortwill already have the zone stripped from the host component (Go'snet.SplitHostPortstrips zone IDs). The only caller that might pass an IPv6 address with a zone ID is a test or caller constructingserverNamedirectly. The implementation is correct for the general case, but the zone-ID stripping path is exercised only by theipv6 zonetest case inTestAppendSNIExtension_OmitsIPLiterals. This is fine as-is, but worth noting.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 58e8295634
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if len(prefix) == 0 { | ||
| // LDAP is the one STARTTLS protocol we intentionally probe without a | ||
| // plaintext banner because anonymous LDAP on 389 stays silent until the | ||
| // client sends the extended operation. This retry stays under | ||
| // startTLSCtx, so silent non-LDAP services still fail within the same | ||
| // bounded fallback budget instead of hanging indefinitely. | ||
| result, startTLSErr := connectViaStartTLS(startTLSCtx, ctx, connectViaStartTLSInput{ | ||
| connectInput: input, | ||
| addr: addr, | ||
| serverName: serverName, | ||
| protocol: startTLSProtocolLDAP, | ||
| }) | ||
| if startTLSErr == nil { | ||
| return result, nil | ||
| } | ||
| slog.Debug("ldap starttls attempt failed, falling back to tls handshake error", "addr", addr, "error", startTLSErr) | ||
| } |
There was a problem hiding this comment.
In connect.go, the ConnectTLS handshake-failure branch (lines 435–515) that handles non-TLS services tries the STARTTLS fallbacks after the prefix is captured from sniffConn. When handshakeErr is a tls.RecordHeaderError (i.e., the server responded with non-TLS bytes), the sniffConn.prefix() should contain those bytes. However, when handshakeErr is a plain network error (timeout, connection reset), prefix may be empty and all STARTTLS fallbacks including LDAP will still be attempted. In the LDAP case (len(prefix) == 0), this means a connection timeout or reset will cause an unnecessary LDAP StartTLS probe attempt before returning the original handshake error. This wastes time. The LDAP probe should only be attempted when handshakeErr is a tls.RecordHeaderError (i.e., the server did respond with non-TLS data) or when there's some positive signal from the prefix—not on pure network errors.
| fmt.Println(string(data)) | ||
| return nil |
There was a problem hiding this comment.
In cmd/certkit/probe_ssh.go line 82, fmt.Println(string(data)) writes JSON output to stdout via fmt.Println. The codebase guideline CLI-1 states stdout is for data and stderr for diagnostics. This is correct. However, fmt.Println adds a trailing newline after json.MarshalIndent's output, which already ends without a newline. This results in a double trailing newline in the JSON output. Other JSON output paths in the codebase (e.g., cmd/certkit/connect.go) should be checked for consistency, but the fmt.Println here produces an extra blank line that may break downstream consumers parsing the JSON stream. Consider using fmt.Print or os.Stdout.Write instead.
Summary
Validation