Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 11 additions & 10 deletions .github/docker/docker-compose.yaml
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
services:
cloudserver:
image: ${CLOUDSERVER_IMAGE}
network_mode: "host"
network_mode: 'host'
volumes:
- /tmp/ssl:/ssl
- /tmp/ssl-kmip:/tmp/ssl-kmip
Expand Down Expand Up @@ -47,6 +47,7 @@ services:
- S3QUOTA
- QUOTA_ENABLE_INFLIGHTS
- S3_VERSION_ID_ENCODING_TYPE
- S3_INTEGRITY_CHECKS_ENABLED
- S3_SERVER_ACCESS_LOGS_MODE=ENABLED
- S3_ENABLE_SERVER_ACCESS_LOGS=true
- RATE_LIMIT_SERVICE_USER_ARN=arn:aws:iam::123456789013:root
Expand All @@ -56,13 +57,13 @@ services:
depends_on:
- redis
extra_hosts:
- "bucketwebsitetester.s3-website-us-east-1.amazonaws.com:127.0.0.1"
- "pykmip.local:127.0.0.1"
- 'bucketwebsitetester.s3-website-us-east-1.amazonaws.com:127.0.0.1'
- 'pykmip.local:127.0.0.1'
redis:
image: redis:alpine
network_mode: "host"
network_mode: 'host'
squid:
network_mode: "host"
network_mode: 'host'
profiles: ['ci-proxy']
image: scality/ci-squid
command: >-
Expand All @@ -76,7 +77,7 @@ services:
volumes:
- /tmp/ssl:/ssl
pykmip:
network_mode: "host"
network_mode: 'host'
profiles: ['pykmip']
image: ${PYKMIP_IMAGE:-ghcr.io/scality/cloudserver/pykmip}
volumes:
Expand All @@ -86,17 +87,17 @@ services:
- ../pykmip/policy.json:/etc/pykmip/policies/policy.json
- ../pykmip/server.conf:/etc/pykmip/server.conf
localkms:
network_mode: "host"
network_mode: 'host'
profiles: ['localkms']
image: ${KMS_IMAGE:-nsmithuk/local-kms:3.11.7}
mongo:
network_mode: "host"
network_mode: 'host'
profiles: ['mongo']
image: ${MONGODB_IMAGE}
volumes:
- /tmp/artifacts/${JOB_NAME}:/logs
sproxyd:
network_mode: "host"
network_mode: 'host'
profiles: ['sproxyd']
image: sproxyd-standalone
build: ./sproxyd
Expand All @@ -109,7 +110,7 @@ services:
profiles: ['vault']
user: root
command: sh -c "chmod 400 tests/utils/keyfile && yarn start > /artifacts/vault.log 2> /artifacts/vault-stderr.log"
network_mode: "host"
network_mode: 'host'
volumes:
- /tmp/artifacts/${JOB_NAME}:/artifacts
- ./vault-config.json:/conf/config.json:ro
Expand Down
49 changes: 49 additions & 0 deletions .github/workflows/tests.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -558,6 +558,55 @@ jobs:
source: /tmp/artifacts
if: always()

# Runs the server with x-amz-checksum-* computation turned off and asserts the
# resulting behaviour. Needs its own job because every other functional suite
# runs against a server with checksums enabled.
checksums-disabled-tests:
runs-on: ubuntu-24.04
needs: build
env:
S3BACKEND: mem
S3VAULT: mem
CLOUDSERVER_IMAGE: ghcr.io/${{ github.repository }}:${{ github.sha }}-testcoverage
MONGODB_IMAGE: ghcr.io/${{ github.repository }}/ci-mongodb:${{ github.sha }}
MPU_TESTING: 'yes'
S3_INTEGRITY_CHECKS_ENABLED: 'false'
JOB_NAME: ${{ github.job }}
steps:
- name: Checkout
uses: actions/checkout@v4

Copy link
Copy Markdown
Contributor

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

- name: Setup CI environment
uses: ./.github/actions/setup-ci
- name: Setup CI services
run: docker compose up -d
working-directory: .github/docker
- name: Run checksums-disabled tests
run: |-
set -o pipefail;
bash wait_for_local_port.bash 8000 40
yarn run ft_checksums_disabled | tee /tmp/artifacts/${{ github.job }}/tests.log
- name: Cleanup and upload coverage
uses: ./.github/actions/cleanup-and-coverage
with:
codecov-token: ${{ secrets.CODECOV_TOKEN }}
if: always()
- name: Upload test results to Codecov
uses: codecov/test-results-action@v1
with:
token: ${{ secrets.CODECOV_TOKEN }}
files: '**/junit/*junit*.xml'
flags: checksums-disabled-tests
if: always() && !cancelled()
- name: Upload logs to artifacts
uses: scality/action-artifacts@v4
with:
method: upload
url: https://artifacts.scality.net
user: ${{ secrets.ARTIFACTS_USER }}
password: ${{ secrets.ARTIFACTS_PASSWORD }}
source: /tmp/artifacts
if: always()

# Configure and run as Integration run S3C tests
s3c-ft-tests:
strategy:
Expand Down
3 changes: 3 additions & 0 deletions config.json
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,9 @@
"multiObjectDelete": 2097152,
"bucketPutPolicy": 20480
},
"integrityChecks": {
"enabled": true
},
"serverAccessLogs": {
"mode": "DISABLED",
"outputFile": "/logs/server-access.log",
Expand Down
55 changes: 55 additions & 0 deletions lib/Config.js
Original file line number Diff line number Diff line change
Expand Up @@ -584,6 +584,59 @@ function parseServerAccessLogs(config) {
return res;
}

/**

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment looks too verbose, it concise

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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 toggle part-way through a multipart upload, and safe to roll out
* progressively across a fleet. Parts uploaded while disabled carry no digest,
* so CompleteMultipartUpload cannot compose a final checksum for that object:
* it completes the upload without one and logs an error naming the parts.
*
* @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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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 🤔

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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
*/
Expand Down Expand Up @@ -1829,6 +1882,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
Expand Down Expand Up @@ -2201,4 +2255,5 @@ module.exports = {
azureGetStorageAccountName,
azureGetLocationCredentials,
parseSupportedLifecycleRules,
parseIntegrityChecks,
};
79 changes: 78 additions & 1 deletion lib/api/apiUtils/integrity/validateChecksums.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -111,6 +116,8 @@ const ChecksumError = Object.freeze({
MPUAlgoNotSupported: 'MPUAlgoNotSupported',
MPUTypeInvalid: 'MPUTypeInvalid',
MPUTypeWithoutAlgo: 'MPUTypeWithoutAlgo',
MPUTypeNotConfigured: 'MPUTypeNotConfigured',
MPUTypeModeMismatch: 'MPUTypeModeMismatch',
MPUInvalidCombination: 'MPUInvalidCombination',
CopyChecksumAlgoNotSupported: 'CopyChecksumAlgoNotSupported',
ContentSHA256Missing: 'ContentSHA256Missing',
Expand All @@ -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);
Expand Down Expand Up @@ -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(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nvm its already whats done for other custom error 🤷

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All errors have a cloudserver definition for unit tests, in this case ChecksumError.MPUTypeNotConfigured only maps to errMPUTypeNotConfigured, but sometimes ChecksumError share the same arsenal error.

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 ` +
Expand All @@ -582,9 +608,11 @@ function arsenalErrorFromChecksumError(err) {
*
* @param {object} headers - request.headers (lowercased keys)
* @param {object|null} finalChecksum - { algorithm, type, value } or null
* @param {boolean} [checksumUnavailable] - no final checksum could be composed
* because one or more parts carry no stored digest.
* @returns {{error: string, details: object}|null}
*/
function validateCompleteMultipartUploadChecksum(headers, finalChecksum) {
function validateCompleteMultipartUploadChecksum(headers, finalChecksum, checksumUnavailable) {
// x-amz-checksum-type can be present in a completeMPU request so we drop it.
// x-amz-checksum-algorithm is ignored by AWS in this context so we drop it.
const valueHeaders = listChecksumValueHeaders(headers);
Expand Down Expand Up @@ -618,6 +646,10 @@ function validateCompleteMultipartUploadChecksum(headers, finalChecksum) {
return { error: ChecksumError.MalformedChecksum, details: { algorithm: foundAlgo, expected: foundValue } };
}

if (checksumUnavailable) {
return null;
}

if (!finalChecksum || finalChecksum.algorithm !== foundAlgo || finalChecksum.value !== foundValue) {
return {
error: ChecksumError.XAmzMismatch,
Expand Down Expand Up @@ -680,6 +712,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);
}

Expand Down Expand Up @@ -758,6 +793,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
// =============================================================================
Expand Down Expand Up @@ -876,8 +951,10 @@ module.exports = {
algorithms,
checksumedMethods,
getChecksumDataFromMPUHeaders,
validateCompleteMPUChecksumType,
computeCompositeMPUChecksum,
computeFullObjectMPUChecksum,
validateCompleteMultipartUploadChecksum,
getCopyObjectChecksumAlgorithm,
areChecksumsEnabled,
};
8 changes: 7 additions & 1 deletion lib/api/apiUtils/object/createAndStoreObject.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ const {
getChecksumDataFromHeaders,
arsenalErrorFromChecksumError,
validateXAmzContentSHA256,
areChecksumsEnabled,
} = require('../integrity/validateChecksums');

const { externalBackends, versioningNotImplBackends } = constants;
Expand All @@ -43,6 +44,11 @@ function zeroSizeBodyChecksumCheck(headers, metadataStoreParams, callback) {
if (contentSHA256Err) {
return callback(arsenalErrorFromChecksumError(contentSHA256Err));
}
// Nothing is computed or stored when checksums are disabled, so a zero-byte
// object ends up with no checksum metadata like every other object.
if (!areChecksumsEnabled()) {
return callback(null);
}
const checksumData = getChecksumDataFromHeaders(headers) || defaultChecksumData;
if (checksumData.error) {
return callback(arsenalErrorFromChecksumError(checksumData));
Expand Down Expand Up @@ -316,7 +322,7 @@ function createAndStoreObject(
}
}

const headerChecksum = getChecksumDataFromHeaders(request.headers);
const headerChecksum = areChecksumsEnabled() ? getChecksumDataFromHeaders(request.headers) : null;
if (headerChecksum && headerChecksum.error) {
return next(arsenalErrorFromChecksumError(headerChecksum));
}
Expand Down
Loading
Loading