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

Encryption at rest support #2424

Closed
wants to merge 13 commits into from
Closed

Encryption at rest support #2424

wants to merge 13 commits into from

Conversation

@ewoutp
Copy link
Contributor

ewoutp commented Jun 8, 2017

What

This PR adds support for encrypting data stored by RocksDB when written to disk.

How

It adds an EncryptedEnv override of the Env class with matching overrides for sequential&random access files.
The encryption itself is done through a configurable EncryptionProvider. This class creates is asked to create BlockAccessCipherStream for a file. This is where the actual encryption/decryption is being done.
Currently there is a Counter mode implementation of BlockAccessCipherStream with a ROT13 block cipher (NOTE the ROT13 is for demo purposes only!!).

The Counter operation mode uses an initial counter & random initialization vector (IV).
Both are created randomly for each file and stored in a 4K (default size) block that is prefixed to that file. The EncryptedEnv implementation is such that clients of the Env class do not see this prefix (nor data, nor in filesize).
The largest part of the prefix block is also encrypted, and there is room left for implementation specific settings/values/keys in there.

Testing

To test the encryption, the DBTestBase class has been extended to consider a new environment variable called ENCRYPTED_ENV. If set, the test will setup a encrypted instance of the Env class to use for all tests.
Typically you would run it like this:

ENCRYPTED_ENV=1 make check_some

There is also an added test that checks that some data inserted into the database is or is not "visible" on disk. With ENCRYPTED_ENV active it must not find plain text strings, with ENCRYPTED_ENV unset, it must find the plain text strings.

ewoutp added 2 commits May 12, 2017
Fetch updates from facebook/rocksdb
Encryption
@facebook-github-bot

This comment has been minimized.

Copy link

facebook-github-bot commented Jun 8, 2017

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@siying siying requested review from ajkr and lightmark Jun 12, 2017
db/db_encryption_test.cc Outdated Show resolved Hide resolved
env/env_encryption.cc Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
@sagar0

This comment has been minimized.

Copy link
Contributor

sagar0 commented Jun 12, 2017

Thanks for your contribution!
Seems like Travis tests failed to kickoff, and there's also a merge conflict; so could you please rebase and submit again?

db/db_encryption_test.cc Outdated Show resolved Hide resolved
@facebook-github-bot

This comment has been minimized.

Copy link

facebook-github-bot commented Jun 13, 2017

@ewoutp updated the pull request - view changes

ewoutp added 2 commits Jun 13, 2017
Import changes from facebook-master
@facebook-github-bot

This comment has been minimized.

Copy link

facebook-github-bot commented Jun 13, 2017

@ewoutp updated the pull request - view changes

@facebook-github-bot

This comment has been minimized.

Copy link

facebook-github-bot commented Jun 13, 2017

@ewoutp updated the pull request - view changes

@facebook-github-bot

This comment has been minimized.

Copy link

facebook-github-bot commented Jun 13, 2017

@ewoutp updated the pull request - view changes

@facebook-github-bot

This comment has been minimized.

Copy link

facebook-github-bot commented Jun 13, 2017

@ewoutp updated the pull request - view changes

@sagar0

This comment has been minimized.

Copy link
Contributor

sagar0 commented Jun 14, 2017

Travis lite_builds are failing on both linux and mac. Can you please take a look?
Also, I just restarted linux platform_dependent test, as it failed with 'No space left on device' error. (rate_limiter test in Mac platform_dependent test suite has been consistently failing due to unrelated issues. So we can disregard it for now).

@ewoutp

This comment has been minimized.

Copy link
Contributor Author

ewoutp commented Jun 14, 2017

@sagar0 Shall I just wrap the entire file in #ifndef ROCKSDB_LITE?

@facebook-github-bot

This comment has been minimized.

Copy link

facebook-github-bot commented Jun 14, 2017

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@facebook-github-bot

This comment has been minimized.

Copy link

facebook-github-bot commented Jun 15, 2017

@ewoutp updated the pull request - view changes

@facebook-github-bot

This comment has been minimized.

Copy link

facebook-github-bot commented Jun 15, 2017

@ewoutp updated the pull request - view changes

@ewoutp

This comment has been minimized.

Copy link
Contributor Author

ewoutp commented Jun 15, 2017

Only java_test is failing. Have no clue how that is related.
Any ideas?

@sagar0

This comment has been minimized.

Copy link
Contributor

sagar0 commented Jun 15, 2017

Thanks for fixing the lite build. Java test is a little flaky ... so we don't need to worry about it most of the time (we are working on making it less flaky though). I'll take a look.

ObiWahn added a commit to obiwahn-abandoned/rocksdb that referenced this pull request Jun 16, 2017
commit 19ee74c
Author: Ewout Prangsma <ewout@prangsma.net>
Date:   Thu Jun 15 09:37:40 2017 +0200

    Excluded headers on ROCKSDB_LITE

commit c688186
Author: Ewout Prangsma <ewout@prangsma.net>
Date:   Thu Jun 15 08:21:34 2017 +0200

    Exclude encryption implementation on ROCKSDB_LITE

commit 28d8074
Author: Ewout Prangsma <ewout@prangsma.net>
Date:   Tue Jun 13 16:27:08 2017 +0200

    Commented `std::cout` in test (see review facebook#2424 (review))

commit d677055
Author: Ewout Prangsma <ewout@prangsma.net>
Date:   Tue Jun 13 16:25:18 2017 +0200

    db_encryption_test.cc to TARGETS file (see review facebook#2424 (review))

commit 47c909b
Author: Ewout Prangsma <ewout@prangsma.net>
Date:   Tue Jun 13 16:22:50 2017 +0200

    Removed unwanted likes (see review facebook#2424 (review))

commit 64fccc8
Merge: b452426 66a5ef2
Author: Ewout Prangsma <ewoutp@users.noreply.github.com>
Date:   Tue Jun 13 16:18:08 2017 +0200

    Merge pull request facebook#5 from arangodb-helper/facebook-master

    Import changes from facebook-master

commit 66a5ef2
Merge: b452426 5d5a28a
Author: Ewout Prangsma <ewout@prangsma.net>
Date:   Tue Jun 13 15:39:25 2017 +0200

    Imported changed from facebook-master

commit b452426
Author: Ewout Prangsma <ewout@prangsma.net>
Date:   Tue Jun 13 15:27:51 2017 +0200

    Added GenerateUniqueId override

commit 9d446a6
Author: Ewout Prangsma <ewoutp@users.noreply.github.com>
Date:   Thu Jun 8 08:55:29 2017 +0200

    Encryption (wip) (facebook#2)

    Encryption

commit 8d846d8
Merge: fe18356 c2be434
Author: Ewout Prangsma <ewoutp@users.noreply.github.com>
Date:   Fri May 12 15:33:34 2017 +0200

    Merge pull request facebook#1 from facebook/master

    Fetch updates from facebook/rocksdb
Copy link
Contributor

sagar0 left a comment

Thanks for the contribution, and making the prior requested changes. This change looks good to me, and is ready to be accepted once the below few nits are fixed.

Could you run a "make format" once to adhere to the formatting rules.

TARGETS Show resolved Hide resolved
src.mk Show resolved Hide resolved
env/env_encryption.cc Outdated Show resolved Hide resolved
@sagar0

This comment has been minimized.

Copy link
Contributor

sagar0 commented Jun 22, 2017

Would it also be possible to contribute a small example to show how this API can be used? It can be done in a separate pull request, when time permits.

@facebook-github-bot

This comment has been minimized.

Copy link

facebook-github-bot commented Jun 23, 2017

@ewoutp updated the pull request - view changes

@ewoutp

This comment has been minimized.

Copy link
Contributor Author

ewoutp commented Jun 23, 2017

Result of make format

$ make format
Makefile:105: Warning: Compiling in debug mode. Don't use the resulting binary in production
build_tools/format-diff.sh
Nothing needs to be reformatted!
@ewoutp

This comment has been minimized.

Copy link
Contributor Author

ewoutp commented Jun 23, 2017

As for sample code.

Typically you would implement a class derived from BlockCipher (see ROT13Cipher) and use it as follows:

ROT13BlockCipher rot13Cipher(16);
// ...
DBOptions options;
options.env = NewEncryptedEnv(Env::Default(), new CTREncryptionProvider(rot13Cipher));
// use options for opening/creating databases
@facebook-github-bot

This comment has been minimized.

Copy link

facebook-github-bot commented Jun 23, 2017

@ewoutp updated the pull request - view changes

@sagar0

This comment has been minimized.

Copy link
Contributor

sagar0 commented Jun 23, 2017

I am trying to import this PR on to Facebook's internal phabricator to run some tests, but unfortunately the import is failing. I am trying to figure out a way to move forward.

@facebook-github-bot

This comment has been minimized.

Copy link

facebook-github-bot commented Jun 26, 2017

@ewoutp updated the pull request - view changes

@sagar0

This comment has been minimized.

Copy link
Contributor

sagar0 commented Jun 26, 2017

@ewoutp Can you please rebase your changes on master?
We are encountering an issue when trying to import this PR onto our internal phabricator for running additional tests, and our workflow does not let us to merge the changes without that step.
I appreciate your patience while we are working through these kinks.

@facebook-github-bot

This comment has been minimized.

Copy link

facebook-github-bot commented Jun 26, 2017

@sdwilsh has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@ewoutp

This comment has been minimized.

Copy link
Contributor Author

ewoutp commented Jun 27, 2017

@sagar0 looks like it's already merged. If anything is still needed, let me know.

@sagar0

This comment has been minimized.

Copy link
Contributor

sagar0 commented Jun 27, 2017

@ewoutp yeah, I merged it yesterday. @sdwilsh fixed an issue on our side in the import script, and helped in the process.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.