[dvasilas]

TLDR

#6346 fixed CLDSRV-910.

But after the fix, a self-copy of an object damaged by CLDSRV-910 to a master key other than the one in its metadata replaces its content with the wrong content and deletes the original.

With this PR, before such a self-copy rewrites the data, it checks which master key decrypts it and uses that key. If none of the keys it tries does, the copy fails and nothing is written.

---

The original bug (CLDSRV-910)

Using a self-copy to re-encrypt an object under another master key changed only the master key ID in the object metadata. The data and its data key stayed encrypted under the old master key.
Reading such an object unwraps the data key with the wrong master key: either GET returns incorrect content, or it fails if KMS detects the wrong key.
Before the CLDSRV-910 fix, a self-copy naming the old master key repaired the object: it changed the master key ID in the metadata back to the old key.

Problem

Since the CLDSRV-910 fix, a self-copy to a master key other than the one in the object metadata rewrites the data.
Here is what happens if such a self-copy is done to an object damaged by CLDSRV-910, even when it names the master key the data is encrypted under.
The self-copy:

• Decrypts the data with the master key in the object metadata (the wrong one)
• Stores the wrong content, encrypted under the destination master key
• Deletes the original data

The original content can no longer be recovered.

Since the CLDSRV-910 fix, a self-copy rewrites the data.
Here is what happens if a self-copy with another master key is done to an object damaged by CLDSRV-910.
The self-copy:

• Decrypts the data with the master key in the object metadata (the wrong one)
• Stores the wrong content
• Deletes the original data.

The object can no longer be repaired.

This affects only objects damaged by CLDSRV-910.
Self-copy for objects not affected by CLDSRV-910 works as expected.

Fix

With this PR, before rewriting the data, a self-copy decrypts the smallest encrypted part with up to 3 master keys, in this order: the master key in the object metadata, the master key recorded on the locations, and the destination master key.
It compares the MD5 of the decrypted part with the MD5 recorded for that part.
The copy reads the data with the first key that matches. If no key matches, or the part has no recorded MD5, the copy fails with InternalError, and nothing is written.

For example, the data is encrypted under K1 and the metadata names K2.
K2 doesn't match; K1 does:

• A copy to K1 leaves the data as it is and records K1 in the metadata.
• A copy to another key, K3, decrypts the data with K1 and encrypts it under K3.

When the new check runs

When the check runs, it costs one read of the smallest part and one KMS decrypt per key tried.

This check runs only when

• The self-copy rewrites the data and replaces it (versioning not enabled), and
• The locations record a master key.

This covers any object updated by a self-copy that didn't rewrite the data (this includes objects damaged by CLDSRV-910), even if its metadata no longer says it is encrypted. A self-copy without SSE, in a bucket without default encryption, removes the encryption from the metadata but leaves the data encrypted.
Other objects are rewritten as before, without the extra read.

---

Notes

• An encrypted object written before S3C 7.1 has no recorded MD5. If its locations record a master key, a self-copy that rewrites it now fails with InternalError.
• The check also covers a self-copy to another location constraint (x-amz-metadata-directive: REPLACE with x-amz-meta-scal-location-constraint). That copy always rewrites the data, so it replaced damaged objects with the wrong content before CLDSRV-910 too.
• Not covered:
  • A self-copy with UploadPartCopy and CompleteMultipartUpload (CLDSRV-1071).
  • A self-copy without SSE headers, in a bucket without default encryption, removes the encryption from the object metadata but leaves the data encrypted (CLDSRV-1072).

[bert-e]

Hello dvasilas,

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	⭐
/bypass_source_branch_lineage	Bypass the cross-branch contamination check	⭐
/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.
/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]

Incorrect fix version

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

• None

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

• 9.3.26

• 9.4.11

• 9.5.3

• 9.6.0

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

[bert-e]

Incorrect fix version

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

• None

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

• 9.3.30

• 9.4.15

• 9.5.5

• 9.6.0

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

[codecov]

❌ 1 Tests Failed:

Tests completed	Failed	Passed	Skipped
8658	1	8657	0

View the top 1 failed test(s) by shortest run time

should complete MPU on AWS with same key as object put to file::Multipart Upload API with AWS Backend should complete MPU on AWS with same key as object put to file

Stack Traces | 60s run time

Timeout of 60000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (.../tests/multipleBackend/multipartUpload.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]

Request integration branches

Waiting for integration branch creation to be requested by the user.

To request integration branches, please comment on this pull request with the following command:

/create_integration_branches

Alternatively, the /approve and /create_pull_requests commands will automatically
create the integration branches.

[leif-scality]

Minor (rare, but the loss is permanent): a wrong key can pass the check on a tiny last part

• Cause: the check decrypts only the smallest part that has an MD5 (sourceMasterKey.js:41-51). With a KMS that never rejects a wrong master key (file and memory), a wrong key turns a one-byte part into the right byte once in 256.
• Effect: the check accepts that key, the copy decrypts every other part with it, stores the result and deletes the original. The client gets 200.
• Odds: 1 in 256 for a one-byte part, 1 in 65,536 for two bytes, 1 in 16.7 million for three, per wrong key tried before the right one. The metadata key is tried first, so every damaged object is exposed to it. Retrying does not change the outcome for a given key and object.
• Not affected: objects that are not multipart (the whole object is checked, so a match means the content is right), and a KMS that rejects a wrong key.
• Test: should not accept a key that decrypts only a one-byte last part (code below). It builds a 5 MiB + 1 byte multipart object under K1, creates keys until one decrypts the last byte correctly, sets it as the metadata key, and self-copies to K3:

  AssertionError [ERR_ASSERTION]: the copy stored wrong content
  

• Fix: do not trust a part of a few bytes alone. Add the next smallest parts until at least 16 bytes are checked.

Test

Goes in describe('which master key decrypts the data') in tests/unit/api/objectCopy.js.

it('should not accept a key that decrypts only a one-byte last part', async function test() {
    this.timeout(60000);
    const { promisify } = require('util');
    const parts = [Buffer.alloc(constants.minimumAllowedPartSize, 'a'), Buffer.from('z')];
    const [k1, k3] = await promisify(createKeys)(2);
    // A two-part object under k1, whose last part is one byte.
    const uploadId = await promisify(initiateMpu)(objectKey, kmsHeaders(k1));
    for (let i = 0; i < parts.length; i++) {
        // eslint-disable-next-line no-await-in-loop
        await new Promise((resolve, reject) =>
            objectPutPart(authInfo, partRequest(objectKey, uploadId, i + 1, parts[i]), undefined, log, err =>
                (err ? reject(err) : resolve()),
            ),
        );
    }
    await promisify(completeMpu)(objectKey, uploadId, parts.map(md5));
    const { location } = await promisify(getObjectMD)();
    const last = location[location.length - 1];
    // Decrypts the stored byte of the last part with a master key, as data.get does.
    const lastByteWith = masterKeyId =>
        new Promise((resolve, reject) =>
            kms.createDecipherBundle(
                {
                    cryptoScheme: last.cryptoScheme,
                    masterKeyId,
                    cipheredDataKey: Buffer.from(last.cipheredDataKey, 'base64'),
                },
                0,
                log,
                (err, bundle) => (err ? reject(err) : resolve(bundle.decipher.update(ds[last.key].value))),
            ),
        );
    // Create keys until one decrypts that byte to 'z': 1 key in 256.
    let k2;
    for (let i = 0; i < 20000 && !k2; i++) {
        // eslint-disable-next-line no-await-in-loop
        const candidate = await promisify(createKey)();
        // eslint-disable-next-line no-await-in-loop
        if ((await lastByteWith(candidate)).equals(parts[1])) {
            k2 = candidate;
        }
    }
    assert(k2, 'no such key found');
    // The damaged state: data under k1, which the locations record, and k2 in the metadata.
    await promisify(updateObjectMD)(md => {
        // eslint-disable-next-line no-param-reassign
        md['x-amz-server-side-encryption-aws-kms-key-id'] = k2;
        recordKeyOnLocations(md, k1);
    });
    await promisify(selfCopy)(kmsHeaders(k3));
    const body = await promisify(getBody)();
    assert(body.equals(Buffer.concat(parts)), 'the copy stored wrong content');
});