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

Provide a default block cipher implementation using AES. #8943

Closed
wants to merge 1 commit into from

Conversation

mulugetam
Copy link
Contributor

@mulugetam mulugetam commented Sep 21, 2021

RocksDB currently has no default block cipher that users can readily use, this patch addresses that. The PR is the latest version of the PR described here. The code has been refactored to make use of the new Encryption API based on object registration. The block cipher used is the AES block cipher with CTR mode of operation.

Summary:

  • supports AES-128, AES-192, and AES-256.
  • uses the CTR mode of operation.
  • example: examples/ippcp_aes_example.cc
  • is based on the Intel® crypto library -- https://github.com/intel/ipp-crypto.

#include <cstdint>
#include <emmintrin.h>
int main() {
const auto x = _mm_set_epi32(0, 0, 0, 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

I would just roll this into the test for enabling IPPCP and don't bother with a HAVE_SSE2 flag. Unnecessary complexity IMHO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pdillinger that is true. Now fixed.

@mulugetam mulugetam force-pushed the aes-encryption branch 3 times, most recently from 44ba35c to 9a05662 Compare September 27, 2021 04:12
Copy link
Contributor

@pdillinger pdillinger left a comment

Choose a reason for hiding this comment

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

We don't like adding code to the base checkout without basic build and unit test coverage, with CircleCI and gtest. Can you add to db_encryption_test? Or perhaps make this the canonical configuration for environment variable ENCRYPTED_ENV=1 (when IPPCP enabled at compile time)? Can add this to CircleCI build-linux-encrypted-env.

//
// Note: a prefix size of 4096 (4K) is chosen for optimal performance.
//
class IppAESCTRProvider : public EncryptionProvider {
Copy link
Contributor

Choose a reason for hiding this comment

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

Also override GetMarker()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

virtual ~IppAESCTRProvider();

private:
#ifdef IPPCP
Copy link
Contributor

Choose a reason for hiding this comment

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

Pretty much except for ROCKSDB_LITE, we need to avoid custom ifdefs in public headers. We cannot rely on client code being compiled with our macro definitions used for compiling RocksDB.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@pdillinger
Copy link
Contributor

Wish list item: good error message if attempting to use sst_dump or ldb or DB::Open on an encrypted DB without appropriate encrypted env.

@mulugetam
Copy link
Contributor Author

Wish list item: good error message if attempting to use sst_dump or ldb or DB::Open on an encrypted DB without appropriate encrypted env.

Can we have a separate patch for this? A good error message is required even for the trivial encryption provider (ROT13) that currently exists and we need to carefully think on how to use the EncryptionProvider::GetMarker().

@mulugetam
Copy link
Contributor Author

mulugetam commented Oct 7, 2021

We don't like adding code to the base checkout without basic build and unit test coverage, with CircleCI and gtest. Can you add to db_encryption_test? Or perhaps make this the canonical configuration for environment variable ENCRYPTED_ENV=1 (when IPPCP enabled at compile time)? Can add this to CircleCI build-linux-encrypted-env.

I have added IPPCP encryption provider as the canonical encryption provider for test when ENCRYPTED_ENV is set to 1 and IPPCP is detected at compile time. I have also run db_encryption_test and it passes.

Summary:
  - supports AES-128, AES-192, and AES-256.
  - uses the CTR mode of operation.
  - is based on the Intel® crypto library (https://github.com/intel/ipp-crypto)
@franklu1
Copy link

I saw this pull request is pending for one year, are you plan to merge it in the master? If not, what's the roadmap for support encryption/decryption as part of code base?

@mulugetam
Copy link
Contributor Author

mulugetam commented May 8, 2023

The latest version of this PR, implemented as a RocksDB plugin, is available here.

@mulugetam mulugetam closed this May 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants