From: Tom Clegg Date: Fri, 4 Nov 2022 16:43:50 +0000 (-0400) Subject: 19240: Restrict login/logout redirects to listed URLs. X-Git-Tag: 2.5.0~44^2~4 X-Git-Url: https://git.arvados.org/arvados.git/commitdiff_plain/578c505d74e0e4daf680c5c39fb1619bb073a592 19240: Restrict login/logout redirects to listed URLs. Arvados-DCO-1.1-Signed-off-by: Tom Clegg --- diff --git a/lib/config/config.default.yml b/lib/config/config.default.yml index 5e46c290da..fd91442dbf 100644 --- a/lib/config/config.default.yml +++ b/lib/config/config.default.yml @@ -878,16 +878,28 @@ Clusters: # by going through login again. IssueTrustedTokens: true - # When the token is returned to a client, the token itself may - # be restricted from viewing/creating other tokens based on whether - # the client is "trusted" or not. The local Workbench1 and - # Workbench2 are trusted by default, but if this is a - # LoginCluster, you probably want to include the other Workbench - # instances in the federation in this list. + # Origins (scheme://host[:port]) of clients trusted to receive + # new tokens via login process. The ExternalURLs of the local + # Workbench1 and Workbench2 are trusted implicitly and do not + # need to be listed here. If this is a LoginCluster, you + # probably want to include the other Workbench instances in the + # federation in this list. + # + # Example: + # + # TrustedClients: + # "https://workbench.other-cluster.example": {} + # "https://workbench2.other-cluster.example": {} TrustedClients: - SAMPLE: - "https://workbench.federate1.example": {} - "https://workbench.federate2.example": {} + SAMPLE: {} + + # Treat any origin whose host part is a private IP address + # (e.g., http://10.0.0.123/) as if it were listed in + # TrustedClients. + # + # Intended only for test/development use. Not appropriate for + # production use. + TrustPrivateNetworks: false Git: # Path to git or gitolite-shell executable. Each authenticated diff --git a/lib/config/export.go b/lib/config/export.go index e7cf094eb0..6352406e90 100644 --- a/lib/config/export.go +++ b/lib/config/export.go @@ -193,6 +193,7 @@ var whitelist = map[string]bool{ "Login.Test.Users": false, "Login.TokenLifetime": false, "Login.TrustedClients": false, + "Login.TrustPrivateNetworks": false, "Mail": true, "Mail.EmailFrom": false, "Mail.IssueReporterEmailFrom": false, @@ -264,6 +265,7 @@ var whitelist = map[string]bool{ "Workbench.ApplicationMimetypesWithViewIcon.*": true, "Workbench.ArvadosDocsite": true, "Workbench.ArvadosPublicDataDocURL": true, + "Workbench.BannerURL": true, "Workbench.DefaultOpenIdPrefix": false, "Workbench.DisableSharingURLsUI": true, "Workbench.EnableGettingStartedPopup": true, @@ -291,7 +293,6 @@ var whitelist = map[string]bool{ "Workbench.UserProfileFormFields.*.*.*": true, "Workbench.UserProfileFormMessage": true, "Workbench.WelcomePageHTML": true, - "Workbench.BannerURL": true, } func redactUnsafe(m map[string]interface{}, mPrefix, lookupPrefix string) error { diff --git a/lib/controller/federation/login_test.go b/lib/controller/federation/login_test.go index c05ebfce69..e1114bf7eb 100644 --- a/lib/controller/federation/login_test.go +++ b/lib/controller/federation/login_test.go @@ -41,25 +41,27 @@ func (s *LoginSuite) TestDeferToLoginCluster(c *check.C) { } func (s *LoginSuite) TestLogout(c *check.C) { + otherOrigin := arvados.URL{Scheme: "https", Host: "app.example.com", Path: "/"} + otherURL := "https://app.example.com/foo" s.cluster.Services.Workbench1.ExternalURL = arvados.URL{Scheme: "https", Host: "workbench1.example.com"} s.cluster.Services.Workbench2.ExternalURL = arvados.URL{Scheme: "https", Host: "workbench2.example.com"} + s.cluster.Login.TrustedClients = map[arvados.URL]struct{}{otherOrigin: {}} s.addHTTPRemote(c, "zhome", &arvadostest.APIStub{}) s.cluster.Login.LoginCluster = "zhome" // s.fed is already set by SetUpTest, but we need to // reinitialize with the above config changes. s.fed = New(s.cluster, nil) - returnTo := "https://app.example.com/foo?bar" for _, trial := range []struct { token string returnTo string target string }{ {token: "", returnTo: "", target: s.cluster.Services.Workbench2.ExternalURL.String()}, - {token: "", returnTo: returnTo, target: returnTo}, - {token: "zzzzzzzzzzzzzzzzzzzzz", returnTo: returnTo, target: returnTo}, - {token: "v2/zzzzz-aaaaa-aaaaaaaaaaaaaaa/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", returnTo: returnTo, target: returnTo}, - {token: "v2/zhome-aaaaa-aaaaaaaaaaaaaaa/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", returnTo: returnTo, target: "http://" + s.cluster.RemoteClusters["zhome"].Host + "/logout?" + url.Values{"return_to": {returnTo}}.Encode()}, + {token: "", returnTo: otherURL, target: otherURL}, + {token: "zzzzzzzzzzzzzzzzzzzzz", returnTo: otherURL, target: otherURL}, + {token: "v2/zzzzz-aaaaa-aaaaaaaaaaaaaaa/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", returnTo: otherURL, target: otherURL}, + {token: "v2/zhome-aaaaa-aaaaaaaaaaaaaaa/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", returnTo: otherURL, target: "http://" + s.cluster.RemoteClusters["zhome"].Host + "/logout?" + url.Values{"return_to": {otherURL}}.Encode()}, } { c.Logf("trial %#v", trial) ctx := s.ctx diff --git a/lib/controller/handler_test.go b/lib/controller/handler_test.go index a240b195ea..1af3ba3626 100644 --- a/lib/controller/handler_test.go +++ b/lib/controller/handler_test.go @@ -272,15 +272,16 @@ func (s *HandlerSuite) TestProxyNotFound(c *check.C) { } func (s *HandlerSuite) TestLogoutGoogle(c *check.C) { + s.cluster.Services.Workbench2.ExternalURL = arvados.URL{Scheme: "https", Host: "wb2.example", Path: "/"} s.cluster.Login.Google.Enable = true s.cluster.Login.Google.ClientID = "test" - req := httptest.NewRequest("GET", "https://0.0.0.0:1/logout?return_to=https://example.com/foo", nil) + req := httptest.NewRequest("GET", "https://0.0.0.0:1/logout?return_to=https://wb2.example/", nil) resp := httptest.NewRecorder() s.handler.ServeHTTP(resp, req) if !c.Check(resp.Code, check.Equals, http.StatusFound) { c.Log(resp.Body.String()) } - c.Check(resp.Header().Get("Location"), check.Equals, "https://example.com/foo") + c.Check(resp.Header().Get("Location"), check.Equals, "https://wb2.example/") } func (s *HandlerSuite) TestValidateV1APIToken(c *check.C) { diff --git a/lib/controller/localdb/conn.go b/lib/controller/localdb/conn.go index 5a3faa7279..5b6964de00 100644 --- a/lib/controller/localdb/conn.go +++ b/lib/controller/localdb/conn.go @@ -8,6 +8,7 @@ import ( "context" "encoding/json" "fmt" + "net" "net/http" "os" "sync" @@ -165,6 +166,26 @@ func (conn *Conn) UserAuthenticate(ctx context.Context, opts arvados.UserAuthent return conn.loginController.UserAuthenticate(ctx, opts) } +var privateNetworks = func() (nets []*net.IPNet) { + for _, s := range []string{ + "127.0.0.0/8", + "10.0.0.0/8", + "172.16.0.0/12", + "192.168.0.0/16", + "169.254.0.0/16", + "::1/128", + "fe80::/10", + "fc00::/7", + } { + _, n, err := net.ParseCIDR(s) + if err != nil { + panic(fmt.Sprintf("privateNetworks: %q: %s", s, err)) + } + nets = append(nets, n) + } + return +}() + func httpErrorf(code int, format string, args ...interface{}) error { return httpserver.ErrorWithStatus(fmt.Errorf(format, args...), code) } diff --git a/lib/controller/localdb/login.go b/lib/controller/localdb/login.go index 2b20491a04..866db08669 100644 --- a/lib/controller/localdb/login.go +++ b/lib/controller/localdb/login.go @@ -10,6 +10,7 @@ import ( "encoding/json" "errors" "fmt" + "net" "net/http" "net/url" "strings" @@ -162,3 +163,36 @@ func (conn *Conn) CreateAPIClientAuthorization(ctx context.Context, rootToken st } return } + +func validateLoginRedirectTarget(cluster *arvados.Cluster, returnTo string) error { + u, err := url.Parse(returnTo) + if err != nil { + return err + } + u, err = u.Parse("/") + if err != nil { + return err + } + if u.Port() == "80" && u.Scheme == "http" { + u.Host = u.Hostname() + } else if u.Port() == "443" && u.Scheme == "https" { + u.Host = u.Hostname() + } + if _, ok := cluster.Login.TrustedClients[arvados.URL(*u)]; ok { + return nil + } + if u.String() == cluster.Services.Workbench1.ExternalURL.String() || + u.String() == cluster.Services.Workbench2.ExternalURL.String() { + return nil + } + if cluster.Login.TrustPrivateNetworks { + if ip := net.ParseIP(u.Hostname()); len(ip) > 0 { + for _, n := range privateNetworks { + if n.Contains(ip) { + return nil + } + } + } + } + return fmt.Errorf("requesting site is not listed in TrustedClients config") +} diff --git a/lib/controller/localdb/login_oidc.go b/lib/controller/localdb/login_oidc.go index 6d6f80f39c..05e5e243b9 100644 --- a/lib/controller/localdb/login_oidc.go +++ b/lib/controller/localdb/login_oidc.go @@ -116,6 +116,9 @@ func (ctrl *oidcLoginController) Login(ctx context.Context, opts arvados.LoginOp if opts.ReturnTo == "" { return loginError(errors.New("missing return_to parameter")) } + if err := validateLoginRedirectTarget(ctrl.Parent.cluster, opts.ReturnTo); err != nil { + return loginError(fmt.Errorf("invalid return_to parameter: %s", err)) + } state := ctrl.newOAuth2State([]byte(ctrl.Cluster.SystemRootToken), opts.Remote, opts.ReturnTo) var authparams []oauth2.AuthCodeOption for k, v := range ctrl.AuthParams { diff --git a/lib/controller/localdb/login_oidc_test.go b/lib/controller/localdb/login_oidc_test.go index b9f0f56e05..49629bb222 100644 --- a/lib/controller/localdb/login_oidc_test.go +++ b/lib/controller/localdb/login_oidc_test.go @@ -42,6 +42,7 @@ type OIDCLoginSuite struct { cluster *arvados.Cluster localdb *Conn railsSpy *arvadostest.Proxy + trustedURL *arvados.URL fakeProvider *arvadostest.OIDCProvider } @@ -53,6 +54,8 @@ func (s *OIDCLoginSuite) TearDownSuite(c *check.C) { } func (s *OIDCLoginSuite) SetUpTest(c *check.C) { + s.trustedURL = &arvados.URL{Scheme: "https", Host: "app.example.com", Path: "/"} + s.fakeProvider = arvadostest.NewOIDCProvider(c) s.fakeProvider.AuthEmail = "active-user@arvados.local" s.fakeProvider.AuthEmailVerified = true @@ -70,6 +73,7 @@ func (s *OIDCLoginSuite) SetUpTest(c *check.C) { 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" @@ -88,9 +92,26 @@ func (s *OIDCLoginSuite) TearDownTest(c *check.C) { } 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) TestGoogleLogin_Start_Bogus(c *check.C) { @@ -118,6 +139,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{ @@ -613,10 +641,14 @@ func (s *OIDCLoginSuite) startLogin(c *check.C, checks ...func(url.Values)) (sta // 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()) } @@ -624,6 +656,55 @@ func (s *OIDCLoginSuite) startLogin(c *check.C, checks ...func(url.Values)) (sta 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://localhost/"}, + {false, true, "https://localhost/"}, + {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/"}, + // non-listed private IP addr => accept only if TrustPrivateNetworks is set + {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) diff --git a/lib/controller/localdb/login_testuser_test.go b/lib/controller/localdb/login_testuser_test.go index 51c2416f59..8717617889 100644 --- a/lib/controller/localdb/login_testuser_test.go +++ b/lib/controller/localdb/login_testuser_test.go @@ -103,7 +103,8 @@ func (s *TestUserSuite) TestLoginForm(c *check.C) { } func (s *TestUserSuite) TestExpireTokenOnLogout(c *check.C) { - returnTo := "https://localhost:12345/logout" + s.cluster.Login.TrustPrivateNetworks = true + returnTo := "https://[::1]:12345/logout" for _, trial := range []struct { requestToken string expiringTokenUUID string diff --git a/lib/controller/localdb/logout.go b/lib/controller/localdb/logout.go index e1603f1448..04e7681ad7 100644 --- a/lib/controller/localdb/logout.go +++ b/lib/controller/localdb/logout.go @@ -33,6 +33,8 @@ func logout(ctx context.Context, cluster *arvados.Cluster, opts arvados.LogoutOp } else { target = cluster.Services.Workbench1.ExternalURL.String() } + } else if err := validateLoginRedirectTarget(cluster, target); err != nil { + return arvados.LogoutResponse{}, httpserver.ErrorWithStatus(fmt.Errorf("invalid return_to parameter: %s", err), http.StatusBadRequest) } return arvados.LogoutResponse{RedirectLocation: target}, nil } diff --git a/sdk/go/arvados/config.go b/sdk/go/arvados/config.go index 64b7fab8d2..2871356e98 100644 --- a/sdk/go/arvados/config.go +++ b/sdk/go/arvados/config.go @@ -200,11 +200,12 @@ type Cluster struct { Enable bool Users map[string]TestUser } - LoginCluster string - RemoteTokenRefresh Duration - TokenLifetime Duration - TrustedClients map[string]struct{} - IssueTrustedTokens bool + LoginCluster string + RemoteTokenRefresh Duration + TokenLifetime Duration + TrustedClients map[URL]struct{} + TrustPrivateNetworks bool + IssueTrustedTokens bool } Mail struct { MailchimpAPIKey string @@ -395,7 +396,7 @@ func (su *URL) UnmarshalText(text []byte) error { } func (su URL) MarshalText() ([]byte, error) { - return []byte(fmt.Sprintf("%s", (*url.URL)(&su).String())), nil + return []byte(su.String()), nil } func (su URL) String() string {