X-Git-Url: https://git.arvados.org/arvados.git/blobdiff_plain/6470f7ce527c2cc44e25de402bd0418c821d13a8..HEAD:/lib/controller/localdb/login_oidc_test.go diff --git a/lib/controller/localdb/login_oidc_test.go b/lib/controller/localdb/login_oidc_test.go index e157b73fc6..f505f5bc49 100644 --- a/lib/controller/localdb/login_oidc_test.go +++ b/lib/controller/localdb/login_oidc_test.go @@ -15,17 +15,18 @@ import ( "net/http" "net/http/httptest" "net/url" + "regexp" "sort" "strings" + "sync" "testing" "time" - "git.arvados.org/arvados.git/lib/config" "git.arvados.org/arvados.git/lib/controller/rpc" + "git.arvados.org/arvados.git/lib/ctrlctx" "git.arvados.org/arvados.git/sdk/go/arvados" "git.arvados.org/arvados.git/sdk/go/arvadostest" "git.arvados.org/arvados.git/sdk/go/auth" - "git.arvados.org/arvados.git/sdk/go/ctxlog" "github.com/jmoiron/sqlx" check "gopkg.in/check.v1" ) @@ -38,56 +39,132 @@ func Test(t *testing.T) { var _ = check.Suite(&OIDCLoginSuite{}) type OIDCLoginSuite struct { - cluster *arvados.Cluster - localdb *Conn - railsSpy *arvadostest.Proxy + localdbSuite + trustedURL *arvados.URL fakeProvider *arvadostest.OIDCProvider } -func (s *OIDCLoginSuite) TearDownSuite(c *check.C) { - // Undo any changes/additions to the user database so they - // don't affect subsequent tests. - arvadostest.ResetEnv() - c.Check(arvados.NewClientFromEnv().RequestAndDecode(nil, "POST", "database/reset", nil, nil), check.IsNil) -} - func (s *OIDCLoginSuite) SetUpTest(c *check.C) { + s.trustedURL = &arvados.URL{Scheme: "https", Host: "app.example.com:443", Path: "/"} + s.fakeProvider = arvadostest.NewOIDCProvider(c) s.fakeProvider.AuthEmail = "active-user@arvados.local" s.fakeProvider.AuthEmailVerified = true s.fakeProvider.AuthName = "Fake User Name" + s.fakeProvider.AuthGivenName = "Fake" + s.fakeProvider.AuthFamilyName = "User Name" s.fakeProvider.ValidCode = fmt.Sprintf("abcdefgh-%d", time.Now().Unix()) s.fakeProvider.PeopleAPIResponse = map[string]interface{}{} - cfg, err := config.NewLoader(nil, ctxlog.TestLogger(c)).Load() - c.Assert(err, check.IsNil) - s.cluster, err = cfg.GetCluster("") - c.Assert(err, check.IsNil) - s.cluster.Login.SSO.Enable = false + s.localdbSuite.SetUpTest(c) + + s.cluster.Login.Test.Enable = false s.cluster.Login.Google.Enable = true s.cluster.Login.Google.ClientID = "test%client$id" s.cluster.Login.Google.ClientSecret = "test#client/secret" + s.cluster.Login.TrustedClients = map[arvados.URL]struct{}{*s.trustedURL: {}} s.cluster.Users.PreferDomainForUsername = "PreferDomainForUsername.example.com" s.fakeProvider.ValidClientID = "test%client$id" s.fakeProvider.ValidClientSecret = "test#client/secret" - s.localdb = NewConn(s.cluster) + s.localdb = NewConn(s.ctx, s.cluster, (&ctrlctx.DBConnector{PostgreSQL: s.cluster.PostgreSQL}).GetDB) c.Assert(s.localdb.loginController, check.FitsTypeOf, (*oidcLoginController)(nil)) s.localdb.loginController.(*oidcLoginController).Issuer = s.fakeProvider.Issuer.URL s.localdb.loginController.(*oidcLoginController).peopleAPIBasePath = s.fakeProvider.PeopleAPI.URL - s.railsSpy = arvadostest.NewProxy(c, s.cluster.Services.RailsAPI) *s.localdb.railsProxy = *rpc.NewConn(s.cluster.ClusterID, s.railsSpy.URL, true, rpc.PassthroughTokenProvider) } -func (s *OIDCLoginSuite) TearDownTest(c *check.C) { - s.railsSpy.Close() -} - func (s *OIDCLoginSuite) TestGoogleLogout(c *check.C) { + s.cluster.Login.TrustedClients[arvados.URL{Scheme: "https", Host: "foo.example", Path: "/"}] = struct{}{} + s.cluster.Login.TrustPrivateNetworks = false + resp, err := s.localdb.Logout(context.Background(), arvados.LogoutOptions{ReturnTo: "https://foo.example.com/bar"}) + c.Check(err, check.NotNil) + c.Check(resp.RedirectLocation, check.Equals, "") + + resp, err = s.localdb.Logout(context.Background(), arvados.LogoutOptions{ReturnTo: "https://127.0.0.1/bar"}) + c.Check(err, check.NotNil) + c.Check(resp.RedirectLocation, check.Equals, "") + + resp, err = s.localdb.Logout(context.Background(), arvados.LogoutOptions{ReturnTo: "https://foo.example/bar"}) + c.Check(err, check.IsNil) + c.Check(resp.RedirectLocation, check.Equals, "https://foo.example/bar") + + s.cluster.Login.TrustPrivateNetworks = true + + resp, err = s.localdb.Logout(context.Background(), arvados.LogoutOptions{ReturnTo: "https://192.168.1.1/bar"}) c.Check(err, check.IsNil) - c.Check(resp.RedirectLocation, check.Equals, "https://foo.example.com/bar") + c.Check(resp.RedirectLocation, check.Equals, "https://192.168.1.1/bar") +} + +func (s *OIDCLoginSuite) checkRPInitiatedLogout(c *check.C, returnTo string) { + if !c.Check(s.fakeProvider.EndSessionEndpoint, check.NotNil, + check.Commentf("buggy test: EndSessionEndpoint not configured")) { + return + } + expURL, err := url.Parse(s.fakeProvider.Issuer.URL) + if !c.Check(err, check.IsNil, check.Commentf("error parsing expected URL")) { + return + } + expURL.Path = expURL.Path + s.fakeProvider.EndSessionEndpoint.Path + + accessToken := s.fakeProvider.ValidAccessToken() + ctx := ctrlctx.NewWithToken(s.ctx, s.cluster, accessToken) + resp, err := s.localdb.Logout(ctx, arvados.LogoutOptions{ReturnTo: returnTo}) + if !c.Check(err, check.IsNil) { + return + } + loc, err := url.Parse(resp.RedirectLocation) + if !c.Check(err, check.IsNil, check.Commentf("error parsing response URL")) { + return + } + + c.Check(loc.Scheme, check.Equals, "https") + c.Check(loc.Host, check.Equals, expURL.Host) + c.Check(loc.Path, check.Equals, expURL.Path) + + var expReturn string + switch returnTo { + case "": + expReturn = s.cluster.Services.Workbench2.ExternalURL.String() + default: + expReturn = returnTo + } + values := loc.Query() + c.Check(values.Get("client_id"), check.Equals, s.cluster.Login.Google.ClientID) + c.Check(values.Get("post_logout_redirect_uri"), check.Equals, expReturn) +} + +func (s *OIDCLoginSuite) TestRPInitiatedLogoutWithoutReturnTo(c *check.C) { + s.fakeProvider.EndSessionEndpoint = &url.URL{Path: "/logout/fromRP"} + s.checkRPInitiatedLogout(c, "") +} + +func (s *OIDCLoginSuite) TestRPInitiatedLogoutWithReturnTo(c *check.C) { + s.fakeProvider.EndSessionEndpoint = &url.URL{Path: "/rp_logout"} + u := arvados.URL{Scheme: "https", Host: "foo.example", Path: "/"} + s.cluster.Login.TrustedClients[u] = struct{}{} + s.checkRPInitiatedLogout(c, u.String()) +} + +func (s *OIDCLoginSuite) TestEndSessionEndpointBadScheme(c *check.C) { + // RP-Initiated Logout 1.0 says: "This URL MUST use the https scheme..." + u := url.URL{Scheme: "http", Host: "example.com"} + s.fakeProvider.EndSessionEndpoint = &u + _, err := s.localdb.Logout(s.ctx, arvados.LogoutOptions{}) + c.Check(err, check.ErrorMatches, + `.*\bend_session_endpoint MUST use HTTPS but does not: `+regexp.QuoteMeta(u.String())) +} + +func (s *OIDCLoginSuite) TestNoRPInitiatedLogoutWithoutToken(c *check.C) { + endPath := "/TestNoRPInitiatedLogoutWithoutToken" + s.fakeProvider.EndSessionEndpoint = &url.URL{Path: endPath} + resp, _ := s.localdb.Logout(s.ctx, arvados.LogoutOptions{}) + u, err := url.Parse(resp.RedirectLocation) + c.Check(err, check.IsNil) + c.Check(strings.HasSuffix(u.Path, endPath), check.Equals, false, + check.Commentf("logout redirected to end_session_endpoint without token")) } func (s *OIDCLoginSuite) TestGoogleLogin_Start_Bogus(c *check.C) { @@ -115,6 +192,13 @@ func (s *OIDCLoginSuite) TestGoogleLogin_Start(c *check.C) { } } +func (s *OIDCLoginSuite) TestGoogleLogin_UnknownClient(c *check.C) { + resp, err := s.localdb.Login(context.Background(), arvados.LoginOptions{ReturnTo: "https://bad-app.example.com/foo?bar"}) + c.Check(err, check.IsNil) + c.Check(resp.RedirectLocation, check.Equals, "") + c.Check(resp.HTML.String(), check.Matches, `(?ms).*requesting site is not listed in TrustedClients.*`) +} + func (s *OIDCLoginSuite) TestGoogleLogin_InvalidCode(c *check.C) { state := s.startLogin(c) resp, err := s.localdb.Login(context.Background(), arvados.LoginOptions{ @@ -165,12 +249,14 @@ func (s *OIDCLoginSuite) TestConfig(c *check.C) { s.cluster.Login.OpenIDConnect.Issuer = "https://accounts.example.com/" s.cluster.Login.OpenIDConnect.ClientID = "oidc-client-id" s.cluster.Login.OpenIDConnect.ClientSecret = "oidc-client-secret" - localdb := NewConn(s.cluster) + s.cluster.Login.OpenIDConnect.AuthenticationRequestParameters = map[string]string{"testkey": "testvalue"} + localdb := NewConn(context.Background(), s.cluster, (&ctrlctx.DBConnector{PostgreSQL: s.cluster.PostgreSQL}).GetDB) ctrl := localdb.loginController.(*oidcLoginController) c.Check(ctrl.Issuer, check.Equals, "https://accounts.example.com/") c.Check(ctrl.ClientID, check.Equals, "oidc-client-id") c.Check(ctrl.ClientSecret, check.Equals, "oidc-client-secret") c.Check(ctrl.UseGooglePeopleAPI, check.Equals, false) + c.Check(ctrl.AuthParams["testkey"], check.Equals, "testvalue") for _, enableAltEmails := range []bool{false, true} { s.cluster.Login.OpenIDConnect.Enable = false @@ -178,12 +264,14 @@ func (s *OIDCLoginSuite) TestConfig(c *check.C) { s.cluster.Login.Google.ClientID = "google-client-id" s.cluster.Login.Google.ClientSecret = "google-client-secret" s.cluster.Login.Google.AlternateEmailAddresses = enableAltEmails - localdb = NewConn(s.cluster) + s.cluster.Login.Google.AuthenticationRequestParameters = map[string]string{"testkey": "testvalue"} + localdb = NewConn(context.Background(), s.cluster, (&ctrlctx.DBConnector{PostgreSQL: s.cluster.PostgreSQL}).GetDB) ctrl = localdb.loginController.(*oidcLoginController) c.Check(ctrl.Issuer, check.Equals, "https://accounts.google.com") c.Check(ctrl.ClientID, check.Equals, "google-client-id") c.Check(ctrl.ClientSecret, check.Equals, "google-client-secret") c.Check(ctrl.UseGooglePeopleAPI, check.Equals, enableAltEmails) + c.Check(ctrl.AuthParams["testkey"], check.Equals, "testvalue") } } @@ -204,54 +292,179 @@ func (s *OIDCLoginSuite) TestOIDCAuthorizer(c *check.C) { json.Unmarshal([]byte(fmt.Sprintf("%q", s.fakeProvider.Issuer.URL)), &s.cluster.Login.OpenIDConnect.Issuer) s.cluster.Login.OpenIDConnect.ClientID = "oidc#client#id" s.cluster.Login.OpenIDConnect.ClientSecret = "oidc#client#secret" + s.cluster.Login.OpenIDConnect.AcceptAccessToken = true + s.cluster.Login.OpenIDConnect.AcceptAccessTokenScope = "" s.fakeProvider.ValidClientID = "oidc#client#id" s.fakeProvider.ValidClientSecret = "oidc#client#secret" db := arvadostest.DB(c, s.cluster) tokenCacheTTL = time.Millisecond tokenCacheRaceWindow = time.Millisecond + tokenCacheNegativeTTL = time.Millisecond oidcAuthorizer := OIDCAccessTokenAuthorizer(s.cluster, func(context.Context) (*sqlx.DB, error) { return db, nil }) accessToken := s.fakeProvider.ValidAccessToken() mac := hmac.New(sha256.New, []byte(s.cluster.SystemRootToken)) io.WriteString(mac, accessToken) - hmac := fmt.Sprintf("%x", mac.Sum(nil)) + apiToken := fmt.Sprintf("%x", mac.Sum(nil)) + checkTokenInDB := func() time.Time { + var exp time.Time + err := db.QueryRow(`select expires_at at time zone 'UTC' from api_client_authorizations where api_token=$1`, apiToken).Scan(&exp) + c.Check(err, check.IsNil) + c.Check(exp.Sub(time.Now()) > -time.Second, check.Equals, true) + c.Check(exp.Sub(time.Now()) < time.Second, check.Equals, true) + return exp + } cleanup := func() { - _, err := db.Exec(`delete from api_client_authorizations where api_token=$1`, hmac) + oidcAuthorizer.cache.Purge() + _, err := db.Exec(`delete from api_client_authorizations where api_token=$1`, apiToken) c.Check(err, check.IsNil) } cleanup() defer cleanup() - ctx := auth.NewContext(context.Background(), &auth.Credentials{Tokens: []string{accessToken}}) - var exp1 time.Time - oidcAuthorizer.WrapCalls(func(ctx context.Context, opts interface{}) (interface{}, error) { - creds, ok := auth.FromContext(ctx) - c.Assert(ok, check.Equals, true) - c.Assert(creds.Tokens, check.HasLen, 1) - c.Check(creds.Tokens[0], check.Equals, accessToken) + ctx := ctrlctx.NewWithToken(s.ctx, s.cluster, accessToken) + + // Check behavior on 5xx/network errors (don't cache) vs 4xx + // (do cache) + { + call := oidcAuthorizer.WrapCalls(func(ctx context.Context, opts interface{}) (interface{}, error) { + return nil, nil + }) - err := db.QueryRowContext(ctx, `select expires_at at time zone 'UTC' from api_client_authorizations where api_token=$1`, hmac).Scan(&exp1) + // If fakeProvider UserInfo endpoint returns 502, we + // should fail, return an error, and *not* cache the + // negative result. + tokenCacheNegativeTTL = time.Minute + s.fakeProvider.UserInfoErrorStatus = 502 + _, err := call(ctx, nil) + c.Check(err, check.NotNil) + + // The negative result was not cached, so retrying + // immediately (with UserInfo working now) should + // succeed. + s.fakeProvider.UserInfoErrorStatus = 0 + _, err = call(ctx, nil) c.Check(err, check.IsNil) - c.Check(exp1.Sub(time.Now()) > -time.Second, check.Equals, true) - c.Check(exp1.Sub(time.Now()) < time.Second, check.Equals, true) - return nil, nil - })(ctx, nil) + checkTokenInDB() + + cleanup() + + // UserInfo 401 => cache the negative result, but + // don't return an error (just pass the token through + // as a v1 token) + s.fakeProvider.UserInfoErrorStatus = 401 + _, err = call(ctx, nil) + c.Check(err, check.IsNil) + ent, ok := oidcAuthorizer.cache.Get(accessToken) + c.Check(ok, check.Equals, true) + c.Check(ent, check.FitsTypeOf, time.Time{}) + + // UserInfo succeeds now, but we still have a cached + // negative result. + s.fakeProvider.UserInfoErrorStatus = 0 + _, err = call(ctx, nil) + c.Check(err, check.IsNil) + ent, ok = oidcAuthorizer.cache.Get(accessToken) + c.Check(ok, check.Equals, true) + c.Check(ent, check.FitsTypeOf, time.Time{}) + + tokenCacheNegativeTTL = time.Millisecond + cleanup() + } + + var exp1 time.Time + concurrent := 4 + s.fakeProvider.HoldUserInfo = make(chan *http.Request) + s.fakeProvider.ReleaseUserInfo = make(chan struct{}) + go func() { + for i := 0; ; i++ { + if i == concurrent { + close(s.fakeProvider.ReleaseUserInfo) + } + <-s.fakeProvider.HoldUserInfo + } + }() + var wg sync.WaitGroup + for i := 0; i < concurrent; i++ { + i := i + wg.Add(1) + go func() { + defer wg.Done() + _, err := oidcAuthorizer.WrapCalls(func(ctx context.Context, opts interface{}) (interface{}, error) { + c.Logf("concurrent req %d/%d", i, concurrent) + + creds, ok := auth.FromContext(ctx) + c.Assert(ok, check.Equals, true) + c.Assert(creds.Tokens, check.HasLen, 1) + c.Check(creds.Tokens[0], check.Equals, accessToken) + exp := checkTokenInDB() + if i == 0 { + exp1 = exp + } + return nil, nil + })(ctx, nil) + c.Check(err, check.IsNil) + }() + } + wg.Wait() + if c.Failed() { + c.Fatal("giving up") + } // If the token is used again after the in-memory cache - // expires, oidcAuthorizer must re-checks the token and update + // expires, oidcAuthorizer must re-check the token and update // the expires_at value in the database. time.Sleep(3 * time.Millisecond) oidcAuthorizer.WrapCalls(func(ctx context.Context, opts interface{}) (interface{}, error) { - var exp time.Time - err := db.QueryRowContext(ctx, `select expires_at at time zone 'UTC' from api_client_authorizations where api_token=$1`, hmac).Scan(&exp) - c.Check(err, check.IsNil) - c.Check(exp.Sub(exp1) > 0, check.Equals, true) - c.Check(exp.Sub(exp1) < time.Second, check.Equals, true) + exp := checkTokenInDB() + c.Check(exp.Sub(exp1) > 0, check.Equals, true, check.Commentf("expect %v > 0", exp.Sub(exp1))) + c.Check(exp.Sub(exp1) < time.Second, check.Equals, true, check.Commentf("expect %v < 1s", exp.Sub(exp1))) return nil, nil })(ctx, nil) + + s.fakeProvider.AccessTokenPayload = map[string]interface{}{"scope": "openid profile foobar"} + accessToken = s.fakeProvider.ValidAccessToken() + ctx = ctrlctx.NewWithToken(s.ctx, s.cluster, accessToken) + + mac = hmac.New(sha256.New, []byte(s.cluster.SystemRootToken)) + io.WriteString(mac, accessToken) + apiToken = fmt.Sprintf("%x", mac.Sum(nil)) + + for _, trial := range []struct { + configEnable bool + configScope string + acceptable bool + shouldRun bool + }{ + {true, "foobar", true, true}, + {true, "foo", false, false}, + {true, "", true, true}, + {false, "", false, true}, + {false, "foobar", false, true}, + } { + c.Logf("trial = %+v", trial) + cleanup() + s.cluster.Login.OpenIDConnect.AcceptAccessToken = trial.configEnable + s.cluster.Login.OpenIDConnect.AcceptAccessTokenScope = trial.configScope + oidcAuthorizer = OIDCAccessTokenAuthorizer(s.cluster, func(context.Context) (*sqlx.DB, error) { return db, nil }) + checked := false + oidcAuthorizer.WrapCalls(func(ctx context.Context, opts interface{}) (interface{}, error) { + var n int + err := db.QueryRowContext(ctx, `select count(*) from api_client_authorizations where api_token=$1`, apiToken).Scan(&n) + c.Check(err, check.IsNil) + if trial.acceptable { + c.Check(n, check.Equals, 1) + } else { + c.Check(n, check.Equals, 0) + } + checked = true + return nil, nil + })(ctx, nil) + c.Check(checked, check.Equals, trial.shouldRun) + } } func (s *OIDCLoginSuite) TestGenericOIDCLogin(c *check.C) { @@ -260,6 +473,7 @@ func (s *OIDCLoginSuite) TestGenericOIDCLogin(c *check.C) { json.Unmarshal([]byte(fmt.Sprintf("%q", s.fakeProvider.Issuer.URL)), &s.cluster.Login.OpenIDConnect.Issuer) s.cluster.Login.OpenIDConnect.ClientID = "oidc#client#id" s.cluster.Login.OpenIDConnect.ClientSecret = "oidc#client#secret" + s.cluster.Login.OpenIDConnect.AuthenticationRequestParameters = map[string]string{"testkey": "testvalue"} s.fakeProvider.ValidClientID = "oidc#client#id" s.fakeProvider.ValidClientSecret = "oidc#client#secret" for _, trial := range []struct { @@ -316,10 +530,12 @@ func (s *OIDCLoginSuite) TestGenericOIDCLogin(c *check.C) { s.railsSpy.Close() } s.railsSpy = arvadostest.NewProxy(c, s.cluster.Services.RailsAPI) - s.localdb = NewConn(s.cluster) + s.localdb = NewConn(context.Background(), s.cluster, (&ctrlctx.DBConnector{PostgreSQL: s.cluster.PostgreSQL}).GetDB) *s.localdb.railsProxy = *rpc.NewConn(s.cluster.ClusterID, s.railsSpy.URL, true, rpc.PassthroughTokenProvider) - state := s.startLogin(c) + state := s.startLogin(c, func(form url.Values) { + c.Check(form.Get("testkey"), check.Equals, "testvalue") + }) resp, err := s.localdb.Login(context.Background(), arvados.LoginOptions{ Code: s.fakeProvider.ValidCode, State: state, @@ -350,7 +566,12 @@ func (s *OIDCLoginSuite) TestGenericOIDCLogin(c *check.C) { } func (s *OIDCLoginSuite) TestGoogleLogin_Success(c *check.C) { - state := s.startLogin(c) + s.cluster.Login.Google.AuthenticationRequestParameters["prompt"] = "consent" + s.cluster.Login.Google.AuthenticationRequestParameters["foo"] = "bar" + state := s.startLogin(c, func(form url.Values) { + c.Check(form.Get("foo"), check.Equals, "bar") + c.Check(form.Get("prompt"), check.Equals, "consent") + }) resp, err := s.localdb.Login(context.Background(), arvados.LoginOptions{ Code: s.fakeProvider.ValidCode, State: state, @@ -365,14 +586,14 @@ func (s *OIDCLoginSuite) TestGoogleLogin_Success(c *check.C) { c.Check(token, check.Matches, `v2/zzzzz-gj3su-.{15}/.{32,50}`) authinfo := getCallbackAuthInfo(c, s.railsSpy) - c.Check(authinfo.FirstName, check.Equals, "Fake User") - c.Check(authinfo.LastName, check.Equals, "Name") + c.Check(authinfo.FirstName, check.Equals, "Fake") + c.Check(authinfo.LastName, check.Equals, "User Name") c.Check(authinfo.Email, check.Equals, "active-user@arvados.local") c.Check(authinfo.AlternateEmails, check.HasLen, 0) // Try using the returned Arvados token. c.Logf("trying an API call with new token %q", token) - ctx := auth.NewContext(context.Background(), &auth.Credentials{Tokens: []string{token}}) + ctx := ctrlctx.NewWithToken(s.ctx, s.cluster, token) cl, err := s.localdb.CollectionList(ctx, arvados.ListOptions{Limit: -1}) c.Check(cl.ItemsAvailable, check.Not(check.Equals), 0) c.Check(cl.Items, check.Not(check.HasLen), 0) @@ -381,7 +602,7 @@ func (s *OIDCLoginSuite) TestGoogleLogin_Success(c *check.C) { // Might as well check that bogus tokens aren't accepted. badtoken := token + "plussomeboguschars" c.Logf("trying an API call with mangled token %q", badtoken) - ctx = auth.NewContext(context.Background(), &auth.Credentials{Tokens: []string{badtoken}}) + ctx = ctrlctx.NewWithToken(s.ctx, s.cluster, badtoken) cl, err = s.localdb.CollectionList(ctx, arvados.ListOptions{Limit: -1}) c.Check(cl.Items, check.HasLen, 0) c.Check(err, check.NotNil) @@ -390,6 +611,7 @@ func (s *OIDCLoginSuite) TestGoogleLogin_Success(c *check.C) { func (s *OIDCLoginSuite) TestGoogleLogin_RealName(c *check.C) { s.fakeProvider.AuthEmail = "joe.smith@primary.example.com" + s.fakeProvider.AuthEmailVerified = true s.fakeProvider.PeopleAPIResponse = map[string]interface{}{ "names": []map[string]interface{}{ { @@ -415,8 +637,10 @@ func (s *OIDCLoginSuite) TestGoogleLogin_RealName(c *check.C) { c.Check(authinfo.LastName, check.Equals, "Psmith") } -func (s *OIDCLoginSuite) TestGoogleLogin_OIDCRealName(c *check.C) { +func (s *OIDCLoginSuite) TestGoogleLogin_OIDCNameWithoutGivenAndFamilyNames(c *check.C) { s.fakeProvider.AuthName = "Joe P. Smith" + s.fakeProvider.AuthGivenName = "" + s.fakeProvider.AuthFamilyName = "" s.fakeProvider.AuthEmail = "joe.smith@primary.example.com" state := s.startLogin(c) s.localdb.Login(context.Background(), arvados.LoginOptions{ @@ -515,18 +739,76 @@ func (s *OIDCLoginSuite) TestGoogleLogin_NoPrimaryEmailAddress(c *check.C) { c.Check(authinfo.Username, check.Equals, "") } -func (s *OIDCLoginSuite) startLogin(c *check.C) (state string) { +func (s *OIDCLoginSuite) startLogin(c *check.C, checks ...func(url.Values)) (state string) { // Initiate login, but instead of following the redirect to // the provider, just grab state from the redirect URL. resp, err := s.localdb.Login(context.Background(), arvados.LoginOptions{ReturnTo: "https://app.example.com/foo?bar"}) c.Check(err, check.IsNil) + c.Check(resp.HTML.String(), check.Not(check.Matches), `(?ms).*error:.*`) target, err := url.Parse(resp.RedirectLocation) c.Check(err, check.IsNil) state = target.Query().Get("state") - c.Check(state, check.Not(check.Equals), "") + if !c.Check(state, check.Not(check.Equals), "") { + c.Logf("Redirect target: %q", target) + c.Logf("HTML: %q", resp.HTML) + } + for _, fn := range checks { + fn(target.Query()) + } + s.cluster.Login.OpenIDConnect.AuthenticationRequestParameters = map[string]string{"testkey": "testvalue"} return } +func (s *OIDCLoginSuite) TestValidateLoginRedirectTarget(c *check.C) { + for _, trial := range []struct { + permit bool + trustPrivate bool + url string + }{ + // wb1, wb2 => accept + {true, false, s.cluster.Services.Workbench1.ExternalURL.String()}, + {true, false, s.cluster.Services.Workbench2.ExternalURL.String()}, + // explicitly listed host => accept + {true, false, "https://app.example.com/"}, + {true, false, "https://app.example.com:443/foo?bar=baz"}, + // non-listed hostname => deny (regardless of TrustPrivateNetworks) + {false, false, "https://bad.example/"}, + {false, true, "https://bad.example/"}, + // non-listed non-private IP addr => deny (regardless of TrustPrivateNetworks) + {false, true, "https://1.2.3.4/"}, + {false, true, "https://1.2.3.4/"}, + {false, true, "https://[ab::cd]:1234/"}, + // localhost or non-listed private IP addr => accept only if TrustPrivateNetworks is set + {false, false, "https://localhost/"}, + {true, true, "https://localhost/"}, + {false, false, "https://[10.9.8.7]:80/foo"}, + {true, true, "https://[10.9.8.7]:80/foo"}, + {false, false, "https://[::1]:80/foo"}, + {true, true, "https://[::1]:80/foo"}, + {true, true, "http://192.168.1.1/"}, + {true, true, "http://172.17.2.0/"}, + // bad url => deny + {false, true, "https://10.1.1.1:blorp/foo"}, // non-numeric port + {false, true, "https://app.example.com:blorp/foo"}, // non-numeric port + {false, true, "https://]:443"}, + {false, true, "https://"}, + {false, true, "https:"}, + {false, true, ""}, + // explicitly listed host but different port, protocol, or user/pass => deny + {false, true, "http://app.example.com/"}, + {false, true, "http://app.example.com:443/"}, + {false, true, "https://app.example.com:80/"}, + {false, true, "https://app.example.com:4433/"}, + {false, true, "https://u:p@app.example.com:443/foo?bar=baz"}, + } { + c.Logf("trial %+v", trial) + s.cluster.Login.TrustPrivateNetworks = trial.trustPrivate + err := validateLoginRedirectTarget(s.cluster, trial.url) + c.Check(err == nil, check.Equals, trial.permit) + } + +} + func getCallbackAuthInfo(c *check.C, railsSpy *arvadostest.Proxy) (authinfo rpc.UserSessionAuthInfo) { for _, dump := range railsSpy.RequestDumps { c.Logf("spied request: %q", dump)