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

thamht4190 · 2019-07-09T02:41:11Z

I open a fresh pull request from the pull #2555 since pull #2555 is too big now. Please let your comments here. Thanks!

majetideepak · 2019-07-10T13:28:14Z

cpp/src/parquet/file_reader.cc

    std::shared_ptr<ArrowInputStream> stream =
        properties_.GetStream(source_, col_start, col_length);
+
+#ifdef PARQUET_ENCRYPTION


Can we remove these ifdef and use conditionals based on the presence of crypto variables? In this case we can check based off col->crypto_metadata()?

Yep, looks like col->crypto_metadata() check does the job here. We'll also handle all other similar ifdefs.

I have removed some but not completely. I'm not sure if it matches your expectation. Can you please take a look again?

missed this question, please see the comment below.

codecov-io · 2019-07-10T13:58:14Z

Codecov Report

Merging #4826 into master will increase coverage by 0.04%.
The diff coverage is 92.74%.

@@            Coverage Diff             @@
##           master    #4826      +/-   ##
==========================================
+ Coverage   88.93%   88.98%   +0.04%     
==========================================
  Files         988     1006      +18     
  Lines      134262   137127    +2865     
  Branches     1501     1501              
==========================================
+ Hits       119412   122025    +2613     
- Misses      14485    14737     +252     
  Partials      365      365

Impacted Files	Coverage Δ
cpp/src/parquet/column_writer.h	`95.65% <ø> (ø)`	⬆️
cpp/src/parquet/metadata.h	`100% <ø> (ø)`	⬆️
cpp/src/parquet/file_writer.h	`81.81% <ø> (ø)`	⬆️
cpp/src/parquet/column_reader.cc	`89.4% <100%> (+0.52%)`	⬆️
cpp/src/parquet/internal_file_encryptor.h	`100% <100%> (ø)`
cpp/src/parquet/types.h	`100% <100%> (ø)`	⬆️
cpp/src/parquet/column_reader.h	`98% <100%> (+0.22%)`	⬆️
cpp/src/parquet/encryption_properties_test.cc	`100% <100%> (ø)`
cpp/src/parquet/properties.h	`97.47% <100%> (+0.2%)`	⬆️
...rc/parquet/encryption_write_configurations_test.cc	`100% <100%> (ø)`
... and 146 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2e53c00...4e9b9af. Read the comment docs.

revit1976 · 2019-07-10T15:22:39Z

@wesm , @majetideepak fyi, all CI issues are now resolved.

wesm · 2019-07-11T15:56:51Z

I won't be able to do a thorough review but would like to get this merged soon. Does the PR into parquet-testing need to be merged before this is?

ggershinsky · 2019-07-11T16:26:46Z

Does the PR into parquet-testing need to be merged before this is?

Not strictly necessary, but allows for better testing. We've expanded the decryption tests to read two sets of files - one produced in real time by the encryption tests; another stored in the parquet-testing repo. The former can miss problems caused by matching bugs in writers and readers.

ggershinsky · 2019-07-11T16:41:52Z

cpp/src/parquet/column_reader.cc

      meta_decryptor_ = ctx->meta_decryptor;
      data_decryptor_ = ctx->data_decryptor;
      if (data_decryptor_ != NULLPTR || meta_decryptor_ != NULLPTR) {
+#ifdef PARQUET_ENCRYPTION


Hi Tham, I believe we need to try to get rid of all #ifdef PARQUET_ENCRYPTION statements (if possible). Lets go over the code early next week (our weekend starts today..).

wesm · 2019-07-11T19:41:17Z

OK. I'll wait for you to address the ifdefs

thamht4190 · 2019-07-16T02:59:55Z

@wesm @majetideepak we've figured out a way to remove all #ifdef PARQUET_ENCRYPTION. To be able to compile without openssl, we create a encryption_internal-nossl.cc instead of encryption_internal.cc with openssl. I'll update the fix, after internal team review, I will push to this pull.

wesm · 2019-07-16T04:06:55Z

Note that "internal review" is not really consistent with the Apache Way

thamht4190 · 2019-07-16T04:40:25Z

Ok @wesm. So I will push the change directly to this pull.

ggershinsky · 2019-07-18T16:47:09Z

@wesm @majetideepak We are done with all todo items on our current list. This PR should be able to pass the CI now (at least the encryption unitests), due to availability of the new parquet-testing files (thanks Wes for merging them). Is there a way to trigger the CI here directly? Or we should add a new commit / rebase the PR?

ggershinsky · 2019-07-18T16:59:55Z

In parallel, @thamht4190 and @revit13 , if you're online now, can you check if this PR can be rebased (a good chance the Arrow master has moved..)

thamht4190 · 2019-07-19T14:57:16Z

@majetideepak CI tests have passed except for R in AppVeyor that needs OpenSSL. It seems we should link openssl libs at line: https://ci.appveyor.com/project/ApacheSoftwareFoundation/arrow/builds/26093493/job/9yen567yr3pw6fh8#L3403. However, I'm not sure how to do it. Can you please take a look?

thamht4190 · 2019-07-19T15:07:25Z

We got this issue when testing our product on Windows with MacAfee (it may happen with MacOS as well):
"Failed to open local file: <path_to_folder>/<file_name>.parquet.encrypted , error: Permission denied"
Then when checking log of MacAfee, I see:
NT AUTHORITY\SYSTEM ran CORTEXSERVICE.EXE, which tried to access <PATH_TO_FOLDER>\<FILE_NAME>.PARQUET.ENCRYPTED, violating the rule "*.encrypted Blocker", and was blocked. For information about how to respond to this event, see KB85494.
It seems .encrypted extension is not so friendly with antivirus softwares.
As I know, parquet tools should not base on file extension, then it's quite safe to use another file extension (without .encrypted) in my application when writing parquet file, however, it seems tricky. Otherwise, it's inconvenient for our customers if keeping .encrypted extension. We are discussing to trade off. Any thoughts? @majetideepak

ggershinsky · 2019-07-19T15:38:27Z

adding @wesm to the loop on the last comment. My personal take on that - technically, you can use files without .encrypted extension in your settings; neither encryption spec nor the code depend on file names (and there are no plans today to introduce such dependency), so parquet tools should be able to read these files in the future.
However, I believe the right thing to do is to re-configure your anti-virus system (to enable say "*.parquet.encrypted" files). These files are encrypted after all.., so that should be handled at a policy level. But, eventually, this is up to the organizational security admin decision; naming these files without ".encrypted" extension can be also an option.

ggershinsky · 2019-07-23T12:41:41Z

@wesm @majetideepak We are done, let us know what's next. All comments have been addressed; the CI tests pass (except for one, R/libarrow, that lacks OpenSSL - Deepak, your help with that will be appreciated).

majetideepak · 2019-07-23T13:09:23Z

@ggershinsky I will make a final pass today and tomorrow. I will also look into the R issue.

majetideepak · 2019-07-25T01:30:07Z

To fix the R failure, we have to link the OpenSSL library here https://github.com/apache/arrow/blob/48ee38f833a51bb46a404da10d89c4b687e952c9/r/src/Makevars.win
or disable Parquet Encryption in this test.
An issue in the current form is that there is no way to disable Parquet Encryption if the system has OpenSSL. I think we have to switch the default PARQUET_REQUIRE_ENCRYPTION to ON and support OFF. I will open a separate JIRA and make that change.

majetideepak · 2019-07-25T14:30:07Z

I decided to leave the current behavior of building encryption support if the system has OpenSSL libraries for now. This was discussed in #4494
We can always change this behavior in the future.

majetideepak · 2019-07-26T21:06:31Z

@thamht4190 can you rebase with master again. The failing rust tests have been fixed on master.

majetideepak · 2019-07-26T21:36:59Z

ci/windows-pkg-arrow-for-r.sh

CC: @nealrichardson

Thanks, that seems ok (though you need to remove the extra : in the middle of the URLs).

Since you had to make this change, I suspect that this means we'll need to update the Homebrew formula too, but unfortunately we don't have that under CI.

Fixed the URLs. What are your thoughts on updating the Homebrew formula?

IMO we should put the C++ homebrew formula in our ci/ directory and have a Travis job, perhaps only run nightly/on-demand if we're concerned about build time, that builds from it and then runs some test suite that requires it (e.g. R, Ruby). https://github.com/Homebrew/homebrew-core/blob/master/CONTRIBUTING.md suggests we can brew install --build-from-source so IIUC we'd use something like that.

Related, I previously tried to add brew install --HEAD to CI but that didn't go anywhere: #4576

I don't think this needs to block this patch, but it would be great to do something with this sooner rather than later. Otherwise we'll keep making releases unnecessarily expensive and that will keep us from doing them as frequently as we should.

thamht4190 · 2019-07-31T07:52:23Z

The CI is green now. Thanks @majetideepak for handling the OpenSSL.
@majetideepak @wesm The pull request is complete and ready for merge, can you please have a look?

majetideepak · 2019-10-07T16:18:41Z

@wesm I am done with my refactoring for now. We can merge this PR if there are no other comments.

majetideepak · 2019-10-08T16:07:17Z

All tests are passing. @thamht4190, @revit1976 @ggershinsky do you have any comments on this PR? Is this good to be merged?

ggershinsky · 2019-10-09T15:52:11Z

no additional comments, looks good to me.

thamht4190 · 2019-10-10T03:21:26Z

LGTM. Thanks Deepak for your change.

revit1976 · 2019-10-10T03:34:34Z

LGTM also. Thanks.

majetideepak · 2019-10-15T15:11:31Z

@wesm Do you plan to make a pass before we merge this?

wesm · 2019-10-17T20:20:13Z

@majetideepak sorry for the delay. I am going to take a brief look through this to check for anything that seems obviously off

wesm · 2019-10-30T01:34:29Z

I apologize, I've been out of pocket the last couple weeks. Taking a look through and if I don't any anything troubling I'll merge

wesm

+1. I'm going to push some stylistic fixes. It's a bummer that this branch is so messed up. I'm going to create a squashed version to merge to master that doesn't have all the unrelated commits in it. I'll have to manually list out the co-authors...

wesm · 2019-10-30T01:51:44Z

cpp/src/parquet/column_reader.cc

-        total_num_rows_(total_num_rows) {
+        total_num_rows_(total_num_rows),
+        decryption_buffer_(AllocateBuffer(pool, 0)) {
+    if (crypto_ctx != NULLPTR) {


Note: it's not necessary to use NULLPTR in .cc files, only in headers.

wesm · 2019-10-30T01:52:46Z

cpp/src/parquet/column_reader.cc


 private:
+  void UpdateDecryption(const std::shared_ptr<Decryptor>& decryptor, int8_t module_type,
+                        const std::string& pageAAD);


Nit: page_aad would probably be a better variable name

wesm · 2019-10-30T01:53:12Z

cpp/src/parquet/column_reader.cc


+void SerializedPageReader::InitDecryption() {
+  // Prepare the AAD for quick update later.
+  if (crypto_ctx_.data_decryptor != NULLPTR) {


wesm · 2019-10-30T01:53:37Z

cpp/src/parquet/column_reader.cc

+    data_pageAAD_ =
+        encryption::CreateModuleAad(crypto_ctx_.data_decryptor->file_aad(),
+                                    encryption::kDataPage, crypto_ctx_.row_group_ordinal,
+                                    crypto_ctx_.column_ordinal, static_cast<int16_t>(-1));


Is this static_cast needed?

wesm · 2019-10-30T01:53:50Z

cpp/src/parquet/column_reader.cc

+void SerializedPageReader::UpdateDecryption(const std::shared_ptr<Decryptor>& decryptor,
+                                            int8_t module_type,
+                                            const std::string& pageAAD) {
+  DCHECK(decryptor != NULLPTR);


wesm · 2019-10-30T01:58:59Z

cpp/src/parquet/column_writer.cc

+                       int16_t column_chunk_ordinal,
+                       MemoryPool* pool = arrow::default_memory_pool(),
+                       std::shared_ptr<Encryptor> meta_encryptor = NULLPTR,
+                       std::shared_ptr<Encryptor> data_encryptor = NULLPTR)


NULLPTR -> nullptr

wesm · 2019-10-30T01:59:35Z

cpp/src/parquet/column_writer.cc

+    const uint8_t* output_data_buffer = compressed_data->data();
+    int32_t output_data_len = static_cast<int32_t>(compressed_data->size());
+
+    std::shared_ptr<Buffer> encrypted_data_buffer = nullptr;


Setting = nullptr is redundant

wesm · 2019-10-30T02:22:16Z

cpp/src/parquet/encryption_read_configurations_test.cc

+}
+
+INSTANTIATE_TEST_CASE_P(
+    decryptionTests, TestDecryptionConfiguration,


A couple non-conforming names in this file

wesm · 2019-10-30T02:26:07Z

cpp/src/parquet/file_reader.cc

+      }
+    }
+  }
+}


I suspect that some code in these methods could be simplified or reused

wesm · 2019-10-30T02:39:36Z

dev/release/verify-release-candidate.sh

 -DARROW_PYTHON=ON
 -DARROW_GANDIVA=ON
 -DARROW_PARQUET=ON
+-DPARQUET_REQUIRE_ENCRYPTION=ON


This may make it harder for people to verify releases. We can see for the next major release

wesm · 2019-10-30T02:53:55Z

Here's the merge-ready commit, I'm going to wait a bit for CI to make progress

wesm@69db453

Do NOT merge this PR as is, please

wesm · 2019-10-30T14:01:08Z

Merging this. Thanks all

ggershinsky · 2020-03-05T11:34:57Z

@majetideepak @wesm While Parquet encryption feature is a part of the Arrow 0.16.0 release, it doesn't appear in https://github.com/apache/arrow/blob/master/CHANGELOG.md. Also, the https://issues.apache.org/jira/browse/PARQUET-1300 is still open. Should it be closed, with setting the "Affects Version/s:" field? Can the 0.16.0 changelog be updated?

majetideepak · 2020-03-05T18:20:32Z

@ggershinsky I think if needed we could do a second minor release ARROW 0.16.1 if there are no objections. Do you want to e-mail the Arrow/Parquet mailing lists to discuss this?

ggershinsky · 2020-03-06T06:44:01Z

@majetideepak We've submitted a paper where we've referenced Arrow 0.16.0 for parquet encryption. So having it in 0.16.1 doesn't solve our immediate (though very minor) problem. I wouldn't suggest running a new release just for adding this to the log. I believe we can think of 0.16.0 as a "silent release" of this feature. I've created an Arrow jira (8018) for adding this feature explicitly in 1.0.0, and closed the PARQUET-1300 as fixed - but if I've done something wrong there, please change as needed.

ggershinsky · 2020-03-09T12:17:24Z

@wesm what should be done for packaging issues for encryption? I would handle this, but not 100% sure about the details.

wesm · 2020-03-09T16:53:32Z

I just opened ARROW-8038 and ARROW-8040 to cover some of the basic packaging stuff. Let's move the discussion there (and to child issues)

revit1976 force-pushed the master branch from ed4e7d6 to 9011411 Compare July 10, 2019 07:49

majetideepak reviewed Jul 10, 2019

View reviewed changes

ggershinsky reviewed Jul 11, 2019

View reviewed changes

thamht4190 force-pushed the master branch from f6700d3 to 6901373 Compare July 17, 2019 09:20

thamht4190 force-pushed the master branch from 9a47424 to e6b3064 Compare July 19, 2019 03:49

kszucs force-pushed the master branch 2 times, most recently from ed180da to 85fe336 Compare July 22, 2019 19:29

thamht4190 force-pushed the master branch from 7996ff5 to 16158c3 Compare July 23, 2019 10:14

majetideepak reviewed Jul 26, 2019

View reviewed changes

revit1976 force-pushed the master branch from 5476980 to d2cbe8c Compare July 28, 2019 07:27

thamht4190 force-pushed the master branch from e27bcc7 to 88d3e82 Compare July 31, 2019 03:26

make format

c0e0d8a

kszucs force-pushed the master branch from fc93312 to af097e6 Compare October 5, 2019 09:47

Deepak Majeti added 7 commits October 5, 2019 18:14

fix tests

3275cbe

merge master

ed693e8

Fix file_reader

af1bd12

refactor metadata

6bc493b

make lint

fa9683d

cpp cli lint

d26a119

fix multi-rowgroup aad

99b9713

Merge branch 'master' into master

c59daf7

wesm approved these changes Oct 30, 2019

View reviewed changes

Stylistic fixes, remove cruft

4e9b9af

wesm closed this in 41753ac Oct 30, 2019

This was referenced Jun 23, 2024

[C++][Parquet] Parquet modular encryption #42820

Closed

[C++][Parquet] LONE - [C++] Parquet modular encryption #42965

Closed

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

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

Uh oh!

Conversation

thamht4190 commented Jul 9, 2019

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!

codecov-io commented Jul 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

revit1976 commented Jul 10, 2019

Uh oh!

wesm commented Jul 11, 2019

Uh oh!

ggershinsky commented Jul 11, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wesm commented Jul 11, 2019

Uh oh!

thamht4190 commented Jul 16, 2019

Uh oh!

wesm commented Jul 16, 2019

Uh oh!

thamht4190 commented Jul 16, 2019

Uh oh!

ggershinsky commented Jul 18, 2019

Uh oh!

ggershinsky commented Jul 18, 2019

Uh oh!

thamht4190 commented Jul 19, 2019

Uh oh!

thamht4190 commented Jul 19, 2019

Uh oh!

ggershinsky commented Jul 19, 2019

Uh oh!

ggershinsky commented Jul 23, 2019

Uh oh!

majetideepak commented Jul 23, 2019

Uh oh!

majetideepak commented Jul 25, 2019

Uh oh!

majetideepak commented Jul 25, 2019

Uh oh!

majetideepak commented Jul 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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 commented Jul 31, 2019

Uh oh!

majetideepak commented Oct 7, 2019

Uh oh!

majetideepak commented Oct 8, 2019

Uh oh!

ggershinsky commented Oct 9, 2019

Uh oh!

thamht4190 commented Oct 10, 2019

Uh oh!

revit1976 commented Oct 10, 2019

Uh oh!

majetideepak commented Oct 15, 2019

Uh oh!

wesm commented Oct 17, 2019

Uh oh!

wesm commented Oct 30, 2019

codecov-io commented Jul 10, 2019 •

edited

Loading

majetideepak commented Jul 26, 2019 •

edited

Loading

wesm left a comment •

edited

Loading