16171: Configurable "email" and "email_verified" OIDC claim keys.
authorTom Clegg <tom@tomclegg.ca>
Tue, 9 Jun 2020 14:22:09 +0000 (10:22 -0400)
committerTom Clegg <tom@tomclegg.ca>
Tue, 9 Jun 2020 14:22:09 +0000 (10:22 -0400)
Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom@tomclegg.ca>

lib/config/config.default.yml
lib/config/generated_config.go
lib/controller/localdb/login.go
lib/controller/localdb/login_oidc.go
lib/controller/localdb/login_oidc_test.go
sdk/go/arvados/config.go

index 219f6ef0ba91a1afb2e3311ca66b94f5a989020f..8b54d94c6a34eb3be8b7da03db606420a1f629fa 100644 (file)
@@ -569,6 +569,17 @@ Clusters:
         ClientID: ""
         ClientSecret: ""
 
+        # OpenID claim field containing the user's email
+        # address. Normally "email"; see
+        # https://openid.net/specs/openid-connect-core-1_0.html#StandardClaims
+        EmailClaim: "email"
+
+        # OpenID claim field containing the email verification
+        # flag. Normally "email_verified".  To accept every returned
+        # email address without checking a "verified" field at all,
+        # use the empty string "".
+        EmailVerifiedClaim: "email_verified"
+
       PAM:
         # (Experimental) Use PAM to authenticate users.
         Enable: false
index 6f8cab462bce2dc15118f31454b40bb35d06e3ff..e4ad42a084aa6190d0c6eb3e947a494da42ae091 100644 (file)
@@ -575,6 +575,17 @@ Clusters:
         ClientID: ""
         ClientSecret: ""
 
+        # OpenID claim field containing the user's email
+        # address. Normally "email"; see
+        # https://openid.net/specs/openid-connect-core-1_0.html#StandardClaims
+        EmailClaim: "email"
+
+        # OpenID claim field containing the email verification
+        # flag. Normally "email_verified".  To accept every returned
+        # email address without checking a "verified" field at all,
+        # use the empty string "".
+        EmailVerifiedClaim: "email_verified"
+
       PAM:
         # (Experimental) Use PAM to authenticate users.
         Enable: false
index 9a0ee746e64006d08ab2d87981f6f47bf8fbcfa6..adc22c7e55023943da2b7f339c2dcaa79af97b07 100644 (file)
@@ -37,14 +37,18 @@ func chooseLoginController(cluster *arvados.Cluster, railsProxy *railsProxy) log
                        ClientID:           cluster.Login.Google.ClientID,
                        ClientSecret:       cluster.Login.Google.ClientSecret,
                        UseGooglePeopleAPI: cluster.Login.Google.AlternateEmailAddresses,
+                       EmailClaim:         "email",
+                       EmailVerifiedClaim: "email_verified",
                }
        case !wantGoogle && wantOpenIDConnect && !wantSSO && !wantPAM && !wantLDAP:
                return &oidcLoginController{
-                       Cluster:      cluster,
-                       RailsProxy:   railsProxy,
-                       Issuer:       cluster.Login.OpenIDConnect.Issuer,
-                       ClientID:     cluster.Login.OpenIDConnect.ClientID,
-                       ClientSecret: cluster.Login.OpenIDConnect.ClientSecret,
+                       Cluster:            cluster,
+                       RailsProxy:         railsProxy,
+                       Issuer:             cluster.Login.OpenIDConnect.Issuer,
+                       ClientID:           cluster.Login.OpenIDConnect.ClientID,
+                       ClientSecret:       cluster.Login.OpenIDConnect.ClientSecret,
+                       EmailClaim:         cluster.Login.OpenIDConnect.EmailClaim,
+                       EmailVerifiedClaim: cluster.Login.OpenIDConnect.EmailVerifiedClaim,
                }
        case !wantGoogle && !wantOpenIDConnect && wantSSO && !wantPAM && !wantLDAP:
                return &ssoLoginController{railsProxy}
index f42b8f8beaf1d2721a78c9883c20353eabd0e43b..a9047995c15809a2a78685e45fdd7fffb2804d1f 100644 (file)
@@ -36,7 +36,9 @@ type oidcLoginController struct {
        Issuer             string // OIDC issuer URL, e.g., "https://accounts.google.com"
        ClientID           string
        ClientSecret       string
-       UseGooglePeopleAPI bool // Use Google People API to look up alternate email addresses
+       UseGooglePeopleAPI bool   // Use Google People API to look up alternate email addresses
+       EmailClaim         string // OpenID claim to use as email address; typically "email"
+       EmailVerifiedClaim string // If non-empty, ensure claim value is true before accepting EmailClaim; typically "email_verified"
 
        // override Google People API base URL for testing purposes
        // (normally empty, set by google pkg to
@@ -145,28 +147,25 @@ func (ctrl *oidcLoginController) getAuthInfo(ctx context.Context, token *oauth2.
        var ret rpc.UserSessionAuthInfo
        defer ctxlog.FromContext(ctx).WithField("ret", &ret).Debug("getAuthInfo returned")
 
-       var claims struct {
-               Name     string `json:"name"`
-               Email    string `json:"email"`
-               Verified bool   `json:"email_verified"`
-       }
+       var claims map[string]interface{}
        if err := idToken.Claims(&claims); err != nil {
                return nil, fmt.Errorf("error extracting claims from ID token: %s", err)
-       } else if claims.Verified {
+       } else if verified, _ := claims[ctrl.EmailVerifiedClaim].(bool); verified || ctrl.EmailVerifiedClaim == "" {
                // Fall back to this info if the People API call
                // (below) doesn't return a primary && verified email.
-               if names := strings.Fields(strings.TrimSpace(claims.Name)); len(names) > 1 {
+               name, _ := claims["name"].(string)
+               if names := strings.Fields(strings.TrimSpace(name)); len(names) > 1 {
                        ret.FirstName = strings.Join(names[0:len(names)-1], " ")
                        ret.LastName = names[len(names)-1]
                } else {
                        ret.FirstName = names[0]
                }
-               ret.Email = claims.Email
+               ret.Email, _ = claims[ctrl.EmailClaim].(string)
        }
 
        if !ctrl.UseGooglePeopleAPI {
                if ret.Email == "" {
-                       return nil, fmt.Errorf("cannot log in with unverified email address %q", claims.Email)
+                       return nil, fmt.Errorf("cannot log in with unverified email address %q", claims[ctrl.EmailClaim])
                }
                return &ret, nil
        }
index aa437218ff79eaae26ee93d8450af72561387919..abbd4b98f6af706f2b0463e1c5c82f595480476b 100644 (file)
@@ -113,6 +113,8 @@ func (s *OIDCLoginSuite) SetUpTest(c *check.C) {
                                "email":          s.authEmail,
                                "email_verified": s.authEmailVerified,
                                "name":           s.authName,
+                               "alt_verified":   true,                    // for custom claim tests
+                               "alt_email":      "alt_email@example.com", // for custom claim tests
                        })
                        json.NewEncoder(w).Encode(struct {
                                AccessToken  string `json:"access_token"`
@@ -299,7 +301,7 @@ func (s *OIDCLoginSuite) TestGoogleLogin_PeopleAPIError(c *check.C) {
        c.Check(resp.RedirectLocation, check.Equals, "")
 }
 
-func (s *OIDCLoginSuite) TestOIDCLogin_Success(c *check.C) {
+func (s *OIDCLoginSuite) TestGenericOIDCLogin(c *check.C) {
        s.cluster.Login.Google.Enable = false
        s.cluster.Login.OpenIDConnect.Enable = true
        json.Unmarshal([]byte(fmt.Sprintf("%q", s.fakeIssuer.URL)), &s.cluster.Login.OpenIDConnect.Issuer)
@@ -307,18 +309,75 @@ func (s *OIDCLoginSuite) TestOIDCLogin_Success(c *check.C) {
        s.cluster.Login.OpenIDConnect.ClientSecret = "oidc#client#secret"
        s.validClientID = "oidc#client#id"
        s.validClientSecret = "oidc#client#secret"
-       s.localdb = NewConn(s.cluster)
-       state := s.startLogin(c)
-       resp, err := s.localdb.Login(context.Background(), arvados.LoginOptions{
-               Code:  s.validCode,
-               State: state,
-       })
-       c.Assert(err, check.IsNil)
-       c.Check(resp.HTML.String(), check.Equals, "")
-       target, err := url.Parse(resp.RedirectLocation)
-       c.Assert(err, check.IsNil)
-       token := target.Query().Get("api_token")
-       c.Check(token, check.Matches, `v2/zzzzz-gj3su-.{15}/.{32,50}`)
+       for _, trial := range []struct {
+               expectEmail string // "" if failure expected
+               setup       func()
+       }{
+               {
+                       expectEmail: "user@oidc.example.com",
+                       setup: func() {
+                               c.Log("=== succeed because email_verified is false but not required")
+                               s.authEmail = "user@oidc.example.com"
+                               s.authEmailVerified = false
+                               s.cluster.Login.OpenIDConnect.EmailVerifiedClaim = ""
+                       },
+               },
+               {
+                       expectEmail: "",
+                       setup: func() {
+                               c.Log("=== fail because email_verified is false and required")
+                               s.authEmail = "user@oidc.example.com"
+                               s.authEmailVerified = false
+                               s.cluster.Login.OpenIDConnect.EmailVerifiedClaim = "email_verified"
+                       },
+               },
+               {
+                       expectEmail: "user@oidc.example.com",
+                       setup: func() {
+                               c.Log("=== succeed because email_verified is false but config uses custom 'verified' claim")
+                               s.authEmail = "user@oidc.example.com"
+                               s.authEmailVerified = false
+                               s.cluster.Login.OpenIDConnect.EmailVerifiedClaim = "alt_verified"
+                       },
+               },
+               {
+                       expectEmail: "alt_email@example.com",
+                       setup: func() {
+                               c.Log("=== succeed with custom 'email' and 'email_verified' claims")
+                               s.authEmail = "bad@wrong.example.com"
+                               s.authEmailVerified = false
+                               s.cluster.Login.OpenIDConnect.EmailClaim = "alt_email"
+                               s.cluster.Login.OpenIDConnect.EmailVerifiedClaim = "alt_verified"
+                       },
+               },
+       } {
+               trial.setup()
+               if s.railsSpy != nil {
+                       s.railsSpy.Close()
+               }
+               s.railsSpy = arvadostest.NewProxy(c, s.cluster.Services.RailsAPI)
+               s.localdb = NewConn(s.cluster)
+               *s.localdb.railsProxy = *rpc.NewConn(s.cluster.ClusterID, s.railsSpy.URL, true, rpc.PassthroughTokenProvider)
+
+               state := s.startLogin(c)
+               resp, err := s.localdb.Login(context.Background(), arvados.LoginOptions{
+                       Code:  s.validCode,
+                       State: state,
+               })
+               c.Assert(err, check.IsNil)
+               if trial.expectEmail == "" {
+                       c.Check(resp.HTML.String(), check.Matches, `(?ms).*Login error.*`)
+                       c.Check(resp.RedirectLocation, check.Equals, "")
+                       continue
+               }
+               c.Check(resp.HTML.String(), check.Equals, "")
+               target, err := url.Parse(resp.RedirectLocation)
+               c.Assert(err, check.IsNil)
+               token := target.Query().Get("api_token")
+               c.Check(token, check.Matches, `v2/zzzzz-gj3su-.{15}/.{32,50}`)
+               authinfo := getCallbackAuthInfo(c, s.railsSpy)
+               c.Check(authinfo.Email, check.Equals, trial.expectEmail)
+       }
 }
 
 func (s *OIDCLoginSuite) TestGoogleLogin_Success(c *check.C) {
index dbd9f71099619203bb38f4dd1118b865f5c2f662..7b07175f1783553e937b8f2c1e3cc8fc170ce9b8 100644 (file)
@@ -157,10 +157,12 @@ type Cluster struct {
                        AlternateEmailAddresses bool
                }
                OpenIDConnect struct {
-                       Enable       bool
-                       Issuer       string
-                       ClientID     string
-                       ClientSecret string
+                       Enable             bool
+                       Issuer             string
+                       ClientID           string
+                       ClientSecret       string
+                       EmailClaim         string
+                       EmailVerifiedClaim string
                }
                PAM struct {
                        Enable             bool