PARQUET-1517: [C++] Crypto package updates to match the final spec #3520

ggershinsky · 2019-01-29T09:29:23Z

An initial version of crypto package is merged. This PR updates the crypto code to

conform the signed off specification (wire protocol updates, signature tag creation, AAD support, etc)
improve performance by extending cipher lifecycle to file writing/reading - instead of creating cipher on each encrypt/decrypt operation

pitrou

It looks like you're fixing some bugs here. Can you explain those briefly in the PR description? Also, do we have some regression tests for this that would deserve updating?

pitrou · 2019-01-29T12:25:29Z

cpp/src/parquet/util/crypto.cc

@@ -196,10 +204,16 @@ int ctr_encrypt(const uint8_t* plaintext, int plaintext_len, uint8_t* key, int k

  uint8_t iv[ctrIvLen];
  memset(iv, 0, ctrIvLen);
+  iv[ctrIvLen - 1] = 1;


Can you add a comment explaining the initialization here?

In Parquet, an input to the CTR cipher is an encryption key, a 16-byte IV and a plaintext. IVs are comprised of a 12-byte nonce and a 4-byte initial counter field. The first 31 bits of the initial counter field are set to 0, the last bit is set to 1.

Will add a comment in the code too.

ggershinsky · 2019-01-30T06:57:40Z

It looks like you're fixing some bugs here. Can you explain those briefly in the PR description? Also, do we have some regression tests for this that would deserve updating?

@pitrou , this PR is a part of developing a new Parquet feature - the columnar encryption. More specifically, here I update the crypto functions to match the encryption specification
https://github.com/apache/parquet-format/blob/encryption/Encryption.md ,
formally signed off by the Parquet community earlier this month. I'll add this to the PR description.

In the C++ Parquet, the encryption is comprised of #2555, this PR and potentially new ones we'll create in the future. We have regression tests for the Java Parquet encryption, will certainly add them to the C++ version.

thamht4190 · 2019-01-30T08:40:37Z

cpp/src/parquet/util/crypto.cc

+  std::copy(bufferSizeArray, bufferSizeArray + bufferSizeLen, ciphertext);
+  std::copy(iv, iv + ctrIvLen, ciphertext + bufferSizeLen);
+
+  return bufferSize;


Now the bufferSize will have more 4 bytes for bufferSizeLen, then I think now we should return bufferSize + bufferSizeLen? or the caller will have to add 4 bytes into bufferSize itself?

@thamht4190 You are right, it should be + sizeLen (or rather, bufferSize should include this addition). I'll update the code.

wesm · 2019-05-22T19:57:09Z

@ggershinsky can you add a PR description that summarizes the changes you've made here, for the project changelog?

wesm

I left some stylistic comments. Since we're trying to flatten out the package and remove parquet/util it might make sense to move this to parquet/encryption-internal.h/cc

wesm · 2019-05-22T19:57:51Z

cpp/src/parquet/util/crypto.cc

+constexpr int GCM_MODE = 0;
+constexpr int CTR_MODE = 1;
+constexpr int CTRIvLength = 16;
+constexpr int BufferSizeLength = 4;


Can you use style-conforming names for these constants in the form kCamelCase?

wesm · 2019-05-22T19:58:41Z

cpp/src/parquet/util/crypto.cc

-  explicit EvpCipher(int cipher, int key_len, int type) {
-    ctx_ = nullptr;
+AesEncryptor::AesEncryptor(ParquetCipher::type alg_id, int key_len, bool metadata,
+                           std::shared_ptr<std::list<AesEncryptor*>> all_encryptors) {


Is there a reason to use std::list instead of the more common std::vector?

wesm · 2019-05-22T20:00:13Z

cpp/src/parquet/util/crypto.cc

+    std::stringstream ss;
+    ss << "Crypto algorithm " << alg_id << " is not supported";
+    throw ParquetException(ss.str());
+  }


I'm generally not in favor of class constructors being able to fail. This maintains the status quo but you might consider doing argument validation in a factory static constructor and passing in sanitized arguments into a private ctor

wesm · 2019-05-22T21:27:43Z

cpp/src/parquet/util/crypto.cc

- private:
-  EVP_CIPHER_CTX* ctx_;
-};
+int AesEncryptor::CiphertextSizeDelta() { return ciphertext_size_delta_; }


ciphertext_size_delta() (since this is just accessing an attribute)

wesm · 2019-05-22T21:27:57Z

cpp/src/parquet/util/crypto.cc


-int gcm_encrypt(const uint8_t* plaintext, int plaintext_len, uint8_t* key, int key_len,
-                uint8_t* aad, int aad_len, uint8_t* ciphertext) {
+int AesEncryptor::gcm_encrypt(const uint8_t* plaintext, int plaintext_len, uint8_t* key,


Take the opportunity to rename this as GcmEncrypt?

wesm · 2019-05-22T21:34:24Z

cpp/src/parquet/util/crypto.h

-            uint8_t* plaintext);
+class AesEncryptor {
+ public:
+  // Can serve one key length only. Possible values: 16, 24, 32 bytes.


Use /// for comments (for the sake of doxygen)

wesm · 2019-05-22T21:34:38Z

cpp/src/parquet/util/crypto.h

+      EVP_CIPHER_CTX_free(ctx_);
+      ctx_ = NULLPTR;
+    }
+  }


Put these implementations in the .cc file?

wesm · 2019-05-22T21:34:53Z

cpp/src/parquet/util/crypto.h

+  }
+
+ private:
+  EVP_CIPHER_CTX* ctx_;


Can you hide this in a PIMPL? If this is an internal header then this is not necessary

wesm · 2019-05-22T21:35:06Z

cpp/src/parquet/util/crypto.h

+  }
+
+ private:
+  EVP_CIPHER_CTX* ctx_;


Same question re using PIMPL

wesm · 2019-05-22T21:35:50Z

cpp/src/parquet/util/crypto.h

@@ -18,29 +18,126 @@
 #ifndef PARQUET_UTIL_CRYPTO_H


Is PARQUET_EXPORT needed on any functions in here? If might be clearer to move this file to the top level of the Parquet codebase and call it encryption-internal.h if it is internal-only

ggershinsky · 2019-05-26T06:31:07Z

We went with Revital over these comments, all of them make sense to us, we'll make the respective changes.

wesm · 2019-05-28T14:38:40Z

Please be sure to run clang-format-7 (using make format) and make lint on the codebase -- it is currently failing lint checks

Address review comments

ggershinsky · 2019-05-29T12:26:59Z

@wesm , we've addressed your comments. If you're ok with the current version, please merge it, so the staging branch can pick up these 2 new files, upon rebase.

wesm · 2019-05-29T14:52:53Z

Thanks. Having a look now

wesm · 2019-05-29T14:54:51Z

FYI, it's a little bit odd to do code reviews on your fork as it's pseudoprivate

ggershinsky#6

In the future, can you adopt a process that permits the code review to be seen more obviously by the community? If you are collaborating on a PR, you can add each other as contributors on your fork and then have the code review on the PR itself

wesm

+1, thank you!

ggershinsky mentioned this pull request Jan 29, 2019

PARQUET-1300: [C++] Implement encrypted Parquet read and write support #2555

Closed

pitrou reviewed Jan 29, 2019

View reviewed changes

ggershinsky changed the title ~~PARQUET-1517: [C++] Crypto package updates~~ PARQUET-1517: [C++] Crypto package updates to match the final spec Jan 30, 2019

thamht4190 reviewed Jan 30, 2019

View reviewed changes

wesm force-pushed the master branch from 3088183 to 0c6b2d2 Compare February 18, 2019 19:34

wesm reviewed May 22, 2019

View reviewed changes

Gidon Gershinsky and others added 16 commits May 28, 2019 08:31

iv changes and buffer length

03ede65

set or check ciphertext length

8e7fe90

iv comment and buffer length

7ecdade

CTR IV fix

8961544

signed footer encryption

dfe98ee

cast fix

1a99725

params order

aa7b2ab

Add functions for AAD calculation and adjust code for API changes

333045b

Fix indentation in crypto.h and crypto.cc

73c8235

stateful encryptor objects

4d832d6

encryption size delta

9f68cab

rm old method

9fa9ef6

fix code style

6c599e9

ctr fixes

24795fa

fix build issue on MacOS

abd76a6

cipher wipe out

e784d9d

ggershinsky force-pushed the p1517-crypto-pack-updates branch from 4d51930 to e784d9d Compare May 28, 2019 05:32

Revital1 Eres added 3 commits May 29, 2019 10:05

Address review comments

9be898e

Delete encryption_internal.cc from CMakeLists.txt

535d0e2

Fixes following Gidon's comments

b570e8e

Revital1 Eres and others added 3 commits May 29, 2019 15:04

Change comment in encryption_internal.h

3ffd606

Fix broken line

ef970e3

Merge pull request #6 from revital76/review_comments

21ce9d0

Address review comments

wesm approved these changes May 29, 2019

View reviewed changes

wesm closed this in f199a77 May 29, 2019

ggershinsky deleted the p1517-crypto-pack-updates branch February 18, 2020 11:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PARQUET-1517: [C++] Crypto package updates to match the final spec #3520

PARQUET-1517: [C++] Crypto package updates to match the final spec #3520

ggershinsky commented Jan 29, 2019 •

edited

Loading

pitrou left a comment

pitrou Jan 29, 2019

ggershinsky Jan 30, 2019

ggershinsky commented Jan 30, 2019

thamht4190 Jan 30, 2019 •

edited

Loading

ggershinsky Jan 30, 2019

wesm commented May 22, 2019

wesm left a comment

wesm May 22, 2019

wesm May 22, 2019

wesm May 22, 2019

wesm May 22, 2019

wesm May 22, 2019

wesm May 22, 2019

wesm May 22, 2019

wesm May 22, 2019

wesm May 22, 2019

wesm May 22, 2019

ggershinsky commented May 26, 2019

wesm commented May 28, 2019

ggershinsky commented May 29, 2019

wesm commented May 29, 2019

wesm commented May 29, 2019

wesm left a comment

PARQUET-1517: [C++] Crypto package updates to match the final spec #3520

PARQUET-1517: [C++] Crypto package updates to match the final spec #3520

Conversation

ggershinsky commented Jan 29, 2019 • edited Loading

pitrou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggershinsky commented Jan 30, 2019

thamht4190 Jan 30, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wesm commented May 22, 2019

wesm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggershinsky commented May 26, 2019

wesm commented May 28, 2019

ggershinsky commented May 29, 2019

wesm commented May 29, 2019

wesm commented May 29, 2019

wesm left a comment

Choose a reason for hiding this comment

ggershinsky commented Jan 29, 2019 •

edited

Loading

thamht4190 Jan 30, 2019 •

edited

Loading