From: Peter Amstutz Date: Fri, 22 Aug 2014 15:22:23 +0000 (-0400) Subject: 3036: Move manifest_text validation into Collection model. Change X-Git-Tag: 1.1.0~2275^2~8 X-Git-Url: https://git.arvados.org/arvados.git/commitdiff_plain/128506128f407047b3dc40e219cc9734afa7090a 3036: Move manifest_text validation into Collection model. Change uuids_for_docker_image to find_all_for_docker_image which returns Collection objects instead of uuids. Remove unused stripped_portable_data_hash helper. Improved error messages. Tests pass. --- diff --git a/services/api/app/controllers/application_controller.rb b/services/api/app/controllers/application_controller.rb index 1934504a2f..ad22accd71 100644 --- a/services/api/app/controllers/application_controller.rb +++ b/services/api/app/controllers/application_controller.rb @@ -104,9 +104,8 @@ class ApplicationController < ActionController::Base if e.respond_to? :backtrace and e.backtrace logger.error e.backtrace.collect { |x| x + "\n" }.join('') end - if (@object and @object.respond_to? :errors and - @object.errors and @object.errors.full_messages and - not @object.errors.full_messages.empty?) + if (@object.respond_to? :errors and + @object.errors.andand.full_messages.andand.any?) errors = @object.errors.full_messages logger.error errors.inspect else diff --git a/services/api/app/controllers/arvados/v1/collections_controller.rb b/services/api/app/controllers/arvados/v1/collections_controller.rb index d31e59a394..23230c6b6c 100644 --- a/services/api/app/controllers/arvados/v1/collections_controller.rb +++ b/services/api/app/controllers/arvados/v1/collections_controller.rb @@ -1,53 +1,9 @@ class Arvados::V1::CollectionsController < ApplicationController def create - if !resource_attrs[:manifest_text] - return send_error("'manifest_text' attribute must be specified", - status: :unprocessable_entity) - end - if resource_attrs[:uuid] and (loc = Locator.parse(resource_attrs[:uuid])) resource_attrs[:portable_data_hash] = loc.to_s resource_attrs.delete :uuid end - - # Check permissions on the collection manifest. - # If any signature cannot be verified, return 403 Permission denied. - api_token = current_api_client_authorization.andand.api_token - signing_opts = { - key: Rails.configuration.blob_signing_key, - api_token: api_token, - ttl: Rails.configuration.blob_signing_ttl, - } - resource_attrs[:manifest_text].lines.each do |entry| - entry.split[1..-1].each do |tok| - if /^[[:digit:]]+:[[:digit:]]+:/.match tok - # This is a filename token, not a blob locator. Note that we - # keep checking tokens after this, even though manifest - # format dictates that all subsequent tokens will also be - # filenames. Safety first! - elsif Blob.verify_signature tok, signing_opts - # OK. - elsif Locator.parse(tok).andand.signature - # Signature provided, but verify_signature did not like it. - logger.warn "Invalid signature on locator #{tok}" - raise ArvadosModel::PermissionDeniedError - elsif Rails.configuration.permit_create_collection_with_unsigned_manifest - # No signature provided, but we are running in insecure mode. - logger.debug "Missing signature on locator #{tok} ignored" - elsif Blob.new(tok).empty? - # No signature provided -- but no data to protect, either. - else - logger.warn "Missing signature on locator #{tok}" - raise ArvadosModel::PermissionDeniedError - end - end - end - - # Remove any permission signatures from the manifest. - munge_manifest_locators(resource_attrs[:manifest_text]) do |loc| - loc.without_signature.to_s - end - super end @@ -200,18 +156,6 @@ class Arvados::V1::CollectionsController < ApplicationController render json: visited end - def self.munge_manifest_locators(manifest) - # Given a manifest text and a block, yield each locator, - # and replace it with whatever the block returns. - manifest.andand.gsub!(/ [[:xdigit:]]{32}(\+[[:digit:]]+)?(\+\S+)/) do |word| - if loc = Locator.parse(word.strip) - " " + yield(loc) - else - " " + word - end - end - end - protected def find_objects_for_index @@ -222,10 +166,6 @@ class Arvados::V1::CollectionsController < ApplicationController super end - def munge_manifest_locators(manifest, &block) - self.class.munge_manifest_locators(manifest, &block) - end - def sign_manifests(*manifests) if current_api_client_authorization signing_opts = { @@ -234,7 +174,7 @@ class Arvados::V1::CollectionsController < ApplicationController ttl: Rails.configuration.blob_signing_ttl, } manifests.each do |text| - munge_manifest_locators(text) do |loc| + Collection.munge_manifest_locators(text) do |loc| Blob.sign_locator(loc.to_s, signing_opts) end end diff --git a/services/api/app/controllers/arvados/v1/groups_controller.rb b/services/api/app/controllers/arvados/v1/groups_controller.rb index 7ee4e555ae..c1d414e47f 100644 --- a/services/api/app/controllers/arvados/v1/groups_controller.rb +++ b/services/api/app/controllers/arvados/v1/groups_controller.rb @@ -6,6 +6,8 @@ class Arvados::V1::GroupsController < ApplicationController uuid: { type: 'string', required: false, default: nil }, + # include_linked returns name links, which are obsolete, so + # remove it when clients have been migrated. include_linked: { type: 'boolean', required: false, default: false }, @@ -34,6 +36,8 @@ class Arvados::V1::GroupsController < ApplicationController def contents # Set @objects: + # include_linked returns name links, which are obsolete, so + # remove it when clients have been migrated. load_searchable_objects(owner_uuid: @object.andand.uuid, include_linked: params[:include_linked]) sql = 'link_class=? and head_uuid in (?)' diff --git a/services/api/app/controllers/arvados/v1/jobs_controller.rb b/services/api/app/controllers/arvados/v1/jobs_controller.rb index 5045e078c3..fedaf27200 100644 --- a/services/api/app/controllers/arvados/v1/jobs_controller.rb +++ b/services/api/app/controllers/arvados/v1/jobs_controller.rb @@ -214,9 +214,7 @@ class Arvados::V1::JobsController < ApplicationController search_list = filter[2].is_a?(Enumerable) ? filter[2] : [filter[2]] filter[2] = search_list.flat_map do |search_term| image_search, image_tag = search_term.split(':', 2) - Collection.uuids_for_docker_image(image_search, image_tag, @read_users).map do |uuid| - Collection.find_by_uuid(uuid).portable_data_hash - end + Collection.find_all_for_docker_image(image_search, image_tag, @read_users).map(&:portable_data_hash) end true else diff --git a/services/api/app/helpers/collections_helper.rb b/services/api/app/helpers/collections_helper.rb index 020bdbe020..30179854a2 100644 --- a/services/api/app/helpers/collections_helper.rb +++ b/services/api/app/helpers/collections_helper.rb @@ -1,10 +1,2 @@ module CollectionsHelper - def stripped_portable_data_hash(uuid) - m = /([a-f0-9]{32}(\+[0-9]+)?)(\+.*)?/.match(uuid) - if m - m[1] - else - nil - end - end end diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb index f95a35e739..205f54ab7f 100644 --- a/services/api/app/models/arvados_model.rb +++ b/services/api/app/models/arvados_model.rb @@ -240,7 +240,7 @@ class ArvadosModel < ActiveRecord::Base rsc_class = ArvadosModel::resource_class_for_uuid owner_uuid unless rsc_class == User or rsc_class == Group - errors.add :owner_uuid, "can only be set to User or Group" + errors.add :owner_uuid, "must be set to User or Group" raise PermissionDeniedError end diff --git a/services/api/app/models/collection.rb b/services/api/app/models/collection.rb index c0c2b7ef7d..f15d0310d2 100644 --- a/services/api/app/models/collection.rb +++ b/services/api/app/models/collection.rb @@ -3,6 +3,8 @@ class Collection < ArvadosModel include KindAndEtag include CommonApiTemplate + before_validation :check_signatures + before_validation :strip_manifest_text before_validation :set_portable_data_hash validate :ensure_hash_matches_manifest_text @@ -22,6 +24,59 @@ class Collection < ArvadosModel }) end + def check_signatures + return false if self.manifest_text.nil? + + return true if current_user.andand.is_admin + + if self.manifest_text_changed? + # Check permissions on the collection manifest. + # If any signature cannot be verified, raise PermissionDeniedError + # which will return 403 Permission denied to the client. + api_token = current_api_client_authorization.andand.api_token + signing_opts = { + key: Rails.configuration.blob_signing_key, + api_token: api_token, + ttl: Rails.configuration.blob_signing_ttl, + } + self.manifest_text.lines.each do |entry| + entry.split[1..-1].each do |tok| + if /^[[:digit:]]+:[[:digit:]]+:/.match tok + # This is a filename token, not a blob locator. Note that we + # keep checking tokens after this, even though manifest + # format dictates that all subsequent tokens will also be + # filenames. Safety first! + elsif Blob.verify_signature tok, signing_opts + # OK. + elsif Locator.parse(tok).andand.signature + # Signature provided, but verify_signature did not like it. + logger.warn "Invalid signature on locator #{tok}" + raise ArvadosModel::PermissionDeniedError + elsif Rails.configuration.permit_create_collection_with_unsigned_manifest + # No signature provided, but we are running in insecure mode. + logger.debug "Missing signature on locator #{tok} ignored" + elsif Blob.new(tok).empty? + # No signature provided -- but no data to protect, either. + else + logger.warn "Missing signature on locator #{tok}" + raise ArvadosModel::PermissionDeniedError + end + end + end + end + true + end + + def strip_manifest_text + if self.manifest_text_changed? + # Remove any permission signatures from the manifest. + Collection.munge_manifest_locators(self[:manifest_text]) do |loc| + loc.without_signature.to_s + end + end + true + end + def set_portable_data_hash if (self.portable_data_hash.nil? or (self.portable_data_hash == "") or (manifest_text_changed? and !portable_data_hash_changed?)) self.portable_data_hash = "#{Digest::MD5.hexdigest(manifest_text)}+#{manifest_text.length}" @@ -136,6 +191,18 @@ class Collection < ArvadosModel end end + def self.munge_manifest_locators(manifest) + # Given a manifest text and a block, yield each locator, + # and replace it with whatever the block returns. + manifest.andand.gsub!(/ [[:xdigit:]]{32}(\+[[:digit:]]+)?(\+\S+)/) do |word| + if loc = Locator.parse(word.strip) + " " + yield(loc) + else + " " + word + end + end + end + def self.normalize_uuid uuid hash_part = nil size_part = nil @@ -152,7 +219,8 @@ class Collection < ArvadosModel [hash_part, size_part].compact.join '+' end - def self.uuids_for_docker_image(search_term, search_tag=nil, readers=nil) + # Return array of Collection objects + def self.find_all_for_docker_image(search_term, search_tag=nil, readers=nil) readers ||= [Thread.current[:user]] base_search = Link. readable_by(*readers). @@ -167,7 +235,7 @@ class Collection < ArvadosModel coll_match = readable_by(*readers).where(portable_data_hash: loc.to_s).limit(1).first if coll_match and (coll_match.files.size == 1) and (coll_match.files[0][1] =~ /^[0-9A-Fa-f]{64}\.tar$/) - return [coll_match.uuid] + return [coll_match] end end @@ -192,21 +260,14 @@ class Collection < ArvadosModel # so that anything with an image timestamp is considered more recent than # anything without; then we use the link's created_at as a tiebreaker. uuid_timestamps = {} - matches.find_each do |link| - c = Collection.find_by_uuid(link.head_uuid) - uuid_timestamps[c.uuid] = - [(-link.properties["image_timestamp"].to_datetime.to_i rescue 0), - -link.created_at.to_i] + matches.all.map do |link| + uuid_timestamps[link.head_uuid] = [(-link.properties["image_timestamp"].to_datetime.to_i rescue 0), + -link.created_at.to_i] end - uuid_timestamps.keys.sort_by { |uuid| uuid_timestamps[uuid] } + Collection.where('uuid in (?)', uuid_timestamps.keys).sort_by { |c| uuid_timestamps[c.uuid] } end def self.for_latest_docker_image(search_term, search_tag=nil, readers=nil) - image_uuid = uuids_for_docker_image(search_term, search_tag, readers).first - if image_uuid.nil? - nil - else - find_by_uuid(image_uuid) - end + find_all_for_docker_image(search_term, search_tag, readers).first end end diff --git a/services/api/app/models/link.rb b/services/api/app/models/link.rb index e319190534..808489e1c2 100644 --- a/services/api/app/models/link.rb +++ b/services/api/app/models/link.rb @@ -72,7 +72,8 @@ class Link < ArvadosModel def name_links_are_obsolete if link_class == 'name' - errors.add('name', 'Name links are obsolete') + errors.add('name', 'Name links are obsolete') + false else true end diff --git a/services/api/db/migrate/20140811184643_collection_use_regular_uuids.rb b/services/api/db/migrate/20140811184643_collection_use_regular_uuids.rb index 5c21c0bb2d..4f83ecab2c 100644 --- a/services/api/db/migrate/20140811184643_collection_use_regular_uuids.rb +++ b/services/api/db/migrate/20140811184643_collection_use_regular_uuids.rb @@ -3,7 +3,7 @@ class CollectionUseRegularUuids < ActiveRecord::Migration add_column :collections, :name, :string add_column :collections, :description, :string add_column :collections, :properties, :text - add_column :collections, :expire_time, :date + add_column :collections, :expires_at, :date remove_column :collections, :locator say_with_time "Step 1. Move manifest hashes into portable_data_hash field" do @@ -175,6 +175,6 @@ where head_uuid like '________________________________+%' or tail_uuid like '___ end def down - # Not gonna happen. + raise ActiveRecord::IrreversibleMigration, "Can't downmigrate changes to collections and links without potentially losing data." end end diff --git a/services/api/lib/has_uuid.rb b/services/api/lib/has_uuid.rb index df175f8602..06c7d0cacf 100644 --- a/services/api/lib/has_uuid.rb +++ b/services/api/lib/has_uuid.rb @@ -36,18 +36,18 @@ module HasUuid if re[1] == self.class.uuid_prefix return true else - self.errors.add(:uuid, "Matched uuid type '#{re[1]}', expected '#{self.class.uuid_prefix}'") + self.errors.add(:uuid, "type field is '#{re[1]}', expected '#{self.class.uuid_prefix}'") return false end else - self.errors.add(:uuid, "'#{self.uuid}' is not a valid Arvados UUID") + self.errors.add(:uuid, "not a valid Arvados uuid '#{self.uuid}'") return false end else if self.new_record? - self.errors.add(:uuid, "Not permitted to specify uuid") + self.errors.add(:uuid, "assignment not permittid") else - self.errors.add(:uuid, "Not permitted to change uuid") + self.errors.add(:uuid, "change not permitted") end return false end diff --git a/services/api/test/functional/arvados/v1/collections_controller_test.rb b/services/api/test/functional/arvados/v1/collections_controller_test.rb index 78d3d748a2..65dfca5e1f 100644 --- a/services/api/test/functional/arvados/v1/collections_controller_test.rb +++ b/services/api/test/functional/arvados/v1/collections_controller_test.rb @@ -102,9 +102,8 @@ class Arvados::V1::CollectionsControllerTest < ActionController::TestCase assert_equal 99999, resp['offset'] end - test "create with unsigned manifest" do - permit_unsigned_manifests - authorize_with :active + test "admin can create collection with unsigned manifest" do + authorize_with :admin test_collection = { manifest_text: <<-EOS . d41d8cd98f00b204e9800998ecf8427e+0 0:0:foo.txt @@ -134,7 +133,7 @@ EOS assert_response :success assert_not_nil assigns(:object) resp = JSON.parse(@response.body) - assert_equal test_collection[:uuid], resp['uuid'] + assert_equal test_collection[:portable_data_hash], resp['portable_data_hash'] # The manifest in the response will have had permission hints added. # Remove any permission hints in the response before comparing it to the source.