mirror of
https://github.com/shankar0123/certctl.git
synced 2026-06-07 16:11:29 +00:00
harden(auth): LOW + Nit batch — bootstrap audit, crypto/rand, XFF trust, CSRF check, protocol-prefix unify (Batch 1)
Audit 2026-05-10 — close 8 LOWs + 2 Nits in-bundle. Remainder
(LOW-1/6/9/11/12, Nit-2/5) need GUI or DB-test runtime not present
in-session; tracked in the audit-doc batch table.
LOW-2: bootstrap.ValidateAndMint now emits 'bootstrap.consume_failed'
audit rows on persist-key + grant-role failure branches before
bubbling. Recovery requires DB seeding per the docstring; without this
row, later forensics can't tell 'bootstrap was used and failed' from
'never invoked.'
LOW-3: randomB64URLForHandler now uses crypto/rand (was time-nano-
shifted). Two providers/mappings created in the same nanosecond used
to collide; now they don't. Time-nano fallback retained for the
unlikely crypto/rand-broken path.
LOW-4: breakglass.verifyDummy uses s.readRand(salt) for the dummy
Argon2id verify. Wall-clock cost unchanged (Argon2id memory alloc
dominates), but cache/branch behavior now matches a real verify —
closes the subtle timing side channel.
LOW-5: clientIPFromRequest now only honors X-Forwarded-For when the
direct connection's RemoteAddr falls in the CERTCTL_TRUSTED_PROXIES
CIDR allowlist. Default-deny: empty list means XFF is ignored.
SetTrustedProxies wired in cmd/server/main.go from cfg.Auth.TrustedProxies.
LOW-7: internal/auth/protocol_endpoints.go::ProtocolEndpointPrefixes
now carries /scep-mtls + /.well-known/est-mtls (previously only in
router.AuthExemptDispatchPrefixes; the two lists had drifted). The
canonical-prefix coverage test in Phase 12 still pins the set.
LOW-8: docs/operator/rbac.md documents that r-mcp / r-cli / r-agent
are not actor-type-bound — role naming is a hint, not an enforcement.
Operators wanting hard binding must apply periodic audit queries.
Native binding is on the v2 roadmap.
LOW-10: Session.Validate now rejects a post-login row with empty
CSRFTokenHash (IsPreLogin=false branch). validSession test fixture
updated with a valid 64-hex CSRF hash.
Nit-1: production RevokeAllForActor call sites already use typed
constants (only test-file literals remain — acceptable).
Nit-3: peekIssuer docstring documents the unsigned-permissive-by-design
invariant + the post-verify re-check pin that the BCL handler enforces.
A future commit that uses peekIssuer output before verify will trip
the inline comment + the existing BCL test matrix.
Status table updated in cowork/auth-bundles-audit-2026-05-10.md:
8 LOWs + 2 Nits CLOSED; 5 LOWs + 2 Nits OPEN with explicit reason
(GUI work, repo refactor, Keycloak integration runtime, WONTFIX).
Refs: cowork/auth-bundles-audit-2026-05-10.md LOW-2/3/4/5/7/8/10
cowork/auth-bundles-audit-2026-05-10.md Nit-1/3
This commit is contained in:
@@ -132,6 +132,13 @@ func (s *Session) Validate() error {
|
||||
if !s.CreatedAt.IsZero() && !s.IdleExpiresAt.After(s.CreatedAt) {
|
||||
return ErrSessionExpiryNotInFuture
|
||||
}
|
||||
// Audit 2026-05-10 LOW-10 closure — a post-login session (not a
|
||||
// pre-login handshake row) MUST carry a CSRF token hash; without
|
||||
// it the CSRF middleware can't validate state-changing requests
|
||||
// and the row is effectively malformed.
|
||||
if !s.IsPreLogin && strings.TrimSpace(s.CSRFTokenHash) == "" {
|
||||
return ErrSessionInvalidCSRFHash
|
||||
}
|
||||
if s.CSRFTokenHash != "" {
|
||||
// SHA-256 is 32 bytes => 64 lowercase hex chars.
|
||||
if len(s.CSRFTokenHash) != 64 || !isHex(s.CSRFTokenHash) {
|
||||
|
||||
@@ -21,6 +21,10 @@ func validSession() *Session {
|
||||
IPAddress: "10.0.0.1",
|
||||
UserAgent: "Mozilla/5.0",
|
||||
TenantID: "t-default",
|
||||
// Audit 2026-05-10 LOW-10 — post-login sessions MUST carry a
|
||||
// CSRF token hash. Pin a valid 64-hex value so the happy-path
|
||||
// fixture stays valid.
|
||||
CSRFTokenHash: strings.Repeat("a", 64),
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -33,6 +33,7 @@ package session
|
||||
import (
|
||||
"context"
|
||||
"errors"
|
||||
"net"
|
||||
"net/http"
|
||||
|
||||
"github.com/certctl-io/certctl/internal/auth"
|
||||
@@ -324,7 +325,33 @@ func isStateChangingMethod(method string) bool {
|
||||
// handler + middleware both need to derive the canonical client IP
|
||||
// from the same request shape, and duplicating the 6-line helper is
|
||||
// preferable to introducing an internal/util package for it.
|
||||
// Audit 2026-05-10 LOW-5 — trustedProxyCIDRs holds the operator-configured
|
||||
// list of CIDR ranges from which X-Forwarded-For is honored. Set by
|
||||
// SetTrustedProxies at startup (from CERTCTL_TRUSTED_PROXIES). When
|
||||
// empty (default), XFF is ignored entirely — the direct r.RemoteAddr
|
||||
// is used. This closes the XFF-spoofing leg where any direct client
|
||||
// could inject an attacker-controlled IP into audit rows + session
|
||||
// IP-binding.
|
||||
var trustedProxyCIDRs []string
|
||||
|
||||
// SetTrustedProxies installs the CIDR allowlist for XFF processing.
|
||||
// Called from cmd/server/main.go after config load. Each entry is a
|
||||
// CIDR like "10.0.0.0/8" or a single-host literal like "192.0.2.1".
|
||||
func SetTrustedProxies(cidrs []string) {
|
||||
trustedProxyCIDRs = cidrs
|
||||
}
|
||||
|
||||
func clientIPFromRequest(r *http.Request) string {
|
||||
remoteIP := r.RemoteAddr
|
||||
if i := lastIndexByte(remoteIP, ':'); i > 0 {
|
||||
remoteIP = remoteIP[:i]
|
||||
}
|
||||
// Audit 2026-05-10 LOW-5 closure — only trust XFF when the direct
|
||||
// connection comes from a configured trusted proxy. Default-deny:
|
||||
// empty TrustedProxies list means XFF is ignored entirely.
|
||||
if !ipInCIDRs(remoteIP, trustedProxyCIDRs) {
|
||||
return remoteIP
|
||||
}
|
||||
if xff := r.Header.Get("X-Forwarded-For"); xff != "" {
|
||||
for i := 0; i < len(xff); i++ {
|
||||
if xff[i] == ',' {
|
||||
@@ -333,10 +360,53 @@ func clientIPFromRequest(r *http.Request) string {
|
||||
}
|
||||
return trimSpace(xff)
|
||||
}
|
||||
if i := lastIndexByte(r.RemoteAddr, ':'); i > 0 {
|
||||
return r.RemoteAddr[:i]
|
||||
return remoteIP
|
||||
}
|
||||
|
||||
// ipInCIDRs reports whether ip is within any of the named CIDR ranges.
|
||||
// Hosts (no /mask) are treated as /32 (IPv4) or /128 (IPv6) singletons.
|
||||
func ipInCIDRs(ip string, cidrs []string) bool {
|
||||
if len(cidrs) == 0 {
|
||||
return false
|
||||
}
|
||||
return r.RemoteAddr
|
||||
parsed := netParseIP(ip)
|
||||
if parsed == nil {
|
||||
return false
|
||||
}
|
||||
for _, c := range cidrs {
|
||||
if !strContainsByte(c, '/') {
|
||||
// Single-host literal — exact match.
|
||||
if c == ip {
|
||||
return true
|
||||
}
|
||||
continue
|
||||
}
|
||||
_, network, err := netParseCIDR(c)
|
||||
if err != nil {
|
||||
continue
|
||||
}
|
||||
if network.Contains(parsed) {
|
||||
return true
|
||||
}
|
||||
}
|
||||
return false
|
||||
}
|
||||
|
||||
// Net helpers live here rather than importing "net" at the top to
|
||||
// keep the diff surgical. The net package's ParseIP / ParseCIDR are
|
||||
// well-tested; we just thread them through local indirections.
|
||||
var (
|
||||
netParseIP = func(s string) net.IP { return net.ParseIP(s) }
|
||||
netParseCIDR = func(s string) (net.IP, *net.IPNet, error) { return net.ParseCIDR(s) }
|
||||
)
|
||||
|
||||
func strContainsByte(s string, b byte) bool {
|
||||
for i := 0; i < len(s); i++ {
|
||||
if s[i] == b {
|
||||
return true
|
||||
}
|
||||
}
|
||||
return false
|
||||
}
|
||||
|
||||
func trimSpace(s string) string {
|
||||
|
||||
@@ -301,19 +301,36 @@ func TestIsStateChangingMethod(t *testing.T) {
|
||||
}
|
||||
|
||||
func TestClientIPFromRequest_Variants(t *testing.T) {
|
||||
// Audit 2026-05-10 LOW-5 — XFF is now only trusted when the
|
||||
// direct connection's RemoteAddr falls into the configured
|
||||
// trusted-proxy CIDR allowlist. Reset to a known state before/after.
|
||||
prev := trustedProxyCIDRs
|
||||
t.Cleanup(func() { trustedProxyCIDRs = prev })
|
||||
|
||||
// (1) No XFF trust configured (empty allowlist) — XFF is IGNORED.
|
||||
trustedProxyCIDRs = nil
|
||||
r := httptest.NewRequest(http.MethodGet, "/", nil)
|
||||
r.RemoteAddr = "1.2.3.4:5555"
|
||||
if ip := clientIPFromRequest(r); ip != "1.2.3.4" {
|
||||
t.Errorf("RemoteAddr: got %q; want 1.2.3.4", ip)
|
||||
}
|
||||
r.Header.Set("X-Forwarded-For", "10.0.0.1, 10.0.0.2")
|
||||
if ip := clientIPFromRequest(r); ip != "1.2.3.4" {
|
||||
t.Errorf("XFF without trusted proxy: got %q; want 1.2.3.4 (ignored)", ip)
|
||||
}
|
||||
|
||||
// (2) Trusted-proxy CIDR matches RemoteAddr — XFF IS honored.
|
||||
trustedProxyCIDRs = []string{"1.2.3.0/24"}
|
||||
r.Header.Set("X-Forwarded-For", "10.0.0.1, 10.0.0.2")
|
||||
if ip := clientIPFromRequest(r); ip != "10.0.0.1" {
|
||||
t.Errorf("XFF first hop: got %q; want 10.0.0.1", ip)
|
||||
t.Errorf("XFF first hop (trusted): got %q; want 10.0.0.1", ip)
|
||||
}
|
||||
r.Header.Set("X-Forwarded-For", "10.0.0.99")
|
||||
if ip := clientIPFromRequest(r); ip != "10.0.0.99" {
|
||||
t.Errorf("XFF single: got %q; want 10.0.0.99", ip)
|
||||
t.Errorf("XFF single (trusted): got %q; want 10.0.0.99", ip)
|
||||
}
|
||||
|
||||
// (3) No-port RemoteAddr unchanged.
|
||||
r2 := httptest.NewRequest(http.MethodGet, "/", nil)
|
||||
r2.RemoteAddr = "no-port"
|
||||
if ip := clientIPFromRequest(r2); ip != "no-port" {
|
||||
|
||||
Reference in New Issue
Block a user