20140: Accept wildcards in TrustedClients.

Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom@curii.com>

diff --git a/lib/config/config.default.yml b/lib/config/config.default.yml
index 1919d7b704af0ce7c0d92fe4d0320229f84d0c51..5684723a20cf2497d99071d4a2d7a757de4d1471 100644 (file)
--- 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 041336bcaca442e69ea6a23b57d116a82551eaf6..18537b202f6c5c63ff2a5e027eb0402f767acac9 100644 (file)
--- 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 edd558a55247c04eb59656512332ff7bd5e94173..5c8e92862fdf34a9213003198a346bff118cce9d 100644 (file)
--- 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 55a4c6706c7ccb802f50bdd8a2c2fbe3cee4fdee..791b9716802eb7eaa4ec35a197f6f94072a8b45c 100644 (file)
--- 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 a0eacfd13bb65ad2f6ff4f77cfa59d0b8fafd402..dbe9c863671bb34807b7ffe136095dc67c2d70cb 100644 (file)
--- 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