Skip to content

Commit ee674c1

Browse files
domdomeggdomdomegg
committed
Remove OIDC authorization flow to fix data race vulnerability
1 parent 53f808a commit ee674c1

File tree

2 files changed

+7
-230
lines changed

2 files changed

+7
-230
lines changed

‎internal/api/handlers/v0/auth/oidc.go‎

Lines changed: 6 additions & 182 deletions
Original file line numberDiff line numberDiff line change
@@ -2,20 +2,16 @@ package auth
22

33
import (
44
"context"
5-
"crypto/rand"
6-
"encoding/base64"
75
"encoding/json"
86
"fmt"
97
"net/http"
108
"strings"
11-
"time"
129

1310
"github.com/coreos/go-oidc/v3/oidc"
1411
"github.com/danielgtaylor/huma/v2"
1512
v0 "github.com/modelcontextprotocol/registry/internal/api/handlers/v0"
1613
"github.com/modelcontextprotocol/registry/internal/auth"
1714
"github.com/modelcontextprotocol/registry/internal/config"
18-
"golang.org/x/oauth2"
1915
)
2016

2117
// OIDCTokenExchangeInput represents the input for OIDC token exchange
@@ -25,17 +21,6 @@ type OIDCTokenExchangeInput struct {
2521
}
2622
}
2723

28-
// OIDCStartInput represents the input for OIDC authorization start
29-
type OIDCStartInput struct {
30-
RedirectURI string `query:"redirect_uri" doc:"Optional redirect URI after authentication"`
31-
}
32-
33-
// OIDCCallbackInput represents the input for OIDC callback
34-
type OIDCCallbackInput struct {
35-
Code string `query:"code" doc:"Authorization code from OIDC provider" required:"true"`
36-
State string `query:"state" doc:"State parameter for CSRF protection" required:"true"`
37-
}
38-
3924
// OIDCClaims represents the claims we extract from any OIDC token
4025
type OIDCClaims struct {
4126
Subject string `json:"sub"`
@@ -47,19 +32,16 @@ type OIDCClaims struct {
4732
// GenericOIDCValidator defines the interface for validating OIDC tokens from any provider
4833
type GenericOIDCValidator interface {
4934
ValidateToken(ctx context.Context, token string) (*OIDCClaims, error)
50-
GetAuthorizationURL(state, nonce string, redirectURI string) string
51-
ExchangeCodeForToken(ctx context.Context, code string, redirectURI string) (string, error)
5235
}
5336

5437
// StandardOIDCValidator validates OIDC tokens using go-oidc library
5538
type StandardOIDCValidator struct {
56-
provider *oidc.Provider
57-
verifier *oidc.IDTokenVerifier
58-
oauth2Config *oauth2.Config
39+
provider *oidc.Provider
40+
verifier *oidc.IDTokenVerifier
5941
}
6042

6143
// NewStandardOIDCValidator creates a new standard OIDC validator using go-oidc
62-
func NewStandardOIDCValidator(issuer, clientID, clientSecret string) (*StandardOIDCValidator, error) {
44+
func NewStandardOIDCValidator(issuer, clientID string) (*StandardOIDCValidator, error) {
6345
ctx := context.Background()
6446

6547
// Initialize the OIDC provider
@@ -74,18 +56,9 @@ func NewStandardOIDCValidator(issuer, clientID, clientSecret string) (*StandardO
7456
}
7557
verifier := provider.Verifier(verifierConfig)
7658

77-
// Create OAuth2 config for authorization flow
78-
oauth2Config := &oauth2.Config{
79-
ClientID: clientID,
80-
ClientSecret: clientSecret,
81-
Endpoint: provider.Endpoint(),
82-
Scopes: []string{oidc.ScopeOpenID, "email", "profile"},
83-
}
84-
8559
return &StandardOIDCValidator{
86-
provider: provider,
87-
verifier: verifier,
88-
oauth2Config: oauth2Config,
60+
provider: provider,
61+
verifier: verifier,
8962
}, nil
9063
}
9164

@@ -138,52 +111,11 @@ func (v *StandardOIDCValidator) ValidateToken(ctx context.Context, tokenString s
138111
return oidcClaims, nil
139112
}
140113

141-
// GetAuthorizationURL constructs the OIDC authorization URL using oauth2
142-
func (v *StandardOIDCValidator) GetAuthorizationURL(state, nonce string, redirectURI string) string {
143-
// Update redirect URI for this request
144-
config := *v.oauth2Config
145-
config.RedirectURL = redirectURI
146-
147-
// Add nonce as additional parameter
148-
authURL := config.AuthCodeURL(state, oauth2.SetAuthURLParam("nonce", nonce))
149-
return authURL
150-
}
151-
152-
// ExchangeCodeForToken exchanges authorization code for ID token using oauth2
153-
func (v *StandardOIDCValidator) ExchangeCodeForToken(ctx context.Context, code string, redirectURI string) (string, error) {
154-
// Update redirect URI for this exchange
155-
config := *v.oauth2Config
156-
config.RedirectURL = redirectURI
157-
158-
// Exchange authorization code for token
159-
token, err := config.Exchange(ctx, code)
160-
if err != nil {
161-
return "", fmt.Errorf("failed to exchange code for token: %w", err)
162-
}
163-
164-
// Extract ID token from OAuth2 token
165-
rawIDToken, ok := token.Extra("id_token").(string)
166-
if !ok {
167-
return "", fmt.Errorf("no ID token found in OAuth2 response")
168-
}
169-
170-
return rawIDToken, nil
171-
}
172-
173114
// OIDCHandler handles configurable OIDC authentication
174115
type OIDCHandler struct {
175116
config *config.Config
176117
jwtManager *auth.JWTManager
177118
validator GenericOIDCValidator
178-
sessions map[string]OIDCSession // In-memory state storage for now
179-
}
180-
181-
// OIDCSession stores OIDC flow state
182-
type OIDCSession struct {
183-
State string
184-
Nonce string
185-
RedirectURI string
186-
CreatedAt time.Time
187119
}
188120

189121
// NewOIDCHandler creates a new OIDC handler
@@ -195,7 +127,7 @@ func NewOIDCHandler(cfg *config.Config) *OIDCHandler {
195127
panic("OIDC issuer is required when OIDC is enabled")
196128
}
197129

198-
validator, err := NewStandardOIDCValidator(cfg.OIDCIssuer, cfg.OIDCClientID, cfg.OIDCClientSecret)
130+
validator, err := NewStandardOIDCValidator(cfg.OIDCIssuer, cfg.OIDCClientID)
199131
if err != nil {
200132
panic(fmt.Sprintf("Failed to initialize OIDC validator: %v", err))
201133
}
@@ -204,7 +136,6 @@ func NewOIDCHandler(cfg *config.Config) *OIDCHandler {
204136
config: cfg,
205137
jwtManager: auth.NewJWTManager(cfg),
206138
validator: validator,
207-
sessions: make(map[string]OIDCSession),
208139
}
209140
}
210141

@@ -239,47 +170,6 @@ func RegisterOIDCEndpoints(api huma.API, cfg *config.Config) {
239170
Body: *response,
240171
}, nil
241172
})
242-
243-
// Authorization start endpoint
244-
huma.Register(api, huma.Operation{
245-
OperationID: "oidc-auth-start",
246-
Method: http.MethodGet,
247-
Path: "/v0/auth/oidc/start",
248-
Summary: "Start OIDC authorization flow",
249-
Description: "Redirects user to OIDC provider for authentication",
250-
Tags: []string{"auth"},
251-
}, func(ctx context.Context, input *OIDCStartInput) (*v0.Response[map[string]string], error) {
252-
authURL, err := handler.StartAuth(ctx, input.RedirectURI)
253-
if err != nil {
254-
return nil, huma.Error400BadRequest("Failed to start OIDC flow", err)
255-
}
256-
257-
return &v0.Response[map[string]string]{
258-
Body: map[string]string{
259-
"authorization_url": authURL,
260-
"message": "Visit the authorization URL to complete authentication",
261-
},
262-
}, nil
263-
})
264-
265-
// Authorization callback endpoint
266-
huma.Register(api, huma.Operation{
267-
OperationID: "oidc-auth-callback",
268-
Method: http.MethodGet,
269-
Path: "/v0/auth/oidc/callback",
270-
Summary: "Handle OIDC authorization callback",
271-
Description: "Handles the callback from OIDC provider after user authorization",
272-
Tags: []string{"auth"},
273-
}, func(ctx context.Context, input *OIDCCallbackInput) (*v0.Response[auth.TokenResponse], error) {
274-
response, err := handler.HandleCallback(ctx, input.Code, input.State)
275-
if err != nil {
276-
return nil, huma.Error400BadRequest("Failed to handle OIDC callback", err)
277-
}
278-
279-
return &v0.Response[auth.TokenResponse]{
280-
Body: *response,
281-
}, nil
282-
})
283173
}
284174

285175
// ExchangeToken exchanges an OIDC ID token for a Registry JWT token
@@ -314,63 +204,6 @@ func (h *OIDCHandler) ExchangeToken(ctx context.Context, oidcToken string) (*aut
314204
return tokenResponse, nil
315205
}
316206

317-
// StartAuth initiates the OIDC authorization flow
318-
func (h *OIDCHandler) StartAuth(_ context.Context, redirectURI string) (string, error) {
319-
// Generate state and nonce for security
320-
state, err := generateRandomString(32)
321-
if err != nil {
322-
return "", fmt.Errorf("failed to generate state: %w", err)
323-
}
324-
325-
nonce, err := generateRandomString(32)
326-
if err != nil {
327-
return "", fmt.Errorf("failed to generate nonce: %w", err)
328-
}
329-
330-
// Store session for callback validation
331-
session := OIDCSession{
332-
State: state,
333-
Nonce: nonce,
334-
RedirectURI: redirectURI,
335-
CreatedAt: time.Now(),
336-
}
337-
h.sessions[state] = session
338-
339-
// Build callback URI - use configured base URL or default
340-
callbackURI := "/v0/auth/oidc/callback"
341-
342-
// Get authorization URL
343-
authURL := h.validator.GetAuthorizationURL(state, nonce, callbackURI)
344-
345-
return authURL, nil
346-
}
347-
348-
// HandleCallback handles the OIDC callback
349-
func (h *OIDCHandler) HandleCallback(ctx context.Context, code, state string) (*auth.TokenResponse, error) {
350-
// Validate state and retrieve session
351-
session, exists := h.sessions[state]
352-
if !exists {
353-
return nil, fmt.Errorf("invalid state parameter")
354-
}
355-
356-
// Clean up session
357-
delete(h.sessions, state)
358-
359-
// Check session expiry (5 minutes)
360-
if time.Since(session.CreatedAt) > 5*time.Minute {
361-
return nil, fmt.Errorf("authentication session expired")
362-
}
363-
364-
// Exchange authorization code for tokens
365-
idToken, err := h.validator.ExchangeCodeForToken(ctx, code, "/v0/auth/oidc/callback")
366-
if err != nil {
367-
return nil, fmt.Errorf("failed to exchange code for token: %w", err)
368-
}
369-
370-
// Now validate the ID token and generate registry token
371-
return h.ExchangeToken(ctx, idToken)
372-
}
373-
374207
// validateExtraClaims validates additional claims based on configuration
375208
func (h *OIDCHandler) validateExtraClaims(claims *OIDCClaims) error {
376209
if h.config.OIDCExtraClaims == "" {
@@ -431,12 +264,3 @@ func (h *OIDCHandler) buildPermissions(_ *OIDCClaims) []auth.Permission {
431264

432265
return permissions
433266
}
434-
435-
// generateRandomString generates a cryptographically secure random string
436-
func generateRandomString(length int) (string, error) {
437-
bytes := make([]byte, length)
438-
if _, err := rand.Read(bytes); err != nil {
439-
return "", err
440-
}
441-
return base64.URLEncoding.EncodeToString(bytes)[:length], nil
442-
}

‎internal/api/handlers/v0/auth/oidc_test.go‎

Lines changed: 1 addition & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -13,9 +13,7 @@ import (
1313

1414
// MockGenericOIDCValidator for testing
1515
type MockGenericOIDCValidator struct {
16-
validateFunc func(ctx context.Context, token string) (*auth.OIDCClaims, error)
17-
authURLFunc func(state, nonce, redirectURI string) string
18-
exchangeCodeFunc func(ctx context.Context, code, redirectURI string) (string, error)
16+
validateFunc func(ctx context.Context, token string) (*auth.OIDCClaims, error)
1917
}
2018

2119
func (m *MockGenericOIDCValidator) ValidateToken(ctx context.Context, token string) (*auth.OIDCClaims, error) {
@@ -25,20 +23,6 @@ func (m *MockGenericOIDCValidator) ValidateToken(ctx context.Context, token stri
2523
return nil, fmt.Errorf("not implemented")
2624
}
2725

28-
func (m *MockGenericOIDCValidator) GetAuthorizationURL(state, nonce, redirectURI string) string {
29-
if m.authURLFunc != nil {
30-
return m.authURLFunc(state, nonce, redirectURI)
31-
}
32-
return ""
33-
}
34-
35-
func (m *MockGenericOIDCValidator) ExchangeCodeForToken(ctx context.Context, code, redirectURI string) (string, error) {
36-
if m.exchangeCodeFunc != nil {
37-
return m.exchangeCodeFunc(ctx, code, redirectURI)
38-
}
39-
return "", fmt.Errorf("not implemented")
40-
}
41-
4226
func TestOIDCHandler_ExchangeToken(t *testing.T) {
4327
tests := []struct {
4428
name string
@@ -121,34 +105,3 @@ func TestOIDCHandler_ExchangeToken(t *testing.T) {
121105
})
122106
}
123107
}
124-
125-
func TestOIDCHandler_StartAuth(t *testing.T) {
126-
config := &config.Config{
127-
OIDCEnabled: true,
128-
OIDCIssuer: "https://accounts.google.com",
129-
OIDCClientID: "test-client-id",
130-
OIDCClientSecret: "test-secret",
131-
JWTPrivateKey: "deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef",
132-
}
133-
134-
mockValidator := &MockGenericOIDCValidator{
135-
authURLFunc: func(state, nonce, redirectURI string) string {
136-
return fmt.Sprintf("https://accounts.google.com/oauth/authorize?client_id=%s&state=%s&nonce=%s&redirect_uri=%s",
137-
config.OIDCClientID, state, nonce, redirectURI)
138-
},
139-
}
140-
141-
handler := auth.NewOIDCHandler(config)
142-
handler.SetValidator(mockValidator)
143-
144-
ctx := context.Background()
145-
authURL, err := handler.StartAuth(ctx, "http://localhost:3000/callback")
146-
147-
require.NoError(t, err)
148-
assert.Contains(t, authURL, "https://accounts.google.com/oauth/authorize")
149-
assert.Contains(t, authURL, "client_id=test-client-id")
150-
assert.Contains(t, authURL, "state=")
151-
assert.Contains(t, authURL, "nonce=")
152-
}
153-
154-
// Note: validateExtraClaims and buildPermissions are tested through ExchangeToken integration tests

0 commit comments

Comments
 (0)