Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix random OpenSSL::Cipher::CipherErrors/iv argument errors #320

Conversation

bfreese
Copy link
Contributor

@bfreese bfreese commented Aug 17, 2018

This fixes defect in which encrypted_attributes state is shared across all instances due to shallow copy which causes random OpenSSL::Cipher::CipherErrors or iv argument errors during concurrent encrypts/decrypts under load.

An example (might need to increase thread count to replicate):

def concurrent_test(threads = 5)
  threads.times do
    Thread.new do
      begin
        2000.times do
          customer = Customer.new
          customer.ssn = '123456789'
          other_customer = Customer.new(encrypted_ssn: customer.encrypted_ssn, encrypted_ssn_iv: customer.encrypted_ssn_iv)
          customer.ssn = '555006666'
          other_customer.ssn
        end
      rescue => e
        puts "ERROR: #{e.inspect}"
      end
    end
  end
end

…l instances of a given class due to shallow dup. This caused random OpenSSL::Cipher::CipherError errors, particularly in cases in which concurrent encrypts/decrypts were occurring.
@amrocco
Copy link

amrocco commented Sep 18, 2018

@saghaulor Any reason this hasn't been merged in yet, outside of not having had the time? I believe this is also at the root of the issue being described here: #323

@amrocco
Copy link

amrocco commented Sep 18, 2018

@bfreese Has this solution been working well for you?

@bfreese
Copy link
Contributor Author

bfreese commented Sep 19, 2018

@amrocco We have been running this code in production for the last 3 months and have not seen any errors.

@amrocco
Copy link

amrocco commented Sep 21, 2018

@bfreese This fix worked like a charm, thanks so much! You're a lifesaver.

I'm curious about how you debugged this issue and arrived at your solution?

@taltcher
Copy link

taltcher commented Oct 7, 2018

We are also experiencing the same issue, and receive periodic must specify an iv error. Do you have an estimation when the PR will be merged?

@saghaulor saghaulor merged commit ee82b3b into attr-encrypted:master Nov 13, 2018
@mgerst
Copy link

mgerst commented Jun 19, 2019

We are experiencing this issue as well. Are there plans to cut a release soon (see also #348)? In the mean time has anyone had any luck safely running off master?

@bfreese
Copy link
Contributor Author

bfreese commented Jun 19, 2019

We are experiencing this issue as well. Are there plans to cut a release soon (see also #348)? In the mean time has anyone had any luck safely running off master?

@mgerst We've been running off master in production for several months with no issues.

@ericrowley
Copy link

Does anyone know when they plan to get a release? We are still seeing this issue on our end.

@ayufan
Copy link

ayufan commented Jan 29, 2020

@saghaulor Do you plan to push a new tag with a new release including that fix? This seems to be critical (introducing data corruption) and breaking for multiple people (including us, GitLab) for multi-threaded environments.

@formigarafa
Copy link

FYI, This does not solve "must specify iv" error. See #344

@saghaulor
Copy link
Contributor

I'll try to push a release this weekend. I apologize for the long pause.

@snovity
Copy link

snovity commented Aug 18, 2020

Would be nice to get this fix out, it is affecting our prod environment because we use sidekiq and threads.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants