diff --git a/lib/auth/v4/streamingV4/constructChunkStringToSign.ts b/lib/auth/v4/streamingV4/constructChunkStringToSign.ts index b606f71fd..a3228ab37 100644 --- a/lib/auth/v4/streamingV4/constructChunkStringToSign.ts +++ b/lib/auth/v4/streamingV4/constructChunkStringToSign.ts @@ -24,8 +24,12 @@ export default function constructChunkStringToSign( currentChunkHash = constants.emptyStringHash; } else { const hash = crypto.createHash('sha256'); - const temp = hash.update(justDataChunk); - currentChunkHash = temp.digest('hex'); + if (typeof justDataChunk === 'string') { + hash.update(justDataChunk, 'binary'); + } else { + hash.update(justDataChunk); + } + currentChunkHash = hash.digest('hex'); } return `AWS4-HMAC-SHA256-PAYLOAD\n${timestamp}\n` + `${credentialScope}\n${lastSignature}\n` + diff --git a/lib/network/kmsAWS/Client.ts b/lib/network/kmsAWS/Client.ts index 49047cc16..64d8ebedb 100644 --- a/lib/network/kmsAWS/Client.ts +++ b/lib/network/kmsAWS/Client.ts @@ -60,7 +60,7 @@ export default class Client implements KMSInterface { constructor(options: ClientOptions) { this._supportsDefaultKeyPerAccount = true; - const { providerName,tls, ak, sk, region, endpoint, noAwsArn } = options.kmsAWS; + const { providerName, tls, ak, sk, region, endpoint, noAwsArn } = options.kmsAWS; const requestHandler = new NodeHttpHandler({ httpAgent: !tls ? new HttpAgent({ @@ -134,6 +134,10 @@ export default class Client implements KMSInterface { // Prefer ARN, but fall back to KeyId if ARN is missing keyId = keyMetadata?.Arn ?? (keyMetadata?.KeyId || ''); } + // May produce double arn prefix: scality arn + aws arn + // arn:scality:kms:external:aws_kms:custom:key/arn:aws:kms:region:accountId:key/cbd69d33-ba8e-4b56-8cfe + // If this is a problem, a config flag should be used to hide the scality arn when returning the KMS KeyId + // or aws arn when creating the KMS Key const arn = `${this.backend.arnPrefix}${keyId}`; cb(null, keyId, arn); }).catch(err => { diff --git a/lib/network/utils.ts b/lib/network/utils.ts index 884ecd000..a318d9ded 100644 --- a/lib/network/utils.ts +++ b/lib/network/utils.ts @@ -1,5 +1,7 @@ import { ArsenalError, errorInstances } from '../errors'; import { allowedKmsErrors } from '../errors/kmsErrors'; +import { S3ServiceException } from '@aws-sdk/client-s3'; +import { KMSServiceException } from '@aws-sdk/client-kms'; /** * Normalize errors according to arsenal definitions with a custom prefix @@ -31,28 +33,14 @@ export function arsenalErrorKMIP(err: string | Error) { const allowedKmsErrorCodes = Object.keys(allowedKmsErrors) as unknown as (keyof typeof allowedKmsErrors)[]; -// Local AWSError type for compatibility with v3 error handling -export type AWSError = Error & { - name?: string; - $fault?: 'client' | 'server'; - $metadata?: { - httpStatusCode?: number; - requestId?: string; - attempts?: number; - totalRetryDelay?: number; - }; - $retryable?: { - throttling?: boolean; - }; - message?: string; -}; - -function isAWSError(err: string | Error | AWSError): err is AWSError { - return (err as AWSError).name !== undefined - && (err as AWSError).$metadata !== undefined; +function isAWSError(err: unknown): + err is S3ServiceException | KMSServiceException | (Error & { name?: string }) { + return (err instanceof S3ServiceException || err instanceof KMSServiceException || + (err instanceof Error && typeof err.name === 'string') + ); } -export function arsenalErrorAWSKMS(err: string | Error | AWSError) { +export function arsenalErrorAWSKMS(err: string | Error | S3ServiceException) { if (isAWSError(err)) { const errorCode = err.name; if (allowedKmsErrorCodes.includes(errorCode as keyof typeof allowedKmsErrors)) { diff --git a/lib/storage/data/external/GCP/GcpService.js b/lib/storage/data/external/GCP/GcpService.js index 3a4678558..871596e11 100644 --- a/lib/storage/data/external/GCP/GcpService.js +++ b/lib/storage/data/external/GCP/GcpService.js @@ -546,7 +546,7 @@ class GcpClient extends S3Client { delete deleteParams.VersionId; } return this.send(new DeleteObjectCommand(deleteParams)) - .then(data => callback && callback(null, data)) + .then(data => callback?.(null, data)) .catch(err => callback?.(err)); } diff --git a/lib/storage/data/external/GcpClient.js b/lib/storage/data/external/GcpClient.js index 64cd06886..c755472e7 100644 --- a/lib/storage/data/external/GcpClient.js +++ b/lib/storage/data/external/GcpClient.js @@ -307,37 +307,37 @@ class GcpClient extends AwsClient { listObjects(params, callback) { return this.send(new ListObjectsCommand(params)) - .then(data => callback && callback(null, data)) + .then(data => callback?.(null, data)) .catch(err => callback?.(err)); } putObject(params, callback) { return this.send(new PutObjectCommand(params)) - .then(data => callback && callback(null, data)) + .then(data => callback?.(null, data)) .catch(err => callback?.(err)); } getObject(params, callback) { return this.send(new GetObjectCommand(params)) - .then(data => callback && callback(null, data)) + .then(data => callback?.(null, data)) .catch(err => callback?.(err)); } deleteObject(params, callback) { return this.send(new DeleteObjectCommand(params)) - .then(data => callback && callback(null, data)) + .then(data => callback?.(null, data)) .catch(err => callback?.(err)); } copyObject(params, callback) { return this.send(new CopyObjectCommand(params)) - .then(data => callback && callback(null, data)) + .then(data => callback?.(null, data)) .catch(err => callback?.(err)); } headObject(params, callback) { return this.send(new HeadObjectCommand(params)) - .then(data => callback && callback(null, data)) + .then(data => callback?.(null, data)) .catch(err => callback?.(err)); } } diff --git a/tests/unit/storage/data/DummyService.js b/tests/unit/storage/data/DummyService.js index ace1df472..7c33805ff 100644 --- a/tests/unit/storage/data/DummyService.js +++ b/tests/unit/storage/data/DummyService.js @@ -88,6 +88,13 @@ class AzureDummyContainerClient { readableStreamBody: new DummyObjectStream(offset, length || OBJECT_SIZE), }; } + + async deleteIfExists() { + if (this.key === 'externalBackendTestBucket/externalBackendMissingKey') { + return { succeeded: false }; + } + return { succeeded: true }; + } } class DummyService { diff --git a/tests/unit/storage/data/external/ExternalClients.spec.js b/tests/unit/storage/data/external/ExternalClients.spec.js index b1d465cf3..6b0ee5764 100644 --- a/tests/unit/storage/data/external/ExternalClients.spec.js +++ b/tests/unit/storage/data/external/ExternalClients.spec.js @@ -1,6 +1,6 @@ const assert = require('assert'); -const async = require('async'); const stream = require('stream'); +const sinon = require('sinon'); const { promisify } = require('util'); const AwsClient = require('../../../../../lib/storage/data/external/AwsClient'); @@ -50,11 +50,21 @@ const backendClients = [ }, ]; const log = new DummyRequestLogger(); +let sandbox; describe('external backend clients', () => { + beforeEach(() => { + sandbox = sinon.createSandbox(); + }); + + afterEach(() => { + sandbox.restore(); + }); + backendClients.forEach(backend => { let testClient; - let headAsync, getAsync, objectPutTaggingAsync, objectDeleteTaggingAsync; + let headAsync, getAsync, deleteAsync, objectPutTaggingAsync, objectDeleteTaggingAsync, + createMPUAsync, uploadPartAsync, abortMPUAsync, listPartsAsync; beforeAll(() => { testClient = new backend.Class(backend.config); @@ -63,10 +73,17 @@ describe('external backend clients', () => { // Promisify the client methods headAsync = promisify(testClient.head.bind(testClient)); getAsync = promisify(testClient.get.bind(testClient)); + deleteAsync = promisify(testClient.delete.bind(testClient)); if (backend.config.type !== 'azure') { + createMPUAsync = promisify(testClient.createMPU.bind(testClient)); + uploadPartAsync = promisify(testClient.uploadPart.bind(testClient)); + abortMPUAsync = promisify(testClient.abortMPU.bind(testClient)); objectPutTaggingAsync = promisify(testClient.objectPutTagging.bind(testClient)); objectDeleteTaggingAsync = promisify(testClient.objectDeleteTagging.bind(testClient)); } + if (backend.config.type === 'aws') { + listPartsAsync = promisify(testClient.listParts.bind(testClient)); + } }); if (backend.config.type !== 'azure') { @@ -92,7 +109,9 @@ describe('external backend clients', () => { const uploadId = 'externalBackendTestUploadId'; testClient.completeMPU(jsonList, null, key, uploadId, bucketName, log, (err, res) => { - if (err) return done(err); + if (err) { + return done(err); + } assert.strictEqual(typeof res.key, 'string'); assert.strictEqual(typeof res.eTag, 'string'); assert.strictEqual(typeof res.dataStoreVersionId, 'string'); @@ -172,6 +191,16 @@ describe('external backend clients', () => { assert.strictEqual(errorHandled, true); }); + it(`${backend.name} delete() should delete the requested key without error`, async () => { + const key = 'externalBackendTestKey'; + const bucketName = 'externalBackendTestBucket'; + const objectInfo = Object.assign({ + deleteVersion: false, + }, testClient.toObjectGetInfo(key, bucketName)); + const result = await deleteAsync(objectInfo, ''); + assert.strictEqual(result, undefined); + }); + if (backend.config.type !== 'azure') { it(`${backend.name} should set tags and then delete it`, async () => { const key = 'externalBackendTestKey'; @@ -226,7 +255,93 @@ describe('external backend clients', () => { assert(err.is.ServiceUnavailable); } }); + + it(`${backend.name} uploadPart() should return sanitized data retrieval info`, async () => { + const key = 'externalBackendTestKey'; + const bucketName = 'externalBackendTestBucket'; + const result = await uploadPartAsync(null, null, + stream.Readable.from(['part data']), + 9, key, 'uploadId-123', 1, bucketName, log); + + assert.strictEqual(result.key, `${bucketName}/${key}`); + assert.strictEqual(result.dataStoreName, backend.config.dataStoreName); + assert(result.dataStoreETag); + assert.strictEqual(result.dataStoreETag.includes('"'), false); + }); + + it(`${backend.name} abortMPU() should resolve without error`, async () => { + const key = 'externalBackendTestKey'; + const bucketName = 'externalBackendTestBucket'; + + const result = await abortMPUAsync(key, 'uploadId-123', bucketName, log); + assert.strictEqual(result, undefined); + }); + + if (backend.config.type === 'aws') { + it(`${backend.name} listParts() should map result parts`, async () => { + const key = 'externalBackendTestKey'; + const bucketName = 'externalBackendTestBucket'; + + const storedParts = await listPartsAsync(key, 'uploadId-123', bucketName, 0, 1000, log); + + assert(Array.isArray(storedParts.Contents)); + assert(storedParts.Contents.length > 0); + const firstPart = storedParts.Contents[0]; + assert.strictEqual(typeof firstPart.partNumber, 'number'); + assert(firstPart.value); + assert.strictEqual(firstPart.value.ETag.includes('"'), false); + }); + } + + it(`${backend.name} createMPU() should trim metadata and forward tagging`, async () => { + const key = 'externalBackendTestKey'; + const bucketName = 'externalBackendTestBucket'; + const metaHeaders = { + 'x-amz-meta-custom-key': 'customValue', + 'x-amz-meta-second-key': 'secondValue', + ignored: 'shouldBeDropped', + }; + const args = [ + key, + metaHeaders, + bucketName, + 'http://redirect', + 'text/plain', + 'max-age=3600', + 'attachment', + 'gzip', + 'k1=v1&k2=v2', + log, + ]; + + if (backend.config.type === 'aws') { + const sendSpy = sandbox.spy(testClient._client, 'send'); + const result = await createMPUAsync(...args); + assert(result); + assert(result.UploadId); + assert(sendSpy.calledOnce); + const command = sendSpy.firstCall.args[0]; + assert.strictEqual(command.constructor.name, 'CreateMultipartUploadCommand'); + assert.deepStrictEqual(command.input.Metadata, { + 'custom-key': 'customValue', + 'second-key': 'secondValue', + }); + assert.strictEqual(command.input.Tagging, 'k1=v1&k2=v2'); + } else { + const createSpy = sandbox.spy(testClient._client, 'createMultipartUpload'); + const result = await createMPUAsync(...args); + assert(result); + assert(result.UploadId); + assert(createSpy.calledOnce); + const capturedParams = createSpy.firstCall.args[0]; + assert.strictEqual(capturedParams.Bucket, backend.config.mpuBucket); + assert.deepStrictEqual(capturedParams.Metadata, { + 'custom-key': 'customValue', + 'second-key': 'secondValue', + }); + assert.strictEqual(capturedParams.Tagging, 'k1=v1&k2=v2'); + } + }); } - // To-Do: test the other external client methods (delete, createMPU ...) }); }); diff --git a/tests/unit/storage/data/external/GcpService.spec.js b/tests/unit/storage/data/external/GcpService.spec.js index 89f4785c9..e96909590 100644 --- a/tests/unit/storage/data/external/GcpService.spec.js +++ b/tests/unit/storage/data/external/GcpService.spec.js @@ -98,6 +98,7 @@ function handler(isPathStyle) { } const invalidDnsBucketNames = [ + '..', '.bucketname', 'bucketname.', 'bucketName.', @@ -107,7 +108,7 @@ const invalidDnsBucketNames = [ function invalidDnsBucketNameHandler() { return (req, res) => { - assert(req.headers.host, host); + assert(req.headers.host, host); const bucketFromUrl = req.url.split('/')[1]; assert.strictEqual(typeof bucketFromUrl, 'string'); assert(invalidDnsBucketNames.includes(bucketFromUrl)); @@ -327,4 +328,132 @@ describe('GcpService dnsStyle tests', () => { operations.forEach(test => it(`GCP::${test.op}`, done => { client[test.op](test.params, err => done(err)); })); -}); \ No newline at end of file +}); + +describe('GcpService helper behavior', () => { + let client; + + beforeEach(() => { + client = new GCP({ + s3Params: { + endpoint: 'http://localhost', + maxAttempts: 1, + forcePathStyle: true, + region: 'us-east-1', + credentials: { + accessKeyId: 'access', + secretAccessKey: 'secret', + }, + }, + bucketName: 'unit-bucket', + dataStoreName: 'unit-location', + }); + }); + + afterEach(() => { + jest.restoreAllMocks(); + }); + + it('putObjectTagging should merge tags into metadata', done => { + jest.spyOn(client, 'headObject') + .mockImplementation((params, cb) => cb(null, { Metadata: { existing: 'alpha' } })); + const copySpy = jest.spyOn(client, 'copyObject') + .mockImplementation((params, cb) => cb(null, { CopyObjectResult: {} })); + + client.putObjectTagging({ + Bucket: 'unit-bucket', + Key: 'tagged-key', + Tagging: { + TagSet: [ + { Key: 'team', Value: 'storage' }, + { Key: 'env', Value: 'prod' }, + ], + }, + }, err => { + assert.ifError(err); + expect(copySpy).toHaveBeenCalledTimes(1); + const metadata = copySpy.mock.calls[0][0].Metadata; + assert.strictEqual(metadata.existing, 'alpha'); + assert.strictEqual(metadata['aws-tag-team'], 'storage'); + assert.strictEqual(metadata['aws-tag-env'], 'prod'); + done(); + }); + }); + + it('deleteObjectTagging should strip tag metadata and add sentinel', done => { + jest.spyOn(client, 'headObject') + .mockImplementation((params, cb) => cb(null, { + Metadata: { + 'aws-tag-project': 'zenko', + }, + })); + const copySpy = jest.spyOn(client, 'copyObject') + .mockImplementation((params, cb) => cb(null, { CopyObjectResult: {} })); + + client.deleteObjectTagging({ + Bucket: 'unit-bucket', + Key: 'tagged-key', + }, err => { + assert.ifError(err); + const metadata = copySpy.mock.calls[0][0].Metadata; + assert.strictEqual(metadata['aws-tag-project'], undefined); + assert.strictEqual(metadata['scal-tags-removed'], 'true'); + done(); + }); + }); + + it('getObjectTagging should return TagSet derived from metadata', done => { + jest.spyOn(client, 'headObject') + .mockImplementation((params, cb) => cb(null, { + Metadata: { + 'aws-tag-owner': 'arsenal', + 'aws-tag-color': 'blue', + misc: 'ignored', + }, + })); + + client.getObjectTagging({ + Bucket: 'unit-bucket', + Key: 'tagged-key', + }, (err, res) => { + assert.ifError(err); + assert.deepStrictEqual(res.TagSet, [ + { Key: 'owner', Value: 'arsenal' }, + { Key: 'color', Value: 'blue' }, + ]); + done(); + }); + }); + + it('createMultipartUpload should reject missing parameters', done => { + client.createMultipartUpload({ Bucket: 'unit-bucket' }, err => { + assert(err); + assert(err.is.InvalidRequest); + done(); + }); + }); + + it('uploadPart should reject invalid part number', done => { + client.uploadPart({ + Bucket: 'unit-bucket', + Key: 'object', + UploadId: 'upload', + PartNumber: 'NaN', + }, err => { + assert(err); + assert(err.is.InvalidArgument); + done(); + }); + }); + + it('uploadPartCopy should reject missing parameters', done => { + client.uploadPartCopy({ + Bucket: 'unit-bucket', + Key: 'object', + }, err => { + assert(err); + assert(err.is.InvalidRequest); + done(); + }); + }); +});