Add flexible checksum support for non-streaming cases #4376

haydenbaker · 2023-08-31T16:51:29Z

Motivation and Context

Flexible checksum support is needed for a few specific cases where a custom checksum can be added to a request
This PR addresses the non-streaming cases for unsigned and signed payload
A follow-up PR will be sent to address the streaming (aws-chunked) case, but there is no plan to support flexible checksums for event-streaming case or non-streaming CRT-V4a

Modifications

Copies SdkChecksum and related code from sdk-core to use internally without importing sdk-core
Add new internal classes to handle flexible checksum cases, and remove the old digest-related stream and subscriber code
Cleans up a couple of APIs that should be internal, not protected

Testing

Run new and existing tests, make sure no failures
Run subscriber TCK test, make sure the implementation is compliant

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)

Checklist

I have read the CONTRIBUTING document
Local run of mvn install succeeds
My code follows the code style of this project
My change requires a change to the Javadoc documentation
I have updated the Javadoc documentation accordingly
[x]I have added tests to cover my changes
All new and existing tests passed
I have added a changelog entry. Adding a new entry must be accomplished by running the scripts/new-change script and following the instructions. Commit the new file created by the script in .changes/next-release with your changes.
My change is to implement 1.11 parity feature and I have updated LaunchChangelog

License

I confirm that this pull request can be released under the Apache 2 license

...ws/src/main/java/software/amazon/awssdk/http/auth/aws/internal/checksums/Crc32CChecksum.java

...aws/src/main/java/software/amazon/awssdk/http/auth/aws/internal/checksums/Crc32Checksum.java

...h-aws/src/main/java/software/amazon/awssdk/http/auth/aws/internal/checksums/Md5Checksum.java

...h-aws/src/main/java/software/amazon/awssdk/http/auth/aws/internal/checksums/SdkChecksum.java

...-aws/src/main/java/software/amazon/awssdk/http/auth/aws/internal/checksums/Sha1Checksum.java

...ws/src/main/java/software/amazon/awssdk/http/auth/aws/internal/checksums/Sha256Checksum.java

...oftware/amazon/awssdk/http/auth/aws/internal/checksums/factory/CrtBasedChecksumProvider.java

.../src/main/java/software/amazon/awssdk/http/auth/aws/internal/checksums/factory/SdkCrc32.java

...src/main/java/software/amazon/awssdk/http/auth/aws/internal/checksums/factory/SdkCrc32C.java

...aws/src/main/java/software/amazon/awssdk/http/auth/aws/internal/io/SdkDigestInputStream.java

...c/main/java/software/amazon/awssdk/http/auth/aws/internal/signer/PrecomputedChecksummer.java

core/http-auth-aws/src/main/java/software/amazon/awssdk/http/auth/aws/signer/Checksummer.java

...c/main/java/software/amazon/awssdk/http/auth/aws/internal/signer/DefaultAwsV4HttpSigner.java

core/http-auth-aws/src/main/java/software/amazon/awssdk/http/auth/aws/signer/Checksummer.java

...ws/src/main/java/software/amazon/awssdk/http/auth/aws/internal/checksums/Crc32CChecksum.java

...st/java/software/amazon/awssdk/http/auth/aws/internal/signer/DefaultAwsV4HttpSignerTest.java

...-auth-aws/src/main/java/software/amazon/awssdk/http/auth/aws/internal/util/ChecksumUtil.java

cenedhryn · 2023-08-31T23:39:02Z

...-auth-aws/src/main/java/software/amazon/awssdk/http/auth/aws/internal/util/ChecksumUtil.java

+import software.amazon.awssdk.http.auth.aws.internal.checksums.Sha256Checksum;
+
+@SdkInternalApi
+public final class ChecksumUtil {


Can we add unit tests for this class?

cenedhryn · 2023-08-31T23:53:33Z

core/http-auth-aws/src/main/java/software/amazon/awssdk/http/auth/aws/signer/Checksummer.java

     */
-    String checksum(ContentStreamProvider payload);
+    void checksum(ContentStreamProvider payload, SdkHttpRequest.Builder request);


Why did we make this change?

Before flexible checksums, the checksummer only ever returned the checksum as a value. Now, the checksummer has to be able to do multiple things (i.e. compute a checksum, add it to the request, compute another checksum, add it to the request somewhere else).

...c/main/java/software/amazon/awssdk/http/auth/aws/internal/signer/DefaultAwsV4HttpSigner.java

zoewangg · 2023-08-31T23:32:59Z

.../src/main/java/software/amazon/awssdk/http/auth/aws/internal/checksums/ConstantChecksum.java

+ * Implementation of {@link SdkChecksum} to provide a constant checksum.
+ */
+@SdkInternalApi
+public class ConstantChecksum implements SdkChecksum {


What's the use-case for this?

Touched upon here: #4376 (comment)

zoewangg · 2023-09-01T00:43:21Z

...-aws/src/main/java/software/amazon/awssdk/http/auth/aws/internal/io/ChecksumInputStream.java

+
+    private final Collection<Checksum> checksums = new ArrayList<>();
+
+    public ChecksumInputStream(InputStream stream, Collection<? extends Checksum> checksums) {


Is there any reason this takes a list of checksums? From the checksum spec, only one checksum algorithm is allowed on the request.

Yep! It takes multiple because multiple "checksums" are computed when a flexible checksum is specified in the signing request - the "flexible"checksum and the sha-256 content hash (or one of the pre-defined constant values, which is why we need the ConstantChecksum)

Do you have any use cases defined in tests for these variants?

Yep. The ChecksumInputStreamTest and FlexibleChecksummerTest should test multiple checksums being configured.

...h-aws/src/main/java/software/amazon/awssdk/http/auth/aws/internal/io/ChecksumSubscriber.java

...c/main/java/software/amazon/awssdk/http/auth/aws/internal/signer/DefaultAwsV4HttpSigner.java

zoewangg · 2023-09-01T00:57:12Z

...c/main/java/software/amazon/awssdk/http/auth/aws/internal/signer/DefaultAwsV4HttpSigner.java

+    private static boolean hasTrailer(SignRequest<?, ? extends AwsCredentialsIdentity> request) {
+        // Flexible checksums dictates adding a trailer when signing, but there may be existing trailers on
+        // the request (in the future)
+        return request.hasProperty(CHECKSUM_ALGORITHM) || request.request().firstMatchingHeader(X_AMZ_TRAILER).isPresent();


Why request.hasProperty(CHECKSUM_ALGORITHM) means it has trailer? Nit: can we move the comment to the javadoc? Comments sometimes clutter the code imo.

I can see how this would be confusing. Changing.

...ws/src/main/java/software/amazon/awssdk/http/auth/aws/internal/checksums/Crc32CChecksum.java

...aws/src/main/java/software/amazon/awssdk/http/auth/aws/internal/checksums/Crc32Checksum.java

...oftware/amazon/awssdk/http/auth/aws/internal/checksums/factory/CrtBasedChecksumProvider.java

zoewangg · 2023-09-01T16:43:06Z

.../src/test/java/software/amazon/awssdk/http/auth/aws/internal/io/ChecksumInputStreamTest.java

+public class ChecksumInputStreamTest {
+
+    @Test
+    public void test_computesCorrectSha256() {


nit, we don't really use "test" as part of test name since it's unnecessary. The patterns we've established are methodToTest_when_expectedBehavior or given_when_then

.../src/main/java/software/amazon/awssdk/http/auth/aws/internal/signer/FlexibleChecksummer.java

zoewangg · 2023-09-01T23:30:22Z

...s/src/main/java/software/amazon/awssdk/http/auth/aws/internal/signer/DefaultChecksummer.java

+ * The default implementation of a checksummer. By default, this will calculate the SHA256 checksum of a payload and add it as the
+ * value for the 'x-amz-content-sha256' header on the request.
+ */
+@SdkInternalApi


Do we need this class? Since technically, this is covered in FlexibleChecksummer by passing just SHA256

Nope, you're right, it can be removed.

zoewangg · 2023-09-01T23:32:40Z

...c/main/java/software/amazon/awssdk/http/auth/aws/internal/signer/PrecomputedChecksummer.java

+ * the cases where the checksum is a pre-defined value that dictates specific behavior by the signer, and flexible checksums is
+ * not enabled for the request (such as aws-chunked payload signing without trailers, unsigned streaming without trailers, etc.).
+ */
+@SdkInternalApi
 public final class PrecomputedChecksummer implements Checksummer {


Do we need this class? It seems like we can use FlexibleChecksummer and pass ConstantChecksumAlgorithm right?

This is an optimization, using this class means it doesn't need to read the stream like it does with Flexible.

Can we rename it to PrecomputedSha256Checksummer to make it more clear?

zoewangg · 2023-09-01T23:40:29Z

...-auth-aws/src/main/java/software/amazon/awssdk/http/auth/aws/internal/util/ChecksumUtil.java

+    /**
+     * Gets the SdkChecksum object based on the given ChecksumAlgorithm.
+     */
+    public static SdkChecksum fromChecksumAlgorithm(ChecksumAlgorithm checksumAlgorithm) {


We may want to create a map for this to make it easier to maintain and may be a micro performance improvement Map<ChecksumAlgorithm, Supplier<SdkChecksum>>

We can do this for the most, we can't for ConstantChecksum, since it takes a value as part of construction. Updating for this case.

.../src/main/java/software/amazon/awssdk/http/auth/aws/internal/signer/FlexibleChecksummer.java

zoewangg · 2023-09-05T19:32:59Z

...c/main/java/software/amazon/awssdk/http/auth/aws/internal/signer/PrecomputedChecksummer.java

+ * the cases where the checksum is a pre-defined value that dictates specific behavior by the signer, and flexible checksums is
+ * not enabled for the request (such as aws-chunked payload signing without trailers, unsigned streaming without trailers, etc.).
+ */
+@SdkInternalApi
 public final class PrecomputedChecksummer implements Checksummer {


Can we rename it to PrecomputedSha256Checksummer to make it more clear?

zoewangg · 2023-09-05T19:39:43Z

core/http-auth-aws/src/main/java/software/amazon/awssdk/http/auth/aws/signer/Checksummer.java

+     * Get a flexible checksummer that performs two checksums: the given checksum-algorithm and a precomputed checksum from the
+     * given checksum string.
+     */
+    static Checksummer create(String checksum, ChecksumAlgorithm checksumAlgorithm) {


Suggesting static Checksummer forFlexibleChecksum(String preComputedSha256, ChecksumAlgorithm checksumAlgorithm)

zoewangg · 2023-09-05T19:43:53Z

.../src/main/java/software/amazon/awssdk/http/auth/aws/internal/signer/FlexibleChecksummer.java

+ */
+@SdkInternalApi
+public final class FlexibleChecksummer implements Checksummer {
+    private final Map<String, SdkChecksum> checksumMap;


Nit: Map<SdkChecksum, String> seems to be more intuitive. Can we rename it to checksumToHeader or headerToChecksum if you want to keep it as is.

zoewangg · 2023-09-05T19:51:56Z

core/http-auth-aws/src/main/java/software/amazon/awssdk/http/auth/aws/signer/Checksummer.java

+     * given checksum string.
+     */
+    static Checksummer create(String checksum, ChecksumAlgorithm checksumAlgorithm) {
+        if (checksumAlgorithm != null) {


Can we require chekcsumAlgorithm to be not null

zoewangg · 2023-09-05T19:52:14Z

core/http-auth-aws/src/main/java/software/amazon/awssdk/http/auth/aws/signer/Checksummer.java

+    /**
+     * Get a precomputed checksummer that results the given checksum string.
+     */
+    static Checksummer create(String checksum) {


Suggesting static Checksummer forPrecomputed256Checksum(String preComputedSha256)

zoewangg · 2023-09-05T19:52:44Z

core/http-auth-aws/src/main/java/software/amazon/awssdk/http/auth/aws/signer/Checksummer.java

-     * Given a payload, calculate a checksum and return it as a string.
+     * Get a flexible checksummer that performs two checksums: the given checksum-algorithm and the default (sha256).
+     */
+    static Checksummer create(ChecksumAlgorithm checksumAlgorithm) {


Suggesting static Checksummer forFlexibleChecksum(ChecksumAlgorithm checksumAlgorithm) and requiring checksumAlgorithims to be not null.

zoewangg

Feel free to merge once require chekcsumAlgorithm thing is fixed.

sonarcloud · 2023-09-06T16:38:50Z

SonarCloud Quality Gate failed.

3 Bugs
0 Vulnerabilities
2 Security Hotspots
352 Code Smells

0.0% Coverage
6.2% Duplication