From a98d2a80860f75a677c56a8302d3e8b6e56f0df0 Mon Sep 17 00:00:00 2001 From: Matt Palmer Date: Tue, 6 Jun 2023 15:47:40 +1000 Subject: [PATCH] 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 | 4 ++- config/site_settings.yml | 2 ++ lib/backup_restore/s3_backup_store.rb | 4 +-- lib/file_store/s3_store.rb | 8 +++-- lib/file_store/to_s3_migration.rb | 2 +- lib/s3_helper.rb | 2 +- lib/site_settings/validations.rb | 6 +++- lib/tasks/s3.rake | 9 +++-- lib/upload_recovery.rb | 8 ++++- spec/lib/file_store/s3_store_spec.rb | 32 ++++++++++++++++++ spec/lib/site_settings/validations_spec.rb | 39 +++++++++++++++++++++- 11 files changed, 103 insertions(+), 13 deletions(-) diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 169ba4d97f7..b70090843eb 100644 --- a/config/locales/server.en.yml +++ b/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." diff --git a/config/site_settings.yml b/config/site_settings.yml index 7aa25a9c4d0..b4cefc973c9 100644 --- a/config/site_settings.yml +++ b/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: diff --git a/lib/backup_restore/s3_backup_store.rb b/lib/backup_restore/s3_backup_store.rb index 2237ccfa8b8..4949ebf7179 100644 --- a/lib/backup_restore/s3_backup_store.rb +++ b/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, }, ) diff --git a/lib/file_store/s3_store.rb b/lib/file_store/s3_store.rb index b458ebcc048..781c6ffb679 100644 --- a/lib/file_store/s3_store.rb +++ b/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 diff --git a/lib/file_store/to_s3_migration.rb b/lib/file_store/to_s3_migration.rb index 1d9de0cc25d..cb0b7e04e44 100644 --- a/lib/file_store/to_s3_migration.rb +++ b/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, diff --git a/lib/s3_helper.rb b/lib/s3_helper.rb index b8dc762c84c..b3d98c40a68 100644 --- a/lib/s3_helper.rb +++ b/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, diff --git a/lib/site_settings/validations.rb b/lib/site_settings/validations.rb index b048ed2ca9e..4a25bc9eda4 100644 --- a/lib/site_settings/validations.rb +++ b/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 diff --git a/lib/tasks/s3.rake b/lib/tasks/s3.rake index 56e5859dafe..7f5a7a4cad2 100644 --- a/lib/tasks/s3.rake +++ b/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, diff --git a/lib/upload_recovery.rb b/lib/upload_recovery.rb index c2e340d9693..5ac0e556a2b 100644 --- a/lib/upload_recovery.rb +++ b/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 diff --git a/spec/lib/file_store/s3_store_spec.rb b/spec/lib/file_store/s3_store_spec.rb index bd7494c34c7..29576c3f1f2 100644 --- a/spec/lib/file_store/s3_store_spec.rb +++ b/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 diff --git a/spec/lib/site_settings/validations_spec.rb b/spec/lib/site_settings/validations_spec.rb index 3822274306c..82064f35932 100644 --- a/spec/lib/site_settings/validations_spec.rb +++ b/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