CLDSRV-620: Ignore trailing checksums in upload requests #5757
Conversation
Hello fredmnl,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
Request integration branchesWaiting for integration branch creation to be requested by the user. To request integration branches, please comment on this pull request with the following command: Alternatively, the |
|
/create_integration_branches |
Integration data createdI have created the integration data for the additional destination branches.
The following branches will NOT be impacted:
You can set option The following options are set: create_integration_branches |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
The following options are set: create_integration_branches |
fredmnl
force-pushed
the
bugfix/CLDSRV-620/strip-trailing-checksums
branch
from
e324d8a to
a9afd5f
Compare
History mismatchMerge commit #e324d8a72894b218324c9f640f175ff54d06cbb0 on the integration branch It is likely due to a rebase of the branch Please use the The following options are set: create_integration_branches |
|
/reset |
Reset completeI have successfully deleted this pull request's integration branches. The following options are set: create_integration_branches |
Integration data createdI have created the integration data for the additional destination branches.
The following branches will NOT be impacted:
You can set option The following options are set: create_integration_branches |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
The following options are set: create_integration_branches |
ConflictA conflict has been raised during the update of Please resolve the conflict on the integration branch ( Here are the steps to resolve this conflict: $ git fetch
$ git checkout w/7.70/bugfix/CLDSRV-620/strip-trailing-checksums
$ git pull # or "git reset --hard origin/w/7.70/bugfix/CLDSRV-620/strip-trailing-checksums"
$ git merge origin/development/7.70
$ # <intense conflict resolution>
$ git commit
$ git merge origin/bugfix/CLDSRV-620/strip-trailing-checksums
$ # <intense conflict resolution>
$ git commit
$ git push -u origin w/7.70/bugfix/CLDSRV-620/strip-trailing-checksumsThe following options are set: create_integration_branches |
fredmnl
changed the title
ConflictA conflict has been raised during the update of Please resolve the conflict on the integration branch ( Here are the steps to resolve this conflict: $ git fetch
$ git checkout w/8.8/bugfix/CLDSRV-620/strip-trailing-checksums
$ git pull # or "git reset --hard origin/w/8.8/bugfix/CLDSRV-620/strip-trailing-checksums"
$ git merge origin/development/8.8
$ # <intense conflict resolution>
$ git commit
$ git merge origin/w/7.70/bugfix/CLDSRV-620/strip-trailing-checksums
$ # <intense conflict resolution>
$ git commit
$ git push -u origin w/8.8/bugfix/CLDSRV-620/strip-trailing-checksumsThe following options are set: create_integration_branches |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
The following options are set: create_integration_branches |
| const trailingChecksumTransform = new TrailingChecksumTransform(log); | ||
| stream.pipe(trailingChecksumTransform); | ||
| trailingChecksumTransform.headers = stream.headers; | ||
| return trailingChecksumTransform; |
There was a problem hiding this comment.
Good to confirm as well if we are still enforcing the common md5 check in this mode (both stored in the object md and that we properly fail in case of mismatch). Could be through a new unit/ft test.
There was a problem hiding this comment.
So I checked and when the trailing checksum mode is activated, the client does not send md5 checksums. Therefore in this mode we do not check integrity. This is not a first, there are already ways to upload where cloudserver does not check payload integrity. In fact, it only checks payload integrity in two scenarios: content-md5 header is provided, or streaming AuthV4 is used (in this case, the signature verification also indirectly verifies payload integrity).
In the absence of integrity check in cloudserver, integrity is still verified at the TCP level (although it's quite a weak verification) and at the HTTPS level.
I have circled back with David to triple check that this is what we want from a product standpoint.
There was a problem hiding this comment.
I see, if we all align, good to mention it in the release notes: integrity is not guaranteed by the S3 service with trailing checksums PUTs.
| const trailingChecksumTransform = new TrailingChecksumTransform(log); | ||
| stream.pipe(trailingChecksumTransform); | ||
| trailingChecksumTransform.headers = stream.headers; | ||
| return trailingChecksumTransform; |
There was a problem hiding this comment.
I see, if we all align, good to mention it in the release notes: integrity is not guaranteed by the S3 service with trailing checksums PUTs.
| // no need to look further than 10 bytes since the field cannot be bigger | ||
| const lineBreakIndex = chunk.subarray(0, 10).indexOf('\r'); | ||
| const bytesToKeep = lineBreakIndex === -1 ? chunk.byteLength : lineBreakIndex; | ||
| if (this.chunkSizeBuffer.byteLength + bytesToKeep > 10) { |
There was a problem hiding this comment.
Should we not only check the number of bytes in the buffer, but also the actual value, and ensure we don't exceed the max supported (5GB)? See constants.maximumAllowedPartSize for the constant you can use.
fredmnl
force-pushed
the
bugfix/CLDSRV-620/strip-trailing-checksums
branch
from
307973e to
d04e248
Compare
History mismatchMerge commit #337ebf7afb214667e813a33db351a8e90a11c35a on the integration branch It is likely due to a rebase of the branch Please use the The following options are set: create_integration_branches |
fredmnl
force-pushed
the
bugfix/CLDSRV-620/strip-trailing-checksums
branch
from
26d52a8 to
4714a4a
Compare
History mismatchMerge commit #a9afd5fa7483e73aa38fa7903a772b7565b978e2 on the integration branch It is likely due to a rebase of the branch Please use the The following options are set: create_integration_branches |
|
/force_reset |
Reset completeI have successfully deleted this pull request's integration branches. The following options are set: create_integration_branches |
ConflictA conflict has been raised during the creation of I have not created the integration branch. Here are the steps to resolve this conflict: $ git fetch
$ git checkout -B w/7.70/bugfix/CLDSRV-620/strip-trailing-checksums origin/development/7.70
$ git merge origin/bugfix/CLDSRV-620/strip-trailing-checksums
$ # <intense conflict resolution>
$ git commit
$ git push -u origin w/7.70/bugfix/CLDSRV-620/strip-trailing-checksumsThe following options are set: create_integration_branches |
ConflictA conflict has been raised during the creation of I have not created the integration branch. Here are the steps to resolve this conflict: $ git fetch
$ git checkout -B w/8.8/bugfix/CLDSRV-620/strip-trailing-checksums origin/development/8.8
$ git merge origin/w/7.70/bugfix/CLDSRV-620/strip-trailing-checksums
$ # <intense conflict resolution>
$ git commit
$ git push -u origin w/8.8/bugfix/CLDSRV-620/strip-trailing-checksumsThe following options are set: create_integration_branches |
ConflictA conflict has been raised during the creation of I have not created the integration branch. Here are the steps to resolve this conflict: $ git fetch
$ git checkout -B w/9.0/bugfix/CLDSRV-620/strip-trailing-checksums origin/development/9.0
$ git merge origin/w/8.8/bugfix/CLDSRV-620/strip-trailing-checksums
$ # <intense conflict resolution>
$ git commit
$ git push -u origin w/9.0/bugfix/CLDSRV-620/strip-trailing-checksumsThe following options are set: create_integration_branches |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
The following options are set: create_integration_branches |
|
/approve |
Build failedThe build for commit did not succeed in branch w/9.0/bugfix/CLDSRV-620/strip-trailing-checksums The following options are set: approve, create_integration_branches |
Build failedThe build for commit did not succeed in branch w/8.8/bugfix/CLDSRV-620/strip-trailing-checksums The following options are set: approve, create_integration_branches |
fredmnl
force-pushed
the
bugfix/CLDSRV-620/strip-trailing-checksums
branch
from
4714a4a to
7ba8551
Compare
History mismatchMerge commit #bd46a9a53d8241f0ff9380ea131b239d288edf77 on the integration branch It is likely due to a rebase of the branch Please use the The following options are set: approve, create_integration_branches |
|
/force_reset |
Reset completeI have successfully deleted this pull request's integration branches. The following options are set: approve, create_integration_branches |
ConflictA conflict has been raised during the creation of I have not created the integration branch. Here are the steps to resolve this conflict: $ git fetch
$ git checkout -B w/7.70/bugfix/CLDSRV-620/strip-trailing-checksums origin/development/7.70
$ git merge origin/bugfix/CLDSRV-620/strip-trailing-checksums
$ # <intense conflict resolution>
$ git commit
$ git push -u origin w/7.70/bugfix/CLDSRV-620/strip-trailing-checksumsThe following options are set: approve, create_integration_branches |
ConflictA conflict has been raised during the creation of I have not created the integration branch. Here are the steps to resolve this conflict: $ git fetch
$ git checkout -B w/8.8/bugfix/CLDSRV-620/strip-trailing-checksums origin/development/8.8
$ git merge origin/w/7.70/bugfix/CLDSRV-620/strip-trailing-checksums
$ # <intense conflict resolution>
$ git commit
$ git push -u origin w/8.8/bugfix/CLDSRV-620/strip-trailing-checksumsThe following options are set: approve, create_integration_branches |
ConflictA conflict has been raised during the creation of I have not created the integration branch. Here are the steps to resolve this conflict: $ git fetch
$ git checkout -B w/9.0/bugfix/CLDSRV-620/strip-trailing-checksums origin/development/9.0
$ git merge origin/w/8.8/bugfix/CLDSRV-620/strip-trailing-checksums
$ # <intense conflict resolution>
$ git commit
$ git push -u origin w/9.0/bugfix/CLDSRV-620/strip-trailing-checksumsThe following options are set: approve, create_integration_branches |
Build failedThe build for commit did not succeed in branch w/9.0/bugfix/CLDSRV-620/strip-trailing-checksums The following options are set: approve, create_integration_branches |
|
I have successfully merged the changeset of this pull request
The following branches have NOT changed:
Please check the status of the associated issue CLDSRV-620. Goodbye fredmnl. The following options are set: approve, create_integration_branches |
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.