[ARVADOS] created: 1.3.0-2-ge3a6d99eb
Git user
git at public.curoverse.com
Thu Dec 6 09:24:05 EST 2018
at e3a6d99eb6fc3f853ee475909d49e819821cd112 (commit)
commit e3a6d99eb6fc3f853ee475909d49e819821cd112
Author: Peter Amstutz <pamstutz at veritasgenetics.com>
Date: Thu Dec 6 09:23:34 2018 -0500
14584: Restrict container reuse to readable containers and outputs
Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <pamstutz at veritasgenetics.com>
diff --git a/services/api/app/models/container.rb b/services/api/app/models/container.rb
index bd586907e..a27b382cd 100644
--- a/services/api/app/models/container.rb
+++ b/services/api/app/models/container.rb
@@ -179,10 +179,11 @@ class Container < ArvadosModel
runtime_auth_scopes: runtime_auth_scopes
}
end
- act_as_system_user do
- if req.use_existing && (reusable = find_reusable(c_attrs))
- reusable
- else
+
+ if req.use_existing && (reusable = find_reusable(c_attrs, current_user, runtime_user))
+ reusable
+ else
+ act_as_system_user do
Container.create!(c_attrs)
end
end
@@ -251,9 +252,15 @@ class Container < ArvadosModel
coll.portable_data_hash
end
- def self.find_reusable(attrs)
- log_reuse_info { "starting with #{Container.all.count} container records in database" }
- candidates = Container.where_serialized(:command, attrs[:command], md5: true)
+ def self.find_reusable(attrs, *users_list)
+ users_list.compact!
+ if users_list.length == 0
+ users_list = [current_user]
+ end
+ candidates = Container.readable_by(*users_list)
+
+ log_reuse_info { "starting with #{candidates.count} container records readable by this user" }
+ candidates = candidates.where_serialized(:command, attrs[:command], md5: true)
log_reuse_info(candidates) { "after filtering on command #{attrs[:command].inspect}" }
candidates = candidates.where('cwd = ?', attrs[:cwd])
@@ -265,24 +272,23 @@ class Container < ArvadosModel
candidates = candidates.where('output_path = ?', attrs[:output_path])
log_reuse_info(candidates) { "after filtering on output_path #{attrs[:output_path].inspect}" }
- image = resolve_container_image(attrs[:container_image])
- candidates = candidates.where('container_image = ?', image)
- log_reuse_info(candidates) { "after filtering on container_image #{image.inspect} (resolved from #{attrs[:container_image].inspect})" }
+ candidates = candidates.where('container_image = ?', attrs[:container_image])
+ log_reuse_info(candidates) { "after filtering on container_image #{attrs[:container_image].inspect}" }
- candidates = candidates.where_serialized(:mounts, resolve_mounts(attrs[:mounts]), md5: true)
+ candidates = candidates.where_serialized(:mounts, attrs[:mounts], md5: true)
log_reuse_info(candidates) { "after filtering on mounts #{attrs[:mounts].inspect}" }
secret_mounts_md5 = Digest::MD5.hexdigest(SafeJSON.dump(self.deep_sort_hash(attrs[:secret_mounts])))
candidates = candidates.where('secret_mounts_md5 = ?', secret_mounts_md5)
log_reuse_info(candidates) { "after filtering on secret_mounts_md5 #{secret_mounts_md5.inspect}" }
- candidates = candidates.where_serialized(:runtime_constraints, resolve_runtime_constraints(attrs[:runtime_constraints]), md5: true)
+ candidates = candidates.where_serialized(:runtime_constraints, attrs[:runtime_constraints], md5: true)
log_reuse_info(candidates) { "after filtering on runtime_constraints #{attrs[:runtime_constraints].inspect}" }
log_reuse_info { "checking for state=Complete with readable output and log..." }
select_readable_pdh = Collection.
- readable_by(current_user).
+ readable_by(*users_list).
select(:portable_data_hash).
to_sql
diff --git a/services/api/test/unit/container_test.rb b/services/api/test/unit/container_test.rb
index 5589cae4d..43595adf0 100644
--- a/services/api/test/unit/container_test.rb
+++ b/services/api/test/unit/container_test.rb
@@ -25,6 +25,7 @@ class ContainerTest < ActiveSupport::TestCase
runtime_constraints: {
"ram" => 12000000000,
"vcpus" => 4,
+ "keep_cache_ram" => Rails.configuration.container_default_keep_cache_ram,
},
mounts: {
"test" => {"kind" => "json"},
@@ -241,7 +242,6 @@ class ContainerTest < ActiveSupport::TestCase
end
test "find_reusable method should select higher priority queued container" do
- Rails.configuration.log_reuse_decisions = true
set_user_from_auth :active
common_attrs = REUSABLE_COMMON_ATTRS.merge({environment:{"var" => "queued"}})
c_low_priority, _ = minimal_new(common_attrs.merge({use_existing:false, priority:1}))
@@ -249,7 +249,7 @@ class ContainerTest < ActiveSupport::TestCase
assert_not_equal c_low_priority.uuid, c_high_priority.uuid
assert_equal Container::Queued, c_low_priority.state
assert_equal Container::Queued, c_high_priority.state
- reused = Container.find_reusable(common_attrs)
+ reused = Container.find_reusable(common_attrs, users(:active))
assert_not_nil reused
assert_equal reused.uuid, c_high_priority.uuid
end
@@ -277,7 +277,7 @@ class ContainerTest < ActiveSupport::TestCase
c_recent.update_attributes!({state: Container::Running})
c_recent.update_attributes!(completed_attrs)
- reused = Container.find_reusable(common_attrs)
+ reused = Container.find_reusable(common_attrs, users(:active))
assert_not_nil reused
assert_equal reused.uuid, c_older.uuid
end
@@ -341,7 +341,8 @@ class ContainerTest < ActiveSupport::TestCase
c_faster_started_second.update_attributes!({state: Container::Locked})
c_faster_started_second.update_attributes!({state: Container::Running,
progress: 0.15})
- reused = Container.find_reusable(common_attrs)
+
+ reused = Container.find_reusable(common_attrs, users(:active))
assert_not_nil reused
# Selected container is the one that started first
assert_equal reused.uuid, c_faster_started_first.uuid
@@ -365,7 +366,8 @@ class ContainerTest < ActiveSupport::TestCase
c_faster_started_second.update_attributes!({state: Container::Locked})
c_faster_started_second.update_attributes!({state: Container::Running,
progress: 0.2})
- reused = Container.find_reusable(common_attrs)
+
+ reused = Container.find_reusable(common_attrs, users(:active))
assert_not_nil reused
# Selected container is the one with most progress done
assert_equal reused.uuid, c_faster_started_second.uuid
@@ -391,7 +393,8 @@ class ContainerTest < ActiveSupport::TestCase
c_faster_started_second.update_attributes!({state: Container::Running,
runtime_status: {'error' => 'Something bad happened'},
progress: 0.2})
- reused = Container.find_reusable(common_attrs)
+
+ reused = Container.find_reusable(common_attrs, users(:active))
assert_not_nil reused
# Selected the non-failing container even if it's the one with less progress done
assert_equal reused.uuid, c_faster_started_first.uuid
@@ -412,7 +415,8 @@ class ContainerTest < ActiveSupport::TestCase
priority: 2})
c_high_priority_newer.update_attributes!({state: Container::Locked,
priority: 2})
- reused = Container.find_reusable(common_attrs)
+
+ reused = Container.find_reusable(common_attrs, users(:active))
assert_not_nil reused
assert_equal reused.uuid, c_high_priority_older.uuid
end
@@ -433,7 +437,8 @@ class ContainerTest < ActiveSupport::TestCase
c_running.update_attributes!({state: Container::Locked})
c_running.update_attributes!({state: Container::Running,
progress: 0.15})
- reused = Container.find_reusable(common_attrs)
+
+ reused = Container.find_reusable(common_attrs, users(:active))
assert_not_nil reused
assert_equal reused.uuid, c_running.uuid
end
@@ -454,7 +459,8 @@ class ContainerTest < ActiveSupport::TestCase
c_running.update_attributes!({state: Container::Locked})
c_running.update_attributes!({state: Container::Running,
progress: 0.15})
- reused = Container.find_reusable(common_attrs)
+
+ reused = Container.find_reusable(common_attrs, users(:active))
assert_not_nil reused
assert_equal c_completed.uuid, reused.uuid
end
@@ -470,7 +476,8 @@ class ContainerTest < ActiveSupport::TestCase
c_running.update_attributes!({state: Container::Locked})
c_running.update_attributes!({state: Container::Running,
progress: 0.15})
- reused = Container.find_reusable(common_attrs)
+
+ reused = Container.find_reusable(common_attrs, users(:active))
assert_not_nil reused
assert_equal reused.uuid, c_running.uuid
end
@@ -483,7 +490,8 @@ class ContainerTest < ActiveSupport::TestCase
assert_not_equal c_queued.uuid, c_locked.uuid
set_user_from_auth :dispatch1
c_locked.update_attributes!({state: Container::Locked})
- reused = Container.find_reusable(common_attrs)
+
+ reused = Container.find_reusable(common_attrs, users(:active))
assert_not_nil reused
assert_equal reused.uuid, c_locked.uuid
end
@@ -497,7 +505,8 @@ class ContainerTest < ActiveSupport::TestCase
c.update_attributes!({state: Container::Running})
c.update_attributes!({state: Container::Complete,
exit_code: 33})
- reused = Container.find_reusable(attrs)
+
+ reused = Container.find_reusable(attrs, users(:active))
assert_nil reused
end
@@ -591,15 +600,56 @@ class ContainerTest < ActiveSupport::TestCase
log: 'b6701533bf043971f4750a777cf5e4e6+47',
output: '08504d4f1ad35c690f09150d0590eb9b+49'
}
- c1, _ = minimal_new(common_attrs.merge({runtime_token: api_client_authorizations(:active).token}))
+ c1, cr1 = minimal_new(common_attrs.merge(runtime_token_attr(:container_runtime_token)))
c1.update_attributes!({state: Container::Locked})
c1.update_attributes!({state: Container::Running})
c1.update_attributes!(completed_attrs)
assert_equal Container::Complete, c1.state
- reused = Container.find_reusable(common_attrs.merge(runtime_token_attr(:container_runtime_token)))
- # "active" user can't read the log and output, should not reuse.
+ reused = Container.find_reusable(common_attrs.merge({runtime_token: api_client_authorizations(:active).token}),
+ users(:active))
+ # "active" alone can't see the container request, output, or log
assert_nil reused
+
+ reused = Container.find_reusable(common_attrs.merge({runtime_token: api_client_authorizations(:active).token}),
+ users(:container_runtime_token_user), users(:active))
+ # Can find it if we check both user permissions at once
+ assert_not_nil reused
+ assert_equal reused.uuid, c1.uuid
+
+ col_out, col_log = act_as_system_user do
+ cr1.update_attributes!(owner_uuid: users(:active).uuid)
+ col_out = Collection.find_by_uuid(collections(:crt1_output).uuid)
+ col_out.update_attributes!(owner_uuid: users(:active).uuid)
+ col_log = Collection.find_by_uuid(collections(:crt1_log).uuid)
+ col_log.update_attributes!(owner_uuid: users(:active).uuid)
+ [col_out, col_log]
+ end
+ reused = Container.find_reusable(common_attrs.merge({runtime_token: api_client_authorizations(:active).token}),
+ users(:active))
+ # update ownership of CR, log and output should allow "active" user to reuse
+ assert_not_nil reused
+ assert_equal reused.uuid, c1.uuid
+
+ # test each field individually
+ [cr1, col_out, col_log].each do |item|
+ act_as_system_user do
+ item.update_attributes!(owner_uuid: users(:container_runtime_token_user).uuid)
+ end
+ reused = Container.find_reusable(common_attrs.merge({runtime_token: api_client_authorizations(:active).token}),
+ users(:active))
+ # update ownership to "container runtime token user" should prevent reuse by "active"
+ assert_nil reused
+
+ act_as_system_user do
+ item.update_attributes!(owner_uuid: users(:active).uuid)
+ end
+ reused = Container.find_reusable(common_attrs.merge({runtime_token: api_client_authorizations(:active).token}),
+ users(:active))
+ # update ownership to "active" should allow reuse
+ assert_not_nil reused
+ assert_equal reused.uuid, c1.uuid
+ end
end
test "Container running" do
commit 73b0c73b3303255a92d8a435b28a5a75c10be0a1
Author: Peter Amstutz <pamstutz at veritasgenetics.com>
Date: Wed Dec 5 16:04:01 2018 -0500
14584: Test confirming reuse bug
It will reuse a container even if the output is not readable by the
current user. This is a security flaw.
Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <pamstutz at veritasgenetics.com>
diff --git a/services/api/test/fixtures/collections.yml b/services/api/test/fixtures/collections.yml
index 8763f3944..6f6d8f0a2 100644
--- a/services/api/test/fixtures/collections.yml
+++ b/services/api/test/fixtures/collections.yml
@@ -971,6 +971,22 @@ collection_with_uri_prop:
properties:
"http://schema.org/example": "value1"
+crt1_output:
+ uuid: zzzzz-4zz18-ayre2yoeb4kivlr
+ owner_uuid: zzzzz-tpzed-l3skomkti0c4vg4
+ portable_data_hash: 08504d4f1ad35c690f09150d0590eb9b+49
+ manifest_text: ". 3c4a71133b1e998cdd256614c1d2c33d+12+A642aee8b814b339e29a479a6130201f3f5a1c327 at 5c1ab0c6 0:12:stdin\n"
+ name: Saved at 2018-12-05 20:57:31 UTC by peter at petervg
+ current_version_uuid: zzzzz-4zz18-ayre2yoeb4kivlr
+
+crt1_log:
+ uuid: zzzzz-4zz18-eo9rz91s06aqs9v
+ owner_uuid: zzzzz-tpzed-l3skomkti0c4vg4
+ portable_data_hash: b6701533bf043971f4750a777cf5e4e6+47
+ manifest_text: ". 7c97df88c87074bdf3f585b49b180c40+9+A0bd0fab88d81313fa97fdbfeafb6e82325064a1b at 5c1ab0f9 0:9:stdin\n"
+ name: Saved at 2018-12-05 20:58:23 UTC by peter at petervg
+ current_version_uuid: zzzzz-4zz18-eo9rz91s06aqs9v
+
# Test Helper trims the rest of the file
# Do not add your fixtures below this line as the rest of this file will be trimmed by test_helper
diff --git a/services/api/test/unit/container_test.rb b/services/api/test/unit/container_test.rb
index 2a9ff5bf4..5589cae4d 100644
--- a/services/api/test/unit/container_test.rb
+++ b/services/api/test/unit/container_test.rb
@@ -582,6 +582,26 @@ class ContainerTest < ActiveSupport::TestCase
assert_equal c1.uuid, reused.uuid
end
+ test "find_reusable method with different runtime_token, different user, unreadable output" do
+ set_user_from_auth :crt_user
+ common_attrs = REUSABLE_COMMON_ATTRS.merge({use_existing:false, priority:1, environment:{"var" => "queued"}})
+ completed_attrs = {
+ state: Container::Complete,
+ exit_code: 0,
+ log: 'b6701533bf043971f4750a777cf5e4e6+47',
+ output: '08504d4f1ad35c690f09150d0590eb9b+49'
+ }
+ c1, _ = minimal_new(common_attrs.merge({runtime_token: api_client_authorizations(:active).token}))
+ c1.update_attributes!({state: Container::Locked})
+ c1.update_attributes!({state: Container::Running})
+ c1.update_attributes!(completed_attrs)
+ assert_equal Container::Complete, c1.state
+
+ reused = Container.find_reusable(common_attrs.merge(runtime_token_attr(:container_runtime_token)))
+ # "active" user can't read the log and output, should not reuse.
+ assert_nil reused
+ end
+
test "Container running" do
set_user_from_auth :active
c, _ = minimal_new priority: 1
-----------------------------------------------------------------------
hooks/post-receive
--
More information about the arvados-commits
mailing list