From 78f9ab4e609f821fc359e8ef81569caca7f6ef9e Mon Sep 17 00:00:00 2001
From: Alan Cruikshanks <alan.cruikshanks@gmail.com>
Date: Fri, 10 Jul 2020 11:25:47 +0100
Subject: [PATCH 1/8] Allow encryption method to be configurable

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 was 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.

From f8b05065440b0d2d2969bb6dfc59f586364c5988 Mon Sep 17 00:00:00 2001
From: Alan Cruikshanks <alan.cruikshanks@gmail.com>
Date: Fri, 10 Jul 2020 11:55:26 +0100
Subject: [PATCH 2/8] Reset to previous behaviour and update tests

Reset back to always using AES256. But update the tests to encompass our new option to use AWS:KMS
---
 .../aws/services/bucket_loader_service.rb     |  2 +-
 .../services/bucket_loader_service_spec.rb    | 67 +++++++++++++------
 2 files changed, 48 insertions(+), 21 deletions(-)

diff --git a/lib/defra_ruby/aws/services/bucket_loader_service.rb b/lib/defra_ruby/aws/services/bucket_loader_service.rb
index 7622e32..e36984d 100644
--- a/lib/defra_ruby/aws/services/bucket_loader_service.rb
+++ b/lib/defra_ruby/aws/services/bucket_loader_service.rb
@@ -24,7 +24,7 @@ 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: :AES256)
         end
       end
     end
diff --git a/spec/lib/defra_ruby/aws/services/bucket_loader_service_spec.rb b/spec/lib/defra_ruby/aws/services/bucket_loader_service_spec.rb
index 92fce9c..98a888c 100644
--- a/spec/lib/defra_ruby/aws/services/bucket_loader_service_spec.rb
+++ b/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 'use_aws_kms_encryption' is not set" do
+          let(:configs) do
+            {
+              credentials: credentials,
+              name: "bulk"
+            }
+          end
+
+          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)
+
+            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 'use_aws_kms_encryption' is set" do
+          let(:configs) do
+            {
+              credentials: credentials,
+              name: "bulk",
+              use_aws_kms_encryption: true
+            }
+          end
+
+          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)
 
-          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

From 731018d99f5b7ecb825ef41131ceb0b1e3bc1537 Mon Sep 17 00:00:00 2001
From: Alan Cruikshanks <alan.cruikshanks@gmail.com>
Date: Fri, 10 Jul 2020 11:59:50 +0100
Subject: [PATCH 3/8] Update code to support new config option

---
 lib/defra_ruby/aws/bucket.rb                             | 9 +++++++++
 lib/defra_ruby/aws/services/bucket_loader_service.rb     | 2 +-
 .../aws/services/bucket_loader_service_spec.rb           | 4 ++--
 3 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/lib/defra_ruby/aws/bucket.rb b/lib/defra_ruby/aws/bucket.rb
index 50a7475..04def86 100644
--- a/lib/defra_ruby/aws/bucket.rb
+++ b/lib/defra_ruby/aws/bucket.rb
@@ -9,6 +9,7 @@ def initialize(configs)
         @credentials = configs[:credentials]
         @bucket_name = configs[:name]
         @region = setup_region(configs[:region])
+        @use_aws_kms_encryption = configs[:use_aws_kms_encryption]
 
         validate!
       end
@@ -21,6 +22,14 @@ def secret_access_key
         credentials[:secret_access_key]
       end
 
+      def encryption_method
+        @_encryption_method ||= if @use_aws_kms_encryption
+                                  "aws:kms"
+                                else
+                                  :AES256
+                                end
+      end
+
       def load(file)
         BucketLoaderService.run(self, file)
       end
diff --git a/lib/defra_ruby/aws/services/bucket_loader_service.rb b/lib/defra_ruby/aws/services/bucket_loader_service.rb
index e36984d..5301fe8 100644
--- a/lib/defra_ruby/aws/services/bucket_loader_service.rb
+++ b/lib/defra_ruby/aws/services/bucket_loader_service.rb
@@ -24,7 +24,7 @@ def run
 
       def response_exe
         lambda do
-          s3_bucket.object(File.basename(file.path)).upload_file(file.path, server_side_encryption: :AES256)
+          s3_bucket.object(File.basename(file.path)).upload_file(file.path, server_side_encryption: bucket.encryption_method)
         end
       end
     end
diff --git a/spec/lib/defra_ruby/aws/services/bucket_loader_service_spec.rb b/spec/lib/defra_ruby/aws/services/bucket_loader_service_spec.rb
index 98a888c..95c43d5 100644
--- a/spec/lib/defra_ruby/aws/services/bucket_loader_service_spec.rb
+++ b/spec/lib/defra_ruby/aws/services/bucket_loader_service_spec.rb
@@ -18,7 +18,7 @@ module Aws
             }
           end
 
-          it "loads the given file to the s3 bucket" do
+          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")
@@ -43,7 +43,7 @@ module Aws
             }
           end
 
-          it "loads the given file to the s3 bucket" do
+          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")

From 6a1e662d07d1ef83127d090bcc0d6dbca30d021a Mon Sep 17 00:00:00 2001
From: Alan Cruikshanks <alan.cruikshanks@gmail.com>
Date: Fri, 10 Jul 2020 12:29:47 +0100
Subject: [PATCH 4/8] Update config name and handle boolean and strings

Updated the name of the property on the bucket to match the terminology used by AWS (see https://docs.aws.amazon.com/sdk-for-ruby/v2/api/Aws/S3/Types/Encryption.html#encryption_type-instance_method)

Also updated the config property name to be clearer and simpler to use. Finally updated our logic around the arg to handle the value being set as either a string or a boolean. Previous version was just relying on something being there.
---
 lib/defra_ruby/aws/bucket.rb                  | 20 +++++++++++--------
 .../aws/services/bucket_loader_service.rb     |  2 +-
 .../services/bucket_loader_service_spec.rb    |  6 +++---
 3 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/lib/defra_ruby/aws/bucket.rb b/lib/defra_ruby/aws/bucket.rb
index 04def86..e4221dc 100644
--- a/lib/defra_ruby/aws/bucket.rb
+++ b/lib/defra_ruby/aws/bucket.rb
@@ -3,13 +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])
-        @use_aws_kms_encryption = configs[:use_aws_kms_encryption]
+        @encrypt_with_kms = setup_encrypt_with_kms(configs[:encrypt_with_kms])
 
         validate!
       end
@@ -22,12 +22,8 @@ def secret_access_key
         credentials[:secret_access_key]
       end
 
-      def encryption_method
-        @_encryption_method ||= if @use_aws_kms_encryption
-                                  "aws:kms"
-                                else
-                                  :AES256
-                                end
+      def encryption_type
+        @_encryption_type ||= @encrypt_with_kms ? "aws:kms" : :AES256
       end
 
       def load(file)
@@ -56,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!
diff --git a/lib/defra_ruby/aws/services/bucket_loader_service.rb b/lib/defra_ruby/aws/services/bucket_loader_service.rb
index 5301fe8..8e855d0 100644
--- a/lib/defra_ruby/aws/services/bucket_loader_service.rb
+++ b/lib/defra_ruby/aws/services/bucket_loader_service.rb
@@ -24,7 +24,7 @@ def run
 
       def response_exe
         lambda do
-          s3_bucket.object(File.basename(file.path)).upload_file(file.path, server_side_encryption: bucket.encryption_method)
+          s3_bucket.object(File.basename(file.path)).upload_file(file.path, server_side_encryption: bucket.encryption_type)
         end
       end
     end
diff --git a/spec/lib/defra_ruby/aws/services/bucket_loader_service_spec.rb b/spec/lib/defra_ruby/aws/services/bucket_loader_service_spec.rb
index 95c43d5..8d0be07 100644
--- a/spec/lib/defra_ruby/aws/services/bucket_loader_service_spec.rb
+++ b/spec/lib/defra_ruby/aws/services/bucket_loader_service_spec.rb
@@ -10,7 +10,7 @@ module Aws
         let(:credentials) { { access_key_id: "key_id", secret_access_key: "secret" } }
         let(:bucket) { Bucket.new(configs) }
 
-        context "when 'use_aws_kms_encryption' is not set" do
+        context "when 'encrypt_with_kms' is not set" do
           let(:configs) do
             {
               credentials: credentials,
@@ -34,12 +34,12 @@ module Aws
           end
         end
 
-        context "when 'use_aws_kms_encryption' is set" do
+        context "when 'encrypt_with_kms' is set" do
           let(:configs) do
             {
               credentials: credentials,
               name: "bulk",
-              use_aws_kms_encryption: true
+              encrypt_with_kms: true
             }
           end
 

From 6ef18f84d78fe1bdc7aa30e76325c1be00c96cf0 Mon Sep 17 00:00:00 2001
From: Alan Cruikshanks <alan.cruikshanks@gmail.com>
Date: Fri, 10 Jul 2020 12:48:14 +0100
Subject: [PATCH 5/8] Add some tests for region

Doing this ahead of adding some for the encryption type. Made sense if I felt the need to cover encryption type we should also be covering region.
---
 spec/lib/defra_ruby/aws/bucket_spec.rb | 51 ++++++++++++++++++++++++--
 1 file changed, 47 insertions(+), 4 deletions(-)

diff --git a/spec/lib/defra_ruby/aws/bucket_spec.rb b/spec/lib/defra_ruby/aws/bucket_spec.rb
index 9d673d4..5983b64 100644
--- a/spec/lib/defra_ruby/aws/bucket_spec.rb
+++ b/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,57 @@ 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
       end
 
       describe "#load" do
         let(:configs) do
           {
             name: "foo",
-            credentials: {
-              secret_access_key: "secret",
-              access_key_id: "access_key"
-            }
+            credentials: credentials
           }
         end
 

From 7dd47641c51da8873ca582b0fe472d61eecafa99 Mon Sep 17 00:00:00 2001
From: Alan Cruikshanks <alan.cruikshanks@gmail.com>
Date: Fri, 10 Jul 2020 12:59:12 +0100
Subject: [PATCH 6/8] Add tests for encryption config

---
 spec/lib/defra_ruby/aws/bucket_spec.rb | 108 +++++++++++++++++++++++++
 1 file changed, 108 insertions(+)

diff --git a/spec/lib/defra_ruby/aws/bucket_spec.rb b/spec/lib/defra_ruby/aws/bucket_spec.rb
index 5983b64..87a7450 100644
--- a/spec/lib/defra_ruby/aws/bucket_spec.rb
+++ b/spec/lib/defra_ruby/aws/bucket_spec.rb
@@ -109,6 +109,114 @@ module Aws
             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

From afcd32bf35e7f07fd739e7adf65d2e19bd8c1a4f Mon Sep 17 00:00:00 2001
From: Alan Cruikshanks <alan.cruikshanks@gmail.com>
Date: Fri, 10 Jul 2020 12:59:31 +0100
Subject: [PATCH 7/8] Update README details

---
 README.md | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/README.md b/README.md
index d083843..4e82a14 100644
--- a/README.md
+++ b/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.
 

From 9327991a58ea946461b70ca238b3f0b334afc9fa Mon Sep 17 00:00:00 2001
From: Alan Cruikshanks <alan.cruikshanks@gmail.com>
Date: Fri, 10 Jul 2020 13:26:18 +0100
Subject: [PATCH 8/8] Fix rubocop issue

---
 lib/defra_ruby/aws/services/bucket_loader_service.rb | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lib/defra_ruby/aws/services/bucket_loader_service.rb b/lib/defra_ruby/aws/services/bucket_loader_service.rb
index 8e855d0..f335d8d 100644
--- a/lib/defra_ruby/aws/services/bucket_loader_service.rb
+++ b/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: bucket.encryption_type)
+          s3_bucket
+            .object(File.basename(file.path))
+            .upload_file(file.path, server_side_encryption: bucket.encryption_type)
         end
       end
     end