Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions internal/ratelimit/key.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import (
"crypto/sha256"
"encoding/hex"
"net"
"net/http"
)

Expand All @@ -21,6 +22,20 @@
}
}

// IPKeySkipLoopback is IPKey with a loopback exemption: peers on
// 127.0.0.0/8 or ::1 return "" so the middleware fails open. Local
// callers (CLI, same-host dashboard) legitimately fan out many
// requests and aren't the DoS surface TierAuth is sized to protect.
func IPKeySkipLoopback(clientIP func(*http.Request) string) Keyer {
return func(r *http.Request) string {
ip := clientIP(r)
if parsed := net.ParseIP(ip); parsed != nil && parsed.IsLoopback() {
return ""
}
return "ip:" + ip
}
}

Check failure on line 37 in internal/ratelimit/key.go

View check run for this annotation

Claude / Claude Code Review

IPKeySkipLoopback uses XFF-derived IP, enabling loopback spoofing to bypass all ipAuth limits when AGENT_VAULT_TRUSTED_PROXIES is set

The new IPKeySkipLoopback function checks the XFF-derived clientIP() result for loopback rather than the actual TCP connection address (r.RemoteAddr); when AGENT_VAULT_TRUSTED_PROXIES is set and the proxy is misconfigured to forward client-supplied XFF verbatim (nginx proxy_set_header X-Forwarded-For $http_x_forwarded_for), an attacker can send X-Forwarded-For: 127.0.0.1 and clientIP() returns 127.0.0.1, causing IPKeySkipLoopback to return "" and the rate-limit middleware to fail open. The fix i
Comment thread
dangtony98 marked this conversation as resolved.
Outdated

// HashToken returns a base16 SHA-256 prefix of s. Used for token
// keys so raw secrets never sit in the limiter's memory or logs.
func HashToken(s string) string {
Expand Down
36 changes: 36 additions & 0 deletions internal/server/ipkeyer_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
package server

import (
"net/http/httptest"
"testing"
)

// TestIPKeyerLoopbackExempt verifies the server-wide ipKeyer skips
// rate-limit bucketing for loopback callers. Regression: `vault run`
// and the dashboard both hit public endpoints like /v1/mitm/ca.pem
// from 127.0.0.1 and tripped the 10-per-5min TierAuth ceiling.
func TestIPKeyerLoopbackExempt(t *testing.T) {
s := &Server{}
keyer := s.ipKeyer()

cases := []struct {
name string
remote string
wantKey string
}{
{"ipv4 loopback", "127.0.0.1:54321", ""},
{"ipv4 loopback range", "127.5.6.7:54321", ""},
{"ipv6 loopback", "[::1]:54321", ""},
{"non-loopback ipv4", "203.0.113.42:54321", "ip:203.0.113.42"},
{"non-loopback ipv6", "[2001:db8::1]:54321", "ip:2001:db8::1"},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
req := httptest.NewRequest("GET", "/v1/mitm/ca.pem", nil)
req.RemoteAddr = tc.remote
if got := keyer(req); got != tc.wantKey {
t.Fatalf("ipKeyer(%q) = %q, want %q", tc.remote, got, tc.wantKey)
}
})
}
}
26 changes: 15 additions & 11 deletions internal/server/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -649,14 +649,16 @@
mux.HandleFunc("DELETE /v1/vaults/{name}/services/{host}", s.requireInitialized(s.requireAuth(actorAuthed(s.handleServiceRemove))))
mux.HandleFunc("DELETE /v1/vaults/{name}/services", s.requireInitialized(s.requireAuth(actorAuthed(s.handleServicesClear))))
mux.HandleFunc("GET /v1/vaults/{name}/services/credential-usage", s.requireInitialized(s.requireAuth(actorAuthed(s.handleServicesCredentialUsage))))
mux.HandleFunc("GET /v1/service-catalog", s.requireInitialized(ipAuth(s.handleServiceCatalog)))
mux.HandleFunc("GET /v1/skills/cli", s.requireInitialized(ipAuth(s.handleSkillCLI)))
mux.HandleFunc("GET /v1/skills/http", s.requireInitialized(ipAuth(s.handleSkillHTTP)))

// Public: transparent-proxy root CA. Safe to expose; clients need it to
// trust the minted leaves. Not wrapped in requireInitialized — the CA
// lifecycle is tied to --mitm-port, not owner registration.
mux.HandleFunc("GET /v1/mitm/ca.pem", ipAuth(s.handleMITMCA))
// Public static reads — immutable payloads with no credentials on
// the wire. TierGlobal is the only useful backstop; TierAuth would
// punish `vault run` (CA fetch per invocation) and the dashboard
// (re-mount poll) without defending any real surface.
mux.HandleFunc("GET /v1/service-catalog", s.requireInitialized(s.handleServiceCatalog))
mux.HandleFunc("GET /v1/skills/cli", s.requireInitialized(s.handleSkillCLI))
mux.HandleFunc("GET /v1/skills/http", s.requireInitialized(s.handleSkillHTTP))
// CA PEM is not wrapped in requireInitialized — the CA lifecycle is
// tied to --mitm-port, not owner registration.
mux.HandleFunc("GET /v1/mitm/ca.pem", s.handleMITMCA)

// Instance-level user invites
mux.HandleFunc("POST /v1/users/invites", s.requireInitialized(s.requireAuth(actorAuthed(limitBody(s.handleUserInviteCreate)))))
Expand Down Expand Up @@ -838,11 +840,13 @@
}
}

// ipKeyer returns a ratelimit.Keyer that keys on the request's client IP
// (honoring AGENT_VAULT_TRUSTED_PROXIES via clientIP).
// ipKeyer keys on the request's client IP (honoring
// AGENT_VAULT_TRUSTED_PROXIES via clientIP), with loopback exempt.
// Login/register still enforce their own in-handler buckets directly
// on the raw IP, so credential-bearing endpoints keep defense-in-depth.
func (s *Server) ipKeyer() ratelimit.Keyer {
return ratelimit.IPKey(clientIP)
return ratelimit.IPKeySkipLoopback(clientIP)
}

Check failure on line 849 in internal/server/server.go

View check run for this annotation

Claude / Claude Code Review

ipKeyer loopback exemption removes rate limiting from register, forgot-password, and resend-verification

The PR's loopback exemption in `ipKeyer()` (server.go:847-849) is overly broad: it bypasses TierAuth for ALL routes using `ipAuth`, not just the four static reads that motivated the change. The PR description claims 'Login/register still enforce their own in-handler buckets directly on the raw IP,' but this is only true for `handleLogin` (handle_auth.go:594-596); `handleRegister`, `handleForgotPassword`, `handleResendVerification`, `handleResetPassword`, and `handleVerify` have NO in-handler IP
Comment thread
dangtony98 marked this conversation as resolved.

// actorKeyer returns a ratelimit.Keyer that keys on the authenticated
// actor (user or agent). Returns "" if no session is on the context;
Expand Down