Skip to content

Commit 64bc545

Browse files
committed
fix(security): derive Helm proxy trust from ingress.enabled, fail the password ceiling closed
Two gaps in the previous commit. The chart defaulted TRUST_PROXY_HEADERS to true, but ingress.enabled defaults to FALSE — so the out-of-the-box install reaches the Service directly (port-forward, LoadBalancer, NodePort) with nothing appending a peer address, and trusted a header written entirely by the caller. Derive the default from ingress.enabled instead: on with the ingress, off without it. An explicit app.env value still wins, for edges the chart cannot see (Gateway API, a service mesh, an external LB that appends). Compare the stringified override, never the raw one — an explicit `false` is falsy in Go templates, so the obvious `if $explicit` silently discarded the one override that turns trust off. Caught by rendering all four combinations; the schema now also accepts a bare YAML boolean, which is what a Helm user writes. The per-resource password ceiling called checkRateLimitDirect without failClosed, and that helper allows on storage error. It is the only bound on distributed guessing at the secret, so failing open removed it during exactly the outage an attacker could wait for. Matches the contact captcha backstop, which already opts in for the same reason.
1 parent eb578bd commit 64bc545

6 files changed

Lines changed: 71 additions & 10 deletions

File tree

apps/sim/app/api/chat/utils.test.ts

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -253,11 +253,35 @@ describe('Chat API Utils', () => {
253253
expect(result.authorized).toBe(false)
254254
expect(mockCheckRateLimitDirect).toHaveBeenCalledWith(
255255
'chat-password:resource:chat-id',
256-
expect.objectContaining({ maxTokens: 500 })
256+
expect.objectContaining({ maxTokens: 500 }),
257+
{ failClosed: true }
257258
)
258259
expect(mockResetRateLimitBucket).not.toHaveBeenCalled()
259260
})
260261

262+
it('checks the per-resource ceiling fail-closed so an outage cannot lift it', async () => {
263+
// It is the only bound on distributed guessing at the secret; failing open
264+
// would silently remove it during exactly the outage an attacker waits for.
265+
const deployment = {
266+
id: 'chat-id',
267+
authType: 'password',
268+
password: 'encrypted-password',
269+
}
270+
const mockRequest = {
271+
method: 'POST',
272+
cookies: { get: vi.fn().mockReturnValue(null) },
273+
} as any
274+
275+
await validateChatAuth('request-id', deployment, mockRequest, {
276+
password: 'correct-password',
277+
})
278+
279+
const resourceCall = mockCheckRateLimitDirect.mock.calls.find((call: unknown[]) =>
280+
String(call[0]).includes(':resource:')
281+
)
282+
expect(resourceCall?.[2]).toEqual({ failClosed: true })
283+
})
284+
261285
it('rejects guesses once the per-resource counter is exhausted, without decrypting', async () => {
262286
const deployment = {
263287
id: 'chat-id',

apps/sim/lib/core/security/deployment-auth.ts

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -148,9 +148,17 @@ export async function validateDeploymentAuth(
148148
}
149149

150150
const resourceKey = `${cookiePrefix}-password:resource:${resource.id}`
151+
/**
152+
* `failClosed` because this is the only bound on distributed guessing at
153+
* the secret: failing open would silently remove it during exactly the
154+
* storage outage an attacker could wait for. The cost is bounded — the
155+
* bucket store is Redis or the app database, and if the database is down
156+
* the deployment is unreachable anyway.
157+
*/
151158
const resourceRateLimit = await rateLimiter.checkRateLimitDirect(
152159
resourceKey,
153-
PASSWORD_RESOURCE_RATE_LIMIT
160+
PASSWORD_RESOURCE_RATE_LIMIT,
161+
{ failClosed: true }
154162
)
155163
if (!resourceRateLimit.allowed) {
156164
logger.warn(

helm/sim/templates/_helpers.tpl

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -446,6 +446,31 @@ Ollama URL
446446
{{- end }}
447447
{{- end }}
448448

449+
{{/*
450+
Whether the app may believe x-forwarded-for / x-real-ip.
451+
452+
Derived from ingress.enabled rather than defaulted to "true": every rule about
453+
which forwarded hop to read presumes a proxy wrote one of them. With the ingress
454+
off, the Service is reached directly (ClusterIP port-forward, LoadBalancer,
455+
NodePort) and the header is authored entirely by the caller — trusting it would
456+
let anyone rotate it for a fresh per-IP rate-limit bucket per request. An
457+
explicit app.env.TRUST_PROXY_HEADERS always wins, for edges the chart cannot see
458+
(a Gateway API listener, a service mesh, an external LB that appends).
459+
*/}}
460+
{{- define "sim.trustProxyHeaders" -}}
461+
{{- $explicit := toString ((default (dict) .Values.app.env).TRUST_PROXY_HEADERS) -}}
462+
{{- /*
463+
Compare the STRINGIFIED value, never the raw one: an explicit `false` is falsy
464+
in Go templates, so `if $explicit` would silently discard the one override
465+
that turns trust off and fall through to the ingress default.
466+
*/ -}}
467+
{{- if or (eq $explicit "") (eq $explicit "<nil>") -}}
468+
{{- ternary "true" "false" .Values.ingress.enabled -}}
469+
{{- else -}}
470+
{{- $explicit -}}
471+
{{- end -}}
472+
{{- end }}
473+
449474
{{/*
450475
PII (Presidio) service URL
451476
*/}}

helm/sim/templates/deployment-app.yaml

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -91,6 +91,8 @@ spec:
9191
value: {{ include "sim.ollamaUrl" . | quote }}
9292
- name: PII_URL
9393
value: {{ include "sim.piiUrl" . | quote }}
94+
- name: TRUST_PROXY_HEADERS
95+
value: {{ include "sim.trustProxyHeaders" . | quote }}
9496
{{- /*
9597
Skip envDefaults keys that the user has explicitly overridden in app.env
9698
with a non-empty value. K8s `env` takes precedence over `envFrom`, so an

helm/sim/values.schema.json

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -160,8 +160,8 @@
160160
"description": "Comma-separated reverse-proxy IPs or CIDR ranges in front of the app (e.g. the ingress pods, '10.42.0.0/24'). When set, Better Auth and Sim's per-IP rate limits both walk x-forwarded-for right to left, skip these hops, and use the first untrusted address as the client IP. Leave empty and the two differ: Better Auth trusts only a single-value header, while Sim's throttles key on the rightmost, proxy-written entry. Do not use a range broad enough to also cover client traffic — a caller inside a trusted range makes the whole chain trusted."
161161
},
162162
"TRUST_PROXY_HEADERS": {
163-
"type": "string",
164-
"description": "Whether x-forwarded-for / x-real-ip may be believed at all. Empty (the default) means yes, which is correct behind the chart's ingress. Set to 'false' only when the app is exposed with no proxy appending the peer address, where those headers are entirely caller-written; per-IP rate limits then collapse to one shared bucket rather than being bypassable per request."
163+
"type": ["string", "boolean"],
164+
"description": "Whether x-forwarded-for / x-real-ip may be believed at all. Leave empty to derive it from ingress.enabled: on with the ingress (which appends the peer address), off without it, since a directly-reached Service sees a header written entirely by the caller and trusting it makes per-IP rate limits bypassable per request. Set explicitly ('true'/'false', quoted or bare) only for an edge the chart cannot see, e.g. a Gateway API listener, a service mesh, or an external load balancer that appends."
165165
},
166166
"SSO_TRUSTED_PROVIDER_IDS": {
167167
"type": "string",

helm/sim/values.yaml

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -91,12 +91,14 @@ app:
9191
# to also cover client traffic: a caller whose own address falls inside a trusted range makes
9292
# the whole chain trusted and can then forge the value Sim keys on.
9393
AUTH_TRUSTED_PROXIES: ""
94-
# TRUST_PROXY_HEADERS: whether x-forwarded-for / x-real-ip may be believed at all. Left empty
95-
# (= true), which is correct here because the chart runs behind an ingress that appends the peer
96-
# address. Set to "false" only if you expose the Service directly with no proxy in front: with
97-
# nothing appending, those headers are written entirely by the caller and believing them lets
98-
# anyone rotate a header for a fresh per-IP rate-limit bucket per request. While false, per-IP
99-
# limits collapse into one shared bucket — blunt, but it fails closed.
94+
# TRUST_PROXY_HEADERS: whether x-forwarded-for / x-real-ip may be believed at all.
95+
# Left empty it is DERIVED from ingress.enabled — on with the ingress (which appends the peer
96+
# address), off without it. That matters because ingress.enabled defaults to false: a Service
97+
# reached directly (port-forward, LoadBalancer, NodePort) sees a header written entirely by the
98+
# caller, and believing it lets anyone rotate a header for a fresh per-IP rate-limit bucket on
99+
# every request. While off, per-IP limits collapse into one shared bucket — blunt, but it fails
100+
# closed. Set it explicitly ("true"/"false") only for an edge the chart cannot see: a Gateway
101+
# API listener, a service mesh, or an external load balancer that appends the peer address.
100102
TRUST_PROXY_HEADERS: ""
101103
# SOCKET_SERVER_URL: Auto-detected when realtime.enabled=true (uses internal service)
102104
# NEXT_PUBLIC_SOCKET_URL: public WebSocket URL for browsers. Leave empty to default to the

0 commit comments

Comments
 (0)