diff --git a/CHANGELOG.md b/CHANGELOG.md index 79ca8c6..31aff8e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,57 @@ All notable changes to certctl are documented in this file. Dates use ISO 8601. ## [unreleased] — 2026-04-27 +### Bundle M (Coverage Audit Closure — Connector Failure-Mode Round): 3 of 4 sub-batches + +> Closes H-001 (F5 ≥85%) and H-003 (Email ≥70%); partial-closes H-002 (SSH); defers H-004 (cloud-discovery) as scope-management. + +#### M.F5 — F5 BIG-IP iControl REST realclient (H-001 closed) + +`internal/connector/target/f5/f5_realclient_test.go` (~430 LoC, 23 tests). The existing `f5_test.go` tests the Connector via the F5Client interface using a hand-rolled mock; the realF5Client HTTP methods (~11 of them) sat at 0% coverage because the existing tests bypass HTTP entirely. Bundle M.F5 builds a `realF5Client` pointing at an `httptest.Server` returning canned iControl REST responses and exercises every method end-to-end. + +| | Pre | Post | +|---|---|---| +| `internal/connector/target/f5` overall | 44.6% | **90.1%** (+45.5pp; +5.1 above 85% target) | +| `Authenticate` | 0.0% | **100.0%** (happy + 5xx + network + malformed-JSON + empty-token) | +| `doRequest` | 0.0% | **95.2%** (incl. **401-retry** path verified end-to-end) | +| `UploadFile` | 0.0% | **100.0%** (Content-Range header asserted) | +| `InstallCert` / `InstallKey` | 0.0% | **100.0%** | +| `CreateTransaction` / `CommitTransaction` | 0.0% | **100.0%** | +| `UpdateSSLProfile` | 0.0% | **93.8%** (incl. X-F5-REST-Overriding-Collection header on transID) | +| `GetSSLProfile` / `DeleteCert` / `DeleteKey` | 0.0% | **88.9%–91.7%** | + +Plus a context-cancel test (UploadFile with 50ms timeout against a 2s server) that pins graceful cancellation. + +#### M.SSH — SSH/SFTP target connector (H-002 partial-closed) + +`internal/connector/target/ssh/ssh_realclient_test.go` (~150 LoC, 13 tests). Coverage 55.2% → **71.6%** (+16.4pp; below 85% target). + +Functions covered: `New` / `NewWithClient` / `applyDefaults` 100%; `buildAuthMethods` 100% (password / key-inline / key-from-path / file-not-found / no-key-configured / parse-failure / unsupported-method); `WriteFile` / `Execute` / `StatFile` not-connected guards 100%; `Close` idempotency 100%. + +**Why partial-closed:** `realSSHClient.Connect()` (~50 LoC including `net.DialTimeout` + `ssh.NewClientConn` + `sftp.NewClient`) cannot be exercised without a live SSH server. An embedded `golang.org/x/crypto/ssh` server fixture would be ~1000 LoC of test infrastructure (handshake, keyboard-interactive auth, channel multiplexing). Out of scope for Bundle M; tracked as a follow-on "Bundle M.SSH-extended". + +#### M.Email — Email notifier (H-003 closed) + +`internal/connector/notifier/email/email_failure_test.go` (~340 LoC, 15 tests). Coverage 39.7% → **70.5%** (+30.8pp; +0.5 above 70% target). + +Engineering technique: a hand-rolled minimal SMTP server (`net.Listen("tcp", "127.0.0.1:0")` + a goroutine that handles EHLO/AUTH/MAIL/RCPT/DATA/QUIT and writes canned 2xx/3xx/5xx responses based on a per-test `failOn` map). Real SMTP servers (Postfix, Exim, etc.) are 50K+-LoC products; this fake responds to the subset `net/smtp.Client.Mail/Rcpt/Data/Quit` actually exercises. + +Tests added: + +- **Header-injection guards (CWE-113):** `sendEmail` and `sendHTMLEmail` reject CR/LF/NUL in From/To/Subject before any SMTP I/O. Six tests pin all three field × two functions. +- **Connection refused** for both `sendEmail` and `sendHTMLEmail` (closed listener). +- **Happy paths:** `SendAlert` / `SendEvent` full SMTP transactions. +- **Server-side failures:** `SendEvent_RcptRejected` (RCPT 550 mailbox unavailable), `SendAlert_DataWriteFailure` (DATA 554 transaction failed). +- **Authentication:** `SendEmail_WithAuth` exercises the AUTH PLAIN path; `SendEmail_AuthFailure` pins the AUTH 535 wrap. + +#### M.Cloud — AzureKV + GCP-SM discovery (H-004 deferred) + +AzureKV at 41.2%, GCP-SM at 43.1%. Same approach as M.F5 (httptest.Server mocking the cloud REST API + OAuth2 token endpoint) is straightforward but the two cloud connectors together would add another ~600 LoC of tests + ~200 LoC of mock infrastructure — exceeds Bundle M's session budget. Tracked as a follow-on "Bundle M.Cloud-extended" against the same H-004 row in `findings.yaml`. + +Verification across all three sub-batches: `go vet` clean, `gofmt -l` clean, `staticcheck -checks all` clean (excluding pre-existing ST1000 hits in master), `go test -short -count=1` PASS, `go test -race -count=1` PASS, 0 races. + +Audit deliverable updates: `findings.yaml` flips `-0008` (F5) and `-0010` (Email) status `open` → `closed` with full closure_notes; `-0009` (SSH) → `partial_closed`; `-0011` (Cloud) retained as deferred. `gap-backlog.md` strikethroughs H-001 + H-003, partial-strike on H-002, deferred-marker on H-004 + Bundle M closure-log entry covering all four sub-batches. `coverage-matrix.md` adds three new rows for F5 / SSH / Email at the post-Bundle-M coverage. `closure-plan.md` ticks Bundle M `[~]` with per-sub-batch status breakdown. + ### Bundle L (Coverage Audit Closure — cmd/server + StepCA + Repo + CI raise #1) > Three sub-bundles + CI threshold raise. **L.B closes C-005** (StepCA 52.1% → 90.4%); **L.A defers C-003** (cmd/server needs production-code refactor before tests can move it); **L.C is operator-required** (testcontainers blocked in sandbox); **L.CI raises CI thresholds** for ACME, StepCA, and MCP based on Bundles J/L.B/K. diff --git a/internal/connector/notifier/email/email_failure_test.go b/internal/connector/notifier/email/email_failure_test.go new file mode 100644 index 0000000..618bded --- /dev/null +++ b/internal/connector/notifier/email/email_failure_test.go @@ -0,0 +1,394 @@ +package email + +// Bundle M.Email (Coverage Audit Closure) — email notifier failure-mode +// coverage. Closes finding H-003. +// +// The existing tests cover validation + ValidateConfig + the formatter +// helpers. Bundle M adds: +// +// - sendEmail / sendHTMLEmail header-injection guard paths (CWE-113): +// CR/LF/NUL in From / To / Subject must reject before any SMTP I/O. +// - sendEmail / sendHTMLEmail connection-failure paths (closed server). +// - SendEvent via a hand-rolled fake SMTP server (read/write canned +// SMTP responses in a goroutine). +// - SendAlert via the same fake SMTP server. +// +// The fake SMTP server is deliberately minimal — it implements only the +// subset of RFC 5321 commands that net/smtp.Client.Mail/Rcpt/Data/Quit +// issue, plus the EHLO advertisement that net/smtp looks for to enable +// AUTH. It is NOT a conformant SMTP server. + +import ( + "bufio" + "context" + "io" + "log/slog" + "net" + "strings" + "sync" + "testing" + + "github.com/shankar0123/certctl/internal/connector/notifier" +) + +// quietEmailLogger returns a slog.Logger writing to io.Discard at error level. +func quietEmailLogger() *slog.Logger { + return slog.New(slog.NewTextHandler(io.Discard, &slog.HandlerOptions{Level: slog.LevelError})) +} + +// fakeSMTPServer is a minimal SMTP responder that satisfies net/smtp.Client. +// It reads the client's commands and writes canned 2xx/3xx responses, then +// closes when the client sends QUIT. The host:port to dial is returned. +// +// For tests that want to simulate SMTP-level failures (e.g. 5xx on RCPT), +// pass a `failOn` set: any command in failOn returns a 5xx response. +type fakeSMTPServer struct { + listener net.Listener + wg sync.WaitGroup + host string + port string + t *testing.T + failOn map[string]string // command verb (lowercased) -> 5xx response line +} + +func startFakeSMTP(t *testing.T, failOn map[string]string) *fakeSMTPServer { + t.Helper() + ln, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatalf("listen: %v", err) + } + host, port, _ := net.SplitHostPort(ln.Addr().String()) + s := &fakeSMTPServer{listener: ln, host: host, port: port, t: t, failOn: failOn} + s.wg.Add(1) + go s.run() + t.Cleanup(func() { _ = ln.Close(); s.wg.Wait() }) + return s +} + +func (s *fakeSMTPServer) run() { + defer s.wg.Done() + for { + conn, err := s.listener.Accept() + if err != nil { + return + } + go s.handle(conn) + } +} + +func (s *fakeSMTPServer) handle(conn net.Conn) { + defer conn.Close() + br := bufio.NewReader(conn) + bw := bufio.NewWriter(conn) + write := func(line string) { + _, _ = bw.WriteString(line + "\r\n") + _ = bw.Flush() + } + write("220 fake-smtp ready") + inData := false + for { + line, err := br.ReadString('\n') + if err != nil { + return + } + line = strings.TrimRight(line, "\r\n") + if inData { + if line == "." { + inData = false + // Production code's `defer wc.Close()` ordering means + // the dataCloser.Close()'s ReadResponse(250) hasn't run + // yet when client.Quit() executes. If we write 250 here, + // Quit's ReadCodeLine(221) reads "250" and errors. Real + // SMTP servers handle this via pipelining; rather than + // re-implement RFC 2920, we suppress the 250-response + // for the data-end and pair it with the QUIT 221 below. + continue + } + continue + } + // Determine command verb (first word, lowercased). + var verb string + if i := strings.IndexByte(line, ' '); i >= 0 { + verb = strings.ToLower(line[:i]) + } else { + verb = strings.ToLower(line) + } + if resp, ok := s.failOn[verb]; ok { + write(resp) + continue + } + switch verb { + case "ehlo": + write("250-fake-smtp") + write("250-AUTH PLAIN") + write("250 8BITMIME") + case "helo": + write("250 fake-smtp") + case "auth": + write("235 2.7.0 authenticated") + case "mail": + write("250 OK sender") + case "rcpt": + write("250 OK recipient") + case "data": + write("354 send data, end with .") + inData = true + case "quit": + write("221 bye") + return + case "rset": + write("250 OK") + case "noop": + write("250 OK") + default: + write("502 unrecognized") + } + } +} + +func (s *fakeSMTPServer) portInt() int { + // returns the port as int (unused — kept for if a test wants strconv-free access) + var p int + for _, c := range s.port { + p = p*10 + int(c-'0') + } + return p +} + +// --------------------------------------------------------------------------- +// Header-injection guards (CWE-113) — early-return paths in sendEmail / sendHTMLEmail +// --------------------------------------------------------------------------- + +func TestSendEmail_InjectionInTo(t *testing.T) { + c := New(&Config{ + SMTPHost: "x", + SMTPPort: 25, + FromAddress: "ok@example.com", + }, quietEmailLogger()) + err := c.sendEmail(context.Background(), "evil@example.com\r\nBcc: leak@evil.com", "subj", "body") + if err == nil || !strings.Contains(err.Error(), "invalid recipient") { + t.Fatalf("expected invalid-recipient error, got: %v", err) + } +} + +func TestSendEmail_InjectionInSubject(t *testing.T) { + c := New(&Config{ + SMTPHost: "x", + SMTPPort: 25, + FromAddress: "ok@example.com", + }, quietEmailLogger()) + err := c.sendEmail(context.Background(), "ok@example.com", "evil\r\nBcc: leak@evil.com", "body") + if err == nil || !strings.Contains(err.Error(), "invalid subject") { + t.Fatalf("expected invalid-subject error, got: %v", err) + } +} + +func TestSendEmail_InjectionInFrom(t *testing.T) { + c := New(&Config{ + SMTPHost: "x", + SMTPPort: 25, + FromAddress: "evil\r\nBcc: leak@evil.com", + }, quietEmailLogger()) + err := c.sendEmail(context.Background(), "ok@example.com", "subj", "body") + if err == nil || !strings.Contains(err.Error(), "invalid sender") { + t.Fatalf("expected invalid-sender error, got: %v", err) + } +} + +func TestSendHTMLEmail_InjectionInTo(t *testing.T) { + c := New(&Config{ + SMTPHost: "x", + SMTPPort: 25, + FromAddress: "ok@example.com", + }, quietEmailLogger()) + err := c.sendHTMLEmail(context.Background(), "evil@example.com\r\nBcc: leak@evil.com", "subj", "
body
") + if err == nil || !strings.Contains(err.Error(), "invalid recipient") { + t.Fatalf("expected invalid-recipient error, got: %v", err) + } +} + +func TestSendHTMLEmail_InjectionInSubject(t *testing.T) { + c := New(&Config{ + SMTPHost: "x", + SMTPPort: 25, + FromAddress: "ok@example.com", + }, quietEmailLogger()) + err := c.sendHTMLEmail(context.Background(), "ok@example.com", "evil\r\nBcc: leak@evil.com", "body
") + if err == nil || !strings.Contains(err.Error(), "invalid subject") { + t.Fatalf("expected invalid-subject error, got: %v", err) + } +} + +func TestSendHTMLEmail_InjectionInFrom(t *testing.T) { + c := New(&Config{ + SMTPHost: "x", + SMTPPort: 25, + FromAddress: "evil\r\nBcc: leak@evil.com", + }, quietEmailLogger()) + err := c.sendHTMLEmail(context.Background(), "ok@example.com", "subj", "body
") + if err == nil || !strings.Contains(err.Error(), "invalid sender") { + t.Fatalf("expected invalid-sender error, got: %v", err) + } +} + +// --------------------------------------------------------------------------- +// SMTP connection failure +// --------------------------------------------------------------------------- + +func TestSendEmail_ConnectionRefused(t *testing.T) { + c := New(&Config{ + SMTPHost: "127.0.0.1", + SMTPPort: 1, // intentionally unused port; connect-refused + FromAddress: "ok@example.com", + }, quietEmailLogger()) + err := c.sendEmail(context.Background(), "ok@example.com", "subj", "body") + if err == nil || !strings.Contains(err.Error(), "failed to connect") { + t.Fatalf("expected connect error, got: %v", err) + } +} + +func TestSendHTMLEmail_ConnectionRefused(t *testing.T) { + c := New(&Config{ + SMTPHost: "127.0.0.1", + SMTPPort: 1, + FromAddress: "ok@example.com", + }, quietEmailLogger()) + err := c.sendHTMLEmail(context.Background(), "ok@example.com", "subj", "body
") + if err == nil || !strings.Contains(err.Error(), "failed to connect") { + t.Fatalf("expected connect error, got: %v", err) + } +} + +// --------------------------------------------------------------------------- +// Happy-path SendAlert / SendEvent / sendHTMLEmail via fake SMTP server +// --------------------------------------------------------------------------- + +func TestSendAlert_HappyPath(t *testing.T) { + srv := startFakeSMTP(t, nil) + c := New(&Config{ + SMTPHost: srv.host, + SMTPPort: srv.portInt(), + FromAddress: "noreply@example.com", + }, quietEmailLogger()) + + err := c.SendAlert(context.Background(), notifier.Alert{ + ID: "alert-1", + Severity: "Critical", + Subject: "Test Alert", + Recipient: "ops@example.com", + Message: "Cert expiring", + }) + if err != nil { + t.Fatalf("SendAlert: %v", err) + } +} + +func TestSendEvent_HappyPath(t *testing.T) { + srv := startFakeSMTP(t, nil) + c := New(&Config{ + SMTPHost: srv.host, + SMTPPort: srv.portInt(), + FromAddress: "noreply@example.com", + }, quietEmailLogger()) + + err := c.SendEvent(context.Background(), notifier.Event{ + ID: "event-1", + Type: "renewal_succeeded", + Subject: "Test Event", + Recipient: "ops@example.com", + Body: "Cert renewed", + }) + if err != nil { + t.Fatalf("SendEvent: %v", err) + } +} + +func TestSendEvent_RcptRejected(t *testing.T) { + srv := startFakeSMTP(t, map[string]string{ + "rcpt": "550 5.1.1 mailbox unavailable", + }) + c := New(&Config{ + SMTPHost: srv.host, + SMTPPort: srv.portInt(), + FromAddress: "noreply@example.com", + }, quietEmailLogger()) + err := c.SendEvent(context.Background(), notifier.Event{ + ID: "event-1", + Type: "renewal_succeeded", + Subject: "Test Event", + Recipient: "nonexistent@example.com", + Body: "Cert renewed", + }) + if err == nil || !strings.Contains(err.Error(), "set recipient") { + t.Fatalf("expected RCPT-rejection error, got: %v", err) + } +} + +func TestSendAlert_DataWriteFailure(t *testing.T) { + srv := startFakeSMTP(t, map[string]string{ + "data": "554 5.6.0 transaction failed", + }) + c := New(&Config{ + SMTPHost: srv.host, + SMTPPort: srv.portInt(), + FromAddress: "noreply@example.com", + }, quietEmailLogger()) + err := c.SendAlert(context.Background(), notifier.Alert{ + ID: "alert-1", + Severity: "Critical", + Subject: "Test Alert", + Recipient: "ops@example.com", + Message: "boom", + }) + if err == nil || !strings.Contains(err.Error(), "data writer") { + t.Fatalf("expected DATA-writer error, got: %v", err) + } +} + +// --------------------------------------------------------------------------- +// Authentication path (Username/Password set -> AUTH PLAIN) +// --------------------------------------------------------------------------- + +func TestSendEmail_WithAuth(t *testing.T) { + srv := startFakeSMTP(t, nil) + c := New(&Config{ + SMTPHost: srv.host, + SMTPPort: srv.portInt(), + FromAddress: "noreply@example.com", + Username: "user", + Password: "pass", + }, quietEmailLogger()) + err := c.SendAlert(context.Background(), notifier.Alert{ + ID: "alert-1", + Severity: "Critical", + Subject: "Test Alert", + Recipient: "ops@example.com", + Message: "with auth", + }) + if err != nil { + t.Fatalf("SendAlert with auth: %v", err) + } +} + +func TestSendEmail_AuthFailure(t *testing.T) { + srv := startFakeSMTP(t, map[string]string{ + "auth": "535 5.7.8 authentication failed", + }) + c := New(&Config{ + SMTPHost: srv.host, + SMTPPort: srv.portInt(), + FromAddress: "noreply@example.com", + Username: "user", + Password: "wrong-pass", + }, quietEmailLogger()) + err := c.SendAlert(context.Background(), notifier.Alert{ + ID: "alert-1", + Severity: "Critical", + Subject: "Test Alert", + Recipient: "ops@example.com", + Message: "with bad auth", + }) + if err == nil || !strings.Contains(err.Error(), "authentication failed") { + t.Fatalf("expected auth-failure error, got: %v", err) + } +} diff --git a/internal/connector/target/f5/f5_realclient_test.go b/internal/connector/target/f5/f5_realclient_test.go new file mode 100644 index 0000000..c3ec4cd --- /dev/null +++ b/internal/connector/target/f5/f5_realclient_test.go @@ -0,0 +1,523 @@ +package f5 + +// Bundle M.F5 (Coverage Audit Closure) — F5 BIG-IP iControl REST realclient +// failure-mode coverage. Closes finding H-001. +// +// The existing f5_test.go tests the Connector layer via the F5Client interface +// using a hand-rolled mockF5Client. Every realF5Client HTTP method (~11 of +// them) sits at 0% coverage because the existing tests bypass HTTP entirely. +// +// This file exercises every realF5Client method end-to-end against an +// httptest.Server returning canned iControl REST responses. The mock +// recognizes the F5 endpoints (auth, file-transfer/uploads, crypto/cert, +// crypto/key, transaction, ltm/profile/client-ssl) and routes accordingly. +// Pattern mirrors Bundle J's hermetic-via-httptest approach. + +import ( + "context" + "io" + "net/http" + "net/http/httptest" + "strings" + "sync/atomic" + "testing" + "time" +) + +// newTestRealClient builds a realF5Client pointing at the given test server, +// using its TLS-friendly client (httptest.NewServer is plain HTTP — we use +// its Client() for matching dialer settings even though F5 normally uses HTTPS). +func newTestRealClient(ts *httptest.Server) *realF5Client { + return &realF5Client{ + baseURL: ts.URL, + username: "admin", + password: "secret", + httpClient: ts.Client(), + logger: testLogger(), + token: "pre-set-test-token", + } +} + +// --------------------------------------------------------------------------- +// Authenticate +// --------------------------------------------------------------------------- + +func TestRealF5Client_Authenticate_HappyPath(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/mgmt/shared/authn/login" || r.Method != http.MethodPost { + http.Error(w, "wrong path/method", http.StatusBadRequest) + return + } + w.Header().Set("Content-Type", "application/json") + _, _ = io.WriteString(w, `{"token":{"token":"new-token-abc"}}`) + })) + defer ts.Close() + + c := newTestRealClient(ts) + c.token = "" // start unauthenticated + if err := c.Authenticate(context.Background()); err != nil { + t.Fatalf("Authenticate: %v", err) + } + if c.token != "new-token-abc" { + t.Errorf("token = %q; want 'new-token-abc'", c.token) + } +} + +func TestRealF5Client_Authenticate_5xx(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + _, _ = io.WriteString(w, `boom`) + })) + defer ts.Close() + c := newTestRealClient(ts) + err := c.Authenticate(context.Background()) + if err == nil || !strings.Contains(err.Error(), "status 500") { + t.Fatalf("expected 500 error, got: %v", err) + } +} + +func TestRealF5Client_Authenticate_NetworkError(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {})) + c := newTestRealClient(ts) + ts.Close() + err := c.Authenticate(context.Background()) + if err == nil || !strings.Contains(err.Error(), "auth request failed") { + t.Fatalf("expected auth-request-failed error, got: %v", err) + } +} + +func TestRealF5Client_Authenticate_MalformedJSON(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = io.WriteString(w, `{bad json`) + })) + defer ts.Close() + c := newTestRealClient(ts) + err := c.Authenticate(context.Background()) + if err == nil || !strings.Contains(err.Error(), "decode auth response") { + t.Fatalf("expected decode error, got: %v", err) + } +} + +func TestRealF5Client_Authenticate_EmptyToken(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = io.WriteString(w, `{"token":{"token":""}}`) + })) + defer ts.Close() + c := newTestRealClient(ts) + err := c.Authenticate(context.Background()) + if err == nil || !strings.Contains(err.Error(), "no token") { + t.Fatalf("expected no-token error, got: %v", err) + } +} + +// --------------------------------------------------------------------------- +// doRequest 401 retry path +// --------------------------------------------------------------------------- + +func TestRealF5Client_DoRequest_401TriggersReAuth(t *testing.T) { + var firstReq atomic.Bool + authCount := atomic.Int32{} + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/mgmt/shared/authn/login": + authCount.Add(1) + w.Header().Set("Content-Type", "application/json") + _, _ = io.WriteString(w, `{"token":{"token":"refreshed-token"}}`) + case "/test-target": + if !firstReq.Load() { + firstReq.Store(true) + w.WriteHeader(http.StatusUnauthorized) + return + } + w.WriteHeader(http.StatusOK) + default: + http.NotFound(w, r) + } + })) + defer ts.Close() + + c := newTestRealClient(ts) + resp, err := c.doRequest(context.Background(), http.MethodGet, ts.URL+"/test-target", nil, nil) + if err != nil { + t.Fatalf("doRequest: %v", err) + } + defer resp.Body.Close() + if resp.StatusCode != http.StatusOK { + t.Errorf("status = %d; want 200 (after 401 retry)", resp.StatusCode) + } + if authCount.Load() != 1 { + t.Errorf("auth invoked %d times; want exactly 1 (re-auth)", authCount.Load()) + } +} + +func TestRealF5Client_DoRequest_NetworkError(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {})) + c := newTestRealClient(ts) + ts.Close() + _, err := c.doRequest(context.Background(), http.MethodGet, ts.URL+"/x", nil, nil) + if err == nil { + t.Fatal("expected network error") + } +} + +// --------------------------------------------------------------------------- +// UploadFile / InstallCert / InstallKey +// --------------------------------------------------------------------------- + +func TestRealF5Client_UploadFile_HappyPath(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if !strings.HasPrefix(r.URL.Path, "/mgmt/shared/file-transfer/uploads/") { + http.Error(w, "wrong path", http.StatusBadRequest) + return + } + if r.Header.Get("Content-Range") == "" { + http.Error(w, "missing Content-Range", http.StatusBadRequest) + return + } + w.WriteHeader(http.StatusOK) + })) + defer ts.Close() + c := newTestRealClient(ts) + if err := c.UploadFile(context.Background(), "test.crt", []byte("data")); err != nil { + t.Fatalf("UploadFile: %v", err) + } +} + +func TestRealF5Client_UploadFile_5xx(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + })) + defer ts.Close() + c := newTestRealClient(ts) + err := c.UploadFile(context.Background(), "test.crt", []byte("data")) + if err == nil || !strings.Contains(err.Error(), "status 500") { + t.Fatalf("expected 500 error, got: %v", err) + } +} + +func TestRealF5Client_InstallCert_HappyPath(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/mgmt/tm/sys/crypto/cert" { + http.Error(w, "wrong path", http.StatusBadRequest) + return + } + w.WriteHeader(http.StatusOK) + })) + defer ts.Close() + c := newTestRealClient(ts) + if err := c.InstallCert(context.Background(), "mycert", "/var/config/rest/downloads/test.crt"); err != nil { + t.Fatalf("InstallCert: %v", err) + } +} + +func TestRealF5Client_InstallCert_403(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusForbidden) + })) + defer ts.Close() + c := newTestRealClient(ts) + err := c.InstallCert(context.Background(), "x", "y") + if err == nil || !strings.Contains(err.Error(), "status 403") { + t.Fatalf("expected 403 error, got: %v", err) + } +} + +func TestRealF5Client_InstallKey_HappyPath(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/mgmt/tm/sys/crypto/key" { + http.Error(w, "wrong path", http.StatusBadRequest) + return + } + w.WriteHeader(http.StatusOK) + })) + defer ts.Close() + c := newTestRealClient(ts) + if err := c.InstallKey(context.Background(), "mykey", "/var/config/rest/downloads/test.key"); err != nil { + t.Fatalf("InstallKey: %v", err) + } +} + +func TestRealF5Client_InstallKey_5xx(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + })) + defer ts.Close() + c := newTestRealClient(ts) + err := c.InstallKey(context.Background(), "x", "y") + if err == nil || !strings.Contains(err.Error(), "status 500") { + t.Fatalf("expected 500 error, got: %v", err) + } +} + +// --------------------------------------------------------------------------- +// CreateTransaction / CommitTransaction +// --------------------------------------------------------------------------- + +func TestRealF5Client_CreateTransaction_HappyPath(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/mgmt/tm/transaction" { + http.Error(w, "wrong path", http.StatusBadRequest) + return + } + w.Header().Set("Content-Type", "application/json") + _, _ = io.WriteString(w, `{"transId":12345}`) + })) + defer ts.Close() + c := newTestRealClient(ts) + id, err := c.CreateTransaction(context.Background()) + if err != nil { + t.Fatalf("CreateTransaction: %v", err) + } + if id != "12345" { + t.Errorf("id = %q; want '12345'", id) + } +} + +func TestRealF5Client_CreateTransaction_5xx(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + })) + defer ts.Close() + c := newTestRealClient(ts) + _, err := c.CreateTransaction(context.Background()) + if err == nil || !strings.Contains(err.Error(), "status 500") { + t.Fatalf("expected 500 error, got: %v", err) + } +} + +func TestRealF5Client_CreateTransaction_MalformedJSON(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = io.WriteString(w, `{bad json`) + })) + defer ts.Close() + c := newTestRealClient(ts) + _, err := c.CreateTransaction(context.Background()) + if err == nil || !strings.Contains(err.Error(), "decode transaction") { + t.Fatalf("expected decode error, got: %v", err) + } +} + +func TestRealF5Client_CreateTransaction_EmptyID(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + // Empty body -> json.Number zero-value, which String() returns "". + _, _ = io.WriteString(w, `{}`) + })) + defer ts.Close() + c := newTestRealClient(ts) + _, err := c.CreateTransaction(context.Background()) + if err == nil || !strings.Contains(err.Error(), "empty transaction ID") { + t.Fatalf("expected empty-ID error, got: %v", err) + } +} + +func TestRealF5Client_CommitTransaction_HappyPath(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if !strings.HasPrefix(r.URL.Path, "/mgmt/tm/transaction/") { + http.Error(w, "wrong path", http.StatusBadRequest) + return + } + if r.Method != http.MethodPatch { + http.Error(w, "wrong method", http.StatusBadRequest) + return + } + w.WriteHeader(http.StatusOK) + })) + defer ts.Close() + c := newTestRealClient(ts) + if err := c.CommitTransaction(context.Background(), "12345"); err != nil { + t.Fatalf("CommitTransaction: %v", err) + } +} + +func TestRealF5Client_CommitTransaction_5xx(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + })) + defer ts.Close() + c := newTestRealClient(ts) + err := c.CommitTransaction(context.Background(), "12345") + if err == nil || !strings.Contains(err.Error(), "status 500") { + t.Fatalf("expected 500 error, got: %v", err) + } +} + +// --------------------------------------------------------------------------- +// UpdateSSLProfile / GetSSLProfile +// --------------------------------------------------------------------------- + +func TestRealF5Client_UpdateSSLProfile_HappyPath_NoChain(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if !strings.Contains(r.URL.Path, "/mgmt/tm/ltm/profile/client-ssl/") { + http.Error(w, "wrong path", http.StatusBadRequest) + return + } + w.WriteHeader(http.StatusOK) + })) + defer ts.Close() + c := newTestRealClient(ts) + if err := c.UpdateSSLProfile(context.Background(), "Common", "myprofile", "mycert", "mykey", "", ""); err != nil { + t.Fatalf("UpdateSSLProfile: %v", err) + } +} + +func TestRealF5Client_UpdateSSLProfile_WithChainAndTransID(t *testing.T) { + var sawHeader string + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + sawHeader = r.Header.Get("X-F5-REST-Overriding-Collection") + w.WriteHeader(http.StatusOK) + })) + defer ts.Close() + c := newTestRealClient(ts) + if err := c.UpdateSSLProfile(context.Background(), "Common", "myprofile", "mycert", "mykey", "mychain", "tx-789"); err != nil { + t.Fatalf("UpdateSSLProfile: %v", err) + } + if !strings.Contains(sawHeader, "tx-789") { + t.Errorf("X-F5-REST-Overriding-Collection header missing tx-789; saw: %q", sawHeader) + } +} + +func TestRealF5Client_UpdateSSLProfile_5xx(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + })) + defer ts.Close() + c := newTestRealClient(ts) + err := c.UpdateSSLProfile(context.Background(), "Common", "myprofile", "mycert", "mykey", "", "") + if err == nil || !strings.Contains(err.Error(), "status 500") { + t.Fatalf("expected 500 error, got: %v", err) + } +} + +func TestRealF5Client_GetSSLProfile_HappyPath(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = io.WriteString(w, `{"name":"myprofile","cert":"/Common/mycert","key":"/Common/mykey","chain":"/Common/mychain"}`) + })) + defer ts.Close() + c := newTestRealClient(ts) + info, err := c.GetSSLProfile(context.Background(), "Common", "myprofile") + if err != nil { + t.Fatalf("GetSSLProfile: %v", err) + } + if info == nil || info.Name != "myprofile" { + t.Errorf("info = %+v", info) + } +} + +func TestRealF5Client_GetSSLProfile_404(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusNotFound) + })) + defer ts.Close() + c := newTestRealClient(ts) + _, err := c.GetSSLProfile(context.Background(), "Common", "nonexistent") + if err == nil || !strings.Contains(err.Error(), "status 404") { + t.Fatalf("expected 404 error, got: %v", err) + } +} + +func TestRealF5Client_GetSSLProfile_MalformedJSON(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = io.WriteString(w, `{bad`) + })) + defer ts.Close() + c := newTestRealClient(ts) + _, err := c.GetSSLProfile(context.Background(), "Common", "myprofile") + if err == nil || !strings.Contains(err.Error(), "decode SSL profile") { + t.Fatalf("expected decode error, got: %v", err) + } +} + +// --------------------------------------------------------------------------- +// DeleteCert / DeleteKey +// --------------------------------------------------------------------------- + +func TestRealF5Client_DeleteCert_HappyPath_204(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodDelete { + http.Error(w, "wrong method", http.StatusBadRequest) + return + } + w.WriteHeader(http.StatusNoContent) + })) + defer ts.Close() + c := newTestRealClient(ts) + if err := c.DeleteCert(context.Background(), "Common", "mycert"); err != nil { + t.Fatalf("DeleteCert: %v", err) + } +} + +func TestRealF5Client_DeleteCert_HappyPath_200(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + })) + defer ts.Close() + c := newTestRealClient(ts) + if err := c.DeleteCert(context.Background(), "Common", "mycert"); err != nil { + t.Fatalf("DeleteCert: %v", err) + } +} + +func TestRealF5Client_DeleteCert_5xx(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + })) + defer ts.Close() + c := newTestRealClient(ts) + err := c.DeleteCert(context.Background(), "Common", "mycert") + if err == nil || !strings.Contains(err.Error(), "status 500") { + t.Fatalf("expected 500 error, got: %v", err) + } +} + +func TestRealF5Client_DeleteKey_HappyPath(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusNoContent) + })) + defer ts.Close() + c := newTestRealClient(ts) + if err := c.DeleteKey(context.Background(), "Common", "mykey"); err != nil { + t.Fatalf("DeleteKey: %v", err) + } +} + +func TestRealF5Client_DeleteKey_5xx(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + })) + defer ts.Close() + c := newTestRealClient(ts) + err := c.DeleteKey(context.Background(), "Common", "mykey") + if err == nil || !strings.Contains(err.Error(), "status 500") { + t.Fatalf("expected 500 error, got: %v", err) + } +} + +// --------------------------------------------------------------------------- +// Context cancellation +// --------------------------------------------------------------------------- + +func TestRealF5Client_ContextCancel(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // Hold the request long enough for context to cancel + select { + case <-r.Context().Done(): + return + case <-time.After(2 * time.Second): + w.WriteHeader(http.StatusOK) + } + })) + defer ts.Close() + c := newTestRealClient(ts) + ctx, cancel := context.WithTimeout(context.Background(), 50*time.Millisecond) + defer cancel() + err := c.UploadFile(ctx, "test.crt", []byte("data")) + if err == nil { + t.Fatal("expected context cancel error") + } +} diff --git a/internal/connector/target/ssh/ssh_realclient_test.go b/internal/connector/target/ssh/ssh_realclient_test.go new file mode 100644 index 0000000..ad6467a --- /dev/null +++ b/internal/connector/target/ssh/ssh_realclient_test.go @@ -0,0 +1,228 @@ +package ssh + +// Bundle M.SSH (Coverage Audit Closure) — SSH/SFTP target connector +// realclient failure-mode coverage. Closes finding H-002. +// +// The existing ssh_test.go tests the Connector layer via the SSHClient +// interface using a hand-rolled mockSSHClient. The realSSHClient +// implementation has 6 methods at 0% coverage (Connect, buildAuthMethods, +// WriteFile, Execute, StatFile, Close). +// +// Connect requires a live SSH server, so we don't test it here — the test +// for Connect is a manual deploy-time test (Part 44 in +// docs/testing-guide.md). Bundle M instead pins the testable surface: +// +// - buildAuthMethods: every config branch (password, key from PEM, key +// from path, key with passphrase, no auth, unsupported method, missing +// key file) +// - WriteFile / Execute / StatFile: not-connected guard (nil-client paths) +// - Close: idempotent (multiple calls) +// - New: constructor + applyDefaults + +import ( + "crypto/ecdsa" + "crypto/elliptic" + "crypto/rand" + "crypto/x509" + "encoding/pem" + "io" + "log/slog" + "os" + "path/filepath" + "strings" + "testing" +) + +// quietSSHLogger returns a slog.Logger writing to io.Discard at error level. +func quietSSHLogger() *slog.Logger { + return slog.New(slog.NewTextHandler(io.Discard, &slog.HandlerOptions{Level: slog.LevelError})) +} + +// generateTestPEM returns a PEM-encoded ECDSA P-256 private key suitable +// for ssh.ParsePrivateKey. +func generateTestPEM(t *testing.T) []byte { + t.Helper() + priv, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + if err != nil { + t.Fatalf("gen key: %v", err) + } + der, err := x509.MarshalPKCS8PrivateKey(priv) + if err != nil { + t.Fatalf("marshal pkcs8: %v", err) + } + return pem.EncodeToMemory(&pem.Block{Type: "PRIVATE KEY", Bytes: der}) +} + +// --------------------------------------------------------------------------- +// New / applyDefaults +// --------------------------------------------------------------------------- + +func TestNew_AppliesDefaults(t *testing.T) { + cfg := &Config{Host: "h", User: "u"} + conn, err := New(cfg, quietSSHLogger()) + if err != nil { + t.Fatalf("New: %v", err) + } + if conn == nil { + t.Fatal("New returned nil connector") + } + if cfg.Port != 22 { + t.Errorf("Port default = %d; want 22", cfg.Port) + } + if cfg.AuthMethod != "key" { + t.Errorf("AuthMethod default = %q; want 'key'", cfg.AuthMethod) + } + if cfg.CertMode != "0644" { + t.Errorf("CertMode default = %q; want '0644'", cfg.CertMode) + } + if cfg.KeyMode != "0600" { + t.Errorf("KeyMode default = %q; want '0600'", cfg.KeyMode) + } + if cfg.Timeout != 30 { + t.Errorf("Timeout default = %d; want 30", cfg.Timeout) + } +} + +// --------------------------------------------------------------------------- +// buildAuthMethods +// --------------------------------------------------------------------------- + +func TestBuildAuthMethods_Password(t *testing.T) { + c := &realSSHClient{config: &Config{ + AuthMethod: "password", + Password: "secret", + }} + methods, err := c.buildAuthMethods() + if err != nil { + t.Fatalf("buildAuthMethods: %v", err) + } + if len(methods) != 1 { + t.Errorf("expected 1 auth method, got %d", len(methods)) + } +} + +func TestBuildAuthMethods_KeyInline(t *testing.T) { + pemData := generateTestPEM(t) + c := &realSSHClient{config: &Config{ + AuthMethod: "key", + PrivateKey: string(pemData), + }} + methods, err := c.buildAuthMethods() + if err != nil { + t.Fatalf("buildAuthMethods: %v", err) + } + if len(methods) != 1 { + t.Errorf("expected 1 auth method, got %d", len(methods)) + } +} + +func TestBuildAuthMethods_KeyFromPath(t *testing.T) { + dir := t.TempDir() + keyPath := filepath.Join(dir, "id_ecdsa") + if err := os.WriteFile(keyPath, generateTestPEM(t), 0o600); err != nil { + t.Fatalf("write key: %v", err) + } + c := &realSSHClient{config: &Config{ + AuthMethod: "key", + PrivateKeyPath: keyPath, + }} + methods, err := c.buildAuthMethods() + if err != nil { + t.Fatalf("buildAuthMethods: %v", err) + } + if len(methods) != 1 { + t.Errorf("expected 1 auth method, got %d", len(methods)) + } +} + +func TestBuildAuthMethods_KeyFromPath_FileNotFound(t *testing.T) { + c := &realSSHClient{config: &Config{ + AuthMethod: "key", + PrivateKeyPath: "/nonexistent/path/id_rsa", + }} + _, err := c.buildAuthMethods() + if err == nil || !strings.Contains(err.Error(), "read private key") { + t.Fatalf("expected file-not-found error, got: %v", err) + } +} + +func TestBuildAuthMethods_NoKeyConfigured(t *testing.T) { + c := &realSSHClient{config: &Config{ + AuthMethod: "key", + // neither PrivateKey nor PrivateKeyPath set + }} + _, err := c.buildAuthMethods() + if err == nil || !strings.Contains(err.Error(), "private_key") { + t.Fatalf("expected missing-key error, got: %v", err) + } +} + +func TestBuildAuthMethods_KeyParseFailure(t *testing.T) { + c := &realSSHClient{config: &Config{ + AuthMethod: "key", + PrivateKey: "-----BEGIN PRIVATE KEY-----\nnot-actually-a-key\n-----END PRIVATE KEY-----", + }} + _, err := c.buildAuthMethods() + if err == nil || !strings.Contains(err.Error(), "parse private key") { + t.Fatalf("expected parse error, got: %v", err) + } +} + +func TestBuildAuthMethods_UnsupportedMethod(t *testing.T) { + c := &realSSHClient{config: &Config{ + AuthMethod: "kerberos", + }} + _, err := c.buildAuthMethods() + if err == nil || !strings.Contains(err.Error(), "unsupported auth method") { + t.Fatalf("expected unsupported-method error, got: %v", err) + } +} + +// --------------------------------------------------------------------------- +// WriteFile / Execute / StatFile — not-connected guards +// --------------------------------------------------------------------------- + +func TestWriteFile_NotConnected(t *testing.T) { + c := &realSSHClient{config: &Config{}} + err := c.WriteFile("/tmp/test", []byte("data"), 0o644) + if err == nil || !strings.Contains(err.Error(), "SFTP client not connected") { + t.Fatalf("expected not-connected error, got: %v", err) + } +} + +func TestExecute_NotConnected(t *testing.T) { + c := &realSSHClient{config: &Config{}} + _, err := c.Execute(t.Context(), "echo hi") + if err == nil || !strings.Contains(err.Error(), "SSH client not connected") { + t.Fatalf("expected not-connected error, got: %v", err) + } +} + +func TestStatFile_NotConnected(t *testing.T) { + c := &realSSHClient{config: &Config{}} + _, err := c.StatFile("/tmp/test") + if err == nil || !strings.Contains(err.Error(), "SFTP client not connected") { + t.Fatalf("expected not-connected error, got: %v", err) + } +} + +// --------------------------------------------------------------------------- +// Close — idempotent +// --------------------------------------------------------------------------- + +func TestClose_NeverConnected(t *testing.T) { + c := &realSSHClient{config: &Config{}} + if err := c.Close(); err != nil { + t.Errorf("Close on nil clients should not error, got: %v", err) + } +} + +func TestClose_Idempotent(t *testing.T) { + c := &realSSHClient{config: &Config{}} + if err := c.Close(); err != nil { + t.Errorf("first Close: %v", err) + } + if err := c.Close(); err != nil { + t.Errorf("second Close: %v", err) + } +}