PARQUET-1228: Format Structures encryption by ggershinsky · Pull Request #613 · apache/parquet-java

ggershinsky · 2019-02-11T13:03:11Z

No description provided.

gszadovszky · 2019-03-18T15:23:52Z

To have a passing travis on your feature branch you may use the dev script feature introduced in PARQUET-1490. See https://github.com/apache/parquet-mr/blob/bloom-filter/dev/travis-before_install-bloom-filter.sh as an example.

ggershinsky · 2019-03-19T11:26:33Z

@gszadovszky Thanks, but this wont help, because the format pull request is not merged yet in that feature branch,
apache/parquet-format#124
Would appreciate if you could check if this can be accelerated..

Fokko

LGTM in general, can you do an autoformat? A lot of weird indenting and trailing whitespace.

Fokko · 2019-05-22T14:27:46Z

parquet-format-structures/src/main/java/org/apache/parquet/format/Util.java

+      }
+    }
+    // Serialize and encrypt the structure
+    TMemoryBuffer thriftMemoryBuffer = new TMemoryBuffer(100);


This one implements Closeable, can we wrap a try-with-resources around this? And why 100 buffer size? Maybe move this to a constant, instead of having a magic variable.

try-with-resources sounds good.
100 is the initial alloc. Usually enough for page headers etc, but will auto-grow for larger ones.
Will make it a constant.

Fokko · 2019-05-22T14:28:31Z

parquet-format-structures/src/main/java/org/apache/parquet/format/Util.java


-  private static <T extends TBase<?,?>> T read(InputStream from, T tbase) throws IOException {
+  private static <T extends TBase<?,?>> T read(InputStream input, T tbase, BlockCipher.Decryptor decryptor, byte[] AAD) throws IOException {
+    InputStream from;


I like to make variables like these final

Fokko · 2019-05-22T14:28:38Z

parquet-format-structures/src/main/java/org/apache/parquet/format/Util.java

        })));
      }
+
+      InputStream from;


I like to make variables like these final

Fokko · 2019-05-22T14:29:03Z

parquet-format-structures/src/main/java/org/apache/parquet/format/Util.java

+  }
+
  public static void readFileMetaData(InputStream from, final FileMetaDataConsumer consumer, boolean skipRowGroups) throws IOException {
+    readFileMetaData(from, consumer, skipRowGroups, (BlockCipher.Decryptor) null, (byte[]) null);


I would omit the casting since it doesn't add any value.

The value is in compiler detection of buggy calls to wrong methods (wrong signature), causes by typeless nulls.

Could you please paste what kind of compiling error you met? Maybe we can set an option in the compiler?

I removed the extra casts, and didn't get any compile error. I'm also curious what was the error message you got.

Yep, it compiles, but the reason is to prevent compiler to cast this to an overloaded method with a different object type in place of the decryptor etc. A sort of precaution.

I can't imagine any scenario where this could happen. I think if there's an overloaded method with different parameters, and the compile can't figure out which method to call, then it would be a compilation error. In that case casting is the way to give hint to the compiler, that you'd like to call the method with BlockCipher.Decryptor and byte[] parameters, but in this case the method invocation is clear without casting too.

I see. Ok, looks like we have two options: 1) leave the code as is - the cast helps code readers to understand what is nulled (in this case, that we call methods without encryption); also, it's harmless 2) remove the casts in this and other pull requests.
Please let me know your preference, I'm fine with either option.

I personally prefer more compact code, and looks like @Fokko thinks the same, so if you don't mind, then let's remove those casts, that are not necessary and don't add any value!

Ok, will do that.

Fokko · 2019-05-22T14:29:08Z

parquet-format-structures/src/main/java/org/apache/parquet/format/Util.java


  public static void readFileMetaData(InputStream from, FileMetaDataConsumer consumer) throws IOException {
-    readFileMetaData(from, consumer, false);
+    readFileMetaData(from, consumer, (BlockCipher.Decryptor) null, (byte[]) null);


I would omit the casting since it doesn't add any value.

Fokko · 2019-05-22T14:30:07Z

parquet-format-structures/src/main/java/org/apache/parquet/format/BlockCipher.java

+import java.io.IOException;
+import java.io.InputStream;
+
+public interface BlockCipher{


I would split the Encryptor and Decryptor in separate files and put them in the BlockCipher package. WDYT?

I'm reluctant to do that. Extra package, with two small files; extra file..

I think keeping these interfaces in the same file is acceptable, they're not too big.

ggershinsky · 2019-05-30T08:42:56Z

Hi Fokko, thanks for the review, much appreciated. Finally, I got some respite from the C++ PRs, now getting to the Java ones.
Regarding autoformat - can do, but please suggest a (Linux cli?) tool for that.

chenjunjiedada · 2019-07-22T14:09:14Z

parquet-format-structures/src/main/java/org/apache/parquet/format/Util.java

+      if (null == decryptor) {
+        from = input;
+      }
+      else {


nit: please move this line to the end of last line.

nit, but not yet addressed, @ggershinsky could you please apply Junjie's recommendation?

will do, sorry for missing this.

chenjunjiedada · 2019-07-22T14:11:56Z

parquet-format-structures/src/main/java/org/apache/parquet/format/Util.java

+  }
+
  public static void readFileMetaData(InputStream from, final FileMetaDataConsumer consumer, boolean skipRowGroups) throws IOException {
+    readFileMetaData(from, consumer, skipRowGroups, (BlockCipher.Decryptor) null, (byte[]) null);


Could you please paste what kind of compiling error you met? Maybe we can set an option in the compiler?

nandorKollar · 2019-08-08T11:48:24Z

parquet-format-structures/src/main/java/org/apache/parquet/format/BlockCipher.java

+     * Encrypts the plaintext.
+     * 
+     * @param plaintext The plaintext starts at offset 0 of the input value, and fills up the entire byte array.
+     * @param AAD Encryption AAD (ignored in case of CTR cipher)


Would be nice to not use abbreviation in the parameter description, but resolve it.
Something like: @param AAD Additional Authenticated Data for the encryption algorithm (ignored in case of CTR cipher)

sounds good

nandorKollar · 2019-08-08T11:49:10Z

parquet-format-structures/src/main/java/org/apache/parquet/format/BlockCipher.java

+     * The ciphertext starts at offset 4  and fills up the rest of the returned byte array.
+     * The ciphertext includes the nonce and (in case of GCM cipher) the tag, as detailed in the 
+     * Parquet Modular Encryption specification.
+     * @throws IOException


Please document when does it throw IOException.

nandorKollar · 2019-08-08T11:50:00Z

parquet-format-structures/src/main/java/org/apache/parquet/format/BlockCipher.java

+     * The ciphertext starts at offset 4  and fills up the rest of the input byte array.
+     * The ciphertext includes the nonce and (in case of GCM cipher) the tag, as detailed in the 
+     * Parquet Modular Encryption specification.
+     * @param AAD Encryption AAD (ignored in case of CTR cipher)


Same as for Encryptor interface

nandorKollar · 2019-08-08T11:53:01Z

parquet-format-structures/src/main/java/org/apache/parquet/format/BlockCipher.java

+    /**
+     * Convenience decryption method that reads the length and ciphertext from the input stream.
+     * 
+     * @param from


Please document parameters, return value and exception. Also, I'd recommend use method overloading, and name this method as decrypt too. Eventually the only change between this and the other decrypt method is in the input parameter: the 'from' in this case is an input stream instead of a byte array, but otherwise they do the same.

sounds good

nandorKollar · 2019-08-08T11:53:14Z

parquet-format-structures/src/main/java/org/apache/parquet/format/BlockCipher.java

+     * Parquet Modular Encryption specification.
+     * @param AAD Encryption AAD (ignored in case of CTR cipher)
+     * @return plaintext The plaintext starts at offset 0 of the input value, and fills up the entire byte array.
+     * @throws IOException


Undocumented throws.

will document

nandorKollar · 2019-08-08T12:04:26Z

parquet-format-structures/src/main/java/org/apache/parquet/format/Util.java

+    to.write(encryptedBuffer);
  }
 }
+


nit: remove empty line here

nandorKollar · 2019-08-08T12:41:00Z

dev/travis-before_install-encryption.sh

@@ -0,0 +1,27 @@
+# Licensed to the Apache Software Foundation (ASF) under one


I'm afraid that in order for this to work, we'll need 3d8eae7 too. @ggershinsky should we rebase this branch onto master?

@nandorKollar please ignore my previous text here:), now I see what you mean. Yes, rebasing this branch onto master sounds good to me.

@nandorKollar can I ask you to rebase this branch? Or let me know if you prefer me to create a pull request for that.

@ggershinsky done, let me retrigger Travis to see the test results.

Seems that Travis fails because the default JDK version is now 11, #665 fixes it.

nandorKollar · 2019-08-12T09:48:10Z

I think Travis fails, because the branch specific install script clones parquet-format inside the parquet-mr working folder (see this path /home/travis/build/apache/parquet-mr/parquet-format/pom.xml), and Rat check fails because parquet-format/blob/master/.github/PULL_REQUEST_TEMPLATE.md is not excluded (in format it is - that's why format builds are not failing, but in mr no exclude pattern matches this file).

I recommend modifying the encryption specific before_install script with cloning and building parquet-format in build directory (before clone, move to TRAVIS_BUILD_DIR cd $TRAVIS_BUILD_DIR), and at the end going back to the original directory. Something like this:

cd $TRAVIS_BUILD_DIR
git clone https://github.com/apache/parquet-format.git
mvn -f $TRAVIS_BUILD_DIR/apache/parquet-format/pom.xm install -DskipTests
cd -

nandorKollar · 2019-08-12T11:38:41Z

Hmm okay, my bad, TRAVIS_BUILD_DIR is the cloned repository path as per https://docs.travis-ci.com/user/environment-variables/.
Then maybe this?

cd ../..
git clone -b encryption --single-branch https://github.com/apache/parquet-format.git
cd parquet-format
mvn install -DskipTests
cd $TRAVIS_BUILD_DIR

nandorKollar · 2019-08-12T12:51:55Z

Ouch, hit log record limit. When building parquet-format, I recommend adding --batch-mode so that download progress report messages are not emitted.

ggershinsky · 2019-08-12T13:58:59Z

yesss. thanks @nandorKollar for helping with this! I'll get working on addressing the comments.

ggershinsky · 2019-08-14T10:31:29Z

Hi guys, I think I've addressed all comments, but let me know if I've missed anything.

nandorKollar · 2019-08-15T18:31:27Z

@ggershinsky I think you missed some of the comments from @Fokko about final variables and formatting.

ggershinsky · 2019-08-16T04:11:24Z

@nandorKollar I have made these variables final as per the recommendation from @Fokko. Have I missed some variables? I'll check again.
Regarding auto-formatting - I don't know how to do that, that's why I've asked for an advise before. Is there a tool for that? Or a simple "ctrl-i" in Eclipse will do?

ggershinsky · 2019-08-19T09:43:35Z

@nandorKollar comments are addressed.
To format the code, I've used the Eclipse formatting tool.

Fokko

Codewise LGTM

* PARQUET-1228: Format Structures encryption (#613) * PARQUET-1286: Crypto package (#614) * PARQUET-1818: Fix bloom/encryption collision in format-structures (#771) * PARQUET-1817: Crypto Properties Factory (#769) * PARQUET-1229: Parquet MR encryption (#776) * PARQUET-1807: Encryption: Interop and Function test suite for Java version (#782) * PARQUET-1373: Encryption key tools (#615) Co-authored-by: shangxinli <31421745+shangxinli@users.noreply.github.com> Co-authored-by: Maya Anderson <mayaa@il.ibm.com>

Gidon Gershinsky and others added 2 commits February 11, 2019 15:00

commit to encryption branch

0f74d4f

plaintext footer fixes

67720b2

branch specific travis

223b021

Fokko reviewed May 22, 2019

View reviewed changes

chenjunjiedada reviewed Jul 22, 2019

View reviewed changes

nandorKollar reviewed Aug 8, 2019

View reviewed changes

nandorKollar closed this Aug 9, 2019

nandorKollar reopened this Aug 9, 2019

update travis setup

f73384c

travis folder fix

b4efe00

ggershinsky added 2 commits August 12, 2019 14:41

travis folder fix2

061edeb

format version to 2.7.0-snapshot

1b784e0

format build in batch mode

50b1932

implementing review suggestions

4eaaf78

no null casts, final var, code format

aa1096f

nandorKollar approved these changes Aug 22, 2019

View reviewed changes

Fokko approved these changes Aug 26, 2019

View reviewed changes

nandorKollar merged commit 3f2d0e7 into apache:encryption Aug 27, 2019

gszadovszky pushed a commit that referenced this pull request Jan 6, 2020

PARQUET-1228: Format Structures encryption (#613)

b6067ac

gszadovszky pushed a commit that referenced this pull request Feb 27, 2020

PARQUET-1228: Format Structures encryption (#613)

b887d49

shangxinli pushed a commit to shangxinli/parquet-mr that referenced this pull request Mar 1, 2020

PARQUET-1228: Format Structures encryption (apache#613)

1cdae82

gszadovszky pushed a commit that referenced this pull request Apr 22, 2020

PARQUET-1228: Format Structures encryption (#613)

c68e688

shangxinli pushed a commit to shangxinli/parquet-mr that referenced this pull request Jul 15, 2020

PARQUET-1228: Format Structures encryption (apache#613)

ac6e6ca

gszadovszky pushed a commit that referenced this pull request Jul 28, 2020

PARQUET-1228: Format Structures encryption (#613)

4516e97

		@@ -0,0 +1,27 @@
		# Licensed to the Apache Software Foundation (ASF) under one

Conversation

ggershinsky commented Feb 11, 2019

Uh oh!

gszadovszky commented Mar 18, 2019

Uh oh!

ggershinsky commented Mar 19, 2019

Uh oh!

Fokko 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!

ggershinsky commented May 30, 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!

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!

ggershinsky Aug 8, 2019 •

edited

Loading