feat(certificate): report how the chain was served#60
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces structural certificate chain analysis to detect common misconfigurations, such as missing intermediates, redundant roots, out-of-order certificates, and duplicates. The feedback focuses on optimizing the implementation by storing the sorting error in the ChainReport to avoid redundant calls to SortChain, and fixing a potential false positive in the out-of-order detection when sorting fails.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| type ChainReport struct { | ||
| // Sent is the chain in the order it was presented. | ||
| Sent []*x509.Certificate | ||
| // Sorted is the chain rebuilt leaf-first. | ||
| Sorted []*x509.Certificate | ||
| // Findings are the problems with how it was presented, in the order they | ||
| // were discovered. | ||
| Findings []ChainFinding | ||
| } |
There was a problem hiding this comment.
To avoid calling SortChain twice (once in AnalyzeChain and once in validate.go), we can store the sorting error in the ChainReport struct. This allows callers to inspect the error if they need to abort on sorting failures, while still allowing AnalyzeChain to complete and return structural findings.
| type ChainReport struct { | |
| // Sent is the chain in the order it was presented. | |
| Sent []*x509.Certificate | |
| // Sorted is the chain rebuilt leaf-first. | |
| Sorted []*x509.Certificate | |
| // Findings are the problems with how it was presented, in the order they | |
| // were discovered. | |
| Findings []ChainFinding | |
| } | |
| type ChainReport struct { | |
| // Sent is the chain in the order it was presented. | |
| Sent []*x509.Certificate | |
| // Sorted is the chain rebuilt leaf-first. | |
| Sorted []*x509.Certificate | |
| // Err is the error returned by SortChain, if any. | |
| Err error | |
| // Findings are the problems with how it was presented, in the order they | |
| // were discovered. | |
| Findings []ChainFinding | |
| } |
| sorted, _ := SortChain(certs) | ||
| report.Sorted = sorted |
There was a problem hiding this comment.
Capture the error returned by SortChain and store it in the ChainReport so that callers can reuse it without re-running the sorting logic.
| sorted, _ := SortChain(certs) | |
| report.Sorted = sorted | |
| sorted, err := SortChain(certs) | |
| report.Sorted = sorted | |
| report.Err = err |
| if !sameOrder(certs, sorted) { | ||
| report.Findings = append(report.Findings, ChainFinding{ | ||
| Problem: ProblemOutOfOrder, | ||
| Subject: displayName(certs[0]), | ||
| Detail: "the certificates were not sent leaf-first; RFC 8446 asks for " + | ||
| "leaf-first order and some embedded TLS stacks require it", | ||
| }) | ||
| } |
There was a problem hiding this comment.
If SortChain fails (e.g., because the certificates are completely unrelated or broken), sameOrder(certs, sorted) will compare against an incomplete or empty sorted slice, leading to a false positive ProblemOutOfOrder finding. We should only report ProblemOutOfOrder if the chain was successfully sorted (err == nil) but was presented in a different order.
| if !sameOrder(certs, sorted) { | |
| report.Findings = append(report.Findings, ChainFinding{ | |
| Problem: ProblemOutOfOrder, | |
| Subject: displayName(certs[0]), | |
| Detail: "the certificates were not sent leaf-first; RFC 8446 asks for " + | |
| "leaf-first order and some embedded TLS stacks require it", | |
| }) | |
| } | |
| if err == nil && !sameOrder(certs, sorted) { | |
| report.Findings = append(report.Findings, ChainFinding{ | |
| Problem: ProblemOutOfOrder, | |
| Subject: displayName(certs[0]), | |
| Detail: "the certificates were not sent leaf-first; RFC 8446 asks for " + | |
| "leaf-first order and some embedded TLS stacks require it", | |
| }) | |
| } |
| // Look at the chain as it was presented, before sorting it: sorting is | ||
| // what destroys the evidence. | ||
| report := certificate.AnalyzeChain(inputCerts) | ||
|
|
||
| chain, err := certificate.SortChain(inputCerts) | ||
| if err != nil { | ||
| logger.Log.Error("Failed to sort certificate chain", zap.Error(err)) | ||
| return err | ||
| } |
There was a problem hiding this comment.
Instead of calling SortChain a second time, we can reuse the Sorted chain and any sorting error (Err) already captured in the ChainReport returned by AnalyzeChain. This avoids redundant work and simplifies the code.
// Look at the chain as it was presented, before sorting it: sorting is
// what destroys the evidence.
report := certificate.AnalyzeChain(inputCerts)
if report.Err != nil {
logger.Log.Error("Failed to sort certificate chain", zap.Error(report.Err))
return report.Err
}
chain := report.Sorted
kanywst
force-pushed
the
feat/connect-stacked
branch
from
ec1abb5 to
9073a35
Compare
|
Thanks, both applied. Reusing the sorted chain. Guarding the ordering check. Agreed on the reasoning: if
Behaviour is unchanged on real servers:
/gemini review |
kanywst
force-pushed
the
feat/chain-report
branch
from
1d9f9a0 to
b31cd2b
Compare
There was a problem hiding this comment.
Code Review
This pull request introduces certificate chain presentation analysis to identify issues like missing issuers, redundant roots, out-of-order certificates, duplicates, and unrelated certificates. The feedback highlights two important robustness improvements in pkg/certificate/chain.go: filtering out nil elements to prevent nil pointer dereference panics in AnalyzeChain, and deduplicating certificates by fingerprint in unrelatedIn to avoid incorrectly flagging duplicate certificates as unrelated.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| func AnalyzeChain(certs []*x509.Certificate) *ChainReport { | ||
| report := &ChainReport{Sent: certs} | ||
| if len(certs) == 0 { | ||
| return report | ||
| } | ||
|
|
||
| sorted, sortErr := SortChain(certs) |
There was a problem hiding this comment.
If the input slice certs contains any nil elements, calling SortChain(certs) or accessing fields on elements of certs later in AnalyzeChain will cause a nil pointer dereference panic. Filtering out nil elements at the very beginning of AnalyzeChain ensures the function is robust and panic-free.
func AnalyzeChain(certs []*x509.Certificate) *ChainReport {
report := &ChainReport{Sent: certs}
var validCerts []*x509.Certificate
for _, cert := range certs {
if cert != nil {
validCerts = append(validCerts, cert)
}
}
certs = validCerts
if len(certs) == 0 {
return report
}
sorted, sortErr := SortChain(certs)| func unrelatedIn(certs []*x509.Certificate) []*x509.Certificate { | ||
| if len(certs) < 2 { | ||
| return nil | ||
| } | ||
|
|
||
| var unrelated []*x509.Certificate | ||
| for i, cert := range certs { | ||
| connected := false | ||
| for j, other := range certs { | ||
| if i == j { | ||
| continue | ||
| } | ||
| if cert.Issuer.String() == other.Subject.String() || | ||
| other.Issuer.String() == cert.Subject.String() { | ||
| connected = true | ||
| break | ||
| } | ||
| } | ||
| if !connected { | ||
| unrelated = append(unrelated, cert) | ||
| } | ||
| } | ||
| return unrelated | ||
| } |
There was a problem hiding this comment.
If the input contains duplicate certificates (e.g., [Leaf, Leaf]), unrelatedIn will incorrectly flag them as unrelated because they are not self-signed and do not have parent-child relationships with each other. Deduplicating the certificates by fingerprint first prevents this logical bug and also guards against nil pointer dereferences.
func unrelatedIn(certs []*x509.Certificate) []*x509.Certificate {
var unique []*x509.Certificate
seen := make(map[string]bool)
for _, cert := range certs {
if cert == nil {
continue
}
fp := FormatFingerprint(cert)
if !seen[fp] {
seen[fp] = true
unique = append(unique, cert)
}
}
if len(unique) < 2 {
return nil
}
var unrelated []*x509.Certificate
for i, cert := range unique {
connected := false
for j, other := range unique {
if i == j {
continue
}
if cert.Issuer.String() == other.Subject.String() ||
other.Issuer.String() == cert.Subject.String() {
connected = true
break
}
}
if !connected {
unrelated = append(unrelated, cert)
}
}
return unrelated
kanywst
force-pushed
the
feat/connect-stacked
branch
from
9073a35 to
0635bf7
Compare
kanywst
force-pushed
the
feat/chain-report
branch
from
b31cd2b to
4ac1bba
Compare
kanywst
force-pushed
the
feat/chain-report
branch
from
4ac1bba to
32cd033
Compare
|
Thanks — the Duplicate reported as unrelated. Two copies of the same certificate are not each other's issuer, so Now collapses duplicates by fingerprint first, so a duplicated leaf is reported only as a duplicate. nil entries.
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces structural analysis of certificate chains as they are presented by a server, reporting issues such as missing issuers, redundant roots, out-of-order certificates, duplicates, and unrelated certificates. This is implemented via a new AnalyzeChain function in pkg/certificate/chain.go and integrated into the validate command. The review feedback highlights two main issues in the analysis logic: first, chainTerminus only traces from the first sorted certificate, which fails to analyze subsequent disjoint chains if multiple are present; second, unrelatedIn can incorrectly flag the primary leaf as unrelated if its intermediate is missing and another unrelated certificate is present. Solutions involving tracing from all identified leaves and using a reachability-based BFS from the primary leaf are suggested.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if terminus := chainTerminus(sorted); terminus != nil && !terminus.IsCA { | ||
| finding := ChainFinding{ | ||
| Problem: ProblemMissingIssuer, | ||
| Subject: displayName(terminus), | ||
| FetchURLs: terminus.IssuingCertificateURL, | ||
| Detail: fmt.Sprintf("the chain stops at a certificate that is not a CA; "+ | ||
| "its issuer %q was never sent, so a client that does not chase AIA "+ | ||
| "(curl, Go, Java) cannot build a chain", | ||
| nameOrUnknown(terminus.Issuer.CommonName)), | ||
| } | ||
| if len(finding.FetchURLs) == 0 { | ||
| finding.Detail += ", and it carries no AIA URL to fetch it from" | ||
| } | ||
| report.Findings = append(report.Findings, finding) | ||
| } |
There was a problem hiding this comment.
If the input contains multiple disjoint chains (which SortChain explicitly supports and logs), chainTerminus only starts tracing from sorted[0] (the leaf of the first chain). Consequently, if any subsequent disjoint chain has a missing intermediate, it will not be analyzed or flagged.
To support multiple disjoint chains, we should identify all "leaves" in the sorted slice (certificates that do not issue any other certificate in the slice) and trace the terminus for each leaf.
| if terminus := chainTerminus(sorted); terminus != nil && !terminus.IsCA { | |
| finding := ChainFinding{ | |
| Problem: ProblemMissingIssuer, | |
| Subject: displayName(terminus), | |
| FetchURLs: terminus.IssuingCertificateURL, | |
| Detail: fmt.Sprintf("the chain stops at a certificate that is not a CA; "+ | |
| "its issuer %q was never sent, so a client that does not chase AIA "+ | |
| "(curl, Go, Java) cannot build a chain", | |
| nameOrUnknown(terminus.Issuer.CommonName)), | |
| } | |
| if len(finding.FetchURLs) == 0 { | |
| finding.Detail += ", and it carries no AIA URL to fetch it from" | |
| } | |
| report.Findings = append(report.Findings, finding) | |
| } | |
| for _, leaf := range findLeaves(sorted) { | |
| if terminus := chainTerminus(sorted, leaf); terminus != nil && !terminus.IsCA { | |
| finding := ChainFinding{ | |
| Problem: ProblemMissingIssuer, | |
| Subject: displayName(terminus), | |
| FetchURLs: terminus.IssuingCertificateURL, | |
| Detail: fmt.Sprintf("the chain stops at a certificate that is not a CA; "+ | |
| "its issuer %q was never sent, so a client that does not chase AIA "+ | |
| "(curl, Go, Java) cannot build a chain", | |
| nameOrUnknown(terminus.Issuer.CommonName)), | |
| } | |
| if len(finding.FetchURLs) == 0 { | |
| finding.Detail += ", and it carries no AIA URL to fetch it from" | |
| } | |
| report.Findings = append(report.Findings, finding) | |
| } | |
| } |
| func chainTerminus(sorted []*x509.Certificate) *x509.Certificate { | ||
| if len(sorted) == 0 { | ||
| return nil | ||
| } | ||
|
|
||
| subjects := make(map[string]*x509.Certificate, len(sorted)) | ||
| for _, cert := range sorted { | ||
| subjects[cert.Subject.String()] = cert | ||
| } | ||
|
|
||
| current := sorted[0] | ||
| visited := make(map[string]bool, len(sorted)) | ||
| for { | ||
| if visited[current.Subject.String()] { | ||
| // Cyclic; there is no terminus to speak of. | ||
| return nil | ||
| } | ||
| visited[current.Subject.String()] = true | ||
|
|
||
| if current.Issuer.String() == current.Subject.String() { | ||
| // Self-signed: the chain ends here, and it is already reported as a | ||
| // redundant root. | ||
| return nil | ||
| } | ||
|
|
||
| parent, ok := subjects[current.Issuer.String()] | ||
| if !ok { | ||
| return current | ||
| } | ||
| current = parent | ||
| } | ||
| } |
There was a problem hiding this comment.
Update chainTerminus to accept a starting certificate so we can trace the terminus for any given leaf in disjoint chains, and add a findLeaves helper function.
func chainTerminus(sorted []*x509.Certificate, start *x509.Certificate) *x509.Certificate {
if len(sorted) == 0 || start == nil {
return nil
}
subjects := make(map[string]*x509.Certificate, len(sorted))
for _, cert := range sorted {
subjects[cert.Subject.String()] = cert
}
current := start
visited := make(map[string]bool, len(sorted))
for {
if visited[current.Subject.String()] {
// Cyclic; there is no terminus to speak of.
return nil
}
visited[current.Subject.String()] = true
if current.Issuer.String() == current.Subject.String() {
// Self-signed: the chain ends here, and it is already reported as a
// redundant root.
return nil
}
parent, ok := subjects[current.Issuer.String()]
if !ok {
return current
}
current = parent
}
}
func findLeaves(certs []*x509.Certificate) []*x509.Certificate {
isIssuer := make(map[string]bool)
for _, cert := range certs {
isIssuer[cert.Issuer.String()] = true
}
var leaves []*x509.Certificate
for _, cert := range certs {
if !isIssuer[cert.Subject.String()] {
leaves = append(leaves, cert)
}
}
return leaves
}| func unrelatedIn(certs []*x509.Certificate) []*x509.Certificate { | ||
| unique := make([]*x509.Certificate, 0, len(certs)) | ||
| seen := make(map[string]bool, len(certs)) | ||
| for _, cert := range certs { | ||
| fingerprint := FormatFingerprint(cert) | ||
| if seen[fingerprint] { | ||
| continue | ||
| } | ||
| seen[fingerprint] = true | ||
| unique = append(unique, cert) | ||
| } | ||
|
|
||
| if len(unique) < 2 { | ||
| return nil | ||
| } | ||
|
|
||
| var unrelated []*x509.Certificate | ||
| for i, cert := range unique { | ||
| connected := false | ||
| for j, other := range unique { | ||
| if i == j { | ||
| continue | ||
| } | ||
| if cert.Issuer.String() == other.Subject.String() || | ||
| other.Issuer.String() == cert.Subject.String() { | ||
| connected = true | ||
| break | ||
| } | ||
| } | ||
| if !connected { | ||
| unrelated = append(unrelated, cert) | ||
| } | ||
| } | ||
| return unrelated | ||
| } |
There was a problem hiding this comment.
In unrelatedIn, if the input contains the main leaf and an unrelated certificate (e.g., [Leaf, Stranger]) but the intermediate is missing, both the Leaf and the Stranger will be flagged as ProblemUnrelated. This is because Leaf has no connections in the bundle. However, the main leaf (certs[0]) is the target of validation and should never be flagged as unrelated.
To fix this, we can use a reachability-based approach (BFS/DFS) starting from the primary leaf (certs[0]). Any certificate reachable from the primary leaf via issuer-subject relationships (in either direction) is considered "related". Any certificate not reachable is "unrelated".
func unrelatedIn(certs []*x509.Certificate) []*x509.Certificate {
if len(certs) < 2 {
return nil
}
// Build adjacency list of connections (issuer <-> subject)
adj := make(map[string][]*x509.Certificate)
for _, c := range certs {
for _, other := range certs {
if c.Equal(other) {
continue
}
if c.Issuer.String() == other.Subject.String() || other.Issuer.String() == c.Subject.String() {
fp := FormatFingerprint(c)
adj[fp] = append(adj[fp], other)
}
}
}
// Start BFS from the primary leaf (certs[0])
reachable := make(map[string]bool)
queue := []*x509.Certificate{certs[0]}
reachable[FormatFingerprint(certs[0])] = true
for len(queue) > 0 {
curr := queue[0]
queue = queue[1:]
currFP := FormatFingerprint(curr)
for _, neighbor := range adj[currFP] {
neighborFP := FormatFingerprint(neighbor)
if !reachable[neighborFP] {
reachable[neighborFP] = true
queue = append(queue, neighbor)
}
}
}
// Any certificate not reachable is unrelated
var unrelated []*x509.Certificate
seen := make(map[string]bool)
for _, c := range certs {
fp := FormatFingerprint(c)
if seen[fp] {
continue
}
seen[fp] = true
if !reachable[fp] {
unrelated = append(unrelated, c)
}
}
return unrelated
}
Verifying a chain and serving it correctly are different questions. A
server can present a chain that browsers accept and that curl refuses,
because browsers chase the AIA URL to fetch a missing intermediate and
curl, Go and Java do not. It is the classic "works in Chrome, breaks in
curl" bug, and no tool reports it: openssl's own docs concede that
s_client -showcerts "displays the server certificate list as sent by the
server ... it is not a verified chain".
AnalyzeChain answers it. Given the certificates in the order they were
presented, it reports a missing issuer (with the AIA URL that would supply
it), a redundant root, certificates sent out of order, duplicates, and
certificates belonging to no chain at all.
The check is deliberately structural: it looks only at what was sent and
never asks a verifier whether the chain builds. That is not fastidiousness.
On macOS, Go's verification delegates to the platform, which fetches the
missing intermediate over the network -- so the very bug this catches
verifies clean:
$ y509 validate incomplete-chain.badssl.com:443
✅ Certificate chain is valid.
Trust anchor: ISRG Root X1
Chain as presented:
• missing issuer: *.badssl.com
the chain stops at a certificate that is not a CA; its issuer "R13"
was never sent, so a client that does not chase AIA cannot build a
chain
fetch from: http://r13.i.lencr.org/
A verifier-based check would have been quietly useless on the platform most
people run it from. Asking "what did you send me" cannot be fooled.
The rule that separates a real missing intermediate from a cross-signed
root is that a chain must terminate at a CA. GTS Root R1, which google.com
sends, has an absent issuer (GlobalSign) and must not be flagged; a leaf
whose intermediate was never sent must be.
AnalyzeChain sorts the chain on its way through, and validate then sorted the same certificates all over again. Carry the result, and the sort error with it, so the caller can take them. Also only report an ordering problem when the chain sorted cleanly. If SortChain ever fails, the sorted slice need not hold the same certificates, and comparing against it would report an out-of-order chain that is really a sorting failure.
Two copies of the same certificate are not each other's issuer, so unrelatedIn flagged a duplicated leaf as unrelated -- once per copy -- on top of the duplicate finding it already carried. Collapse duplicates by fingerprint before looking for unrelated certificates. AnalyzeChain is also exported, so it now drops nil entries up front rather than dereferencing them.
The pairwise "has any neighbour" test had two failures. It flagged the primary leaf as unrelated whenever its issuer was not sent -- reporting the validation target itself as baggage on top of the missing-issuer finding. And it missed a whole disjoint second chain, because each of its members had a neighbour within that chain even though none connected to the leaf. Walk outward from the leaf instead. Anything reachable by issuer/subject links is part of its chain; everything else is unrelated. The leaf is the starting point, so it is never unrelated, and a disjoint chain is unrelated as a unit.
kanywst
force-pushed
the
feat/chain-report
branch
from
32cd033 to
8d1d7e7
Compare
|
Thanks — the Reachability (applied). The pairwise "has any neighbour" test flagged the primary leaf as unrelated whenever its issuer was not sent — reporting the validation target itself as baggage on top of the missing-issuer finding. Reproduced with Now Per-leaf missing-issuer for disjoint chains (declined). Tracing every disjoint leaf's terminus would flag a random non-CA stranger as "missing issuer" on top of "unrelated" — strictly noisier. This tool validates one leaf (the first presented); anything disconnected from it is reported as unrelated baggage, not separately dissected. Digging into a stranger's internal chain shape is out of scope and would produce confusing double-findings.
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces structural certificate chain analysis to detect presentation issues such as missing issuers, redundant roots, out-of-order certificates, duplicates, and unrelated certificates. It updates the validate command to report these findings and adds comprehensive unit tests. The reviewer feedback highlights two critical issues: first, chainTerminus can falsely detect cycles and perform inaccurate parent lookups when dealing with duplicate subject names (such as cross-signed intermediates), which can be resolved by using fingerprints and signature verification; second, a lone self-signed certificate is incorrectly flagged as a redundant root, which can be fixed by ensuring the check only applies when multiple certificates are present.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| visited := make(map[string]bool, len(sorted)) | ||
| for { | ||
| if visited[current.Subject.String()] { | ||
| // Cyclic; there is no terminus to speak of. | ||
| return nil | ||
| } | ||
| visited[current.Subject.String()] = true | ||
|
|
||
| if current.Issuer.String() == current.Subject.String() { | ||
| // Self-signed: the chain ends here, and it is already reported as a | ||
| // redundant root. | ||
| return nil | ||
| } | ||
|
|
||
| parent, ok := subjects[current.Issuer.String()] | ||
| if !ok { | ||
| return current | ||
| } | ||
| current = parent | ||
| } |
There was a problem hiding this comment.
There are two issues in chainTerminus related to handling certificates with duplicate subject names (which is common for cross-signed intermediates):
- False Cycle Detection: Cycle detection uses
current.Subject.String(). If a chain contains two different certificates with the same subject name (e.g., a cross-signed intermediate and the original intermediate), it will falsely detect a cycle and returnnil, failing to report a missing issuer further up. - Inaccurate Parent Lookup: Parent lookup uses only subject name matching (
subjects[current.Issuer.String()]). If there are multiple certificates with the same subject name, it can select the wrong parent.
To fix these, we should use the certificate's fingerprint for cycle detection, and verify the actual signature using CheckSignatureFrom during parent lookup.
visited := make(map[string]bool, len(sorted))
for {
fp := FormatFingerprint(current)
if visited[fp] {
// Cyclic; there is no terminus to speak of.
return nil
}
visited[fp] = true
if current.Issuer.String() == current.Subject.String() {
// Self-signed: the chain ends here, and it is already reported as a
// redundant root.
return nil
}
// Find the parent in sorted that actually signed current, to handle
// different certificates with the same subject name (e.g. cross-signed).
var parent *x509.Certificate
for _, cert := range sorted {
if current.Issuer.String() == cert.Subject.String() {
if current.CheckSignatureFrom(cert) == nil {
parent = cert
break
}
}
}
if parent == nil {
return current
}
current = parent
}| if cert.Issuer.String() == cert.Subject.String() { | ||
| report.Findings = append(report.Findings, ChainFinding{ | ||
| Problem: ProblemRedundantRoot, | ||
| Subject: displayName(cert), | ||
| Detail: "a self-signed root was included; clients ignore it and " + | ||
| "trust their own copy, so it only adds bytes to every handshake", | ||
| }) | ||
| } |
There was a problem hiding this comment.
A lone self-signed certificate (when len(certs) == 1) is flagged as ProblemRedundantRoot. This is a false positive because a lone self-signed certificate is the leaf certificate itself and must be sent by the server to establish a connection; it is not redundant. We should only flag self-signed certificates as redundant if there are other certificates in the chain (i.e., len(certs) > 1).
| if cert.Issuer.String() == cert.Subject.String() { | |
| report.Findings = append(report.Findings, ChainFinding{ | |
| Problem: ProblemRedundantRoot, | |
| Subject: displayName(cert), | |
| Detail: "a self-signed root was included; clients ignore it and " + | |
| "trust their own copy, so it only adds bytes to every handshake", | |
| }) | |
| } | |
| if cert.Issuer.String() == cert.Subject.String() && len(certs) > 1 { | |
| report.Findings = append(report.Findings, ChainFinding{ | |
| Problem: ProblemRedundantRoot, | |
| Subject: displayName(cert), | |
| Detail: "a self-signed root was included; clients ignore it and " + | |
| "trust their own copy, so it only adds bytes to every handshake", | |
| }) | |
| } |
1 participant
The gap
Verifying a chain and serving it correctly are different questions, and nothing reports the second one. A server can present a chain your browser accepts and
curlrefuses — browsers chase the AIA URL to fetch a missing intermediate; curl, Go and Java do not. It is the classic "works in Chrome, breaks in curl" bug.openssl explicitly declines to help. Its own docs:
s_client -showcerts"displays the server certificate list as sent by the server … it is not a verified chain."This is the one proposal in the audit that no competitor ships. certigo has an open issue asking for it.
What it reports
AnalyzeChaintakes the certificates in the order they were presented and reports:The design decision worth reviewing
The check is purely structural. It never asks a verifier whether the chain builds.
That is not fastidiousness — I tried the verifier-based approach first and it was actively broken. On macOS, Go's verification delegates to the platform verifier, which chases the AIA URL and fetches the missing intermediate over the network. So the exact bug this is meant to catch verifies perfectly clean.
Here is the live proof, and it is also the best demo of the feature:
The chain verified and is still misconfigured. A verifier-based check would have been quietly useless on the platform most people run it from. Asking "what did you actually send me" cannot be fooled.
Avoiding the false positive
The rule separating a real missing intermediate from a cross-signed root is: a chain must terminate at a CA.
GTS Root R1— which google.com sends — has an absent issuer (GlobalSign Root CA) and must not be flagged, because the client trusts the root itself. A leaf whose intermediate was never sent must be. My first attempt flagged google.com; this one doesn't. Both cases are pinned by tests.Verified against real servers
Tests are offline and synthetic — well-formed, missing intermediate, cross-signed root, redundant root, out of order, duplicate, unrelated, lone self-signed, empty — so they cannot be swayed by the platform verifier either.
make lintclean,markdownlintclean, full suite green.