From: Tom Clegg <tom@curoverse.com>
Date: Wed, 8 Jun 2016 13:52:30 +0000 (-0400)
Subject: 9278: Ensure locator signatures expire no later than expires_at.
X-Git-Tag: 1.1.0~900^2
X-Git-Url: https://git.arvados.org/arvados.git/commitdiff_plain/bd872ed1fbda80e4e20c2b1e916d210f670afe4e

9278: Ensure locator signatures expire no later than expires_at.
---

diff --git a/services/api/app/models/collection.rb b/services/api/app/models/collection.rb
index df969e3315..4a612924b6 100644
--- a/services/api/app/models/collection.rb
+++ b/services/api/app/models/collection.rb
@@ -39,7 +39,10 @@ class Collection < ArvadosModel
                 # expose signed_manifest_text as manifest_text in the
                 # API response, and never let clients select the
                 # manifest_text column.
-                'manifest_text' => ['manifest_text'],
+                #
+                # We need expires_at to determine the correct
+                # timestamp in signed_manifest_text.
+                'manifest_text' => ['manifest_text', 'expires_at'],
                 )
   end
 
@@ -213,14 +216,19 @@ class Collection < ArvadosModel
   def signed_manifest_text
     if has_attribute? :manifest_text
       token = current_api_client_authorization.andand.api_token
-      @signed_manifest_text = self.class.sign_manifest manifest_text, token
+      exp = [db_current_time.to_i + Rails.configuration.blob_signature_ttl,
+             expires_at].compact.map(&:to_i).min
+      @signed_manifest_text = self.class.sign_manifest manifest_text, token, exp
     end
   end
 
-  def self.sign_manifest manifest, token
+  def self.sign_manifest manifest, token, exp=nil
+    if exp.nil?
+      exp = db_current_time.to_i + Rails.configuration.blob_signature_ttl
+    end
     signing_opts = {
       api_token: token,
-      expire: db_current_time.to_i + Rails.configuration.blob_signature_ttl,
+      expire: exp,
     }
     m = munge_manifest_locators(manifest) do |match|
       Blob.sign_locator(match[0], signing_opts)
diff --git a/services/api/test/unit/collection_test.rb b/services/api/test/unit/collection_test.rb
index 536d9ca47e..91568927ae 100644
--- a/services/api/test/unit/collection_test.rb
+++ b/services/api/test/unit/collection_test.rb
@@ -307,6 +307,29 @@ class CollectionTest < ActiveSupport::TestCase
     end
   end
 
+  test 'signature expiry does not exceed expires_at' do
+    act_as_user users(:active) do
+      t0 = db_current_time
+      c = Collection.create!(manifest_text: ". d41d8cd98f00b204e9800998ecf8427e+0 0:0:x\n", name: 'foo')
+      c.update_attributes! expires_at: (t0 + 1.hours)
+      c.reload
+      sig_exp = /\+A[0-9a-f]{40}\@([0-9]+)/.match(c.signed_manifest_text)[1].to_i
+      assert_operator sig_exp.to_i, :<=, (t0 + 1.hours).to_i
+    end
+  end
+
+  test 'far-future expiry date cannot be used to circumvent configured permission ttl' do
+    act_as_user users(:active) do
+      c = Collection.create!(manifest_text: ". d41d8cd98f00b204e9800998ecf8427e+0 0:0:x\n",
+                             name: 'foo',
+                             expires_at: db_current_time + 1.years)
+      sig_exp = /\+A[0-9a-f]{40}\@([0-9]+)/.match(c.signed_manifest_text)[1].to_i
+      expect_max_sig_exp = db_current_time.to_i + Rails.configuration.blob_signature_ttl
+      assert_operator c.expires_at.to_i, :>, expect_max_sig_exp
+      assert_operator sig_exp.to_i, :<=, expect_max_sig_exp
+    end
+  end
+
   test "create collection with properties" do
     act_as_system_user do
       c = Collection.create(manifest_text: ". acbd18db4cc2f85cedef654fccc4a4d8+3 0:3:foo\n",