Improvement/s3 c 9769/ignore trailing checksum #5751

fredmnl · 2025-02-26T21:26:53Z

Motivation

Since aws-cli 2.23 (and associated SDK version), put-object now defaults to using unsigned payload with trailing checksums for integrity.

This is particularly nice from a computational point of view because the client only needs to read their file once to perform the upload. With AuthV4 signed payload, you would need to first compute the checksum of the object, to eventually include it in the signature contained in the header. With trailing checksums (and unsigned payload), the checksum is computed while the upload is happening and the checksum is sent at the end of the body.

Implementation

In this first iteration we are only interested in accepting the incoming requests using trailing checksums and unsigned payload. We are not yet verifying the checksum although the current proposal is extensible to perform this checksum verification as well.

bert-e · 2025-02-26T21:26:56Z

Hello fredmnl,

My role is to assist you with the merge of this
pull request. Please type @bert-e help to get information
on this process, or consult the user documentation.

Available options

name	description	privileged	authored
`/after_pull_request`	Wait for the given pull request id to be merged before continuing with the current one.
`/bypass_author_approval`	Bypass the pull request author's approval	⭐
`/bypass_build_status`	Bypass the build and test status	⭐
`/bypass_commit_size`	Bypass the check on the size of the changeset `TBA`	⭐
`/bypass_incompatible_branch`	Bypass the check on the source branch prefix	⭐
`/bypass_jira_check`	Bypass the Jira issue check	⭐
`/bypass_peer_approval`	Bypass the pull request peers' approval	⭐
`/bypass_leader_approval`	Bypass the pull request leaders' approval	⭐
`/approve`	Instruct Bert-E that the author has approved the pull request.		✍️
`/create_pull_requests`	Allow the creation of integration pull requests.
`/create_integration_branches`	Allow the creation of integration branches.
`/no_octopus`	Prevent Wall-E from doing any octopus merge and use multiple consecutive merge instead
`/unanimity`	Change review acceptance criteria from `one reviewer at least` to `all reviewers`
`/wait`	Instruct Bert-E not to run until further notice.

Available commands

name	description	privileged
`/help`	Print Bert-E's manual in the pull request.
`/status`	Print Bert-E's current status in the pull request `TBA`
`/clear`	Remove all comments from Bert-E from the history `TBA`
`/retry`	Re-start a fresh build `TBA`
`/build`	Re-start a fresh build `TBA`
`/force_reset`	Delete integration branches & pull requests, and restart merge process from the beginning.
`/reset`	Try to remove integration branches unless there are commits on them which do not appear on the source branch.

Status report is not available.

bert-e · 2025-02-26T21:27:06Z

Incorrect Jira project

The Jira issue S3C-9769 specified in the source
branch name, does not belong to project CLDSRV.

lib/auth/streamingV4/trailingChecksumTransform.js

fredmnl · 2025-03-03T21:24:17Z

@jonathan-gramain Thanks again for the review, I reworked the whole class to address your concerns.

I thought that my tests were covering all of the tricky splits that could happen but actually the http request module used was squashing the chunks together before sending. I resorted to coding a unit tests for the class which is much neater. I might have overdone it but they run pretty quickly so we could keep them all. 2487 passing (389ms)

I reworked the logic in a way which I think is much neater. I'm only detecting \r instead of \r\n, which can't be split across chunks. I'm not actually checking for the '\n', but I think that's acceptable as the number of bytes cannot contain \r anyway. (We have never checked the \r\n after data for example). I'm effectively looking for regex [^\r]+\r..{number_of_bytes}.. until 0\r is found.

I think that eventually we will want to refactor V4Transform to integrate this as well as checksum checking.

S3C-9776-repro.txt

conf/lab-ca-csr.cnf

lib/auth/streamingV4/trailingChecksumTransform.js

jonathan-gramain · 2025-03-03T23:16:06Z

lib/auth/streamingV4/trailingChecksumTransform.js

+                continue;
+            }
+
+            const lineBreakIndex2 = chunk.indexOf('\r');


It looks functional to me, I still have a slight preference for finding the full sequence \r\n in the chunkSizeBuffer because it also validates the stream contents more strongly. But it's not blocking for me, I'm fine with this approach too.

I think I'd like to refactor both Transforms (the AuthV4 too) into a single processor that would actually work on a configurable FSM to both validate and process the stream. I think it would also make the V4Transform much more readable. With that in mind, we could deliver this iteration as is (we're aiming for a timely release on this fix). There will at some point be a further iteration to validate the checksums (prioritization aside)

tests/unit/auth/TrailingChecksumTransform.js

fredmnl · 2025-03-05T07:50:34Z

@jonathan-gramain I think we're ready for another review here

jonathan-gramain

LGTM with some minor suggestions

jonathan-gramain · 2025-03-06T18:19:48Z

lib/auth/streamingV4/trailingChecksumTransform.js

+            const lineBreakIndex = chunk.indexOf('\r');
+            if (lineBreakIndex === -1) {
+                if (this.chunkSizeBuffer.byteLength + chunk.byteLength > 10) {
+                    this.log.debug('chunk size field too big', {


Two suggestions:

Turn into info level, as it can be useful for debugging on production platforms in case their app sends chunks that are too big for us to support

You could do first:

this.chunkSizeBuffer = Buffer.concat([this.chunkSizeBuffer, chunk]);

then check that the size is below what we support.

Then you can log directly this.chunkSizeBuffer.subarray(0, 16).toString():

You can remove the hex encoding since it's already JSON-encoded in logs

I think 16 chars allow to see the full size, while 8 chars may mask the least significant digits of the hex-encoded length

(Also see comment below, which I wrote first)

5GB is an AWS limit, it's forbidden to perform a single upload of more than 5GB hence the hard specification limit here, and 5GB needs 9 hex characters to be written, we give it an extra character for \r.

this.chunkSizeBuffer = Buffer.concat([this.chunkSizeBuffer, chunk]);

could potentially make us perform a huge buffer concatenation (although I think that Node would rechunk things before reaching this part of the code)

👍 on hex

👍 Yes 16 is better, in case the chunkSizeBuffer is empty, we would not see the whole 10 characters that we could have been interested in.

jonathan-gramain · 2025-03-06T18:27:37Z

lib/auth/streamingV4/trailingChecksumTransform.js

+            }
+            const dataSize = Number.parseInt(this.chunkSizeBuffer.toString(), 16);
+            if (Number.isNaN(dataSize)) {
+                this.log.debug('unable to parse chunk size', {


This case now looks unlikely due to the sanity check, so I think you can log it with error level

Sounds good, will do 👍

lib/auth/streamingV4/trailingChecksumTransform.js

tests/unit/auth/TrailingChecksumTransform.js

fredmnl · 2025-03-07T08:48:22Z

/approve

bert-e · 2025-03-07T08:48:31Z

Incorrect Jira project

The Jira issue S3C-9769 specified in the source
branch name, does not belong to project CLDSRV.

The following options are set: approve

williamlardier

First review iteration, I will need to complete my review of the TrailingChecksumTransform class

williamlardier · 2025-03-07T14:40:17Z

lib/api/apiUtils/object/prepareStream.js

+    if (stream.headers['x-amz-trailer'] === undefined ||
+        stream.headers['x-amz-trailer'] === '') {
+        return stream;


We can just check if the value is set (both cases would return true here, and I believe we want to handle null the same way)

Suggested change

if (stream.headers['x-amz-trailer'] === undefined ||

stream.headers['x-amz-trailer'] === '') {

return stream;

if (!stream.headers['x-amz-trailer']) {

return stream;

williamlardier · 2025-03-07T14:44:07Z

lib/api/apiUtils/object/validateChecksumHeaders.js

+
+    if (headers['x-amz-trailer'] !== undefined &&
+        headers['x-amz-content-sha256'] !== 'STREAMING-UNSIGNED-PAYLOAD-TRAILER') {
+        return errors.BadRequest.customizeDescription('signed trailing checksum is not supported');


usually BadRequest (error 400) is for errors from client side, here, I would instead return a NotImplemented (error code 501)

lib/auth/streamingV4/trailingChecksumTransform.js

williamlardier · 2025-03-07T14:51:29Z

lib/auth/streamingV4/trailingChecksumTransform.js

+    constructor(log, errCb) {
+        super({});
+        this.log = log;
+        this.errCb = errCb;


We store this error callback in the class (which is a bit strange as passing such error function as a member of a class makes it complex to know when this will be called, and the impacts on the call path for the API).

But the main point is that this callback is not used anywhere: either we expect no error and we should remove this callback, or we can have error (handling of the Transform events?) and we need to use it somehow...

williamlardier · 2025-03-07T14:53:56Z

lib/api/apiUtils/object/prepareStream.js

+        return stream;
+    }
+
+    const trailingChecksumTransform = new TrailingChecksumTransform(log, errCb);


We should not pass a callback for error handling here: we just pipe two streams, and then we can handle the errors in the same place as before.

Note: piping streams, in the past, led to memory leaks. We must ensure all errors /events are properly handled not to miss something here. See here for an example.

williamlardier · 2025-03-07T14:57:20Z

lib/auth/streamingV4/trailingChecksumTransform.js

+                this.log.info('chunk size is not a valid hex number', {
+                    chunkSizeBuffer: this.chunkSizeBuffer.toString(),
+                });


Suggested change

this.log.info('chunk size is not a valid hex number', {

chunkSizeBuffer: this.chunkSizeBuffer.toString(),

});

this.log.error('chunk size is not a valid hex number', {

chunkSizeBuffer: this.chunkSizeBuffer.toString(),

});

williamlardier · 2025-03-07T14:57:31Z

lib/auth/streamingV4/trailingChecksumTransform.js

+                    this.log.info('chunk size field too big', {
+                        chunkSizeBuffer: this.chunkSizeBuffer.toString(),
+                        truncatedChunk: chunk.subarray(0, 16).toString(),


Suggested change

this.log.info('chunk size field too big', {

chunkSizeBuffer: this.chunkSizeBuffer.toString(),

truncatedChunk: chunk.subarray(0, 16).toString(),

this.log.error('chunk size field too big', {

chunkSizeBuffer: this.chunkSizeBuffer.toString(),

truncatedChunk: chunk.subarray(0, 16).toString(),

williamlardier · 2025-03-07T15:57:13Z

lib/auth/streamingV4/trailingChecksumTransform.js

+            }
+
+            this.chunkSizeBuffer = Buffer.concat([this.chunkSizeBuffer, chunk.subarray(0, lineBreakIndex)]);
+            chunk = chunk.subarray(lineBreakIndex);


I'm not sure this is correct (I may be wrong)
The delimiter is \r, but don't we have \n\r at the end of each chunk?

In such case, should we do something like

if (lineBreakIndex === -1 || lineBreakIndex + 1 >= chunk.length || chunk[lineBreakIndex + 1] !== '\n'.charCodeAt(0)) { // Handle error or wait for more data } this.chunkSizeBuffer = Buffer.concat([this.chunkSizeBuffer, chunk.subarray(0, lineBreakIndex)]); chunk = chunk.subarray(lineBreakIndex + 2);

fredmnl · 2025-03-17T10:12:57Z

Closing this PR in favor of #5757

fredmnl force-pushed the improvement/S3C-9769/ignore-trailing-checksum branch from 76addf8 to bc68eb3 Compare February 27, 2025 18:42

jonathan-gramain requested changes Feb 27, 2025

View reviewed changes

jonathan-gramain reviewed Mar 3, 2025

View reviewed changes

fredmnl force-pushed the improvement/S3C-9769/ignore-trailing-checksum branch from c34a49d to dba1a83 Compare March 4, 2025 14:46

fredmnl marked this pull request as ready for review March 4, 2025 17:38

fredmnl requested a review from jonathan-gramain March 5, 2025 07:46

fredmnl force-pushed the improvement/S3C-9769/ignore-trailing-checksum branch from dba1a83 to 7862de5 Compare March 5, 2025 07:49

tcarmet approved these changes Mar 6, 2025

View reviewed changes

jonathan-gramain approved these changes Mar 6, 2025

View reviewed changes

fredmnl force-pushed the improvement/S3C-9769/ignore-trailing-checksum branch from f8c4593 to 20011c1 Compare March 7, 2025 08:35

S3C-9769: Ignore trailing checksums in upload requests

7b413ba

fredmnl force-pushed the improvement/S3C-9769/ignore-trailing-checksum branch from 20011c1 to 7b413ba Compare March 7, 2025 08:36

francoisferrand requested a review from williamlardier March 7, 2025 13:18

williamlardier reviewed Mar 7, 2025

View reviewed changes

fredmnl closed this Mar 17, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improvement/s3 c 9769/ignore trailing checksum #5751

Improvement/s3 c 9769/ignore trailing checksum #5751

fredmnl commented Feb 26, 2025 •

edited

Loading

bert-e commented Feb 26, 2025

bert-e commented Feb 26, 2025

fredmnl commented Mar 3, 2025

jonathan-gramain Mar 3, 2025

fredmnl Mar 4, 2025

fredmnl commented Mar 5, 2025

jonathan-gramain left a comment

jonathan-gramain Mar 6, 2025

fredmnl Mar 7, 2025

jonathan-gramain Mar 6, 2025

fredmnl Mar 7, 2025

fredmnl commented Mar 7, 2025

bert-e commented Mar 7, 2025

williamlardier left a comment

williamlardier Mar 7, 2025

williamlardier Mar 7, 2025

williamlardier Mar 7, 2025

williamlardier Mar 7, 2025

williamlardier Mar 7, 2025

williamlardier Mar 7, 2025

williamlardier Mar 7, 2025

fredmnl commented Mar 17, 2025

Improvement/s3 c 9769/ignore trailing checksum #5751

Improvement/s3 c 9769/ignore trailing checksum #5751

Conversation

fredmnl commented Feb 26, 2025 • edited Loading

Motivation

Implementation

bert-e commented Feb 26, 2025

Hello fredmnl,

bert-e commented Feb 26, 2025

Incorrect Jira project

fredmnl commented Mar 3, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fredmnl commented Mar 5, 2025

jonathan-gramain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fredmnl commented Mar 7, 2025

bert-e commented Mar 7, 2025

Incorrect Jira project

williamlardier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fredmnl commented Mar 17, 2025

fredmnl commented Feb 26, 2025 •

edited

Loading