Merge branch 'sh-fix-container-registry-s3-redirects' into 'master'

Properly handle container registry redirects to fix metadata stored on an S3 backend

Closes #22403

See merge request !11429

4 files changed, 54 insertions, 5 deletions

diff --git a/changelogs/unreleased/sh-fix-container-registry-s3-redirects.yml b/changelogs/unreleased/sh-fix-container-registry-s3-redirects.yml
new file mode 100644
index 00000000000..1e783811b66
--- /dev/null
+++ b/changelogs/unreleased/sh-fix-container-registry-s3-redirects.yml
@@ -0,0 +1,4 @@
+---
+title: Properly handle container registry redirects to fix metadata stored on a S3 backend
+merge_request:
+author:
diff --git a/lib/container_registry/client.rb b/lib/container_registry/client.rb
index 7f5f6d9ddb6..c7263f302ab 100644
--- a/lib/container_registry/client.rb
+++ b/lib/container_registry/client.rb
@@ -75,10 +75,7 @@ module ContainerRegistry
     def redirect_response(location)
       return unless location
 
-      # We explicitly remove authorization token
-      faraday_blob.get(location) do |req|
-        req['Authorization'] = ''
-      end
+      faraday_redirect.get(location)
     end
 
     def faraday
@@ -93,5 +90,14 @@ module ContainerRegistry
         initialize_connection(conn, @options)
       end
     end
+
+    # Create a new request to make sure the Authorization header is not inserted
+    # via the Faraday middleware
+    def faraday_redirect
+      @faraday_redirect ||= Faraday.new(@base_uri) do |conn|
+        conn.request :json
+        conn.adapter :net_http
+      end
+    end
   end
 end
diff --git a/spec/lib/container_registry/blob_spec.rb b/spec/lib/container_registry/blob_spec.rb
index f06e5fd54a2..ab010c6dfeb 100644
--- a/spec/lib/container_registry/blob_spec.rb
+++ b/spec/lib/container_registry/blob_spec.rb
@@ -98,7 +98,7 @@ describe ContainerRegistry::Blob do
       context 'for a valid address' do
         before do
           stub_request(:get, location).
-            with(headers: { 'Authorization' => nil }).
+            with { |request| !request.headers.include?('Authorization') }.
             to_return(
               status: 200,
               headers: { 'Content-Type' => 'application/json' },
diff --git a/spec/lib/container_registry/client_spec.rb b/spec/lib/container_registry/client_spec.rb
new file mode 100644
index 00000000000..ec03b533383
--- /dev/null
+++ b/spec/lib/container_registry/client_spec.rb
@@ -0,0 +1,39 @@
+# coding: utf-8
+require 'spec_helper'
+
+describe ContainerRegistry::Client do
+  let(:token) { '12345' }
+  let(:options) { { token: token } }
+  let(:client) { described_class.new("http://container-registry", options) }
+
+  describe '#blob' do
+    it 'GET /v2/:name/blobs/:digest' do
+      stub_request(:get, "http://container-registry/v2/group/test/blobs/sha256:0123456789012345").
+        with(headers: {
+               'Accept' => 'application/octet-stream',
+               'Authorization' => "bearer #{token}"
+             }).
+        to_return(status: 200, body: "Blob")
+
+      expect(client.blob('group/test', 'sha256:0123456789012345')).to eq('Blob')
+    end
+
+    it 'follows 307 redirect for GET /v2/:name/blobs/:digest' do
+      stub_request(:get, "http://container-registry/v2/group/test/blobs/sha256:0123456789012345").
+        with(headers: {
+               'Accept' => 'application/octet-stream',
+               'Authorization' => "bearer #{token}"
+             }).
+        to_return(status: 307, body: "", headers: { Location: 'http://redirected' })
+      # We should probably use hash_excluding here, but that requires an update to WebMock:
+      # https://github.com/bblimke/webmock/blob/master/lib/webmock/matchers/hash_excluding_matcher.rb
+      stub_request(:get, "http://redirected/").
+        with { |request| !request.headers.include?('Authorization') }.
+        to_return(status: 200, body: "Successfully redirected")
+
+      response = client.blob('group/test', 'sha256:0123456789012345')
+
+      expect(response).to eq('Successfully redirected')
+    end
+  end
+end