From 5210f91eefb6bb2dd4f55b9ba50790335012280d Mon Sep 17 00:00:00 2001 From: dushixiang Date: Sun, 23 Aug 2026 21:33:28 +0800 Subject: [PATCH] fix(auth): make OIDC discovery resilient --- internal/config/config.go | 32 ++++ internal/config/config_test.go | 62 ++++++++ internal/handler/account_handler.go | 2 +- internal/service/account_service.go | 4 +- internal/service/oidc_service.go | 201 ++++++++++++++++++-------- internal/service/oidc_service_test.go | 197 +++++++++++++++++++++++++ 6 files changed, 438 insertions(+), 60 deletions(-) create mode 100644 internal/config/config_test.go create mode 100644 internal/service/oidc_service_test.go diff --git a/internal/config/config.go b/internal/config/config.go index 4601a51..8a32b5f 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -1,5 +1,10 @@ package config +import ( + "fmt" + "strings" +) + // AppConfig 应用配置 type AppConfig struct { JWT JWTConfig `json:"JWT"` @@ -31,6 +36,33 @@ type OIDCConfig struct { RedirectURL string `json:"RedirectURL"` // 回调URL } +// Validate 校验启用 OIDC 所需的静态配置。 +// Provider 是否可访问属于运行时状态，不应在应用启动阶段参与配置有效性判断。 +func (c *OIDCConfig) Validate() error { + if c == nil || !c.Enabled { + return nil + } + + missing := make([]string, 0, 4) + if strings.TrimSpace(c.Issuer) == "" { + missing = append(missing, "Issuer") + } + if strings.TrimSpace(c.ClientID) == "" { + missing = append(missing, "ClientID") + } + if strings.TrimSpace(c.ClientSecret) == "" { + missing = append(missing, "ClientSecret") + } + if strings.TrimSpace(c.RedirectURL) == "" { + missing = append(missing, "RedirectURL") + } + if len(missing) > 0 { + return fmt.Errorf("缺少必要配置: %s", strings.Join(missing, ", ")) + } + + return nil +} + // GitHubOAuthConfig GitHub OAuth认证配置 type GitHubOAuthConfig struct { Enabled bool `json:"Enabled"` // 是否启用GitHub登录 diff --git a/internal/config/config_test.go b/internal/config/config_test.go new file mode 100644 index 0000000..8237445 --- /dev/null +++ b/internal/config/config_test.go @@ -0,0 +1,62 @@ +package config + +import ( + "strings" + "testing" +) + +func TestOIDCConfigValidate(t *testing.T) { + tests := []struct { + name string + config *OIDCConfig + wantMissing []string + }{ + { + name: "nil config", + config: nil, + }, + { + name: "disabled config", + config: &OIDCConfig{}, + }, + { + name: "complete config", + config: &OIDCConfig{ + Enabled: true, + Issuer: "https://accounts.example.com", + ClientID: "client-id", + ClientSecret: "client-secret", + RedirectURL: "https://pika.example.com/admin/oidc/callback", + }, + }, + { + name: "reports every missing field", + config: &OIDCConfig{ + Enabled: true, + Issuer: " ", + }, + wantMissing: []string{"Issuer", "ClientID", "ClientSecret", "RedirectURL"}, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + err := test.config.Validate() + if len(test.wantMissing) == 0 { + if err != nil { + t.Fatalf("Validate() returned unexpected error: %v", err) + } + return + } + + if err == nil { + t.Fatal("Validate() returned nil, want missing field error") + } + for _, field := range test.wantMissing { + if !strings.Contains(err.Error(), field) { + t.Errorf("Validate() error %q does not mention %s", err, field) + } + } + }) + } +} diff --git a/internal/handler/account_handler.go b/internal/handler/account_handler.go index 67fac8c..09735ac 100644 --- a/internal/handler/account_handler.go +++ b/internal/handler/account_handler.go @@ -76,7 +76,7 @@ func (r AccountHandler) GetAuthConfig(c *echo.Context) error { // GetOIDCAuthURL 获取 OIDC 认证 URL func (r AccountHandler) GetOIDCAuthURL(c *echo.Context) error { - authURL, err := r.accountService.GetOIDCAuthURL() + authURL, err := r.accountService.GetOIDCAuthURL(c.Request().Context()) if err != nil { return echo.NewHTTPError(http.StatusBadRequest, err.Error()) } diff --git a/internal/service/account_service.go b/internal/service/account_service.go index aa2a8b6..b5e09eb 100644 --- a/internal/service/account_service.go +++ b/internal/service/account_service.go @@ -188,12 +188,12 @@ type OIDCAuthURL struct { } // GetOIDCAuthURL 获取 OIDC 认证 URL -func (s *AccountService) GetOIDCAuthURL() (*OIDCAuthURL, error) { +func (s *AccountService) GetOIDCAuthURL(ctx context.Context) (*OIDCAuthURL, error) { if !s.oidcService.IsEnabled() { return nil, errors.New("OIDC 未启用") } - authURL, state, err := s.oidcService.GenerateAuthURL() + authURL, state, err := s.oidcService.GenerateAuthURL(ctx) if err != nil { return nil, err } diff --git a/internal/service/oidc_service.go b/internal/service/oidc_service.go index c722ef9..4ee7abf 100644 --- a/internal/service/oidc_service.go +++ b/internal/service/oidc_service.go @@ -6,6 +6,7 @@ import ( "encoding/base64" "errors" "fmt" + "sync" "time" "github.com/coreos/go-oidc/v3/oidc" @@ -14,13 +15,27 @@ import ( "golang.org/x/oauth2" ) +const ( + oidcProviderInitTimeout = 10 * time.Second + oidcProviderRetryDelay = 5 * time.Second +) + +type oidcProviderInitAttempt struct { + done chan struct{} + err error +} + // OIDCService OIDC 认证服务 type OIDCService struct { logger *zap.Logger config *config.OIDCConfig - provider *oidc.Provider + providerMu sync.Mutex oauth2Config oauth2.Config verifier *oidc.IDTokenVerifier + initAttempt *oidcProviderInitAttempt + lastInitErr error + retryAfter time.Time + stateMu sync.Mutex stateStore map[string]time.Time // 简单的 state 存储（生产环境应使用 Redis 等） } @@ -29,55 +44,27 @@ func NewOIDCService(logger *zap.Logger, appConfig *config.AppConfig) *OIDCServic if appConfig.OIDC == nil || !appConfig.OIDC.Enabled { logger.Info("OIDC 认证未启用") return &OIDCService{ - logger: logger, - config: nil, + logger: logger, + stateStore: make(map[string]time.Time), } } - oidcConfig := appConfig.OIDC - - // 验证配置 - if oidcConfig.Issuer == "" || oidcConfig.ClientID == "" || oidcConfig.ClientSecret == "" { - logger.Error("OIDC 配置不完整，OIDC 认证将被禁用") + if err := appConfig.OIDC.Validate(); err != nil { + logger.Error("OIDC 配置不完整，OIDC 认证将被禁用", zap.Error(err)) return &OIDCService{ - logger: logger, - config: nil, + logger: logger, + stateStore: make(map[string]time.Time), } } - ctx := context.Background() - - // 初始化 OIDC Provider - provider, err := oidc.NewProvider(ctx, oidcConfig.Issuer) - if err != nil { - logger.Error("初始化 OIDC Provider 失败，OIDC 认证将被禁用", zap.Error(err)) - return &OIDCService{ - logger: logger, - config: nil, - } - } - - // 配置 OAuth2 - oauth2Config := oauth2.Config{ - ClientID: oidcConfig.ClientID, - ClientSecret: oidcConfig.ClientSecret, - RedirectURL: oidcConfig.RedirectURL, - Endpoint: provider.Endpoint(), - Scopes: []string{oidc.ScopeOpenID, "profile", "email"}, - } - - // 创建 ID Token 验证器 - verifier := provider.Verifier(&oidc.Config{ClientID: oidcConfig.ClientID}) - - logger.Info("OIDC 服务初始化成功", zap.String("issuer", oidcConfig.Issuer)) + // 配置在应用生命周期内只读，复制一份以避免调用方意外修改。 + oidcConfig := *appConfig.OIDC + logger.Info("OIDC 配置已加载，Provider 将在首次登录时初始化", zap.String("issuer", oidcConfig.Issuer)) return &OIDCService{ - logger: logger, - config: oidcConfig, - provider: provider, - oauth2Config: oauth2Config, - verifier: verifier, - stateStore: make(map[string]time.Time), + logger: logger, + config: &oidcConfig, + stateStore: make(map[string]time.Time), } } @@ -87,11 +74,16 @@ func (s *OIDCService) IsEnabled() bool { } // GenerateAuthURL 生成认证 URL -func (s *OIDCService) GenerateAuthURL() (string, string, error) { +func (s *OIDCService) GenerateAuthURL(ctx context.Context) (string, string, error) { if !s.IsEnabled() { return "", "", errors.New("OIDC 未启用") } + oauth2Config, _, err := s.runtimeConfig(ctx) + if err != nil { + return "", "", err + } + // 生成随机 state state, err := s.generateState() if err != nil { @@ -99,12 +91,12 @@ func (s *OIDCService) GenerateAuthURL() (string, string, error) { } // 存储 state（有效期 10 分钟） + s.stateMu.Lock() s.stateStore[state] = time.Now().Add(10 * time.Minute) + s.cleanExpiredStatesLocked() + s.stateMu.Unlock() - // 清理过期的 state - s.cleanExpiredStates() - - authURL := s.oauth2Config.AuthCodeURL(state) + authURL := oauth2Config.AuthCodeURL(state) return authURL, state, nil } @@ -114,16 +106,18 @@ func (s *OIDCService) ExchangeCode(ctx context.Context, code, state string) (str return "", "", errors.New("OIDC 未启用") } - // 验证 state - if !s.validateState(state) { + oauth2Config, verifier, err := s.runtimeConfig(ctx) + if err != nil { + return "", "", err + } + + // state 仅允许消费一次，校验和删除必须是原子操作。 + if !s.consumeState(state) { return "", "", errors.New("无效的 state") } - // 删除已使用的 state - delete(s.stateStore, state) - // 交换授权码 - oauth2Token, err := s.oauth2Config.Exchange(ctx, code) + oauth2Token, err := oauth2Config.Exchange(ctx, code) if err != nil { return "", "", fmt.Errorf("交换授权码失败: %w", err) } @@ -135,7 +129,7 @@ func (s *OIDCService) ExchangeCode(ctx context.Context, code, state string) (str } // 验证 ID Token - idToken, err := s.verifier.Verify(ctx, rawIDToken) + idToken, err := verifier.Verify(ctx, rawIDToken) if err != nil { return "", "", fmt.Errorf("验证 ID Token 失败: %w", err) } @@ -174,6 +168,95 @@ func (s *OIDCService) ExchangeCode(ctx context.Context, code, state string) (str return username, nickname, nil } +// runtimeConfig 返回可用于当前请求的 OIDC 运行时配置。 +// 并发请求共享同一次 Discovery，等待方可以随请求上下文取消；失败后经过短暂退避再重试。 +func (s *OIDCService) runtimeConfig(ctx context.Context) (oauth2.Config, *oidc.IDTokenVerifier, error) { + for { + if err := ctx.Err(); err != nil { + return oauth2.Config{}, nil, err + } + + s.providerMu.Lock() + if s.verifier != nil { + oauth2Config, verifier := s.oauth2Config, s.verifier + s.providerMu.Unlock() + return oauth2Config, verifier, nil + } + + if s.initAttempt == nil && s.lastInitErr != nil && time.Now().Before(s.retryAfter) { + err := s.lastInitErr + s.providerMu.Unlock() + return oauth2.Config{}, nil, err + } + + attempt := s.initAttempt + if attempt == nil { + attempt = &oidcProviderInitAttempt{done: make(chan struct{})} + s.initAttempt = attempt + go s.initializeProvider(attempt) + } + s.providerMu.Unlock() + + select { + case <-attempt.done: + if attempt.err != nil { + // 即使等待方被延迟调度到退避期之后，也共享它实际等待的这次失败结果。 + return oauth2.Config{}, nil, attempt.err + } + // 初始化结果已发布，重新读取成功的运行时配置。 + case <-ctx.Done(): + return oauth2.Config{}, nil, ctx.Err() + } + } +} + +// initializeProvider 在独立、受限时长的上下文中执行 Discovery，避免首个调用方取消时影响其他等待者。 +func (s *OIDCService) initializeProvider(attempt *oidcProviderInitAttempt) { + discoveryCtx, cancel := context.WithTimeout(context.Background(), oidcProviderInitTimeout) + defer cancel() + + provider, err := oidc.NewProvider(discoveryCtx, s.config.Issuer) + if err != nil { + err = fmt.Errorf("初始化 OIDC Provider 失败: %w", err) + } + + var oauth2Config oauth2.Config + var verifier *oidc.IDTokenVerifier + if err == nil { + oauth2Config = oauth2.Config{ + ClientID: s.config.ClientID, + ClientSecret: s.config.ClientSecret, + RedirectURL: s.config.RedirectURL, + Endpoint: provider.Endpoint(), + Scopes: []string{oidc.ScopeOpenID, "profile", "email"}, + } + verifier = provider.Verifier(&oidc.Config{ClientID: s.config.ClientID}) + } + + s.providerMu.Lock() + if err != nil { + s.lastInitErr = err + s.retryAfter = time.Now().Add(oidcProviderRetryDelay) + } else { + s.oauth2Config = oauth2Config + s.verifier = verifier + s.lastInitErr = nil + s.retryAfter = time.Time{} + } + attempt.err = err + s.initAttempt = nil + close(attempt.done) + s.providerMu.Unlock() + + if err != nil { + s.logger.Warn("初始化 OIDC Provider 失败，退避后允许重试", + zap.Duration("retryAfter", oidcProviderRetryDelay), + zap.Error(err)) + return + } + s.logger.Info("OIDC Provider 初始化成功", zap.String("issuer", s.config.Issuer)) +} + // generateState 生成随机 state func (s *OIDCService) generateState() (string, error) { b := make([]byte, 32) @@ -183,17 +266,21 @@ func (s *OIDCService) generateState() (string, error) { return base64.URLEncoding.EncodeToString(b), nil } -// validateState 验证 state -func (s *OIDCService) validateState(state string) bool { +// consumeState 验证并消费 state。 +func (s *OIDCService) consumeState(state string) bool { + s.stateMu.Lock() + defer s.stateMu.Unlock() + expiresAt, exists := s.stateStore[state] if !exists { return false } + delete(s.stateStore, state) return time.Now().Before(expiresAt) } -// cleanExpiredStates 清理过期的 state -func (s *OIDCService) cleanExpiredStates() { +// cleanExpiredStatesLocked 清理过期的 state，调用方必须持有 stateMu。 +func (s *OIDCService) cleanExpiredStatesLocked() { now := time.Now() for state, expiresAt := range s.stateStore { if now.After(expiresAt) { diff --git a/internal/service/oidc_service_test.go b/internal/service/oidc_service_test.go new file mode 100644 index 0000000..63bf313 --- /dev/null +++ b/internal/service/oidc_service_test.go @@ -0,0 +1,197 @@ +package service + +import ( + "context" + "encoding/json" + "errors" + "net/http" + "net/http/httptest" + "net/url" + "sync" + "sync/atomic" + "testing" + "time" + + "github.com/pika-monitor/pika/internal/config" + "go.uber.org/zap" +) + +func TestOIDCProviderDiscoveryRetriesAfterFailure(t *testing.T) { + var attempts atomic.Int32 + var provider *httptest.Server + provider = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/.well-known/openid-configuration" { + http.NotFound(w, r) + return + } + if attempts.Add(1) == 1 { + http.Error(w, "provider is starting", http.StatusServiceUnavailable) + return + } + + w.Header().Set("Content-Type", "application/json") + if err := json.NewEncoder(w).Encode(map[string]any{ + "issuer": provider.URL, + "authorization_endpoint": provider.URL + "/authorize", + "token_endpoint": provider.URL + "/token", + "jwks_uri": provider.URL + "/keys", + }); err != nil { + t.Errorf("encode discovery response: %v", err) + } + })) + t.Cleanup(provider.Close) + + service := NewOIDCService(zap.NewNop(), &config.AppConfig{ + OIDC: &config.OIDCConfig{ + Enabled: true, + Issuer: provider.URL, + ClientID: "client-id", + ClientSecret: "client-secret", + RedirectURL: "https://pika.example.com/admin/oidc/callback", + }, + }) + + if !service.IsEnabled() { + t.Fatal("OIDC should be shown as enabled before Provider Discovery") + } + if got := attempts.Load(); got != 0 { + t.Fatalf("NewOIDCService performed %d discovery requests, want 0", got) + } + + if _, _, err := service.GenerateAuthURL(context.Background()); err == nil { + t.Fatal("first GenerateAuthURL() succeeded while provider returned an error") + } + if !service.IsEnabled() { + t.Fatal("transient Provider Discovery failure permanently disabled OIDC") + } + + // 退避期内直接复用最近一次错误，不应再次请求 Provider。 + if _, _, err := service.GenerateAuthURL(context.Background()); err == nil { + t.Fatal("GenerateAuthURL() succeeded during discovery failure backoff") + } + if got, want := attempts.Load(), int32(1); got != want { + t.Fatalf("discovery attempts during backoff = %d, want %d", got, want) + } + + // 无需真实等待退避时间；过期后下一次请求应重新执行 Discovery。 + service.providerMu.Lock() + service.retryAfter = time.Now().Add(-time.Second) + service.providerMu.Unlock() + + authURL, state, err := service.GenerateAuthURL(context.Background()) + if err != nil { + t.Fatalf("GenerateAuthURL() did not recover on retry: %v", err) + } + parsedURL, err := url.Parse(authURL) + if err != nil { + t.Fatalf("parse generated auth URL: %v", err) + } + if got, want := parsedURL.Path, "/authorize"; got != want { + t.Errorf("auth URL path = %q, want %q", got, want) + } + if got := parsedURL.Query().Get("state"); got != state { + t.Errorf("auth URL state = %q, want %q", got, state) + } + if got, want := attempts.Load(), int32(2); got != want { + t.Fatalf("discovery attempts = %d, want %d", got, want) + } + + if _, _, err := service.GenerateAuthURL(context.Background()); err != nil { + t.Fatalf("GenerateAuthURL() with cached Provider failed: %v", err) + } + if got, want := attempts.Load(), int32(2); got != want { + t.Errorf("cached Provider caused another discovery request: got %d attempts, want %d", got, want) + } +} + +func TestOIDCProviderDiscoverySharesFailureAndAllowsCanceledWaiters(t *testing.T) { + var attempts atomic.Int32 + discoveryStarted := make(chan struct{}) + releaseDiscovery := make(chan struct{}) + var releaseOnce sync.Once + + provider := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + attempts.Add(1) + select { + case <-discoveryStarted: + default: + close(discoveryStarted) + } + <-releaseDiscovery + http.Error(w, "provider unavailable", http.StatusServiceUnavailable) + })) + t.Cleanup(provider.Close) + // 后注册以保证清理时先解除 Handler 阻塞，再关闭测试服务器。 + t.Cleanup(func() { releaseOnce.Do(func() { close(releaseDiscovery) }) }) + + service := NewOIDCService(zap.NewNop(), &config.AppConfig{ + OIDC: &config.OIDCConfig{ + Enabled: true, + Issuer: provider.URL, + ClientID: "client-id", + ClientSecret: "client-secret", + RedirectURL: "https://pika.example.com/admin/oidc/callback", + }, + }) + + const callers = 20 + results := make(chan error, callers) + for range callers { + go func() { + _, _, err := service.GenerateAuthURL(context.Background()) + results <- err + }() + } + + select { + case <-discoveryStarted: + case <-time.After(2 * time.Second): + t.Fatal("Provider Discovery did not start") + } + + // 等待中的调用方必须能随请求上下文取消，不能被互斥锁留住。 + canceledResult := make(chan error, 1) + waiterCtx, cancelWaiter := context.WithCancel(context.Background()) + go func() { + _, _, err := service.GenerateAuthURL(waiterCtx) + canceledResult <- err + }() + cancelWaiter() + select { + case err := <-canceledResult: + if !errors.Is(err, context.Canceled) { + t.Fatalf("canceled waiter error = %v, want context.Canceled", err) + } + case <-time.After(time.Second): + t.Fatal("canceled waiter remained blocked by Provider Discovery") + } + + releaseOnce.Do(func() { close(releaseDiscovery) }) + for range callers { + select { + case err := <-results: + if err == nil { + t.Error("GenerateAuthURL() succeeded while Provider Discovery failed") + } + case <-time.After(2 * time.Second): + t.Fatal("concurrent caller did not receive shared discovery result") + } + } + + if got, want := attempts.Load(), int32(1); got != want { + t.Fatalf("concurrent discovery attempts = %d, want %d", got, want) + } +} + +func TestOIDCServiceRejectsIncompleteConfig(t *testing.T) { + service := NewOIDCService(zap.NewNop(), &config.AppConfig{ + OIDC: &config.OIDCConfig{ + Enabled: true, + Issuer: "https://accounts.example.com", + }, + }) + + if service.IsEnabled() { + t.Fatal("OIDC with incomplete static config should not be enabled") + } +}