Skip to content

Commit

Permalink
V2 Encryption Bug 4MB file downloads (#41813) (#42213)
Browse files Browse the repository at this point in the history
* V2 Encryption Bug 4MB file downloads (#41813)

* testing, need to get this branch on my other workstation

* adding broken test

* updating tests

* tests with logging

* updating test and adding fix

* making test paramaterized and adding change for only encryption v2 downloads

* fixing imports

* wip context addition

* context implementation and sync / async tests

* adding back unpulled tests

* consolidating one of the async util methods

* addressing all comments besides 1

* removing leftover changes and adding back test for V2

* kyle test

* adjusting unencrypted blob length calculation

* recording tests and fixing style

* addressing comments

* changelog update

* fixing messed up recorded test

* adding more test cases and updating adjustment method signature

* adding missed recordings and fixing style

* changing offsetAdjustment to a long to prevent int overflow, and added support for unencrypted blob length for blobinputstream

* adding test for integer overflow, removing redundant test, adding helper method to blobinputstream

* adjusting changelong

* removing unused imports

* changing blobinputstream util method to use long instead of Long

* update versions

* update readmes

* update changelogs

---------

Co-authored-by: Isabelle <[email protected]>
  • Loading branch information
srnagar and ibrandes authored Oct 8, 2024
1 parent 5076a39 commit 5e81fa0
Show file tree
Hide file tree
Showing 31 changed files with 333 additions and 57 deletions.
6 changes: 3 additions & 3 deletions eng/versioning/version_client.txt
Original file line number Diff line number Diff line change
Expand Up @@ -186,12 +186,12 @@ com.azure:azure-security-keyvault-perf;1.0.0-beta.1;1.0.0-beta.1
com.azure:azure-sdk-template;1.1.1234;1.2.2-beta.1
com.azure:azure-sdk-template-two;1.0.0-beta.1;1.0.0-beta.1
com.azure:azure-sdk-template-three;1.0.0-beta.1;1.0.0-beta.1
com.azure:azure-storage-blob;12.27.1;12.28.0
com.azure:azure-storage-blob;12.27.1;12.28.1
com.azure:azure-storage-blob-batch;12.23.1;12.24.0
com.azure:azure-storage-blob-changefeed;12.0.0-beta.24;12.0.0-beta.25
com.azure:azure-storage-blob-cryptography;12.26.1;12.27.0
com.azure:azure-storage-blob-cryptography;12.26.1;12.27.1
com.azure:azure-storage-blob-nio;12.0.0-beta.25;12.0.0-beta.26
com.azure:azure-storage-common;12.26.1;12.27.0
com.azure:azure-storage-common;12.26.1;12.27.1
com.azure:azure-storage-file-share;12.23.1;12.24.0
com.azure:azure-storage-file-datalake;12.20.1;12.21.0
com.azure:azure-storage-internal-avro;12.12.1;12.13.0
Expand Down
4 changes: 2 additions & 2 deletions sdk/storage/azure-storage-blob-batch/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@
<dependency>
<groupId>com.azure</groupId>
<artifactId>azure-storage-blob</artifactId>
<version>12.28.0</version> <!-- {x-version-update;com.azure:azure-storage-blob;current} -->
<version>12.28.1</version> <!-- {x-version-update;com.azure:azure-storage-blob;current} -->
</dependency>

<!-- Added this dependency to include necessary annotations used by reactor core.
Expand All @@ -81,7 +81,7 @@
<dependency>
<groupId>com.azure</groupId>
<artifactId>azure-storage-common</artifactId>
<version>12.27.0</version> <!-- {x-version-update;com.azure:azure-storage-common;current} -->
<version>12.27.1</version> <!-- {x-version-update;com.azure:azure-storage-common;current} -->
<classifier>tests</classifier>
<type>test-jar</type>
<scope>test</scope>
Expand Down
4 changes: 2 additions & 2 deletions sdk/storage/azure-storage-blob-changefeed/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@
<dependency>
<groupId>com.azure</groupId>
<artifactId>azure-storage-blob</artifactId>
<version>12.28.0</version> <!-- {x-version-update;com.azure:azure-storage-blob;current} -->
<version>12.28.1</version> <!-- {x-version-update;com.azure:azure-storage-blob;current} -->
</dependency>

<!-- Added this dependency to include necessary annotations used by reactor core.
Expand All @@ -93,7 +93,7 @@
<dependency>
<groupId>com.azure</groupId>
<artifactId>azure-storage-common</artifactId>
<version>12.27.0</version> <!-- {x-version-update;com.azure:azure-storage-common;current} -->
<version>12.27.1</version> <!-- {x-version-update;com.azure:azure-storage-common;current} -->
<classifier>tests</classifier>
<type>test-jar</type>
<scope>test</scope>
Expand Down
9 changes: 9 additions & 0 deletions sdk/storage/azure-storage-blob-cryptography/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,14 @@
# Release History

## 12.27.1 (2024-10-08)

### Bugs Fixed
- Fixed a bug where downloadToFile and openInputStream was throwing an InvalidRange exception if the target file size was a multiple of the
authenticated region length.

#### Dependency Updates
- Upgraded `azure-storage-blob` from `12.28.0` to version `12.28.1`.

## 12.27.0 (2024-09-17)

### Features Added
Expand Down
2 changes: 1 addition & 1 deletion sdk/storage/azure-storage-blob-cryptography/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ add the direct dependency to your project as follows.
<dependency>
<groupId>com.azure</groupId>
<artifactId>azure-storage-blob-cryptography</artifactId>
<version>12.27.0</version>
<version>12.27.1</version>
</dependency>
```
[//]: # ({x-version-update-end})
Expand Down
2 changes: 1 addition & 1 deletion sdk/storage/azure-storage-blob-cryptography/assets.json
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,5 @@
"AssetsRepo": "Azure/azure-sdk-assets",
"AssetsRepoPrefixPath": "java",
"TagPrefix": "java/storage/azure-storage-blob-cryptography",
"Tag": "java/storage/azure-storage-blob-cryptography_e245db55c0"
"Tag": "java/storage/azure-storage-blob-cryptography_9fd41be58a"
}
6 changes: 3 additions & 3 deletions sdk/storage/azure-storage-blob-cryptography/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@

<groupId>com.azure</groupId>
<artifactId>azure-storage-blob-cryptography</artifactId>
<version>12.27.0</version> <!-- {x-version-update;com.azure:azure-storage-blob-cryptography;current} -->
<version>12.27.1</version> <!-- {x-version-update;com.azure:azure-storage-blob-cryptography;current} -->

<name>Microsoft Azure client library for Blob Storage cryptography</name>
<description>This module contains client library for Microsoft Azure Blob Storage cryptography.</description>
Expand Down Expand Up @@ -71,13 +71,13 @@
<dependency>
<groupId>com.azure</groupId>
<artifactId>azure-storage-blob</artifactId>
<version>12.28.0</version> <!-- {x-version-update;com.azure:azure-storage-blob;current} -->
<version>12.28.1</version> <!-- {x-version-update;com.azure:azure-storage-blob;current} -->
</dependency>

<dependency>
<groupId>com.azure</groupId>
<artifactId>azure-storage-common</artifactId>
<version>12.27.0</version> <!-- {x-version-update;com.azure:azure-storage-common;current} -->
<version>12.27.1</version> <!-- {x-version-update;com.azure:azure-storage-common;current} -->
<classifier>tests</classifier>
<type>test-jar</type>
<scope>test</scope>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -750,9 +750,14 @@ private <T> Mono<T> populateRequestConditionsAndContext(BlobRequestConditions re

String encryptionDataKey = StorageImplUtils.getEncryptionDataKey(response.getValue().getMetadata());
if (encryptionDataKey != null) {
result = result.contextWrite(context -> context.put(ENCRYPTION_DATA_KEY,
EncryptionData.getAndValidateEncryptionData(encryptionDataKey, requiresEncryption)));
EncryptionData encryptionData = EncryptionData.getAndValidateEncryptionData(encryptionDataKey,
requiresEncryption);

result = result.contextWrite(context -> context.put(ENCRYPTION_DATA_KEY, encryptionData))
.contextWrite(context -> context.put(Constants.ADJUSTED_BLOB_LENGTH_KEY,
EncryptedBlobLength.computeAdjustedBlobLength(encryptionData, response.getValue().getBlobSize())));
}

return result;
});
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -489,8 +489,12 @@ private Context populateRequestConditionsAndContext(BlobRequestConditions reques

String encryptionDataKey = StorageImplUtils.getEncryptionDataKey(initialProperties.getMetadata());
if (encryptionDataKey != null) {
context = context.addData(ENCRYPTION_DATA_KEY, EncryptionData.getAndValidateEncryptionData(
encryptionDataKey, encryptedBlobAsyncClient.isEncryptionRequired()));
EncryptionData encryptionData = EncryptionData.getAndValidateEncryptionData(
encryptionDataKey, encryptedBlobAsyncClient.isEncryptionRequired());

context = context.addData(ENCRYPTION_DATA_KEY, encryptionData)
.addData(Constants.ADJUSTED_BLOB_LENGTH_KEY,
EncryptedBlobLength.computeAdjustedBlobLength(encryptionData, initialProperties.getBlobSize()));
}
return context;
}
Expand Down Expand Up @@ -572,5 +576,4 @@ public BlobQueryResponse queryWithResponse(BlobQueryOptions queryOptions,
BlobClientSideEncryptionOptions getClientSideEncryptionOptions() {
return encryptedBlobAsyncClient.getClientSideEncryptionOptions();
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

package com.azure.storage.blob.specialized.cryptography;

import com.azure.core.util.logging.ClientLogger;

import static com.azure.storage.blob.specialized.cryptography.CryptographyConstants.ENCRYPTION_PROTOCOL_V1;
import static com.azure.storage.blob.specialized.cryptography.CryptographyConstants.ENCRYPTION_PROTOCOL_V2;
import static com.azure.storage.blob.specialized.cryptography.CryptographyConstants.ENCRYPTION_PROTOCOL_V2_1;
import static com.azure.storage.blob.specialized.cryptography.CryptographyConstants.NONCE_LENGTH;
import static com.azure.storage.blob.specialized.cryptography.CryptographyConstants.TAG_LENGTH;

/**
* This class provides helper methods for adjusting encrypted downloads.
*/
final class EncryptedBlobLength {
private static final ClientLogger LOGGER = new ClientLogger(EncryptedBlobLength.class);

static Long computeAdjustedBlobLength(EncryptionData encryptionData, Long encryptedLength) {
switch (encryptionData.getEncryptionAgent().getProtocol()) {
/*
Technically, the total unencrypted length may be different for v1,
but because this helper method is only used for partitioning ranged downloads,
the size does not need to be adjusted for v1.
*/
case ENCRYPTION_PROTOCOL_V1:
return encryptedLength;
case ENCRYPTION_PROTOCOL_V2:
case ENCRYPTION_PROTOCOL_V2_1:
long regionLength = encryptionData.getEncryptedRegionInfo().getDataLength();
long region = (long) Math.ceil((double) encryptedLength / (double) (regionLength + NONCE_LENGTH + TAG_LENGTH));
long offset = (NONCE_LENGTH + TAG_LENGTH) * region;
return encryptedLength - offset;
default:
throw LOGGER.logExceptionAsError(new IllegalArgumentException("Unexpected protocol version"));
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ final class EncryptedBlobRange {
* 0-31 for v1
* 0-(4mb+12-1)
*/
private final int offsetAdjustment;
private final Long offsetAdjustment;

/**
* How many bytes to download, including the adjustments for encryption block boundaries and the IV.
Expand Down Expand Up @@ -78,7 +78,7 @@ static EncryptedBlobRange getEncryptedBlobRangeFromHeader(String stringRange, En
EncryptedBlobRange(BlobRange originalRange, EncryptionData encryptionData) {
if (originalRange == null) {
this.originalRange = new BlobRange(0);
this.offsetAdjustment = 0;
this.offsetAdjustment = 0L;
this.amountPlaintextToSkip = 0; // In cases where this block is executed, this value does not matter
return;
}
Expand Down Expand Up @@ -111,7 +111,7 @@ static EncryptedBlobRange getEncryptedBlobRangeFromHeader(String stringRange, En
}
}

this.offsetAdjustment = tempOffsetAdjustment;
this.offsetAdjustment = (long) tempOffsetAdjustment;

/*
Align adjustedDownloadCount with encryption block boundary at the end of the range. Note that it is impossible
Expand Down Expand Up @@ -153,7 +153,7 @@ static EncryptedBlobRange getEncryptedBlobRangeFromHeader(String stringRange, En
}

// Offset adjustment is difference in two starting values
this.offsetAdjustment = (int) (originalRange.getOffset() - regionStartOffset);
this.offsetAdjustment = (originalRange.getOffset() - regionStartOffset);

break;
default:
Expand All @@ -171,7 +171,7 @@ BlobRange getOriginalRange() {
/**
* @return Offset from beginning of BlobRange, 0-31.
*/
int getOffsetAdjustment() {
Long getOffsetAdjustment() {
return this.offsetAdjustment;
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

package com.azure.storage.blob.specialized.cryptography;

import com.azure.storage.common.implementation.Constants;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.Arguments;
import org.junit.jupiter.params.provider.MethodSource;

import java.util.stream.Stream;

import static com.azure.storage.blob.specialized.cryptography.CryptographyConstants.GCM_ENCRYPTION_REGION_LENGTH;
import static com.azure.storage.blob.specialized.cryptography.CryptographyConstants.NONCE_LENGTH;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertThrows;

public class EncryptedBlobLengthTests extends BlobCryptographyTestBase {

//to prevent having to cast int -> long everywhere
private static final long FOUR_MB = 4 * Constants.MB;
private static final long SIXTEEN_MB = 16 * Constants.MB;
private static final long ONE_MB = Constants.MB;
private static final long ONE_KB = Constants.KB;

@ParameterizedTest
@MethodSource("correctAdjustedLengthV1Supplier")
public void correctAdjustedLengthV1(Long encryptedLength) {
EncryptionData encryptionData = new EncryptionData();
encryptionData.setEncryptionAgent(new EncryptionAgent("1.0", null));

Long newLength = EncryptedBlobLength.computeAdjustedBlobLength(encryptionData, encryptedLength);
assertEquals(encryptedLength, newLength);
}

private static Stream<Arguments> correctAdjustedLengthV1Supplier() {
return Stream.of(
Arguments.of(FOUR_MB),
Arguments.of(SIXTEEN_MB)
);
}

@ParameterizedTest
@MethodSource("correctAdjustedLengthV2Supplier")
public void correctAdjustedLengthV2(Long encryptedLength, Long expectedLength) {
EncryptionData encryptionData = new EncryptionData();
encryptionData.setEncryptionAgent(new EncryptionAgent("2.0", null));
encryptionData.setEncryptedRegionInfo(new EncryptedRegionInfo(GCM_ENCRYPTION_REGION_LENGTH, NONCE_LENGTH));

Long newLength = EncryptedBlobLength.computeAdjustedBlobLength(encryptionData, encryptedLength);
assertEquals(expectedLength, newLength);
}

private static Stream<Arguments> correctAdjustedLengthV2Supplier() {
return Stream.of(
Arguments.of(FOUR_MB + 28, FOUR_MB),
Arguments.of(FOUR_MB + 57, FOUR_MB + 1),
Arguments.of(SIXTEEN_MB + 112, SIXTEEN_MB),
Arguments.of(28L, 0L),
Arguments.of(0L, 0L),
Arguments.of(ONE_MB + 28, ONE_MB)
);
}

@ParameterizedTest
@MethodSource("correctAdjustedLengthVariableRegionSupplier")
public void correctAdjustedLengthVariableRegion(Long encryptedLength, Long expectedLength, Long regionLength) {
EncryptionData encryptionData = new EncryptionData();
encryptionData.setEncryptionAgent(new EncryptionAgent("2.1", null));
encryptionData.setEncryptedRegionInfo(new EncryptedRegionInfo(regionLength, NONCE_LENGTH));

Long newLength = EncryptedBlobLength.computeAdjustedBlobLength(encryptionData, encryptedLength);
assertEquals(expectedLength, newLength);
}

private static Stream<Arguments> correctAdjustedLengthVariableRegionSupplier() {
return Stream.of(
Arguments.of(FOUR_MB + 112, FOUR_MB, ONE_MB),
Arguments.of(SIXTEEN_MB + 448, SIXTEEN_MB, ONE_MB),
Arguments.of(FOUR_MB + 114688, FOUR_MB, ONE_KB),
Arguments.of(SIXTEEN_MB + 458752, SIXTEEN_MB, ONE_KB),
Arguments.of(FOUR_MB + 448, FOUR_MB, 256 * ONE_KB),
Arguments.of(SIXTEEN_MB + 1792, SIXTEEN_MB, 256 * ONE_KB),
Arguments.of(ONE_MB + 28672, ONE_MB, ONE_KB),
Arguments.of(ONE_MB + 28, ONE_MB, ONE_MB)
);
}

@Test
public void badProtocol() {
EncryptionData encryptionData = new EncryptionData();
encryptionData.setEncryptionAgent(new EncryptionAgent("garbage", null));

assertThrows(IllegalArgumentException.class, () -> EncryptedBlobLength.computeAdjustedBlobLength(encryptionData,
null));
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -114,8 +114,7 @@ public void encryptedBlobRangeFromEncryptionData(int regionLength) {

@ParameterizedTest
@MethodSource("provideRanges")
public void testAdjustedBlobRange(int originalOffset, long originalCount, int expectedNewOffset, long expectedNewCount) {
int regionLength = 16;
public void testAdjustedBlobRange(long originalOffset, long originalCount, long expectedNewOffset, long expectedNewCount, int regionLength) {
EncryptionData encryptionData = new EncryptionData()
.setEncryptionAgent(new EncryptionAgent(ENCRYPTION_PROTOCOL_V2, EncryptionAlgorithm.AES_GCM_256))
.setEncryptedRegionInfo(new EncryptedRegionInfo(regionLength, NONCE_LENGTH));
Expand All @@ -131,10 +130,11 @@ public void testAdjustedBlobRange(int originalOffset, long originalCount, int ex

private static Stream<Arguments> provideRanges() {
return Stream.of(
Arguments.of(5, 10, 0, 44), // Entirely within a single region, adjustment includes nonce and tag
Arguments.of(16, 16, 44, 44), // Exactly one region
Arguments.of(15, 35, 0, 176), // Straddles across four regions
Arguments.of(32, 15, 88, 44) // Starts exactly at the second region
Arguments.of(5L, 10L, 0L, 44L, 16), // Entirely within a single region, adjustment includes nonce and tag
Arguments.of(16L, 16L, 44L, 44L, 16), // Exactly one region
Arguments.of(15L, 35L, 0L, 176L, 16), // Straddles across four regions
Arguments.of(32L, 15L, 88L, 44L, 16), // Starts exactly at the second region
Arguments.of(3674210304L, 10066367L, 8818104720L, 24159312L, 20) // Tests for integer overflow with a very large blob
);
}
}
Loading

0 comments on commit 5e81fa0

Please sign in to comment.