Skip to content
This repository has been archived by the owner on Nov 27, 2024. It is now read-only.

DEVOPS-15838: add checksum header for S3 storage #2

Open
wants to merge 1 commit into
base: CE-14395_v5.2.0
Choose a base branch
from

Conversation

mn-coupa
Copy link

JIRA

Complete the URLs for the main JIRA ticket and propagation subtask.

Code reviewers

@jmathadu @PNarode

Summary of issue

  • Paperclip upload to S3 fails due to missing checksum header
  • The checksum header is required for Object Lock with Retention enabled
  • Default MD5 checksum cannot be used due to FIPS mode

Summary of change

  • Overwrite the PaperClip S3 storage to add checksum SHA256

Testing approach

Use Control UI to upload CSV file
The file is saved to paperclip-us-east-1-coupadev-com bucket, which has Object Lock enabled
Here is the S3 link
https://us-east-1.console.aws.amazon.com/s3/buckets/paperclip-us-east-1-coupadev-com?region=us-east-1&bucketType=general&prefix=control-ops-qa.coupadev.com/data_sources/files/000/000/112/original/&showversions=false

@@ -387,6 +387,10 @@ def flush_writes #:nodoc:
write_options[:metadata] = @s3_metadata unless @s3_metadata.empty?
write_options.merge!(@s3_headers)

sha256_checksum = Digest::SHA256.hexdigest(File.read(file.path))
encoded_sha256_checksum = Base64.encode64([sha256_checksum].pack("H*")).strip
write_options[:checksum_sha256] = encoded_sha256_checksum # Required for FIPS (SHA256) and object locking (checksum)

Choose a reason for hiding this comment

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

@charchitarora isn't this already added by the aws-sdk-s3, is that what you meant by this is already being done

Copy link

@charchitarora charchitarora Nov 27, 2024

Choose a reason for hiding this comment

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

@abdulchaudhrycoupa - I was referring to - https://github.com/coupa-ops/paperclip/blob/CE-14395_v5.2.0/lib/paperclip/attachment.rb#L36 - is already there. We need not add this to paperclip initializer.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants