Support AWS S3 FIPS endpoints #2763
Conversation
matt-domsch-sp
force-pushed
the
govcloud-fips-entrypoint
branch
2 times, most recently
from
782e5ca to
d3f5f9f
Compare
S3 endpoints even when ENV['AWS_USE_FIPS_ENDPOINT']=='true'. When FIPS is called for, and we are in a region where FIPS endpoints are available, this method should return the FIPS endpoint. Furthermore, when S3 Transfer Acceleration (S3TA) is requested by configuration, the above endpoint gets overridden to select the S3TA endpoint. However, S3TA is not avaialble in GovCloud, and has no FIPS endpoint equivalents. In this instance, if the region is a GovCloud region, or if FIPS mode is called for, do not override the endpoint to use S3TA. This is functionally equivalent to an issue submitted to the fog-aws project. fog/fog-aws#729
matt-domsch-sp
force-pushed
the
govcloud-fips-entrypoint
branch
from
d3f5f9f to
3c8ad1f
Compare
The code can be simplified if we move the test for GovCloud or FIPS outside of the if use_virtual_hosted_style block. This makes it explicit what it's trying to do - which is disable the use of acceleration. We won't emit a warning here. The corresponding code going into the fog-aws gem will emit a warning once that code is merged. We don't need two warnings for the same thing.
The fog-aws gem won't emit a warning if we've disabled S3TA ourselves. It would only emit the warning if _it_ disabled S3TA using the same test. Therefore, we want the same warning emitted in Carrierwave as is in fog-aws after all.
|
The equivalent patch to fog-aws has been merged into that upstream. |
lib/carrierwave/storage/fog.rb
Outdated
|
|
||
| # 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') |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
ok, I can remove this section.
lib/carrierwave/storage/fog.rb
Outdated
| "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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
A change needs to have corresponding RSpec test examples, so it doesn't break unintentionally in future. |
matt-domsch-sp
force-pushed
the
govcloud-fips-entrypoint
branch
3 times, most recently
from
815051c to
e1a2453
Compare
This adds the [:fog_aws_fips] option, default false. When true, it causes the default endpoint hostname to be changed from `s3` to `s3-fips`.
matt-domsch-sp
force-pushed
the
govcloud-fips-entrypoint
branch
from
e1a2453 to
ccd6603
Compare
|
Thank you for the review and feedback. Code cleaned up, :fog_aws_fips option and specs added. The added specs fail the Cyclomatic complexity rule. Please advise how you'd like this to be handled. |
matt-domsch-sp
force-pushed
the
govcloud-fips-entrypoint
branch
2 times, most recently
from
a78252b to
6c80ddb
Compare
public_url returns a path-based URL when :endpoint is given. AWS FIPS endpoints only work with virtual host-style URLs per https://aws.amazon.com/compliance/fips/ Add a warning and return nil if both :endpoint is given and :fog_aws_fips=true. Add tests for :endpoint for both :fog_aws_fips=false (default) and true.
matt-domsch-sp
force-pushed
the
govcloud-fips-entrypoint
branch
from
6c80ddb to
0d5a661
Compare
mshibuya
changed the title
|
OK I can take care of the rest. Thank you! |
- Reorganize spec/storage/fog_helper.rb so it doesn't have high cyclomatic complexity
- Raises an error instead of returning nil - Reorganize spec/storage/fog_helper.rb so it doesn't have high cyclomatic complexity
S3 endpoints even when ENV['AWS_USE_FIPS_ENDPOINT']=='true'. When FIPS is called for, and we are in a region where FIPS endpoints are available, this method should return the FIPS endpoint.
Furthermore, when S3 Transfer Acceleration (S3TA) is requested by configuration, the above endpoint gets overridden to select the S3TA endpoint. However, S3TA is not avaialble in GovCloud, and has no FIPS endpoint equivalents. In this instance, if the region is a GovCloud region, or if FIPS mode is called for, do not override the endpoint to use S3TA.
This is functionally equivalent to an issue submitted to the fog-aws project. fog/fog-aws#729