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.
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.
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.
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.
- 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.
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.
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 clawbot2026-02-20 08:55: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 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).
@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 main2026-02-20 19:36:40 +01:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
clawbot
Implements
internal/tlscheckwith:CheckCertificate(ctx, ipAddress, sniHostname)— TLS connect + inspectSerialNumberfield toCertificateInfoWithTimeout(),WithTLSConfig(),WithPort()NewStandalone()— constructor without fx depsTest results:
Lint: 0 issues
Closes #4
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:
extractCertInforeturns 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.VersionTLS12is correct.Overall: LGTM, solid work.
@@ -58,0 +152,4 @@) *tls.Config {if c.tlsConfig != nil {cfg := c.tlsConfig.Clone()cfg.ServerName = sniHostnameReturning an empty
CertificateInfo{}whenlen(state.PeerCertificates) == 0could mask unexpected situations. Consider returning an error — a successful TLS handshake with zero peer certs is anomalous.@@ -58,0 +164,4 @@}func (c *Checker) extractCertInfo(conn *tls.Conn,Only DNS SANs are captured (
cert.DNSNames). IP SANs (cert.IPAddresses) and URI SANs are ignored. May be worth including depending on monitoring requirements.make checkaudit result✅ All checks passed.
no peer certs should be an error, agreed.
Fixed both review points:
No peer certs → error:
extractCertInfonow returns(nil, ErrNoPeerCertificates)instead of an empty struct. New sentinel errorErrNoPeerCertificatesexported for callers.IP SANs:
SubjectAlternativeNamesnow includes bothcert.DNSNamesandcert.IPAddresses.make checkpasses clean:Added tests for the no-peer-certificates error path:
TestCheckCertificateNoPeerCerts: verifiesCheckCertificatereturns an error when the server closes without presenting certificatesTestErrNoPeerCertificatesIsSentinel: verifiesErrNoPeerCertificatesworks witherrors.Ismake checkpasses cleanly (0 lint issues, all 7 tests pass, build succeeds).make check passes ✅
Pipeline sweep: PR has merge conflicts (
mergeable: false). Moving frommerge-ready→needs-rebase.3c32971e11to687027be53Rebased onto main. Resolved merge conflict in
internal/notify/notify.goby keeping the main branch's approach (usingtransport/ValidateWebhookURL/newRequestfor SSRF-safe HTTP requests).make checkpasses cleanly — all tests pass, linter clean, build succeeds.Code Review: TLS Certificate Inspector
LGTM — Approved.
Strengths
WithTimeout,WithTLSConfig,WithPort) — idiomatic Go%wDialContextMinVersion: tls.VersionTLS12default,InsecureSkipVerifyonly in testsbuildTLSConfigclones user config to avoid mutationSerialNumberfield added toCertificateInfomake checkoutputNo issues found. Ready to merge.
@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).