[ARVADOS] created: 2.1.0-743-g8e8a767e5

Git user git at public.arvados.org
Mon May 3 15:06:16 UTC 2021


        at  8e8a767e5162cc21af45218067a52e4a95219dc1 (commit)


commit 8e8a767e5162cc21af45218067a52e4a95219dc1
Author: Tom Clegg <tom at curii.com>
Date:   Mon May 3 11:06:00 2021 -0400

    17610: Check scopes when using a remote token.
    
    Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom at curii.com>

diff --git a/services/api/app/controllers/arvados/v1/api_client_authorizations_controller.rb b/services/api/app/controllers/arvados/v1/api_client_authorizations_controller.rb
index 99446688d..22bcb6c1d 100644
--- a/services/api/app/controllers/arvados/v1/api_client_authorizations_controller.rb
+++ b/services/api/app/controllers/arvados/v1/api_client_authorizations_controller.rb
@@ -49,7 +49,12 @@ class Arvados::V1::ApiClientAuthorizationsController < ApplicationController
   end
 
   def current
-    @object = Thread.current[:api_client_authorization]
+    @object = Thread.current[:api_client_authorization].dup
+    if params[:remote]
+      # Client is validating a salted token. Don't return the unsalted
+      # secret!
+      @object.api_token = nil
+    end
     show
   end
 
diff --git a/services/api/app/controllers/arvados/v1/schema_controller.rb b/services/api/app/controllers/arvados/v1/schema_controller.rb
index 9e1939799..8b71f7237 100644
--- a/services/api/app/controllers/arvados/v1/schema_controller.rb
+++ b/services/api/app/controllers/arvados/v1/schema_controller.rb
@@ -36,7 +36,7 @@ class Arvados::V1::SchemaController < ApplicationController
         # format is YYYYMMDD, must be fixed width (needs to be lexically
         # sortable), updated manually, may be used by clients to
         # determine availability of API server features.
-        revision: "20201210",
+        revision: "20210503",
         source_version: AppVersion.hash,
         sourceVersion: AppVersion.hash, # source_version should be deprecated in the future
         packageVersion: AppVersion.package_version,
diff --git a/services/api/app/middlewares/arvados_api_token.rb b/services/api/app/middlewares/arvados_api_token.rb
index be4e8bb0b..2c240984c 100644
--- a/services/api/app/middlewares/arvados_api_token.rb
+++ b/services/api/app/middlewares/arvados_api_token.rb
@@ -25,6 +25,7 @@ class ArvadosApiToken
     reader_tokens = nil
     if params["remote"] && request.get? && (
          request.path.start_with?('/arvados/v1/groups') ||
+         request.path.start_with?('/arvados/v1/api_client_authorizations/current') ||
          request.path.start_with?('/arvados/v1/users/current'))
       # Request from a remote API server, asking to validate a salted
       # token.
diff --git a/services/api/app/models/api_client_authorization.rb b/services/api/app/models/api_client_authorization.rb
index ee63c4d93..9e4356b90 100644
--- a/services/api/app/models/api_client_authorization.rb
+++ b/services/api/app/models/api_client_authorization.rb
@@ -249,21 +249,34 @@ class ApiClientAuthorization < ArvadosModel
 
     remote_user_prefix = remote_user['uuid'][0..4]
 
-    if token_uuid == ''
-      # Use the same UUID as the remote when caching the token.
-      begin
-        remote_token = SafeJSON.load(
-          clnt.get_content('https://' + host + '/arvados/v1/api_client_authorizations/current',
-                           {'remote' => Rails.configuration.ClusterID},
-                           {'Authorization' => 'Bearer ' + token}))
-        token_uuid = remote_token['uuid']
-        if !token_uuid.match(HasUuid::UUID_REGEX) || token_uuid[0..4] != upstream_cluster_id
-          raise "remote cluster #{upstream_cluster_id} returned invalid token uuid #{token_uuid.inspect}"
-        end
-      rescue => e
-        Rails.logger.warn "error getting remote token details for #{token.inspect}: #{e}"
-        return nil
+    # Get token scope, and make sure we use the same UUID as the
+    # remote when caching the token.
+    remote_token = nil
+    begin
+      remote_token = SafeJSON.load(
+        clnt.get_content('https://' + host + '/arvados/v1/api_client_authorizations/current',
+                         {'remote' => Rails.configuration.ClusterID},
+                         {'Authorization' => 'Bearer ' + token}))
+      Rails.logger.debug "retrieved remote token #{remote_token.inspect}"
+      if !token_uuid.match(HasUuid::UUID_REGEX) || token_uuid[0..4] != upstream_cluster_id
+        raise "remote cluster #{upstream_cluster_id} returned invalid token uuid #{token_uuid.inspect}"
+      end
+      token_uuid = remote_token['uuid']
+    rescue HTTPClient::BadResponseError => e
+      if e.res.status != 401
+        raise
       end
+      rev = SafeJSON.load(clnt.get_content('https://' + host + '/discovery/v1/apis/arvados/v1/rest'))['revision']
+      if rev >= '20010101' && rev < '20210503'
+        Rails.logger.warn "remote cluster #{upstream_cluster_id} at #{host} with api rev #{rev} does not provide token expiry and scopes; using scopes=['all']"
+      else
+        # remote server is new enough that it should have accepted
+        # this request if the token was valid
+        raise
+      end
+    rescue => e
+      Rails.logger.warn "error getting remote token details for #{token.inspect}: #{e}"
+      return nil
     end
 
     # Clusters can only authenticate for their own users.
@@ -339,26 +352,31 @@ class ApiClientAuthorization < ArvadosModel
         end
       end
 
-      # We will accept this token (and avoid reloading the user
-      # record) for 'RemoteTokenRefresh' (default 5 minutes).
-      # Possible todo:
-      # Request the actual api_client_auth record from the remote
-      # server in case it wants the token to expire sooner.
-      auth = ApiClientAuthorization.find_or_create_by(uuid: token_uuid) do |auth|
-        auth.user = user
-        auth.api_client_id = 0
-      end
       # If stored_secret is set, we save stored_secret in the database
       # but return the real secret to the caller. This way, if we end
       # up returning the auth record to the client, they see the same
       # secret they supplied, instead of the HMAC we saved in the
       # database.
       stored_secret = stored_secret || secret
+
+      # We will accept this token (and avoid reloading the user
+      # record) for 'RemoteTokenRefresh' (default 5 minutes).
+      exp = [db_current_time + Rails.configuration.Login.RemoteTokenRefresh,
+             remote_token.andand['expires_at']].compact.min
+      scopes = remote_token.andand['scopes'] || ['all']
+      auth = ApiClientAuthorization.find_or_create_by(uuid: token_uuid) do |auth|
+        auth.user = user
+        auth.api_token = stored_secret
+        auth.api_client_id = 0
+        auth.scopes = scopes
+        auth.expires_at = exp
+      end
       auth.update_attributes!(user: user,
                               api_token: stored_secret,
                               api_client_id: 0,
-                              expires_at: db_current_time + Rails.configuration.Login.RemoteTokenRefresh)
-      Rails.logger.debug "cached remote token #{token_uuid} with secret #{stored_secret} in local db"
+                              scopes: scopes,
+                              expires_at: exp)
+      Rails.logger.debug "cached remote token #{token_uuid} with secret #{stored_secret} and scopes #{scopes} in local db"
       auth.api_token = secret
       return auth
     end
diff --git a/services/api/test/functional/arvados/v1/api_client_authorizations_controller_test.rb b/services/api/test/functional/arvados/v1/api_client_authorizations_controller_test.rb
index c2c71ca54..bf407afcd 100644
--- a/services/api/test/functional/arvados/v1/api_client_authorizations_controller_test.rb
+++ b/services/api/test/functional/arvados/v1/api_client_authorizations_controller_test.rb
@@ -81,6 +81,15 @@ class Arvados::V1::ApiClientAuthorizationsControllerTest < ActionController::Tes
     end
   end
 
+  [:admin, :active].each do |token|
+    test "using '#{token}', get token details via 'current'" do
+      authorize_with token
+      get :current
+      assert_response 200
+      assert_equal json_response['scopes'], ['all']
+    end
+  end
+
   [# anyone can look up the token they're currently using
    [:admin, :admin, 200, 200, 1],
    [:active, :active, 200, 200, 1],
diff --git a/services/api/test/integration/api_client_authorizations_api_test.rb b/services/api/test/integration/api_client_authorizations_api_test.rb
index ce79fc557..14e3bb361 100644
--- a/services/api/test/integration/api_client_authorizations_api_test.rb
+++ b/services/api/test/integration/api_client_authorizations_api_test.rb
@@ -167,4 +167,16 @@ class ApiClientAuthorizationsApiTest < ActionDispatch::IntegrationTest
       end
     end
   end
+
+  test "get current token using salted token" do
+    salted = salt_token(fixture: :active, remote: 'abcde')
+    get('/arvados/v1/api_client_authorizations/current',
+        params: {remote: 'abcde'},
+        headers: {'HTTP_AUTHORIZATION' => "Bearer #{salted}"})
+    assert_response :success
+    assert_equal(json_response['uuid'], api_client_authorizations(:active).uuid)
+    assert_equal(json_response['scopes'], ['all'])
+    assert_not_nil(json_response['expires_at'])
+    assert_nil(json_response['api_token'])
+  end
 end
diff --git a/services/api/test/integration/remote_user_test.rb b/services/api/test/integration/remote_user_test.rb
index 432388452..568d5088d 100644
--- a/services/api/test/integration/remote_user_test.rb
+++ b/services/api/test/integration/remote_user_test.rb
@@ -67,6 +67,20 @@ class RemoteUsersTest < ActionDispatch::IntegrationTest
         res.status = @stub_status
         res.body = @stub_content.is_a?(String) ? @stub_content : @stub_content.to_json
       end
+      srv.mount_proc '/arvados/v1/api_client_authorizations/current' do |req, res|
+        if clusterid == 'zbbbb' and req.header['authorization'][0][10..14] == 'zbork'
+          # asking zbbbb about zbork should yield an error, zbbbb doesn't trust zbork
+          res.status = 401
+          return
+        end
+        res.status = @stub_token_status
+        if res.status == 200
+          res.body = {
+            uuid: api_client_authorizations(:active).uuid.sub('zzzzz', clusterid),
+            scopes: @stub_token_scopes,
+          }.to_json
+        end
+      end
       Thread.new do
         srv.start
       end
@@ -86,6 +100,8 @@ class RemoteUsersTest < ActionDispatch::IntegrationTest
       is_active: true,
       is_invited: true,
     }
+    @stub_token_status = 200
+    @stub_token_scopes = ["all"]
   end
 
   teardown do
@@ -94,6 +110,31 @@ class RemoteUsersTest < ActionDispatch::IntegrationTest
     end
   end
 
+  test 'authenticate with remote token that has limited scope' do
+    get '/arvados/v1/collections',
+        params: {format: 'json'},
+        headers: auth(remote: 'zbbbb')
+    assert_response :success
+
+    @stub_token_scopes = ["GET /arvados/v1/users/current"]
+
+    # re-authorize before cache expires
+    get '/arvados/v1/collections',
+        params: {format: 'json'},
+        headers: auth(remote: 'zbbbb')
+    assert_response :success
+
+    # simulate cache expiry
+    ApiClientAuthorization.where('uuid like ?', 'zbbbb-%').
+      update_all(expires_at: db_current_time - 1.minute)
+
+    # re-authorize after cache expires
+    get '/arvados/v1/collections',
+        params: {format: 'json'},
+        headers: auth(remote: 'zbbbb')
+    assert_response 403
+  end
+
   test 'authenticate with remote token' do
     get '/arvados/v1/users/current',
       params: {format: 'json'},
@@ -115,8 +156,7 @@ class RemoteUsersTest < ActionDispatch::IntegrationTest
     assert_response :success
 
     # simulate cache expiry
-    ApiClientAuthorization.where(
-      uuid: salted_active_token(remote: 'zbbbb').split('/')[1]).
+    ApiClientAuthorization.where('uuid like ?', 'zbbbb-%').
       update_all(expires_at: db_current_time - 1.minute)
 
     # re-authorize after cache expires

-----------------------------------------------------------------------


hooks/post-receive
-- 




More information about the arvados-commits mailing list