Convergent encryption support #1

finalwharf · 2018-10-01T12:08:14Z

What does this PR do?

Adds support for convergent encryption

Where should the reviewer start?

lib/vault/rails.rb
lib/vault/encrypted_model.rb

Any background context you want to provide?

Vault supports convergent encryption since v0.6.1, but this gem
does not take advantage of this functionality.

/cc @popovm @bliof please review

popovm · 2018-10-04T12:51:23Z

spec/integration/rails_spec.rb

+      expect(first_person.email_encrypted).to eq second_person.email_encrypted
+    end
+
+    it 'generates different ciphertexts different plaintexts' do


for different plaintexts 👴

popovm · 2018-10-04T13:22:15Z

lib/vault/rails.rb

@@ -27,6 +27,10 @@ module Rails
    DEV_WARNING = "[vault-rails] Using in-memory cipher - this is not secure " \
      "and should never be used in production-like environments!".freeze

+    CONVERGENT_ENCRYPTION_CONTEXT = Base64.strict_encode64(
+      ENV.fetch('VAULT_CONVERGENT_ENCRYPTION_CONTEXT', 'convergent-encryption-context')


Let's remove the default value here

bliof · 2018-10-05T08:09:50Z

lib/vault/rails.rb

@@ -30,7 +30,7 @@ module Rails
    class << self
      # API client object based off the configured options in {Configurable}.
      #
-      # @return [Vault::Client]
+      # @return [Vault::Client] for use with convergent encryption


You can drop the addition, you can use it without the convergent encryption

Accidental copy/paste 🤷🏻‍♂️

lib/vault/rails.rb

finalwharf · 2018-10-05T10:01:21Z

Some context: Vault::Rails has two protected methods - memory_encrypt and memory_decrypt, that is uses for encryption in development and testing environments instead of accessing Vault. The two methods are from the official repo, and use AES 128 CBC without an Initialization Vector.

Both using the same IV and no IV in the first place poses the same security issues. The reason for using the same IV is to make the gem behave the same both in development and production as the current PR adds support for convergent encryption for production as well.

The way that the VAULT_CONVERGENT_ENCRYPTION_CONTEXT is used is not the same in dev and prod. In development I use it as an IV (to keep things simple), but in prod, Vault uses it for key derivation.

Again, memory encryption is used only when Vault is not enabled (dev and test envs).

bliof

You are missing specs that verify that vault_decrypt and vault_encrypt correctly pass the options to the client.

Otherwise LGTM

lib/vault/rails.rb

subodhsawant · 2018-10-08T10:08:22Z

@finalwharf @bliof - RE: memory_encrypt / memory_decrypt / AES CBC 128 / static IV - this is fine from a security perspective as long as it is used for development and testing

spec/unit/rails_spec.rb

bliof · 2018-10-08T14:18:07Z

spec/unit/rails_spec.rb

+
+    describe '.vault_encrypt' do
+      it 'sends the correct paramaters to vault client' do
+


bliof · 2018-10-08T14:18:36Z

spec/unit/rails_spec.rb

+          with(expected_route, expected_options).
+          and_return(spy('Vault::Secret'))
+
+        Vault::Rails.send(:vault_encrypt, 'path', 'key', 'plaintext', Vault::Rails.client, true)


Please use the public methods to test this

bliof

LGTM 👍

bliof · 2018-10-08T14:49:14Z

spec/unit/rails_spec.rb

+    end
+  end
+
+  describe '.decrypt' do


tbo you can delete this and the one above

popovm

LGTM 👍

What does this PR do? --------------------- * Adds support for convergent encryption Where should the reviewer start? -------------------------------- * `lib/vault/rails.rb` * `lib/vault/encrypted_model.rb` Any background context you want to provide? ------------------------------------------- * Vault supports convergent encryption since v0.6.1, but this gem does not take advantage of this functionality.

finalwharf force-pushed the master branch 2 times, most recently from b816bff to 54b6047 Compare October 4, 2018 07:58

finalwharf force-pushed the convergent_encryption branch 2 times, most recently from 544c52f to 28c3d67 Compare October 4, 2018 12:44

finalwharf requested a review from popovm October 4, 2018 12:46

popovm reviewed Oct 4, 2018

View reviewed changes

finalwharf force-pushed the convergent_encryption branch 2 times, most recently from fa1feee to 6b0e6bf Compare October 5, 2018 06:30

finalwharf requested a review from bliof October 5, 2018 06:57

bliof reviewed Oct 5, 2018

View reviewed changes

lib/vault/rails.rb Show resolved Hide resolved

finalwharf force-pushed the convergent_encryption branch from 6b0e6bf to 069c75e Compare October 5, 2018 08:49

subodhsawant requested review from maryammouse and michaellennox October 5, 2018 08:55

bliof suggested changes Oct 5, 2018

View reviewed changes

lib/vault/rails.rb Outdated Show resolved Hide resolved

finalwharf force-pushed the convergent_encryption branch from 069c75e to 22113ec Compare October 5, 2018 13:43

subodhsawant self-requested a review October 8, 2018 09:06

finalwharf force-pushed the convergent_encryption branch 2 times, most recently from 1ebdde1 to 37516fa Compare October 8, 2018 11:59

bliof suggested changes Oct 8, 2018

View reviewed changes

finalwharf force-pushed the convergent_encryption branch 2 times, most recently from e4f4df2 to 6ab4e33 Compare October 8, 2018 14:42

bliof approved these changes Oct 8, 2018

View reviewed changes

bliof reviewed Oct 8, 2018

View reviewed changes

spec/unit/rails_spec.rb Outdated

end

end

describe '.decrypt' do

Copy link

bliof Oct 8, 2018

Choose a reason for hiding this comment

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

tbo you can delete this and the one above

popovm approved these changes Oct 8, 2018

View reviewed changes

finalwharf force-pushed the convergent_encryption branch 2 times, most recently from 14365fc to 8fc09aa Compare October 9, 2018 05:50

finalwharf force-pushed the convergent_encryption branch from 8fc09aa to f71550c Compare October 9, 2018 05:56

finalwharf merged commit fc715f2 into master Oct 9, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convergent encryption support #1

Convergent encryption support #1

finalwharf commented Oct 1, 2018 •

edited

Loading

popovm Oct 4, 2018

popovm Oct 4, 2018

bliof Oct 5, 2018 •

edited

Loading

finalwharf Oct 5, 2018 •

edited

Loading

finalwharf commented Oct 5, 2018

bliof left a comment

subodhsawant commented Oct 8, 2018

bliof Oct 8, 2018

bliof Oct 8, 2018

bliof left a comment

bliof Oct 8, 2018

popovm left a comment


		describe '.vault_encrypt' do
		it 'sends the correct paramaters to vault client' do

Convergent encryption support #1

Convergent encryption support #1

Conversation

finalwharf commented Oct 1, 2018 • edited Loading

What does this PR do?

Where should the reviewer start?

Any background context you want to provide?

popovm Oct 4, 2018

Choose a reason for hiding this comment

popovm Oct 4, 2018

Choose a reason for hiding this comment

bliof Oct 5, 2018 • edited Loading

Choose a reason for hiding this comment

finalwharf Oct 5, 2018 • edited Loading

Choose a reason for hiding this comment

finalwharf commented Oct 5, 2018

bliof left a comment

Choose a reason for hiding this comment

subodhsawant commented Oct 8, 2018

bliof Oct 8, 2018

Choose a reason for hiding this comment

bliof Oct 8, 2018

Choose a reason for hiding this comment

bliof left a comment

Choose a reason for hiding this comment

bliof Oct 8, 2018

Choose a reason for hiding this comment

popovm left a comment

Choose a reason for hiding this comment

finalwharf commented Oct 1, 2018 •

edited

Loading

bliof Oct 5, 2018 •

edited

Loading

finalwharf Oct 5, 2018 •

edited

Loading