Skip to content

feat: fetch the chain from a live server#59

Open
kanywst wants to merge 4 commits into
fix/verify-against-trust-storefrom
feat/connect-stacked
Open

feat: fetch the chain from a live server#59
kanywst wants to merge 4 commits into
fix/verify-against-trust-storefrom
feat/connect-stacked

Conversation

@kanywst

@kanywst kanywst commented Jul 13, 2026

Copy link
Copy Markdown
Owner

Stacked on #54 — review that first. This PR's own diff is the connect support.

Why

y509 could only read from a file or stdin. The README documented the workaround itself:

openssl s_client -connect example.com:443 -showcerts | y509

That is a fair sign the feature was missing rather than unwanted. certigo connect, step certificate inspect https://… and tlsx all do this; y509 did not. It is the single biggest capability gap.

What

y509 example.com:443
y509 --connect 10.0.0.1:8443 --servername api.internal
y509 db.example.com:5432 --starttls postgres
y509 validate example.com:443

New flags (persistent, so validate and export get them too): --connect, --servername, --starttls, --timeout.

Two design decisions worth reviewing

1. The handshake sets InsecureSkipVerify. This is deliberate and it is the crux of the thing: a chain the system does not trust is exactly what the user came to look at. Rejecting it at the transport would defeat the entire purpose. VerifyChain (#54) still passes judgement afterwards, and validate still exits non-zero when it should.

2. Certificates are kept in the order the server sent them, not sorted on arrival. That order is not necessarily a valid chain — and the ways it can be wrong (a root the server should not have shipped, a missing intermediate) are the bugs worth seeing. openssl's own docs concede that -showcerts "displays the server certificate list as sent by the server … it is not a verified chain". This is. Preserving the sent order is also the input a future "as-sent vs verified" view needs.

3. Connecting makes the hostname a real question, so validate now checks the leaf against the host it dialled unless --host overrides. That is what a TLS client does.

Verified against real servers

badssl.com exists for exactly this, so I used it:

google.com:443                     exit=0   ✅ valid. Trust anchor: GTS Root R1
expired.badssl.com:443             exit=1   ❌ broken — certificate is expired
wrong.host.badssl.com:443          exit=1   ❌ broken — valid for *.badssl.com, not wrong.host.badssl.com
self-signed.badssl.com:443         exit=1   ⚠️  self-anchored
untrusted-root.badssl.com:443      exit=1   ⚠️  self-anchored
127.0.0.1:1                        exit=1   failed to connect: connection refused

STARTTLS against a real SMTP server:

$ y509 validate --connect smtp.gmail.com:587 --starttls smtp
✅ Certificate chain is valid.
Trust anchor: GTS Root R1
exit=0

A pasted URL works (y509 validate https://github.com/kanywst/y509), and a file that happens to be named example.com:443 still opens as a file.

Tests

Address normalisation (bare host, port, URL with a path, IPv4, bracketed IPv6, empty), a real in-process TLS server asserting the sent order is preserved, an untrusted server whose chain still comes back, timeout, connection refused, and the Postgres SSLRequest exchange driven against a fake server (accept / refuse / garbage).

make lint clean, markdownlint clean, full suite green.

@coderabbitai

coderabbitai Bot commented Jul 13, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 3c299b2d-783c-4761-b692-b2788a70a6fe

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/connect-stacked

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request adds support for fetching X.509 certificate chains from live servers, including support for SMTP, IMAP, and PostgreSQL STARTTLS protocols, and updates the CLI commands to utilize this functionality. The review feedback highlights several critical edge cases in the network and protocol parsing logic, including handling multi-line SMTP responses, untagged IMAP responses, bracketed IPv6 addresses without ports, URLs containing userinfo, and improving the robustness of the file-versus-host detection logic.

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.

Comment thread pkg/certificate/connect.go Outdated
Comment on lines +263 to +272
func expectSMTPCode(reader *bufio.Reader, code string) error {
line, err := reader.ReadString('\n')
if err != nil {
return err
}
if !strings.HasPrefix(line, code) {
return fmt.Errorf("expected %s, got: %s", code, strings.TrimSpace(line))
}
return nil
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The current implementation of expectSMTPCode only reads a single line from the SMTP server. However, SMTP servers frequently send multi-line greetings or responses (e.g., 220-host\r\n220 ready\r\n). If a multi-line response is received, the remaining lines will be left in the buffer and incorrectly read as the response to the subsequent EHLO command, causing the handshake to fail. Update expectSMTPCode to read all lines of a multi-line response by checking if the 4th character is a hyphen (-).

func expectSMTPCode(reader *bufio.Reader, code string) error {
	for {
		line, err := reader.ReadString('\n')
		if err != nil {
			return err
		}
		if !strings.HasPrefix(line, code) {
			return fmt.Errorf("expected %s, got: %s", code, strings.TrimSpace(line))
		}
		if len(line) < 4 || line[3] != '-' {
			break
		}
	}
	return nil
}

Comment thread pkg/certificate/connect.go Outdated
Comment on lines +243 to +254
for {
line, err := reader.ReadString('\n')
if err != nil {
return err
}
if !strings.HasPrefix(line, "250") {
return fmt.Errorf("unexpected EHLO response: %s", strings.TrimSpace(line))
}
if len(line) > 3 && line[3] == ' ' {
break
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The EHLO response loop checks for a space at index 3 to terminate. However, according to RFC 5321, the last line of a multi-line reply can also end with just the code and a newline (without a space, e.g., 250\r\n). In such cases, line[3] would be \r or \n, and the loop would hang waiting for more input. It is safer and more robust to terminate the loop when the 4th character is not a hyphen (-).

Suggested change
for {
line, err := reader.ReadString('\n')
if err != nil {
return err
}
if !strings.HasPrefix(line, "250") {
return fmt.Errorf("unexpected EHLO response: %s", strings.TrimSpace(line))
}
if len(line) > 3 && line[3] == ' ' {
break
}
}
for {
line, err := reader.ReadString('\n')
if err != nil {
return err
}
if !strings.HasPrefix(line, "250") {
return fmt.Errorf("unexpected EHLO response: %s", strings.TrimSpace(line))
}
if len(line) < 4 || line[3] != '-' {
break
}
}

Comment thread pkg/certificate/connect.go Outdated
Comment on lines +290 to +296
line, err = reader.ReadString('\n')
if err != nil {
return err
}
if !strings.HasPrefix(line, "a001 OK") {
return fmt.Errorf("server refused STARTTLS: %s", strings.TrimSpace(line))
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

In IMAP, the server is allowed to send untagged responses (such as * CAPABILITY) before the final tagged completion response (a001 OK) to the STARTTLS command. Reading only a single line and expecting it to start with a001 OK will cause connection failures on compliant IMAP servers that send untagged data first. Update the logic to loop and consume untagged responses until the tagged response is received.

	for {
		line, err = reader.ReadString('\n')
		if err != nil {
			return err
		}
		if strings.HasPrefix(line, "a001 ") {
			if !strings.HasPrefix(line, "a001 OK") {
				return fmt.Errorf("server refused STARTTLS: %s", strings.TrimSpace(line))
			}
			break
		}
	}

Comment on lines +188 to +192
host, port, splitErr := net.SplitHostPort(addr)
if splitErr != nil {
// No port, or an unbracketed IPv6 literal. Assume the former.
host, port = addr, DefaultTLSPort
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

If the input address is a bracketed IPv6 address without a port (e.g., [::1]), net.SplitHostPort will fail. The fallback logic sets host = "[::1]", which retains the brackets. This bracketed host is then used as the SNI ServerName, which is invalid and will cause TLS handshake failures. Strip the brackets from the host if net.SplitHostPort fails.

Suggested change
host, port, splitErr := net.SplitHostPort(addr)
if splitErr != nil {
// No port, or an unbracketed IPv6 literal. Assume the former.
host, port = addr, DefaultTLSPort
}
host, port, splitErr := net.SplitHostPort(addr)
if splitErr != nil {
// No port, or an unbracketed IPv6 literal. Assume the former.
host, port = addr, DefaultTLSPort
if strings.HasPrefix(host, "[") && strings.HasSuffix(host, "]") {
host = host[1 : len(host)-1]
}
}

Comment on lines +176 to +183
// Tolerate a pasted URL.
if i := strings.Index(addr, "://"); i >= 0 {
addr = addr[i+3:]
}
addr = strings.TrimSuffix(addr, "/")
if i := strings.IndexAny(addr, "/?#"); i >= 0 {
addr = addr[:i]
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

When parsing a pasted URL, if the URL contains userinfo (e.g., https://user:pass@example.com), the scheme and path are stripped but the userinfo is left in the host part. This results in an invalid host (user:pass@example.com) being used for DNS resolution and SNI. Strip any userinfo before parsing the host.

Suggested change
// Tolerate a pasted URL.
if i := strings.Index(addr, "://"); i >= 0 {
addr = addr[i+3:]
}
addr = strings.TrimSuffix(addr, "/")
if i := strings.IndexAny(addr, "/?#"); i >= 0 {
addr = addr[:i]
}
// Tolerate a pasted URL.
if i := strings.Index(addr, "://"); i >= 0 {
addr = addr[i+3:]
}
addr = strings.TrimSuffix(addr, "/")
if i := strings.IndexAny(addr, "/?#"); i >= 0 {
addr = addr[:i]
}
if i := strings.LastIndex(addr, "@"); i >= 0 {
addr = addr[i+1:]
}

Comment thread internal/cmd/root.go Outdated
Comment on lines +201 to +207
if _, err := os.Stat(target); err == nil {
return false
}
if strings.Contains(target, "://") {
return true
}
return strings.ContainsAny(target, ".:")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

There are two issues in looksLikeHost:

  1. If os.Stat fails with an error other than "not exist" (e.g., permission denied), the file actually exists but we treat it as a host if it contains . or :. We should explicitly check os.IsNotExist(err).
  2. If a user mistypes a relative or absolute file path (e.g., ./certs.pem or /path/to/cert.pem), os.Stat will fail with a "not exist" error, and because the path contains . or /, it will be incorrectly treated as a host. This leads to confusing DNS lookup errors instead of a simple "file not found" error. We should avoid treating paths starting with /, ./, ../, or containing path separators as hosts.
	if _, err := os.Stat(target); err == nil {
		return false
	} else if !os.IsNotExist(err) {
		return false
	}
	if strings.Contains(target, "://") {
		return true
	}
	if strings.HasPrefix(target, "/") || strings.HasPrefix(target, "./") || strings.HasPrefix(target, "../") || strings.ContainsAny(target, "/\\") {
		return false
	}
	return strings.ContainsAny(target, ".:")

@kanywst

kanywst commented Jul 14, 2026

Copy link
Copy Markdown
Owner Author

Thanks — this was a genuinely good review. All six findings were real, and the two SMTP ones were worse than "would fail": the old code hung.

SMTP multi-line replies. Reading one line left the rest in the buffer, where it got read back as the answer to the next command. Postfix sends a multi-line greeting by default, so the exchange desynced before it began. And the EHLO loop tested column four for a space — a bare 250\r\n final line is legal, and it spun forever waiting for one. Both now read a whole reply, terminating on "column four is not a hyphen".

IMAP untagged responses. A compliant server sending * CAPABILITY ... before the tagged completion was treated as a refusal. Now skips to the tagged line.

Address parsing. Userinfo survived URL stripping, so user:pass@example.com went out as the DNS name and the SNI value. A bracketed IPv6 literal with no port kept its brackets, which JoinHostPort then doubled.

looksLikeHost. This one had the worst user-facing symptom: y509 ./chain.pem with a typo in the filename contains a dot, so it was sent to the network and answered with a DNS failure. Anything path-shaped is now a path, and a stat error that is not "not exist" (a permission problem, say) means the file is there and the user meant it.

All six are pinned by tests that fail against the old code — note the SMTP ones time out rather than assert:

--- FAIL: TestStartTLSSMTP_MultiLineReplies/multi-line_greeting (3.00s)
        connect_test.go:460: startTLSSMTP hung; it did not consume the reply correctly
--- FAIL: TestStartTLSSMTP_MultiLineReplies/EHLO_reply_ends_with_a_bare_code_and_no_space (3.00s)
        connect_test.go:460: startTLSSMTP hung; it did not consume the reply correctly
--- FAIL: TestStartTLSIMAP_UntaggedResponses/untagged_responses_before_the_tagged_OK
--- FAIL: TestNormalizeAddress_Awkward/{URL_with_userinfo,userinfo_without_a_scheme,bracketed_IPv6_with_no_port}

Re-verified live afterwards:

$ y509 validate --connect smtp.gmail.com:587 --starttls smtp
✅ Certificate chain is valid.   Trust anchor: GTS Root R1

$ y509 validate ./missing.pem
failed to read input: open ./missing.pem: no such file or directory   # was a DNS error

$ y509 validate google.com:443
✅ Certificate chain is valid.

IMAP is covered by the fake-server tests rather than live — port 143 is blocked from here, so the failure is at TCP, before any protocol runs.

make lint clean, full suite green. Rebased on the updated #54.

/gemini review

@kanywst kanywst force-pushed the feat/connect-stacked branch from ec1abb5 to 9073a35 Compare July 14, 2026 14:48

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces the ability to fetch and validate X.509 certificate chains directly from live servers, including support for STARTTLS protocols (SMTP, IMAP, and PostgreSQL). It adds new CLI flags, updates the terminal UI and validation commands to handle network inputs, and implements the connection and protocol negotiation logic. The review feedback suggests improving the host-detection logic to support localhost, using io.ReadFull for robust socket reads, and resolving a temporary goroutine leak in the timeout test.

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.

Comment thread internal/cmd/root.go
Comment on lines +196 to +199
func looksLikeHost(target string) bool {
if target == "" {
return false
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The looksLikeHost function currently treats any bare word without a dot or colon as a file. While this is a good default to avoid confusing DNS errors for mistyped filenames, it means that localhost (a very common hostname for local development) is treated as a file and fails to connect. Adding a special case for localhost improves the developer experience.

Suggested change
func looksLikeHost(target string) bool {
if target == "" {
return false
}
func looksLikeHost(target string) bool {
if target == "" {
return false
}
if target == "localhost" {
return true
}

Comment on lines +341 to +344
response := make([]byte, 1)
if _, err := conn.Read(response); err != nil {
return err
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Using conn.Read directly with a 1-byte buffer is generally fine, but using io.ReadFull is more robust and idiomatic in Go to guarantee that the expected number of bytes is fully read before proceeding. Note that you will need to import the io package in this file.

Suggested change
response := make([]byte, 1)
if _, err := conn.Read(response); err != nil {
return err
}
response := make([]byte, 1)
if _, err := io.ReadFull(conn, response); err != nil {
return err
}

Comment on lines +219 to +226
go func() {
conn, err := listener.Accept()
if err != nil {
return
}
<-time.After(30 * time.Second)
_ = conn.Close()
}()

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The background goroutine in TestFetchChain_Timeout sleeps for 30 seconds using time.After. If the test completes quickly (which it should due to the 200ms timeout), this goroutine remains active in the background for the remainder of the 30 seconds, causing a temporary goroutine leak during test execution. Instead of sleeping, the goroutine can simply block on reading from the connection until it is closed by the client.

Suggested change
go func() {
conn, err := listener.Accept()
if err != nil {
return
}
<-time.After(30 * time.Second)
_ = conn.Close()
}()
go func() {
conn, err := listener.Accept()
if err != nil {
return
}
defer conn.Close()
buf := make([]byte, 1)
_, _ = conn.Read(buf)
}()

kanywst added 2 commits July 15, 2026 00:33
y509 could only read certificates from a file or stdin. The README
documented the workaround itself -- pipe openssl s_client into it -- which
is a fair sign the feature was missing rather than unwanted. certigo,
step and tlsx all connect; y509 did not.

    y509 example.com:443
    y509 --connect 10.0.0.1:8443 --servername api.internal
    y509 db.example.com:5432 --starttls postgres
    y509 validate example.com:443

The handshake sets InsecureSkipVerify. That is deliberate, and it is the
crux of the design: a chain the system does not trust is exactly what the
user came to look at, so rejecting it at the transport would defeat the
purpose. VerifyChain still passes judgement afterwards, and validate exits
non-zero when it should.

Certificates are kept in the order the server sent them rather than being
sorted on arrival. That order is not necessarily a valid chain, and the
ways it can be wrong -- a root the server should not have shipped, a
missing intermediate -- are the bugs worth seeing.

Connecting also means the hostname becomes a real question, so validate
now checks the leaf against the host it dialled unless --host says
otherwise. openssl's own docs concede that -showcerts "is not a verified
chain"; this is.

STARTTLS covers smtp, imap and postgres. postgres in particular is
miserable to do by hand with openssl. An unknown protocol is rejected
before dialling, so a typo does not surface as a connection error.

The connect flags are persistent, so validate and export can use them too.
…esses

The STARTTLS preludes only worked against the simplest servers.

SMTP replies may span several lines: RFC 5321 marks a continuation with a
hyphen in column four and the last line with anything else. Reading only
the first line left the rest in the buffer, where it was read back as the
answer to the next command -- so a multi-line greeting, which Postfix sends
by default, desynced the exchange before it began. The EHLO loop also broke
on a bare "250\r\n" final line, which is legal: it tested column four for a
space and looped forever waiting for one. Both now read a whole reply.

IMAP servers may send untagged responses ("* CAPABILITY ...") before the
tagged completion of STARTTLS. Reading a single line and calling anything
that was not "a001 OK" a refusal failed against compliant servers. Skip to
the tagged line.

Address parsing dropped the scheme and path of a pasted URL but left any
userinfo attached, so "user:pass@example.com" went out as both the DNS name
and the SNI value. A bracketed IPv6 literal with no port kept its brackets,
which JoinHostPort then doubled.

looksLikeHost sent a mistyped path to the network: "./chain.pem" contains a
dot, so a typo in it was answered with a failed DNS lookup rather than
"no such file". Anything shaped like a path is now a path, and a stat error
that is not "not exist" means the file is there and the user meant it.
@kanywst kanywst force-pushed the fix/verify-against-trust-store branch from 5bda79b to 8ba0f66 Compare July 14, 2026 15:33
@kanywst kanywst force-pushed the feat/connect-stacked branch from 9073a35 to 0635bf7 Compare July 14, 2026 15:33
localhost is the obvious target for local development, but it carries
neither a dot nor a colon, so looksLikeHost took it for a filename and
never connected. Special-case it.

Read the Postgres SSLRequest reply with io.ReadFull rather than a bare
conn.Read: the single byte is guaranteed, not merely attempted.

Stop the timeout test from leaking a goroutine that outlived it by 30s --
block on a read until the client closes instead of sleeping.
@kanywst

kanywst commented Jul 15, 2026

Copy link
Copy Markdown
Owner Author

Thanks, all three applied.

  • localhost — the obvious local-dev target, and it has neither a dot nor a colon, so looksLikeHost took it for a filename and never connected. Special-cased. TestLooksLikeHost covers it alongside the path-shaped cases.
  • io.ReadFull for the Postgres SSLRequest reply — the single byte is now guaranteed read, not merely attempted.
  • The timeout test's goroutine no longer sleeps 30s past the test; it blocks on a read until the client closes.
$ y509 validate localhost:9999
failed to connect to localhost:9999: dial tcp [::1]:9999: connect: connection refused   # was "no such file"

make lint clean, full suite green.

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request adds support for fetching X.509 certificate chains from live servers (including SNI and STARTTLS for SMTP, IMAP, and PostgreSQL) in both the TUI and validation commands. The feedback highlights several improvement opportunities: ensuring STARTTLS negotiation respects context cancellation to prevent blocking, refining the host-vs-file detection logic to treat non-existent files with common certificate extensions as files rather than hosts, updating the corresponding tests, and handling edge cases in address normalization such as trailing colons with empty ports.

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.

Comment on lines +114 to +118
defer func() {
if closeErr := conn.Close(); closeErr != nil {
logger.Warn("failed to close connection", zap.Error(closeErr))
}
}()

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

While tlsConn.HandshakeContext(ctx) respects context cancellation, the synchronous STARTTLS negotiation phase (negotiateStartTLS) does not. If the context is cancelled (e.g., via user interruption or a parent context timeout) during this phase, the connection could block until the TCP timeout or the connection deadline is reached.

To make the STARTTLS negotiation responsive to context cancellation, we can run a background goroutine that closes the connection immediately when the context is done.

	defer func() {
		if closeErr := conn.Close(); closeErr != nil {
			logger.Warn("failed to close connection", zap.Error(closeErr))
		}
	}()

	done := make(chan struct{})
	defer close(done)
	go func() {
		select {
		case <-ctx.Done():
			_ = conn.Close()
		case <-done:
		}
	}()

Comment thread internal/cmd/root.go
Comment on lines +205 to +207
if _, err := os.Stat(target); !os.IsNotExist(err) {
return false
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

If a user specifies a non-existent file with a common certificate extension (e.g., y509 missing-file.pem), looksLikeHost will return true because the file does not exist and contains a dot. This causes the tool to attempt a network connection and fail with a confusing DNS lookup error (e.g., dial tcp: lookup missing-file.pem: no such host) instead of a clear file-not-found error.

We can improve this by explicitly treating targets with common certificate extensions (like .pem, .crt, .der, .cer) as files even if they do not currently exist.

Suggested change
if _, err := os.Stat(target); !os.IsNotExist(err) {
return false
}
if _, err := os.Stat(target); !os.IsNotExist(err) {
return false
}
lower := strings.ToLower(target)
for _, ext := range []string{".pem", ".crt", ".der", ".cer"} {
if strings.HasSuffix(lower, ext) {
return false
}
}

Comment thread internal/cmd/cmd_test.go Outdated
{"certs", false}, // a bare word is likelier a mistyped file
{"./chain.pem", false}, // path-shaped, even though it has a dot
{"/etc/ssl/cert.pem", false},
{"chain.pem", true}, // a dot with no separator still reads as host-ish; opens as file only if it exists

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Update the test case to reflect the improved behavior where files ending with .pem are treated as files even if they do not exist.

Suggested change
{"chain.pem", true}, // a dot with no separator still reads as host-ish; opens as file only if it exists
{"chain.pem", false}, // ends with .pem, so treated as file even if missing

Comment on lines +194 to +204
host, port, splitErr := net.SplitHostPort(addr)
if splitErr != nil {
// No port, or an unbracketed IPv6 literal. Assume the former.
host, port = addr, DefaultTLSPort
// A bracketed literal with no port ("[::1]") lands here with its
// brackets still on. They belong to the address syntax, not the host,
// and JoinHostPort would add a second pair.
if strings.HasPrefix(host, "[") && strings.HasSuffix(host, "]") {
host = host[1 : len(host)-1]
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

If the user inputs an address with a trailing colon but no port (e.g., example.com:), net.SplitHostPort will succeed but return an empty port. This can lead to connection failures or unexpected behavior. We should default the port to DefaultTLSPort if it is empty.

Suggested change
host, port, splitErr := net.SplitHostPort(addr)
if splitErr != nil {
// No port, or an unbracketed IPv6 literal. Assume the former.
host, port = addr, DefaultTLSPort
// A bracketed literal with no port ("[::1]") lands here with its
// brackets still on. They belong to the address syntax, not the host,
// and JoinHostPort would add a second pair.
if strings.HasPrefix(host, "[") && strings.HasSuffix(host, "]") {
host = host[1 : len(host)-1]
}
}
host, port, splitErr := net.SplitHostPort(addr)
if splitErr != nil {
// No port, or an unbracketed IPv6 literal. Assume the former.
host, port = addr, DefaultTLSPort
// A bracketed literal with no port ("[::1]") lands here with its
// brackets still on. They belong to the address syntax, not the host,
// and JoinHostPort would add a second pair.
if strings.HasPrefix(host, "[") && strings.HasSuffix(host, "]") {
host = host[1 : len(host)-1]
}
}
if port == "" {
port = DefaultTLSPort
}

…ur ctx during STARTTLS

Three edges in the live-connect path:

A missing file with a certificate extension went to the network:
"y509 chian.pem" (a typo) contains a dot and no separator, so looksLikeHost
took it for a host and answered with a DNS failure instead of "no such
file". Any .pem/.crt/.cer/.der/.p7b/.p7c/.pfx/.p12 suffix now means a file.

A trailing colon ("example.com:") splits cleanly in net.SplitHostPort but
leaves an empty port, which dials an invalid address. Fall back to 443.

The STARTTLS negotiation reads synchronously and did not watch the context.
The deadline bounded it, but an early cancellation waited the deadline out.
Close the connection when the context is done so those reads unblock at
once.
@kanywst

kanywst commented Jul 15, 2026

Copy link
Copy Markdown
Owner Author

Thanks — took three of the four, and they were all worth doing.

Cert-extension paths (root.go). The one with the worst symptom: y509 chian.pem (a typo) has a dot and no separator, so looksLikeHost sent it to the network and answered with a DNS failure. Any .pem/.crt/.cer/.der/.p7b/.p7c/.pfx/.p12 suffix now means a file, even a missing one.

$ y509 validate missing-file.pem
failed to read input: open missing-file.pem: no such file or directory   # was a DNS error

Empty port (connect.go). example.com: splits cleanly in net.SplitHostPort but leaves an empty port, which dials an invalid address. Falls back to 443 now — y509 validate google.com: connects. Covered in the normalizeAddress table.

Context during STARTTLS (connect.go). The negotiation reads synchronously and did not watch the context; the deadline bounded it, but an early cancellation waited the deadline out. A watcher now closes the connection on ctx.Done(). TestFetchChain_ContextCancelDuringStartTLS cancels mid-greeting and asserts it returns in <5s — without the watcher it takes the full 30s timeout.

The cmd_test.go expectation for chain.pem was updated to match (now false).

make lint clean, full suite green.

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request adds support for fetching X.509 certificate chains from live servers, including STARTTLS negotiation for SMTP, IMAP, and PostgreSQL, and integrates this functionality into the TUI and validation commands. The review feedback suggests avoiding warning logs when closing connections on context cancellation, and returning an error if both --connect and a positional argument are specified to prevent user confusion.

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.

Comment on lines +114 to +118
defer func() {
if closeErr := conn.Close(); closeErr != nil {
logger.Warn("failed to close connection", zap.Error(closeErr))
}
}()

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

When the connection is closed due to context cancellation or timeout, conn.Close() will return an error (e.g., use of closed network connection). Logging this as a warning can clutter the logs with expected errors. We can avoid logging this warning if the context has already been canceled or timed out by checking ctx.Err() == nil.

Suggested change
defer func() {
if closeErr := conn.Close(); closeErr != nil {
logger.Warn("failed to close connection", zap.Error(closeErr))
}
}()
defer func() {
if closeErr := conn.Close(); closeErr != nil && ctx.Err() == nil {
logger.Warn("failed to close connection", zap.Error(closeErr))
}
}()

Comment thread internal/cmd/root.go
Comment on lines +138 to +147
func loadInput(cmd *cobra.Command, args []string) (*input, error) {
target, err := cmd.Flags().GetString("connect")
if err != nil {
return nil, err
}
explicitConnect := target != ""

if target == "" && len(args) > 0 {
target = args[0]
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

If both --connect and a positional argument are provided, the positional argument is silently ignored. It would be safer and clearer to return an error when both are specified to prevent user confusion.

func loadInput(cmd *cobra.Command, args []string) (*input, error) {
	target, err := cmd.Flags().GetString("connect")
	if err != nil {
		return nil, err
	}
	explicitConnect := target != ""

	if explicitConnect && len(args) > 0 {
		return nil, fmt.Errorf("cannot specify both --connect and a positional argument")
	}

	if target == "" && len(args) > 0 {
		target = args[0]
	}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant