[CHEF-5356-gcm(2)] Encrypted data bags should use different HMAC key and include the IV in the HMAC

[zuazo]

Note: issue #1590 rebased onto master, conflicts fixed.

Reopening issue #1504 with some fixes (alternative to issue #1474 using GCM).

https://tickets.opscode.com/browse/CHEF-5356

• Requires OpenSSL >= 1.0.1 and ruby >= 2.
• Avoid testing v3 when requirements are not met.
• DOC_CHANGES.md updated.
• Rebased onto master.

[sersut]

Thanks for the quick patch @zuazo. We will review this further but one thing that came up is to catch the OpenSSL error when >= 1.0.1 is not there and give a more meaningful error message to the user.

Stay tuned for more review 😄

[danielsdeleo]

BTW, here's what happens for unsupported ciphers:

irb(main):003:0> OpenSSL::Cipher::Cipher.new('unknown-thing')
RuntimeError: unsupported cipher algorithm (unknown-thing)

[zuazo]

@sersut, @danielsdeleo, thanks for your review and your help 😃

I added an Assertions module with the GCM support check to avoid code duplication and moved there the other assertions. Let me know if you do not like this way to approach it.

Note: And by the way, using Cipher::Cipher instead of Cipher is deprecated since Ruby 1.8.7. Tell me if you want me to fix that.

[danielsdeleo]

@zuazo yeah, it would be good to fix deprecation warnings. Will the code work on 1.8.7 if we fix that?

[zuazo]

@danielsdeleo, yes, it works with 1.8.7. From the official 1.8.7 documentation of Cipher::Cipher:

http://ruby-doc.org/stdlib-1.8.7/libdoc/openssl/rdoc/OpenSSL/Cipher/Cipher.html:

This class is only provided for backwards compatibility. Use OpenSSL::Digest in the future.

This is very old. The change was added to Ruby in the commit 953e8aca (2007-04-05) and merged onto 1.8.7 in commit 18342ff8 (2007-06-08). Ruby 1.8.7 was released in 2008-05-31.

[zuazo]

But, of course, you still require Ruby 2 to use GCM.

[danielsdeleo]

@zuazo sounds good. We just need to be able to run tests and such against ruby 1.8 until we drop support for it (which we're planning for Chef 12.0). Otherwise we'd have to wait to merge it.

[zuazo]

I removed the Cipher::Cipher class usage and the Ruby 1.8.7 tests passed. But others have failed.

Ruby 1.9.3 tests results:

No output has been received in the last 10 minutes, this potentially indicates a stalled build or something wrong with the build itself.
The build has been terminated

Ruby 2.0.0 tests results:

1. knife show When the Chef server has one of each thing When the local repository also has one of each thing knife show /cookbooks/x/metadata.rb shows the remote version
   Failure/Error: knife('show /cookbooks/x/metadata.rb').should_succeed <<EOM
   NoMethodError:
   undefined method 'shutdown' for nil:NilClass
   # /home/travis/build/opscode/chef/lib/chef/local_mode.rb:79:in 'destroy_server_connectivity'
   # /home/travis/build/opscode/chef/lib/chef/local_mode.rb:40:in 'with_server_connectivity'
   # /home/travis/build/opscode/chef/lib/chef/knife.rb:492:in 'run_with_pretty_exceptions'
   # /home/travis/build/opscode/chef/spec/support/shared/integration/knife_support.rb:86:in 'block in knife'
   # /home/travis/build/opscode/chef/spec/support/shared/integration/knife_support.rb:40:in 'knife'
   # /home/travis/build/opscode/chef/spec/integration/knife/show_spec.rb:48:in 'block (4 levels) in <top (required)>'

They seem to be random unrelated errors. Can someone please review this errors or re-run the failed jobs?

[danielsdeleo]

Re-running the tests now.

[zuazo]

Thanks @danielsdeleo. Tests passed.

[danielsdeleo]

Is it possible to put this logic in the rspec config in spec_helper? Personally I don't have a preference for one style or the other, but everything is in spec_helper right now so I'd like to be consistent.

[danielsdeleo]

Overall it looks good except for the nitpicks about how the tests handle the platform requirements.

[zuazo]

@danielsdeleo, totally agree with you. I added RSpec filters and added some tests using stubs instead of conditionals. I have not removed the conditional tests because I think that makes sense considering that I have not checked the GCM support according to the version, but according to functionality:

def assert_aead_requirements_met!(algorithm)
  unless OpenSSL::Cipher.method_defined?(:auth_data=)
    raise # ...
  end
  unless OpenSSL::Cipher.ciphers.include?(algorithm)
    raise # ...
  end
end

So these conditional tests are used to verify that the functionality checks are fine.

[danielsdeleo]

Do we actually call these methods on this module? I didn't see any case where we do.

[zuazo]

Decryptor module calls #assert_format_version_acceptable! below. I could have avoided moving that function to Assertions. But I moved to have all the assertions together.

[danielsdeleo]

@zuazo Ok, your explanation for making the tests conditional makes sense to me. I found one more thing I'm curious about, and everything else looks good.

[zuazo]

Thanks @danielsdeleo. I've made ​​some small fixes. Nothing important.

[danielsdeleo]

Okay, this looks good to me now, 👍