Skip to content

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

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

Merged
merged 15 commits into from
May 14, 2025
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
4 changes: 2 additions & 2 deletions .github/workflows/release.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ on:

jobs:
build-federation-image:
runs-on: ubuntu-20.04
runs-on: ubuntu-latest
steps:
- name: Checkout
uses: actions/checkout@v4
Expand All @@ -34,7 +34,7 @@ jobs:
cache-to: type=gha,mode=max,scope=federation

build-image:
runs-on: ubuntu-20.04
runs-on: ubuntu-latest
steps:
- name: Checkout
uses: actions/checkout@v4
Expand Down
4 changes: 2 additions & 2 deletions .github/workflows/tests.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ jobs:
if: always()

build:
runs-on: ubuntu-20.04
runs-on: ubuntu-latest
steps:
- name: Checkout
uses: actions/checkout@v4
Expand Down Expand Up @@ -158,7 +158,7 @@ jobs:
cache-to: type=gha,mode=max,scope=pykmip

build-federation-image:
runs-on: ubuntu-20.04
runs-on: ubuntu-latest
steps:
- name: Checkout
uses: actions/checkout@v4
Expand Down
2 changes: 2 additions & 0 deletions config.json
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,9 @@
}
],
"defaultEncryptionKeyPerAccount": true,
"kmsHideScalityArn": false,
"kmsAWS": {
"providerName": "aws",
"region": "us-east-1",
"endpoint": "http://127.0.0.1:8080",
"ak": "tbd",
Expand Down
91 changes: 89 additions & 2 deletions lib/Config.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,13 @@ const { azureAccountNameRegex, base64Regex,
} = require('../constants');
const { utapiVersion } = require('utapi');
const { versioning } = require('arsenal');
const {
KmsType,
KmsProtocol,
isValidProvider,
isValidType,
isValidProtocol,
} = require('arsenal/build/lib/network/KMSInterface');

const versionIdUtils = versioning.VersionID;

Expand Down Expand Up @@ -401,8 +408,9 @@ class Config extends EventEmitter {

// Read config automatically
this._getLocationConfig();
this._getConfig();
const config = this._getConfig();
this._configureBackends();
this._sseMigration(config);
}

_parseKmsAWS(config) {
Expand All @@ -411,13 +419,19 @@ class Config extends EventEmitter {
}
let kmsAWS = {};

const { region, endpoint, ak, sk, tls } = config.kmsAWS;
const { providerName, region, endpoint, ak, sk, tls, noAwsArn } = config.kmsAWS;

assert(providerName, 'Configuration Error: providerName must be defined in kmsAWS');
assert(isValidProvider(providerName),
'Configuration Error: kmsAWS.providerNamer must be lowercase alphanumeric only');
assert(endpoint, 'Configuration Error: endpoint must be defined in kmsAWS');
assert(ak, 'Configuration Error: ak must be defined in kmsAWS');
assert(sk, 'Configuration Error: sk must be defined in kmsAWS');
assert(['undefined', 'boolean'].some(type => type === typeof noAwsArn),
'Configuration Error:: kmsAWS.noAwsArn must be a boolean or not set');

kmsAWS = {
providerName,
endpoint,
ak,
sk,
Expand All @@ -427,6 +441,10 @@ class Config extends EventEmitter {
kmsAWS.region = region;
}

if (noAwsArn) {
kmsAWS.noAwsArn = noAwsArn;
}

if (tls) {
kmsAWS.tls = {};
if (tls.rejectUnauthorized !== undefined) {
Expand Down Expand Up @@ -1095,8 +1113,12 @@ class Config extends EventEmitter {

this.kms = {};
if (config.kms) {
assert(config.kms.providerName, 'config.kms.providerName must be provided');
assert(isValidProvider(config.kms.providerName),
'config.kms.providerName must be lowercase alphanumeric only');
assert(typeof config.kms.userName === 'string');
assert(typeof config.kms.password === 'string');
this.kms.providerName = config.kms.providerName;
this.kms.userName = config.kms.userName;
this.kms.password = config.kms.password;
if (config.kms.helperProgram !== undefined) {
Expand Down Expand Up @@ -1163,6 +1185,10 @@ class Config extends EventEmitter {
},
};
if (config.kmip) {
assert(config.kmip.providerName, 'config.kmip.providerName must be defined');
assert(isValidProvider(config.kmip.providerName),
'config.kmip.providerName must be lowercase alphanumeric only');
this.kmip.providerName = config.kmip.providerName;
if (config.kmip.client) {
if (config.kmip.client.compoundCreateActivate) {
assert(typeof config.kmip.client.compoundCreateActivate ===
Expand Down Expand Up @@ -1229,6 +1255,11 @@ class Config extends EventEmitter {
assert(typeof this.defaultEncryptionKeyPerAccount === 'boolean',
'config.defaultEncryptionKeyPerAccount must be a boolean');

this.kmsHideScalityArn = Object.hasOwnProperty.call(config, 'kmsHideScalityArn')
? config.kmsHideScalityArn
: true; // By default hide scality arn to keep backward compatibility and simplicity
assert.strictEqual(typeof this.kmsHideScalityArn, 'boolean');

this.healthChecks = defaultHealthChecks;
if (config.healthChecks && config.healthChecks.allowFrom) {
assert(config.healthChecks.allowFrom instanceof Array,
Expand Down Expand Up @@ -1410,6 +1441,7 @@ class Config extends EventEmitter {
}

this.lifecycleRoleName = config.lifecycleRoleName || null;
return config;
}

_configureBackends() {
Expand Down Expand Up @@ -1485,6 +1517,61 @@ class Config extends EventEmitter {
};
}

_sseMigration(config) {
if (config.sseMigration) {
/**
* For data that was encrypted internally by default and a new external provider is setup.
* This config helps detect the existing encryption key to decrypt with the good provider.
* The key format will be migrated automatically on GET/HEADs to include provider details.
*/
this.sseMigration = {};
const { previousKeyType, previousKeyProtocol, previousKeyProvider } = config.sseMigration;
if (!previousKeyType) {
assert.fail(
'NotImplemented: No dynamic KMS key migration. Set sseMigration.previousKeyType');
}

// If previousKeyType is provided it's used as static value to migrate the format of the key
// without additional dynamic evaluation if the key provider is unknown.
assert(isValidType(previousKeyType),
'ssenMigration.previousKeyType must be "internal" or "external"');
this.sseMigration.previousKeyType = previousKeyType;

let expectedProtocol;
if (previousKeyType === KmsType.internal) {
// For internal key type default protocol is file and provider is scality
this.sseMigration.previousKeyProtocol = previousKeyProtocol || KmsProtocol.file;
this.sseMigration.previousKeyProvider = previousKeyProvider || 'scality';
expectedProtocol = [KmsProtocol.scality, KmsProtocol.mem, KmsProtocol.file];
} else if (previousKeyType === KmsType.external) {
// No defaults allowed for external provider
assert(previousKeyProtocol,
'sseMigration.previousKeyProtocol must be defined for external provider');
this.sseMigration.previousKeyProtocol = previousKeyProtocol;
assert(previousKeyProvider,
'sseMigration.previousKeyProvider must be defined for external provider');
this.sseMigration.previousKeyProvider = previousKeyProvider;
expectedProtocol = [KmsProtocol.kmip, KmsProtocol.aws_kms];
}

assert(isValidProtocol(previousKeyType, this.sseMigration.previousKeyProtocol),
`sseMigration.previousKeyProtocol must be one of ${expectedProtocol}`);
assert(isValidProvider(previousKeyProvider),
'sseMigration.previousKeyProvider must be lowercase alphanumeric only');

if (this.sseMigration.previousKeyType === KmsType.external) {
if ([KmsProtocol.file, KmsProtocol.mem].includes(this.backends.kms)) {
assert.fail(
`sseMigration.previousKeyType "external" can't migrate to "internal" KMS provider ${
this.backends.kms}`
);
}
// We'd have to compare protocol & providerName
assert.fail('sseMigration.previousKeyType "external" is not yet available');
}
}
}

_verifyRedisPassword(password) {
return typeof password === 'string';
}
Expand Down
14 changes: 13 additions & 1 deletion lib/api/apiUtils/bucket/bucketEncryption.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ const { errors } = require('arsenal');
const metadata = require('../../../metadata/wrapper');
const kms = require('../../../kms/wrapper');
const { parseString } = require('xml2js');
const { isScalityKmsArn } = require('arsenal/build/lib/network/KMSInterface');

/**
* ServerSideEncryptionInfo - user configuration for server side encryption
Expand Down Expand Up @@ -95,6 +96,12 @@ function parseEncryptionXml(xml, log, cb) {
}

result.configuredMasterKeyId = encConfig.KMSMasterKeyID[0];
// If key is not in a scality arn format include a scality arn prefix
// of the currently selected KMS client.
// To keep track of KMS type, protocol and provider used
if (!isScalityKmsArn(result.configuredMasterKeyId)) {
result.configuredMasterKeyId = `${kms.arnPrefix}${result.configuredMasterKeyId}`;
}
}
return cb(null, result);
});
Expand All @@ -119,7 +126,12 @@ function hydrateEncryptionConfig(algorithm, configuredMasterKeyId, mandatory = n
const sseConfig = { algorithm, mandatory };

if (algorithm === 'aws:kms' && configuredMasterKeyId) {
sseConfig.configuredMasterKeyId = configuredMasterKeyId;
// If key is not in a scality arn format include a scality arn prefix
// of the currently selected KMS client.
// To keep track of KMS type, protocol and provider used
sseConfig.configuredMasterKeyId = isScalityKmsArn(configuredMasterKeyId)
? configuredMasterKeyId
: `${kms.arnPrefix}${configuredMasterKeyId}`;
}

if (mandatory !== null) {
Expand Down
141 changes: 141 additions & 0 deletions lib/api/apiUtils/bucket/updateEncryption.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,141 @@
const { getVersionSpecificMetadataOptions } = require('../object/versioning');
// const getReplicationInfo = require('../object/getReplicationInfo');
const { config } = require('../../../Config');
const kms = require('../../../kms/wrapper');
const metadata = require('../../../metadata/wrapper');
const { isScalityKmsArn, makeScalityArnPrefix } = require('arsenal/build/lib/network/KMSInterface');

// Bucket need a key from the new KMS, not a simple reformating
function updateBucketEncryption(bucket, log, cb) {
const sse = bucket.getServerSideEncryption();

if (!sse) {
return cb(null, bucket);
}

const masterKey = sse.masterKeyId;
const configuredKey = sse.configuredMasterKeyId;

// Note: if migration is from an external to an external, absence of arn is not enough
// a comparison of arn will be necessary but config validation blocks this for now
const updateMaster = masterKey && !isScalityKmsArn(masterKey);
const updateConfigured = configuredKey && !isScalityKmsArn(configuredKey);

if (!updateMaster && !updateConfigured) {
return cb(null, bucket);
}
log.debug('trying to update bucket encryption', { oldKey: masterKey || configuredKey });
// this should trigger vault account key update as well
return kms.createBucketKey(bucket, log, (err, newSse) => {
if (err) {
return cb(err, bucket);
Copy link
Contributor

Choose a reason for hiding this comment

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

should we log something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}
// if both keys needs migration, it is ok the use the same KMS key
// as the configured one should be used and the only way to use the
// masterKeyId is to PutBucketEncryption to AES256 but then nothing
// will break and the same KMS key will continue to be used.
// And the key is managed (created) by Scality, not passed from input.
if (updateMaster) {
sse.masterKeyId = newSse.masterKeyArn;
}
if (updateConfigured) {
sse.configuredMasterKeyId = newSse.masterKeyArn;
}
// KMS account key will not be deleted when bucket is deleted
if (newSse.isAccountEncryptionEnabled) {
sse.isAccountEncryptionEnabled = newSse.isAccountEncryptionEnabled;
}

log.info('updating bucket encryption', {
oldKey: masterKey || configuredKey,
newKey: newSse.masterKeyArn,
isAccount: newSse.isAccountEncryptionEnabled,
});
return metadata.updateBucket(bucket.getName(), bucket, log, err => cb(err, bucket));
});
}

// Only reformat the key, don't generate a new one.
// Use opts.skipObjectUpdate to only prepare objMD without sending the update to metadata
// if a metadata.putObjectMD is expected later in call flow. (Downside: update skipped if error)
function updateObjectEncryption(bucket, objMD, objectKey, log, keyArnPrefix, opts, cb) {
if (!objMD) {
return cb(null, bucket, objMD);
}

const key = objMD['x-amz-server-side-encryption-aws-kms-key-id'];

if (!key || isScalityKmsArn(key)) {
return cb(null, bucket, objMD);
}
const newKey = `${keyArnPrefix}${key}`;
// eslint-disable-next-line no-param-reassign
objMD['x-amz-server-side-encryption-aws-kms-key-id'] = newKey;
// Doesn't seem to be used but update as well
for (const dataLocator of objMD.location || []) {
if (dataLocator.masterKeyId) {
dataLocator.masterKeyId = `${keyArnPrefix}${dataLocator.masterKeyId}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could it be that the key and dataLocator.masterKeyId were, or could have been, updated separately?

  • What if key has the old format, and dataLocator.masterKeyId has the new one?
    In that case, dataLocator.masterKeyId might be updated with the same prefix multiple times.

  • What if key has the new format, and dataLocator.masterKeyId still uses the old one?
    In that case, dataLocator.masterKeyId might never get updated to the new format.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so, they are not used to get the object as the GetObject rewrite those to decrypt the data:

for (let i = 0; i < dataLocator.length; i++) {
dataLocator[i].masterKeyId =
objMD['x-amz-server-side-encryption-aws-kms-key-id'];
dataLocator[i].algorithm =
objMD['x-amz-server-side-encryption'];

I'll check why they are set on objects sometimes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For any GET | Website | Copy the dataLocator sse is always rewritten with the metadata sse.

I suspect it should not be saved into DB but it's set on PUT for the dataWrapper to encrypt the data but in some cases end up being written into metadata

}
}
// eslint-disable-next-line no-param-reassign
objMD.originOp = 's3:ObjectCreated:Copy';
// Copy should be tested for 9.5 in INTGR-1038
// to make sure it does not impact backbeat CRR / bucket notif
const params = getVersionSpecificMetadataOptions(objMD, config.nullVersionCompatMode);

log.info('reformating object encryption key', { oldKey: key, newKey, skipUpdate: opts.skipObjectUpdate });
if (opts.skipObjectUpdate) {
return cb(null, bucket, objMD);
}
return metadata.putObjectMD(bucket.getName(), objectKey, objMD, params,
log, err => cb(err, bucket, objMD));
}

/**
* Update encryption of bucket and object if kms provider changed
*
* @param {Error} err - error coming from metadata validate before the action handling
* @param {BucketInfo} bucket - bucket
* @param {Object} [objMD] - object metadata
* @param {string} objectKey - objectKey from request.
* @param {Logger} log - request logger
* @param {Object} opts - options for sseMigration
* @param {boolean} [opts.skipObject] - ignore object update
* @param {boolean} [opts.skipObjectUpdate] - don't update metadata but prepare objMD for later update
* @param {Function} cb - callback (err, bucket, objMD)
* @returns {undefined}
*/
function updateEncryption(err, bucket, objMD, objectKey, log, opts, cb) {
// Error passed here to call the function inbetween the metadataValidate and its callback
if (err) {
return cb(err);
}
// if objMD missing, still try updateBucketEncryption
if (!config.sseMigration) {
return cb(null, bucket, objMD);
}

const { previousKeyType, previousKeyProtocol, previousKeyProvider } = config.sseMigration;
// previousKeyType is required and validated in Config.js
// for now it is the only implementation we need.
// See TAD Seamless decryption with internal and external KMS: https://scality.atlassian.net/wiki/x/EgADu
// for other method of migration without a previousKeyType

const keyArnPrefix = makeScalityArnPrefix(previousKeyType, previousKeyProtocol, previousKeyProvider);

return updateBucketEncryption(bucket, log, (err, bucket) => {
// Any error in updating encryption at bucket or object level is returned to client.
// Other possibilities: ignore error, include sse migration notice in error message.
if (err) {
return cb(err, bucket, objMD);
}
if (opts.skipObject) {
return cb(err, bucket, objMD);
}
return updateObjectEncryption(bucket, objMD, objectKey, log, keyArnPrefix, opts, cb);
});
}

module.exports = {
updateEncryption,
};
Loading
Loading