CLDSRV-636: SSE with both internal & external KMS (HF 9.2.0.12) #5788

BourgoisMickael · 2025-04-11T18:40:12Z

This is generic but simplified for file to aws use case and will be extended in latest version to handle any backend.

Store all KMS keys with an arn describing backend provider (see ARSN-485: Prefix KMS Keys with an arn (HF cldsrv 9.2.0.12) Arsenal#2349)
- option (hideScalityArn) to keep backward compatibility
Migrate KMS Keys with sseMigration config
- For bucket, replace file key with aws key
- For object reformat key into an arn
Allow usage of keys from previous KMS provider (file) to decript and read files (or for ongoing MPU)

Overall example of a new Key Format and migration

flowchart TD
    GetObject:::red --> bktArn
    objArn --NO GetObject--> objUpdate[Update Object: Prefix KMS key with arn]:::red
    %% Get 0-1 Links

    %% Common route
    bktArn{Bucket SSE is arn?}:::purple
        --NO--> bktKey["
            Create new external KMS Key with arn prefix
            Update Bucket attribute
        "]:::purple
    bktArn
        --Then--> objArn{Object SSE is arn?}:::purple
        --Then--> ...:::purple
        --> KMS
    subgraph KMS[KMS#nbsp;Object#nbsp;Decrypt/Encrypt]
        extract[Extract KMS provider and KeyId from arn]:::purple
        --> client[Pick or instantiate KMS client based on provider from Key arn]:::purple
        --> send["
            Backend Decrypts/Encrypts with KeyId
            But if Arn is sent, client still extract KeyId from arn
        "]:::purple
    end
    %% 2-7 Links common

    PutObject:::blue --> bktArn
    objArn --NO PutObject--> PutPrefix[Prefix KMS Key with arn if missing]:::blue
    %% PUT 8-9 Links

    classDef red stroke:red
    classDef blue stroke:blue
    classDef purple stroke:purple

    linkStyle 0,1 stroke:red
    linkStyle 8,9 stroke:blue
    linkStyle 2,3,4,5,6,7 stroke:purple

bert-e · 2025-04-11T18:40:16Z

Hello bourgoismickael,

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	⭐
`/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 `TBA`
`/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 · 2025-04-11T20:31:00Z

Waiting for approval

The following approvals are needed before I can proceed with the merge:

the author
2 peers

lib/api/apiUtils/bucket/updateEncryption.js

nicolas2bert · 2025-04-14T14:02:35Z

lib/api/apiUtils/bucket/updateEncryption.js

+    // this should trigger vault account key update as well
+    return kms.createBucketKey(bucket, log, (err, { masterKeyArn }) => {
+        if (err) {
+            return cb(err, bucket);


should we log something?

errors should already be logged by kms implementation. But we could append a message to the error to describe the error is due to the SSE migration

nicolas2bert · 2025-04-14T14:22:13Z

lib/api/apiUtils/bucket/updateEncryption.js

+            sse.masterKeyId = masterKeyArn;
+        }
+        if (updateConfigured) {
+            sse.configuredMasterKeyId = masterKeyArn;


Are sse.masterKeyId and sse.configuredMasterKeyId not suppose to be different?
Does it break something if we have them both be the same value?

Yes they should be different, I need to fix this

Actually nothing should break, right now they are different because they are set at different times, but having the same key is fine

lib/api/apiUtils/bucket/updateEncryption.js

nicolas2bert · 2025-04-15T08:26:25Z

lib/api/objectGet.js

@@ -51,6 +52,7 @@ function objectGet(authInfo, request, returnTagCount, log, callback) {
    };

    return metadataValidateBucketAndObj(mdValParams, log,
+    (err, bucket, objMD) => updateEncryption(err, bucket, objMD, objectKey, log, {},


Should we fail the GET/HEAD object call if update encryption fails?

Right now it will fail, the error is passed to the next callback.

But we could provide a flag through config to ignore migration failure and still return the object.

The only reason where the update could fail but not the get is if you can't write, because disk is full but still wants to read from it.

nicolas2bert · 2025-04-15T09:02:03Z

lib/kms/wrapper.js

@@ -309,17 +377,21 @@ class KMS {
            cryptoScheme: serverSideEncryptionInfo.cryptoScheme,
            decipher: null,
        };
+
+        // shadowing global client for key - implName already used can't be shadowed here
+        const { client, implName: _impl, key } = getClientForKey(serverSideEncryptionInfo.masterKeyId);


We always need to check the client because there is no way to know that a migration never happened before? If we had a way to know that no migration happened before, we would be safe to use the default KMS, right? How is the impact of getClientForKey on the GET object latency?

If we know the migration never happened before, we'd rather know we should use the previous KMS instead of the new default external KMS.

But the way to know for now for outscale is if the scality kms arnPrefix is present or not.

I've also pre instantiated the previous KMS as file is the only allowed for now in migration, to avoid latency of first GET after startup.

The impact on GET object latency is:

extract key detail from arn: split('/') and split(':') on the key: https://github.com/scality/Arsenal/pull/2349/files#diff-1c24d37a19bc08405b548ef9b5ac12a2ef83b7edb3a2ee62cc8426c7df8e5b95R122-R137

if key is scality arnPrefix

validate the key parts: https://github.com/scality/Arsenal/pull/2349/files#diff-1c24d37a19bc08405b548ef9b5ac12a2ef83b7edb3a2ee62cc8426c7df8e5b95R145-R167

concatenate the clientIdentifier string: https://github.com/scality/cloudserver/pull/5788/files#diff-c506ce0254987c99cd5ff5a10cd1ecabc6e1bc59e643fe2401103b9feb7b27a6R131

return the client and KeyId

if key is not scality arnPrefix

return the client (default or previous based on sseMigration configuration presence) and the KeyId

But as the GetObject migrate to new scality arn format, it should be expected the first Get on a bucket and object is slower for migration, and then we fall in the case where it's a scality arnPrefix

nicolas2bert · 2025-04-15T09:03:50Z

lib/kms/wrapper.js

@@ -196,10 +261,13 @@ class KMS {
            log.debug('using user configured kms master key id');
            masterKeyId = configuredMasterKeyId;
        }
+        // shadowing global client for key
+        // but should not happen to cipher for another client as Puts should use current KMS
+        const { client, implName, key } = getClientForKey(masterKeyId);


Is getClientForKey needed for PUT object? We know that the client will always be the default one. What is the impact on our PUT object latency?

It's needed if kms key is passed in scality arn format on a PUT request. Then the function will extract the KeyId from the arn and validate the arn scality format.

It also returns the client identifier so we can block usage of other client like if an arn for the file KMS is used when there is a configured external KMS, it should be forbidden: https://github.com/scality/cloudserver/pull/5788/files#diff-c506ce0254987c99cd5ff5a10cd1ecabc6e1bc59e643fe2401103b9feb7b27a6R332-R339

And for any future case where multiple KMS providers can be used it will become useful

As for PUT latency, it will be same as GET:

The impact on PUT object latency is:

extract key detail from arn: split('/') and split(':') on the key: https://github.com/scality/Arsenal/pull/2349/files#diff-1c24d37a19bc08405b548ef9b5ac12a2ef83b7edb3a2ee62cc8426c7df8e5b95R122-R137

if key is scality arnPrefix

validate the key parts: https://github.com/scality/Arsenal/pull/2349/files#diff-1c24d37a19bc08405b548ef9b5ac12a2ef83b7edb3a2ee62cc8426c7df8e5b95R145-R167

concatenate the clientIdentifier string: https://github.com/scality/cloudserver/pull/5788/files#diff-c506ce0254987c99cd5ff5a10cd1ecabc6e1bc59e643fe2401103b9feb7b27a6R131

return the client and KeyId

if key is not scality arnPrefix (should not happen as the PUT will prefix the key with a scality arn)

return the client (default or previous based on sseMigration configuration presence) and the KeyId

lib/kms/wrapper.js

> This is a scheduled Ubuntu 20.04 brownout. Ubuntu 20.04 LTS runner will be removed on 2025-04-15. For more details, see actions/runner-images#11101 (cherry picked from commit 5c4a1b3)

Avoid print the warning if scality-kms is not configured: ```json {"name":"S3","time":1742220545805,"error":{"code":"MODULE_NOT_FOUND","requireStack":["/home/mickael/scality/cloudserver/lib/kms/wrapper.js","/home/mickael/scality/cloudserver/lib/data/wrapper.js","/home/mickael/scality/cloudserver/lib/utilities/healthcheckHandler.js","/home/mickael/scality/cloudserver/lib/utilities/internalHandlers.js","/home/mickael/scality/cloudserver/lib/server.js","/home/mickael/scality/cloudserver/index.js"]},"level":"warn","message":"scality kms unavailable. Using file kms backend unless mem specified.","hostname":"mickael-xps","pid":139701} ``` (cherry picked from commit 9e8c505)

lib/Config.js

lib/kms/wrapper.js

lib/Config.js

lib/api/apiUtils/bucket/updateEncryption.js

lib/kms/wrapper.js

lib/api/objectPutPart.js

lib/api/apiUtils/object/sseHeaders.js

package.json

anurag4DSB

Thanks for the changes

To avoid breaking changes for clients

Issue: S3C-9996 MPU was not accepting configuredMasterKeyId (from object or bucket) for Create and Complete MPU. But used the configuredMasterKeyId (from object or bucket) for UploadPart making it possible to have parts encrypted with a different key than the key in object metadata after completion. Resulting in potential error on GetObject later: - If bucket had configuredMasterKeyId but no masterKeyId (no AES before or aws:kms without configuredKey) it would crash on decryption. - If bucket had configuredMasterKeyId and a masterKeyId it would be used to decrypt parts potentially encrypted with a configuredMasterKeyId, decrypting gibberish - On UploadPart object level encryption could be provided (outside sdk/cli) which is bad. ___ The expected behavior fixed here is to use the provided SSE on CreateMPU and then for UploadPart and CompleteMPU use the SSE provided at CreateMPU. The MPU calls also return the SSE headers now. The CopyObject returned SSE headers are fixed as well, the configuredMasterKeyId was ignored This fix allow seamless continuation of ongoing MPU during SSE migration. ___ There is no migration to fix already existing completed MPU.

BourgoisMickael requested review from jonathan-gramain, tcarmet, tmacro, dvasilas and a team April 11, 2025 18:40

scality deleted a comment from bert-e Apr 12, 2025

tcarmet approved these changes Apr 14, 2025

View reviewed changes

nicolas2bert reviewed Apr 15, 2025

View reviewed changes

BourgoisMickael force-pushed the improvement/CLDSRV-636-kms-provider branch from a7f5219 to b40068c Compare April 17, 2025 01:15

BourgoisMickael added 3 commits April 30, 2025 18:08

CLDSRV-635: Fix ubuntu 20 in CI

cf85e2a

> This is a scheduled Ubuntu 20.04 brownout. Ubuntu 20.04 LTS runner will be removed on 2025-04-15. For more details, see actions/runner-images#11101 (cherry picked from commit 5c4a1b3)

CLDSRV-636: Speed a test up with fake timer (10s)

db43aab

BourgoisMickael force-pushed the improvement/CLDSRV-636-kms-provider branch 3 times, most recently from 642fb3f to 9cd6ec0 Compare May 2, 2025 03:42

anurag4DSB reviewed May 2, 2025

View reviewed changes

lib/Config.js Outdated Show resolved Hide resolved

BourgoisMickael changed the base branch from hotfix/7.70.21 to hotfix/7.70.21-outscale May 2, 2025 18:16

anurag4DSB reviewed May 6, 2025

View reviewed changes

BourgoisMickael requested a review from anurag4DSB May 7, 2025 15:44

anurag4DSB approved these changes May 7, 2025

View reviewed changes

BourgoisMickael added 2 commits May 8, 2025 20:58

CLDSRV-636: Prefix KMS Key with an arn

6bbe2a8

CLDSRV-636: Option for KMS to hide scality arn

f762831

To avoid breaking changes for clients

BourgoisMickael force-pushed the improvement/CLDSRV-636-kms-provider branch from 8ff7b58 to cd4bca8 Compare May 8, 2025 22:03

BourgoisMickael changed the base branch from hotfix/7.70.21-outscale to hotfix/7.70.21.outscale May 13, 2025 16:13

BourgoisMickael added 4 commits May 13, 2025 19:42

CLDSRV-636: SSE Migration function

e4adb8f

CLDSRV-636: Migrate SSE on object(Get|Head|Put)

0429cd7

CLDSRV-636: Handle multi KMS clients for migration

1be8cd8

BourgoisMickael added 6 commits May 13, 2025 19:42

CLDSRV-636: Allow previous KMS for MPU parts

415781a

CLDSRV-636: Migrate SSE on CopyObject and init MPU

eb282f4

CLDSRV-636: Fix unit tests with KMS arn prefix

ea6fa9f

CLDSRV-636: Fix functional tests encryption

1324204

CLDSRV-636: Bump arsenal

784bb78

CLDSRV-636: Bump version

c2f045f

BourgoisMickael force-pushed the improvement/CLDSRV-636-kms-provider branch from cd4bca8 to c2f045f Compare May 13, 2025 17:44

BourgoisMickael mentioned this pull request May 14, 2025

CLDSRV-636: SSE with both internal/external KMS (Cherry-pick) #5800

Merged

tcarmet merged commit 37162e0 into hotfix/7.70.21.outscale May 14, 2025
8 checks passed

tcarmet deleted the improvement/CLDSRV-636-kms-provider branch May 14, 2025 22:25

CLDSRV-636: SSE with both internal & external KMS (HF 9.2.0.12) #5788

CLDSRV-636: SSE with both internal & external KMS (HF 9.2.0.12) #5788

Uh oh!

Conversation

BourgoisMickael commented Apr 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overall example of a new Key Format and migration

Uh oh!

bert-e commented Apr 11, 2025

Hello bourgoismickael,

Uh oh!

bert-e commented Apr 11, 2025

Waiting for approval

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

anurag4DSB left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

BourgoisMickael commented Apr 11, 2025 •

edited

Loading