ARROW-9318: [C++] Parquet encryption key management by thamht4190 · Pull Request #8023 · apache/arrow

thamht4190 · 2020-08-22T07:43:33Z

This PR is C++ implementation for parquet key tool, based on the Java implementation and the design doc.

The major parts of this PR are:

higher level of encryption/decryption configuration, including key management configuration.
KMS connection configuration
Abstract class KmsClient, KmsClientFactory
PropertiesDrivenCryptoFactory class to convert these above configurations to the lower level FileEncryptionProperties, FileDecryptionProperties
unit test using InMemoryKms (an sample of KmsClient).

Comparing to Java version, this C++ pull doesn't contain externally storing key material using hadoop file system (only storing key material internally in parquet file is supported for now). The reason is lack of understanding about Hadoop file system, can be implemented it later in another pull.

Thanks!

github-actions · 2020-08-22T07:54:08Z

Thanks for opening a pull request!

Could you open an issue for this pull request on JIRA?
https://issues.apache.org/jira/browse/ARROW

Then could you also rename pull request title in the following format?

ARROW-${JIRA_ID}: [${COMPONENT}] ${SUMMARY}

See also:

github-actions · 2020-08-22T08:05:28Z

https://issues.apache.org/jira/browse/ARROW-9318

emkornfield

I made some style comments but didn't flag every place that they were an issue. Please self review to fix (it also looks like you might need to run clang-format as the lint is failing).

I would expect additional tests around individual files and not just end-to-end tests (any place with non trivial implementations should probably have at least one or two tests. excercising of error conditions on json parsing I think would also be useful.

Another issue is thread-safey. It would be good to clarify which classes are expected/not-expected to be thread safe. For instance, I would at lease expect client caches to be thread safe but it doesn't look like they were implemented to be.

Also this PR is quite large is there any way that it could possibly be broken up into smaller units?

pitrou · 2020-09-03T16:13:19Z

Just a drive-by comment for now, but given the number of files added I think we should create a src/parquet/encryption subdirectory.

thamht4190 · 2021-02-28T13:50:30Z

I fixed all the comments in this pull, can you please take a look? @emkornfield @pitrou @bkietz cc @ggershinsky

pitrou · 2021-03-10T17:57:07Z

Ok, I've pushed some updates to try and conform with coding conventions a bit more.

Some high-level comments:

In general, I'm not particularly happy with the API (for example, why is the configuration mutated while in use?)
There are some roundtrip tests but no tests with reference data; so there's no guarantee that this actually implements the spec faithfully
I'm concerned that the crypto code isn't very mindful of safety (see https://issues.apache.org/jira/browse/PARQUET-1997); this PR builds on that and also seems to keep sensitive data around (decryption keys) in various places

I also don't have a personal interest in this currently and I recognize that the PR implements a useful functionality.
@ggershinsky @thamht4190 Do you plan to address some of these concerns in this PR? Otherwise, I may merge this and users of this code will have to be mindful of potential issues.

ggershinsky · 2021-03-11T14:12:21Z

@pitrou My personal preference is the latter - merging this pr and letting the users of this code to be mindful of potential issues. This is safer than proceeding with the current low-level interface work, which has many more security and other issues. When this pr is merged, it will unblock the users who'd create the high-level interface in Python and address the API point (I'll add a link to this thread in the Python design doc; the Python API is to be reviewed; the C++ API could be synced accordingly).

Regarding the second point - we'll be testing the interop with the Java counterpart, to make sure the spec is implemented properly. Regarding the third point, I think the code is safe, as per our discussion in that jira.

pitrou · 2021-03-11T14:19:57Z

As for testing, I'd still welcome unit tests with actual pieces of JSON as per the spec, but I won't insist on it here :-)

Last question: do we want to mark those APIs experimental so that we feel free to change them in the future?

ggershinsky · 2021-03-11T14:44:01Z

Thanks! Yep, I think it's a good idea to mark them as experimental, there is a chance they'll be updated in the course of the Python API work.

emkornfield · 2021-03-11T16:25:00Z

I also don't have a personal interest in this currently and I recognize that the PR implements a useful functionality.
@ggershinsky @thamht4190 Do you plan to address some of these concerns in this PR? Otherwise, I may merge this and users of this code will have to be mindful of potential issues.

I'll try to look tonight but this makes it sound like there are general concerns around quality here, and that doesn't seem like something we want to merge (especially if it is security related). If I get distracted and don't give feedback by monday, it is OK not to block on me.

ggershinsky · 2021-03-11T16:35:15Z

Thanks. To clarify - the security concerns I've mentioned above, relate to low level encryption, not to this pull request.

pitrou · 2021-03-15T15:49:59Z

@emkornfield Feel free to take a last look.

emkornfield · 2021-03-16T04:25:38Z

It looks like there are JSON changes in here? does this need a rebase?

pitrou · 2021-03-16T12:51:06Z

@emkornfield The JSON changes are expected, the encryption layer needs to parse and generate some JSON.

emkornfield · 2021-03-16T16:14:35Z

@emkornfield The JSON changes are expected, the encryption layer needs to parse and generate some JSON.

Ah right, I forgot, will start looking again.

emkornfield · 2021-03-17T07:31:40Z

Sorry didn't get a chance to look. @pitrou if you are comfortable with changes please go ahead and merge

pitrou · 2021-03-17T14:46:36Z

I'll merge then. Thank you @thamht4190 and @ggershinsky for contributing this!

…files. Exposes in PyArrow the high-level C++ API for Parquet encryption that was added in #8023. Design document: https://docs.google.com/document/d/1i1M5f5azLEmASj9XQZ_aQLl5Fr5F0CvnyPPVu1xaD9U A test is added for writing and reading encrypted parquet files using a simple in-memory KMS for testing, that is not to be used as an example of a KMS client. In addition, there is an example KMS client using Vault KMS. This PR handles only the file-level encryption and decryption. Dataset is handled in separate PRs. The investigation of the multithreading model of PME is separated into a separate issue, independent of this one. Closes #10450 from andersonm-ibm/encryption Lead-authored-by: Maya Anderson <[email protected]> Co-authored-by: andersonm-ibm <[email protected]> Co-authored-by: roee88 <[email protected]> Co-authored-by: Antoine Pitrou <[email protected]> Signed-off-by: Antoine Pitrou <[email protected]>

thamht4190 changed the title ~~Arrow 9318 encryption key management~~ ARROW-9318: [C++] Parquet encryption key management Aug 22, 2020

emkornfield reviewed Aug 24, 2020

View reviewed changes

emkornfield requested changes Aug 24, 2020

View reviewed changes

Comment thread cpp/src/parquet/test_encryption_util.h Resolved

Comment thread cpp/src/parquet/CMakeLists.txt Resolved

Comment thread cpp/src/parquet/encryption.h Resolved

Comment thread cpp/src/parquet/encryption_internal.h Resolved

thamht4190 force-pushed the arrow-9318-encryption-key-management branch from a96c7e4 to 7206be0 Compare September 3, 2020 14:54

ggershinsky reviewed Sep 4, 2020

View reviewed changes

Comment thread cpp/src/parquet/file_key_wrapper.h Resolved

Comment thread cpp/src/parquet/file_key_wrapper.h Resolved

Comment thread cpp/src/parquet/file_key_wrapper.h Resolved

Comment thread cpp/src/parquet/key_toolkit.h Resolved

ggershinsky reviewed Sep 9, 2020

View reviewed changes

Comment thread cpp/src/parquet/properties_driven_crypto_factory.h Resolved

pitrou reviewed Sep 9, 2020

View reviewed changes

Comment thread cpp/src/parquet/key_toolkit.h Resolved

pitrou reviewed Sep 9, 2020

View reviewed changes

Comment thread cpp/src/parquet/key_toolkit.h Resolved

pitrou reviewed Sep 9, 2020

View reviewed changes

Comment thread cpp/src/parquet/key_toolkit.h Resolved

ggershinsky reviewed Sep 9, 2020

View reviewed changes

Comment thread cpp/src/parquet/properties_driven_crypto_factory.cc Resolved

thamht4190 added 17 commits September 11, 2020 09:05

RemoteKmsClient class

d060f21

fix build issue

93fb87a

TwoLevelCacheWithExpiration class

4c22208

KeyEncryptionKey class

a35c8bb

KmsClientCache in KeyToolkit

cb80ada

HadoopFSKeyMaterialStore class

518b0d0

some compile fix

c3ae5a0

KeyMetadata & KeyMaterial

9d6cac2

FileKeyWrapper

ec0faf1

some minor fix

329f55e

properties driven (encryption)

bf562f7

remove HadoopFSKeyMaterialStore

95d12ed

use KeyAccessToken class instead of a string

df41a60

remove hadoop config

bad8a20

InMemoryKms

9c6ec65

fix issue

6ecf307

FileKeyUpwrapper

8de430f

fix warnings treated as errors

c8c5aa2

thamht4190 force-pushed the arrow-9318-encryption-key-management branch from 794ece0 to c8c5aa2 Compare February 27, 2021 15:39

thamht4190 requested review from bkietz, emkornfield and pitrou February 28, 2021 13:45

Some updates

b06ab26

Fix cast

4c108f0

pitrou added 2 commits March 11, 2021 15:33

Merge branch 'master' into arrow-9318-encryption-key-management

c828b65

Hopefully fix Windows linking error

48456d7

pitrou closed this in 5b14d53 Mar 17, 2021

This was referenced Apr 8, 2021

ARROW-11644: [Python][Parquet] Low-level Parquet decryption in Python #9631

Closed

ARROW-12104: [Go][Parquet] Second chunk of Ported Go Parquet code #9817

Closed

andersonm-ibm mentioned this pull request Jun 4, 2021

ARROW-9947: [Python] High-level Python API for Parquet encryption of files. #10450

Closed

asfimport mentioned this pull request Jul 27, 2021

[C++][Parquet] Encryption key management tools #25405

Closed

Conversation

thamht4190 commented Aug 22, 2020

Uh oh!

github-actions Bot commented Aug 22, 2020

Uh oh!

github-actions Bot commented Aug 22, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

emkornfield left a comment • 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pitrou commented Sep 3, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

pitrou commented Mar 10, 2021

Uh oh!

ggershinsky commented Mar 11, 2021

Uh oh!

pitrou commented Mar 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ggershinsky commented Mar 11, 2021

Uh oh!

emkornfield commented Mar 11, 2021

Uh oh!

ggershinsky commented Mar 11, 2021

Uh oh!

pitrou commented Mar 15, 2021

Uh oh!

emkornfield commented Mar 16, 2021

Uh oh!

pitrou commented Mar 16, 2021

Uh oh!

emkornfield commented Mar 16, 2021

Uh oh!

emkornfield commented Mar 17, 2021

Uh oh!

pitrou commented Mar 17, 2021

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

emkornfield left a comment •

edited

Loading

thamht4190 commented Feb 28, 2021 •

edited

Loading

pitrou commented Mar 11, 2021 •

edited

Loading