FIX: Use dualstack S3 endpoint for direct uploads (#29611)

When we added direct S3 uploads to Discourse, which use
presigned URLs, we never took into account the dualstack
endpoints for IPv6 on S3.

This commit fixes the issue by using the dualstack endpoints
for presigned URLs and requests, which are used in the
get-presigned-put and batch-presign-urls endpoints used when
directly uploading to S3.

It also makes regular S3 requests for `put` and so on use
dualstack URLs. It doesn't seem like there is a downside to
doing this, but a bunch of specs needed to be updated to reflect this.

app/models/site_setting.rb

@@ -168,6 +168,10 @@ class SiteSetting < ActiveRecord::Base
       end
     end
 
+    def self.use_dualstack_endpoint
+      !SiteSetting.Upload.s3_region.start_with?("cn-")
+    end
+
     def self.enable_s3_uploads
       SiteSetting.enable_s3_uploads || GlobalSetting.use_s3?
     end
@@ -190,10 +194,10 @@ class SiteSetting < ActiveRecord::Base
 
       # cf. http://docs.aws.amazon.com/general/latest/gr/rande.html#s3_region
       if SiteSetting.s3_endpoint.blank? || SiteSetting.s3_endpoint.end_with?("amazonaws.com")
-        if SiteSetting.Upload.s3_region.start_with?("cn-")
-          "//#{bucket}.s3.#{SiteSetting.Upload.s3_region}.amazonaws.com.cn"
-        else
+        if SiteSetting.Upload.use_dualstack_endpoint
           "//#{bucket}.s3.dualstack.#{SiteSetting.Upload.s3_region}.amazonaws.com"
+        else
+          "//#{bucket}.s3.#{SiteSetting.Upload.s3_region}.amazonaws.com.cn"
         end
       else
         "//#{bucket}.#{url_basename}"

lib/s3_helper.rb

@@ -50,6 +50,7 @@ class S3Helper
     options[:client] = s3_client if s3_client.present?
     options[:use_accelerate_endpoint] = !for_backup &&
       SiteSetting.Upload.enable_s3_transfer_acceleration
+    options[:use_dualstack_endpoint] = SiteSetting.Upload.use_dualstack_endpoint
 
     bucket =
       if for_backup
@@ -265,6 +266,7 @@ class S3Helper
 
     opts[:endpoint] = SiteSetting.s3_endpoint if SiteSetting.s3_endpoint.present?
     opts[:http_continue_timeout] = SiteSetting.s3_http_continue_timeout
+    opts[:use_dualstack_endpoint] = SiteSetting.Upload.use_dualstack_endpoint
 
     unless obj.s3_use_iam_profile
       opts[:access_key_id] = obj.s3_access_key_id
@@ -362,6 +364,7 @@ class S3Helper
         key: key,
         expires_in: expires_in,
         use_accelerate_endpoint: @s3_options[:use_accelerate_endpoint],
+        use_dualstack_endpoint: @s3_options[:use_dualstack_endpoint],
       }.merge(opts),
     )
   end
@@ -380,6 +383,7 @@ class S3Helper
         key: key,
         expires_in: expires_in,
         use_accelerate_endpoint: @s3_options[:use_accelerate_endpoint],
+        use_dualstack_endpoint: @s3_options[:use_dualstack_endpoint],
       }.merge(opts),
     )
   end

spec/lib/backup_restore/s3_backup_store_spec.rb

@@ -82,9 +82,8 @@ RSpec.describe BackupRestore::S3BackupStore do
       end,
     )
 
+    setup_s3
     SiteSetting.s3_backup_bucket = "s3-backup-bucket"
-    SiteSetting.s3_access_key_id = "s3-access-key-id"
-    SiteSetting.s3_secret_access_key = "s3-secret-access-key"
     SiteSetting.backup_location = BackupLocationSiteSetting::S3
   end
 

spec/lib/backup_restore/shared_examples_for_backup_store.rb

@@ -171,16 +171,14 @@ RSpec.shared_examples "backup store" do
       end
 
       it "runs if SiteSetting.automatic_backups_enabled? is true" do
-        stub_request(
-          :get,
-          "https://s3-backup-bucket.s3.amazonaws.com/?list-type=2&prefix=default/",
-        ).to_return(status: 200, body: "", headers: {})
-        stub_request(:head, "https://s3-backup-bucket.s3.amazonaws.com/").to_return(
+        base_backup_s3_url = "https://s3-backup-bucket.s3.dualstack.us-west-1.amazonaws.com"
+        stub_request(:get, "#{base_backup_s3_url}/?list-type=2&prefix=default/").to_return(
           status: 200,
           body: "",
           headers: {
           },
         )
+        stub_request(:head, "#{base_backup_s3_url}/").to_return(status: 200, body: "", headers: {})
 
         SiteSetting.automatic_backups_enabled = true
         scheduleBackup = Jobs::ScheduleBackup.new

spec/lib/s3_helper_spec.rb

@@ -41,10 +41,10 @@ RSpec.describe "S3Helper" do
 
     stub_request(
       :get,
-      "https://bob.s3.#{SiteSetting.s3_region}.amazonaws.com/?lifecycle",
+      "https://bob.s3.dualstack.#{SiteSetting.s3_region}.amazonaws.com/?lifecycle",
     ).to_return(status: 200, body: @lifecycle, headers: {})
 
-    stub_request(:put, "https://bob.s3.#{SiteSetting.s3_region}.amazonaws.com/?lifecycle")
+    stub_request(:put, "https://bob.s3.dualstack.#{SiteSetting.s3_region}.amazonaws.com/?lifecycle")
       .with do |req|
         hash = Hash.from_xml(req.body.to_s)
         rules = hash["LifecycleConfiguration"]["Rule"]
@@ -248,4 +248,42 @@ RSpec.describe "S3Helper" do
       s3_helper.delete_objects(%w[object/one.txt object/two.txt])
     end
   end
+
+  describe "#presigned_url" do
+    let(:s3_helper) { S3Helper.new("test-bucket", "", client: client) }
+
+    it "uses the S3 dualstack endpoint" do
+      expect(s3_helper.presigned_url("test/key.jpeg", method: :get_object)).to include("dualstack")
+    end
+
+    context "for a China S3 region" do
+      before { SiteSetting.s3_region = "cn-northwest-1" }
+
+      it "does not use the S3 dualstack endpoint" do
+        expect(s3_helper.presigned_url("test/key.jpeg", method: :get_object)).not_to include(
+          "dualstack",
+        )
+      end
+    end
+  end
+
+  describe "#presigned_request" do
+    let(:s3_helper) { S3Helper.new("test-bucket", "", client: client) }
+
+    it "uses the S3 dualstack endpoint" do
+      expect(s3_helper.presigned_request("test/key.jpeg", method: :get_object)[0]).to include(
+        "dualstack",
+      )
+    end
+
+    context "for a China S3 region" do
+      before { SiteSetting.s3_region = "cn-northwest-1" }
+
+      it "does not use the S3 dualstack endpoint" do
+        expect(s3_helper.presigned_request("test/key.jpeg", method: :get_object)[0]).not_to include(
+          "dualstack",
+        )
+      end
+    end
+  end
 end

spec/models/optimized_image_spec.rb

@@ -334,7 +334,7 @@ RSpec.describe OptimizedImage do
             )
             stub_request(
               :put,
-              %r{https://#{SiteSetting.s3_upload_bucket}\.s3\.#{SiteSetting.s3_region}\.amazonaws.com#{optimized_path}},
+              %r{https://#{SiteSetting.s3_upload_bucket}\.s3\.dualstack\.#{SiteSetting.s3_region}\.amazonaws.com#{optimized_path}},
             ).to_return(status: 200, headers: { "ETag" => "someetag" })
           end
 

spec/requests/admin/backups_controller_spec.rb

@@ -880,15 +880,13 @@ RSpec.describe Admin::BackupsController do
       SiteSetting.enable_direct_s3_uploads = true
       SiteSetting.s3_backup_bucket = "s3-backup-bucket"
       SiteSetting.backup_location = BackupLocationSiteSetting::S3
-      stub_request(:head, "https://s3-backup-bucket.s3.us-west-1.amazonaws.com/").to_return(
-        status: 200,
-        body: "",
-        headers: {
-        },
-      )
       stub_request(
         :head,
-        "https://s3-backup-bucket.s3.us-west-1.amazonaws.com/default/test.tar.gz",
+        "https://s3-backup-bucket.s3.dualstack.us-west-1.amazonaws.com/",
+      ).to_return(status: 200, body: "", headers: {})
+      stub_request(
+        :head,
+        "https://s3-backup-bucket.s3.dualstack.us-west-1.amazonaws.com/default/test.tar.gz",
       ).to_return(backup_file_exists_response)
     end
 
@@ -936,7 +934,7 @@ RSpec.describe Admin::BackupsController do
         XML
         stub_request(
           :post,
-          "https://s3-backup-bucket.s3.us-west-1.amazonaws.com/temp/default/#{test_bucket_prefix}/28fccf8259bbe75b873a2bd2564b778c/2u98j832nx93272x947823.gz?uploads",
+          "https://s3-backup-bucket.s3.dualstack.us-west-1.amazonaws.com/temp/default/#{test_bucket_prefix}/28fccf8259bbe75b873a2bd2564b778c/2u98j832nx93272x947823.gz?uploads",
         ).to_return(status: 200, body: create_multipart_result)
       end
 

spec/requests/uploads_controller_spec.rb

@@ -874,6 +874,7 @@ RSpec.describe UploadsController do
         expect(result["key"]).to include(FileStore::S3Store::TEMPORARY_UPLOAD_PREFIX)
         expect(result["url"]).to include(FileStore::S3Store::TEMPORARY_UPLOAD_PREFIX)
         expect(result["url"]).to include("Amz-Expires")
+        expect(result["url"]).to include("dualstack")
       end
 
       it "includes accepted metadata in the response when provided" do
@@ -1040,7 +1041,7 @@ RSpec.describe UploadsController do
         XML
         stub_request(
           :post,
-          "https://s3-upload-bucket.s3.us-west-1.amazonaws.com/uploads/default/#{test_bucket_prefix}/temp/28fccf8259bbe75b873a2bd2564b778c/test.png?uploads",
+          "https://s3-upload-bucket.s3.dualstack.us-west-1.amazonaws.com/uploads/default/#{test_bucket_prefix}/temp/28fccf8259bbe75b873a2bd2564b778c/test.png?uploads",
         ).to_return({ status: 200, body: create_multipart_result })
       end
 
@@ -1184,7 +1185,7 @@ RSpec.describe UploadsController do
         XML
         stub_request(
           :get,
-          "https://s3-upload-bucket.#{SiteSetting.enable_s3_transfer_acceleration ? "s3-accelerate" : "s3.us-west-1"}.amazonaws.com/#{external_upload_stub.key}?max-parts=1&uploadId=#{mock_multipart_upload_id}",
+          "https://s3-upload-bucket.#{SiteSetting.enable_s3_transfer_acceleration ? "s3-accelerate.dualstack" : "s3.dualstack.us-west-1"}.amazonaws.com/#{external_upload_stub.key}?max-parts=1&uploadId=#{mock_multipart_upload_id}",
         ).to_return({ status: 200, body: list_multipart_result })
       end
 
@@ -1268,6 +1269,19 @@ RSpec.describe UploadsController do
         )
       end
 
+      it "uses dualstack endpoint for presigned URLs based on S3 region" do
+        stub_list_multipart_request
+        post "/uploads/batch-presign-multipart-parts.json",
+             params: {
+               unique_identifier: external_upload_stub.unique_identifier,
+               part_numbers: [2, 3, 4],
+             }
+
+        expect(response.status).to eq(200)
+        result = response.parsed_body
+        expect(result["presigned_urls"]["2"]).to include("dualstack")
+      end
+
       context "when enable_s3_transfer_acceleration is true" do
         before { SiteSetting.enable_s3_transfer_acceleration = true }
 
@@ -1328,7 +1342,7 @@ RSpec.describe UploadsController do
 
   describe "#complete_multipart" do
     let(:upload_base_url) do
-      "https://#{SiteSetting.s3_upload_bucket}.#{SiteSetting.enable_s3_transfer_acceleration ? "s3-accelerate" : "s3.#{SiteSetting.s3_region}"}.amazonaws.com"
+      "https://#{SiteSetting.s3_upload_bucket}.#{SiteSetting.enable_s3_transfer_acceleration ? "s3-accelerate.dualstack" : "s3.dualstack.#{SiteSetting.s3_region}"}.amazonaws.com"
     end
     let(:mock_multipart_upload_id) do
       "ibZBv_75gd9r8lH_gqXatLdxMVpAlj6CFTR.OwyF3953YdwbcQnMA2BLGn8Lx12fQNICtMw5KyteFeHw.Sjng--"
@@ -1549,7 +1563,7 @@ RSpec.describe UploadsController do
 
   describe "#abort_multipart" do
     let(:upload_base_url) do
-      "https://#{SiteSetting.s3_upload_bucket}.#{SiteSetting.enable_s3_transfer_acceleration ? "s3-accelerate" : "s3.#{SiteSetting.s3_region}"}.amazonaws.com"
+      "https://#{SiteSetting.s3_upload_bucket}.#{SiteSetting.enable_s3_transfer_acceleration ? "s3-accelerate.dualstack" : "s3.dualstack.#{SiteSetting.s3_region}"}.amazonaws.com"
     end
     let(:mock_multipart_upload_id) do
       "ibZBv_75gd9r8lH_gqXatLdxMVpAlj6CFTR.OwyF3953YdwbcQnMA2BLGn8Lx12fQNICtMw5KyteFeHw.Sjng--"

spec/support/system_helpers.rb

@@ -139,6 +139,10 @@ module SystemHelpers
     SiteSetting.s3_secret_access_key = MinioRunner.config.minio_root_password
     SiteSetting.s3_endpoint = MinioRunner.config.minio_server_url
 
+    # This is necessary for Minio because you cannot use dualstack
+    # at the same time as using a custom S3 endpoint.
+    SiteSetting.Upload.stubs(:use_dualstack_endpoint).returns(false)
+
     SiteSetting.enable_direct_s3_uploads = enable_direct_s3_uploads
     SiteSetting.secure_uploads = enable_secure_uploads
 

spec/support/uploads_helpers.rb

@@ -10,9 +10,11 @@ module UploadsHelpers
     SiteSetting.s3_access_key_id = "some key"
     SiteSetting.s3_secret_access_key = "some secrets3_region key"
 
+    dualstack = SiteSetting.Upload.use_dualstack_endpoint ? ".dualstack" : ""
+
     stub_request(
       :head,
-      "https://#{SiteSetting.s3_upload_bucket}.s3.#{SiteSetting.s3_region}.amazonaws.com/",
+      "https://#{SiteSetting.s3_upload_bucket}.s3#{dualstack}.#{SiteSetting.s3_region}.amazonaws.com/",
     )
   end
 
@@ -22,8 +24,10 @@ module UploadsHelpers
   end
 
   def stub_upload(upload)
+    dualstack = SiteSetting.Upload.use_dualstack_endpoint ? ".dualstack" : ""
+
     url =
-      %r{https://#{SiteSetting.s3_upload_bucket}.s3.#{SiteSetting.s3_region}.amazonaws.com/original/\d+X.*#{upload.sha1}.#{upload.extension}\?acl}
+      %r{https://#{SiteSetting.s3_upload_bucket}.s3#{dualstack}.#{SiteSetting.s3_region}.amazonaws.com/original/\d+X.*#{upload.sha1}.#{upload.extension}\?acl}
     stub_request(:put, url)
   end