19240: Restrict login/logout redirects to listed URLs.
authorTom Clegg <tom@curii.com>
Fri, 4 Nov 2022 16:43:50 +0000 (12:43 -0400)
committerTom Clegg <tom@curii.com>
Fri, 4 Nov 2022 16:43:50 +0000 (12:43 -0400)
Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom@curii.com>

lib/config/config.default.yml
lib/config/export.go
lib/controller/federation/login_test.go
lib/controller/handler_test.go
lib/controller/localdb/conn.go
lib/controller/localdb/login.go
lib/controller/localdb/login_oidc.go
lib/controller/localdb/login_oidc_test.go
lib/controller/localdb/login_testuser_test.go
lib/controller/localdb/logout.go
sdk/go/arvados/config.go

index 5e46c290da6424f15870ac4bb6cf03731e21e338..fd91442dbf5b5ecb03688b20b0424713d50e3f7a 100644 (file)
@@ -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
index e7cf094eb02b63f8a613b881696940e2cec505ab..6352406e90e95dc255d9eb43ff1ca13f0e721fd1 100644 (file)
@@ -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 {
index c05ebfce69820b3be781a3d18be8a591aaa94eb2..e1114bf7eb21fd6752598ee7f31fe08199f9ef74 100644 (file)
@@ -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
index a240b195ea9ff583eede71720ab8c9dfdf3c3798..1af3ba3626c94dc517fd657270a24ed067eaace5 100644 (file)
@@ -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) {
index 5a3faa72790899aa69ba4408fbf47df7997c27af..5b6964de00d105ec89938e3c2f4e556688fd4722 100644 (file)
@@ -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)
 }
index 2b20491a04a426f50dbb354b9c8e0a7e86f833ea..866db086691ae6821eba0bca234b45939e78b036 100644 (file)
@@ -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")
+}
index 6d6f80f39c70ac5427578ddd6ed5eb3e78b6a136..05e5e243b99d574fa4956e41cfcaee8c24cbe5ab 100644 (file)
@@ -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 {
index b9f0f56e058482eb74eb527b038136e56979feff..49629bb222c0ab0b11dcfd3007603cbda6120977 100644 (file)
@@ -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)
index 51c2416f59bcb7c2c7f55e9425489ae003653ee5..8717617889bcd8d8fbf5a3e8e43c4d6852834a70 100644 (file)
@@ -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
index e1603f14485eb0a4f664a2a00b080a1497c64d2a..04e7681ad7bef728bb11e5c745c2b8391094b2d5 100644 (file)
@@ -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
 }
index 64b7fab8d2267b4f9d4f96cbdad15954965c58db..2871356e9827059352026432082c5fcdee2f3fce 100644 (file)
@@ -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 {