Skip to content

Commit dab80fe

Browse files
committed
fix possible SSRF via unverified JWT claims
1 parent da3bdf0 commit dab80fe

File tree

16 files changed

+685
-23
lines changed

16 files changed

+685
-23
lines changed

internal/apiproto/api.pb.go

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

internal/apiproto/api_grpc.pb.go

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

internal/app/log.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,9 @@ func logStartWarnings(cfg config.Config, cfgMeta config.Meta) {
1919
if cfg.Admin.Insecure {
2020
log.Warn().Msg("INSECURE admin mode enabled, make sure you understand risks")
2121
}
22+
if cfg.Client.InsecureSkipTokenSignatureVerify {
23+
log.Warn().Msg("INSECURE skip token signature verify enabled, make sure you understand risks")
24+
}
2225
if cfg.Debug.Enabled {
2326
log.Warn().Msg("DEBUG mode enabled, see on " + cfg.Debug.HandlerPrefix)
2427
}

internal/cli/configdoc/schema.json

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2750,6 +2750,16 @@
27502750
"default": "",
27512751
"comment": "",
27522752
"is_complex_type": false
2753+
},
2754+
{
2755+
"field": "client.token.insecure_skip_jwks_endpoint_safety_check",
2756+
"name": "insecure_skip_jwks_endpoint_safety_check",
2757+
"go_name": "InsecureSkipJWKSEndpointSafetyCheck",
2758+
"level": 3,
2759+
"type": "bool",
2760+
"default": "",
2761+
"comment": "",
2762+
"is_complex_type": false
27532763
}
27542764
]
27552765
},
@@ -2882,6 +2892,16 @@
28822892
"default": "",
28832893
"comment": "",
28842894
"is_complex_type": false
2895+
},
2896+
{
2897+
"field": "client.subscription_token.insecure_skip_jwks_endpoint_safety_check",
2898+
"name": "insecure_skip_jwks_endpoint_safety_check",
2899+
"go_name": "InsecureSkipJWKSEndpointSafetyCheck",
2900+
"level": 3,
2901+
"type": "bool",
2902+
"default": "",
2903+
"comment": "",
2904+
"is_complex_type": false
28852905
}
28862906
]
28872907
},

internal/confighelpers/jwt.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@ func MakeVerifierConfig(tokenConf configtypes.Token) (jwtverify.VerifierConfig,
3838
cfg.AudienceRegex = tokenConf.AudienceRegex
3939
cfg.Issuer = tokenConf.Issuer
4040
cfg.IssuerRegex = tokenConf.IssuerRegex
41+
cfg.InsecureSkipJWKSEndpointSafetyCheck = tokenConf.InsecureSkipJWKSEndpointSafetyCheck
4142

4243
if tokenConf.UserIDClaim != "" {
4344
cfg.UserIDClaim = tokenConf.UserIDClaim

internal/configtypes/types.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,8 @@ type Token struct {
6161
AudienceRegex string `mapstructure:"audience_regex" json:"audience_regex" envconfig:"audience_regex" yaml:"audience_regex" toml:"audience_regex"`
6262
Issuer string `mapstructure:"issuer" json:"issuer" envconfig:"issuer" yaml:"issuer" toml:"issuer"`
6363
IssuerRegex string `mapstructure:"issuer_regex" json:"issuer_regex" envconfig:"issuer_regex" yaml:"issuer_regex" toml:"issuer_regex"`
64-
UserIDClaim string `mapstructure:"user_id_claim" json:"user_id_claim" envconfig:"user_id_claim" yaml:"user_id_claim" toml:"user_id_claim"`
64+
UserIDClaim string `mapstructure:"user_id_claim" json:"user_id_claim" envconfig:"user_id_claim" yaml:"user_id_claim" toml:"user_id_claim"`
65+
InsecureSkipJWKSEndpointSafetyCheck bool `mapstructure:"insecure_skip_jwks_endpoint_safety_check" json:"insecure_skip_jwks_endpoint_safety_check" envconfig:"insecure_skip_jwks_endpoint_safety_check" yaml:"insecure_skip_jwks_endpoint_safety_check" toml:"insecure_skip_jwks_endpoint_safety_check"`
6566
}
6667

6768
// SubscriptionToken can be used to set custom configuration for subscription tokens.

internal/jwks/manager.go

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,9 +8,11 @@ import (
88
"io"
99
"net/http"
1010
"net/url"
11+
"path"
1112
"time"
1213

1314
"github.com/rakutentech/jwk-go/jwk"
15+
"github.com/rs/zerolog/log"
1416
"github.com/valyala/fasttemplate"
1517
"golang.org/x/sync/singleflight"
1618
)
@@ -130,6 +132,20 @@ func (m *Manager) loadData(req *http.Request) ([]byte, error) {
130132

131133
func (m *Manager) fetchKey(ctx context.Context, kid string, tokenVars map[string]any) (*JWK, error) {
132134
jwkURL := m.url.ExecuteString(tokenVars)
135+
136+
// Reject requests where template substitution produced a URL with path traversal.
137+
u, err := url.Parse(jwkURL)
138+
if err != nil {
139+
return nil, fmt.Errorf("error parsing JWKS URL: %w", err)
140+
}
141+
if u.Path != "" {
142+
if cleanPath := path.Clean(u.Path); cleanPath != u.Path {
143+
log.Info().Str("path", u.Path).Str("clean_path", cleanPath).
144+
Msg("JWKS URL path contains traversal sequences, request rejected")
145+
return nil, fmt.Errorf("JWKS URL path contains traversal sequences: %q", u.Path)
146+
}
147+
}
148+
133149
req, err := http.NewRequestWithContext(ctx, http.MethodGet, jwkURL, nil)
134150
if err != nil {
135151
return nil, err

internal/jwks/manager_test.go

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -143,6 +143,45 @@ func TestManagerInitialFetchKey(t *testing.T) {
143143
}
144144
}
145145

146+
func TestManagerFetchKey_PathTraversalRejected(t *testing.T) {
147+
kid := "test-kid"
148+
149+
ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
150+
t.Fatal("request should have been rejected before reaching the server")
151+
}))
152+
defer ts.Close()
153+
154+
manager, err := NewManager(ts.URL + "/tenants/{{tenant}}/jwks.json")
155+
require.NoError(t, err)
156+
157+
// Path traversal via ".." — must be rejected.
158+
tokenVars := map[string]any{"tenant": "../../etc"}
159+
_, err = manager.FetchKey(context.Background(), kid, tokenVars)
160+
require.Error(t, err)
161+
require.Contains(t, err.Error(), "traversal")
162+
163+
// Single ".." segment — also rejected.
164+
tokenVars = map[string]any{"tenant": ".."}
165+
_, err = manager.FetchKey(context.Background(), kid, tokenVars)
166+
require.Error(t, err)
167+
require.Contains(t, err.Error(), "traversal")
168+
169+
// Clean value — must succeed (uses real server).
170+
_, pubKey, err := randomKeys()
171+
require.NoError(t, err)
172+
173+
ts2 := httptest.NewServer(jwksHandler(testKey{kid, pubKey}))
174+
defer ts2.Close()
175+
176+
manager, err = NewManager(ts2.URL + "/tenants/{{tenant}}/jwks.json")
177+
require.NoError(t, err)
178+
179+
tokenVars = map[string]any{"tenant": "acme"}
180+
key, err := manager.FetchKey(context.Background(), kid, tokenVars)
181+
require.NoError(t, err)
182+
require.Equal(t, kid, key.Kid)
183+
}
184+
146185
func TestManagerCachedFetchKey(t *testing.T) {
147186
testCases := []struct {
148187
Name string

internal/jwtverify/token_verifier_jwt.go

Lines changed: 52 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,12 @@ type VerifierConfig struct {
7171
// UserIDClaim allows overriding default claim used to extract user ID from token.
7272
// By default, Centrifugo uses "sub" and we recommend keeping the default if possible.
7373
UserIDClaim string
74+
75+
// InsecureSkipJWKSEndpointSafetyCheck disables the config-time safety validation of
76+
// JWKS endpoint URL templates. This is INSECURE and exists only as a temporary escape
77+
// hatch for users upgrading with existing permissive regex patterns. This option will
78+
// be removed in a future release.
79+
InsecureSkipJWKSEndpointSafetyCheck bool
7480
}
7581

7682
func (c VerifierConfig) Validate() error {
@@ -80,6 +86,15 @@ func (c VerifierConfig) Validate() error {
8086
if c.Issuer != "" && c.IssuerRegex != "" {
8187
return errors.New("can not use both token_issuer and token_issuer_regex, configure only one of them")
8288
}
89+
if c.JWKSPublicEndpoint != "" {
90+
if err := validateJWKSEndpointSafety(c.JWKSPublicEndpoint, c.IssuerRegex, c.AudienceRegex); err != nil {
91+
if c.InsecureSkipJWKSEndpointSafetyCheck {
92+
log.Warn().Err(err).Msg("JWKS endpoint template safety check skipped — this is INSECURE and the escape hatch will be removed in a future release, please update your regex to use an explicit list of allowed values")
93+
} else {
94+
return err
95+
}
96+
}
97+
}
8398
return nil
8499
}
85100

@@ -456,19 +471,15 @@ func (verifier *VerifierJWT) verifySignatureByJWK(token *jwt.Token, tokenVars ma
456471
return verifier.jwksManager.verify(token, tokenVars)
457472
}
458473

459-
func (verifier *VerifierJWT) validateClaims(claims jwt.RegisteredClaims, tokenVars map[string]any) error {
460-
if verifier.audience != "" && !claims.IsForAudience(verifier.audience) {
461-
return fmt.Errorf("%w: invalid audience", ErrInvalidToken)
462-
}
463-
464-
if verifier.issuer != "" && !claims.IsIssuer(verifier.issuer) {
465-
return fmt.Errorf("%w: invalid issuer", ErrInvalidToken)
466-
}
467-
474+
// extractTokenVars extracts named group matches from issuer/audience regex into tokenVars.
475+
// This must be called before signature verification because the extracted values may be
476+
// needed to construct the JWKS endpoint URL. Returns an error if regex is configured but
477+
// the claim doesn't match.
478+
func (verifier *VerifierJWT) extractTokenVars(claims jwt.RegisteredClaims, tokenVars map[string]any) error {
468479
if verifier.issuerRe != nil {
469480
match := verifier.issuerRe.FindStringSubmatch(claims.Issuer)
470481
if len(match) == 0 {
471-
return fmt.Errorf("%w: issuer not matched", ErrInvalidToken)
482+
return errors.New("issuer not matched")
472483
}
473484
for i, name := range verifier.issuerRe.SubexpNames() {
474485
if i != 0 && name != "" {
@@ -493,13 +504,27 @@ func (verifier *VerifierJWT) validateClaims(claims jwt.RegisteredClaims, tokenVa
493504
break
494505
}
495506
if !matched {
496-
return fmt.Errorf("%w: audience not matched", ErrInvalidToken)
507+
return errors.New("audience not matched")
497508
}
498509
}
499510

500511
return nil
501512
}
502513

514+
// validateClaims checks issuer and audience claims against configured values.
515+
// Must be called after signature verification.
516+
func (verifier *VerifierJWT) validateClaims(claims jwt.RegisteredClaims) error {
517+
if verifier.audience != "" && !claims.IsForAudience(verifier.audience) {
518+
return errors.New("invalid audience")
519+
}
520+
521+
if verifier.issuer != "" && !claims.IsIssuer(verifier.issuer) {
522+
return errors.New("invalid issuer")
523+
}
524+
525+
return nil
526+
}
527+
503528
func (verifier *VerifierJWT) VerifyConnectToken(t string, skipVerify bool) (ConnectToken, error) {
504529
token, err := jwt.ParseNoVerify([]byte(t)) // Will be verified later.
505530
if err != nil {
@@ -513,8 +538,9 @@ func (verifier *VerifierJWT) VerifyConnectToken(t string, skipVerify bool) (Conn
513538

514539
tokenVars := map[string]any{}
515540

516-
if err := verifier.validateClaims(claims.RegisteredClaims, tokenVars); err != nil {
517-
return ConnectToken{}, err
541+
// Extract token vars before signature verification — needed to construct JWKS URL.
542+
if err = verifier.extractTokenVars(claims.RegisteredClaims, tokenVars); err != nil {
543+
return ConnectToken{}, fmt.Errorf("%w: %v", ErrInvalidToken, err)
518544
}
519545

520546
if !skipVerify {
@@ -528,6 +554,11 @@ func (verifier *VerifierJWT) VerifyConnectToken(t string, skipVerify bool) (Conn
528554
}
529555
}
530556

557+
// Validate claims after signature verification.
558+
if err = verifier.validateClaims(claims.RegisteredClaims); err != nil {
559+
return ConnectToken{}, fmt.Errorf("%w: %v", ErrInvalidToken, err)
560+
}
561+
531562
if claims.Channel != "" {
532563
return ConnectToken{}, fmt.Errorf(
533564
"%w: connection JWT can not contain channel claim, only subscription JWT can", ErrInvalidToken)
@@ -679,8 +710,9 @@ func (verifier *VerifierJWT) VerifySubscribeToken(t string, skipVerify bool) (Su
679710

680711
tokenVars := map[string]any{}
681712

682-
if err := verifier.validateClaims(claims.RegisteredClaims, tokenVars); err != nil {
683-
return SubscribeToken{}, err
713+
// Extract token vars before signature verification — needed to construct JWKS URL.
714+
if err = verifier.extractTokenVars(claims.RegisteredClaims, tokenVars); err != nil {
715+
return SubscribeToken{}, fmt.Errorf("%w: %v", ErrInvalidToken, err)
684716
}
685717

686718
if !skipVerify {
@@ -694,6 +726,11 @@ func (verifier *VerifierJWT) VerifySubscribeToken(t string, skipVerify bool) (Su
694726
}
695727
}
696728

729+
// Validate claims after signature verification.
730+
if err = verifier.validateClaims(claims.RegisteredClaims); err != nil {
731+
return SubscribeToken{}, fmt.Errorf("%w: %v", ErrInvalidToken, err)
732+
}
733+
697734
now := time.Now()
698735
if !claims.IsValidExpiresAt(now) || !claims.IsValidNotBefore(now) {
699736
return SubscribeToken{}, ErrTokenExpired

0 commit comments

Comments
 (0)