Adds strict_mode that, when false, avoids an error when data to be encrypted with AES is the empty string #32

Closed
wants to merge 4 commits into
from

Projects

None yet

2 participants

@Capncavedan

strict_mode is true by default. when false, it skips encryption and decryption of empty strings.

where do you think the best place to document this/provide an example is?

@jmazzi jmazzi and 1 other commented on an outdated diff Mar 9, 2013
lib/crypt_keeper/provider/aes.rb
@@ -24,6 +27,10 @@ def initialize(options = {})
raise ArgumentError, "Missing :key"
end
+ @strict_mode = options.fetch(:strict_mode) do
@jmazzi
jmazzi Mar 9, 2013

@strict_mode = options.fetch(:strict_mode, true)

@jmazzi jmazzi and 1 other commented on an outdated diff Mar 9, 2013
lib/crypt_keeper/provider/aes.rb
end
# Public: Decrypt a string
#
# Returns a string
def decrypt(value)
- iv, value = Base64::decode64(value.to_s).split(SEPARATOR)
- aes.decrypt
- aes.key = key
- aes.iv = iv
- aes.update(value) + aes.final
+ if value == '' && !strict_mode
+ value
+ else
+ iv, value = Base64::decode64(value.to_s).split(SEPARATOR)
+ aes.decrypt
+ aes.key = key
+ aes.iv = iv
+ aes.update(value) + aes.final
+ end
@jmazzi
jmazzi Mar 9, 2013

How about moving the logic to a encryptable? method

@Capncavedan

@jmazzi have you had a chance to look at this? Let me know if it isn't up to snuff and I'll work to address any concerns.

@jmazzi jmazzi and 1 other commented on an outdated diff Mar 14, 2013
lib/crypt_keeper/provider/aes.rb
@@ -13,6 +13,9 @@ class Aes
# Public: An instance of OpenSSL::Cipher::Cipher
attr_accessor :aes
+ # Public: Whether blank string is accepted as data for encryption
+ attr_accessor :strict_mode
+
# Public: Initializes the class
#
# options - A hash of options. :key is required
@jmazzi
jmazzi Mar 14, 2013

Can you document :strict_mode, @Capncavedan?

@Capncavedan
Capncavedan Mar 14, 2013

Absolutely. Is right here the correct place, or should I submit a patched example of AES encryption?

@jmazzi
jmazzi Mar 14, 2013
@Capncavedan

I corrected a problem where strict_mode was being too permissive.

I also added strict_mode documentation as suggested.

I think it's ready, @jmazzi

@jmazzi jmazzi commented on the diff Mar 21, 2013
lib/crypt_keeper/provider/aes.rb
@@ -31,20 +40,43 @@ def initialize(options = {})
#
# Returns a string
def encrypt(value)
- aes.encrypt
- aes.key = key
- Base64::encode64("#{aes.random_iv}#{SEPARATOR}#{aes.update(value.to_s) + aes.final}")
+ if encryptable?(value)
+ return '' if value == ''
@jmazzi
jmazzi Mar 21, 2013

This line is not needed now. encryptable? already handles that check

@Capncavedan
Capncavedan Mar 21, 2013

encryptable? is more answering the question whether an empty-string should be accepted. it checks if it's empty-string and strict_mode, and returns false if both, true otherwise. The actual special-case handling of the empty string to avoid the AES library error should happen here.

aes.update requires non-empty data be passed in, so when we do allow empty-string (strict_mode: false), we need to ensure we don't attempt to encrypt it.

@jmazzi jmazzi commented on the diff Mar 21, 2013
lib/crypt_keeper/provider/aes.rb
end
# Public: Decrypt a string
#
# Returns a string
def decrypt(value)
- iv, value = Base64::decode64(value.to_s).split(SEPARATOR)
- aes.decrypt
- aes.key = key
- aes.iv = iv
- aes.update(value) + aes.final
+ if encryptable?(value)
+ return '' if value == ''
@jmazzi
jmazzi Mar 21, 2013

Line not needed

@Capncavedan
Capncavedan Mar 21, 2013

it's here for symmetry when an empty-string has been stored, it's returned as such

@Capncavedan

@jmazzi - is this mergeable? I'd love to be able to use an updated crypt_keeper gem in my app ASAP.

Are you comfortable with changes?

@jmazzi
Owner
@jmazzi
Owner

@Capncavedan after seeing this implemented, I'm inclined to say lets remove strict mode and make returning the orignal string the default behavior when strings are nil or empty.

@Capncavedan

@jmazzi I agree - that seems pretty clean. I'll work that up as a separate request.

@jmazzi
Owner

@Capncavedan cool, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment