feat: add key_exchange field for negotiated TLS key exchange group#948
Open
usernametooshort wants to merge 1 commit intoprojectdiscovery:mainfrom
Open
feat: add key_exchange field for negotiated TLS key exchange group#948usernametooshort wants to merge 1 commit intoprojectdiscovery:mainfrom
usernametooshort wants to merge 1 commit intoprojectdiscovery:mainfrom
Conversation
Neo - PR Security ReviewCaution Review could not be completed Review could not be completed. Please retry with Suggestion: Try again with Comment |
WalkthroughAdded a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/runner/banner.go`:
- Line 34: The probeSpecified predicate in banner.go is missing several probe
flags (e.g., RespOnly, Serial, SAN, ProbeStatus), so update the expression that
builds probeSpecified (the line using r.options.SO || r.options.TLSVersion ||
...) to also OR in r.options.RespOnly, r.options.Serial, r.options.SAN, and
r.options.ProbeStatus (and any other probe boolean flags introduced on the
options struct) so combinations like --resp-only/--serial/--san/--probe-status
are treated as probes and trigger the same validation logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 22fddda5-3676-4ff3-a4f6-6b066afc4ac2
📒 Files selected for processing (5)
cmd/tlsx/main.gointernal/runner/banner.gopkg/output/output.gopkg/tlsx/clients/clients.gopkg/tlsx/tls/tls.go
internal/runner/banner.go
Outdated
| r.options.Retries = 1 | ||
| } | ||
| probeSpecified := r.options.SO || r.options.TLSVersion || r.options.Cipher || r.options.Expired || r.options.SelfSigned || r.options.Hash != "" || r.options.Jarm || r.options.MisMatched || r.options.Revoked || r.options.WildcardCertCheck | ||
| probeSpecified := r.options.SO || r.options.TLSVersion || r.options.Cipher || r.options.KeyExchange || r.options.Expired || r.options.SelfSigned || r.options.Hash != "" || r.options.Jarm || r.options.MisMatched || r.options.Revoked || r.options.WildcardCertCheck |
There was a problem hiding this comment.
probeSpecified is still incomplete.
This predicate still misses several probe flags, so combinations like --resp-only --serial or --san --probe-status bypass the validation below even though the error text says SAN/CN and resp-only can't be mixed with other probes.
Suggested fix
-probeSpecified := r.options.SO || r.options.TLSVersion || r.options.Cipher || r.options.KeyExchange || r.options.Expired || r.options.SelfSigned || r.options.Hash != "" || r.options.Jarm || r.options.MisMatched || r.options.Revoked || r.options.WildcardCertCheck
+probeSpecified := r.options.SO || r.options.TLSVersion || r.options.Cipher || r.options.KeyExchange || r.options.ProbeStatus || r.options.Expired || r.options.SelfSigned || r.options.Untrusted || r.options.Hash != "" || r.options.Jarm || r.options.Ja3 || r.options.Ja3s || r.options.MisMatched || r.options.Revoked || r.options.WildcardCertCheck || r.options.TlsVersionsEnum || r.options.TlsCiphersEnum || r.options.Serial📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| probeSpecified := r.options.SO || r.options.TLSVersion || r.options.Cipher || r.options.KeyExchange || r.options.Expired || r.options.SelfSigned || r.options.Hash != "" || r.options.Jarm || r.options.MisMatched || r.options.Revoked || r.options.WildcardCertCheck | |
| probeSpecified := r.options.SO || r.options.TLSVersion || r.options.Cipher || r.options.KeyExchange || r.options.ProbeStatus || r.options.Expired || r.options.SelfSigned || r.options.Untrusted || r.options.Hash != "" || r.options.Jarm || r.options.Ja3 || r.options.Ja3s || r.options.MisMatched || r.options.Revoked || r.options.WildcardCertCheck || r.options.TlsVersionsEnum || r.options.TlsCiphersEnum || r.options.Serial |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/runner/banner.go` at line 34, The probeSpecified predicate in
banner.go is missing several probe flags (e.g., RespOnly, Serial, SAN,
ProbeStatus), so update the expression that builds probeSpecified (the line
using r.options.SO || r.options.TLSVersion || ...) to also OR in
r.options.RespOnly, r.options.Serial, r.options.SAN, and r.options.ProbeStatus
(and any other probe boolean flags introduced on the options struct) so
combinations like --resp-only/--serial/--san/--probe-status are treated as
probes and trigger the same validation logic.
Read ConnectionState.CurveID after TLS handshake and expose it as key_exchange in JSON output. Supports TLS 1.3 post-quantum key exchange detection (X25519MLKEM768 etc). Closes projectdiscovery#935
usernametooshort
force-pushed
the
feat/key-exchange-field
branch
from
3eb80e5 to
cb29af6
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/tlsx/tls/tls.go (1)
304-329: Usetls.CurveIDconstants instead of magic numbers and remove the redundant condition.The function can be improved for maintainability and clarity:
- Magic numbers: Replace numeric literals (
23,24,25,29) with the predefined constants (tls.CurveP256,tls.CurveP384,tls.CurveP521,tls.X25519).- Misleading comment: Lines 322–323 mention "Try using the built-in String() method" but the code doesn't actually use it.
- Redundant condition: After handling
case 0:, the checkif curveID > 0in the default branch is always true (sincetls.CurveIDisuint16).- Simplification: In Go 1.24,
tls.CurveID.String()is available and returns proper names for known curves; use it for the default case instead of the manual formatting.♻️ Proposed refactor
// curveIDToString converts CurveID to a human-readable string func curveIDToString(curveID tls.CurveID) string { - // Standard curve IDs from crypto/tls + if curveID == 0 { + return "" // No curve negotiated + } + // Handle known curves explicitly for consistency across Go versions switch curveID { - case 23: + case tls.CurveP256: return "CurveP256" - case 24: + case tls.CurveP384: return "CurveP384" - case 25: + case tls.CurveP521: return "CurveP521" - case 29: + case tls.X25519: return "X25519" case 4588: + // X25519MLKEM768 - post-quantum hybrid (Go 1.24+) return "X25519MLKEM768" - case 0: - return "" // No curve negotiated default: - // Try using the built-in String() method if available - // Otherwise return the numeric form - if curveID > 0 { - return fmt.Sprintf("CurveID(%d)", curveID) - } - return "" + return curveID.String() } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/tlsx/tls/tls.go` around lines 304 - 329, Replace magic numeric literals in curveIDToString with the corresponding tls.CurveID constants (tls.CurveP256, tls.CurveP384, tls.CurveP521, tls.X25519), remove the redundant if curveID > 0 check in the default branch, and call curveID.String() in the default case to return the name for known curves (falling back to "" for tls.CurveID(0)); update the comment about using the built-in String() method to reflect that it is actually used.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/tlsx/tls/tls.go`:
- Around line 304-329: Replace magic numeric literals in curveIDToString with
the corresponding tls.CurveID constants (tls.CurveP256, tls.CurveP384,
tls.CurveP521, tls.X25519), remove the redundant if curveID > 0 check in the
default branch, and call curveID.String() in the default case to return the name
for known curves (falling back to "" for tls.CurveID(0)); update the comment
about using the built-in String() method to reflect that it is actually used.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 28ccdd51-1993-4549-8ef1-032e83b184fe
📒 Files selected for processing (2)
pkg/tlsx/clients/clients.gopkg/tlsx/tls/tls.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/tlsx/clients/clients.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #935.
Summary
crypto/tlsConnectionState.CurveIDhas been populated after the handshake since Go 1.24. This PR exposes it in tlsx output askey_exchange(JSON) and via a new-ke/--key-exchangeCLI flag.In TLS 1.3, the cipher suite name no longer encodes the key exchange mechanism — it is negotiated separately. Without this field, there is no way to distinguish a post-quantum secured connection from a classical one when scanning at scale.
Changes
pkg/tlsx/clients/clients.go— addKeyExchange booloption andKeyExchange stringtoResponsepkg/tlsx/tls/tls.go— readconnectionState.CurveID.String()and populateresponse.KeyExchangecmd/tlsx/main.go— add-ke/--key-exchangeflagpkg/output/output.go— display in text output (cyan, alongside cipher)internal/runner/banner.go— includeKeyExchangeinprobeSpecifiedUsage
Notes
ctls(standardcrypto/tls) client; ztls does not exposeCurveIDin itsServerHellostructCurveID.String()in Go 1.24+ handles all known groups includingX25519MLKEM768(CurveID 4588)TestClientCertRequiredfailure in tls and ztls packages is unrelated to this PR (TLS 1.0 handshake rejection on Go 1.25)Summary by CodeRabbit