[arvados] created: 2.5.0-307-gbdde690c4
git repository hosting
git at public.arvados.org
Mon Mar 27 18:30:04 UTC 2023
at bdde690c4479ae294707a65f4f4d259427611d70 (commit)
commit bdde690c4479ae294707a65f4f4d259427611d70
Author: Tom Clegg <tom at curii.com>
Date: Mon Mar 27 14:29:38 2023 -0400
20140: Accept wildcards in TrustedClients.
Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom at curii.com>
diff --git a/lib/config/config.default.yml b/lib/config/config.default.yml
index 1919d7b70..5684723a2 100644
--- a/lib/config/config.default.yml
+++ b/lib/config/config.default.yml
@@ -909,6 +909,9 @@ Clusters:
# probably want to include the other Workbench instances in the
# federation in this list.
#
+ # A wildcard like "https://*.example" will match client URLs
+ # like "https://a.example" and "https://a.b.c.example".
+ #
# Example:
#
# TrustedClients:
diff --git a/lib/controller/localdb/login.go b/lib/controller/localdb/login.go
index 041336bca..18537b202 100644
--- a/lib/controller/localdb/login.go
+++ b/lib/controller/localdb/login.go
@@ -175,7 +175,17 @@ func validateLoginRedirectTarget(cluster *arvados.Cluster, returnTo string) erro
}
target := origin(*u)
for trusted := range cluster.Login.TrustedClients {
- if origin(url.URL(trusted)) == target {
+ trustedOrigin := origin(url.URL(trusted))
+ if trustedOrigin == target {
+ return nil
+ }
+ // If TrustedClients has https://*.bar.example, we
+ // trust https://foo.bar.example. Note origin() has
+ // already stripped the incoming Path, so we won't
+ // accidentally trust
+ // https://attacker.example/pwn.bar.example here. See
+ // tests.
+ if strings.HasPrefix(trustedOrigin, u.Scheme+"://*.") && strings.HasSuffix(target, trustedOrigin[len(u.Scheme)+4:]) {
return nil
}
}
diff --git a/lib/controller/localdb/login_test.go b/lib/controller/localdb/login_test.go
index edd558a55..5c8e92862 100644
--- a/lib/controller/localdb/login_test.go
+++ b/lib/controller/localdb/login_test.go
@@ -35,6 +35,27 @@ func (s *loginSuite) TestValidateLoginRedirectTarget(c *check.C) {
{true, "https://wb1.example/", "https://wb2.example/", "https://good.wb2.example/", "https://good.wb2.example"},
{true, "https://wb1.example/", "https://wb2.example/", "https://good.wb2.example:443/", "https://good.wb2.example"},
{true, "https://wb1.example/", "https://wb2.example/", "https://good.wb2.example:443", "https://good.wb2.example/"},
+
+ {true, "https://wb1.example/", "https://wb2.example/", "https://*.wildcard.example", "https://ok.wildcard.example/"},
+ {true, "https://wb1.example/", "https://wb2.example/", "https://*.wildcard.example", "https://ok.ok.wildcard.example/"},
+ {true, "https://wb1.example/", "https://wb2.example/", "https://*.wildcard.example", "https://[ok.ok.wildcard.example]:443/"},
+ {true, "https://wb1.example/", "https://wb2.example/", "https://[*.wildcard.example]:443", "https://ok.ok.wildcard.example/"},
+ {true, "https://wb1.example/", "https://wb2.example/", "https://*.wildcard.example:443", "https://ok.wildcard.example/"},
+ {true, "https://wb1.example/", "https://wb2.example/", "https://*.wildcard.example", "https://ok.wildcard.example:443/"},
+ {true, "https://wb1.example/", "https://wb2.example/", "https://*.wildcard.example:443", "https://ok.wildcard.example:443/"},
+
+ {false, "https://wb1.example/", "https://wb2.example/", "https://*.wildcard.example", "http://wildcard.example/"},
+ {false, "https://wb1.example/", "https://wb2.example/", "https://*.wildcard.example", "http://.wildcard.example/"},
+ {false, "https://wb1.example/", "https://wb2.example/", "https://*.wildcard.example", "http://wrongscheme.wildcard.example/"},
+ {false, "https://wb1.example/", "https://wb2.example/", "https://*.wildcard.example", "http://wrongscheme.wildcard.example:443/"},
+ {false, "https://wb1.example/", "https://wb2.example/", "https://*.wildcard.example", "https://wrongport.wildcard.example:80/"},
+ {false, "https://wb1.example/", "https://wb2.example/", "https://*.wildcard.example", "https://notmatching-wildcard.example/"},
+ {false, "https://wb1.example/", "https://wb2.example/", "https://*.wildcard.example", "http://notmatching.wildcard.example/"},
+ {false, "https://wb1.example/", "https://wb2.example/", "https://*.wildcard.example:443", "https://attacker.example/ok.wildcard.example/"},
+ {false, "https://wb1.example/", "https://wb2.example/", "https://*.wildcard.example", "https://attacker.example/ok.wildcard.example/"},
+ {false, "https://wb1.example/", "https://wb2.example/", "https://*.wildcard.example", "https://attacker.example/?https://ok.wildcard.example/"},
+ {false, "https://wb1.example/", "https://wb2.example/", "https://*.wildcard.example", "https://attacker.example/#https://ok.wildcard.example/"},
+ {false, "https://wb1.example/", "https://wb2.example/", "https://*-wildcard.example", "https://notsupported-wildcard.example/"},
} {
c.Logf("trial %+v", trial)
// We use json.Unmarshal() to load the test strings
diff --git a/services/api/app/models/api_client.rb b/services/api/app/models/api_client.rb
index 55a4c6706..791b97168 100644
--- a/services/api/app/models/api_client.rb
+++ b/services/api/app/models/api_client.rb
@@ -32,7 +32,13 @@ class ApiClient < ArvadosModel
end
Rails.configuration.Login.TrustedClients.keys.each do |url|
- if norm_url_prefix == norm(url)
+ trusted = norm(url)
+ if norm_url_prefix == trusted
+ return true
+ end
+ if trusted.host.to_s.starts_with?("*.") &&
+ norm_url_prefix.to_s.starts_with?(trusted.scheme + "://") &&
+ norm_url_prefix.to_s.ends_with?(trusted.to_s[trusted.scheme.length + 4...])
return true
end
end
@@ -49,6 +55,8 @@ class ApiClient < ArvadosModel
url.port = "80"
end
url.path = "/"
+ url.query = nil
+ url.fragment = nil
url
end
end
diff --git a/services/api/test/unit/api_client_test.rb b/services/api/test/unit/api_client_test.rb
index a0eacfd13..dbe9c8636 100644
--- a/services/api/test/unit/api_client_test.rb
+++ b/services/api/test/unit/api_client_test.rb
@@ -40,4 +40,31 @@ class ApiClientTest < ActiveSupport::TestCase
end
end
end
+
+ [
+ [true, "https://ok.example", "https://ok.example"],
+ [true, "https://ok.example:443/", "https://ok.example"],
+ [true, "https://ok.example", "https://ok.example:443/"],
+ [true, "https://ok.example", "https://ok.example/foo/bar"],
+ [true, "https://ok.example", "https://ok.example?foo/bar"],
+ [true, "https://ok.example/waz?quux", "https://ok.example/foo?bar#baz"],
+ [false, "https://ok.example", "http://ok.example"],
+ [false, "https://ok.example", "http://ok.example:443"],
+
+ [true, "https://*.wildcard.example", "https://ok.wildcard.example"],
+ [true, "https://*.wildcard.example", "https://ok.ok.ok.wildcard.example"],
+ [false, "https://*.wildcard.example", "http://wrongscheme.wildcard.example"],
+ [false, "https://*.wildcard.example", "https://wrongport.wildcard.example:80"],
+ [false, "https://*.wildcard.example", "https://ok.wildcard.example.attacker.example/"],
+ [false, "https://*.wildcard.example", "https://attacker.example/https://ok.wildcard.example/"],
+ [false, "https://*.wildcard.example", "https://attacker.example/?https://ok.wildcard.example/"],
+ [false, "https://*.wildcard.example", "https://attacker.example/#https://ok.wildcard.example/"],
+ [false, "https://*-wildcard.example", "https://notsupported-wildcard.example"],
+ ].each do |pass, trusted, current|
+ test "is_trusted(#{current}) returns #{pass} based on #{trusted} in TrustedClients" do
+ Rails.configuration.Login.TrustedClients = ActiveSupport::OrderedOptions.new
+ Rails.configuration.Login.TrustedClients[trusted.to_sym] = ActiveSupport::OrderedOptions.new
+ assert_equal pass, ApiClient.new(url_prefix: current).is_trusted
+ end
+ end
end
commit fd63f537dfc8399330f16c47599ea2e9947972ee
Author: Tom Clegg <tom at curii.com>
Date: Mon Mar 27 11:38:11 2023 -0400
20264: Ignore superfluous :443 and :80 in trusted origin check.
Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom at curii.com>
diff --git a/lib/controller/localdb/login.go b/lib/controller/localdb/login.go
index a1ac2c55b..041336bca 100644
--- a/lib/controller/localdb/login.go
+++ b/lib/controller/localdb/login.go
@@ -173,16 +173,14 @@ func validateLoginRedirectTarget(cluster *arvados.Cluster, returnTo string) erro
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
+ target := origin(*u)
+ for trusted := range cluster.Login.TrustedClients {
+ if origin(url.URL(trusted)) == target {
+ return nil
+ }
}
- if u.String() == cluster.Services.Workbench1.ExternalURL.String() ||
- u.String() == cluster.Services.Workbench2.ExternalURL.String() {
+ if target == origin(url.URL(cluster.Services.Workbench1.ExternalURL)) ||
+ target == origin(url.URL(cluster.Services.Workbench2.ExternalURL)) {
return nil
}
if cluster.Login.TrustPrivateNetworks {
@@ -199,3 +197,19 @@ func validateLoginRedirectTarget(cluster *arvados.Cluster, returnTo string) erro
}
return fmt.Errorf("requesting site is not listed in TrustedClients config")
}
+
+// origin returns the canonical origin of a URL, e.g.,
+// origin("https://example:443/foo") returns "https://example/"
+func origin(u url.URL) string {
+ origin := url.URL{
+ Scheme: u.Scheme,
+ Host: u.Host,
+ Path: "/",
+ }
+ if origin.Port() == "80" && origin.Scheme == "http" {
+ origin.Host = origin.Hostname()
+ } else if origin.Port() == "443" && origin.Scheme == "https" {
+ origin.Host = origin.Hostname()
+ }
+ return origin.String()
+}
diff --git a/lib/controller/localdb/login_oidc_test.go b/lib/controller/localdb/login_oidc_test.go
index cf9cf30ec..9469fdfd3 100644
--- a/lib/controller/localdb/login_oidc_test.go
+++ b/lib/controller/localdb/login_oidc_test.go
@@ -44,7 +44,7 @@ type OIDCLoginSuite struct {
}
func (s *OIDCLoginSuite) SetUpTest(c *check.C) {
- s.trustedURL = &arvados.URL{Scheme: "https", Host: "app.example.com", Path: "/"}
+ s.trustedURL = &arvados.URL{Scheme: "https", Host: "app.example.com:443", Path: "/"}
s.fakeProvider = arvadostest.NewOIDCProvider(c)
s.fakeProvider.AuthEmail = "active-user at arvados.local"
diff --git a/lib/controller/localdb/login_test.go b/lib/controller/localdb/login_test.go
new file mode 100644
index 000000000..edd558a55
--- /dev/null
+++ b/lib/controller/localdb/login_test.go
@@ -0,0 +1,54 @@
+// Copyright (C) The Arvados Authors. All rights reserved.
+//
+// SPDX-License-Identifier: AGPL-3.0
+
+package localdb
+
+import (
+ "encoding/json"
+
+ "git.arvados.org/arvados.git/sdk/go/arvados"
+ check "gopkg.in/check.v1"
+)
+
+var _ = check.Suite(&loginSuite{})
+
+type loginSuite struct{}
+
+func (s *loginSuite) TestValidateLoginRedirectTarget(c *check.C) {
+ var cluster arvados.Cluster
+ for _, trial := range []struct {
+ pass bool
+ wb1 string
+ wb2 string
+ trusted string
+ target string
+ }{
+ {true, "https://wb1.example/", "https://wb2.example/", "", "https://wb2.example/"},
+ {true, "https://wb1.example:443/", "https://wb2.example:443/", "", "https://wb2.example/"},
+ {true, "https://wb1.example:443/", "https://wb2.example:443/", "", "https://wb2.example"},
+ {true, "https://wb1.example:443", "https://wb2.example:443", "", "https://wb2.example/"},
+ {true, "http://wb1.example:80/", "http://wb2.example:80/", "", "http://wb2.example/"},
+ {false, "https://wb1.example:80/", "https://wb2.example:80/", "", "https://wb2.example/"},
+ {false, "https://wb1.example:1234/", "https://wb2.example:1234/", "", "https://wb2.example/"},
+ {false, "https://wb1.example/", "https://wb2.example/", "", "https://bad.wb2.example/"},
+ {true, "https://wb1.example/", "https://wb2.example/", "https://good.wb2.example/", "https://good.wb2.example"},
+ {true, "https://wb1.example/", "https://wb2.example/", "https://good.wb2.example:443/", "https://good.wb2.example"},
+ {true, "https://wb1.example/", "https://wb2.example/", "https://good.wb2.example:443", "https://good.wb2.example/"},
+ } {
+ c.Logf("trial %+v", trial)
+ // We use json.Unmarshal() to load the test strings
+ // because we're testing behavior when the config file
+ // contains string X.
+ err := json.Unmarshal([]byte(`"`+trial.wb1+`"`), &cluster.Services.Workbench1.ExternalURL)
+ c.Assert(err, check.IsNil)
+ err = json.Unmarshal([]byte(`"`+trial.wb2+`"`), &cluster.Services.Workbench2.ExternalURL)
+ c.Assert(err, check.IsNil)
+ if trial.trusted != "" {
+ err = json.Unmarshal([]byte(`{"`+trial.trusted+`": {}}`), &cluster.Login.TrustedClients)
+ c.Assert(err, check.IsNil)
+ }
+ err = validateLoginRedirectTarget(&cluster, trial.target)
+ c.Check(err == nil, check.Equals, trial.pass)
+ }
+}
-----------------------------------------------------------------------
hooks/post-receive
--
More information about the arvados-commits
mailing list