mirror of
https://github.com/mattermost/mattermost.git
synced 2026-09-19 02:06:37 +08:00
[MM-68577] Add OAuth2/OpenID Connect provider status to support packet (#36451)
* [MM-68577] Add OAuth2/OpenID Connect provider status to support packet Probe configured GitLab, Google, Office365, and OpenID providers and report their connectivity status in the support packet diagnostics. For providers with a DiscoveryEndpoint, the probe verifies a valid OIDC discovery document (JSON with an "issuer" field) is returned; otherwise it probes the TokenEndpoint host, treating any HTTP response as reachable since token endpoints reject GETs. Disabled providers report status: disabled, enabled providers report ok or fail with the underlying error. No secrets are read or transmitted; only public endpoint URLs are probed. * [MM-68577] Drain response body in probeOAuthTokenEndpoint Closing resp.Body without first reading it leaves unread bytes on the wire, which prevents net/http from returning the underlying TCP connection to the idle pool for keep-alive reuse. Drain with io.Copy + io.LimitReader (1MB cap to bound a misbehaving server) and use defer for the close. * [MM-68577] Extract drainAndCloseBody helper for HTTP probe responses Three call sites in this file (probeOIDCDiscovery, probeOAuthTokenEndpoint, testPushProxyConnection) now share the same drain-then-close idiom needed to keep TCP connections eligible for keep-alive reuse. Replace the inline copies with a single drainAndCloseBody helper that bounds the discard at 1 MiB to limit exposure to a misbehaving server. Also fixes the same un-drained Close() bug in the pre-existing testPushProxyConnection while we're here. * Add comment explaining 1 MiB discovery response size cap Addresses review feedback asking for clarity on the 1<<20 limit.
This commit is contained in:
@@ -8,6 +8,7 @@ import (
|
||||
"context"
|
||||
"encoding/json"
|
||||
"fmt"
|
||||
"io"
|
||||
"net/http"
|
||||
"net/url"
|
||||
"os"
|
||||
@@ -296,6 +297,12 @@ func (ps *PlatformService) getSupportPacketDiagnostics(rctx request.CTX) (*model
|
||||
d.Notifications.Email.Status = model.StatusDisabled
|
||||
}
|
||||
|
||||
/* OAuth2 / OpenID Connect Providers */
|
||||
d.OAuthProviders.GitLab = probeOAuthProvider(rctx.Context(), &ps.Config().GitLabSettings)
|
||||
d.OAuthProviders.Google = probeOAuthProvider(rctx.Context(), &ps.Config().GoogleSettings)
|
||||
d.OAuthProviders.Office365 = probeOAuthProvider(rctx.Context(), ps.Config().Office365Settings.SSOSettings())
|
||||
d.OAuthProviders.OpenID = probeOAuthProvider(rctx.Context(), &ps.Config().OpenIdSettings)
|
||||
|
||||
/* Push Notifications */
|
||||
if model.SafeDereference(ps.Config().EmailSettings.SendPushNotifications) {
|
||||
pushServerURL := model.SafeDereference(ps.Config().EmailSettings.PushNotificationServer)
|
||||
@@ -321,6 +328,88 @@ func (ps *PlatformService) getSupportPacketDiagnostics(rctx request.CTX) (*model
|
||||
return fileData, rErr.ErrorOrNil()
|
||||
}
|
||||
|
||||
// probeOAuthProvider checks connectivity for an OAuth2/OpenID Connect provider.
|
||||
// If the provider has a DiscoveryEndpoint configured, it issues an HTTP GET to
|
||||
// that URL and verifies the response is a valid OIDC discovery document.
|
||||
// Otherwise it probes the TokenEndpoint host: any HTTP response (including
|
||||
// 4xx/5xx) is treated as reachable, since token endpoints typically reject GETs.
|
||||
func probeOAuthProvider(ctx context.Context, sso *model.SSOSettings) model.OAuthProviderStatus {
|
||||
if !model.SafeDereference(sso.Enable) {
|
||||
return model.OAuthProviderStatus{Status: model.StatusDisabled}
|
||||
}
|
||||
|
||||
ctx, cancel := context.WithTimeout(ctx, 10*time.Second)
|
||||
defer cancel()
|
||||
|
||||
if discoveryEndpoint := model.SafeDereference(sso.DiscoveryEndpoint); discoveryEndpoint != "" {
|
||||
if err := probeOIDCDiscovery(ctx, discoveryEndpoint); err != nil {
|
||||
return model.OAuthProviderStatus{Status: model.StatusFail, Error: err.Error()}
|
||||
}
|
||||
return model.OAuthProviderStatus{Status: model.StatusOk}
|
||||
}
|
||||
|
||||
if tokenEndpoint := model.SafeDereference(sso.TokenEndpoint); tokenEndpoint != "" {
|
||||
if err := probeOAuthTokenEndpoint(ctx, tokenEndpoint); err != nil {
|
||||
return model.OAuthProviderStatus{Status: model.StatusFail, Error: err.Error()}
|
||||
}
|
||||
return model.OAuthProviderStatus{Status: model.StatusOk}
|
||||
}
|
||||
|
||||
return model.OAuthProviderStatus{Status: model.StatusFail, Error: "no discovery or token endpoint configured"}
|
||||
}
|
||||
|
||||
func probeOIDCDiscovery(ctx context.Context, discoveryURL string) error {
|
||||
req, err := http.NewRequestWithContext(ctx, http.MethodGet, discoveryURL, nil)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
resp, err := http.DefaultClient.Do(req)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
defer drainAndCloseBody(resp.Body)
|
||||
if resp.StatusCode >= http.StatusBadRequest {
|
||||
return fmt.Errorf("discovery endpoint returned unexpected status %d", resp.StatusCode)
|
||||
}
|
||||
// Cap the discovery document at 1 MiB; real OIDC discovery responses are a few KiB.
|
||||
body, err := io.ReadAll(io.LimitReader(resp.Body, 1<<20))
|
||||
if err != nil {
|
||||
return errors.Wrap(err, "failed to read discovery response")
|
||||
}
|
||||
var doc struct {
|
||||
Issuer string `json:"issuer"`
|
||||
}
|
||||
if err := json.Unmarshal(body, &doc); err != nil {
|
||||
return errors.Wrap(err, "discovery endpoint did not return valid JSON")
|
||||
}
|
||||
if doc.Issuer == "" {
|
||||
return fmt.Errorf("discovery endpoint response missing required 'issuer' field")
|
||||
}
|
||||
return nil
|
||||
}
|
||||
|
||||
func probeOAuthTokenEndpoint(ctx context.Context, tokenURL string) error {
|
||||
req, err := http.NewRequestWithContext(ctx, http.MethodGet, tokenURL, nil)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
resp, err := http.DefaultClient.Do(req)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
defer drainAndCloseBody(resp.Body)
|
||||
return nil
|
||||
}
|
||||
|
||||
// drainAndCloseBody fully reads and discards an HTTP response body (up to 1 MiB
|
||||
// to bound a misbehaving server) and closes it. Draining before closing allows
|
||||
// net/http to return the underlying TCP connection to the idle pool for
|
||||
// keep-alive reuse on subsequent requests.
|
||||
func drainAndCloseBody(body io.ReadCloser) {
|
||||
_, _ = io.Copy(io.Discard, io.LimitReader(body, 1<<20))
|
||||
_ = body.Close()
|
||||
}
|
||||
|
||||
// TODO: move this into its own push proxy package once one exists (see also pushNotificationClient in server.go)
|
||||
func (ps *PlatformService) testPushProxyConnection(ctx context.Context, serverURL string) error {
|
||||
ctx, cancel := context.WithTimeout(ctx, 10*time.Second)
|
||||
@@ -337,7 +426,7 @@ func (ps *PlatformService) testPushProxyConnection(ctx context.Context, serverUR
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
resp.Body.Close()
|
||||
defer drainAndCloseBody(resp.Body)
|
||||
if resp.StatusCode >= http.StatusBadRequest {
|
||||
return fmt.Errorf("push proxy returned unexpected status %d", resp.StatusCode)
|
||||
}
|
||||
|
||||
@@ -273,6 +273,12 @@ func TestGetSupportPacketDiagnostics(t *testing.T) {
|
||||
assert.Equal(t, model.StatusDisabled, d.ElasticSearch.Status)
|
||||
assert.Empty(t, d.ElasticSearch.ServerVersion)
|
||||
assert.Empty(t, d.ElasticSearch.ServerPlugins)
|
||||
|
||||
/* OAuth Providers (all disabled by default) */
|
||||
assert.Equal(t, model.StatusDisabled, d.OAuthProviders.GitLab.Status)
|
||||
assert.Equal(t, model.StatusDisabled, d.OAuthProviders.Google.Status)
|
||||
assert.Equal(t, model.StatusDisabled, d.OAuthProviders.Office365.Status)
|
||||
assert.Equal(t, model.StatusDisabled, d.OAuthProviders.OpenID.Status)
|
||||
})
|
||||
|
||||
t.Run("filestore fails", func(t *testing.T) {
|
||||
@@ -823,6 +829,171 @@ func TestGetSupportPacketDiagnostics(t *testing.T) {
|
||||
assert.Equal(t, model.StatusFail, packet.Notifications.Email.Status)
|
||||
assert.NotEmpty(t, packet.Notifications.Email.Error)
|
||||
})
|
||||
|
||||
t.Run("OpenID disabled", func(t *testing.T) {
|
||||
th.Service.UpdateConfig(func(cfg *model.Config) {
|
||||
cfg.OpenIdSettings.Enable = model.NewPointer(false)
|
||||
})
|
||||
|
||||
packet := getDiagnostics(t)
|
||||
|
||||
assert.Equal(t, model.StatusDisabled, packet.OAuthProviders.OpenID.Status)
|
||||
assert.Empty(t, packet.OAuthProviders.OpenID.Error)
|
||||
})
|
||||
|
||||
t.Run("OpenID reachable via discovery endpoint", func(t *testing.T) {
|
||||
idp := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
require.Equal(t, "/.well-known/openid-configuration", r.URL.Path)
|
||||
w.Header().Set("Content-Type", "application/json")
|
||||
_, _ = w.Write([]byte(`{"issuer":"https://idp.example.com","authorization_endpoint":"https://idp.example.com/auth"}`))
|
||||
}))
|
||||
defer idp.Close()
|
||||
|
||||
th.Service.UpdateConfig(func(cfg *model.Config) {
|
||||
cfg.OpenIdSettings.Enable = model.NewPointer(true)
|
||||
cfg.OpenIdSettings.DiscoveryEndpoint = model.NewPointer(idp.URL + "/.well-known/openid-configuration")
|
||||
})
|
||||
t.Cleanup(func() {
|
||||
th.Service.UpdateConfig(func(cfg *model.Config) {
|
||||
cfg.OpenIdSettings.Enable = model.NewPointer(false)
|
||||
cfg.OpenIdSettings.DiscoveryEndpoint = model.NewPointer("")
|
||||
})
|
||||
})
|
||||
|
||||
packet := getDiagnostics(t)
|
||||
|
||||
assert.Equal(t, model.StatusOk, packet.OAuthProviders.OpenID.Status)
|
||||
assert.Empty(t, packet.OAuthProviders.OpenID.Error)
|
||||
})
|
||||
|
||||
t.Run("OpenID discovery endpoint returns invalid JSON", func(t *testing.T) {
|
||||
idp := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
_, _ = w.Write([]byte(`not-json`))
|
||||
}))
|
||||
defer idp.Close()
|
||||
|
||||
th.Service.UpdateConfig(func(cfg *model.Config) {
|
||||
cfg.OpenIdSettings.Enable = model.NewPointer(true)
|
||||
cfg.OpenIdSettings.DiscoveryEndpoint = model.NewPointer(idp.URL + "/.well-known/openid-configuration")
|
||||
})
|
||||
t.Cleanup(func() {
|
||||
th.Service.UpdateConfig(func(cfg *model.Config) {
|
||||
cfg.OpenIdSettings.Enable = model.NewPointer(false)
|
||||
cfg.OpenIdSettings.DiscoveryEndpoint = model.NewPointer("")
|
||||
})
|
||||
})
|
||||
|
||||
packet := getDiagnostics(t)
|
||||
|
||||
assert.Equal(t, model.StatusFail, packet.OAuthProviders.OpenID.Status)
|
||||
assert.Contains(t, packet.OAuthProviders.OpenID.Error, "valid JSON")
|
||||
})
|
||||
|
||||
t.Run("OpenID discovery endpoint missing issuer field", func(t *testing.T) {
|
||||
idp := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
w.Header().Set("Content-Type", "application/json")
|
||||
_, _ = w.Write([]byte(`{"authorization_endpoint":"https://idp.example.com/auth"}`))
|
||||
}))
|
||||
defer idp.Close()
|
||||
|
||||
th.Service.UpdateConfig(func(cfg *model.Config) {
|
||||
cfg.OpenIdSettings.Enable = model.NewPointer(true)
|
||||
cfg.OpenIdSettings.DiscoveryEndpoint = model.NewPointer(idp.URL + "/.well-known/openid-configuration")
|
||||
})
|
||||
t.Cleanup(func() {
|
||||
th.Service.UpdateConfig(func(cfg *model.Config) {
|
||||
cfg.OpenIdSettings.Enable = model.NewPointer(false)
|
||||
cfg.OpenIdSettings.DiscoveryEndpoint = model.NewPointer("")
|
||||
})
|
||||
})
|
||||
|
||||
packet := getDiagnostics(t)
|
||||
|
||||
assert.Equal(t, model.StatusFail, packet.OAuthProviders.OpenID.Status)
|
||||
assert.Contains(t, packet.OAuthProviders.OpenID.Error, "issuer")
|
||||
})
|
||||
|
||||
t.Run("OpenID discovery endpoint unreachable", func(t *testing.T) {
|
||||
th.Service.UpdateConfig(func(cfg *model.Config) {
|
||||
cfg.OpenIdSettings.Enable = model.NewPointer(true)
|
||||
cfg.OpenIdSettings.DiscoveryEndpoint = model.NewPointer("http://127.0.0.1:1/.well-known/openid-configuration")
|
||||
})
|
||||
t.Cleanup(func() {
|
||||
th.Service.UpdateConfig(func(cfg *model.Config) {
|
||||
cfg.OpenIdSettings.Enable = model.NewPointer(false)
|
||||
cfg.OpenIdSettings.DiscoveryEndpoint = model.NewPointer("")
|
||||
})
|
||||
})
|
||||
|
||||
packet := getDiagnostics(t)
|
||||
|
||||
assert.Equal(t, model.StatusFail, packet.OAuthProviders.OpenID.Status)
|
||||
assert.NotEmpty(t, packet.OAuthProviders.OpenID.Error)
|
||||
})
|
||||
|
||||
t.Run("GitLab enabled with reachable token endpoint", func(t *testing.T) {
|
||||
// GitLab has no DiscoveryEndpoint by default, so we fall through to the
|
||||
// TokenEndpoint host probe. Token endpoints reject GETs, so any HTTP
|
||||
// response (including 4xx/5xx) is treated as reachable.
|
||||
idp := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
w.WriteHeader(http.StatusMethodNotAllowed)
|
||||
}))
|
||||
defer idp.Close()
|
||||
|
||||
th.Service.UpdateConfig(func(cfg *model.Config) {
|
||||
cfg.GitLabSettings.Enable = model.NewPointer(true)
|
||||
cfg.GitLabSettings.DiscoveryEndpoint = model.NewPointer("")
|
||||
cfg.GitLabSettings.TokenEndpoint = model.NewPointer(idp.URL + "/oauth/token")
|
||||
})
|
||||
t.Cleanup(func() {
|
||||
th.Service.UpdateConfig(func(cfg *model.Config) {
|
||||
cfg.GitLabSettings.Enable = model.NewPointer(false)
|
||||
cfg.GitLabSettings.TokenEndpoint = model.NewPointer("")
|
||||
})
|
||||
})
|
||||
|
||||
packet := getDiagnostics(t)
|
||||
|
||||
assert.Equal(t, model.StatusOk, packet.OAuthProviders.GitLab.Status)
|
||||
assert.Empty(t, packet.OAuthProviders.GitLab.Error)
|
||||
})
|
||||
|
||||
t.Run("GitLab enabled with unreachable token endpoint", func(t *testing.T) {
|
||||
th.Service.UpdateConfig(func(cfg *model.Config) {
|
||||
cfg.GitLabSettings.Enable = model.NewPointer(true)
|
||||
cfg.GitLabSettings.DiscoveryEndpoint = model.NewPointer("")
|
||||
cfg.GitLabSettings.TokenEndpoint = model.NewPointer("http://127.0.0.1:1/oauth/token")
|
||||
})
|
||||
t.Cleanup(func() {
|
||||
th.Service.UpdateConfig(func(cfg *model.Config) {
|
||||
cfg.GitLabSettings.Enable = model.NewPointer(false)
|
||||
cfg.GitLabSettings.TokenEndpoint = model.NewPointer("")
|
||||
})
|
||||
})
|
||||
|
||||
packet := getDiagnostics(t)
|
||||
|
||||
assert.Equal(t, model.StatusFail, packet.OAuthProviders.GitLab.Status)
|
||||
assert.NotEmpty(t, packet.OAuthProviders.GitLab.Error)
|
||||
})
|
||||
|
||||
t.Run("GitLab enabled with no endpoints configured", func(t *testing.T) {
|
||||
th.Service.UpdateConfig(func(cfg *model.Config) {
|
||||
cfg.GitLabSettings.Enable = model.NewPointer(true)
|
||||
cfg.GitLabSettings.DiscoveryEndpoint = model.NewPointer("")
|
||||
cfg.GitLabSettings.TokenEndpoint = model.NewPointer("")
|
||||
})
|
||||
t.Cleanup(func() {
|
||||
th.Service.UpdateConfig(func(cfg *model.Config) {
|
||||
cfg.GitLabSettings.Enable = model.NewPointer(false)
|
||||
})
|
||||
})
|
||||
|
||||
packet := getDiagnostics(t)
|
||||
|
||||
assert.Equal(t, model.StatusFail, packet.OAuthProviders.GitLab.Status)
|
||||
assert.Contains(t, packet.OAuthProviders.GitLab.Error, "no discovery or token endpoint")
|
||||
})
|
||||
}
|
||||
|
||||
func TestGetSanitizedConfigFile(t *testing.T) {
|
||||
|
||||
@@ -106,6 +106,22 @@ type SupportPacketDiagnostics struct {
|
||||
ServerPlugins []string `yaml:"server_plugins,omitempty"`
|
||||
Error string `yaml:"error,omitempty"`
|
||||
} `yaml:"elastic"`
|
||||
|
||||
OAuthProviders OAuthProviders `yaml:"oauth_providers,omitempty"`
|
||||
}
|
||||
|
||||
// OAuthProviderStatus reports the connectivity status of a single OAuth2/OpenID Connect provider.
|
||||
type OAuthProviderStatus struct {
|
||||
Status string `yaml:"status,omitempty"` // ok / fail / disabled
|
||||
Error string `yaml:"error,omitempty"`
|
||||
}
|
||||
|
||||
// OAuthProviders aggregates the connectivity status for the configured OAuth2/OpenID Connect providers.
|
||||
type OAuthProviders struct {
|
||||
GitLab OAuthProviderStatus `yaml:"gitlab,omitempty"`
|
||||
Google OAuthProviderStatus `yaml:"google,omitempty"`
|
||||
Office365 OAuthProviderStatus `yaml:"office365,omitempty"`
|
||||
OpenID OAuthProviderStatus `yaml:"openid,omitempty"`
|
||||
}
|
||||
|
||||
type SupportPacketStats struct {
|
||||
|
||||
Reference in New Issue
Block a user