From 6de730b2ee4b5460437a9116583298c1ba7c5871 Mon Sep 17 00:00:00 2001 From: Tommie Gannert Date: Wed, 25 May 2022 18:33:11 +0200 Subject: [PATCH] Fixes for SSO. * Verbose logging. * Cookie needs a path. * Configurable callback URL. * Various sanity checks. --- clientapi/auth/sso/oauth2.go | 4 ++ clientapi/auth/sso/oidc.go | 42 +++++++++++++++++-- clientapi/auth/sso/sso.go | 8 +++- clientapi/routing/routing.go | 10 +++-- clientapi/routing/sso.go | 70 +++++++++++++++++++++++++++++--- setup/config/config_clientapi.go | 8 ++++ 6 files changed, 127 insertions(+), 15 deletions(-) diff --git a/clientapi/auth/sso/oauth2.go b/clientapi/auth/sso/oauth2.go index 287e187bc..b8d12b59e 100644 --- a/clientapi/auth/sso/oauth2.go +++ b/clientapi/auth/sso/oauth2.go @@ -101,6 +101,10 @@ func (p *oauth2IdentityProvider) ProcessCallback(ctx context.Context, callbackUR return nil, err } + if subject == "" { + return nil, fmt.Errorf("no subject from SSO provider") + } + return &CallbackResult{ Identifier: &UserIdentifier{ Namespace: uapi.SSOIDNamespace, diff --git a/clientapi/auth/sso/oidc.go b/clientapi/auth/sso/oidc.go index ac9c56a57..660816d9d 100644 --- a/clientapi/auth/sso/oidc.go +++ b/clientapi/auth/sso/oidc.go @@ -35,8 +35,8 @@ type oidcIdentityProvider struct { mu sync.Mutex } -func newOIDCIdentityProvider(cfg *config.IdentityProvider, hc *http.Client) *oidcIdentityProvider { - return &oidcIdentityProvider{ +func newOIDCIdentityProvider(ctx context.Context, cfg *config.IdentityProvider, hc *http.Client) (*oidcIdentityProvider, error) { + p := &oidcIdentityProvider{ oauth2IdentityProvider: &oauth2IdentityProvider{ cfg: cfg, hc: hc, @@ -49,6 +49,17 @@ func newOIDCIdentityProvider(cfg *config.IdentityProvider, hc *http.Client) *oid suggestedUserIDPath: "preferred_username", }, } + + // TODO: Complement starts and waits for the "base image" without + // first starting httpmockserver, which means we cannot always do + // this sanity check. + if false { + if _, _, err := p.get(ctx); err != nil { + return nil, err + } + } + + return p, nil } func (p *oidcIdentityProvider) AuthorizationURL(ctx context.Context, callbackURL, nonce string) (string, error) { @@ -129,7 +140,20 @@ func oidcDiscover(ctx context.Context, url string) (*oidcDiscovery, error) { var disc oidcDiscovery if err := json.NewDecoder(hresp.Body).Decode(&disc); err != nil { - return nil, err + return nil, fmt.Errorf("decoding OIDC discovery response from %q: %w", url, err) + } + + if !validWebURL(disc.Issuer) { + return nil, fmt.Errorf("issuer identifier is invalid in %q", url) + } + if !validWebURL(disc.AuthorizationEndpoint) { + return nil, fmt.Errorf("authorization endpoint is invalid in %q", url) + } + if !validWebURL(disc.TokenEndpoint) { + return nil, fmt.Errorf("token endpoint is invalid in %q", url) + } + if !validWebURL(disc.UserinfoEndpoint) { + return nil, fmt.Errorf("userinfo endpoint is invalid in %q", url) } if disc.ScopesSupported != nil { @@ -149,6 +173,18 @@ func oidcDiscover(ctx context.Context, url string) (*oidcDiscovery, error) { return &disc, nil } +func validWebURL(s string) bool { + if s == "" { + return false + } + + u, err := url.Parse(s) + if err != nil { + return false + } + return u.Scheme != "" && u.Host != "" +} + func stringSliceContains(ss []string, s string) bool { for _, s2 := range ss { if s2 == s { diff --git a/clientapi/auth/sso/sso.go b/clientapi/auth/sso/sso.go index 5923d8491..593fded3a 100644 --- a/clientapi/auth/sso/sso.go +++ b/clientapi/auth/sso/sso.go @@ -29,7 +29,7 @@ type Authenticator struct { providers map[string]identityProvider } -func NewAuthenticator(cfg *config.SSO) (*Authenticator, error) { +func NewAuthenticator(ctx context.Context, cfg *config.SSO) (*Authenticator, error) { hc := &http.Client{ Timeout: 10 * time.Second, Transport: &http.Transport{ @@ -49,7 +49,11 @@ func NewAuthenticator(cfg *config.SSO) (*Authenticator, error) { switch typ { case config.SSOTypeOIDC: - a.providers[pcfg.ID] = newOIDCIdentityProvider(&pcfg, hc) + p, err := newOIDCIdentityProvider(ctx, &pcfg, hc) + if err != nil { + return nil, fmt.Errorf("failed to create OpenID Connect provider %q: %w", pcfg.ID, err) + } + a.providers[pcfg.ID] = p case config.SSOTypeGitHub: a.providers[pcfg.ID] = newGitHubIdentityProvider(&pcfg, hc) default: diff --git a/clientapi/routing/routing.go b/clientapi/routing/routing.go index ebc7003d7..657954adb 100644 --- a/clientapi/routing/routing.go +++ b/clientapi/routing/routing.go @@ -63,6 +63,8 @@ func Setup( extRoomsProvider api.ExtraPublicRoomsProvider, mscCfg *config.MSCs, natsClient *nats.Conn, ) { + ctx := context.Background() + prometheus.MustRegister(amtRegUsers, sendEventDuration) rateLimits := httputil.NewRateLimits(&cfg.RateLimiting) @@ -71,7 +73,7 @@ func Setup( var ssoAuthenticator *sso.Authenticator if cfg.Login.SSO.Enabled { var err error - ssoAuthenticator, err = sso.NewAuthenticator(&cfg.Login.SSO) + ssoAuthenticator, err = sso.NewAuthenticator(ctx, &cfg.Login.SSO) if err != nil { logrus.WithError(err).Fatal("failed to create SSO authenticator") } @@ -139,7 +141,7 @@ func Setup( // server notifications if cfg.Matrix.ServerNotices.Enabled { logrus.Info("Enabling server notices at /_synapse/admin/v1/send_server_notice") - serverNotificationSender, err := getSenderDevice(context.Background(), userAPI, cfg) + serverNotificationSender, err := getSenderDevice(ctx, userAPI, cfg) if err != nil { logrus.WithError(err).Fatal("unable to get account for sending sending server notices") } @@ -581,14 +583,14 @@ func Setup( v3mux.Handle("/login/sso/redirect", httputil.MakeExternalAPI("login", func(req *http.Request) util.JSONResponse { - return SSORedirect(req, "", ssoAuthenticator) + return SSORedirect(req, "", ssoAuthenticator, &cfg.Login.SSO) }), ).Methods(http.MethodGet, http.MethodOptions) v3mux.Handle("/login/sso/redirect/{idpID}", httputil.MakeExternalAPI("login", func(req *http.Request) util.JSONResponse { vars := mux.Vars(req) - return SSORedirect(req, vars["idpID"], ssoAuthenticator) + return SSORedirect(req, vars["idpID"], ssoAuthenticator, &cfg.Login.SSO) }), ).Methods(http.MethodGet, http.MethodOptions) diff --git a/clientapi/routing/sso.go b/clientapi/routing/sso.go index 6e2e1d967..55df5c49b 100644 --- a/clientapi/routing/sso.go +++ b/clientapi/routing/sso.go @@ -17,6 +17,7 @@ package routing import ( "context" "encoding/base64" + "fmt" "net/http" "net/url" "strings" @@ -25,6 +26,7 @@ import ( "github.com/matrix-org/dendrite/clientapi/auth/sso" "github.com/matrix-org/dendrite/clientapi/jsonerror" "github.com/matrix-org/dendrite/clientapi/userutil" + "github.com/matrix-org/dendrite/setup/config" uapi "github.com/matrix-org/dendrite/userapi/api" "github.com/matrix-org/gomatrixserverlib" "github.com/matrix-org/util" @@ -36,7 +38,10 @@ func SSORedirect( req *http.Request, idpID string, auth *sso.Authenticator, + cfg *config.SSO, ) util.JSONResponse { + ctx := req.Context() + if auth == nil { return util.JSONResponse{ Code: http.StatusNotFound, @@ -59,27 +64,71 @@ func SSORedirect( } } - callbackURL := req.URL.ResolveReference(&url.URL{Path: "../callback", RawQuery: url.Values{"provider": []string{idpID}}.Encode()}) - nonce := formatNonce(redirectURL) - u, err := auth.AuthorizationURL(req.Context(), idpID, callbackURL.String(), nonce) + callbackURL, err := buildCallbackURLFromRedirect(cfg, req) if err != nil { + util.GetLogger(ctx).WithError(err).Error("Failed to build callback URL") return util.JSONResponse{ Code: http.StatusInternalServerError, JSON: err, } } + callbackURL = callbackURL.ResolveReference(&url.URL{ + RawQuery: url.Values{"provider": []string{idpID}}.Encode(), + }) + nonce := formatNonce(redirectURL) + u, err := auth.AuthorizationURL(ctx, idpID, callbackURL.String(), nonce) + if err != nil { + util.GetLogger(ctx).WithError(err).Error("Failed to get SSO authorization URL") + return util.JSONResponse{ + Code: http.StatusInternalServerError, + JSON: err, + } + } + + util.GetLogger(ctx).Infof("SSO redirect to %s.", u) + resp := util.RedirectResponse(u) resp.Headers["Set-Cookie"] = (&http.Cookie{ Name: "oidc_nonce", Value: nonce, + Path: "/", Expires: time.Now().Add(10 * time.Minute), - Secure: true, + Secure: callbackURL.Scheme != "http", SameSite: http.SameSiteStrictMode, }).String() return resp } +// buildCallbackURLFromRedirect builds a callback URL from a redirect +// request and configuration. +func buildCallbackURLFromRedirect(cfg *config.SSO, req *http.Request) (*url.URL, error) { + u := &url.URL{ + Scheme: "https", + User: req.URL.User, + Host: req.Host, + Path: req.URL.Path, + } + if req.TLS == nil { + u.Scheme = "http" + } + + // Find the v3mux base, handling both `redirect` and + // `redirect/{idp}` and not hard-coding the Matrix version. + const redirectPath = "/login/sso/redirect" + i := strings.Index(u.Path, redirectPath) + if i < 0 { + return nil, fmt.Errorf("cannot find %q to replace in URL %q", redirectPath, u.Path) + } + u.Path = u.Path[:i] + "/login/sso/callback" + + cu, err := url.Parse(cfg.CallbackURL) + if err != nil { + return nil, err + } + return u.ResolveReference(cu), nil +} + // SSOCallback implements /login/sso/callback. // https://spec.matrix.org/v1.2/client-server-api/#handling-the-callback-from-the-authentication-server func SSOCallback( @@ -131,11 +180,13 @@ func SSOCallback( } result, err := auth.ProcessCallback(ctx, idpID, callbackURL.String(), nonce.Value, query) if err != nil { + util.GetLogger(ctx).WithError(err).Error("Failed to process callback") return util.JSONResponse{ Code: http.StatusInternalServerError, JSON: err, } } + util.GetLogger(ctx).WithField("result", result).Info("SSO callback done") if result.Identifier == nil { // Not authenticated yet. @@ -144,7 +195,7 @@ func SSOCallback( localpart, err := verifySSOUserIdentifier(ctx, userAPI, result.Identifier, serverName) if err != nil { - util.GetLogger(ctx).WithError(err).WithField("identifier", result.Identifier).Error("failed to find user") + util.GetLogger(ctx).WithError(err).WithField("ssoIdentifier", result.Identifier).Error("failed to find user") return util.JSONResponse{ Code: http.StatusUnauthorized, JSON: jsonerror.Forbidden("ID not associated with a local account"), @@ -153,10 +204,16 @@ func SSOCallback( if localpart == "" { // The user doesn't exist. // TODO: let the user select the local part, and whether to associate email addresses. + util.GetLogger(ctx).WithField("localpart", result.SuggestedUserID).WithField("ssoIdentifier", result.Identifier).Info("SSO registering account") localpart = result.SuggestedUserID + if localpart == "" { + util.GetLogger(ctx).WithError(err).WithField("ssoIdentifier", result.Identifier).Info("no suggested user ID from SSO provider") + localpart = result.Identifier.Subject + } + ok, resp := registerSSOAccount(ctx, userAPI, result.Identifier, localpart) if !ok { - util.GetLogger(ctx).WithError(err).WithField("identifier", result.Identifier).WithField("localpart", localpart).Error("failed to create account") + util.GetLogger(ctx).WithError(err).WithField("ssoIdentifier", result.Identifier).WithField("localpart", localpart).Error("failed to register account") return resp } } @@ -166,6 +223,7 @@ func SSOCallback( util.GetLogger(ctx).WithError(err).Errorf("PerformLoginTokenCreation failed") return jsonerror.InternalServerError() } + util.GetLogger(ctx).WithField("localpart", localpart).WithField("ssoIdentifier", result.Identifier).Info("SSO created token") rquery := finalRedirectURL.Query() rquery.Set("loginToken", token.Token) diff --git a/setup/config/config_clientapi.go b/setup/config/config_clientapi.go index 2106c341c..342ad2945 100644 --- a/setup/config/config_clientapi.go +++ b/setup/config/config_clientapi.go @@ -117,6 +117,14 @@ type SSO struct { // Enabled determines whether SSO should be allowed. Enabled bool `yaml:"enabled"` + // CallbackURL is the absolute URL where a user agent can reach + // the Dendrite `/_matrix/v3/login/sso/callback` endpoint. This is + // used to create SSO redirect URLs passed to identity + // providers. If this is empty, a default is inferred from request + // headers. When Dendrite is running behind a proxy, this may not + // always be the right information. + CallbackURL string `yaml:"callback_url"` + // Providers list the identity providers this server is capable of confirming an // identity with. Providers []IdentityProvider `yaml:"providers"`