Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

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

2 participants

Dan Buettner Justin Mazzi
Dan Buettner

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?

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
Justin Mazzi Owner
jmazzi added a note

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

Nicer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
lib/crypt_keeper/provider/aes.rb
((14 lines not shown))
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
Justin Mazzi Owner
jmazzi added a note

How about moving the logic to a encryptable? method

yes.

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

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

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
Justin Mazzi Owner
jmazzi added a note

Can you document :strict_mode, @Capncavedan?

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

Justin Mazzi Owner
jmazzi added a note
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Dan Buettner

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

Justin Mazzi jmazzi commented on the diff
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 == ''
Justin Mazzi Owner
jmazzi added a note

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Justin Mazzi jmazzi commented on the diff
lib/crypt_keeper/provider/aes.rb
((15 lines not shown))
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 == ''
Justin Mazzi Owner
jmazzi added a note

Line not needed

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

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

@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?

Justin Mazzi
Owner
Justin Mazzi
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.

Dan Buettner

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

Justin Mazzi
Owner

@Capncavedan cool, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 8, 2013
Commits on Mar 9, 2013
Commits on Mar 21, 2013
  1. add strict_mode documentation

    Dan Buettner authored
This page is out of date. Refresh to see the latest.
Showing with 87 additions and 25 deletions.
  1. +41 −9 lib/crypt_keeper/provider/aes.rb
  2. +46 −16 spec/provider/aes_spec.rb
50 lib/crypt_keeper/provider/aes.rb
View
@@ -13,9 +13,16 @@ class Aes
# Public: An instance of OpenSSL::Cipher::Cipher
attr_accessor :aes
+ # Public: When true (default), blank string is not accepted as data for encryption
+ attr_accessor :strict_mode
+
# Public: Initializes the class
#
- # options - A hash of options. :key is required
+ # options - A hash of options.
+ # :key is required
+ # :strict_mode can be true or false; true is the default
+ # when true, encryting or decrypting an empty string will raise an exception
+ # when false an empty string will be accepted (but will not be encrypted; will be returned as empty string)
def initialize(options = {})
@aes = ::OpenSSL::Cipher::Cipher.new("AES-256-CBC")
@aes.padding = 1
@@ -24,6 +31,8 @@ def initialize(options = {})
raise ArgumentError, "Missing :key"
end
+ @strict_mode = options.fetch(:strict_mode, true)
+
@key = Digest::SHA256.digest(key)
end
@@ -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 == ''
Justin Mazzi Owner
jmazzi added a note

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ aes.encrypt
+ aes.key = key
+ Base64::encode64("#{aes.random_iv}#{SEPARATOR}#{aes.update(value.to_s) + aes.final}")
+ else
+ raise ArgumentError, "empty string not allowed in strict mode"
+ end
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 == ''
Justin Mazzi Owner
jmazzi added a note

Line not needed

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ iv, value = Base64::decode64(value.to_s).split(SEPARATOR)
+ aes.decrypt
+ aes.key = key
+ aes.iv = iv
+ aes.update(value) + aes.final
+ else
+ raise ArgumentError, "empty string not allowed in strict mode"
+ end
+ end
+
+ protected
+
+ # Protected: Determine if value can be encrypted
+ #
+ # Returns a boolean
+ def encryptable?(value)
+ if (value == '' && strict_mode)
+ false
+ else
+ true
+ end
end
end
end
62 spec/provider/aes_spec.rb
View
@@ -3,32 +3,62 @@
module CryptKeeper
module Provider
describe Aes do
- subject { Aes.new(key: 'cake') }
+ context "strict mode (default)" do
+ subject { Aes.new(key: 'cake') }
- describe "#initialize" do
- let(:hexed_key) do
- Digest::SHA256.digest('cake')
+ describe "#initialize" do
+ let(:hexed_key) do
+ Digest::SHA256.digest('cake')
+ end
+
+ its(:key) { should == hexed_key }
+ its(:strict_mode) { should be_true }
+ specify { expect { Aes.new }.to raise_error(ArgumentError, "Missing :key") }
end
- its(:key) { should == hexed_key }
- specify { expect { Aes.new }.to raise_error(ArgumentError, "Missing :key") }
- end
+ describe "#encrypt" do
+ let(:encrypted) do
+ subject.encrypt 'string'
+ end
+
+ specify { encrypted.should_not == 'string' }
+ specify { encrypted.should_not be_blank }
+ specify { expect { subject.encrypt('') }.to raise_error(ArgumentError, "empty string not allowed in strict mode") }
+ end
+
+ describe "#decrypt" do
+ let(:decrypted) do
+ subject.decrypt "MC41MDk5MjI2NjgxMDI1MDI2OmNyeXB0X2tlZXBlcjpPI/8dCqWXDMVj7Jqs\nuwf/\n"
+ end
+
+ specify { decrypted.should == 'string' }
- describe "#encrypt" do
- let(:encrypted) do
- subject.encrypt 'string'
+ specify { expect { subject.decrypt('') }.to raise_error(ArgumentError, "empty string not allowed in strict mode") }
end
- specify { encrypted.should_not == 'string' }
- specify { encrypted.should_not be_blank }
+ describe "#encryptable?" do
+ specify { subject.send(:encryptable?, '').should be_false }
+ specify { subject.send(:encryptable?, ' ').should be_true }
+ end
end
- describe "#decrypt" do
- let(:decrypted) do
- subject.decrypt "MC41MDk5MjI2NjgxMDI1MDI2OmNyeXB0X2tlZXBlcjpPI/8dCqWXDMVj7Jqs\nuwf/\n"
+ context "not-strict mode" do
+ subject { Aes.new(key: 'cake', strict_mode: false) }
+
+ its(:strict_mode) { should be_false }
+
+ describe "#encrypt" do
+ specify { expect { subject.encrypt('') }.not_to raise_error(ArgumentError) }
end
- specify { decrypted.should == 'string' }
+ describe "#decrypt" do
+ specify { expect { subject.decrypt('') }.not_to raise_error(ArgumentError) }
+ end
+
+ describe "#encryptable?" do
+ specify { subject.send(:encryptable?, '').should be_true }
+ specify { subject.send(:encryptable?, ' ').should be_true }
+ end
end
end
end
Something went wrong with that request. Please try again.