Allow encryption method to be configurable (#21)

https://eaflood.atlassian.net/browse/RUBY-1156

We recently switched the encryption method we use on the gem from AES256 to AWS:KMS. From our testing, there were no issues with this. However, after deploying this change to a live environment it has caused problems for 3rd parties trying to access our exported files.

They use different AWS credentials to access the files, and these credentials we have found need access to the same KMS key against our credentials (and used for encryption) to decrypt the files.

This has further implications that need consideration. So short term we need to revert back to AES256. Longer-term we would like to switch back.

So this change is about updating the gem to make the encryption method configurable.

README.md

@@ -41,7 +41,9 @@ DefraRuby::Aws.configure do |config|
       secret_access_key: "SECRET_ACCESS_KEY"
     },
     # optional - Default to "eu-west-1"
-    region: "eu-west-1"
+    region: "eu-west-2",
+    # optional - Default to false. Will use AES256
+    encrypt_with_kms: true
   }]
 end
 ```
@@ -71,6 +73,7 @@ presigned_url = bucket.presigned_url("test-upload-file.csv")
 ```
 
 ### Delete a file from the bucket
+
 ```ruby
 bucket = DefraRuby::Aws.get_bucket("defra-ruby-aws")
 response = bucket.delete("test-upload-file.csv")
@@ -93,7 +96,7 @@ All contributions should be submitted via a pull request.
 
 THIS INFORMATION IS LICENSED UNDER THE CONDITIONS OF THE OPEN GOVERNMENT LICENCE found at:
 
-http://www.nationalarchives.gov.uk/doc/open-government-licence/version/3
+<http://www.nationalarchives.gov.uk/doc/open-government-licence/version/3>
 
 The following attribution statement MUST be cited in your products and applications when using this information.
 

lib/defra_ruby/aws/bucket.rb

@@ -3,12 +3,13 @@
 module DefraRuby
   module Aws
     class Bucket
-      attr_reader :bucket_name, :credentials, :region
+      attr_reader :bucket_name, :credentials, :region, :encrypt_with_kms
 
       def initialize(configs)
         @credentials = configs[:credentials]
         @bucket_name = configs[:name]
         @region = setup_region(configs[:region])
+        @encrypt_with_kms = setup_encrypt_with_kms(configs[:encrypt_with_kms])
 
         validate!
       end
@@ -21,6 +22,10 @@ def secret_access_key
         credentials[:secret_access_key]
       end
 
+      def encryption_type
+        @_encryption_type ||= @encrypt_with_kms ? "aws:kms" : :AES256
+      end
+
       def load(file)
         BucketLoaderService.run(self, file)
       end
@@ -47,6 +52,14 @@ def default_region
         "eu-west-1"
       end
 
+      def setup_encrypt_with_kms(encrypt_with_kms)
+        return false if encrypt_with_kms.nil?
+
+        # Handle the argument being either a string or a boolean, or some other
+        # value e.g. "foo"
+        encrypt_with_kms.to_s.downcase == "true"
+      end
+
       def validate!
         validate_presence_of_name!
         validate_presence_of_credentials!

lib/defra_ruby/aws/services/bucket_loader_service.rb

@@ -24,7 +24,9 @@ def run
 
       def response_exe
         lambda do
-          s3_bucket.object(File.basename(file.path)).upload_file(file.path, server_side_encryption: "aws:kms")
+          s3_bucket
+            .object(File.basename(file.path))
+            .upload_file(file.path, server_side_encryption: bucket.encryption_type)
         end
       end
     end

spec/lib/defra_ruby/aws/bucket_spec.rb

@@ -7,6 +7,8 @@ module Aws
     RSpec.describe Bucket do
       subject(:bucket) { described_class.new(configs) }
 
+      let(:credentials) { { access_key_id: "access_key", secret_access_key: "secret" } }
+
       describe "#initialize" do
         context "when a name configuration is missing" do
           let(:configs) do
@@ -63,16 +65,165 @@ module Aws
             expect { bucket }.to raise_error("DefraRuby::Aws buckets configurations: missing secret access key for bucket foo")
           end
         end
+
+        context "when 'region' is not set" do
+          context "because it has not been added to the config" do
+            let(:configs) do
+              {
+                name: "foo",
+                credentials: credentials
+              }
+            end
+
+            it "defaults the region to 'eu-west-1'" do
+              expect(bucket.region).to eq("eu-west-1")
+            end
+          end
+
+          context "because its value is an empty string" do
+            let(:configs) do
+              {
+                name: "foo",
+                credentials: credentials,
+                region: ""
+              }
+            end
+
+            it "defaults the region to 'eu-west-1'" do
+              expect(bucket.region).to eq("eu-west-1")
+            end
+          end
+        end
+
+        context "when 'region' is set" do
+          let(:region) { "eu-west-2" }
+          let(:configs) do
+            {
+              name: "foo",
+              credentials: credentials,
+              region: region
+            }
+          end
+
+          it "sets the region to match" do
+            expect(bucket.region).to eq(region)
+          end
+        end
+
+        context "when 'encrypt_with_kms' is not set" do
+          context "because it has not been added to the config" do
+            let(:configs) do
+              {
+                name: "foo",
+                credentials: credentials
+              }
+            end
+
+            it "defaults encrypt_with_kms to false" do
+              expect(bucket.encrypt_with_kms).to be false
+            end
+
+            it "sets encryption_type to :AES256" do
+              expect(bucket.encryption_type).to eq(:AES256)
+            end
+          end
+
+          context "because its value is an empty string" do
+            let(:configs) do
+              {
+                name: "foo",
+                credentials: credentials,
+                encrypt_with_kms: ""
+              }
+            end
+
+            it "defaults encrypt_with_kms to false" do
+              expect(bucket.encrypt_with_kms).to be false
+            end
+
+            it "sets encryption_type to :AES256" do
+              expect(bucket.encryption_type).to eq(:AES256)
+            end
+          end
+        end
+
+        context "when 'encrypt_with_kms' is set" do
+          let(:encrypt_with_kms) { true }
+          let(:configs) do
+            {
+              name: "foo",
+              credentials: credentials,
+              encrypt_with_kms: encrypt_with_kms
+            }
+          end
+
+          context "to true as a boolean" do
+            let(:encrypt_with_kms) { true }
+
+            it "defaults encrypt_with_kms to true" do
+              expect(bucket.encrypt_with_kms).to be true
+            end
+
+            it "sets encryption_type to aws:kms" do
+              expect(bucket.encryption_type).to eq("aws:kms")
+            end
+          end
+
+          context "to true as a string" do
+            let(:encrypt_with_kms) { "true" }
+
+            it "defaults encrypt_with_kms to true" do
+              expect(bucket.encrypt_with_kms).to be true
+            end
+
+            it "sets encryption_type to aws:kms" do
+              expect(bucket.encryption_type).to eq("aws:kms")
+            end
+          end
+
+          context "to false as a boolean" do
+            let(:encrypt_with_kms) { false }
+
+            it "defaults encrypt_with_kms to false" do
+              expect(bucket.encrypt_with_kms).to be false
+            end
+
+            it "sets encryption_type to aws:kms" do
+              expect(bucket.encryption_type).to eq(:AES256)
+            end
+          end
+
+          context "to false as a string" do
+            let(:encrypt_with_kms) { "false" }
+
+            it "defaults encrypt_with_kms to false" do
+              expect(bucket.encrypt_with_kms).to be false
+            end
+
+            it "sets encryption_type to aws:kms" do
+              expect(bucket.encryption_type).to eq(:AES256)
+            end
+          end
+
+          context "to something not recognised" do
+            let(:encrypt_with_kms) { "bar" }
+
+            it "defaults encrypt_with_kms to false" do
+              expect(bucket.encrypt_with_kms).to be false
+            end
+
+            it "sets encryption_type to :AES256" do
+              expect(bucket.encryption_type).to eq(:AES256)
+            end
+          end
+        end
       end
 
       describe "#load" do
         let(:configs) do
           {
             name: "foo",
-            credentials: {
-              secret_access_key: "secret",
-              access_key_id: "access_key"
-            }
+            credentials: credentials
           }
         end
 

spec/lib/defra_ruby/aws/services/bucket_loader_service_spec.rb

@@ -5,31 +5,58 @@
 module DefraRuby
   module Aws
     RSpec.describe BucketLoaderService do
+
       describe "#run" do
-        let(:configs) do
-          {
-            credentials: {
-              access_key_id: "key_id",
-              secret_access_key: "secret"
-            },
-            name: "bulk"
-          }
-        end
+        let(:credentials) { { access_key_id: "key_id", secret_access_key: "secret" } }
         let(:bucket) { Bucket.new(configs) }
 
-        it "loads the given file to the s3 bucket" do
-          aws_resource = double(:aws_resource)
-          s3_bucket = double(:s3_bulk_bucket)
-          file = double(:file, path: "foo/bar/baz/test.csv")
-          s3_object = double(:s3_object)
-          result = double(:result)
+        context "when 'encrypt_with_kms' is not set" do
+          let(:configs) do
+            {
+              credentials: credentials,
+              name: "bulk"
+            }
+          end
+
+          it "loads the given file to the s3 bucket using :AES256" do
+            aws_resource = double(:aws_resource)
+            s3_bucket = double(:s3_bulk_bucket)
+            file = double(:file, path: "foo/bar/baz/test.csv")
+            s3_object = double(:s3_object)
+            result = double(:result)
+
+            expect(::Aws::S3::Resource).to receive(:new).and_return(aws_resource)
+            expect(aws_resource).to receive(:bucket).with("bulk").and_return(s3_bucket)
+            expect(s3_bucket).to receive(:object).with("test.csv").and_return(s3_object)
+            expect(s3_object).to receive(:upload_file).with("foo/bar/baz/test.csv", server_side_encryption: :AES256).and_return(result)
+
+            expect(described_class.run(bucket, file)).to be_a(Response)
+          end
+        end
+
+        context "when 'encrypt_with_kms' is set" do
+          let(:configs) do
+            {
+              credentials: credentials,
+              name: "bulk",
+              encrypt_with_kms: true
+            }
+          end
+
+          it "loads the given file to the s3 bucket using AWS:KMS" do
+            aws_resource = double(:aws_resource)
+            s3_bucket = double(:s3_bulk_bucket)
+            file = double(:file, path: "foo/bar/baz/test.csv")
+            s3_object = double(:s3_object)
+            result = double(:result)
 
-          expect(::Aws::S3::Resource).to receive(:new).and_return(aws_resource)
-          expect(aws_resource).to receive(:bucket).with("bulk").and_return(s3_bucket)
-          expect(s3_bucket).to receive(:object).with("test.csv").and_return(s3_object)
-          expect(s3_object).to receive(:upload_file).with("foo/bar/baz/test.csv", server_side_encryption: "aws:kms").and_return(result)
+            expect(::Aws::S3::Resource).to receive(:new).and_return(aws_resource)
+            expect(aws_resource).to receive(:bucket).with("bulk").and_return(s3_bucket)
+            expect(s3_bucket).to receive(:object).with("test.csv").and_return(s3_object)
+            expect(s3_object).to receive(:upload_file).with("foo/bar/baz/test.csv", server_side_encryption: "aws:kms").and_return(result)
 
-          expect(described_class.run(bucket, file)).to be_a(Response)
+            expect(described_class.run(bucket, file)).to be_a(Response)
+          end
         end
       end
     end