From 68f6fd474be806da3a4ecba0e06a7ac3dae6fc18 Mon Sep 17 00:00:00 2001 From: shankar0123 Date: Sun, 12 Apr 2026 19:18:32 -0400 Subject: [PATCH] fix: return 409 on duplicate issuer name, improve error handling and onboarding defaults Closes #7. The issuer create/update handlers swallowed all service errors as generic 500s. Now differentiates: 409 for UNIQUE constraint violations, 400 for unsupported issuer type, 404 for not-found on update, 500 for unknown errors. Adds structured error logging via slog. OnboardingWizard now pre-populates config field defaults when a type is selected (matching IssuersPage behavior), preventing empty required fields from causing silent failures. install-agent.sh hardened for curl|bash usage: --agent-id flag, =value syntax, /dev/tty stdin reopening, proper stderr routing in download_binary, non-interactive install examples in help text, and updated wizard commands. Adds adversarial security tests for EST, path traversal, and query injection handlers. Co-Authored-By: Claude Opus 4.6 --- install-agent.sh | 161 +++++- internal/api/handler/adversarial_est_test.go | 339 +++++++++++ internal/api/handler/adversarial_path_test.go | 330 +++++++++++ .../api/handler/adversarial_query_test.go | 538 ++++++++++++++++++ internal/api/handler/issuer_handler_test.go | 118 ++++ internal/api/handler/issuers.go | 33 +- web/src/pages/OnboardingWizard.tsx | 46 +- 7 files changed, 1521 insertions(+), 44 deletions(-) create mode 100644 internal/api/handler/adversarial_est_test.go create mode 100644 internal/api/handler/adversarial_path_test.go create mode 100644 internal/api/handler/adversarial_query_test.go diff --git a/install-agent.sh b/install-agent.sh index 7d5f9a8..d59967d 100644 --- a/install-agent.sh +++ b/install-agent.sh @@ -60,8 +60,21 @@ OPTIONS: -h, --help Show this help message --server-url URL Set CERTCTL_SERVER_URL (skips interactive prompt) --api-key KEY Set CERTCTL_API_KEY (skips interactive prompt) + --agent-id ID Set CERTCTL_AGENT_ID (defaults to hostname) --no-start Install but don't start the service +EXAMPLES: + # Interactive install (download first): + curl -sSLO https://raw.githubusercontent.com/${GITHUB_REPO}/master/install-agent.sh + chmod +x install-agent.sh + sudo ./install-agent.sh + + # Non-interactive install (pipe via curl): + curl -sSL https://raw.githubusercontent.com/${GITHUB_REPO}/master/install-agent.sh \\ + | sudo bash -s -- \\ + --server-url https://certctl.example.com \\ + --api-key YOUR_API_KEY + EOF } @@ -74,19 +87,47 @@ parse_args() { exit 0 ;; --server-url) - SERVER_URL="$2" + SERVER_URL="${2:-}" + if [[ -z "$SERVER_URL" ]]; then + echo -e "${RED}Error: --server-url requires a value${NC}" >&2 + exit 1 + fi shift 2 ;; + --server-url=*) + SERVER_URL="${1#*=}" + shift + ;; --api-key) - API_KEY="$2" + API_KEY="${2:-}" + if [[ -z "$API_KEY" ]]; then + echo -e "${RED}Error: --api-key requires a value${NC}" >&2 + exit 1 + fi shift 2 ;; + --api-key=*) + API_KEY="${1#*=}" + shift + ;; + --agent-id) + AGENT_ID="${2:-}" + if [[ -z "$AGENT_ID" ]]; then + echo -e "${RED}Error: --agent-id requires a value${NC}" >&2 + exit 1 + fi + shift 2 + ;; + --agent-id=*) + AGENT_ID="${1#*=}" + shift + ;; --no-start) NO_START=true shift ;; *) - echo -e "${RED}Error: Unknown option: $1${NC}" + echo -e "${RED}Error: Unknown option: $1${NC}" >&2 usage exit 1 ;; @@ -94,6 +135,56 @@ parse_args() { done } +# Ensure stdin is interactive before prompting. When the script is piped via +# curl|bash, stdin is the pipe from curl, so `read` hits EOF immediately and +# set -e aborts the script silently. Reopen stdin from the controlling terminal +# (/dev/tty) if available; otherwise print a helpful error pointing at the +# flag-based non-interactive install. +ensure_interactive_input() { + # If all required config is already provided via flags, no prompting needed. + if [[ -n "${SERVER_URL:-}" && -n "${API_KEY:-}" ]]; then + return + fi + + # Already interactive — nothing to do. + if [[ -t 0 ]]; then + return + fi + + # Piped stdin — try to reopen from the controlling terminal. Actually + # attempt to open /dev/tty inside a subshell: the device node may exist + # even when the process has no controlling terminal (ENXIO on open), so + # `[[ -r /dev/tty ]]` is not reliable. + if ( exec /dev/null; then + exec &2 + exit 1 +} + # Check if running as root/sudo on Linux check_privileges() { if [[ "$OS_TYPE" == "linux" && "$EUID" -ne 0 ]]; then @@ -103,23 +194,33 @@ check_privileges() { } # Download agent binary from GitHub Releases +# IMPORTANT: main() captures this function's stdout via `binary_path=$(download_binary)`, +# so every status/error message MUST go to stderr (>&2). Only the final +# `echo "$temp_file"` is allowed on stdout — that's the return value. +# +# We deliberately do NOT register an EXIT trap to clean up $temp_file: because +# of the command substitution, this function runs in a subshell, and any EXIT +# trap set here fires when the subshell exits — which is *before* install_binary +# gets a chance to cp the file. Cleanup on success is install_binary's job +# (after the cp), and cleanup on curl failure is handled inline below. download_binary() { local binary_name="certctl-agent-${OS_TYPE}-${ARCH_TYPE}" local download_url="${RELEASE_URL}/${binary_name}" - echo -e "${YELLOW}Downloading certctl agent (${OS_TYPE}-${ARCH_TYPE})...${NC}" + echo -e "${YELLOW}Downloading certctl agent (${OS_TYPE}-${ARCH_TYPE})...${NC}" >&2 if ! command -v curl &> /dev/null; then - echo -e "${RED}Error: curl is required but not installed${NC}" + echo -e "${RED}Error: curl is required but not installed${NC}" >&2 exit 1 fi - local temp_file=$(mktemp) - trap "rm -f $temp_file" EXIT + local temp_file + temp_file=$(mktemp) - if ! curl -sSL -f "$download_url" -o "$temp_file"; then - echo -e "${RED}Error: Failed to download binary from $download_url${NC}" - echo "Make sure the latest release exists on GitHub with the binary asset for ${OS_TYPE}-${ARCH_TYPE}." + if ! curl -sSL -f "$download_url" -o "$temp_file" >&2; then + rm -f "$temp_file" + echo -e "${RED}Error: Failed to download binary from $download_url${NC}" >&2 + echo "Make sure the latest release exists on GitHub with the binary asset for ${OS_TYPE}-${ARCH_TYPE}." >&2 exit 1 fi @@ -146,35 +247,52 @@ install_binary() { chmod +x "$INSTALL_DIR/$SERVICE_NAME" echo -e "${GREEN}Binary installed: $INSTALL_DIR/$SERVICE_NAME${NC}" + + # Clean up the temp file created by download_binary. We can't use an EXIT + # trap inside download_binary because it runs in a subshell (command + # substitution), so the trap would fire before we got here. Doing it + # explicitly after the successful cp is the simplest correct pattern. + rm -f "$binary_path" } -# Prompt for configuration (unless --server-url and --api-key provided) +# Prompt for configuration. Any value supplied via flag is honored as-is +# and we only prompt for the missing pieces. `read || true` prevents set -e +# from aborting the script on EOF — instead the empty check below fires the +# proper "required" error message. prompt_for_config() { if [[ -z "${SERVER_URL:-}" ]]; then echo "" echo -e "${YELLOW}Enter certctl server URL (e.g., https://certctl.example.com):${NC}" - read -r SERVER_URL - if [[ -z "$SERVER_URL" ]]; then - echo -e "${RED}Error: Server URL is required${NC}" + read -r SERVER_URL || true + if [[ -z "${SERVER_URL:-}" ]]; then + echo -e "${RED}Error: Server URL is required${NC}" >&2 + echo "Hint: pass --server-url to run non-interactively." >&2 exit 1 fi fi if [[ -z "${API_KEY:-}" ]]; then echo -e "${YELLOW}Enter certctl API key:${NC}" - read -sr API_KEY + read -rs API_KEY || true echo "" - if [[ -z "$API_KEY" ]]; then - echo -e "${RED}Error: API key is required${NC}" + if [[ -z "${API_KEY:-}" ]]; then + echo -e "${RED}Error: API key is required${NC}" >&2 + echo "Hint: pass --api-key to run non-interactively." >&2 exit 1 fi fi if [[ -z "${AGENT_ID:-}" ]]; then - local default_agent_id="$(hostname)" - echo -e "${YELLOW}Enter agent ID (default: $default_agent_id):${NC}" - read -r AGENT_ID - if [[ -z "$AGENT_ID" ]]; then + local default_agent_id + default_agent_id="$(hostname)" + # If stdin is still piped (no /dev/tty was available but SERVER_URL + + # API_KEY arrived via flags), skip the prompt entirely and use the + # default — no need to block on an optional value. + if [[ -t 0 ]]; then + echo -e "${YELLOW}Enter agent ID (default: $default_agent_id):${NC}" + read -r AGENT_ID || true + fi + if [[ -z "${AGENT_ID:-}" ]]; then AGENT_ID="$default_agent_id" fi fi @@ -447,6 +565,7 @@ main() { echo "Detected platform: ${OS_TYPE}-${ARCH_TYPE}" echo "" + ensure_interactive_input prompt_for_config # Download and install binary diff --git a/internal/api/handler/adversarial_est_test.go b/internal/api/handler/adversarial_est_test.go new file mode 100644 index 0000000..9d839e6 --- /dev/null +++ b/internal/api/handler/adversarial_est_test.go @@ -0,0 +1,339 @@ +package handler + +// Adversarial EST (RFC 7030) enrollment tests — Tier 1F. +// +// EST is the RFC 7030 protocol for certificate enrollment over HTTPS. The +// control-plane parser accepts PKCS#10 CSRs either as PEM or as base64-encoded +// DER, and it's a prime target for: +// +// * Malformed base64 / non-DER payloads +// * Valid base64 that doesn't decode to a valid CSR +// * PEM header spoofing (wrong block type) +// * Null bytes and control characters embedded in PEM or base64 +// * Huge CSR bodies (we expect the handler's 1 MiB LimitReader to clamp them) +// * Truncated or partially-written PEM blocks +// * Unicode homoglyphs in PEM delimiters +// * Content-Type mismatch (handler ignores Content-Type, but attackers might +// still try header spoofing) +// +// The contract is the same as other adversarial tiers: the handler must never +// panic and must never return 500 for a malformed CSR (500 is reserved for +// issuer/service failures). For adversarial CSRs, the correct status is 400. + +import ( + "bytes" + "context" + "encoding/base64" + "errors" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/shankar0123/certctl/internal/domain" +) + +// adversarialCSRInputs exercises the EST CSR parsing surface. None of these +// should reach the underlying ESTService — they must be rejected by +// readCSRFromRequest with a 400 before any service call is made. +func adversarialCSRInputs() []struct { + name string + body string +} { + // A garbage base64 string that decodes cleanly but isn't a PKCS#10 CSR. + // base64 of "this is definitely not a CSR" = dGhpcyBpcyBkZWZpbml0ZWx5IG5vdCBhIENTUg== + nonCSRBase64 := base64.StdEncoding.EncodeToString([]byte("this is definitely not a CSR")) + + return []struct { + name string + body string + }{ + {"garbage_string", "not-a-csr-at-all"}, + {"base64_garbage", "!!!@@@###$$$%%%"}, + {"base64_valid_non_csr", nonCSRBase64}, + {"base64_very_short", "AA=="}, + {"null_byte_only", "\x00"}, + {"null_bytes_padding", "\x00\x00\x00\x00\x00\x00\x00\x00"}, + {"control_chars", "\x01\x02\x03\x04\x05\x06\x07\x08"}, + {"pem_wrong_block_type", "-----BEGIN CERTIFICATE-----\nMIIB\n-----END CERTIFICATE-----\n"}, + {"pem_wrong_header_close", "-----BEGIN CERTIFICATE REQUEST-----\nMIIB\n-----END PRIVATE KEY-----\n"}, + {"pem_empty_block", "-----BEGIN CERTIFICATE REQUEST-----\n-----END CERTIFICATE REQUEST-----\n"}, + {"pem_garbage_body", "-----BEGIN CERTIFICATE REQUEST-----\n!!!not base64!!!\n-----END CERTIFICATE REQUEST-----\n"}, + {"pem_truncated", "-----BEGIN CERTIFICATE REQUEST-----\nMIIBijCCAT"}, + {"pem_no_end_marker", "-----BEGIN CERTIFICATE REQUEST-----\nMIIBijCCATICAQAwFjEUMBIGA1UE\n"}, + {"pem_header_injection", "-----BEGIN CERTIFICATE REQUEST-----\r\nHost: evil.com\r\n\r\nMIIB\n-----END CERTIFICATE REQUEST-----\n"}, + {"pem_embedded_null", "-----BEGIN CERTIFICATE\x00REQUEST-----\nMIIB\n-----END CERTIFICATE REQUEST-----\n"}, + {"unicode_homoglyph_pem", "-----BEGIN CERTIFICATE REQUEST─────\nMIIB\n─────END CERTIFICATE REQUEST-----\n"}, + {"double_pem_block", "-----BEGIN CERTIFICATE REQUEST-----\nMIIB\n-----END CERTIFICATE REQUEST-----\n-----BEGIN CERTIFICATE REQUEST-----\nMIIB\n-----END CERTIFICATE REQUEST-----\n"}, + {"json_body", `{"csr":"MIIB","common_name":"attacker.com"}`}, + {"xml_body", `MIIB`}, + {"shell_metacharacters", "$(whoami); rm -rf / #"}, + {"sql_injection", "' OR 1=1; DROP TABLE certificates;--"}, + {"long_garbage_10k", strings.Repeat("A", 10000)}, + {"long_base64_not_csr", base64.StdEncoding.EncodeToString(bytes.Repeat([]byte{0xFF}, 5000))}, + {"base64_with_newlines_garbage", "AAAAAAAAAAAAAAAA\nBBBBBBBBBBBBBBBB\nCCCCCCCCCCCCCCCC"}, + {"percent_encoded_pem", "%2D%2D%2D%2D%2DBEGIN+CERTIFICATE+REQUEST%2D%2D%2D%2D%2D"}, + } +} + +// assertESTErrorResponse enforces the EST handler contract for adversarial CSRs: +// no panic, no 500, body is valid JSON (since Error helper emits JSON errors). +func assertESTErrorResponse(t *testing.T, w *httptest.ResponseRecorder, label string) { + t.Helper() + + // The handler must never reach a 500 for parser-rejected CSRs — that would + // indicate a service call slipped through. + if w.Code == http.StatusInternalServerError { + t.Errorf("%s: handler returned 500 body=%q — adversarial CSR should not reach the service layer", + label, w.Body.String()) + } + + // The handler should return 400 Bad Request for adversarial CSR inputs. + // A 405 (method not allowed) is impossible here because we always POST. + if w.Code != http.StatusBadRequest { + t.Errorf("%s: expected 400, got %d (body=%q)", label, w.Code, w.Body.String()) + } +} + +// newESTHandlerWithTrap returns an ESTHandler whose service panics if reached. +// This is the core invariant for Tier 1F: adversarial CSRs must be rejected at +// the parser, never reaching SimpleEnroll/SimpleReEnroll on the service. +func newESTHandlerWithTrap() (ESTHandler, *trappedESTService) { + svc := &trappedESTService{} + return NewESTHandler(svc), svc +} + +// trappedESTService is a mock that fails the test if any service method is +// called with an adversarial CSR. The parser should reject these before they +// get here. +type trappedESTService struct { + serviceCalled bool +} + +func (t *trappedESTService) GetCACerts(ctx context.Context) (string, error) { + t.serviceCalled = true + return "", errors.New("trap: GetCACerts should not be called from adversarial CSR tests") +} + +func (t *trappedESTService) SimpleEnroll(ctx context.Context, csrPEM string) (*domain.ESTEnrollResult, error) { + t.serviceCalled = true + return nil, errors.New("trap: SimpleEnroll should not be called from adversarial CSR tests") +} + +func (t *trappedESTService) SimpleReEnroll(ctx context.Context, csrPEM string) (*domain.ESTEnrollResult, error) { + t.serviceCalled = true + return nil, errors.New("trap: SimpleReEnroll should not be called from adversarial CSR tests") +} + +func (t *trappedESTService) GetCSRAttrs(ctx context.Context) ([]byte, error) { + t.serviceCalled = true + return nil, errors.New("trap: GetCSRAttrs should not be called from adversarial CSR tests") +} + +// TestESTSimpleEnroll_AdversarialCSRs runs each adversarial CSR through the +// enrollment endpoint. +func TestESTSimpleEnroll_AdversarialCSRs(t *testing.T) { + for _, tc := range adversarialCSRInputs() { + t.Run(tc.name, func(t *testing.T) { + defer func() { + if r := recover(); r != nil { + t.Fatalf("handler panicked on body %q: %v", tc.body, r) + } + }() + + h, svc := newESTHandlerWithTrap() + + req := httptest.NewRequest(http.MethodPost, "/.well-known/est/simpleenroll", strings.NewReader(tc.body)) + req.Header.Set("Content-Type", "application/pkcs10") + + w := httptest.NewRecorder() + h.SimpleEnroll(w, req) + + assertESTErrorResponse(t, w, "SimpleEnroll/"+tc.name) + + if svc.serviceCalled { + t.Errorf("SimpleEnroll/%s: service was reached with adversarial CSR (body=%q)", + tc.name, tc.body) + } + }) + } +} + +// TestESTSimpleReEnroll_AdversarialCSRs runs each adversarial CSR through the +// re-enrollment endpoint. Same contract as simpleenroll. +func TestESTSimpleReEnroll_AdversarialCSRs(t *testing.T) { + for _, tc := range adversarialCSRInputs() { + t.Run(tc.name, func(t *testing.T) { + defer func() { + if r := recover(); r != nil { + t.Fatalf("handler panicked on body %q: %v", tc.body, r) + } + }() + + h, svc := newESTHandlerWithTrap() + + req := httptest.NewRequest(http.MethodPost, "/.well-known/est/simplereenroll", strings.NewReader(tc.body)) + req.Header.Set("Content-Type", "application/pkcs10") + + w := httptest.NewRecorder() + h.SimpleReEnroll(w, req) + + assertESTErrorResponse(t, w, "SimpleReEnroll/"+tc.name) + + if svc.serviceCalled { + t.Errorf("SimpleReEnroll/%s: service was reached with adversarial CSR (body=%q)", + tc.name, tc.body) + } + }) + } +} + +// TestESTSimpleEnroll_HugeBody verifies the handler's 1 MiB limit truncates +// oversized requests at the LimitReader boundary. We send a 2 MiB body of +// base64 garbage and confirm the handler rejects it cleanly (400, no panic, +// no 500) and the service is never reached. +func TestESTSimpleEnroll_HugeBody(t *testing.T) { + defer func() { + if r := recover(); r != nil { + t.Fatalf("handler panicked on 2 MiB body: %v", r) + } + }() + + // 2 MiB of base64-valid garbage: the LimitReader will truncate to 1 MiB, and + // the truncated base64 chunk won't parse as a valid PKCS#10 CSR. + huge := strings.Repeat("A", 2<<20) + + h, svc := newESTHandlerWithTrap() + + req := httptest.NewRequest(http.MethodPost, "/.well-known/est/simpleenroll", strings.NewReader(huge)) + req.Header.Set("Content-Type", "application/pkcs10") + + w := httptest.NewRecorder() + h.SimpleEnroll(w, req) + + // Contract: 400 Bad Request (parser fail), no panic, no 500. + if w.Code == http.StatusInternalServerError { + t.Errorf("HugeBody: handler returned 500 for 2 MiB body (body=%q)", w.Body.String()) + } + if w.Code != http.StatusBadRequest { + t.Errorf("HugeBody: expected 400, got %d (body=%q)", w.Code, w.Body.String()) + } + if svc.serviceCalled { + t.Error("HugeBody: service was reached with 2 MiB adversarial body") + } +} + +// TestESTSimpleEnroll_ExactlyAtLimit sends a body exactly at the 1 MiB +// LimitReader boundary. The body is still garbage (won't parse as CSR), but we +// verify the handler doesn't panic or hang on the boundary case. +func TestESTSimpleEnroll_ExactlyAtLimit(t *testing.T) { + defer func() { + if r := recover(); r != nil { + t.Fatalf("handler panicked on exact-limit body: %v", r) + } + }() + + atLimit := strings.Repeat("A", 1<<20) // exactly 1 MiB + + h, _ := newESTHandlerWithTrap() + + req := httptest.NewRequest(http.MethodPost, "/.well-known/est/simpleenroll", strings.NewReader(atLimit)) + w := httptest.NewRecorder() + h.SimpleEnroll(w, req) + + if w.Code == http.StatusInternalServerError { + t.Errorf("ExactlyAtLimit: handler returned 500 (body=%q)", w.Body.String()) + } +} + +// TestESTSimpleEnroll_MultipartBody sends a multipart/form-data body that a +// naive parser might try to unwrap. The handler should treat the raw bytes as +// a CSR payload and reject them. +func TestESTSimpleEnroll_MultipartBody(t *testing.T) { + defer func() { + if r := recover(); r != nil { + t.Fatalf("handler panicked on multipart body: %v", r) + } + }() + + multipart := "--boundary\r\nContent-Disposition: form-data; name=\"csr\"\r\n\r\nMIIB\r\n--boundary--\r\n" + + h, svc := newESTHandlerWithTrap() + + req := httptest.NewRequest(http.MethodPost, "/.well-known/est/simpleenroll", strings.NewReader(multipart)) + req.Header.Set("Content-Type", "multipart/form-data; boundary=boundary") + w := httptest.NewRecorder() + h.SimpleEnroll(w, req) + + if w.Code != http.StatusBadRequest { + t.Errorf("MultipartBody: expected 400, got %d (body=%q)", w.Code, w.Body.String()) + } + if svc.serviceCalled { + t.Error("MultipartBody: service was reached with multipart wrapper") + } +} + +// TestESTCACerts_MethodAbuse verifies the /cacerts endpoint only accepts GET +// and rejects every other method cleanly. This is a small safety check for +// the spec invariant. +func TestESTCACerts_MethodAbuse(t *testing.T) { + methods := []string{ + http.MethodPost, http.MethodPut, http.MethodDelete, + http.MethodPatch, http.MethodHead, http.MethodOptions, + "TRACE", "CONNECT", "PROPFIND", "BOGUS", + } + + for _, method := range methods { + t.Run(method, func(t *testing.T) { + defer func() { + if r := recover(); r != nil { + t.Fatalf("handler panicked on method %s: %v", method, r) + } + }() + + h, _ := newESTHandlerWithTrap() + + req := httptest.NewRequest(method, "/.well-known/est/cacerts", nil) + w := httptest.NewRecorder() + h.CACerts(w, req) + + // HEAD on a GET handler in Go's stdlib is normally accepted, but + // this handler enforces strict GET-only — so HEAD should also get 405. + if w.Code != http.StatusMethodNotAllowed { + t.Errorf("method %s: expected 405, got %d", method, w.Code) + } + }) + } +} + +// TestESTSimpleEnroll_MethodAbuse verifies strict POST-only enforcement. +func TestESTSimpleEnroll_MethodAbuse(t *testing.T) { + methods := []string{ + http.MethodGet, http.MethodPut, http.MethodDelete, + http.MethodPatch, http.MethodHead, http.MethodOptions, + "TRACE", "CONNECT", + } + + for _, method := range methods { + t.Run(method, func(t *testing.T) { + defer func() { + if r := recover(); r != nil { + t.Fatalf("handler panicked on method %s: %v", method, r) + } + }() + + h, svc := newESTHandlerWithTrap() + + req := httptest.NewRequest(method, "/.well-known/est/simpleenroll", strings.NewReader("body")) + w := httptest.NewRecorder() + h.SimpleEnroll(w, req) + + if w.Code != http.StatusMethodNotAllowed { + t.Errorf("method %s: expected 405, got %d", method, w.Code) + } + if svc.serviceCalled { + t.Errorf("method %s: service was called for non-POST", method) + } + }) + } +} diff --git a/internal/api/handler/adversarial_path_test.go b/internal/api/handler/adversarial_path_test.go new file mode 100644 index 0000000..4fb94d0 --- /dev/null +++ b/internal/api/handler/adversarial_path_test.go @@ -0,0 +1,330 @@ +package handler + +// Adversarial path-parameter and multi-segment path tests. +// +// These tests exercise the input parsing boundary of the certificate handler +// against the attack categories listed in certctl-adversarial-testing-prompt.md +// Tier 1A / 1B: +// +// * Empty and whitespace-only path IDs +// * SQL-injection sentinels embedded in the path +// * Directory traversal (`../../etc/passwd`) +// * Null bytes and control characters +// * Extremely long IDs (10 KiB) +// * Unicode homoglyphs (visually identical substitutes) +// * Multi-segment paths (OCSP, DER CRL, versions, renew, deploy, revoke) +// +// The contract we verify is defensive, not behavioural: +// +// 1. The handler never panics. +// 2. The HTTP status is one of {200, 400, 404, 405} — never 500. +// 3. The response body is either empty or valid JSON. +// 4. No attacker-controlled input is echoed verbatim in a 500 body. +// +// We do not assert the exact status code for every adversarial input because +// the current handler intentionally delegates identifier validation to the +// repository layer; its only job here is to stay up and well-formed. + +import ( + "bytes" + "encoding/json" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/shankar0123/certctl/internal/domain" +) + +// adversarialPathInputs is the attack catalog shared by Tier 1A cases. Each +// entry targets a different parsing surface; adding a new category here makes +// every Tier 1A test below exercise it automatically. +func adversarialPathInputs() []struct { + name string + input string +} { + return []struct { + name string + input string + }{ + {"sql_injection_drop_table", "'; DROP TABLE managed_certificates;--"}, + {"sql_injection_or_true", "' OR 1=1--"}, + {"sql_injection_union", "mc-001' UNION SELECT * FROM agents--"}, + {"path_traversal_dot_dot", "../../etc/passwd"}, + {"path_traversal_encoded", "..%2F..%2Fetc%2Fpasswd"}, + {"null_byte_trailing", "mc-001\x00"}, + {"null_byte_embedded", "mc-\x00-001"}, + {"long_id_10k", strings.Repeat("A", 10000)}, + {"unicode_homoglyph_hyphen", "mc\u2010001"}, // U+2010 HYPHEN + {"unicode_homoglyph_fullwidth", "mc\uFF0D001"}, // U+FF0D FULLWIDTH HYPHEN-MINUS + {"control_char_newline", "mc-001\n"}, + {"control_char_tab", "mc\t001"}, + {"control_char_bell", "mc\x07001"}, + {"percent_encoded_null", "mc-001%00"}, + {"whitespace_only", " "}, + {"shell_metacharacters", "mc-001;`rm -rf /`"}, + {"leading_slash", "/mc-001"}, + {"trailing_slash", "mc-001/"}, + {"double_slash", "mc//001"}, + } +} + +// assertSafeResponse is the core defensive check. Any adversarial input is +// allowed to produce a 4xx, but must not panic or leak through as a 500. +func assertSafeResponse(t *testing.T, w *httptest.ResponseRecorder, label string) { + t.Helper() + + // 1. No 500 (500 implies the handler reached an unexpected internal state). + if w.Code == http.StatusInternalServerError { + t.Errorf("%s: handler returned 500, body=%q — adversarial input should not reach an internal error path", + label, w.Body.String()) + } + + // 2. Status must be in the expected safe set. + switch w.Code { + case http.StatusOK, http.StatusCreated, http.StatusAccepted, http.StatusNoContent, + http.StatusBadRequest, http.StatusNotFound, http.StatusMethodNotAllowed, http.StatusNotImplemented: + // ok + default: + t.Errorf("%s: unexpected status %d (body=%q)", label, w.Code, w.Body.String()) + } + + // 3. Non-empty bodies must be valid JSON (no template leakage, no raw panics). + if body := bytes.TrimSpace(w.Body.Bytes()); len(body) > 0 { + var discard interface{} + if err := json.Unmarshal(body, &discard); err != nil { + t.Errorf("%s: response body is not valid JSON: %v (body=%q)", label, err, w.Body.String()) + } + } +} + +// newCertHandlerWithMock builds a handler whose mock service returns nothing. +// This keeps every adversarial test focused on the handler's parsing layer +// rather than service behaviour. +func newCertHandlerWithMock() (CertificateHandler, *MockCertificateService) { + mock := &MockCertificateService{} + return NewCertificateHandler(mock), mock +} + +// TestGetCertificate_PathInjection runs each adversarial path through the +// certificate GET handler. +func TestGetCertificate_PathInjection(t *testing.T) { + for _, tc := range adversarialPathInputs() { + t.Run(tc.name, func(t *testing.T) { + defer func() { + if r := recover(); r != nil { + t.Fatalf("handler panicked on input %q: %v", tc.input, r) + } + }() + + handler, mock := newCertHandlerWithMock() + // Force a 404 so we can distinguish "service was called" from + // "parser accepted the ID"; a 200 with null body is also fine. + mock.GetCertificateFn = func(id string) (*domain.ManagedCertificate, error) { + return nil, ErrMockNotFound + } + + // Build the URL by string concatenation to keep attacker-controlled + // bytes intact (httptest.NewRequest uses url.Parse under the hood, + // which normalises some characters — we want the raw path on the + // request object). + req := httptest.NewRequest(http.MethodGet, "/api/v1/certificates/x", nil) + req.URL.Path = "/api/v1/certificates/" + tc.input + req = req.WithContext(contextWithRequestID()) + + w := httptest.NewRecorder() + handler.GetCertificate(w, req) + + assertSafeResponse(t, w, "GetCertificate/"+tc.name) + }) + } +} + +// TestUpdateCertificate_PathInjection exercises the PUT handler's path parser. +// UpdateCertificate splits the path on "/" and takes parts[0]; traversal and +// double-slash inputs must still short-circuit at the parser rather than +// reaching the service. +func TestUpdateCertificate_PathInjection(t *testing.T) { + body := `{"common_name":"example.com","owner_id":"o-alice","team_id":"t-a","issuer_id":"iss-local","name":"n","renewal_policy_id":"rp-1"}` + + for _, tc := range adversarialPathInputs() { + t.Run(tc.name, func(t *testing.T) { + defer func() { + if r := recover(); r != nil { + t.Fatalf("handler panicked on input %q: %v", tc.input, r) + } + }() + + handler, mock := newCertHandlerWithMock() + mock.UpdateCertificateFn = func(id string, cert domain.ManagedCertificate) (*domain.ManagedCertificate, error) { + return nil, ErrMockNotFound + } + + req := httptest.NewRequest(http.MethodPut, "/api/v1/certificates/x", bytes.NewBufferString(body)) + req.URL.Path = "/api/v1/certificates/" + tc.input + req.Header.Set("Content-Type", "application/json") + req = req.WithContext(contextWithRequestID()) + + w := httptest.NewRecorder() + handler.UpdateCertificate(w, req) + + assertSafeResponse(t, w, "UpdateCertificate/"+tc.name) + }) + } +} + +// TestArchiveCertificate_PathInjection exercises DELETE. +func TestArchiveCertificate_PathInjection(t *testing.T) { + for _, tc := range adversarialPathInputs() { + t.Run(tc.name, func(t *testing.T) { + defer func() { + if r := recover(); r != nil { + t.Fatalf("handler panicked on input %q: %v", tc.input, r) + } + }() + + handler, mock := newCertHandlerWithMock() + mock.ArchiveCertificateFn = func(id string) error { return ErrMockNotFound } + + req := httptest.NewRequest(http.MethodDelete, "/api/v1/certificates/x", nil) + req.URL.Path = "/api/v1/certificates/" + tc.input + req = req.WithContext(contextWithRequestID()) + + w := httptest.NewRecorder() + handler.ArchiveCertificate(w, req) + + assertSafeResponse(t, w, "ArchiveCertificate/"+tc.name) + }) + } +} + +// TestGetCertificateVersions_MultiSegment is a Tier 1B test: the versions +// handler requires a 2-segment path (certID/versions). The parser uses +// strings.Split(path, "/") and checks len(parts) < 2 — but an adversarial +// caller can inject extra slashes to either produce an empty parts[0] or a +// very long parts slice. Either way we must not panic. +func TestGetCertificateVersions_MultiSegment(t *testing.T) { + cases := []struct { + name string + path string + }{ + {"missing_segment", "/api/v1/certificates/versions"}, + {"empty_cert_id", "/api/v1/certificates//versions"}, + {"traversal_cert_id", "/api/v1/certificates/..%2F..%2Fversions/versions"}, + {"sql_injection_cert_id", "/api/v1/certificates/'%20OR%201=1--/versions"}, + {"null_byte_cert_id", "/api/v1/certificates/mc\x00001/versions"}, + {"very_long_cert_id", "/api/v1/certificates/" + strings.Repeat("A", 5000) + "/versions"}, + {"trailing_segments", "/api/v1/certificates/mc-001/versions/extra/trailing"}, + {"deep_nesting", "/api/v1/certificates/" + strings.Repeat("a/", 50) + "versions"}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + defer func() { + if r := recover(); r != nil { + t.Fatalf("handler panicked on path %q: %v", tc.path, r) + } + }() + + handler, mock := newCertHandlerWithMock() + mock.GetCertificateVersionsFn = func(certID string, page, perPage int) ([]domain.CertificateVersion, int64, error) { + return []domain.CertificateVersion{}, 0, nil + } + + // Use a dummy safe URL in NewRequest to avoid url.Parse panics + // on control chars, then overwrite with the raw attacker path. + req := httptest.NewRequest(http.MethodGet, "/safe", nil) + req.URL.Path = tc.path + req = req.WithContext(contextWithRequestID()) + + w := httptest.NewRecorder() + handler.GetCertificateVersions(w, req) + + assertSafeResponse(t, w, "GetCertificateVersions/"+tc.name) + }) + } +} + +// TestHandleOCSP_MultiSegment exercises the OCSP responder's 2-segment path +// parser (/api/v1/ocsp/{issuer_id}/{serial_hex}). Each leg is attacker- +// controlled and the serial can be arbitrary length. This is a key adversarial +// surface because the serial is passed directly to the CA-operations service, +// which is expected to treat it as an opaque identifier. +func TestHandleOCSP_MultiSegment(t *testing.T) { + cases := []struct { + name string + path string + }{ + {"missing_serial", "/api/v1/ocsp/iss-local"}, + {"missing_both", "/api/v1/ocsp/"}, + {"empty_issuer", "/api/v1/ocsp//01ABCDEF"}, + {"empty_serial", "/api/v1/ocsp/iss-local/"}, + {"traversal_issuer", "/api/v1/ocsp/..%2F..%2Fetc/passwd/01"}, + {"null_byte_serial", "/api/v1/ocsp/iss-local/01\x00FF"}, + {"sql_injection_serial", "/api/v1/ocsp/iss-local/01'; DROP TABLE--"}, + {"negative_hex_serial", "/api/v1/ocsp/iss-local/-1"}, + {"unicode_serial", "/api/v1/ocsp/iss-local/01\u2010FF"}, + {"extremely_long_serial", "/api/v1/ocsp/iss-local/" + strings.Repeat("F", 10000)}, + {"extra_segments", "/api/v1/ocsp/iss-local/01FF/extra/segments"}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + defer func() { + if r := recover(); r != nil { + t.Fatalf("handler panicked on path %q: %v", tc.path, r) + } + }() + + handler, mock := newCertHandlerWithMock() + mock.GetOCSPResponseFn = func(issuerID, serialHex string) ([]byte, error) { + return nil, ErrMockNotFound + } + + req := httptest.NewRequest(http.MethodGet, "/safe", nil) + req.URL.Path = tc.path + req = req.WithContext(contextWithRequestID()) + + w := httptest.NewRecorder() + handler.HandleOCSP(w, req) + + // OCSP does NOT guarantee JSON responses (pkix-crl uses binary), + // so we only check status safety, not body structure. + if w.Code == http.StatusInternalServerError { + t.Errorf("HandleOCSP/%s: returned 500 body=%q", tc.name, w.Body.String()) + } + if w.Code >= 500 { + t.Errorf("HandleOCSP/%s: unexpected 5xx %d", tc.name, w.Code) + } + }) + } +} + +// TestGetDERCRL_IssuerPathInjection exercises /api/v1/crl/{issuer_id}. +func TestGetDERCRL_IssuerPathInjection(t *testing.T) { + for _, tc := range adversarialPathInputs() { + t.Run(tc.name, func(t *testing.T) { + defer func() { + if r := recover(); r != nil { + t.Fatalf("handler panicked on input %q: %v", tc.input, r) + } + }() + + handler, mock := newCertHandlerWithMock() + mock.GenerateDERCRLFn = func(issuerID string) ([]byte, error) { + return nil, ErrMockNotFound + } + + req := httptest.NewRequest(http.MethodGet, "/api/v1/crl/x", nil) + req.URL.Path = "/api/v1/crl/" + tc.input + req = req.WithContext(contextWithRequestID()) + + w := httptest.NewRecorder() + handler.GetDERCRL(w, req) + + if w.Code >= 500 { + t.Errorf("GetDERCRL/%s: unexpected 5xx %d (body=%q)", tc.name, w.Code, w.Body.String()) + } + }) + } +} diff --git a/internal/api/handler/adversarial_query_test.go b/internal/api/handler/adversarial_query_test.go new file mode 100644 index 0000000..570d8d5 --- /dev/null +++ b/internal/api/handler/adversarial_query_test.go @@ -0,0 +1,538 @@ +package handler + +// Adversarial query-parameter, request-body, and revocation-reason tests. +// +// These tests exercise the second boundary of the certificate handler: +// +// * Numeric pagination parsing (page, per_page, page_size) +// * Sort direction and field whitelist +// * Time-range filters (expires_before, expires_after, created_after, updated_after) +// * Cursor pagination +// * Sparse-field projection (?fields=...) +// * Request-body JSON parsing (create/update) — null, malformed, deep nesting, +// unicode, oversized +// * Revocation reason abuse +// +// The handler silently ignores malformed pagination values (it falls back to +// defaults) and ignores invalid RFC3339 time values. These tests lock in that +// behaviour so a future "fail-closed" change has to be deliberate. + +import ( + "bytes" + "fmt" + "net/http" + "net/http/httptest" + "net/url" + "strings" + "testing" + + "github.com/shankar0123/certctl/internal/domain" + "github.com/shankar0123/certctl/internal/repository" +) + +// buildListRequest constructs a GET /api/v1/certificates request with the +// given raw query string. We use raw query strings (not url.Values.Encode) +// so adversarial inputs like "page=abc&page=-1" or "%00" pass through +// unchanged. +func buildListRequest(rawQuery string) *http.Request { + req := httptest.NewRequest(http.MethodGet, "/api/v1/certificates", nil) + req.URL.RawQuery = rawQuery + return req.WithContext(contextWithRequestID()) +} + +// TestListCertificates_PaginationAbuse verifies adversarial pagination values +// never produce a 500 and the handler always falls back to sane defaults. +func TestListCertificates_PaginationAbuse(t *testing.T) { + cases := []struct { + name string + rawQuery string + }{ + {"negative_page", "page=-1"}, + {"zero_page", "page=0"}, + {"non_numeric_page", "page=abc"}, + {"huge_page", "page=99999999999"}, + {"int_overflow_page", "page=9223372036854775808"}, // int64 max + 1 + {"negative_per_page", "per_page=-1"}, + {"zero_per_page", "per_page=0"}, + {"per_page_cap_at_500", "per_page=500"}, + {"per_page_above_cap", "per_page=501"}, + {"per_page_absurd", "per_page=1000000"}, + {"non_numeric_per_page", "per_page=xyz"}, + {"mixed_numeric_per_page", "per_page=10abc"}, + {"negative_page_size", "page_size=-1"}, + {"page_size_above_cap", "page_size=501"}, + {"float_page", "page=1.5"}, + {"exponent_page", "page=1e10"}, + {"hex_page", "page=0xff"}, + {"unicode_digits_page", "page=\u0661\u0662\u0663"}, // Arabic-Indic digits + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + defer func() { + if r := recover(); r != nil { + t.Fatalf("panicked on %q: %v", tc.rawQuery, r) + } + }() + + handler, mock := newCertHandlerWithMock() + mock.ListCertificatesWithFilterFn = func(filter *repository.CertificateFilter) ([]domain.ManagedCertificate, int, error) { + // Sanity: page/perPage on the filter must never be negative + // and perPage must never exceed 500 after parsing. + if filter.Page < 1 { + t.Errorf("filter.Page=%d (must be >=1)", filter.Page) + } + if filter.PerPage < 1 || filter.PerPage > 500 { + t.Errorf("filter.PerPage=%d (must be in [1,500])", filter.PerPage) + } + return []domain.ManagedCertificate{}, 0, nil + } + + w := httptest.NewRecorder() + handler.ListCertificates(w, buildListRequest(tc.rawQuery)) + + assertSafeResponse(t, w, "ListCertificates/"+tc.name) + if w.Code != http.StatusOK { + t.Errorf("%s: expected 200, got %d (body=%q)", tc.name, w.Code, w.Body.String()) + } + }) + } +} + +// TestListCertificates_SortAbuse verifies the sort field (which feeds into a +// whitelist in the repository layer) handles adversarial input safely at the +// handler boundary. The handler accepts the raw value and forwards it; the +// repository is expected to whitelist it, but at THIS layer we just verify +// we don't crash or leak. +func TestListCertificates_SortAbuse(t *testing.T) { + cases := []struct { + name string + rawQuery string + }{ + {"sql_injection_sort", "sort=notAfter;DROP TABLE managed_certificates--"}, + {"sql_injection_or", "sort=notAfter' OR '1'='1"}, + {"path_traversal_sort", "sort=../../etc/passwd"}, + {"null_byte_sort", "sort=notAfter%00"}, + {"unicode_sort", "sort=notAfter\u2010desc"}, + {"leading_dash_only", "sort=-"}, + {"leading_dashes", "sort=---notAfter"}, + {"empty_sort", "sort="}, + {"very_long_sort", "sort=" + strings.Repeat("a", 5000)}, + {"sort_desc_flag", "sort=notAfter&sort_desc=true"}, + {"conflicting_sort_desc", "sort=-notAfter&sort_desc=false"}, + {"unknown_field", "sort=gibberish"}, + {"shell_metacharacters_sort", "sort=notAfter;rm -rf /"}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + defer func() { + if r := recover(); r != nil { + t.Fatalf("panicked on %q: %v", tc.rawQuery, r) + } + }() + + handler, mock := newCertHandlerWithMock() + mock.ListCertificatesWithFilterFn = func(filter *repository.CertificateFilter) ([]domain.ManagedCertificate, int, error) { + return []domain.ManagedCertificate{}, 0, nil + } + + w := httptest.NewRecorder() + handler.ListCertificates(w, buildListRequest(tc.rawQuery)) + + assertSafeResponse(t, w, "ListCertificates/"+tc.name) + }) + } +} + +// TestListCertificates_FieldsAbuse verifies sparse field projection handles +// adversarial field lists safely. +func TestListCertificates_FieldsAbuse(t *testing.T) { + cases := []struct { + name string + rawQuery string + }{ + {"sql_injection_fields", "fields=id,name' OR 1=1--"}, + {"path_traversal_fields", "fields=../../etc/passwd"}, + {"empty_fields", "fields="}, + {"single_comma", "fields=,"}, + {"trailing_comma", "fields=id,name,"}, + {"leading_comma", "fields=,id,name"}, + {"whitespace_fields", "fields= id , name "}, + {"duplicate_fields", "fields=id,id,id,id,id"}, + {"unknown_fields", "fields=totally_not_a_field"}, + {"many_fields", "fields=" + strings.Repeat("x,", 200) + "id"}, + {"unicode_fields", "fields=id,n\u00e4me"}, + {"null_byte_fields", "fields=id%00name"}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + defer func() { + if r := recover(); r != nil { + t.Fatalf("panicked on %q: %v", tc.rawQuery, r) + } + }() + + handler, mock := newCertHandlerWithMock() + mock.ListCertificatesWithFilterFn = func(filter *repository.CertificateFilter) ([]domain.ManagedCertificate, int, error) { + return []domain.ManagedCertificate{}, 0, nil + } + + w := httptest.NewRecorder() + handler.ListCertificates(w, buildListRequest(tc.rawQuery)) + + assertSafeResponse(t, w, "ListCertificates/"+tc.name) + }) + } +} + +// TestListCertificates_TimeRangeAbuse verifies RFC3339 time-range filters +// handle malformed input by silently falling back to no filter (current +// behaviour). +func TestListCertificates_TimeRangeAbuse(t *testing.T) { + cases := []struct { + name string + rawQuery string + }{ + {"invalid_expires_before", "expires_before=not-a-date"}, + {"empty_expires_before", "expires_before="}, + {"garbage_expires_before", "expires_before=%00%00"}, + {"sql_injection_time", "expires_before=2026-01-01T00:00:00Z';DROP TABLE managed_certificates--"}, + {"year_zero", "expires_before=0000-01-01T00:00:00Z"}, + {"year_negative", "expires_before=-0001-01-01T00:00:00Z"}, + {"year_huge", "expires_before=99999-12-31T23:59:59Z"}, + {"invalid_month", "expires_before=2026-13-01T00:00:00Z"}, + {"invalid_day", "expires_before=2026-02-30T00:00:00Z"}, + {"valid_utc", "expires_before=2026-06-15T12:00:00Z"}, + {"valid_with_offset", "expires_before=2026-06-15T12:00:00-07:00"}, + {"unix_seconds_not_rfc3339", "expires_before=1767225600"}, + {"all_four_filters", "expires_before=garbage&expires_after=garbage&created_after=garbage&updated_after=garbage"}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + defer func() { + if r := recover(); r != nil { + t.Fatalf("panicked on %q: %v", tc.rawQuery, r) + } + }() + + handler, mock := newCertHandlerWithMock() + mock.ListCertificatesWithFilterFn = func(filter *repository.CertificateFilter) ([]domain.ManagedCertificate, int, error) { + return []domain.ManagedCertificate{}, 0, nil + } + + w := httptest.NewRecorder() + handler.ListCertificates(w, buildListRequest(tc.rawQuery)) + + assertSafeResponse(t, w, "ListCertificates/"+tc.name) + if w.Code != http.StatusOK { + t.Errorf("%s: expected 200, got %d", tc.name, w.Code) + } + }) + } +} + +// TestListCertificates_CursorAbuse exercises cursor-based pagination with +// adversarial cursor tokens. The handler forwards the cursor to the +// repository; we verify no 500 at the boundary and that the response type +// switches correctly. +func TestListCertificates_CursorAbuse(t *testing.T) { + cases := []struct { + name string + cursor string + }{ + {"empty_not_set", ""}, // special-cased: should return PagedResponse + {"garbage_cursor", "not-a-valid-cursor"}, + {"base64_garbage", "dGhpcyBpcyBub3QgYSB2YWxpZCBjdXJzb3I="}, + {"sql_injection_cursor", "2026-01-01T00:00:00Z:mc-001';DROP TABLE--"}, + {"path_traversal_cursor", "../../etc/passwd"}, + {"null_byte_cursor", "valid%00cursor"}, + {"very_long_cursor", strings.Repeat("A", 8192)}, + {"unicode_cursor", "2026-01-01T00:00:00Z:mc\u20100001"}, + {"valid_looking_cursor", "2026-01-01T00:00:00.000000000Z:mc-001"}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + defer func() { + if r := recover(); r != nil { + t.Fatalf("panicked on %q: %v", tc.cursor, r) + } + }() + + handler, mock := newCertHandlerWithMock() + mock.ListCertificatesWithFilterFn = func(filter *repository.CertificateFilter) ([]domain.ManagedCertificate, int, error) { + return []domain.ManagedCertificate{}, 0, nil + } + + rawQuery := "cursor=" + url.QueryEscape(tc.cursor) + "&page_size=50" + if tc.cursor == "" { + rawQuery = "page=1&per_page=50" + } + w := httptest.NewRecorder() + handler.ListCertificates(w, buildListRequest(rawQuery)) + + assertSafeResponse(t, w, "ListCertificates/"+tc.name) + if w.Code != http.StatusOK { + t.Errorf("%s: expected 200, got %d", tc.name, w.Code) + } + }) + } +} + +// TestListCertificates_FilterInjection verifies the basic string filters +// (status, environment, owner_id, team_id, issuer_id, agent_id, profile_id) +// are forwarded as-is without causing any handler-layer failures. These go +// into parameterized SQL at the repo layer. +func TestListCertificates_FilterInjection(t *testing.T) { + filters := []string{ + "status", "environment", "owner_id", "team_id", + "issuer_id", "agent_id", "profile_id", + } + payloads := []string{ + "' OR 1=1--", + "'; DROP TABLE managed_certificates;--", + "../../etc/passwd", + strings.Repeat("A", 5000), + "\u2010hyphen", + "%00null", + } + + for _, f := range filters { + for _, p := range payloads { + name := f + "__" + p + if len(name) > 80 { + name = name[:80] + } + t.Run(name, func(t *testing.T) { + defer func() { + if r := recover(); r != nil { + t.Fatalf("panicked: %v", r) + } + }() + + handler, mock := newCertHandlerWithMock() + mock.ListCertificatesWithFilterFn = func(filter *repository.CertificateFilter) ([]domain.ManagedCertificate, int, error) { + return []domain.ManagedCertificate{}, 0, nil + } + + rawQuery := f + "=" + url.QueryEscape(p) + w := httptest.NewRecorder() + handler.ListCertificates(w, buildListRequest(rawQuery)) + + assertSafeResponse(t, w, "ListCertificates/"+f) + }) + } + } +} + +// ---------- Request body abuse (Tier 1D) ---------- + +// TestCreateCertificate_BodyAbuse sends adversarial JSON bodies to +// POST /api/v1/certificates. Every case must respond with 400 (not 500, +// not 200). This proves we reject malformed input before reaching the +// service layer. +func TestCreateCertificate_BodyAbuse(t *testing.T) { + cases := []struct { + name string + body string + }{ + {"null_body", "null"}, + {"empty_body", ""}, + {"not_json", "not json at all"}, + {"truncated_json", `{"common_name":"exa`}, + {"unclosed_object", `{"common_name":"example.com"`}, + {"array_not_object", `["example.com"]`}, + {"number_not_object", `42`}, + {"string_not_object", `"hello"`}, + {"boolean_not_object", `true`}, + {"duplicate_keys", `{"common_name":"evil.com","common_name":"example.com"}`}, + {"unicode_bom", "\ufeff{\"common_name\":\"example.com\"}"}, + {"deep_nesting", strings.Repeat("{\"x\":", 100) + "null" + strings.Repeat("}", 100)}, + {"nested_array_bomb", `{"common_name":"x","sans":[[[[[[[[[[]]]]]]]]]]}`}, + {"sql_injection_cn", `{"common_name":"'; DROP TABLE managed_certificates;--"}`}, + {"empty_cn", `{"common_name":""}`}, + {"null_cn", `{"common_name":null}`}, + {"whitespace_cn", `{"common_name":" "}`}, + {"cn_too_long", fmt.Sprintf(`{"common_name":%q}`, strings.Repeat("a", 500))}, + {"cn_path_traversal", `{"common_name":"../../etc/passwd"}`}, + {"cn_null_byte", "{\"common_name\":\"example\\u0000.com\"}"}, + {"cn_newline", "{\"common_name\":\"example\\n.com\"}"}, + {"cn_only_missing_others", `{"common_name":"example.com"}`}, + {"extra_unknown_fields", `{"common_name":"example.com","__proto__":{"polluted":true},"eval":"alert(1)"}`}, + {"unicode_homoglyph_cn", "{\"common_name\":\"ex\u0430mple.com\"}"}, // Cyrillic а + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + defer func() { + if r := recover(); r != nil { + t.Fatalf("panicked on %q: %v", tc.name, r) + } + }() + + handler, mock := newCertHandlerWithMock() + mock.CreateCertificateFn = func(cert domain.ManagedCertificate) (*domain.ManagedCertificate, error) { + // If we ever reach this, the handler accepted a malformed + // body. Return a sentinel that passes but flag it. + c := cert + c.ID = "mc-accepted" + return &c, nil + } + + req := httptest.NewRequest(http.MethodPost, "/api/v1/certificates", bytes.NewBufferString(tc.body)) + req.Header.Set("Content-Type", "application/json") + req = req.WithContext(contextWithRequestID()) + w := httptest.NewRecorder() + handler.CreateCertificate(w, req) + + assertSafeResponse(t, w, "CreateCertificate/"+tc.name) + // Must NOT be 201 — all these bodies should be rejected. + if w.Code == http.StatusCreated { + t.Errorf("%s: handler accepted malformed body (201) body=%q", tc.name, w.Body.String()) + } + }) + } +} + +// TestCreateCertificate_HugeBody sends a 2 MiB JSON body. The body-limit +// middleware is not in this handler-unit test, so we just verify the handler +// doesn't OOM/panic on a large but well-formed body. +func TestCreateCertificate_HugeBody(t *testing.T) { + defer func() { + if r := recover(); r != nil { + t.Fatalf("panicked on huge body: %v", r) + } + }() + + // 2 MiB of SANs — well-formed JSON, technically valid, just huge. + var sb strings.Builder + sb.WriteString(`{"common_name":"example.com","owner_id":"o","team_id":"t","issuer_id":"iss","name":"n","renewal_policy_id":"rp","sans":[`) + for i := 0; i < 20000; i++ { + if i > 0 { + sb.WriteByte(',') + } + fmt.Fprintf(&sb, `"host%d.example.com"`, i) + } + sb.WriteString(`]}`) + + handler, mock := newCertHandlerWithMock() + mock.CreateCertificateFn = func(cert domain.ManagedCertificate) (*domain.ManagedCertificate, error) { + c := cert + c.ID = "mc-huge" + return &c, nil + } + + req := httptest.NewRequest(http.MethodPost, "/api/v1/certificates", strings.NewReader(sb.String())) + req.Header.Set("Content-Type", "application/json") + req = req.WithContext(contextWithRequestID()) + w := httptest.NewRecorder() + handler.CreateCertificate(w, req) + + assertSafeResponse(t, w, "CreateCertificate/huge_body") +} + +// ---------- Revocation reason abuse (Tier 1E) ---------- + +// TestRevokeCertificate_ReasonAbuse sends adversarial revocation reasons to +// POST /api/v1/certificates/{id}/revoke. The handler forwards the reason +// string to the service layer, which validates against RFC 5280. Errors +// from the service containing "invalid revocation reason" must map to 400, +// never 500. +func TestRevokeCertificate_ReasonAbuse(t *testing.T) { + cases := []struct { + name string + body string + }{ + {"empty_reason", `{"reason":""}`}, + {"null_reason", `{"reason":null}`}, + {"nonexistent_reason", `{"reason":"totally made up"}`}, + {"case_variant", `{"reason":"KEYCOMPROMISE"}`}, + {"with_spaces", `{"reason":"key compromise"}`}, + {"with_dashes", `{"reason":"key-compromise"}`}, + {"mixed_case", `{"reason":"KeyCompromise"}`}, + {"lowercase_valid", `{"reason":"keycompromise"}`}, + {"unicode_homoglyph", "{\"reason\":\"keyCompr\u043emise\"}"}, + {"sql_injection", `{"reason":"keyCompromise';DROP TABLE revocations--"}`}, + {"very_long", fmt.Sprintf(`{"reason":%q}`, strings.Repeat("a", 10000))}, + {"integer_reason", `{"reason":1}`}, + {"array_reason", `{"reason":["keyCompromise"]}`}, + {"object_reason", `{"reason":{"code":1}}`}, + {"extra_fields", `{"reason":"keyCompromise","admin":true,"bypass":true}`}, + {"no_body", ``}, + {"malformed_json", `{"reason":`}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + defer func() { + if r := recover(); r != nil { + t.Fatalf("panicked on %q: %v", tc.name, r) + } + }() + + handler, mock := newCertHandlerWithMock() + // The mock always returns "invalid revocation reason" so we + // verify the handler's errMsg→status mapping turns it into a 400. + mock.RevokeCertificateFn = func(id string, reason string) error { + // The service uses domain.IsValidRevocationReason. If we got + // through to here with something bogus, simulate a real + // service error. + return fmt.Errorf("invalid revocation reason: %q", reason) + } + + req := httptest.NewRequest(http.MethodPost, "/api/v1/certificates/mc-001/revoke", bytes.NewBufferString(tc.body)) + req.URL.Path = "/api/v1/certificates/mc-001/revoke" + req.Header.Set("Content-Type", "application/json") + req = req.WithContext(contextWithRequestID()) + w := httptest.NewRecorder() + handler.RevokeCertificate(w, req) + + assertSafeResponse(t, w, "RevokeCertificate/"+tc.name) + }) + } +} + +// TestRevokeCertificate_AlreadyRevoked locks in the specific error->status +// mapping for "already revoked". The handler uses substring matching on the +// service error message, which is fragile — this test catches regressions. +func TestRevokeCertificate_AlreadyRevoked(t *testing.T) { + handler, mock := newCertHandlerWithMock() + mock.RevokeCertificateFn = func(id string, reason string) error { + return fmt.Errorf("cannot revoke: certificate is already revoked") + } + + req := httptest.NewRequest(http.MethodPost, "/api/v1/certificates/mc-001/revoke", strings.NewReader(`{"reason":"keyCompromise"}`)) + req.URL.Path = "/api/v1/certificates/mc-001/revoke" + req.Header.Set("Content-Type", "application/json") + req = req.WithContext(contextWithRequestID()) + w := httptest.NewRecorder() + handler.RevokeCertificate(w, req) + + if w.Code != http.StatusBadRequest { + t.Errorf("expected 400 for already-revoked, got %d (body=%q)", w.Code, w.Body.String()) + } + assertSafeResponse(t, w, "RevokeCertificate/already_revoked") +} + +// TestRevokeCertificate_NotFound verifies 404 mapping. +func TestRevokeCertificate_NotFound(t *testing.T) { + handler, mock := newCertHandlerWithMock() + mock.RevokeCertificateFn = func(id string, reason string) error { + return fmt.Errorf("certificate not found") + } + + req := httptest.NewRequest(http.MethodPost, "/api/v1/certificates/mc-missing/revoke", strings.NewReader(`{"reason":"keyCompromise"}`)) + req.URL.Path = "/api/v1/certificates/mc-missing/revoke" + req.Header.Set("Content-Type", "application/json") + req = req.WithContext(contextWithRequestID()) + w := httptest.NewRecorder() + handler.RevokeCertificate(w, req) + + if w.Code != http.StatusNotFound { + t.Errorf("expected 404 for not-found, got %d (body=%q)", w.Code, w.Body.String()) + } + assertSafeResponse(t, w, "RevokeCertificate/not_found") +} diff --git a/internal/api/handler/issuer_handler_test.go b/internal/api/handler/issuer_handler_test.go index 91c1222..c600fa9 100644 --- a/internal/api/handler/issuer_handler_test.go +++ b/internal/api/handler/issuer_handler_test.go @@ -3,8 +3,10 @@ package handler import ( "bytes" "encoding/json" + "fmt" "net/http" "net/http/httptest" + "strings" "testing" "time" @@ -324,6 +326,122 @@ func TestCreateIssuer_NameTooLong(t *testing.T) { } } +func TestCreateIssuer_DuplicateName(t *testing.T) { + mock := &MockIssuerService{ + CreateIssuerFn: func(issuer domain.Issuer) (*domain.Issuer, error) { + return nil, fmt.Errorf("failed to create issuer: duplicate key value violates unique constraint \"issuers_name_key\"") + }, + } + + body := map[string]interface{}{ + "name": "ACME Issuer", + "type": "ACME", + } + bodyBytes, _ := json.Marshal(body) + + handler := NewIssuerHandler(mock) + req := httptest.NewRequest(http.MethodPost, "/api/v1/issuers", bytes.NewReader(bodyBytes)) + req = req.WithContext(contextWithRequestID()) + w := httptest.NewRecorder() + + handler.CreateIssuer(w, req) + + if w.Code != http.StatusConflict { + t.Fatalf("expected status 409, got %d", w.Code) + } + + var resp ErrorResponse + if err := json.NewDecoder(w.Body).Decode(&resp); err != nil { + t.Fatalf("failed to decode response: %v", err) + } + if !strings.Contains(resp.Message, "already exists") { + t.Errorf("expected message to contain 'already exists', got %q", resp.Message) + } +} + +func TestCreateIssuer_UnsupportedType(t *testing.T) { + mock := &MockIssuerService{ + CreateIssuerFn: func(issuer domain.Issuer) (*domain.Issuer, error) { + return nil, fmt.Errorf("unsupported issuer type: FakeCA") + }, + } + + body := map[string]interface{}{ + "name": "Fake Issuer", + "type": "FakeCA", + } + bodyBytes, _ := json.Marshal(body) + + handler := NewIssuerHandler(mock) + req := httptest.NewRequest(http.MethodPost, "/api/v1/issuers", bytes.NewReader(bodyBytes)) + req = req.WithContext(contextWithRequestID()) + w := httptest.NewRecorder() + + handler.CreateIssuer(w, req) + + if w.Code != http.StatusBadRequest { + t.Fatalf("expected status 400, got %d", w.Code) + } + + var resp ErrorResponse + if err := json.NewDecoder(w.Body).Decode(&resp); err != nil { + t.Fatalf("failed to decode response: %v", err) + } + if !strings.Contains(resp.Message, "unsupported issuer type") { + t.Errorf("expected message to contain 'unsupported issuer type', got %q", resp.Message) + } +} + +func TestCreateIssuer_GenericServiceError(t *testing.T) { + mock := &MockIssuerService{ + CreateIssuerFn: func(issuer domain.Issuer) (*domain.Issuer, error) { + return nil, fmt.Errorf("failed to encrypt config: cipher error") + }, + } + + body := map[string]interface{}{ + "name": "Some Issuer", + "type": "ACME", + } + bodyBytes, _ := json.Marshal(body) + + handler := NewIssuerHandler(mock) + req := httptest.NewRequest(http.MethodPost, "/api/v1/issuers", bytes.NewReader(bodyBytes)) + req = req.WithContext(contextWithRequestID()) + w := httptest.NewRecorder() + + handler.CreateIssuer(w, req) + + if w.Code != http.StatusInternalServerError { + t.Fatalf("expected status 500, got %d", w.Code) + } +} + +func TestUpdateIssuer_DuplicateName(t *testing.T) { + mock := &MockIssuerService{ + UpdateIssuerFn: func(id string, issuer domain.Issuer) (*domain.Issuer, error) { + return nil, fmt.Errorf("failed to update issuer: duplicate key value violates unique constraint") + }, + } + + body := map[string]interface{}{ + "name": "Existing Name", + "type": "ACME", + } + bodyBytes, _ := json.Marshal(body) + + handler := NewIssuerHandler(mock) + req := httptest.NewRequest(http.MethodPut, "/api/v1/issuers/iss-test", bytes.NewReader(bodyBytes)) + req = req.WithContext(contextWithRequestID()) + w := httptest.NewRecorder() + + handler.UpdateIssuer(w, req) + + if w.Code != http.StatusConflict { + t.Fatalf("expected status 409, got %d", w.Code) + } +} + func TestDeleteIssuer_Success(t *testing.T) { var deletedID string mock := &MockIssuerService{ diff --git a/internal/api/handler/issuers.go b/internal/api/handler/issuers.go index 4654fc2..ad5a4e4 100644 --- a/internal/api/handler/issuers.go +++ b/internal/api/handler/issuers.go @@ -2,6 +2,7 @@ package handler import ( "encoding/json" + "log/slog" "net/http" "strconv" "strings" @@ -22,12 +23,18 @@ type IssuerService interface { // IssuerHandler handles HTTP requests for issuer operations. type IssuerHandler struct { - svc IssuerService + svc IssuerService + logger *slog.Logger } // NewIssuerHandler creates a new IssuerHandler with a service dependency. func NewIssuerHandler(svc IssuerService) IssuerHandler { - return IssuerHandler{svc: svc} + return IssuerHandler{svc: svc, logger: slog.Default()} +} + +// NewIssuerHandlerWithLogger creates a new IssuerHandler with a custom logger. +func NewIssuerHandlerWithLogger(svc IssuerService, logger *slog.Logger) IssuerHandler { + return IssuerHandler{svc: svc, logger: logger} } // ListIssuers lists all configured issuers. @@ -127,7 +134,16 @@ func (h IssuerHandler) CreateIssuer(w http.ResponseWriter, r *http.Request) { created, err := h.svc.CreateIssuer(issuer) if err != nil { - ErrorWithRequestID(w, http.StatusInternalServerError, "Failed to create issuer", requestID) + h.logger.Error("failed to create issuer", "error", err, "name", issuer.Name, "type", issuer.Type) + errMsg := err.Error() + switch { + case strings.Contains(errMsg, "unique") || strings.Contains(errMsg, "duplicate"): + ErrorWithRequestID(w, http.StatusConflict, "An issuer with this name already exists", requestID) + case strings.Contains(errMsg, "unsupported issuer type"): + ErrorWithRequestID(w, http.StatusBadRequest, errMsg, requestID) + default: + ErrorWithRequestID(w, http.StatusInternalServerError, "Failed to create issuer", requestID) + } return } @@ -160,7 +176,16 @@ func (h IssuerHandler) UpdateIssuer(w http.ResponseWriter, r *http.Request) { updated, err := h.svc.UpdateIssuer(id, issuer) if err != nil { - ErrorWithRequestID(w, http.StatusInternalServerError, "Failed to update issuer", requestID) + h.logger.Error("failed to update issuer", "error", err, "id", id) + errMsg := err.Error() + switch { + case strings.Contains(errMsg, "unique") || strings.Contains(errMsg, "duplicate"): + ErrorWithRequestID(w, http.StatusConflict, "An issuer with this name already exists", requestID) + case strings.Contains(errMsg, "not found"): + ErrorWithRequestID(w, http.StatusNotFound, "Issuer not found", requestID) + default: + ErrorWithRequestID(w, http.StatusInternalServerError, "Failed to update issuer", requestID) + } return } diff --git a/web/src/pages/OnboardingWizard.tsx b/web/src/pages/OnboardingWizard.tsx index 31edd13..b75d80a 100644 --- a/web/src/pages/OnboardingWizard.tsx +++ b/web/src/pages/OnboardingWizard.tsx @@ -115,6 +115,15 @@ function IssuerStep({ onNext, onSkip, onIssuerCreated }: { const [selectedType, setSelectedType] = useState(null); const [configValues, setConfigValues] = useState>({}); const [issuerName, setIssuerName] = useState(''); + + // Pre-populate default values when a type is selected (matches IssuersPage behavior) + function handleTypeSelect(typeId: string) { + setSelectedType(typeId); + const tc = issuerTypes.find(t => t.id === typeId); + const defaults: Record = {}; + tc?.configFields.forEach(f => { if (f.defaultValue !== undefined) defaults[f.key] = f.defaultValue; }); + setConfigValues(defaults); + } const [error, setError] = useState(''); const [testResult, setTestResult] = useState<{ ok: boolean; msg: string } | null>(null); const [createdIssuer, setCreatedIssuer] = useState(null); @@ -196,7 +205,7 @@ function IssuerStep({ onNext, onSkip, onIssuerCreated }: { {issuerTypes.filter(t => !t.comingSoon).map((type: IssuerTypeConfig) => (