Detect post-quantum key exchange and fix certificate reporting - #2
Merged
Merged
Conversation
The analyzer could never report a quantum-safe connection. parseKeyExchange returned a hardcoded classical X25519 for every TLS 1.3 connection and quantumSafe was a constant false in three places, so a server negotiating X25519MLKEM768 was reported as quantum vulnerable. Read the negotiated group from tls.ConnectionState.CurveID instead, and recognize X25519MLKEM768, SecP256r1MLKEM768 and SecP384r1MLKEM1024. Key exchange results now also list the groups a server supports, not only the one this client negotiated. Certificate fingerprints were not hashes. sha256Fingerprint returned hex(der[:20])+"..." and sha1Fingerprint returned hex(der[:10])+"...", so both values were prefixes of the certificate, began with the same DER header bytes and matched no real fingerprint. They are now genuine digests that agree with openssl x509 -fingerprint. Also fixed: elliptic curve certificates reported publicKeyBits 0 because the type switch matched only types implementing Size() int; TLS_AES_128_GCM_SHA256 reported 256 bits because Contains(name, "256") matched the SHA256 suffix; concurrent protocol probes appended in completion order while the code assumed declaration order, so output varied between identical runs; generated reports recorded scanner version 0.1.0 regardless of the installed release; and CBOM output failed CycloneDX 1.6 validation on a primitive outside the enum and two empty bom-ref strings. Quantum risk is now weighted 80/20 toward key exchange rather than 60/40. Under the old weighting a server running hybrid ML-KEM with a classical certificate scored 48/100, graded HIGH and was told to begin hybrid PQC implementation, work it had already completed. No publicly trusted CA issues ML-DSA certificates, so that dimension is not remediable by any operator today, and key exchange is the only dimension that applies retroactively. Requires Go 1.25 for tls.ConnectionState.CurveID. CI moved from Go 1.21. Fixes #1
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
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.
Fixes #1.
The reported problem
@manel1874 reported in January that
tls-analyzernever reports a quantum-safeconnection, and pointed at the two exact lines responsible. That was correct.
parseKeyExchangereturned a hardcoded classicalX25519for every TLS 1.3connection, and
quantumSafewas a constantfalsein three separate places.A server negotiating
X25519MLKEM768was reported as quantum vulnerable, so noscan could ever produce a quantum-safe result.
The code comment claimed hybrid detection "requires raw handshake access or
TLS extension parsing" and a custom TLS stack. That is no longer true. Go 1.24
offers X25519MLKEM768 by default, and Go 1.25 exposes the negotiated group as
tls.ConnectionState.CurveID.Before, against the reporter's own endpoint:
After:
Verified against
openssl s_client, which reportsNegotiated TLS1.3 group: X25519MLKEM768for the same host.Answers to the reporter's questions
X25519MLKEM768,SecP256r1MLKEM768andSecP384r1MLKEM1024aredetected as of this change.
standard library now reports the negotiated group directly.
Beyond the report
Scans now also enumerate which groups the server supports, not only the one this
client negotiated, since readiness is a question about the server rather than
about our client's preferences.
Four further defects surfaced while reproducing this:
sha256Fingerprintreturnedhex(der[:20])+"...". Both "fingerprints" were prefixes of the certificate,started with the same DER header bytes, and matched no real fingerprint. They
now agree with
openssl x509 -fingerprintbyte for byte.publicKeyBits: 0, because the type switchmatched only types implementing
Size() int.TLS_AES_128_GCM_SHA256reported 256 bits, becauseContains(name, "256")matched the
SHA256suffix first.appended in completion order while the code assumed declaration order, so the
list order and the
preferredmarker varied between identical runs.Reports also recorded scanner version
0.1.0regardless of the installedrelease, and the CBOM output failed CycloneDX 1.6 validation on a
primitivevalue outside the schema enum plus two empty
bom-refstrings.Scoring change
Quantum risk is now weighted 80/20 toward key exchange, from 60/40.
Under the old weighting a server running hybrid ML-KEM with a classical
certificate scored 48/100, graded HIGH, and was told to "begin hybrid PQC
implementation" that it had already completed. No publicly trusted CA issues
ML-DSA certificates, so the certificate dimension is not remediable by any
operator at any price, and it structurally capped every real site. Key exchange
is also the only dimension that applies retroactively, through
harvest-now-decrypt-later. The same server now scores 64/100 and grades MEDIUM.
Testing
New regression tests cover group mapping, cipher key sizes, fingerprint
correctness, public key sizing, merge determinism, CBOM schema conformance, and
the advice regression. Verified they fail against the pre-fix code rather than
passing vacuously.
go test -race ./...is clean.Requires Go 1.25 for
ConnectionState.CurveID. CI moved from Go 1.21, which hadalso drifted from the 1.23 declared in go.mod.