GCM encryption stream by ggershinsky · Pull Request #3231 · apache/iceberg

ggershinsky · 2021-10-06T12:29:00Z

Implements #2060

Besides encrypting and signing metadata (avro, json, puffin), this stream can also encrypt Avro data.

jackye1995 · 2021-10-12T20:13:23Z

core/src/main/java/org/apache/iceberg/encryption/AesGcmOutputStream.java

+
+public class AesGcmOutputStream extends PositionOutputStream {
+  // AES-GCM parameters
+  public static final int GCM_NONCE_LENGTH = 12; // in bytes


looks like we should make these variables in a shared util class, instead of importing this from the output stream to the input stream

jackye1995 · 2021-10-12T20:17:24Z

core/src/main/java/org/apache/iceberg/encryption/AesGcmInputStream.java

+      gcmCipher = Cipher.getInstance("AES/GCM/NoPadding");
+    } catch (GeneralSecurityException e) {
+      throw new IOException(e);
+    }


nit: newline after control statement

jackye1995 · 2021-10-12T20:25:55Z

Did a comparison against my internal impl, mostly look correct to me. Will take another look after all the remaining methods are implemented.

flyrain · 2021-10-19T16:56:35Z

core/src/main/java/org/apache/iceberg/encryption/AesGcmInputStream.java

+import static org.apache.iceberg.encryption.AesGcmOutputStream.HEADER_SIZE_LENGTH;
+
+public class AesGcmInputStream extends SeekableInputStream {
+  private SeekableInputStream sourceStream;


Can we make it final?

sure, will change in the next commit

flyrain · 2021-10-19T16:57:10Z

core/src/main/java/org/apache/iceberg/encryption/AesGcmInputStream.java

+  private int numberOfBlocks;
+  private int lastBlockSize;
+  private long plainStreamSize;
+  private byte[] fileAadPrefix;


It is not used anywhere. Do we need it?

Oh, there is a TODO item referring it in the method read, never mind.

Both plainStreamSize and fileAadPrefix could be final.

flyrain · 2021-10-19T17:00:52Z

core/src/main/java/org/apache/iceberg/encryption/AesGcmInputStream.java

+  private long plainStreamSize;
+  private byte[] fileAadPrefix;
+
+  AesGcmInputStream(SeekableInputStream sourceStream, long sourceLength,


Do we need it to be public since the class itself is public? Or we can have a builder.

Always created by an AesGcmInputFile in the same package.

flyrain · 2021-10-19T17:02:28Z

core/src/main/java/org/apache/iceberg/encryption/AesGcmInputStream.java

+  private SeekableInputStream sourceStream;
+  private long netSourceFileSize;
+
+  private Cipher gcmCipher;


Nit: make it final.

flyrain · 2021-10-19T18:44:21Z

core/src/main/java/org/apache/iceberg/encryption/AesGcmOutputStream.java

+  private PositionOutputStream targetStream;
+
+  private Cipher gcmCipher;
+  private SecureRandom random;
+  private SecretKey key;
+  private byte[] nonce;


These fields can be final.

flyrain

Thanks @ggershinsky for the patch. Can we add a unit test for these two classes?

liujinhui1994 · 2022-01-06T09:54:44Z

@ggershinsky Is this PR still going on?

ggershinsky · 2022-01-06T11:12:27Z

same as in #2639 (comment)

ggershinsky · 2022-06-21T07:57:42Z

Thanks @ggershinsky for the patch. Can we add a unit test for these two classes?

done

flyrain · 2022-06-21T18:59:53Z

core/src/main/java/org/apache/iceberg/encryption/AesGcmInputFile.java

+
+  @Override
+  public long getLength() {
+    Preconditions.checkArgument(plaintextLength >= 0, "Length is known after new stream is created");


Should we set the length if field plaintextLength is -1? Looks like this method is valid only after newStream() is called.

if(plaintextLength == -1) { this.newStream(); }

When do we close the stream in this case? Or do we just make and close it in that case?

Both suggestions sound good to me.

flyrain · 2022-06-21T19:04:48Z

core/src/main/java/org/apache/iceberg/encryption/AesGcmInputStream.java

+      int toLoad = lastBlock ? lastBlockSize : cipherBlockSize;
+      int loaded = sourceStream.read(ciphertextBlockBuffer, 0, toLoad);
+      if (loaded != toLoad) {
+        throw new IOException("Read " + loaded + " instead of " + toLoad);


Minor suggestion: Would it be a bit easier to understand if message is "Should read toLoad data, but only get loaded data"?

flyrain · 2022-06-21T19:05:33Z

core/src/main/java/org/apache/iceberg/encryption/AesGcmInputStream.java

+  @Override
+  public void seek(long newPos) throws IOException {
+    if (newPos >= plainStreamSize) {
+      throw new IOException("At or beyond max stream size " + plainStreamSize + ", " + newPos);


The massage could be

"The seeking position " + newPos + " has reached or exceeded the max stream size " + plainStreamSize.

flyrain · 2022-06-21T19:06:01Z

core/src/main/java/org/apache/iceberg/encryption/AesGcmInputStream.java

+
+  @Override
+  public synchronized void reset() throws IOException {
+    throw new UnsupportedOperationException();


Looks like we don't have to implement it, the abstract class InputStream has the same logic.

flyrain · 2022-06-21T19:06:28Z

core/src/main/java/org/apache/iceberg/encryption/AesGcmOutputFile.java

+    try {
+      return new AesGcmOutputStream(targetFile.create(), dataKey, null);
+    } catch (IOException e) {
+      throw new UncheckedIOException(e);


Can we add message like, "Failed to create file: %s", targetFile.location()?

flyrain · 2022-06-21T19:06:48Z

core/src/main/java/org/apache/iceberg/encryption/AesGcmOutputFile.java

+    try {
+      return new AesGcmOutputStream(targetFile.createOrOverwrite(), dataKey, null);
+    } catch (IOException e) {
+      throw new UncheckedIOException(e);


Can we add message like, "Failed to create or overwrite file: %s", targetFile.location()?

flyrain · 2022-06-21T19:07:24Z

core/src/main/java/org/apache/iceberg/encryption/AesGcmOutputStream.java

+  private SecretKey key;
+  private byte[] nonce;
+
+  private int blockSize = 1024 * 1024;


Make it a final static field?

flyrain · 2022-06-21T19:07:42Z

core/src/main/java/org/apache/iceberg/encryption/AesGcmOutputStream.java

+  private int positionInBuffer;
+  private long streamPosition;
+  private int currentBlockIndex;
+  private byte[] fileAadPrefix;


It can be a final field.

flyrain · 2022-06-21T19:08:52Z

core/src/main/java/org/apache/iceberg/encryption/AesGcmOutputStream.java

+    try {
+      int encrypted = gcmCipher.doFinal(plaintextBlockBuffer, 0, positionInBuffer, cipherText, GCM_NONCE_LENGTH);
+      Preconditions.checkArgument((encrypted == (positionInBuffer + GCM_TAG_LENGTH)),
+          "Wrong length of encrypted output: " + encrypted + " vs " + (positionInBuffer + GCM_TAG_LENGTH));


How about a message like this?

"Wrong length of encrypted output: expected " + (positionInBuffer + GCM_TAG_LENGTH)) + " but " + encrypted;

flyrain · 2022-06-21T19:09:20Z

core/src/main/java/org/apache/iceberg/encryption/AesGcmOutputStream.java

+    targetStream.write(cipherText);
+  }
+
+  static byte[] calculateAAD(byte[] fileAadPrefix, int currentBlockIndex) {


This can go to a common place as well since both classes need it. We may have a util class, e.g. GcmUtil

flyrain · 2022-06-21T19:12:12Z

core/src/main/java/org/apache/iceberg/encryption/AesGcmInputStream.java

+    try {
+      gcmCipher = Cipher.getInstance("AES/GCM/NoPadding");
+    } catch (GeneralSecurityException e) {
+      throw new IOException(e);


Add message for easier debugging?

new IOException("Failed to get an instance of GCM cipher", e)

flyrain · 2022-06-21T19:15:46Z

core/src/main/java/org/apache/iceberg/encryption/AesGcmInputStream.java

+    Preconditions.checkArgument(fetched == AesGcmOutputStream.PREFIX_LENGTH,
+        "Insufficient read " + fetched);


They can be in the same line.
IIUC, this means the stream length is not long enough. Can we say something like "The stream length should be at least " + AesGcmOutputStream.PREFIX_LENGTH?

Aren't we also in danger of the read being less than prefix bytes?

flyrain · 2022-06-21T19:17:22Z

core/src/main/java/org/apache/iceberg/encryption/AesGcmInputStream.java

+  private int currentBlockIndex;
+  private int currentOffsetInPlainBlock;
+  private int numberOfBlocks;
+  private int lastBlockSize;


Can we rename it to lastCipherBlockSize to make it more readable?

flyrain · 2022-06-21T19:17:31Z

core/src/main/java/org/apache/iceberg/encryption/AesGcmInputStream.java

+  private SecretKey key;
+  private byte[] nonce;


final as well.

RussellSpitzer · 2022-06-22T16:31:08Z

core/src/test/java/org/apache/iceberg/encryption/TestGcmStreams.java

+  public TemporaryFolder temp = new TemporaryFolder();
+
+  @Test
+  public void testRandomWriteRead() throws IOException {


I like this, could I ask for just 4 more test cases

Empty File (no bits)
File that aligns perfectly with encryption Chunk Size
File that is exactly one byte to larger than the aligned and one that is one byte smaller than the aligned file. (we probably hit this unaligned version with the testFileSize below but just to make sure)

Sure, will add

RussellSpitzer · 2022-06-22T17:16:09Z

core/src/main/java/org/apache/iceberg/encryption/AesGcmInputStream.java

+    System.arraycopy(prefixBytes, 0, magic, 0, AesGcmOutputStream.MAGIC_ARRAY.length);
+
+    Preconditions.checkArgument(Arrays.equals(AesGcmOutputStream.MAGIC_ARRAY, magic),
+        "File with wrong magic string. Should start with " + AesGcmOutputStream.MAGIC_STRING);


Instead of "file with wrong magic string" -> "Cannot read unencrypted file, missing header containing: ..."?

RussellSpitzer · 2022-06-22T17:53:09Z

core/src/main/java/org/apache/iceberg/encryption/AesGcmInputStream.java

+    cipherBlockSize = plainBlockSize + AesGcmOutputStream.GCM_NONCE_LENGTH + AesGcmOutputStream.GCM_TAG_LENGTH;
+
+    try {
+      gcmCipher = Cipher.getInstance("AES/GCM/NoPadding");


Constant for this?

Yep. We actually already have a Ciphers class; let's re-use it (with some additions) for all GCM-related things

Refactor AesGcmInputStream.

ggershinsky · 2023-07-31T07:45:41Z

I had a lot of comments, so I went ahead and implemented most of them on top of this. I think it simplified how the input stream works quite a bit and I fixed a lot of the comments that I made. Please see ggershinsky#6.

Thanks! Merged the PR.

rdblue · 2023-07-31T23:27:56Z

core/src/main/java/org/apache/iceberg/encryption/AesGcmInputStream.java

+  }
+
+  private int availableInCurrentBlock() {
+    if (currentPlainBlockIndex < 0) {


When I was refactoring, there was a bug that I fixed when the block for plainStreamPosition didn't match currentPlainBlockIndex. I ended up fixing the problem by setting currentPlainBlockIndex = -1 in a place that I had missed. Now that I'm thinking about it more, I think the right solution is actually to update the check here instead. That way the bytes available will only be non-zero if the current block matches the position.

Here's a diff that does what I'm talking about and still passes tests:

diff --git a/core/src/main/java/org/apache/iceberg/encryption/AesGcmInputStream.java b/core/src/main/java/org/apache/iceberg/encryption/AesGcmInputStream.java index a63134f31d..88e9b36c25 100644 --- a/core/src/main/java/org/apache/iceberg/encryption/AesGcmInputStream.java +++ b/core/src/main/java/org/apache/iceberg/encryption/AesGcmInputStream.java @@ -97,7 +97,7 @@ public class AesGcmInputStream extends SeekableInputStream { } private int availableInCurrentBlock() { - if (currentPlainBlockIndex < 0) { + if (blockIndex(plainStreamPosition) != currentPlainBlockIndex) { return 0; } @@ -130,10 +130,6 @@ public class AesGcmInputStream extends SeekableInputStream { remainingBytesToRead -= bytesToCopy; resultBufferOffset += bytesToCopy; this.plainStreamPosition += bytesToCopy; - if (blockIndex(plainStreamPosition) != currentPlainBlockIndex) { - // invalidate the current block - this.currentPlainBlockIndex = -1; - } } else if (available() > 0) { decryptBlock(blockIndex(plainStreamPosition)); @@ -157,10 +153,6 @@ public class AesGcmInputStream extends SeekableInputStream { } this.plainStreamPosition = newPos; - if (blockIndex(plainStreamPosition) != currentPlainBlockIndex) { - // invalidate the current block - this.currentPlainBlockIndex = -1; - } } @Override @@ -177,10 +169,6 @@ public class AesGcmInputStream extends SeekableInputStream { } this.plainStreamPosition += n; - if (blockIndex(plainStreamPosition) != currentPlainBlockIndex) { - // invalidate the current block - this.currentPlainBlockIndex = -1; - } return n; }

SGTM. Applied this change.

core/src/main/java/org/apache/iceberg/encryption/AesGcmOutputFile.java

rdblue · 2023-07-31T23:34:10Z

core/src/main/java/org/apache/iceberg/encryption/AesGcmOutputStream.java

+    this.gcmEncryptor = new Ciphers.AesGcmEncryptor(aesKey);
+    this.plainBlockBuffer = new byte[Ciphers.PLAIN_BLOCK_SIZE];
+    this.positionInBuffer = 0;
+    this.streamPosition = 0;


Shouldn't this always be targetStream.pos() + positionInBuffer?

this value is returned by

@Override public long getPos() throws IOException { return streamPosition; }

so is not related directly to targetStream.pos() and positionInBuffer.
I'll rename positionInBuffer to positionInPlainBlock, and plainBlockBuffer to plainBlock, to make the naming more consistent.

I don't see why what I said wouldn't always be true. If so, we could eliminate this extra state.

core/src/main/java/org/apache/iceberg/encryption/AesGcmOutputStream.java

core/src/main/java/org/apache/iceberg/encryption/Ciphers.java

core/src/main/java/org/apache/iceberg/encryption/AesGcmOutputStream.java

core/src/main/java/org/apache/iceberg/encryption/Ciphers.java

rdblue · 2023-08-13T19:30:14Z

core/src/main/java/org/apache/iceberg/encryption/Ciphers.java

+                  + enciphered
+                  + ". Must be "
+                  + plaintextLength
+                  + GCM_TAG_LENGTH);


I think this can be more clear because it doesn't tell the reader that the encryption didn't work right.

Instead, how about Failed to encrypt block: expected %s encrypted bytes but produced %s bytes?

core/src/main/java/org/apache/iceberg/encryption/Ciphers.java

core/src/test/java/org/apache/iceberg/encryption/TestCiphers.java

rdblue · 2023-09-04T20:05:28Z

@ggershinsky, I opened ggershinsky#8 with the remaining changes for this PR.

The main change is to write an empty block for empty files. Otherwise an attacker could replace a valid file with an empty file without detection.

Final changes for AES GCM encryption streams

ggershinsky · 2023-09-05T08:57:51Z

Cool! LGTM. This could have been detected via external check of details (such as the file length, stored in parent metadata), but I agree doing this in the stream is cleaner and more reliable. Thanks for the PR, merged.

github-actions bot added the core label Oct 6, 2021

jackye1995 reviewed Oct 12, 2021

View reviewed changes

flyrain reviewed Oct 19, 2021

View reviewed changes

flyrain reviewed Jun 21, 2022

View reviewed changes

RussellSpitzer reviewed Jun 22, 2022

View reviewed changes

ggershinsky and others added 2 commits July 31, 2023 10:27

Merge pull request #6 from rdblue/gcm_encryption_stream

91305e9

Refactor AesGcmInputStream.

spotless apply

82fc5b0

address review comments

375ef20

rdblue reviewed Jul 31, 2023

View reviewed changes

core/src/main/java/org/apache/iceberg/encryption/AesGcmOutputFile.java Outdated Show resolved Hide resolved

rdblue reviewed Jul 31, 2023

View reviewed changes

core/src/main/java/org/apache/iceberg/encryption/AesGcmOutputFile.java Outdated Show resolved Hide resolved

rdblue reviewed Jul 31, 2023

View reviewed changes

core/src/main/java/org/apache/iceberg/encryption/AesGcmOutputStream.java Outdated Show resolved Hide resolved

rdblue requested changes Jul 31, 2023

View reviewed changes

address new review comments

cf8827f

rdblue reviewed Aug 13, 2023

View reviewed changes

core/src/main/java/org/apache/iceberg/encryption/AesGcmOutputStream.java Outdated Show resolved Hide resolved

rdblue reviewed Aug 13, 2023

View reviewed changes

core/src/main/java/org/apache/iceberg/encryption/AesGcmOutputStream.java Outdated Show resolved Hide resolved

rdblue reviewed Aug 13, 2023

View reviewed changes

core/src/main/java/org/apache/iceberg/encryption/Ciphers.java Outdated Show resolved Hide resolved

rdblue reviewed Aug 13, 2023

View reviewed changes

core/src/main/java/org/apache/iceberg/encryption/Ciphers.java Show resolved Hide resolved

rdblue reviewed Aug 13, 2023

View reviewed changes

core/src/main/java/org/apache/iceberg/encryption/Ciphers.java Show resolved Hide resolved

rdblue reviewed Aug 13, 2023

View reviewed changes

core/src/test/java/org/apache/iceberg/encryption/TestCiphers.java Outdated Show resolved Hide resolved

ggershinsky and others added 4 commits August 14, 2023 14:09

address comments

de84a93

Merge branch 'master' into gcm_encryption_stream

7cdba0e

Add tests and minor updates to streams.

a64f4f4

Support AAD validation for empty files.

89f3015

rdblue mentioned this pull request Sep 4, 2023

Final changes for AES GCM encryption streams ggershinsky/iceberg#8

Merged

rdblue and others added 2 commits September 4, 2023 13:15

Add check for minimum stream length.

fcb1ba0

Merge pull request #8 from rdblue/gcm_encryption_stream

792a7ce

Final changes for AES GCM encryption streams

rdblue merged commit 7d513f4 into apache:master Sep 10, 2023

bhoberman mentioned this pull request Nov 7, 2023

[Feature Request][Other] Encrypted Delta Lakes delta-io/delta#2269

Open

8 tasks

ggershinsky mentioned this pull request Nov 10, 2025

Core, Data: File Format API interfaces #12774

Merged

		Preconditions.checkArgument(fetched == AesGcmOutputStream.PREFIX_LENGTH,
		"Insufficient read " + fetched);

Conversation

ggershinsky commented Oct 6, 2021 • 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!

jackye1995 commented Oct 12, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ggershinsky Jun 21, 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!

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!

flyrain left a comment

Choose a reason for hiding this comment

Uh oh!

liujinhui1994 commented Jan 6, 2022

Uh oh!

ggershinsky commented Jan 6, 2022

Uh oh!

ggershinsky commented Jun 21, 2022

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!

Choose a reason for hiding this comment

ggershinsky commented Oct 6, 2021 •

edited

Loading

ggershinsky Jun 21, 2022 •

edited

Loading