PARQUET-1301: [C++] Crypto package in parquet-cpp #464

ggershinsky · 2018-05-15T07:53:10Z

No description provided.

jamesclampffer · 2018-06-14T14:57:19Z

src/parquet/util/crypto.h

+#include "parquet/types.h" 
+
+namespace parquet {
+


Can the definition of encrypt and decrypt be pushed into a .cc file? There's enough going on here that call overhead is negligible and splitting it up will make it easier to read and reduce build times.

jamesclampffer · 2018-06-14T14:58:57Z

src/parquet/util/crypto.h

+
+  if (Encryption::PARQUET_AES_GCM_V1 != alg_id) {
+    std::stringstream ss;
+    ss << "Crypto algorithm " << alg_id << " currently unsupported\n";


Nit: remove the trailing newline in the exception descriptions to be consistent with other error strings.

jamesclampffer · 2018-06-14T14:59:51Z

src/parquet/util/crypto.h

+  int plaintext_len;
+  int ret;
+
+  int tag_len = 16;


Make these const? Same thing in encrypt.

jamesclampffer · 2018-06-14T15:03:43Z

src/parquet/util/crypto.h

+    throw parquet::ParquetException(ss.str());
+  }
+
+  EVP_CIPHER_CTX *ctx;


Consider initializing this set of variables to something invalid. Makes it a little easier to figure out what's going on when looking at core files.

jamesclampffer · 2018-06-14T15:37:50Z

src/parquet/util/crypto.h

+    throw parquet::ParquetException(ss.str());
+  }
+
+  EVP_CIPHER_CTX *ctx;


Manage ctx with RAII; otherwise any exception after line 63 is going to leak memory.

You could just use a unique_ptr with a custom deleter e.g.

struct EVPContextDeleter { void operator()(EVP_CIPHER_CTX *ctx) { if(ctx) { // would need to initialize to null in case there's a throw before the init EVP_CIPHER_CTX_free(ctx); } } }; ... stuff ... std::unique_ptr<EVP_CYPHER_CTX, EVPContextDeleter> ctx;

thamht4190 · 2018-06-21T07:02:11Z

src/parquet/util/crypto.h

+  int len;
+  int ciphertext_len;
+  uint8_t tag[tag_len];
+  uint8_t iv[iv_len];


I think that letting tag_len and iv_len be const will be more proper in C++

thamht4190 · 2018-06-28T04:07:42Z

src/parquet/util/crypto.cc

+#include "parquet/exception.h"
+#include "parquet/types.h" 
+
+#include "parquet/util/crypto.h"


I have a build issue on Windows in types.h (line 93), it recognize OPTIONAL as something else rather than an enum. When I include openssl/*.h files below line 28 (must be included after types.h) in crypto.cc, it works. It seems that there is some macro on openssl related to OPTIONAL. So could you please change the order of include here?

Yup, will do.

thamht4190 · 2018-06-29T02:41:44Z

src/parquet/util/crypto.cc

+#include <iostream>
+#include "parquet/exception.h"
+#include "parquet/util/crypto.h"
+#include "parquet/types.h" 


actually, this line #include "parquet/types.h" is not necessary because the file is included in crypto.h as well. What I really mean is:

#include "parquet/util/crypto.h"

#include <openssl/aes.h>
#include <openssl/evp.h>
#include <openssl/rand.h>

majetideepak · 2018-07-15T21:15:24Z

src/parquet/util/crypto.cc

+
+namespace parquet {
+
+const int gcm_tag_len = 16;


constexpr is more suited here since the values are known at compile time.
Same for the other two below

majetideepak · 2018-07-15T21:17:01Z

src/parquet/util/crypto.cc

+const int ctr_iv_len = 16;
+
+void handleError(const char *message, EVP_CIPHER_CTX *ctx) {
+  if (NULL != ctx) EVP_CIPHER_CTX_free(ctx);


nullptr instead of NULL

please use {} even for a single statement if condition.

handleError does not need EVP_CIPHER_CTX *ctx argument if we use EvpCipherCtxPtr

majetideepak · 2018-07-15T21:17:57Z

src/parquet/util/crypto.cc

+                   uint8_t *key, int key_len, uint8_t *aad, int aad_len, 
+                   uint8_t *ciphertext) 
+{
+  EVP_CIPHER_CTX *ctx = NULL;


using a smart pointer with a custom deleter will simplify freeing resources.
Define the following on top and use EvpCipherCtxPtr ctx(EVP_CIPHER_CTX_new()); everywhere

struct EvpCipherCtxDeleter { void operator()(EVP_CIPHER_CTX *ctx) const { if (nullptr != ctx) { EVP_CIPHER_CTX_free(ctx); } } }; using EvpCipherCtxPtr = std::unique_ptr<EVP_CIPHER_CTX, EvpCipherCtxDeleter>;

majetideepak · 2018-07-15T21:26:07Z

src/parquet/util/crypto.cc

+  uint8_t iv[gcm_iv_len];
+
+  // Random IV
+  RAND_load_file("/dev/urandom", 32);


declare constexpr long max_bytes = 32; on top and then use it here?

majetideepak · 2018-07-15T21:26:38Z

src/parquet/util/crypto.cc

+  uint8_t iv[ctr_iv_len];
+
+  // Random IV
+  RAND_load_file("/dev/urandom", 32);


declare constexpr long max_bytes = 32; on top and then use it here?

majetideepak · 2018-07-15T22:15:00Z

src/parquet/util/crypto.cc

+    throw parquet::ParquetException(ss.str());
+  }
+
+  if (16 != key_len) {


declare constexpr int key_length = 16; on top and then use it here?

majetideepak · 2018-07-15T22:15:28Z

src/parquet/util/crypto.cc

+                   uint8_t *key, int key_len, uint8_t *aad, int aad_len, 
+                   uint8_t *plaintext)
+{
+  EVP_CIPHER_CTX *ctx = NULL;


use EvpCipherCtxPtr ctx(EVP_CIPHER_CTX_new());

majetideepak · 2018-07-15T22:16:33Z

src/parquet/util/crypto.cc

+}
+
+int gcm_decrypt(const uint8_t *ciphertext, int ciphertext_len,  
+                   uint8_t *key, int key_len, uint8_t *aad, int aad_len, 


is key_len used in the body?

majetideepak · 2018-07-15T22:17:22Z

src/parquet/util/crypto.cc

+}
+
+int ctr_decrypt(const uint8_t *ciphertext, int ciphertext_len,  
+                   uint8_t *key, int key_len, uint8_t *plaintext)


I don't see key_len being used in the body

majetideepak · 2018-07-15T22:18:14Z

src/parquet/util/crypto.cc

+    throw parquet::ParquetException(ss.str());
+  }
+
+  if (16 != key_len) {


use key_length declared above.

majetideepak · 2018-07-15T22:19:32Z

Travis CI failure has been fixed on master. Can you please rebase?

wesm

Made some stylistic comments. clang-format (make format) will fix many of these problems

wesm · 2018-07-29T12:33:43Z

src/parquet/util/crypto.cc

+#include <string>
+#include <sstream>
+#include <iostream>
+#include "parquet/exception.h"


Order of includes does not follow our style guide

wesm · 2018-07-29T12:34:05Z

src/parquet/util/crypto.cc

+constexpr int gcm_tag_len = 16;
+constexpr int gcm_iv_len = 12;
+constexpr int ctr_iv_len = 16;
+constexpr long max_bytes = 32;


use kFooBarBaz naming style for global const/constexpr

wesm · 2018-07-29T12:34:24Z

src/parquet/util/crypto.cc

+constexpr long max_bytes = 32;
+
+struct EvpCipherCtxDeleter {
+  void operator()(EVP_CIPHER_CTX *ctx) const {


Consistent style for pointers is EVP_CIPHER_CTX* ctx

wesm · 2018-07-29T12:34:48Z

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) 
+{


Can you run clang-format on this file? Will fix a lot of the style issues

wesm · 2018-07-29T12:37:34Z

src/parquet/util/crypto.cc

+  if (16 == key_len) {
+    // Init AES-GCM with 128-bit key
+    if(1 != EVP_EncryptInit_ex(ctx.get(), EVP_aes_128_gcm(), nullptr, nullptr, nullptr)) {
+      throw ParquetException("Couldn't init AES_GCM_128 encryption");


Perhaps create a macro like

ENCRYPT_CHECK(EXPR, 1, "AES_GCM_128");

This will help with code verbosity throughout this file

wesm · 2018-07-29T12:38:25Z

src/parquet/util/crypto.h

+
+int decrypt(std::shared_ptr<EncryptionProperties> encryption_props, bool metadata,
+                   const uint8_t *ciphertext, int ciphertext_len,
+                   uint8_t *plaintext);


Are these public APIs? If not public should perhaps put them in an internal namespace

Also, can you capitalize the names of these functions to confirm with the Google style guide? Thanks

wesm · 2018-07-29T12:43:53Z

src/parquet/util/crypto.cc

+  }
+};
+
+using EvpCipherCtxPtr = std::unique_ptr<EVP_CIPHER_CTX, EvpCipherCtxDeleter>;


Why not use simply define a class/struct with a destructor? Why is std::unique_ptr necessary?

Suggested by @majetideepak and @jamesclampffer . Let me know guys which one should I use.

I think this is overly elaborate. Unless you expect to need to transfer ownership of a smart pointer to some other call frame, using RAII with a simple class with a destructor that frees the resource is the simplest solution

I usually prefer the std::unique_ptr approach to avoid creating a new class. std::unique_ptr follows the RAII design principle. I am OK with either RAII approaches.

Well the deleter has already created a new class. Which of these do you prefer?

struct EvpCipherCtxDeleter { void operator()(EVP_CIPHER_CTX *ctx) const { if (nullptr != ctx) { EVP_CIPHER_CTX_free(ctx); } } }; using EvpCipherCtxPtr = std::unique_ptr<EVP_CIPHER_CTX, EvpCipherCtxDeleter>; 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 len; int ciphertext_len; uint8_t tag[gcm_tag_len]; uint8_t iv[gcm_iv_len]; // Random IV RAND_load_file("/dev/urandom", max_bytes); RAND_bytes(iv, sizeof(iv)); // Init cipher context EvpCipherCtxPtr ctx(EVP_CIPHER_CTX_new()); if(nullptr == ctx.get()) { throw ParquetException("Couldn't init cipher context"); } if (16 == key_len) { // Init AES-GCM with 128-bit key if(1 != EVP_EncryptInit_ex(ctx.get(), EVP_aes_128_gcm(), nullptr, nullptr, nullptr)) { throw ParquetException("Couldn't init AES_GCM_128 encryption"); } } else if (24 == key_len) { // Init AES-GCM with 192-bit key if(1 != EVP_EncryptInit_ex(ctx.get(), EVP_aes_192_gcm(), nullptr, nullptr, nullptr)) { throw ParquetException("Couldn't init AES_GCM_192 encryption"); } } else if (32 == key_len) { // Init AES-GCM with 256-bit key if(1 != EVP_EncryptInit_ex(ctx.get(), EVP_aes_256_gcm(), nullptr, nullptr, nullptr)) { throw ParquetException("Couldn't init AES_GCM_256 encryption"); } } // Setting key and IV if(1 != EVP_EncryptInit_ex(ctx.get(), nullptr, nullptr, key, iv)) { throw ParquetException("Couldn't set key and IV"); } ...

or

class EvpCipher { public: explicit EvpCipher(int key_len) { ctx_ = EVP_CIPHER_CTX_new(); if(nullptr == ctx_) { throw ParquetException("Couldn't init cipher context"); } Init(key_len); } ~EvpCipher() { if (nullptr != ctx_) { EVP_CIPHER_CTX_free(ctx_); } } void Init(const key_len) { if (16 == key_len) { // Init AES-GCM with 128-bit key if(1 != EVP_EncryptInit_ex(ctx_, EVP_aes_128_gcm(), nullptr, nullptr, nullptr)) { throw ParquetException("Couldn't init AES_GCM_128 encryption"); } } else if (24 == key_len) { // Init AES-GCM with 192-bit key if(1 != EVP_EncryptInit_ex(ctx_, EVP_aes_192_gcm(), nullptr, nullptr, nullptr)) { throw ParquetException("Couldn't init AES_GCM_192 encryption"); } } else if (32 == key_len) { // Init AES-GCM with 256-bit key if(1 != EVP_EncryptInit_ex(ctx_, EVP_aes_256_gcm(), nullptr, nullptr, nullptr)) { throw ParquetException("Couldn't init AES_GCM_256 encryption"); } } // Setting key and IV if(1 != EVP_EncryptInit_ex(ctx_, nullptr, nullptr, key, iv)) { throw ParquetException("Couldn't set key and IV"); } } private: EVP_CIPHER_CTX ctx_; }; 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 len; int ciphertext_len; uint8_t tag[gcm_tag_len]; uint8_t iv[gcm_iv_len]; // Random IV RAND_load_file("/dev/urandom", max_bytes); RAND_bytes(iv, sizeof(iv)); EvpCipher cipher(key_len); ...

I think that encapsulation is a good idea

@wesm , @majetideepak Thank you for the comments and suggestions. I'm posting a new commit that takes them into account, let me know if additional changes are required.

wesm · 2018-08-01T13:51:56Z

Thanks, can you fix the cpplint failures while we review in the meantime?

Done processing /home/travis/build/apache/parquet-cpp/src/parquet/util/comparison-test.cc
/home/travis/build/apache/parquet-cpp/src/parquet/util/crypto.cc:37:  Use int16/int64/etc, rather than the C type long  [runtime/int] [4]
/home/travis/build/apache/parquet-cpp/src/parquet/util/crypto.cc:40:  If/else bodies with multiple statements require braces  [readability/braces] [4]
/home/travis/build/apache/parquet-cpp/src/parquet/util/crypto.cc:44:  If/else bodies with multiple statements require braces  [readability/braces] [4]
/home/travis/build/apache/parquet-cpp/src/parquet/util/crypto.cc:304:  Add #include <algorithm> for copy  [build/include_what_you_use] [4]
/home/travis/build/apache/parquet-cpp/src/parquet/util/crypto.h:24:  Do not use namespace using-directives.  Use using-declarations instead.  [build/namespaces] [5]

ggershinsky · 2018-08-01T14:20:15Z

Sure, will do.

majetideepak · 2018-08-01T20:08:36Z

src/parquet/util/crypto.cc

+constexpr int rndMaxBytes = 32;
+
+#define ENCRYPT_INIT(ALG)                                              \
+  if (1 != EVP_EncryptInit_ex(ctx_, ALG, nullptr, nullptr, nullptr)) { \


I think the coding conventions require only the class members names to end with an underscore. Same below

majetideepak · 2018-08-01T20:12:20Z

src/parquet/util/crypto.cc

+
+class EvpCipher {
+ public:
+  explicit EvpCipher(int cipher, int key_len, int type) {


ctx_ = nullptr here so that destructor does not see an uninitialized value

majetideepak · 2018-08-01T20:18:03Z

src/parquet/util/crypto.cc

+
+  // Setting additional authenticated data
+  if (nullptr != aad) {
+    if (1 != EVP_DecryptUpdate(cipher.get(), nullptr, &len, aad, aad_len)) {


Nit: combine the two if conditions?

majetideepak · 2018-08-01T20:24:25Z

src/parquet/util/crypto.cc

+  int len;
+  int plaintext_len;
+
+  uint8_t tag[gcmTagLen];


safer to zero-initialize all of them.

majetideepak

I think developers will benefit from a description of some of the related security concepts in the code or in the documentation.
You could cut-copy from the design document as well
https://docs.google.com/document/d/1T89G7xR0zHFV1f2pjTO28jtfVm8qoNVGEJQ70Rsk-bY/edit

ggershinsky · 2018-08-02T12:18:26Z

@majetideepak , your comments are factored in. As for the documentation, I'm working on an encryption.md doc, that will be published in the parquet-format repo, similar to docs for other Parquet features.

majetideepak

+1 LGTM. Thanks for making all the changes!

jamesclampffer reviewed Jun 14, 2018

View reviewed changes

thamht4190 reviewed Jun 21, 2018

View reviewed changes

thamht4190 reviewed Jun 28, 2018

View reviewed changes

thamht4190 reviewed Jun 29, 2018

View reviewed changes

majetideepak reviewed Jul 15, 2018

View reviewed changes

ggershinsky force-pushed the p1301-crypto-package branch from b9eabae to 9beef24 Compare July 29, 2018 06:09

wesm reviewed Jul 29, 2018

View reviewed changes

Gidon Gershinsky added 9 commits August 1, 2018 18:06

The C++ implementation of basic AES-GCM encryption and decryption

8e384d4

version fix

63fa9a3

2nd cipher + review comments

a11507e

version fix

fc5d77d

change import order

b3affc5

fix import order

35a4830

EncryptionProperties input and other review changes

20b496f

post-review changes

b5e40b1

fix lint issues

ad17fbe

ggershinsky force-pushed the p1301-crypto-package branch from ca10c85 to ad17fbe Compare August 1, 2018 15:07

majetideepak reviewed Aug 1, 2018

View reviewed changes

src/parquet/util/crypto.cc

int len;

int plaintext_len;

uint8_t tag[gcmTagLen];

Copy link

majetideepak Aug 1, 2018

Choose a reason for hiding this comment

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

safer to zero-initialize all of them.

majetideepak reviewed Aug 1, 2018

View reviewed changes

post-review changes

42fe125

majetideepak approved these changes Aug 2, 2018

View reviewed changes

majetideepak closed this in 63f41b0 Aug 4, 2018

This was referenced Jun 23, 2024

[C++][Parquet] rypto package in parquet-cpp apache/arrow#42821

Closed

[C++][Parquet] LONE - [C++] Crypto package in parquet-cpp apache/arrow#42966

Closed

PARQUET-1301: [C++] Crypto package in parquet-cpp #464

PARQUET-1301: [C++] Crypto package in parquet-cpp #464

Uh oh!

Conversation

ggershinsky commented May 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thamht4190 Jun 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thamht4190 Jun 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

majetideepak commented Jul 15, 2018

Uh oh!

wesm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

thamht4190 Jun 28, 2018 •

edited

Loading

thamht4190 Jun 29, 2018 •

edited

Loading

wesm Aug 1, 2018 •

edited

Loading

majetideepak Aug 1, 2018 •

edited

Loading