CLDSRV-848: add missing checksum algorithms by leif-scality · Pull Request #6081 · scality/cloudserver

leif-scality · 2026-02-17T21:49:45Z

Add CRC32, CRC32C, CRC64NVME, SHA1, SHA256 checksum support for non streaming methods (all methods except PutObject and PutPart).

bert-e · 2026-02-17T21:49:49Z

Hello leif-scality,

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 · 2026-02-17T21:49:55Z

Incorrect fix version

The Fix Version/s in issue CLDSRV-848 contains:

None

Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:

9.2.29
9.3.3

Please check the Fix Version/s of CLDSRV-848, or the target
branch of this pull request.

codecov · 2026-02-17T22:00:45Z

❌ 2 Tests Failed:

Tests completed	Failed	Passed	Skipped
8311	2	8309	0

View the full list of 2 ❄️ flaky test(s)

"after each" hook for "should put an object and set the acl via query param"::PUT object With v4 signature "after each" hook for "should put an object and set the acl via query param"

Flake rate in main: 100.00% (Passed 0 times, Failed 18 times)

Stack Traces | 0.021s run time

Expected values to be strictly equal:
+ actual - expected

+ '404 NOT FOUND'
- '200 OK'

"after each" hook for "should put an object and set the acl via query param"::PUT object With v4 signature "after each" hook for "should put an object and set the acl via query param"

Flake rate in main: 100.00% (Passed 0 times, Failed 18 times)

Stack Traces | 0.021s run time

done() called multiple times in hook <PUT object With v4 signature "after each" hook for "should put an object and set the acl via query param"> of file .../test/object/put.js

To view more test analytics, go to the Test Analytics Dashboard
_{📋 Got 3 mins? Take this short survey to help us improve Test Analytics.}

bert-e · 2026-02-18T17:30:28Z

Incorrect fix version

The Fix Version/s in issue CLDSRV-848 contains:

None

Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:

9.2.29
9.3.4

Please check the Fix Version/s of CLDSRV-848, or the target
branch of this pull request.

bert-e · 2026-02-23T19:15:12Z

Incorrect fix version

The Fix Version/s in issue CLDSRV-848 contains:

None

Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:

9.2.29
9.3.5

Please check the Fix Version/s of CLDSRV-848, or the target
branch of this pull request.

bert-e · 2026-02-25T09:22:27Z

Incorrect fix version

The Fix Version/s in issue CLDSRV-848 contains:

None

Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:

9.2.29
9.3.5
9.4.0

Please check the Fix Version/s of CLDSRV-848, or the target
branch of this pull request.

bert-e · 2026-02-25T09:23:05Z

Incorrect fix version

The Fix Version/s in issue CLDSRV-848 contains:

None

Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:

9.4.0

Please check the Fix Version/s of CLDSRV-848, or the target
branch of this pull request.

jonathan-gramain · 2026-02-26T01:19:45Z

lib/api/apiUtils/integrity/validateChecksums.js

 });

+const algorithms = {
+    'crc64nvme': {


Normally you can remove all the quotes from property names

jonathan-gramain · 2026-02-26T01:21:15Z

lib/api/apiUtils/integrity/validateChecksums.js

+            const result = await crc.digest();
+            return Buffer.from(result).toString('base64');
+        },
+        'checkExpected': expected => {


What about naming this callback isValidDigest (I understand you mean to check the "expected" digest, as if the digest is expected to match the contents' digest, but doesn't come obvious when reading the code, I think isValidDigest is more appropriate as it implies it returns true/false if the digest is a valid digest or not based on the expected type, without actually checking it against any content.

I don't know if it matters, but optionally we could also check that characters are valid within the expected encoding.

renamed to isValidDigest and added base64 regex test

jonathan-gramain · 2026-02-26T01:33:03Z

lib/api/apiUtils/integrity/validateChecksums.js

+
+async function validateXAmzChecksums(headers, body) {
+    const checksumHeaders = Object.keys(headers).filter(header => header.startsWith('x-amz-checksum-'));
+    const xAmzCheckumCnt = checksumHeaders.length;


Suggested change

const xAmzCheckumCnt = checksumHeaders.length;

const xAmzChecksumCnt = checksumHeaders.length;

jonathan-gramain · 2026-02-26T01:38:21Z

lib/api/apiUtils/integrity/validateChecksums.js

+        return { error: ChecksumError.AlgoNotSupported, details: { algorithm: algo } };
+    }
+
+    if (algo in algorithms === false) {


nitpick: it's more typical to avoid explicit boolean value checks

Suggested change

if (algo in algorithms === false) {

if (!(algo in algorithms)) {

jonathan-gramain · 2026-02-26T01:51:51Z

lib/api/apiUtils/integrity/validateChecksums.js

 const { config } = require('../../../Config');

 const ChecksumError = Object.freeze({
    MD5Mismatch: 'MD5Mismatch',


Instead of creating internal error types that are not used for any logic nor returned to the client, we could instead directly construct the error when we see it and return it as is with an appropriate descriptive error message (i.e. construct directly an error with InvalidRequest.customizeDescription(...)). We can keep the special errors that we need special handling for (MissingChecksum etc.)

In the unit tests, you can simply check that the error is InvalidRequest, and if you want to be fancy, also check a part of the error message.

The internal errors are have more details than the AWS errors(used for logging), they are also easier to test.
I prefer having the building of the AMZ error outside the validation function.

jonathan-gramain · 2026-02-26T01:53:25Z

lib/api/apiUtils/integrity/validateChecksums.js

-    return null;
+    log.debug('failed checksum validation', { method: request.apiMethod }, err);
+
+    switch (err.error) {


See my suggestion about returning directly errors to avoid having to do this conversion.

The internal errors are have more details than the AWS errors(used for logging), they are also easier to test.
I prefer having the building of the AMZ error outside the validation function.

jonathan-gramain · 2026-02-26T01:54:44Z

lib/api/apiUtils/integrity/validateChecksums.js

        const validationFunc = methodValidationFunc[request.apiMethod];
        if (!validationFunc) {
-            return null;
+            return null; //await defaultValidationFunc2(request, body, log);


Why adding this comment?

jonathan-gramain · 2026-02-26T02:00:06Z

tests/functional/raw-node/test/xAmzChecksum.js

+        );
+    });
+
+    itSkipIfAWS('should respond InvalidRequest with if invalid x-amz-checksum- algorithm', done => {


Suggested change

itSkipIfAWS('should respond InvalidRequest with if invalid x-amz-checksum- algorithm', done => {

itSkipIfAWS('should respond InvalidRequest with invalid x-amz-checksum- algorithm', done => {

jonathan-gramain · 2026-02-26T02:01:59Z

tests/functional/raw-node/test/xAmzChecksum.js

+                                data += chunk;
+                            });
+                            res.on('end', () => {
+                                assert(!data.includes('BadDigest'));


Could we more simply check that res.statusCode == 200?

The request is going to fail, because it is not a complete request, so we only check that it is not a BadDigest meaning that we passed the checksum checks.

I added missing asserts to cover the other errors (InvalidDigest, ...)

lib/api/apiUtils/integrity/validateChecksums.js

jonathan-gramain

Comments on error names, otherwise LGTM

jonathan-gramain · 2026-02-26T17:52:28Z

lib/api/apiUtils/integrity/validateChecksums.js

+    AlgoNotSupportedSDK: 'AlgoNotSupportedSDK',
+    MultipleChecksumTypes: 'MultipleChecksumTypes',
+    MissingCorresponding: 'MissingCorresponding',
+    InvalidAlgoValue: 'InvalidAlgoValue',


This error seems to be used not when the algorithm is invalid, but when the checksum is malformed. So I suggest to rename the error to MalformedChecksum.

jonathan-gramain · 2026-02-26T17:56:07Z

lib/api/apiUtils/integrity/validateChecksums.js

+    AlgoNotSupported: 'AlgoNotSupported',
+    AlgoNotSupportedSDK: 'AlgoNotSupportedSDK',
+    MultipleChecksumTypes: 'MultipleChecksumTypes',
+    MissingCorresponding: 'MissingCorresponding',


nitpick: rename to MissingExpectedChecksum?

I prefer MissingCorresponding because it points to this specific error x-amz-sdk-checksum-algorithm specified, but no corresponding x-amz-checksum-* or x-amz-trailer headers were found..

MissingExpectedChecksum is not as precise

…256] headers

…unctional tests

bert-e · 2026-02-27T16:04:22Z

Waiting for approval

The following approvals are needed before I can proceed with the merge:

the author
2 peers

leif-scality · 2026-02-27T16:44:48Z

/approve

bert-e · 2026-02-27T16:44:58Z

Build failed

The build for commit did not succeed in branch improvement/CLDSRV-848-add-missing-checksum-algorithms

The following options are set: approve

bert-e · 2026-02-27T17:38:42Z

Build failed

The build for commit did not succeed in branch improvement/CLDSRV-848-add-missing-checksum-algorithms

The following options are set: approve

bert-e · 2026-02-27T17:55:06Z

I have successfully merged the changeset of this pull request
into targetted development branches:

✔️ development/9.4

The following branches have NOT changed:

development/7.10
development/7.4
development/7.70
development/8.8
development/9.0
development/9.1
development/9.2
development/9.3

Please check the status of the associated issue CLDSRV-848.

Goodbye leif-scality.

The following options are set: approve

leif-scality force-pushed the improvement/CLDSRV-848-add-missing-checksum-algorithms branch from 5c6e2cd to 7498887 Compare February 17, 2026 21:56

leif-scality force-pushed the improvement/CLDSRV-848-add-missing-checksum-algorithms branch 2 times, most recently from e519604 to 12f1006 Compare February 18, 2026 17:17

leif-scality force-pushed the improvement/CLDSRV-848-add-missing-checksum-algorithms branch 5 times, most recently from 52c366f to 8493f53 Compare February 23, 2026 19:15

leif-scality force-pushed the improvement/CLDSRV-848-add-missing-checksum-algorithms branch from bdf8518 to 6833dde Compare February 24, 2026 18:37

leif-scality changed the title ~~Improvement/cldsrv 848 add missing checksum algorithms~~ CLDSRV-848: add missing checksum algorithms Feb 24, 2026

leif-scality marked this pull request as ready for review February 24, 2026 19:27

leif-scality force-pushed the improvement/CLDSRV-848-add-missing-checksum-algorithms branch from 59a4a0e to 5ed0d44 Compare February 25, 2026 09:22

leif-scality changed the base branch from development/9.2 to development/9.4 February 25, 2026 09:22

leif-scality force-pushed the improvement/CLDSRV-848-add-missing-checksum-algorithms branch from 5ed0d44 to ec2d446 Compare February 25, 2026 15:47

unholy0X approved these changes Feb 25, 2026

View reviewed changes

jonathan-gramain reviewed Feb 26, 2026

View reviewed changes

leif-scality force-pushed the improvement/CLDSRV-848-add-missing-checksum-algorithms branch 3 times, most recently from 1b5e6e0 to 26d6e14 Compare February 26, 2026 09:55

BourgoisMickael reviewed Feb 26, 2026

View reviewed changes

lib/api/apiUtils/integrity/validateChecksums.js Outdated Show resolved Hide resolved