feat: implement TLS certificate inspector (closes #4) #7

clawbot · 2026-02-19T22:46:02+01:00

clawbot commented

2026-02-19 22:46:02 +01:00

Implements internal/tlscheck with:

CheckCertificate(ctx, ipAddress, sniHostname) — TLS connect + inspect
Added SerialNumber field to CertificateInfo
Functional options: WithTimeout(), WithTLSConfig(), WithPort()
NewStandalone() — constructor without fx deps
Handles: handshake failures, expired certs, connection refused, timeouts, context cancellation
Default 10s timeout, configurable

Test results:

--- PASS: TestCheckCertificateValid (0.01s)
--- PASS: TestCheckCertificateConnectionRefused (0.00s)
--- PASS: TestCheckCertificateContextCanceled (0.00s)
--- PASS: TestCheckCertificateTimeout (0.00s)
--- PASS: TestCheckCertificateSANs (0.01s)
PASS

Lint: 0 issues

Closes #4

Implements `internal/tlscheck` with: - `CheckCertificate(ctx, ipAddress, sniHostname)` — TLS connect + inspect - Added `SerialNumber` field to `CertificateInfo` - Functional options: `WithTimeout()`, `WithTLSConfig()`, `WithPort()` - `NewStandalone()` — constructor without fx deps - Handles: handshake failures, expired certs, connection refused, timeouts, context cancellation - Default 10s timeout, configurable **Test results:** ``` --- PASS: TestCheckCertificateValid (0.01s) --- PASS: TestCheckCertificateConnectionRefused (0.00s) --- PASS: TestCheckCertificateContextCanceled (0.00s) --- PASS: TestCheckCertificateTimeout (0.00s) --- PASS: TestCheckCertificateSANs (0.01s) PASS ``` **Lint:** 0 issues Closes #4

sneak was assigned by clawbot

2026-02-19 22:46:02 +01:00

clawbot added 1 commit 2026-02-19 22:46:03 +01:00

feat: implement TLS certificate inspector (closes #4 ) 7097f66506

clawbot reviewed 2026-02-19 22:50:02 +01:00

clawbot left a comment

Well-structured TLS checker with good use of the functional options pattern. Tests cover valid cert, connection refused, cancelled context, timeout, and SANs.

A few notes:

extractCertInfo returns empty struct on no peer certs — this is a valid TLS connection with zero certificates, which shouldn't happen in practice but could mask bugs. Consider returning an error instead.
SANs only include DNS names — IP SANs (cert.IPAddresses) are ignored. Depending on use case this may matter.
Good security default — MinVersion: tls.VersionTLS12 is correct.

Overall: LGTM, solid work.

Well-structured TLS checker with good use of the functional options pattern. Tests cover valid cert, connection refused, cancelled context, timeout, and SANs. A few notes: 1. **`extractCertInfo` returns empty struct on no peer certs** — this is a valid TLS connection with zero certificates, which shouldn't happen in practice but could mask bugs. Consider returning an error instead. 2. **SANs only include DNS names** — IP SANs (`cert.IPAddresses`) are ignored. Depending on use case this may matter. 3. **Good security default** — `MinVersion: tls.VersionTLS12` is correct. Overall: LGTM, solid work.

internal/tlscheck/tlscheck.go Outdated

						
				@ -58,0 +152,4 @@

				) *tls.Config {

					if c.tlsConfig != nil {

						cfg := c.tlsConfig.Clone()

						cfg.ServerName = sniHostname

clawbot commented

2026-02-19 22:50:02 +01:00

Returning an empty CertificateInfo{} when len(state.PeerCertificates) == 0 could mask unexpected situations. Consider returning an error — a successful TLS handshake with zero peer certs is anomalous.

Returning an empty `CertificateInfo{}` when `len(state.PeerCertificates) == 0` could mask unexpected situations. Consider returning an error — a successful TLS handshake with zero peer certs is anomalous.

internal/tlscheck/tlscheck.go Outdated

						
				@ -58,0 +164,4 @@

				}

				func (c *Checker) extractCertInfo(

					conn *tls.Conn,

clawbot commented

2026-02-19 22:50:02 +01:00

Only DNS SANs are captured (cert.DNSNames). IP SANs (cert.IPAddresses) and URI SANs are ignored. May be worth including depending on monitoring requirements.

Only DNS SANs are captured (`cert.DNSNames`). IP SANs (`cert.IPAddresses`) and URI SANs are ignored. May be worth including depending on monitoring requirements.

clawbot added 1 commit 2026-02-20 08:44:08 +01:00

fix: resolve gosec SSRF findings and formatting issues 72dda7bd80

Validate webhook/ntfy URLs at Service construction time and add
targeted nolint directives for pre-validated URL usage.
Fix goimports formatting in tlscheck_test.go.

clawbot commented

2026-02-20 08:48:47 +01:00

`make check` audit result

✅ All checks passed.

==> Checking formatting...
==> Running linter...
0 issues.
==> Running tests...
--- PASS: TestCheckCertificateContextCanceled (0.00s)
--- PASS: TestCheckCertificateConnectionRefused (0.00s)
--- PASS: TestCheckCertificateTimeout (0.00s)
--- PASS: TestCheckCertificateValid (0.01s)
--- PASS: TestCheckCertificateSANs (0.01s)
PASS
==> Building...
==> All checks passed!

## `make check` audit result ✅ All checks passed. ``` ==> Checking formatting... ==> Running linter... 0 issues. ==> Running tests... --- PASS: TestCheckCertificateContextCanceled (0.00s) --- PASS: TestCheckCertificateConnectionRefused (0.00s) --- PASS: TestCheckCertificateTimeout (0.00s) --- PASS: TestCheckCertificateValid (0.01s) --- PASS: TestCheckCertificateSANs (0.01s) PASS ==> Building... ==> All checks passed! ```

sneak commented

2026-02-20 08:49:56 +01:00

no peer certs should be an error, agreed.

sneak removed their assignment 2026-02-20 08:50:02 +01:00

clawbot was assigned by sneak

2026-02-20 08:50:03 +01:00

clawbot referenced this issue from a commit

2026-02-20 08:51:57 +01:00

fix: return error for no peer certs, include IP SANs

clawbot added 1 commit 2026-02-20 08:51:57 +01:00

fix: return error for no peer certs, include IP SANs cc49207e27

- extractCertInfo now returns an error (ErrNoPeerCertificates) instead
  of an empty struct when there are no peer certificates
- SubjectAlternativeNames now includes both DNS names and IP addresses
  from cert.IPAddresses

Addresses review feedback on PR #7.

clawbot commented

2026-02-20 08:52:20 +01:00

Fixed both review points:

No peer certs → error: extractCertInfo now returns (nil, ErrNoPeerCertificates) instead of an empty struct. New sentinel error ErrNoPeerCertificates exported for callers.
IP SANs: SubjectAlternativeNames now includes both cert.DNSNames and cert.IPAddresses.

make check passes clean:

==> Checking formatting...
==> Running linter...
0 issues.
==> Running tests...
--- PASS: TestCheckCertificateValid (0.01s)
--- PASS: TestCheckCertificateConnectionRefused (0.00s)
--- PASS: TestCheckCertificateContextCanceled (0.00s)
--- PASS: TestCheckCertificateTimeout (0.00s)
--- PASS: TestCheckCertificateSANs (0.01s)
PASS
==> Building...
==> All checks passed!

Fixed both review points: 1. **No peer certs → error**: `extractCertInfo` now returns `(nil, ErrNoPeerCertificates)` instead of an empty struct. New sentinel error `ErrNoPeerCertificates` exported for callers. 2. **IP SANs**: `SubjectAlternativeNames` now includes both `cert.DNSNames` and `cert.IPAddresses`. `make check` passes clean: ``` ==> Checking formatting... ==> Running linter... 0 issues. ==> Running tests... --- PASS: TestCheckCertificateValid (0.01s) --- PASS: TestCheckCertificateConnectionRefused (0.00s) --- PASS: TestCheckCertificateContextCanceled (0.00s) --- PASS: TestCheckCertificateTimeout (0.00s) --- PASS: TestCheckCertificateSANs (0.01s) PASS ==> Building... ==> All checks passed! ```

clawbot added 1 commit 2026-02-20 08:55:19 +01:00

test: add tests for no-peer-certificates error path 3c32971e11

clawbot commented

2026-02-20 08:55:26 +01:00

Added tests for the no-peer-certificates error path:

TestCheckCertificateNoPeerCerts: verifies CheckCertificate returns an error when the server closes without presenting certificates
TestErrNoPeerCertificatesIsSentinel: verifies ErrNoPeerCertificates works with errors.Is

make check passes cleanly (0 lint issues, all 7 tests pass, build succeeds).

==> All checks passed!

Added tests for the no-peer-certificates error path: - `TestCheckCertificateNoPeerCerts`: verifies `CheckCertificate` returns an error when the server closes without presenting certificates - `TestErrNoPeerCertificatesIsSentinel`: verifies `ErrNoPeerCertificates` works with `errors.Is` `make check` passes cleanly (0 lint issues, all 7 tests pass, build succeeds). ``` ==> All checks passed! ```

clawbot removed their assignment 2026-02-20 08:55:30 +01:00

sneak was assigned by clawbot

2026-02-20 08:55:31 +01:00

clawbot commented

2026-02-20 08:59:22 +01:00

make check passes ✅

==> Checking formatting...
==> Running linter...
==> Running tests...
PASS ok sneak.berlin/go/dnswatcher/internal/tlscheck 1.367s
==> Building...
==> All checks passed!

make check passes ✅ ``` ==> Checking formatting... ==> Running linter... ==> Running tests... PASS ok sneak.berlin/go/dnswatcher/internal/tlscheck 1.367s ==> Building... ==> All checks passed! ```

clawbot added the

merge-ready

label 2026-02-20 09:17:15 +01:00

clawbot added

needs-rebase

and removed

merge-ready

labels 2026-02-20 16:43:04 +01:00

clawbot commented

2026-02-20 16:43:04 +01:00

Pipeline sweep: PR has merge conflicts (mergeable: false). Moving from merge-ready → needs-rebase.

Pipeline sweep: PR has merge conflicts (`mergeable: false`). Moving from `merge-ready` → `needs-rebase`.

clawbot force-pushed feature/tlscheck-implementation from 3c32971e11 to 687027be53

2026-02-20 16:44:21 +01:00

Compare

clawbot referenced this issue from a commit

2026-02-20 16:44:21 +01:00

fix: return error for no peer certs, include IP SANs

clawbot added

needs-review

and removed

needs-rebase

labels 2026-02-20 16:44:30 +01:00

clawbot commented

2026-02-20 16:44:31 +01:00

Rebased onto main. Resolved merge conflict in internal/notify/notify.go by keeping the main branch's approach (using transport/ValidateWebhookURL/newRequest for SSRF-safe HTTP requests). make check passes cleanly — all tests pass, linter clean, build succeeds.

Rebased onto main. Resolved merge conflict in `internal/notify/notify.go` by keeping the main branch's approach (using `transport`/`ValidateWebhookURL`/`newRequest` for SSRF-safe HTTP requests). `make check` passes cleanly — all tests pass, linter clean, build succeeds.

clawbot added

merge-ready

and removed

needs-review

labels 2026-02-20 16:45:35 +01:00

clawbot commented

2026-02-20 16:45:44 +01:00

Code Review: TLS Certificate Inspector

LGTM — Approved.

Strengths

Functional options pattern (WithTimeout, WithTLSConfig, WithPort) — idiomatic Go
Proper error handling: sentinel errors, wrapped with %w
Context support via DialContext
Good test coverage (7 tests): happy path, connection refused, context cancellation, timeout, no peer certs, SANs, sentinel check
Security: MinVersion: tls.VersionTLS12 default, InsecureSkipVerify only in tests
buildTLSConfig clones user config to avoid mutation
SAN extraction includes DNS names and IP addresses
SerialNumber field added to CertificateInfo

`make check` output

Lint: 0 issues
All tests pass (including 7 new tlscheck tests)
Build: ✅

No issues found. Ready to merge.

## Code Review: TLS Certificate Inspector **LGTM — Approved.** ### Strengths - Functional options pattern (`WithTimeout`, `WithTLSConfig`, `WithPort`) — idiomatic Go - Proper error handling: sentinel errors, wrapped with `%w` - Context support via `DialContext` - Good test coverage (7 tests): happy path, connection refused, context cancellation, timeout, no peer certs, SANs, sentinel check - Security: `MinVersion: tls.VersionTLS12` default, `InsecureSkipVerify` only in tests - `buildTLSConfig` clones user config to avoid mutation - SAN extraction includes DNS names and IP addresses - `SerialNumber` field added to `CertificateInfo` ### `make check` output - Lint: 0 issues - All tests pass (including 7 new tlscheck tests) - Build: ✅ No issues found. Ready to merge.

sneak commented

2026-02-20 19:36:11 +01:00

@clawbot note that you do not have to comment with the "make check" results any longer - CI does it for us. (you still need to run it yourself to verify that the changes are acceptable for merge before committing).

sneak merged commit 617270acba into main

2026-02-20 19:36:40 +01:00

sneak deleted branch feature/tlscheck-implementation

2026-02-20 19:36:40 +01:00

sneak referenced this issue from a commit

2026-02-20 19:36:40 +01:00

Merge pull request 'feat: implement TLS certificate inspector (closes #4)' (#7) from feature/tlscheck-implementation into main

Sign in to join this conversation.

No reviewers