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

Decrypt S3 files #25

Merged
merged 8 commits into from Apr 17, 2014
Merged

Decrypt S3 files #25

merged 8 commits into from Apr 17, 2014

Conversation

joekiller
Copy link
Collaborator

I saw that you would welcome a pull request such as this on the chef cookbook forums. I developed the decryption technique while using a s3 method that used the aws-sdk but this cookbook allows to do without the sdk so I have integrated (and improved!) our decryption feature. Might as well share it.

This provides a method to decrypt S3 files on the fly. Files that are sensitive can be encrypted using the AES-256-CBC cipher by a 256bit (32 character) key that is kept secret.

The pull request provides utilities in the bin directory for facilitating encryption and decryption of files prior to putting them to S3 and a utility to generate a valid SHA256 key.

@adamsb6
Copy link
Owner

adamsb6 commented Apr 15, 2014

Thanks for the contribution! One thing I'd like to see before merging is to replace the few bits that use Trollop with either something in the Ruby standard library or something that gets installed along with Chef. Can you make that change?

One of the goals of this LWRP is to be free of dependencies that aren't included in Chef. The only library in use that's not part of stdlib is rest-client, and that's because rest-client is already installed as a Chef dependency.

@joekiller
Copy link
Collaborator Author

Okay check it out now. I unified the helper functions and made it only use core libs.

@adamsb6
Copy link
Owner

adamsb6 commented Apr 15, 2014

Thanks! I'm running it through tests in my staging environment now.

Brandon Adams
360.510.2837

On Tue, Apr 15, 2014 at 12:53 PM, Joseph Lawson notifications@github.comwrote:

Okay check it out now. I unified the helper functions and made it only use
core libs.


Reply to this email directly or view it on GitHubhttps://github.com//pull/25#issuecomment-40526233
.

@joekiller
Copy link
Collaborator Author

Yeah I think I found one bug so hold off until I say please.

@joekiller
Copy link
Collaborator Author

Deff slightly busted. I'll fix this tomorrow.

@adamsb6
Copy link
Owner

adamsb6 commented Apr 15, 2014

No problem. FWIW, my staging tests passed, but I wasn't exercising the new crypto features. When I attempted to use the s3_crypto utility to encrypt a file I got this:

bin/s3_crypto:91:in `aes256_encrypt': undefined local variable or method `encrypted_file' for main:Object (NameError)
    from bin/s3_crypto:141:in `<main>'

@joekiller
Copy link
Collaborator Author

Thanks. Doing like 8 changes at once isn't always advised. I'll work these out and then this should be a nice feature.

@joekiller
Copy link
Collaborator Author

FYI this should be good to go now. I also updated the MD5 checksum to use a buffered read to help with memory consumption and added a parameter called decrypted_file_checksum which is the SHA256 (keeping in remote_file spirit even though this doesn't extend it) checksum of the decrypted file so that the resource can determine if it really needs to download the file again.

local_md5 = Digest::MD5.hexdigest(::File.read(new_resource.path))
if decryption_key.nil?
if new_resource.decrypted_file_checksum.nil?
s3_md5 = S3FileLib::get_md5_from_s3(bucket, remote_path, access_key_id, secret_access_key, token)
Copy link
Owner

Choose a reason for hiding this comment

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

Line 38 of providers/default.rb is throwing errors in my testing environment. It looks like the arguments are a little off. They should be (new_resource.bucket, remote_path, aws_access_key_id, aws_secret_access_key, token).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You are correct. When porting the changes back (we just create a s3_file
provider which extends remote_file and install it directly to chef) I
forgot that bucket was a new reference. I'll change it when I get to q PC.
Did you encounter any other errors if that was corrected?
On Apr 16, 2014 5:54 PM, "Brandon Adams" notifications@github.com wrote:

In providers/default.rb:

@@ -32,15 +33,30 @@
end

if ::File.exists?(new_resource.path)

  • s3_md5 = S3FileLib::get_md5_from_s3(new_resource.bucket, remote_path, aws_access_key_id, aws_secret_access_key, token)
  • local_md5 = Digest::MD5.hexdigest(::File.read(new_resource.path))
  • if decryption_key.nil?
  •  if new_resource.decrypted_file_checksum.nil?
    
  •    s3_md5 = S3FileLib::get_md5_from_s3(bucket, remote_path, access_key_id, secret_access_key, token)
    

Line 38 of providers/default.rb is throwing errors in my testing
environment. It looks like the arguments are a little off. They should be
(new_resource.bucket, remote_path, aws_access_key_id,
aws_secret_access_key, token).

Reply to this email directly or view it on GitHubhttps://github.com//pull/25/files#r11709629
.

@joekiller
Copy link
Collaborator Author

Okay fixed the bad md5 call.

@adamsb6
Copy link
Owner

adamsb6 commented Apr 17, 2014

Thanks, this is passing my tests, and I've been able to use the tool in bin/ to successfully encrypt and decrypt a file.

I'm going to run one more test tomorrow to download and decrypt a file in a Chef recipe. If that works, I'll merge, rev versions, update the changelog, and push to the community site.

adamsb6 added a commit that referenced this pull request Apr 17, 2014
@adamsb6 adamsb6 merged commit 972cfc6 into adamsb6:master Apr 17, 2014
@adamsb6
Copy link
Owner

adamsb6 commented Apr 17, 2014

Tests passed! Thanks!

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

2 participants