FEATURE: allow S3 ACLs to be disabled (#21769)

AWS recommends running buckets without ACLs, and to use resource policies to manage access control instead.
This is not a bad idea, because S3 ACLs are whack, and while resource policies are also whack, they're a more constrained form of whack.
Further, some compliance regimes get antsy if you don't go with the vendor's recommended settings, and arguing that you need to enable ACLs on a bucket just to store images in there is more hassle than it's worth.
The new site setting (s3_use_acls) cannot be disabled when secure
uploads is enabled -- the latter relies on private ACLs for security
at this point in time. We may want to reexamine this in future.

config/locales/server.en.yml

@@ -216,7 +216,8 @@ en:
       page_publishing_requirements: "Page publishing cannot be enabled if secure media is enabled."
       s3_backup_requires_s3_settings: "You cannot use S3 as backup location unless you've provided the '%{setting_name}'."
       s3_bucket_reused: "You cannot use the same bucket for 's3_upload_bucket' and 's3_backup_bucket'. Choose a different bucket or use a different path for each bucket."
-      secure_uploads_requirements: "S3 uploads must be enabled before enabling secure uploads."
+      secure_uploads_requirements: "S3 uploads and S3 ACLs must be enabled before enabling secure uploads."
+      s3_use_acls_requirements: "You must have S3 ACLs enabled when secure uploads are enabled."
       share_quote_facebook_requirements: "You must set a Facebook app id to enable quote sharing for Facebook."
       second_factor_cannot_enforce_with_socials: "You cannot enforce 2FA with social logins enabled. You must first disable login via: %{auth_provider_names}"
       second_factor_cannot_be_enforced_with_disabled_local_login: "You cannot enforce 2FA if local logins are disabled."
@@ -1793,6 +1794,7 @@ en:
     s3_configure_tombstone_policy: "Enable automatic deletion policy for tombstone uploads. IMPORTANT: If disabled, no space will be reclaimed after uploads are deleted."
     s3_disable_cleanup: "Prevent removal of old backups from S3 when there are more backups than the maximum allowed."
     enable_s3_inventory: "Generate reports and verify uploads using Amazon S3 inventory. IMPORTANT: requires valid S3 credentials (both access key id & secret access key)."
+    s3_use_acls: "AWS recommends not using ACLs on S3 buckets; if you are following this advice, uncheck this option. This must be enabled if you are using secure uploads."
     backup_time_of_day: "Time of day UTC when the backup should occur."
     backup_with_uploads: "Include uploads in scheduled backups. Disabling this will only backup the database."
     backup_location: "Location where backups are stored. IMPORTANT: S3 requires valid S3 credentials entered in Files settings."

config/site_settings.yml

@@ -1450,6 +1450,8 @@ files:
     regex: '^https?:\/\/.+[^\/]$'
   s3_configure_tombstone_policy:
     default: true
+  s3_use_acls:
+    default: true
   enable_s3_inventory:
     default: false
   s3_configure_inventory_policy:

lib/backup_restore/s3_backup_store.rb

@@ -80,7 +80,7 @@ module BackupRestore
         expires_in: expires_in,
         opts: {
           metadata: metadata,
-          acl: "private",
+          acl: SiteSetting.s3_use_acls ? "private" : nil,
         },
       )
     end
@@ -115,7 +115,7 @@ module BackupRestore
         existing_external_upload_key,
         File.join(s3_helper.s3_bucket_folder_path, original_filename),
         options: {
-          acl: "private",
+          acl: SiteSetting.s3_use_acls ? "private" : nil,
           apply_metadata_to_destination: true,
         },
       )

lib/file_store/s3_store.rb

@@ -87,7 +87,7 @@ module FileStore
       # cache file locally when needed
       cache_file(file, File.basename(path)) if opts[:cache_locally]
       options = {
-        acl: opts[:private_acl] ? "private" : "public-read",
+        acl: SiteSetting.s3_use_acls ? (opts[:private_acl] ? "private" : "public-read") : nil,
         cache_control: "max-age=31556952, public, immutable",
         content_type:
           opts[:content_type].presence || MiniMime.lookup_by_filename(filename)&.content_type,
@@ -262,7 +262,7 @@ module FileStore
         expires_in: expires_in,
         opts: {
           metadata: metadata,
-          acl: "private",
+          acl: SiteSetting.s3_use_acls ? "private" : nil,
         },
       )
     end
@@ -397,7 +397,9 @@ module FileStore
 
     def update_ACL(key, secure)
       begin
-        object_from_path(key).acl.put(acl: secure ? "private" : "public-read")
+        object_from_path(key).acl.put(
+          acl: SiteSetting.s3_use_acls ? (secure ? "private" : "public-read") : nil,
+        )
       rescue Aws::S3::Errors::NoSuchKey
         Rails.logger.warn("Could not update ACL on upload with key: '#{key}'. Upload is missing.")
       end

lib/file_store/to_s3_migration.rb

@@ -241,7 +241,7 @@ module FileStore
         end
 
         options = {
-          acl: "public-read",
+          acl: SiteSetting.s3_use_acls ? "public-read" : nil,
           body: File.open(path, "rb"),
           bucket: bucket,
           content_type: MiniMime.lookup_by_filename(name)&.content_type,

lib/s3_helper.rb

@@ -293,7 +293,7 @@ class S3Helper
   def create_multipart(key, content_type, metadata: {})
     response =
       s3_client.create_multipart_upload(
-        acl: "private",
+        acl: SiteSetting.s3_use_acls ? "private" : nil,
         bucket: s3_bucket_name,
         key: key,
         content_type: content_type,

lib/site_settings/validations.rb

@@ -168,11 +168,15 @@ module SiteSettings::Validations
   end
 
   def validate_secure_uploads(new_val)
-    if new_val == "t" && !SiteSetting.Upload.enable_s3_uploads
+    if new_val == "t" && (!SiteSetting.Upload.enable_s3_uploads || !SiteSetting.s3_use_acls)
       validate_error :secure_uploads_requirements
     end
   end
 
+  def validate_s3_use_acls(new_val)
+    validate_error :s3_use_acls_requirements if new_val == "f" && SiteSetting.secure_uploads
+  end
+
   def validate_enable_page_publishing(new_val)
     validate_error :page_publishing_requirements if new_val == "t" && SiteSetting.secure_uploads?
   end

lib/tasks/s3.rake

@@ -28,7 +28,7 @@ def upload(path, remote_path, content_type, content_encoding = nil)
   options = {
     cache_control: "max-age=31556952, public, immutable",
     content_type: content_type,
-    acl: "public-read",
+    acl: SiteSetting.s3_use_acls ? "public-read" : nil,
   }
 
   options[:content_encoding] = content_encoding if content_encoding
@@ -104,6 +104,11 @@ end
 task "s3:correct_acl" => :environment do
   ensure_s3_configured!
 
+  if !SiteSetting.s3_use_acls
+    $stderr.puts "Not correcting ACLs as the site is configured to not use ACLs"
+    return
+  end
+
   puts "ensuring public-read is set on every upload and optimized image"
 
   i = 0
@@ -158,7 +163,7 @@ task "s3:correct_cachecontrol" => :environment do
         object = Discourse.store.s3_helper.object(key)
         object.copy_from(
           copy_source: "#{object.bucket_name}/#{object.key}",
-          acl: "public-read",
+          acl: SiteSetting.s3_use_acls ? "public-read" : nil,
           cache_control: cache_control,
           content_type: object.content_type,
           content_disposition: object.content_disposition,

lib/upload_recovery.rb

@@ -150,7 +150,13 @@ class UploadRecovery
           old_key = key
           key = key.sub(tombstone_prefix, "")
 
-          Discourse.store.s3_helper.copy(old_key, key, options: { acl: "public-read" })
+          Discourse.store.s3_helper.copy(
+            old_key,
+            key,
+            options: {
+              acl: SiteSetting.s3_use_acls ? "public-read" : nil,
+            },
+          )
         end
 
         next if upload_exists

spec/lib/file_store/s3_store_spec.rb

@@ -130,6 +130,38 @@ RSpec.describe FileStore::S3Store do
           expect(store.url_for(upload)).to eq(upload.url)
         end
       end
+
+      describe "when ACLs are disabled" do
+        it "doesn't supply an ACL" do
+          SiteSetting.s3_use_acls = false
+          SiteSetting.authorized_extensions = "pdf|png|jpg|gif"
+          upload =
+            Fabricate(:upload, original_filename: "small.pdf", extension: "pdf", secure: true)
+
+          s3_helper.expects(:s3_bucket).returns(s3_bucket)
+          s3_bucket
+            .expects(:object)
+            .with(regexp_matches(%r{original/\d+X.*/#{upload.sha1}\.pdf}))
+            .returns(s3_object)
+          s3_object
+            .expects(:put)
+            .with(
+              {
+                acl: nil,
+                cache_control: "max-age=31556952, public, immutable",
+                content_type: "application/pdf",
+                content_disposition:
+                  "attachment; filename=\"#{upload.original_filename}\"; filename*=UTF-8''#{upload.original_filename}",
+                body: uploaded_file,
+              },
+            )
+            .returns(Aws::S3::Types::PutObjectOutput.new(etag: "\"#{etag}\""))
+
+          expect(store.store_upload(uploaded_file, upload)).to match(
+            %r{//s3-upload-bucket\.s3\.dualstack\.us-west-1\.amazonaws\.com/original/\d+X.*/#{upload.sha1}\.pdf},
+          )
+        end
+      end
     end
 
     describe "#store_optimized_image" do

spec/lib/site_settings/validations_spec.rb

@@ -265,10 +265,36 @@ RSpec.describe SiteSettings::Validations do
       end
     end
 
+    describe "#validate_s3_use_acls" do
+      context "when the new value is true" do
+        it "is ok" do
+          expect { subject.validate_s3_use_acls("t") }.not_to raise_error
+        end
+      end
+
+      context "when the new value is false" do
+        it "is ok" do
+          expect { subject.validate_s3_use_acls("f") }.not_to raise_error
+        end
+
+        context "if secure uploads is enabled" do
+          let(:error_message) { I18n.t("errors.site_settings.s3_use_acls_requirements") }
+          before { enable_secure_uploads }
+
+          it "is not ok" do
+            expect { subject.validate_s3_use_acls("f") }.to raise_error(
+              Discourse::InvalidParameters,
+              error_message,
+            )
+          end
+        end
+      end
+    end
+
     describe "#validate_secure_uploads" do
       let(:error_message) { I18n.t("errors.site_settings.secure_uploads_requirements") }
 
-      context "when the new value is true" do
+      context "when the new secure uploads value is true" do
         context "if site setting for enable_s3_uploads is enabled" do
           before { SiteSetting.enable_s3_uploads = true }
 
@@ -295,6 +321,17 @@ RSpec.describe SiteSettings::Validations do
             end
           end
         end
+
+        context "if site setting for s3_use_acls is not enabled" do
+          before { SiteSetting.s3_use_acls = false }
+
+          it "is not ok" do
+            expect { subject.validate_secure_uploads("t") }.to raise_error(
+              Discourse::InvalidParameters,
+              error_message,
+            )
+          end
+        end
       end
     end