db/pgbackrest
1
0
Fork 0
mirror of https://github.com/pgbackrest/pgbackrest.git synced 2025-10-30 23:37:45 +02:00
Code Issues Releases Activity

Revert "calculate content-md5 on S3 only when required".

Browse Source 
20bfd14 removed content-md5 where allowed by the specification but failed to notice that either content-md5 or x-amz-content-* is required for PUT when object lock is enabled.

On top of that it appears Scality S3 (at least?) won't accept alternate content checksums when object lock is enabled. Technically this is a violation of the specification but nonetheless the change breaks working installations.

For now it seems safer to revert this change and pursue a better solution for a future feature release.
This commit is contained in:
David Steele
2025-04-29 15:12:11 -04:00
committed by GitHub
parent 9958b17834
commit 96dfdce085
6 changed files with 31 additions and 20 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
doc/resource/git-history.cache
View File

@@ -85,7 +85,7 @@
"commit": "20bfd14b73ad6d75b6fb1169565b18dcfa183c9b",
"date": "2025-04-09 12:27:27 -0500",
"subject": "Calculate content-md5 on S3 only when required.",
"body": "The content-md5 header was generated for all requests with content but it is only required for batch delete requests. It is not clear why this header is required when x-amz-content-sha256 is also provided or why it\nis required only for this request but the documentation is clear on the matter. However, the content for these requests is relatively small compared to uploading files so omitting content-md5 where possible will\nsave some CPU cycles.\n\nCurrent AWS S3 and recent Minio don't complain if this header is missing but since it is still required by older versions of Minio and it is specified in the documentation for batch delete it is makes sense to\nkeep it."
"body": "The content-md5 header was generated for all requests with content but it is only required for batch delete requests. It is not clear why this header is required when x-amz-content-sha256 is also provided or why it is required only for this request but the documentation is clear on the matter. However, the content for these requests is relatively small compared to uploading files so omitting content-md5 where possible will save some CPU cycles.\n\nCurrent AWS S3 and recent Minio don't complain if this header is missing but since it is still required by older versions of Minio and it is specified in the documentation for batch delete it is makes sense to keep it."
},
{
"commit": "c925832e1737f1b5cac347c482a725a87f2237cb",

13
doc/xml/release/2025/2.56.0.xml
View File

@@ -1,6 +1,19 @@
<release date="XXXX-XX-XX" version="2.56.0dev" title="UNDER DEVELOPMENT">
<release-core-list>
<release-bug-list>
<release-item>
<github-issue id="2613"/>
<github-pull-request id="2615"/>
<release-item-contributor-list>
<release-item-ideator id="frank.brendel"/>
<release-item-contributor id="david.steele"/>
<release-item-reviewer id="david.christensen"/>
</release-item-contributor-list>
<p>Revert <quote>calculate content-md5 on S3 only when required</quote>.</p>
</release-item>
<release-item>
<github-issue id="2610"/>
<github-pull-request id="2611"/>

5
doc/xml/release/contributor.xml
View File

@@ -388,6 +388,11 @@
<contributor-id type="github">fmbiete</contributor-id>
</contributor>
<contributor id="frank.brendel">
<contributor-name-display>Frank Brendel</contributor-name-display>
<contributor-id type="github">fbrendel</contributor-id>
</contributor>
<contributor id="floris.van.nee">
<contributor-name-display>Floris van Nee</contributor-name-display>
<contributor-id type="github">fvannee</contributor-id>

15
src/storage/s3/storage.c
View File

@@ -452,7 +452,6 @@ storageS3RequestAsync(StorageS3 *const this, const String *const verb, const Str
FUNCTION_LOG_PARAM(HTTP_HEADER, param.header);
FUNCTION_LOG_PARAM(HTTP_QUERY, param.query);
FUNCTION_LOG_PARAM(BUFFER, param.content);
FUNCTION_LOG_PARAM(BOOL, param.contentMd5);
FUNCTION_LOG_PARAM(BOOL, param.sseKms);
FUNCTION_LOG_PARAM(BOOL, param.sseC);
FUNCTION_LOG_PARAM(BOOL, param.tag);
@@ -474,11 +473,9 @@ storageS3RequestAsync(StorageS3 *const this, const String *const verb, const Str
requestHeader, HTTP_HEADER_CONTENT_LENGTH_STR,
param.content == NULL || bufEmpty(param.content) ? ZERO_STR : strNewFmt("%zu", bufUsed(param.content)));
// Calculate content-md5 header when required
if (param.contentMd5)
// Calculate content-md5 header if there is content
if (param.content != NULL)
{
ASSERT(param.content != NULL && !bufEmpty(param.content));
httpHeaderAdd(
requestHeader, HTTP_HEADER_CONTENT_MD5_STR,
strNewEncode(encodingBase64, cryptoHashOne(hashTypeMd5, param.content)));
@@ -610,7 +607,6 @@ storageS3Request(StorageS3 *const this, const String *const verb, const String *
FUNCTION_LOG_PARAM(HTTP_HEADER, param.header);
FUNCTION_LOG_PARAM(HTTP_QUERY, param.query);
FUNCTION_LOG_PARAM(BUFFER, param.content);
FUNCTION_LOG_PARAM(BOOL, param.contentMd5);
FUNCTION_LOG_PARAM(BOOL, param.allowMissing);
FUNCTION_LOG_PARAM(BOOL, param.contentIo);
FUNCTION_LOG_PARAM(BOOL, param.sseKms);
@@ -619,8 +615,8 @@ storageS3Request(StorageS3 *const this, const String *const verb, const String *
FUNCTION_LOG_END();
HttpRequest *const request = storageS3RequestAsyncP(
this, verb, path, .header = param.header, .query = param.query, .content = param.content, .contentMd5 = param.contentMd5,
.sseKms = param.sseKms, .sseC = param.sseC, .tag = param.tag);
this, verb, path, .header = param.header, .query = param.query, .content = param.content, .sseKms = param.sseKms,
.sseC = param.sseC, .tag = param.tag);
HttpResponse *const result = storageS3ResponseP(
request, .allowMissing = param.allowMissing, .contentIo = param.contentIo);
@@ -1045,8 +1041,7 @@ storageS3PathRemoveInternal(StorageS3 *const this, HttpRequest *const request, X
HttpQuery *const query = httpQueryAdd(httpQueryNewP(), S3_QUERY_DELETE_STR, EMPTY_STR);
Buffer *const content = xmlDocumentBuf(xml);
result = storageS3RequestAsyncP(
this, HTTP_VERB_POST_STR, FSLASH_STR, .query = query, .content = content, .contentMd5 = true);
result = storageS3RequestAsyncP(this, HTTP_VERB_POST_STR, FSLASH_STR, .query = query, .content = content);
httpQueryFree(query);
bufFree(content);

2
src/storage/s3/storage.intern.h
View File

@@ -22,7 +22,6 @@ typedef struct StorageS3RequestAsyncParam
const HttpHeader *header; // Headers
const HttpQuery *query; // Query parameters
const Buffer *content; // Request content
bool contentMd5; // MD5 content checksum required?
bool sseKms; // Enable server-side encryption?
bool sseC; // Enable server-side encryption with customer-provided keys?
bool tag; // Add tags when available?
@@ -53,7 +52,6 @@ typedef struct StorageS3RequestParam
const HttpHeader *header; // Headers
const HttpQuery *query; // Query parameters
const Buffer *content; // Request content
bool contentMd5; // MD5 content checksum required?
bool allowMissing; // Allow missing files (caller can check response code)
bool contentIo; // Is IoRead interface required to read content?
bool sseKms; // Enable server-side encryption?

14
test/src/module/storage/s3Test.c
View File

@@ -34,7 +34,6 @@ typedef struct TestRequestParam
const char *token;
const char *tag;
bool requesterPays;
bool contentMd5;
} TestRequestParam;
#define testRequestP(write, s3, verb, path, ...) \
@@ -69,7 +68,7 @@ testRequest(IoWrite *write, Storage *s3, const char *verb, const char *path, Tes
"authorization:AWS4-HMAC-SHA256 Credential=%s/\?\?\?\?\?\?\?\?/us-east-1/s3/aws4_request,SignedHeaders=",
param.accessKey == NULL ? strZ(driver->accessKey) : param.accessKey);
if (param.contentMd5)
if (param.content != NULL)
strCatZ(request, "content-md5;");
strCatZ(request, "host;");
@@ -106,7 +105,7 @@ testRequest(IoWrite *write, Storage *s3, const char *verb, const char *path, Tes
strCatFmt(request, "content-length:%zu\r\n", param.content != NULL ? strlen(param.content) : 0);
// Add md5
if (param.contentMd5)
if (param.content != NULL)
{
strCatFmt(
request, "content-md5:%s\r\n", strZ(strNewEncode(encodingBase64, cryptoHashOne(hashTypeMd5, BUFSTRZ(param.content)))));
@@ -870,6 +869,7 @@ testRun(void)
"*** Request Headers ***:\n"
"authorization: <redacted>\n"
"content-length: 205\n"
"content-md5: 37smUM6Ah2/EjZbp420dPw==\n"
"host: bucket.s3.amazonaws.com\n"
"x-amz-content-sha256: 0838a79dfbddc2128d28fb4fa8d605e0a8e6d1355094000f39b6eb3feff4641f\n"
"x-amz-date: <redacted>\n"
@@ -1317,7 +1317,7 @@ testRun(void)
"</ListBucketResult>");
testRequestP(
service, s3, HTTP_VERB_POST, "/bucket/?delete=", .contentMd5 = true,
service, s3, HTTP_VERB_POST, "/bucket/?delete=",
.content =
"<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n"
"<Delete><Quiet>true</Quiet>"
@@ -1378,7 +1378,7 @@ testRun(void)
"</ListBucketResult>");
testRequestP(
service, s3, HTTP_VERB_POST, "/bucket/?delete=", .contentMd5 = true,
service, s3, HTTP_VERB_POST, "/bucket/?delete=",
.content =
"<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n"
"<Delete><Quiet>true</Quiet>"
@@ -1388,7 +1388,7 @@ testRun(void)
testResponseP(service);
testRequestP(
service, s3, HTTP_VERB_POST, "/bucket/?delete=", .contentMd5 = true,
service, s3, HTTP_VERB_POST, "/bucket/?delete=",
.content =
"<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n"
"<Delete><Quiet>true</Quiet>"
@@ -1417,7 +1417,7 @@ testRun(void)
"</ListBucketResult>");
testRequestP(
service, s3, HTTP_VERB_POST, "/bucket/?delete=", .contentMd5 = true,
service, s3, HTTP_VERB_POST, "/bucket/?delete=",
.content =
"<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n"
"<Delete><Quiet>true</Quiet>"