All committers have signed the CLA.

Workflow [PR], commit [b9de102]

Summary: ✅

• Performance Comparison: Performance dashboard

AI Review

Summary

This PR adds s3_upload_checksum_algorithm, threads it through the upload paths, and special-cases S3Express/GCS. The remaining gap is the new GCS bypass: it avoids the header breakage, but it also sidesteps the runtime validation path when s3_validate_request_settings = 0, so invalid or unsupported explicit values can now be silently accepted on storage.googleapis.com uploads instead of being rejected.

PR Metadata

💡 Changelog category is correct (Improvement), but the current Changelog entry no longer matches the code for GCS endpoints: the setting is ignored there and the empty FIPS case does not switch to flexible SHA256. A closer replacement would be:
Added the s3_upload_checksum_algorithm setting to choose MD5, CRC32, or SHA256 for S3 uploads. CRC32 and SHA256 use AWS flexible x-amz-checksum-* headers, while MD5 uses the SDK Content-MD5 path. s3_disable_checksum suppresses upload checksums on non-S3Express buckets. In FIPS mode, the empty value switches to SHA256 for AWS S3/S3-compatible uploads; GCS endpoints keep their previous behavior because they do not accept AWS flexible checksum headers.

Findings

⚠️ Majors

• [src/IO/WriteBufferFromS3.cpp:512, src/IO/S3/copyS3File.cpp:98] The new GCS short-circuit returns nullopt before calling RequestChecksum::getUploadChecksumAlgorithm, which is now the runtime validation layer for s3_validate_request_settings = 0. That reopens the original validation hole for storage.googleapis.com: upload_checksum_algorithm = 'MD4' is silently ignored, and in FIPS mode even an explicit MD5 is no longer rejected on the upload path. Parse and validate the non-empty setting first, then decide whether GCS should ignore or reject flexible algorithms.

💡 Nits

• [src/Core/Settings.cpp:713] The public setting docs still describe the FIPS -> SHA256 fallback and explicit algorithm choice as universal, but the new isClientForGCS() branch makes GCS a documented exception. Please mention that here, and mirror the same caveat in the PR changelog entry.

Final Verdict

⚠️ Needs changes before merge: the GCS workaround fixed the header incompatibility, but it reintroduced a runtime-validation bypass for this setting.

hi @nickitat (apologies if you're not the best person to tag, I tried to look for a maintainer with s3 knowledge),

I was wondering if this change makes sense from your PoV? Or if you think something needs to be done differently?
I am still working on getting the build green, but wanted to check in with a maintainer first.

  For S3, the Content-MD5 header is computed by the bundled AWS SDK, which routes through aws-c-cal. On Linux that calls OpenSSL directly:

  // contrib/aws-c-cal/source/unix/opensslcrypto_hash.c:72
  if (!g_aws_openssl_evp_md_ctx_table->init_ex_fn(ctx, EVP_md5(), NULL)) {
      s_destroy(hash);
      aws_raise_error(AWS_ERROR_UNKNOWN);
      return NULL;
  }

  What FIPS-CH actually does on a normal bucket: silently sends no checksum

  The MD5 computation happens here:

  // contrib/aws/src/aws-cpp-sdk-core/source/client/AWSClient.cpp:902
  auto md5HashResult = Aws::Utils::Crypto::CreateMD5Implementation()->Calculate(*body);
  body->clear();
  if (md5HashResult.IsSuccess())   // <-- only set on success
      httpRequest->SetHeaderValue(Http::CONTENT_MD5_HEADER, HashingUtils::Base64Encode(md5HashResult.GetResult()));

  Under FIPS, aws_md5_default_new fails at init_ex_fn(ctx, EVP_md5(), NULL) (opensslcrypto_hash.c:72) and returns NULL. The CRT Hash wraps the null handle, ComputeOneShot
  fails, and CRTHash::Calculate returns false (CRTHash.cpp). So md5HashResult.IsSuccess() is false → the Content-MD5 header is simply omitted, no exception thrown.

It doesn't look right to silently skip checksumming if it is enabled. Also, it doesn't look right to deliberately fall back to CRC. So, I think yes, the change makes sense in general. I'd probably condition the default algorithm on isFIPSEnabled: for normal builds, use MD5 (we should have it as an option), and for FIPS, default to SHA.
I will take a closer look at the code later this week.

  For S3, the Content-MD5 header is computed by the bundled AWS SDK, which routes through aws-c-cal. On Linux that calls OpenSSL directly:

  // contrib/aws-c-cal/source/unix/opensslcrypto_hash.c:72
  if (!g_aws_openssl_evp_md_ctx_table->init_ex_fn(ctx, EVP_md5(), NULL)) {
      s_destroy(hash);
      aws_raise_error(AWS_ERROR_UNKNOWN);
      return NULL;
  }

  What FIPS-CH actually does on a normal bucket: silently sends no checksum

  The MD5 computation happens here:

  // contrib/aws/src/aws-cpp-sdk-core/source/client/AWSClient.cpp:902
  auto md5HashResult = Aws::Utils::Crypto::CreateMD5Implementation()->Calculate(*body);
  body->clear();
  if (md5HashResult.IsSuccess())   // <-- only set on success
      httpRequest->SetHeaderValue(Http::CONTENT_MD5_HEADER, HashingUtils::Base64Encode(md5HashResult.GetResult()));

  Under FIPS, aws_md5_default_new fails at init_ex_fn(ctx, EVP_md5(), NULL) (opensslcrypto_hash.c:72) and returns NULL. The CRT Hash wraps the null handle, ComputeOneShot
  fails, and CRTHash::Calculate returns false (CRTHash.cpp). So md5HashResult.IsSuccess() is false → the Content-MD5 header is simply omitted, no exception thrown.

It doesn't look right to silently skip checksumming if it is enabled. Also, it doesn't look right to deliberately fall back to CRC. So, I think yes, the change makes sense in general. I'd probably condition the default algorithm on isFIPSEnabled: for normal builds, use MD5 (we should have it as an option), and for FIPS, default to SHA. I will take a closer look at the code later this week.

Thanks for checking! I will make some more changes and add MD5/the FIPS default like you suggested (I didn't know isFIPSEnabled was a thing), and also get it AI-approved and green.

Also, a couple of cosmetic changes that I think would be better explained in code: https://pastila.clickhouse.com/?005b9765/c3a0323f6ce7f3726842098165b72996#zJQf6bFvxVcJvJTJx/7PBw==GCM

Unfortunately I can't access this page.

I did try to fix most of the other comments.

Also, a couple of cosmetic changes that I think would be better explained in code: pastila.clickhouse.com?005b9765/c3a0323f6ce7f3726842098165b72996#zJQf6bFvxVcJvJTJx/7PBw==GCM

Unfortunately I can't access this page.

I did try to fix most of the other comments.

Sorry, my bad. Another link: https://pastila.nl/?00385e67/7687f6486c8860490b4e4c48ff4c16cd#KZaixV40HSjsxKbFU9nFOg==GCM

LLVM Coverage Report

Changed lines: Changed C/C++ lines covered: 444/486 (91.36%) · Uncovered code

Full report · Diff report

@copilot resolve the merge conflicts in this pull request