Bundle E: Mechanical sweeps & defensive polish — 6 findings closed; L-004 deferred

Closes L-009 + L-010 + L-011 + L-013 + L-020 + L-021 from
comprehensive-audit-2026-04-25. L-004 deferred — recon found NO
rotation infrastructure exists at all; building it from scratch is
a feature project, not a Bundle-E mechanical sweep.

L-009 — ZeroSSL EAB URL configurable
  Audit's 'no timeout' claim was wrong: ari.go:329 has 15s timeout.
  internal/connector/issuer/acme/acme.go: zeroSSLEABEndpoint now
  lazily reads CERTCTL_ZEROSSL_EAB_URL from env at package init;
  defaults to ZeroSSL public endpoint. Pre-existing test override
  path preserved.

L-010 — Verified-already-clean
  grep -rn 'mock\.Anything' --include='*_test.go' . returned 0.
  certctl uses hand-rolled struct mocks (mockJobRepo, mockAuditRepo,
  etc.) with explicit method bodies; no testify-style mocks anywhere.

L-011 — IPv6 bracket-aware dialing pinned
  Every production net.Dial / DialTimeout site audited:
    cmd/agent/main.go:293 — intentional IPv4 literal '8.8.8.8:80'
    verify.go / tlsprobe / network_scan — net.Dialer (no string addr)
    email.go — net.JoinHostPort (bracket-aware)
    ssh.go — addr derives from JoinHostPort upstream
    ssrf.go — net.Dialer
  internal/connector/notifier/email/email_ipv6_test.go (NEW):
    TestJoinHostPort_IPv6BracketsRoundTrip pins IPv4/IPv6/zone variants;
    TestSMTPDialerUsesJoinHostPort source-greps email.go and fails CI
    if a future refactor swaps in 'host:port' concatenation.

L-013 — Verified-already-clean (monotonic-safe)
  Only one site uses now.Sub: middleware.go:393 in tokenBucket.allow().
  Both 'now' and tb.lastRefill come from time.Now() which carries
  monotonic-clock readings per Go's time package contract;
  intra-process now.Sub is monotonic-safe by construction. Doc
  comment block added above the call to make the invariant explicit.

L-020 (CWE-563) — ineffassign sweep, 8 unique sites
  certificate.go:135 — sortDir initial value dropped (set
    unconditionally below by SortDesc branch).
  certificate.go:169,175 — argCount post-increments dropped (var
    not read past the LIMIT/OFFSET formatting).
  agent_group.go, profile.go — page/perPage truly vestigial,
    replaced with _ = page; _ = perPage.
  issuer.go:633, owner.go:131, target.go:267, team.go:131 — same
    treatment for the audit-flagged second-function ListXxx clamps.
  First-function List() in issuer/owner/target/team KEEPS its
    clamp because page/perPage is used for in-memory slice
    pagination — ineffassign correctly didn't flag those.
  Build + tests green post-sweep.

L-021 — Transitive CVE bump
  go get golang.org/x/crypto@v0.45.0 golang.org/x/net@v0.47.0
    (crypto required net@0.47.0). go-text@v0.31.0 transitively
    bumped.
  Per tool-output govulncheck-verbose: x/net@v0.45.0 fixes
    GO-2026-4441 + GO-2026-4440; x/crypto@v0.45.0 fixes
    GO-2025-4134 + GO-2025-4135 + GO-2025-4116 — all 5 advisories
    cleared. Bundle B's ISV grep guard + Bundle D's release-time
    govulncheck step are the going-forward monitor + bump pass.

L-004 — Deferred to dedicated bundle
  Recon: zero hits for RotateAPIKey / rotated_at / key_status
    anywhere in source. API keys configured via
    CERTCTL_API_KEYS_NAMED env var; rotation is operator-managed
    (edit env + restart). Building rotation infrastructure from
    scratch is a feature project, not a mechanical sweep.
  Documented in audit-report.md with scope-pivot note.

Audit deliverables:
  audit-report.md: score 46/55 -> 52/55 closed
    (Low 14/19 -> 19/19 — 100% Low closed except L-004 deferred)
  findings.yaml: 6 status flips
  certctl/CHANGELOG.md: Bundle E section

Verification:
  go test -count=1 -short ./internal/service ./internal/connector/issuer/acme
    ./internal/connector/notifier/email                      green
  go vet on changed packages                                  clean
This commit is contained in:
Shankar
2026-04-27 01:17:15 +00:00
parent 7bece7a11b
commit e776327f71
13 changed files with 194 additions and 48 deletions
+8
View File
@@ -389,6 +389,14 @@ func (tb *tokenBucket) allow() bool {
tb.mu.Lock()
defer tb.mu.Unlock()
// Bundle E / Audit L-013 (monotonic clock): both `now` and
// `tb.lastRefill` come from `time.Now()`, which carries a
// monotonic-clock reading per the time package contract. `t1.Sub(t2)`
// uses the monotonic component when both ts have it, so this elapsed
// computation is NOT affected by wall-clock drift, NTP slew, DST, or
// `clock_settime` adjustments. The audit's general concern about
// `time.Now().Sub` was about wall-clock-only deltas across process
// boundaries; this is intra-process and monotonic-safe.
now := time.Now()
elapsed := now.Sub(tb.lastRefill).Seconds()
tb.tokens += elapsed * tb.rate
+18 -3
View File
@@ -16,6 +16,7 @@ import (
"net"
"net/http"
"net/url"
"os"
"strings"
"sync"
"time"
@@ -302,9 +303,23 @@ func (c *Connector) ensureClient(ctx context.Context) error {
return nil
}
// zeroSSLEABEndpoint is the ZeroSSL API endpoint for auto-generating EAB credentials.
// Variable (not const) to allow test overrides.
var zeroSSLEABEndpoint = "https://api.zerossl.com/acme/eab-credentials-email"
// zeroSSLEABEndpoint is the ZeroSSL API endpoint for auto-generating EAB
// credentials. Variable (not const) to allow test overrides AND operator
// overrides at startup via the CERTCTL_ZEROSSL_EAB_URL env var.
//
// Bundle E / Audit L-009: pre-bundle the URL was hardcoded; if ZeroSSL
// changed the endpoint or an operator wanted to point at an internal
// proxy/mirror, only a code change would have done it. Now any non-empty
// CERTCTL_ZEROSSL_EAB_URL at process start replaces the default. The
// HTTP client at the call site already enforces a 15-second timeout
// (line ~329) — audit's "no timeout" claim was incorrect; the timeout
// has been in place since the auto-EAB feature shipped.
var zeroSSLEABEndpoint = func() string {
if v := os.Getenv("CERTCTL_ZEROSSL_EAB_URL"); v != "" {
return v
}
return "https://api.zerossl.com/acme/eab-credentials-email"
}()
// isZeroSSL returns true if the ACME directory URL points to ZeroSSL.
func isZeroSSL(directoryURL string) bool {
@@ -0,0 +1,92 @@
package email
import (
"net"
"os"
"strings"
"testing"
)
var osReadFile = os.ReadFile
// Bundle E / Audit L-011 (IPv6 dual-stack handling): every production
// `net.Dial`/`net.DialTimeout` call site was audited; the SMTP / email
// notifier path uses `net.JoinHostPort(SMTPHost, port)` which is
// bracket-aware by spec. This test pins the JoinHostPort shape so a
// future refactor that switches to bare `host + ":" + port`
// concatenation — which would silently break IPv6 literals — fails CI.
//
// Other production net.Dial sites are out of scope for this test:
// - cmd/agent/main.go:293 uses literal "8.8.8.8:80" intentionally
// (IPv4 route-discovery hack)
// - cmd/agent/verify.go, internal/tlsprobe/probe.go,
// internal/service/network_scan.go use net.Dialer (no string addr)
// - internal/connector/target/ssh/ssh.go uses an addr derived from
// net.JoinHostPort upstream
// The audit's per-site analysis confirms each is bracket-aware or
// intentionally IPv4-literal.
func TestJoinHostPort_IPv6BracketsRoundTrip(t *testing.T) {
cases := []struct {
name string
host string
port string
want string
}{
{"ipv4_literal", "10.0.0.1", "587", "10.0.0.1:587"},
{"ipv6_literal", "::1", "587", "[::1]:587"},
{"ipv6_full", "2001:db8::1", "25", "[2001:db8::1]:25"},
{"hostname", "smtp.example.com", "465", "smtp.example.com:465"},
{"ipv6_zone", "fe80::1%eth0", "587", "[fe80::1%eth0]:587"},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
got := net.JoinHostPort(tc.host, tc.port)
if got != tc.want {
t.Errorf("net.JoinHostPort(%q, %q) = %q, want %q",
tc.host, tc.port, got, tc.want)
}
// Round-trip via SplitHostPort.
rh, rp, err := net.SplitHostPort(got)
if err != nil {
t.Fatalf("net.SplitHostPort(%q): %v", got, err)
}
// IPv6-zone hosts come back without the literal brackets.
expectedHost := tc.host
if rh != expectedHost {
t.Errorf("round-trip host: got %q, want %q", rh, expectedHost)
}
if rp != tc.port {
t.Errorf("round-trip port: got %q, want %q", rp, tc.port)
}
})
}
}
func TestSMTPDialerUsesJoinHostPort(t *testing.T) {
// Source-grep regression pin: the email notifier MUST use
// net.JoinHostPort when assembling SMTP addresses, never bare
// "host:port" string concatenation. We don't actually dial a
// server here — we just assert the source pattern.
//
// Ridiculously cheap test, but a future refactor that swaps in
// `fmt.Sprintf("%s:%d", host, port)` would silently break IPv6
// SMTP destinations and this test catches it pre-merge.
body := mustReadFile(t, "email.go")
if !strings.Contains(body, "net.JoinHostPort") {
t.Fatal("internal/connector/notifier/email/email.go must use net.JoinHostPort for IPv6 bracket-awareness (L-011)")
}
// Additionally make sure no bare "%s:%d" SMTP pattern slipped in.
if strings.Contains(body, `fmt.Sprintf("%s:%d"`) {
t.Error("found bare host:port concatenation; use net.JoinHostPort (L-011)")
}
}
func mustReadFile(t *testing.T, path string) string {
t.Helper()
body, err := osReadFile(path)
if err != nil {
t.Fatalf("read %s: %v", path, err)
}
return string(body)
}
+8 -6
View File
@@ -130,9 +130,11 @@ func (r *CertificateRepository) List(ctx context.Context, filter *repository.Cer
return nil, 0, fmt.Errorf("failed to count certificates: %w", err)
}
// Determine sort field and direction
// Determine sort field and direction. Bundle E / Audit L-020:
// sortDir is set unconditionally below by the SortDesc branch; the
// previous initial value was an ineffectual assignment (CWE-563).
sortField := "created_at"
sortDir := "DESC"
var sortDir string
sortFieldMap := map[string]string{
"notAfter": "expires_at",
"expiresAt": "expires_at",
@@ -163,16 +165,16 @@ func (r *CertificateRepository) List(ctx context.Context, filter *repository.Cer
var limitClause string
var offset int
if filter.Cursor != "" {
// Cursor-based pagination
// Cursor-based pagination. Bundle E / Audit L-020: argCount is
// not read past this point so the post-increment is dropped.
limitClause = fmt.Sprintf("LIMIT $%d", argCount)
args = append(args, pageSize)
argCount++
} else {
// Page-based pagination
// Page-based pagination. Bundle E / Audit L-020: same as above
// for the +=2 post-increment.
offset = (filter.Page - 1) * pageSize
limitClause = fmt.Sprintf("LIMIT $%d OFFSET $%d", argCount, argCount+1)
args = append(args, pageSize, offset)
argCount += 2
}
query := fmt.Sprintf(`
+6 -6
View File
@@ -29,12 +29,12 @@ func NewAgentGroupService(
// ListAgentGroups returns paginated agent groups (handler interface method).
func (s *AgentGroupService) ListAgentGroups(ctx context.Context, page, perPage int) ([]domain.AgentGroup, int64, error) {
if page < 1 {
page = 1
}
if perPage < 1 {
perPage = 50
}
// Bundle E / Audit L-020: page/perPage are unused; the underlying repo
// List() does not yet take pagination params. Marked explicitly so
// ineffassign sees no dead store and future maintainers see the
// vestigial params rather than a misleading default-applied clamp.
_ = page
_ = perPage
groups, err := s.groupRepo.List(ctx)
if err != nil {
+6 -6
View File
@@ -629,12 +629,12 @@ func (s *IssuerService) buildEnvVarSeeds(cfg *config.Config) []*domain.Issuer {
// ListIssuers returns paginated issuers (handler interface method).
func (s *IssuerService) ListIssuers(ctx context.Context, page, perPage int) ([]domain.Issuer, int64, error) {
if page < 1 {
page = 1
}
if perPage < 1 {
perPage = 50
}
// Bundle E / Audit L-020: page/perPage are unused; the underlying repo
// List() does not yet take pagination params. Marked explicitly so
// ineffassign sees no dead store and future maintainers see the
// vestigial params rather than a misleading default-applied clamp.
_ = page
_ = perPage
issuers, err := s.issuerRepo.List(ctx)
if err != nil {
+6 -6
View File
@@ -127,12 +127,12 @@ func (s *OwnerService) Delete(ctx context.Context, id string, actor string) erro
// ListOwners returns paginated owners (handler interface method).
func (s *OwnerService) ListOwners(ctx context.Context, page, perPage int) ([]domain.Owner, int64, error) {
if page < 1 {
page = 1
}
if perPage < 1 {
perPage = 50
}
// Bundle E / Audit L-020: page/perPage are unused; the underlying repo
// List() does not yet take pagination params. Marked explicitly so
// ineffassign sees no dead store and future maintainers see the
// vestigial params rather than a misleading default-applied clamp.
_ = page
_ = perPage
owners, err := s.ownerRepo.List(ctx)
if err != nil {
+6 -6
View File
@@ -29,12 +29,12 @@ func NewProfileService(
// ListProfiles returns all profiles (handler interface method).
func (s *ProfileService) ListProfiles(ctx context.Context, page, perPage int) ([]domain.CertificateProfile, int64, error) {
if page < 1 {
page = 1
}
if perPage < 1 {
perPage = 50
}
// Bundle E / Audit L-020: page/perPage are unused; the underlying repo
// List() does not yet take pagination params. Marked explicitly so
// ineffassign sees no dead store and future maintainers see the
// vestigial params rather than a misleading default-applied clamp.
_ = page
_ = perPage
profiles, err := s.profileRepo.List(ctx)
if err != nil {
+6 -6
View File
@@ -263,12 +263,12 @@ func (s *TargetService) TestConnection(ctx context.Context, id string) error {
// ListTargets returns paginated targets (handler interface method).
func (s *TargetService) ListTargets(ctx context.Context, page, perPage int) ([]domain.DeploymentTarget, int64, error) {
if page < 1 {
page = 1
}
if perPage < 1 {
perPage = 50
}
// Bundle E / Audit L-020: page/perPage are unused; the underlying repo
// List() does not yet take pagination params. Marked explicitly so
// ineffassign sees no dead store and future maintainers see the
// vestigial params rather than a misleading default-applied clamp.
_ = page
_ = perPage
targets, err := s.targetRepo.List(ctx)
if err != nil {
+6 -6
View File
@@ -127,12 +127,12 @@ func (s *TeamService) Delete(ctx context.Context, id string, actor string) error
// ListTeams returns paginated teams (handler interface method).
func (s *TeamService) ListTeams(ctx context.Context, page, perPage int) ([]domain.Team, int64, error) {
if page < 1 {
page = 1
}
if perPage < 1 {
perPage = 50
}
// Bundle E / Audit L-020: page/perPage are unused; the underlying repo
// List() does not yet take pagination params. Marked explicitly so
// ineffassign sees no dead store and future maintainers see the
// vestigial params rather than a misleading default-applied clamp.
_ = page
_ = perPage
teams, err := s.teamRepo.List(ctx)
if err != nil {