Commit a26ee1d
authored
* security: JWT issuer validation, JWKS rate limiting, audience check (#472)
Implement three security hardening items from issue #472:
SEC-003: Validate JWT iss claim before JWKS routing - must be HTTPS URL
with non-empty host, max 512 chars. Rejects http/file/javascript schemes.
SEC-004: Per-issuer JWKS fetch rate limiting with 10s cooldown to prevent
DoS amplification via tokens with valid issuers but unknown kid values.
SEC-005: When IdentityProvider clientID is configured, validate JWT aud
claim against it to prevent cross-service token confusion.
Items already addressed in prior PRs (confirmed, no changes needed):
- SEC-006: X-Request-ID sanitization (isValidRequestID)
- SEC-008: Build info endpoint (GetPublicBuildInfo)
- SEC-015: hardenedIDPHints defaults to true
- SEC-007: SAR webhook auth documented
- SEC-017: Token storage documented
Closes #472
* fix: make audience validation configurable via expectedAudience field
* fix: address Copilot review findings on JWT security hardening
- Add singleflight deduplication for concurrent JWKS fetches (SEC-004)
- Move jwksFetchLimiter.Store to after successful JWKS fetch
- Clean up rate limiter entries on LRU cache eviction
- Use bounded metric labels (resolved IDP name) instead of raw issuer
- Tighten isValidIssuer to reject query/fragment/userinfo components
- Apply audience validation in tryExtractUserIdentity
- Differentiate CHANGELOG SEC-005 entries (SEC-005a/SEC-005b)
- Add multi-IDP audience validation tests
- Expand rate limit test coverage (cooldown expiry)
* fix: address review comments on security hardening PR
- Canonicalize JWT issuer (trim trailing slashes) before cache/limiter
key lookup to prevent bypass via slash variants
- Rename Prometheus label from 'issuer' to 'identity_provider' on
JWT validation metrics to match actual emitted values (IDP names)
and prevent unbounded cardinality from attacker-controlled issuers
- Replace NUL bytes in long issuer test with repeated 'a' characters
for portability and clarity
- Update metrics.md to reflect label rename
* security: validate JWKS URI from OIDC discovery endpoint
Defense-in-depth: validate the jwks_uri returned by OIDC discovery
before using it, preventing SSRF if a configured IDP is compromised
and returns a malicious URI (e.g., file://, http://, internal hosts).
* fix: record metrics with resolved IDP name, canonicalize issuer in tryExtractUserIdentity, refresh expectedAudience on cache hit
* fix: TTL-based audience refresh on JWKS cache hit, consistent 'unknown' label for early failures, expanded JWT error classification
* fix: address review comments on SEC-003/SEC-004/SEC-005
- Fix data race: read entry.expectedAudience and audienceRefreshedAt under
jwksMutex before releasing the lock
- Normalize selectedIDP to 'unknown' for all JWT validation metrics (failure,
success, duration) for consistent bounded cardinality
- Rename isValidIssuer to isValidHTTPSURL since it validates both JWT issuer
and OIDC discovery jwks_uri URLs
- Update JWKS cache hit/miss metric labels from 'issuer' to 'identity_provider'
to match actual values (IDP names) and align with other JWT metrics
- Add clarifying comment on rate limiter scope (keyfunc background refreshes
are bounded by RefreshInterval)
* fix: address review comments - split JWKS/issuer URL validators, add request counters to early returns, rename test, update CHANGELOG
* docs: add metrics label rename to upgrade guide
* fix: cache IDP name to avoid double lookup, TrimSuffix for single slash, lowercase rate-limit error with retry hint
* fix: count JWKS-failure validation attempts, trim all trailing slashes, correct metrics docs
* fix: snapshot JWKS cache fields under lock, capture elapsed time once in rate-limiter
* fix: validate ExpectedAudience against whitespace, fix wasCacheHit race, align metric test labels
- Add Pattern=^\S+$ validation to ExpectedAudience field to reject
whitespace-only values, consistent with ClientID validation
- Return cacheHit from getJWKSForIssuer() instead of pre-checking cache
state, eliminating race between check and actual fetch
- Rename test variable from 'issuer' to 'idpName' in TestJWTMetrics to
match the 'identity_provider' label semantics
* fix: limit OIDC discovery response body size, use structured logging
- Add io.LimitReader (1 MiB) to OIDC discovery response decoding to
prevent OOM from malicious OIDC providers returning oversized payload
- Replace Warnf with Warnw for structured logging consistency
* fix: address review feedback for SEC hardening
- Normalize trailing slashes in LoadIdentityProviderByIssuer primary
match path (consistent with auth layer canonicalization)
- Enforce JWKS URI host matches authority host to prevent SSRF via
compromised OIDC discovery endpoints
- Wrap audience refresh in singleflight to prevent thundering herd
when many requests arrive after refresh interval elapses
- Add unit tests for jwksHostMatchesAuthority, trailing slash
normalization, and authority fallback normalization
- Update CHANGELOG with new security entries
* fix: address second round of review feedback
- Rename isValidHTTPSURL to isValidIssuerURL for consistency with SEC-003 terminology
- Update rate limiter comment to accurately describe initial JWKS client creation scope
- Document issuer URL constraint on query strings, fragments, and userinfo in security docs
- Correct SEC-004 docs: cooldown applies to initial load only; keyfunc manages refresh internally
- Short-circuit empty issuer in optional auth path to prevent unnecessary loader calls and log noise
* fix: retry session creation on escalation-not-found during informer sync
E2E tests create escalations via direct K8s client then immediately call
the REST API to create sessions. The API uses an informer cache to look
up escalations, which may not have synced yet, causing transient 403
'no escalation found for requested group' errors.
Add retry logic in CreateSession and CreateDebugSession to handle this
transient condition with a 2s backoff between attempts (up to 3 retries).
Only the specific 'no escalation found' 403 is retried — other 403
errors (identity mismatch, unauthorized group) are still immediate
failures.
* fix(e2e): add MailHog port-forward to Single-Cluster E2E setup step
The notification e2e tests fail with 'connection refused' on port 8025
because the MailHog port-forward started by kind-setup-single.sh dies
before the tests run. Add an explicit MailHog port-forward restart in
the 'Setup port-forwards for E2E tests' step alongside Kafka and audit
webhook receiver.
* ci: increase multi-cluster E2E timeout to 45m
The multi-cluster E2E test suite runs 280 tests including two OIDC
offline-token tests that each take ~400s. Total runtime reaches ~30m,
which is exactly the current timeout limit. The retry logic added for
informer cache flakes adds a few seconds of backoff that can push the
suite past the 30m deadline.
Increase from 30m to 45m to provide adequate headroom. The job-level
timeout-minutes remains at 60.
* fix: address review comments — port validation, AddToScheme errors, retry context, CHANGELOG
- jwksHostMatchesAuthority: validate hostname AND port (default 443 for HTTPS)
to prevent SSRF via same-host-different-port redirects
- identity_provider_loader_test.go: require.NoError for all AddToScheme calls
- e2e/helpers/api.go: add sleepOrCancel for context-aware retries, preventing
stale retries after test timeout/cancellation
- e2e/helpers/api.go: isEscalationNotFound now checks status=403
- e2e/helpers/api.go: isTemplateNotFound now requires 'template' substring
- auth_test.go: rename 'internal IP rejected' → 'different host rejected
(internal IP)', add port-validation test cases
- CHANGELOG.md: merge duplicate ### Changed sections, clarify SEC-004 scope
(initial JWKS client creation only, not keyfunc/v3 internal refreshes),
update JWKS URI validation to mention origin (hostname+port)
* fix: address gosec findings — decompression limits, HTTP context, ReadHeaderTimeout
- G110: Add io.LimitReader (500MB) for tar/zip extraction to prevent decompression bombs
- G107: Replace http.Get with http.NewRequestWithContext for all HTTP requests in bgctl update
- G112: Add ReadHeaderTimeout to OIDC callback HTTP server to prevent slowloris attacks
- G204: Annotate intentional browser-open subprocess calls
- G302: Annotate intentional 0755 permissions for executable binaries
- G117: Annotate intentional ClientSecret inclusion in internal API transport
* fix: merge duplicate CHANGELOG Fixed sections, add HTTP server timeouts
- Merge duplicate '### Fixed' sections in Unreleased CHANGELOG into one
- Add ReadTimeout, WriteTimeout, IdleTimeout to OIDC callback HTTP server
to fully mitigate slowloris-style attacks (complements ReadHeaderTimeout)
* fix: detect oversized binaries during archive extraction
Replace direct io.Copy+io.LimitReader calls with limitedCopy helper
that returns an error when the source data exceeds the size limit,
preventing silently truncated binaries from being installed.
Addresses copilot review comments on extractTarGz and extractZipEntry.
* fix review findings for auth and bgctl update
* harden bgctl update network and extraction paths
* sanitize identity provider JSON and align cluster-scoped tests
* back off repeated audience refresh failures
* fix latest copilot review findings
* validate IDP authority URLs before discovery
* harden config JSON sanitization and release tag URL handling
* use stable single-idp JWT metric label
* split metadata and download timeouts for bgctl update
1 parent 6d31d40 commit a26ee1d
File tree
21 files changed
+1430
-157
lines changed- api/v1alpha1
- applyconfiguration/api/v1alpha1
- config
- crd/bases
- samples
- docs
- e2e/helpers
- pkg
- api
- bgctl
- auth
- cmd
- config
- metrics
21 files changed
+1430
-157
lines changed| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
11 | 11 | | |
12 | 12 | | |
13 | 13 | | |
14 | | - | |
15 | | - | |
16 | | - | |
17 | | - | |
18 | | - | |
19 | | - | |
20 | | - | |
21 | | - | |
22 | | - | |
23 | | - | |
24 | | - | |
25 | | - | |
26 | | - | |
27 | | - | |
28 | | - | |
29 | | - | |
30 | | - | |
31 | 14 | | |
| 15 | + | |
32 | 16 | | |
33 | 17 | | |
34 | 18 | | |
35 | | - | |
36 | | - | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
37 | 26 | | |
38 | 27 | | |
39 | 28 | | |
| |||
43 | 32 | | |
44 | 33 | | |
45 | 34 | | |
| 35 | + | |
46 | 36 | | |
47 | 37 | | |
48 | 38 | | |
| |||
62 | 52 | | |
63 | 53 | | |
64 | 54 | | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
65 | 67 | | |
66 | 68 | | |
67 | 69 | | |
| |||
Lines changed: 16 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
76 | 76 | | |
77 | 77 | | |
78 | 78 | | |
| 79 | + | |
| 80 | + | |
| 81 | + | |
| 82 | + | |
| 83 | + | |
| 84 | + | |
| 85 | + | |
| 86 | + | |
| 87 | + | |
| 88 | + | |
| 89 | + | |
| 90 | + | |
79 | 91 | | |
80 | 92 | | |
81 | 93 | | |
| |||
Lines changed: 12 additions & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
177 | 177 | | |
178 | 178 | | |
179 | 179 | | |
| 180 | + | |
| 181 | + | |
| 182 | + | |
| 183 | + | |
| 184 | + | |
| 185 | + | |
| 186 | + | |
| 187 | + | |
| 188 | + | |
| 189 | + | |
| 190 | + | |
| 191 | + | |
180 | 192 | | |
181 | 193 | | |
182 | 194 | | |
| |||
Lines changed: 3 additions & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
15 | 15 | | |
16 | 16 | | |
17 | 17 | | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
18 | 21 | | |
19 | 22 | | |
20 | 23 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
44 | 44 | | |
45 | 45 | | |
46 | 46 | | |
| 47 | + | |
47 | 48 | | |
48 | 49 | | |
49 | 50 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
542 | 542 | | |
543 | 543 | | |
544 | 544 | | |
545 | | - | |
546 | | - | |
547 | | - | |
548 | | - | |
549 | | - | |
550 | | - | |
| 545 | + | |
| 546 | + | |
| 547 | + | |
| 548 | + | |
| 549 | + | |
| 550 | + | |
551 | 551 | | |
552 | 552 | | |
553 | 553 | | |
| |||
0 commit comments