-
Notifications
You must be signed in to change notification settings - Fork 258
CLDSRV-965: checksums feature flag #6248
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 11 commits
d846e35
2275d85
0949b18
b4e2b21
b945b92
0db5a7c
d840272
9d68467
1a579c1
38f420c
8218847
feb4abf
758cb0d
bb10e46
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -584,6 +584,58 @@ function parseServerAccessLogs(config) { | |
| return res; | ||
| } | ||
|
|
||
| /** | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This comment looks too verbose, it concise
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is verbose but it documents what the feature flag does and doesn't do. There is no useless info in it |
||
| * Parse the `integrityChecks` config section. | ||
| * | ||
| * `enabled` is a kill switch for `x-amz-checksum-*` digests, defaulting to true. | ||
| * Setting it to false stops CloudServer computing them at all — the point is to | ||
| * reclaim the CPU spent hashing every payload byte, so nothing is compared and | ||
| * no checksum is stored. Objects and MPU parts then carry no checksum metadata, | ||
| * the same state as objects predating checksum support. | ||
| * | ||
| * The headers are then ignored outright rather than merely unenforced: a | ||
| * malformed or unsupported `x-amz-checksum-*` value is accepted instead of | ||
| * rejected, and neither CreateMultipartUpload nor CompleteMultipartUpload | ||
| * validates `x-amz-checksum-algorithm`/`-type`. | ||
| * | ||
| * Content-MD5 and x-amz-content-sha256 are unaffected and remain enforced. | ||
| * | ||
| * Safe to disable part-way through a multipart upload, but not to re-enable: | ||
| * parts uploaded while disabled carry no digest, so a later CompleteMPU cannot | ||
| * compose the final checksum and fails. | ||
| * | ||
| * @param {object} config - raw parsed config file contents | ||
| * @return {{enabled: boolean}} the parsed integrityChecks section | ||
| */ | ||
| function parseIntegrityChecks(config) { | ||
| const res = { enabled: true }; | ||
|
|
||
| if (config && config.integrityChecks) { | ||
| assert( | ||
| typeof config.integrityChecks === 'object' && !Array.isArray(config.integrityChecks), | ||
| 'bad config: integrityChecks must be an object', | ||
| ); | ||
|
|
||
|
Comment on lines
+614
to
+619
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: isn't this kind of stuff preferably handled by the Joi library directly 🤔
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think its doable with joi, with like joi.default({enabled: true}) or something But honestly whatever, half the file is joi and half the file is manual check
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yea, I followed the existing pattern |
||
| if ('enabled' in config.integrityChecks) { | ||
| assert( | ||
| typeof config.integrityChecks.enabled === 'boolean', | ||
| 'bad config: integrityChecks.enabled must be a boolean', | ||
| ); | ||
| res.enabled = config.integrityChecks.enabled; | ||
| } | ||
| } | ||
|
|
||
| if (process.env.S3_INTEGRITY_CHECKS_ENABLED !== undefined) { | ||
| assert( | ||
| ['true', 'false'].includes(process.env.S3_INTEGRITY_CHECKS_ENABLED), | ||
| "bad config: S3_INTEGRITY_CHECKS_ENABLED must be 'true' or 'false'", | ||
| ); | ||
| res.enabled = process.env.S3_INTEGRITY_CHECKS_ENABLED === 'true'; | ||
| } | ||
|
|
||
| return res; | ||
| } | ||
|
|
||
| /** | ||
| * Reads from a config file and returns the content as a config object | ||
| */ | ||
|
|
@@ -1829,6 +1881,7 @@ class Config extends EventEmitter { | |
| this.apiBodySizeLimits[apiKey] = limit; | ||
| } | ||
| } | ||
| this.integrityChecks = parseIntegrityChecks(config); | ||
| this.serverAccessLogs = parseServerAccessLogs(config); | ||
| /** | ||
| * S3C-10336: PutObject max size of 5GB is new in 9.5.1 | ||
|
|
@@ -2201,4 +2254,5 @@ module.exports = { | |
| azureGetStorageAccountName, | ||
| azureGetLocationCredentials, | ||
| parseSupportedLifecycleRules, | ||
| parseIntegrityChecks, | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,6 +3,7 @@ const crypto = require('crypto'); | |
| const { crc32: crtCrc32, crc32c: crtCrc32c } = require('aws-crt').checksums; | ||
| const { CrtCrc64Nvme } = require('@aws-sdk/crc64-nvme-crt'); | ||
| const { errors: ArsenalErrors, errorInstances } = require('arsenal'); | ||
| const { config } = require('../../../Config'); | ||
| const { combinePartCrcs } = require('./crcCombine'); | ||
| const { supportedSignatureChecksums, unsupportedSignatureChecksums } = require('../../../../constants'); | ||
|
|
||
|
|
@@ -37,6 +38,10 @@ const errMPUTypeInvalid = errorInstances.InvalidRequest.customizeDescription( | |
| const errMPUTypeWithoutAlgo = errorInstances.InvalidRequest.customizeDescription( | ||
| 'The x-amz-checksum-type header can only be used with the x-amz-checksum-algorithm header.', | ||
| ); | ||
| const errMPUTypeNotConfigured = errorInstances.InvalidRequest.customizeDescription( | ||
| 'The upload was not created with a checksum mode. ' + | ||
| 'The complete request must not include a x-amz-checksum-type header.', | ||
| ); | ||
|
|
||
| // TODO(S3C-11278): Update with 'MD5', 'SHA512', 'XXHASH128', 'XXHASH3', 'XXHASH64' when they are introduced. | ||
| // https://scality.atlassian.net/browse/S3C-11278 | ||
|
|
@@ -111,6 +116,8 @@ const ChecksumError = Object.freeze({ | |
| MPUAlgoNotSupported: 'MPUAlgoNotSupported', | ||
| MPUTypeInvalid: 'MPUTypeInvalid', | ||
| MPUTypeWithoutAlgo: 'MPUTypeWithoutAlgo', | ||
| MPUTypeNotConfigured: 'MPUTypeNotConfigured', | ||
| MPUTypeModeMismatch: 'MPUTypeModeMismatch', | ||
| MPUInvalidCombination: 'MPUInvalidCombination', | ||
| CopyChecksumAlgoNotSupported: 'CopyChecksumAlgoNotSupported', | ||
| ContentSHA256Missing: 'ContentSHA256Missing', | ||
|
|
@@ -120,6 +127,18 @@ const ChecksumError = Object.freeze({ | |
|
|
||
| const base64Regex = /^[A-Za-z0-9+/]*={0,2}$/; | ||
|
|
||
| /** | ||
| * When false, x-amz-checksum-* headers are ignored: no digest is calculated, | ||
| * compared, or stored. | ||
| * | ||
| * Content-MD5 and x-amz-content-sha256 are not impacted. | ||
| * | ||
| * @return {boolean} true when checksums are enabled. | ||
| */ | ||
| function areChecksumsEnabled() { | ||
| return config.integrityChecks?.enabled !== false; | ||
| } | ||
|
|
||
| function uint32ToBase64(num) { | ||
| const buf = Buffer.alloc(4); | ||
| buf.writeUInt32BE(num, 0); | ||
|
|
@@ -558,6 +577,13 @@ function arsenalErrorFromChecksumError(err) { | |
| return errMPUTypeInvalid; | ||
| case ChecksumError.MPUTypeWithoutAlgo: | ||
| return errMPUTypeWithoutAlgo; | ||
| case ChecksumError.MPUTypeNotConfigured: | ||
| return errMPUTypeNotConfigured; | ||
| case ChecksumError.MPUTypeModeMismatch: | ||
| return errorInstances.InvalidRequest.customizeDescription( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: a bit weird that you bothered defining errMPUTypeNotConfigured at the top of the file, but not this error 🤔 If it's because you need the details.type, you can still define it at the top with a function that take a param and return the error
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nvm its already whats done for other custom error 🤷
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All errors have a cloudserver definition for unit tests, in this case The goal is to separate domain errors from XML arsenal / AWS S3 error messages |
||
| `The upload was created using the ${err.details.type} checksum mode. ` + | ||
| 'The complete request must use the same checksum mode.', | ||
| ); | ||
| case ChecksumError.MPUInvalidCombination: | ||
| return errorInstances.InvalidRequest.customizeDescription( | ||
| `The ${err.details.type} checksum type cannot be used ` + | ||
|
|
@@ -680,6 +706,9 @@ async function validateMethodChecksumNoChunking(request, body, log) { | |
| } | ||
|
|
||
| if (request.apiMethod in checksumedMethods) { | ||
| if (!areChecksumsEnabled()) { | ||
| return md5OnlyValidationFunc(request, body, log); | ||
| } | ||
| return await defaultValidationFunc(request, body, log); | ||
| } | ||
|
|
||
|
|
@@ -758,6 +787,46 @@ function getChecksumDataFromMPUHeaders(headers) { | |
| return { algorithm: algo, type: defaultChecksumType[algo], isDefault: false }; | ||
| } | ||
|
|
||
| /** | ||
| * Validate the x-amz-checksum-type header on a CompleteMultipartUpload request | ||
| * against the checksum type the MPU was created with. | ||
| * | ||
| * x-amz-checksum-algorithm is deliberately not validated: AWS ignores a mismatch | ||
| * on that header for CompleteMultipartUpload. | ||
| * | ||
| * @param {object} headers - request headers (lowercased keys) | ||
| * @param {string|undefined} mpuChecksumType - the checksum type recorded on the | ||
| * MPU at CreateMPU time; falsy for a legacy MPU predating type tracking | ||
| * @param {boolean} isExternal - external-backend MPU. Those record no checksum | ||
| * config (CLDSRV-964), so an absent type means "not tracked here" rather than | ||
| * a legacy upload, and the header is ignored instead of rejected. | ||
| * @returns {{error: string, details: object}|null} null when valid | ||
| */ | ||
| function validateCompleteMPUChecksumType(headers, mpuChecksumType, isExternal) { | ||
| const headerType = headers['x-amz-checksum-type']; | ||
| if (!headerType) { | ||
| return null; | ||
| } | ||
|
|
||
| const headerTypeUpper = headerType.toUpperCase(); | ||
| if (!validMPUTypes.has(headerTypeUpper)) { | ||
| return { error: ChecksumError.MPUTypeInvalid, details: { type: headerType } }; | ||
| } | ||
|
|
||
| if (!mpuChecksumType) { | ||
| if (isExternal) { | ||
| return null; | ||
| } | ||
| return { error: ChecksumError.MPUTypeNotConfigured, details: { type: headerType } }; | ||
| } | ||
|
|
||
| if (headerTypeUpper !== mpuChecksumType.toUpperCase()) { | ||
| return { error: ChecksumError.MPUTypeModeMismatch, details: { type: mpuChecksumType } }; | ||
| } | ||
|
|
||
| return null; | ||
| } | ||
|
|
||
| // ============================================================================= | ||
| // MPU final-object checksum computation | ||
| // ============================================================================= | ||
|
|
@@ -876,8 +945,10 @@ module.exports = { | |
| algorithms, | ||
| checksumedMethods, | ||
| getChecksumDataFromMPUHeaders, | ||
| validateCompleteMPUChecksumType, | ||
| computeCompositeMPUChecksum, | ||
| computeFullObjectMPUChecksum, | ||
| validateCompleteMultipartUploadChecksum, | ||
| getCopyObjectChecksumAlgorithm, | ||
| areChecksumsEnabled, | ||
| }; | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: checkout v4 is getting old 🧐 v7 available