Skip to content

Allow MessageEncryptor to take advantage authenticated encryption modes#25874

Closed
bdewater wants to merge 5 commits intorails:masterfrom
bdewater:aead_message_encryptor
Closed

Allow MessageEncryptor to take advantage authenticated encryption modes#25874
bdewater wants to merge 5 commits intorails:masterfrom
bdewater:aead_message_encryptor

Conversation

@bdewater
Copy link
Contributor

@bdewater bdewater commented Jul 18, 2016

AEAD modes have been supported since Ruby 2.0 (doc) and OpenSSL 1.0.1. It is faster:

require 'benchmark/ips'
require 'active_support/message_encryptor'
require 'active_support/key_generator'

Benchmark.ips do |x|
  salt  = SecureRandom.random_bytes(64)
  key   = ActiveSupport::KeyGenerator.new('password').generate_key(salt)
  data  = 'my secret data'

  x.report("default roundtrip") do
    crypt = ActiveSupport::MessageEncryptor.new(key)
    encrypted_data = crypt.encrypt_and_sign(data)
    crypt.decrypt_and_verify(encrypted_data)
  end

  x.report("aes-256-gcm roundtrip") do
    crypt = ActiveSupport::MessageEncryptor.new(key, cipher: 'aes-256-gcm')
    encrypted_data = crypt.encrypt_and_sign(data)
    crypt.decrypt_and_verify(encrypted_data)
  end

  x.compare!
end
Warming up --------------------------------------
   default roundtrip     2.148k i/100ms
aes-256-gcm roundtrip
                         4.698k i/100ms
Calculating -------------------------------------
   default roundtrip     21.947k (± 3.4%) i/s -    111.696k in   5.095390s
aes-256-gcm roundtrip
                         48.374k (± 4.2%) i/s -    244.296k in   5.058709s

Comparison:
aes-256-gcm roundtrip:    48374.4 i/s
   default roundtrip:    21946.7 i/s - 2.20x slower

And it produces smaller ciphertexts:

irb(main):090:0> ActiveSupport::MessageEncryptor.new(key).encrypt_and_sign(data).length
=> 138
irb(main):091:0> ActiveSupport::MessageEncryptor.new(key, cipher: 'aes-256-gcm').encrypt_and_sign(data).length
=> 76

@rafaelfranca
Copy link
Member

rafaelfranca commented Jul 20, 2016

r? @matthewd @jeremy

@jeremy
Copy link
Member

jeremy commented Jul 20, 2016

Dig it! Can you ensure compatibility with MessageEncryptor that already use an AEAD cipher?

@bdewater
Copy link
Contributor Author

bdewater commented Jul 20, 2016

That could probably work by counting the -- occurrences, I'll work on it somewhere later this week.

@bdewater bdewater force-pushed the aead_message_encryptor branch 2 times, most recently from b351f7e to 9b476e5 Compare July 20, 2016 11:09
@bdewater
Allow MessageEncryptor to take advantage authenticated encryption modes
4c05835 
AEAD modes like `aes-256-gcm` provide both confidentiality and data authenticity, eliminating the need to use MessageVerifier to check if the encrypted data has been tampered with.
@bdewater bdewater force-pushed the aead_message_encryptor branch from 9b476e5 to d38cd27 Compare July 20, 2016 11:14
@bdewater
Copy link
Contributor Author

bdewater commented Jul 20, 2016 •
edited
Loading

@jeremy that should not work right now. MessageVerifier will tell that the message is valid but OpenSSL::Cipher::CipherError will be raised during decryption because the auth_tag is not set.

I've also done a quick test with aes-256-ccm (the only other AEAD chipher on my system with OpenSSL 1.0.2h according to OpenSSL::Cipher.ciphers) on my branch and it looks like that the Ruby bindings do not support this mode as authenticated. I've changed the #aead_mode? check to use what the bindings report about the cipher.

Edit: already fixed in Ruby trunk at ruby/ruby@9f70378

@bdewater
The OpenSSL bindings can tell us if a cipher supports AEAD
0e130be 
Using `aes-256-ccm` as a cipher on Ruby 2.3.1 results in "OpenSSL::Cipher::CipherError: authentication tag not supported by this cipher" even though it is an authenticated mode: https://wiki.openssl.org/index.php/EVP_Authenticated_Encryption_and_Decryption#Authenticated_Encryption_using_CCM_mode - the same will probably be true for `chacha20-poly1305` when OpenSSL 1.1.0 is released without a fix upstream in the bindings..
@bdewater bdewater force-pushed the aead_message_encryptor branch from d38cd27 to 5d5f291 Compare July 20, 2016 11:37
@bdewater bdewater force-pushed the aead_message_encryptor branch from 5d5f291 to 41cb447 Compare July 20, 2016 12:18
@bdewater bdewater mentioned this pull request Jul 21, 2016
Closed
jeremy
jeremy reviewed Jul 21, 2016
View reviewed changes
activesupport/CHANGELOG.md Outdated
@@ -1,3 +1,12 @@
* Allow MessageEncryptor to take advantage authenticated encryption modes.
Copy link
Member

@jeremy jeremy Jul 21, 2016

Choose a reason for hiding this comment

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

take advantage *of

@jeremy jeremy added this to the 5.1.0 milestone Jul 21, 2016
@bdewater
Copy link
Contributor Author

bdewater commented Jul 21, 2016

Thanks for the feedback @jeremy, PR updated! However #23297 is also on the 5.1.0 milestone, in which direction does the team want to go with MessageEncryptor?

@jeremy
Copy link
Member

jeremy commented Jul 21, 2016

Squashed in d4ea18a

@jeremy jeremy closed this Jul 21, 2016
@jeremy
Copy link
Member

jeremy commented Jul 21, 2016

Thanks @bdewater! #23297 is a much larger effort to switch to a new message envelope (and more). Your work is independent and good for release even if the JWE work slips to 5.2.

@vipulnsward
Copy link
Member

vipulnsward commented Jul 22, 2016 •
edited
Loading

We should be adding a note that, switching modes busts previous auth messages that were set with current environment.

@vipulnsward
Copy link
Member

vipulnsward commented Jul 22, 2016

Hmm, nevermind, I don't see how this will affect existing auth

bdewater added a commit to bdewater/rails that referenced this pull request Jul 27, 2016
@bdewater
Add rationale for manually checking auth_tag length, which got lost w…
73209aa 
…hen rails#25874 was squashed before merging [skip ci]
@bdewater bdewater deleted the aead_message_encryptor branch February 8, 2017 19:18
@bdewater bdewater mentioned this pull request Feb 24, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@jeremy jeremy

@matthewd matthewd

Labels

activesupport

Projects

None yet

Milestone

5.1.0

Development

Successfully merging this pull request may close these issues.

6 participants

@bdewater @rafaelfranca @jeremy @vipulnsward @matthewd @maclover7