From a172b6ed3bd8578f0e678c89a6fc40a67ea0d205 Mon Sep 17 00:00:00 2001 From: shankar0123 Date: Sun, 26 Apr 2026 23:35:13 +0000 Subject: [PATCH] Bundle B CI follow-up: G-3 env-var docs + M-028 closure (final 5 SA1019 sites) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two CI failures on master after Bundle B merge: 1. Frontend Build / G-3 env-var docs guardrail Bundle B introduced CERTCTL_RATE_LIMIT_PER_USER_RPS and CERTCTL_RATE_LIMIT_PER_USER_BURST without adding them to docs/features.md. The guardrail step that scans Go source for getEnv* calls and asserts each appears in a doc page failed. Fix: docs/features.md rate-limit section extended with both new env vars + a paragraph explaining the per-key keying contract from M-025. 2. Go Build & Test / staticcheck SA1019 hits (6 errors) The CI workflow runs staticcheck without continue-on-error. Bundle 7 opened M-028 to track 6 deprecated-API sites; Bundle 9 closed 1 of them (the elliptic.Marshal in local.go) but kept a deliberate regression-oracle reference in bundle9_coverage_test.go protected only by golangci-lint's //nolint comment — staticcheck-as-CLI does not honor that, only its native //lint:ignore directive. Closure of remaining 5 sites: cmd/server/main_test.go:47, 163, 192, 465 — 4 × middleware.NewAuth migrated to middleware.NewAuthWithNamedKeys with explicit NamedAPIKey entries. The auth=none case at line 465 maps to a nil NamedAPIKey slice (no-op pass-through, matches the NewAuthWithNamedKeys contract for empty input). Audit count was 3; recon found a 4th at line 465 that was missed. internal/api/handler/scep.go:266 — csr.Attributes is a real RFC 2985 §5.4.1 challengePassword carve-out. Go's stdlib deprecation note explicitly applies only to OID 1.2.840.113549.1.9.14 (requestedExtensions), NOT to OID 1.2.840.113549.1.9.7 (challengePassword), for which there is no non-deprecated stdlib API. Suppressed with native //lint:ignore SA1019 + comment block citing the RFC. internal/connector/issuer/local/bundle9_coverage_test.go:342 — deliberate regression-oracle that calls elliptic.Marshal to prove the new crypto/ecdh path is byte-identical. Comment converted from //nolint:staticcheck to native //lint:ignore SA1019 so staticcheck-as-CLI honors the suppression. Audit deliverables: cowork/comprehensive-audit-2026-04-25/audit-report.md: M-028 box flipped [x]; score 30/55 -> 31/55 (Medium 12/27 -> 13/27). cowork/comprehensive-audit-2026-04-25/findings.yaml: M-028 status partial_closed -> closed with closure note. Verification: go test -count=1 -short ./cmd/server ./internal/api/handler ./internal/connector/issuer/local ./internal/api/middleware ./internal/config — all green. staticcheck on each changed package — 0 SA1019 hits. Bundle C had M-028 in scope; this CI-fix lift moves it forward so master CI goes green immediately. Bundle C scope adjusts to remove M-028 and focuses on M-006 / M-015 / M-016 / M-019 / M-020 plus the M-007 / M-008 coverage gaps. --- cmd/server/main_test.go | 20 ++++++++----------- docs/features.md | 13 ++++++++++-- internal/api/handler/scep.go | 12 +++++++++++ .../issuer/local/bundle9_coverage_test.go | 11 +++++----- 4 files changed, 37 insertions(+), 19 deletions(-) diff --git a/cmd/server/main_test.go b/cmd/server/main_test.go index f39a216..04e0346 100644 --- a/cmd/server/main_test.go +++ b/cmd/server/main_test.go @@ -44,9 +44,8 @@ func TestMain_HealthEndpointBypassesAuth(t *testing.T) { }) // Build the handler chain the same way main.go does - authMiddleware := middleware.NewAuth(middleware.AuthConfig{ - Type: "api-key", - Secret: "test-secret-key", + authMiddleware := middleware.NewAuthWithNamedKeys([]middleware.NamedAPIKey{ + {Name: "test", Key: "test-secret-key"}, }) // API handler with auth @@ -160,9 +159,8 @@ func TestMain_AuthMiddlewareRejectsUnauthorized(t *testing.T) { }) // Wrap with auth middleware - authMiddleware := middleware.NewAuth(middleware.AuthConfig{ - Type: "api-key", - Secret: "test-secret-key", + authMiddleware := middleware.NewAuthWithNamedKeys([]middleware.NamedAPIKey{ + {Name: "test", Key: "test-secret-key"}, }) chainedHandler := middleware.Chain(protectedHandler, authMiddleware) @@ -189,9 +187,8 @@ func TestMain_AuthMiddlewareAllowsWithValidKey(t *testing.T) { }) // Wrap with auth middleware - authMiddleware := middleware.NewAuth(middleware.AuthConfig{ - Type: "api-key", - Secret: testKey, + authMiddleware := middleware.NewAuthWithNamedKeys([]middleware.NamedAPIKey{ + {Name: "test", Key: testKey}, }) chainedHandler := middleware.Chain(protectedHandler, authMiddleware) @@ -462,9 +459,8 @@ func TestMain_AuthNoneMode(t *testing.T) { }) // Wrap with auth middleware in "none" mode - authMiddleware := middleware.NewAuth(middleware.AuthConfig{ - Type: "none", - }) + // auth=none equivalent: empty named-keys list is a no-op pass-through. + authMiddleware := middleware.NewAuthWithNamedKeys(nil) chainedHandler := middleware.Chain(protectedHandler, authMiddleware) diff --git a/docs/features.md b/docs/features.md index 39f06dd..7c83f49 100644 --- a/docs/features.md +++ b/docs/features.md @@ -60,11 +60,20 @@ Two endpoints are served without auth so the GUI can detect auth mode before log Token bucket algorithm protecting the control plane from misbehaving clients. +Bundle B (Audit M-025 / OWASP ASVS L2 §11.2.1): per-key keying. Each +authenticated caller gets a bucket keyed on their API-key name; each +unauthenticated source IP gets its own bucket. Bucket creation is +on-demand under a `sync.RWMutex`; no eviction (the leak is bounded by +realistic operator IP fan-out — appropriate for the OWASP ASVS L2 threat +model of abuse-by-known-clients, not infinite-cardinality scanners). + | Env Var | Default | Description | |---|---|---| | `CERTCTL_RATE_LIMIT_ENABLED` | `true` | Enable/disable | -| `CERTCTL_RATE_LIMIT_RPS` | `50` | Requests per second | -| `CERTCTL_RATE_LIMIT_BURST` | `100` | Burst capacity | +| `CERTCTL_RATE_LIMIT_RPS` | `50` | Per-key requests per second (default applies to IP-keyed buckets; user-keyed buckets fall back to this when `PER_USER_RPS` is unset) | +| `CERTCTL_RATE_LIMIT_BURST` | `100` | Per-key burst capacity (default applies to IP-keyed buckets; user-keyed buckets fall back to this when `PER_USER_BURST` is unset) | +| `CERTCTL_RATE_LIMIT_PER_USER_RPS` | `0` | Override RPS for authenticated callers. `0` means "use `RATE_LIMIT_RPS`". Set higher than `RATE_LIMIT_RPS` to grant authenticated clients a more generous budget than anonymous probes. | +| `CERTCTL_RATE_LIMIT_PER_USER_BURST` | `0` | Override burst for authenticated callers. `0` means "use `RATE_LIMIT_BURST`". | Exceeded requests receive `429 Too Many Requests` with a `Retry-After` header. diff --git a/internal/api/handler/scep.go b/internal/api/handler/scep.go index da91ede..7fb5e03 100644 --- a/internal/api/handler/scep.go +++ b/internal/api/handler/scep.go @@ -263,6 +263,18 @@ func extractCSRFields(csrDER []byte) ([]byte, string, string, error) { // Attributes is []pkix.AttributeTypeAndValueSET where each has Type (OID) // and Value ([][]pkix.AttributeTypeAndValue). The challenge password value // is stored as a string in the inner AttributeTypeAndValue.Value field. + // + // Audit M-028 carve-out: Go's stdlib deprecates `csr.Attributes` for the + // specific use case of parsing the "requestedExtensions" CSR attribute + // (OID 1.2.840.113549.1.9.14), pointing callers at `csr.Extensions` / + // `csr.ExtraExtensions`. challengePassword (OID 1.2.840.113549.1.9.7) + // per RFC 2985 §5.4.1 is a SEPARATE CSR attribute that cannot be + // retrieved via Extensions. There is no non-deprecated stdlib API for + // it; callers either accept the deprecation warning or parse the raw + // `csr.RawAttributes` ASN.1 themselves. We accept the warning; the + // staticcheck.conf and golangci-lint rules suppress SA1019 for this + // specific line per the audit closure note. + //lint:ignore SA1019 RFC 2985 challengePassword has no non-deprecated stdlib API; see comment above. for _, attr := range csr.Attributes { if attr.Type.Equal(oidChallengePassword) { if len(attr.Value) > 0 && len(attr.Value[0]) > 0 { diff --git a/internal/connector/issuer/local/bundle9_coverage_test.go b/internal/connector/issuer/local/bundle9_coverage_test.go index 7f3bed0..22b29e9 100644 --- a/internal/connector/issuer/local/bundle9_coverage_test.go +++ b/internal/connector/issuer/local/bundle9_coverage_test.go @@ -334,11 +334,12 @@ func TestHashPublicKey_ECDSA_RoundTripPin(t *testing.T) { t.Fatalf("ecdsaToECDH: %v", err) } ecdhBytes := ecdhPub.Bytes() - //nolint:staticcheck // SA1019: pin assertion — we DELIBERATELY use - // the deprecated API here as a regression oracle to prove the - // new crypto/ecdh path produces byte-identical output. If - // elliptic.Marshal is removed in a future Go release this test - // must be deleted (and the migration is then irreversibly proven). + // Pin assertion — we DELIBERATELY use the deprecated API here + // as a regression oracle to prove the new crypto/ecdh path + // produces byte-identical output. If elliptic.Marshal is + // removed in a future Go release this test must be deleted + // (and the migration is then irreversibly proven). + //lint:ignore SA1019 deliberate regression oracle for M-028 round-trip pin legacy := elliptic.Marshal(k.Curve, k.X, k.Y) if !bytes.Equal(ecdhBytes, legacy) { t.Fatalf("ECDH .Bytes() != legacy elliptic.Marshal output\n new: %x\n old: %x", ecdhBytes, legacy)