Add decryption functionality to presto by shangxinli · Pull Request #17728 · prestodb/presto

shangxinli · 2022-05-04T05:27:10Z

Co-authored-by: ggershinsky ggershinsky@users.noreply.github.com

Summary: This is to port parquet-mr decryption funtionality. The main commits in parquet-mr for encryption/decryption is apache/parquet-java@65b95fb and several other fixes. This change only port the decryption only.

Test plan - (Please fill in how you tested your changes)

Please make sure your submission complies with our Development, Formatting, and Commit Message guidelines. Don't forget to follow our attribution guidelines for any code copied from other projects.

Fill in the release notes towards the bottom of the PR description.
See Release Notes Guidelines for details.

== RELEASE NOTES ==

General Changes
* ...
* ...

Hive Changes
* ...
* ...

If release note is NOT required, use:

== NO RELEASE NOTE ==

shangxinli · 2022-05-05T15:27:13Z

@beinan @zhenxiao I just opened this new PR because of the conflict. I close the old PR.

BTW, the CI build sometime has the error 'No configuration was found in your project. Please refer to https://circleci.com/docs/2.0/ to get started with your configuration.', which seems unrelated to my change. Please let me know if this is not the case.

zhenxiao

nice work, @shangxinli
mostly looks good. A few minor things

zhenxiao · 2022-05-09T11:56:26Z

is it possible we import MAGIC and EMAGIC from Parquet code?

I think we can do.

zhenxiao · 2022-05-09T12:04:25Z

how about we add comment for aad:
additional authenticated data for AES cipher

Good idea! Just added for this one and PageReader.java

zhenxiao · 2022-05-09T12:08:37Z

the comment is misleading, how about:
detect files not encrypted but with fileDecryptor set

Changed it to 'Detect that the file is not encrypted by mistake'.

zhenxiao · 2022-05-09T12:10:21Z

how about adding comment to this case:
plaintext footer, with file is crypt

Added the comments "Reader attached fileDecryptor. The file could be encrypted with plaintext footer or the whole file is plaintext".

zhenxiao · 2022-05-09T12:11:30Z

keep this line before encryptedFooter and fileDecryptor check

Good catch!

zhenxiao · 2022-05-09T12:15:52Z

...improve it later

Good catch!

zhenxiao · 2022-05-09T12:17:14Z

shall we add a log in case of KeyAccessDeniedException?

Yeah, Let me figure out how to add it. Not see log4j being used in Presto-Parquet project.

It seems we need to add the following dependency to the subproject if we want to log in presto-parquet. I am afraid of doing that because just for adding this line of log, we need to add a new dependency. Please let me know you. thoughts
com.facebook.airlift log

zhenxiao · 2022-05-09T12:18:14Z

static import AesCipher.NONCE_LENGTH and AesCipher.GCM_TAG_LENGTH?

Make sense!

zhenxiao · 2022-05-09T12:19:37Z

put final variables together

zhenxiao · 2022-05-09T12:22:13Z

s/colDecSetup/columnDecryption/g

make sense!

beinan

lgtm except a couple of very minor issues

beinan · 2022-05-10T06:15:32Z

unnecessary this

Suggested change

return this.fileName;

return fileName;

beinan · 2022-05-10T06:15:40Z

unnecessary this

beinan · 2022-05-10T06:16:23Z

inline fileSize?

Suggested change

long fileSize = path.getFileSystem(new Configuration()).getFileStatus(path).getLen();

return fileSize;

return path.getFileSystem(new Configuration()).getFileStatus(path).getLen();

beinan · 2022-05-10T06:18:21Z

unnecessary this

Suggested change

throw new HiddenColumnException(this.path.toArray(), this.filePath);

throw new HiddenColumnException(path.toArray(), filePath);

beinan · 2022-05-10T06:18:46Z

unnecessary this

beinan · 2022-05-10T06:18:55Z

unnecessary this

beinan · 2022-05-10T06:21:55Z

Suggested change

if (null != headerBlockDecryptor) {

if (headerBlockDecryptor != null) {

shangxinli · 2022-05-11T18:08:08Z

@zhenxiao @beinan Any idea why "ci/circleci: Build Error". The error says 'No configuration was found in your project', but my change doesn't change anything in terms of the project's configuration. I only changed java code. Any idea on how to fix that?

shangxinli · 2022-05-16T05:07:28Z

I just created a very simple PR and then revert it. So everything should pass, but I still get the same error "ci/circleci: Build Error". I assume this error is not related to my change. Given all other tests and checks passed, I assume my change should be clean now. Let me know if this is not the case.

zhenxiao · 2022-05-17T05:23:58Z

hi @shangxinli I think your code should be good. While, we do require all tests passed before any code could merge. Could you please squash all commits into one, and submit under this PR again?

Co-authored-by: ggershinsky <ggershinsky@users.noreply.github.com> Summary: This is to port parquet-mr decryption funtionality. The main commits in parquet-mr for encryption/decryption are apache/parquet-java@65b95fb and several other fixes. This change only port the decryption only.

shangxinli · 2022-05-18T00:03:13Z

Just did it, thanks @zhenxiao @beinan for review.

beinan · 2022-05-18T06:02:00Z

the ci/circleci is still failing, hmmm I cannot rerun this one. But the code looks good to me. @zhenxiao are you able to rerun the ci/circleci?

zhenxiao · 2022-05-18T06:27:34Z

the same to me. Could not rerun it. maybe we submit the code as a new PR, and see how it goes? @shangxinli @beinan

shangxinli · 2022-05-18T14:20:44Z

@zhenxiao I have created another PR with no change. The ci/circleci still failed.

@aweisberg Can you help with the ci/circleci issue?

@mshang816 Do you think it could be related to 59f86ca?

shangxinli · 2022-05-20T23:20:24Z

Re-created a new PR #17791

shangxinli requested a review from a team as a code owner May 4, 2022 05:27

shangxinli requested a review from presto-oss May 4, 2022 05:27

shangxinli force-pushed the column_indexes_dev_new_4 branch 5 times, most recently from 18e2f98 to 3535aa3 Compare May 5, 2022 15:22

shangxinli force-pushed the column_indexes_dev_new_4 branch 2 times, most recently from 4932cb3 to 3283ad8 Compare May 6, 2022 14:40

highker requested a review from kewang1024 May 8, 2022 08:05

zhenxiao requested changes May 9, 2022

View reviewed changes

beinan reviewed May 10, 2022

View reviewed changes

shangxinli force-pushed the column_indexes_dev_new_4 branch from 56031ca to 475c896 Compare May 17, 2022 18:59

shangxinli mentioned this pull request May 20, 2022

Add decryption functionality to presto. #17791

Closed

shangxinli mentioned this pull request Jun 14, 2022

Add parquet ecryption functionality into presto #17881

Merged

shangxinli closed this Jul 6, 2022

	long fileSize = path.getFileSystem(new Configuration()).getFileStatus(path).getLen();
	return fileSize;
	return path.getFileSystem(new Configuration()).getFileStatus(path).getLen();

	throw new HiddenColumnException(this.path.toArray(), this.filePath);
	throw new HiddenColumnException(path.toArray(), filePath);

	if (null != headerBlockDecryptor) {
	if (headerBlockDecryptor != null) {

Conversation

shangxinli commented May 4, 2022

Uh oh!

shangxinli commented May 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zhenxiao 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

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!

shangxinli May 9, 2022 • 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!

beinan 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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shangxinli commented May 5, 2022 •

edited

Loading

shangxinli May 9, 2022 •

edited

Loading

shangxinli commented May 16, 2022 •

edited

Loading