Fixes for SSO.

* Verbose logging.
* Cookie needs a path.
* Configurable callback URL.
* Various sanity checks.

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,

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 {

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:

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)
 

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)

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"`