Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support AWS S3 FIPS endpoints #2763

Merged
merged 7 commits into from
Dec 1, 2024
Merged

Support AWS S3 FIPS endpoints #2763

Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
3c8ad1f
The CarrierWave::Storage::File#public_url method returns the standard
matt-domsch-sp Nov 18, 2024
ae2a277
Move test for GovCloud or FIPS higher
matt-domsch-sp Nov 20, 2024
738a5ce
Add warning for S3TA disabling
matt-domsch-sp Nov 20, 2024
504bf0e
fix typo in warning message
matt-domsch-sp Nov 21, 2024
c35e37b
remove disabling S3TA
matt-domsch-sp Nov 23, 2024
ccd6603
Add fog_aws_fips config option, tests
matt-domsch-sp Nov 25, 2024
0d5a661
Return nil if both :endpoint given and :fog_aws_fips=true
matt-domsch-sp Nov 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 15 additions & 6 deletions lib/carrierwave/storage/fog.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,8 @@ def connection

class File
DEFAULT_S3_REGION = 'us-east-1'.freeze
AWS_FIPS_REGIONS = %w(us-east-1 us-east-2 us-west-1 us-west-2 us-gov-east-1 us-gov-west-1 ca-central-1 ca-west-1).freeze
AWS_GOVCLOUD_REGIONS = %w(us-gov-east-1 us-gov-west-1).freeze

include CarrierWave::Utilities::Uri
include CarrierWave::Utilities::FileName
Expand Down Expand Up @@ -383,12 +385,19 @@ def public_url
use_virtual_hosted_style = @uploader.fog_directory.to_s =~ subdomain_regex && !(protocol == 'https' && @uploader.fog_directory =~ /\./)

region = @uploader.fog_credentials[:region].to_s
regional_host = case region
when DEFAULT_S3_REGION, ''
's3.amazonaws.com'
else
"s3.#{region}.amazonaws.com"
end
regional_host = 's3.amazonaws.com' # used for DEFAULT_S3_REGION or no region set
if ENV['AWS_USE_FIPS_ENDPOINT'] == 'true' && AWS_FIPS_REGIONS.include?(region)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't use an environment variable here. You should create a config like fog_aws_fips, in a same manner with fog_aws_accelerate. You don't need to check AWS region here, just letting developer choose is enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the same environment variable that the AWS SDK uses to determine if it should choose the FIPS endpoints, hence my use of it here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CarrierWave is not the AWS SDK. Every software has its own way of configuration, and you need to follow it. Otherwise there'll be a mess.

regional_host = "s3-fips.#{region}.amazonaws.com" # https://aws.amazon.com/compliance/fips/
elsif ![DEFAULT_S3_REGION, ''].include?(region)
regional_host = "s3.#{region}.amazonaws.com"
end

# GovCloud doesn't support S3 Transfer Acceleration https://docs.aws.amazon.com/govcloud-us/latest/UserGuide/govcloud-s3.html
# S3 Transfer Acceleration doesn't support FIPS endpoints. When both fog_aws_accelerate=true and AWS_USE_FIPS_ENDPOINT=true, don't use Accelerate.
if @uploader.fog_aws_accelerate && (AWS_GOVCLOUD_REGIONS.include?(region) || ENV['AWS_USE_FIPS_ENDPOINT'] == 'true')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need to embed the knowledge about AWS regions into CarrierWave. If an app is using AWS regions that supports S3 Transfer Acceleration it can be set as config.fog_aws_accelerate = true, that's it. We should let developers decide.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, I can remove this section.

warn "S3 Transfer Acceleration is not available in GovCloud regions or when AWS_USE_FIPS_ENDPOINT=true. Disabling acceleration."
@uploader.fog_aws_accelerate = false
end

if use_virtual_hosted_style
regional_host = 's3-accelerate.amazonaws.com' if @uploader.fog_aws_accelerate
Expand Down