fixups post reviews

Author: benzekrimaha

lib/auth/v2/getCanonicalizedAmzHeaders.ts

@@ -1,4 +1,6 @@
-export default function getCanonicalizedAmzHeaders(headers: Record<string, string>, clientType: string) {
+import type { ArsenalRequestHeaders } from '../../types/ArsenalRequest';
+
+export default function getCanonicalizedAmzHeaders(headers: ArsenalRequestHeaders, clientType: string) {
     /*
     Iterate through headers and pull any headers that are x-amz headers.
     Need to include 'x-amz-date' here even though AWS docs
@@ -14,8 +16,8 @@ export default function getCanonicalizedAmzHeaders(headers: Record<string, strin
             // AWS SDK v3 can pass header values as arrays (for multiple values),
             // strings, or other types. We need to normalize them before calling .trim()
             // Per HTTP spec and AWS Signature v2, multiple values are joined with commas
-            const stringValue = Array.isArray(headerValue) 
-                ? headerValue.join(',') 
+            const stringValue = Array.isArray(headerValue)
+                ? headerValue.join(',')
                 : String(headerValue);
             return [val.trim(), stringValue.trim()];
         });

lib/auth/v4/streamingV4/constructChunkStringToSign.ts

@@ -24,8 +24,10 @@ export default function constructChunkStringToSign(
         currentChunkHash = constants.emptyStringHash;
     } else {
         const hash = crypto.createHash('sha256');
-        const temp = hash.update(justDataChunk);
-        currentChunkHash = temp.digest('hex');
+        const chunkHash = typeof justDataChunk === 'string'
+            ? hash.update(justDataChunk, 'binary')
+            : hash.update(justDataChunk);
+        currentChunkHash = chunkHash.digest('hex');
     }
     return `AWS4-HMAC-SHA256-PAYLOAD\n${timestamp}\n` +
         `${credentialScope}\n${lastSignature}\n` +

lib/network/kmsAWS/Client.ts

@@ -3,15 +3,16 @@
 import { arsenalErrorAWSKMS } from '../utils';
 import { Agent as HttpAgent } from 'http';
 import { Agent as HttpsAgent } from 'https';
-import { KMSClient, 
-    CreateKeyCommand, 
+import {
+    KMSClient,
+    CreateKeyCommand,
     ScheduleKeyDeletionCommand,
-    GenerateDataKeyCommand, 
-    EncryptCommand, 
-    DecryptCommand, 
-    ListKeysCommand, 
-    NotFoundException, 
-    KMSInvalidStateException } from '@aws-sdk/client-kms';
+    GenerateDataKeyCommand,
+    EncryptCommand,
+    DecryptCommand,
+    NotFoundException,
+    KMSInvalidStateException,
+} from '@aws-sdk/client-kms';
 const { NodeHttpHandler } = require('@smithy/node-http-handler');
 import * as werelogs from 'werelogs';
 import assert from 'assert';
@@ -60,7 +61,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,9 +135,13 @@ 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 => {
+        }).catch((err: Error) => {
             const error = arsenalErrorAWSKMS(err);
             logger.error('AWS KMS: failed to create master encryption key', { err });
             cb(error);
@@ -170,9 +175,10 @@ export default class Client implements KMSInterface {
                 return;
             }
             cb(null);
-        }).catch(err => {
+        }).catch((err: Error) => {
             if (err instanceof NotFoundException || err instanceof KMSInvalidStateException) {
-                logger.info('AWS KMS: key does not exist or is already pending deletion', { masterKeyId, error: err });
+                // master key does not exist or is already pending deletion
+                logger.warn('AWS KMS: key does not exist or is already pending deletion', { masterKeyId, error: err });
                 return cb(null);
             }
             const error = arsenalErrorAWSKMS(err);
@@ -204,7 +210,7 @@ export default class Client implements KMSInterface {
             const isolatedPlaintext = this.safePlaintext(data.Plaintext as Buffer);
             logger.debug('AWS KMS: data key generated');
             cb(null, isolatedPlaintext, Buffer.from(data.CiphertextBlob as Uint8Array));
-        }).catch(err => {
+        }).catch((err: Error) => {
             const error = arsenalErrorAWSKMS(err);
             logger.error('AWS KMS: failed to generate data key', { err });
             cb(error);
@@ -238,8 +244,7 @@ export default class Client implements KMSInterface {
 
             logger.debug('AWS KMS: data key ciphered');
             cb(null, Buffer.from(data.CiphertextBlob as Uint8Array));
-            return;
-        }).catch(err => {
+        }).catch((err: Error) => {
             const error = arsenalErrorAWSKMS(err);
             logger.error('AWS KMS: failed to cipher data key', { err });
             cb(error);
@@ -274,30 +279,23 @@ export default class Client implements KMSInterface {
 
             logger.debug('AWS KMS: data key deciphered');
             cb(null, isolatedPlaintext);
-        }).catch(err => {
+        }).catch((err: Error) => {
             const error = arsenalErrorAWSKMS(err);
             logger.error('AWS KMS: failed to decipher data key', { err });
             cb(error);
         });
     }
 
     /**
-     * Healthcheck function to verify KMS connectivity
+     * NOTE1: S3C-4833 KMS healthcheck is disabled in CloudServer.
+     *
+     * For the Arsenal client library we intentionally keep this as a no-op
+     * to avoid making extra AWS KMS calls (which can incur costs and require
+     * additional permissions). Callers should rely on higher-level health
+     * checks provided by their services instead of this method.
      */
     healthcheck(logger: werelogs.Logger, cb: (err: Error | null) => void): void {
-        logger.debug("AWS KMS: performing healthcheck");
-    
-        const command = new ListKeysCommand({
-            Limit: 1,
-        });
-    
-        this.client.send(command).then(() => {
-            logger.debug("AWS KMS healthcheck: list keys succeeded");
-            cb(null);
-        }).catch(err => {
-            const error = arsenalErrorAWSKMS(err);
-            logger.error("AWS KMS healthcheck: failed to list keys", { err });
-            cb(error);
-        });
+        logger.debug("AWS KMS: healthcheck is a no-op (disabled)");
+        cb(null);
     }
 }

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,40 +33,50 @@ 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;
-};
+type AwsSdkError = (S3ServiceException | KMSServiceException | (Error & {
+    name: string;
+    $metadata?: { [key: string]: unknown };
+}));
 
-function isAWSError(err: string | Error | AWSError): err is AWSError {
-    return (err as AWSError).name !== undefined
-        && (err as AWSError).$metadata !== undefined;
+function getAwsErrorCode(err: unknown): string | undefined {
+    if (err instanceof S3ServiceException || err instanceof KMSServiceException) {
+        return err.name;
+    }
+
+    if (err instanceof Error && typeof err.name === 'string') {
+        // AWS SDK v3 errors inherit from Error but are not always instances of the
+        // exported Exception classes once they cross async boundaries.
+        // They still expose metadata markers such as `$metadata` and an error `name`.
+        const maybeAwsMetadata = (err as AwsSdkError).$metadata;
+        if (maybeAwsMetadata && typeof maybeAwsMetadata === 'object') {
+            return err.name;
+        }
+
+        if (allowedKmsErrorCodes.includes(err.name as keyof typeof allowedKmsErrors)) {
+            return err.name;
+        }
+    }
+
+    return undefined;
 }
 
-export function arsenalErrorAWSKMS(err: string | Error | AWSError) {
-    if (isAWSError(err)) {
-        const errorCode = err.name;
+export function arsenalErrorAWSKMS(err: string | Error | S3ServiceException) {
+    const awsErrorCode = getAwsErrorCode(err);
+
+    if (awsErrorCode) {
+        const errorCode = awsErrorCode;
+        const errorMessage = err instanceof Error ? err.message : String(err);
+
         if (allowedKmsErrorCodes.includes(errorCode as keyof typeof allowedKmsErrors)) {
-            return errorInstances[`KMS.${errorCode}`].customizeDescription(err.message);
+            return errorInstances[`KMS.${errorCode}`].customizeDescription(errorMessage);
         } else {
             // Encapsulate into a generic ArsenalError but keep the aws error code
             return ArsenalError.unflatten({
                 is_arsenal_error: true,
                 type: `KMS.${errorCode}`, // aws s3 prefix kms errors with KMS.
                 code: 500,
                 description: `unexpected AWS_KMS error`,
-                stack: err.stack,
+                stack: err instanceof Error ? err.stack : undefined,
             });
         }
     }

lib/storage/data/LocationConstraintParser.js

@@ -1,6 +1,5 @@
 const { http, https } = require('httpagent');
 const url = require('url');
-const { S3Client } = require('@aws-sdk/client-s3');
 const { NodeHttpHandler } = require('@smithy/node-http-handler');
 const { fromIni } = require('@aws-sdk/credential-providers');
 const Sproxy = require('sproxydclient');

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));
     }
 
@@ -857,6 +857,7 @@ function gcpSigningMiddleware(config) {
                     pathForSigning = key ? `/${args.input.Bucket}/${key}` : `/${args.input.Bucket}/`;
                 }
             }
+            request.path = pathForSigning;
             // Build string to sign
             const fakeRequest = {
                 method: request.method,

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));
     }
 }

lib/storage/data/external/utils.js

@@ -45,7 +45,7 @@ const utils = {
         const metaObj = obj || {};
         Object.keys(metaObj).forEach(key => {
             const newKey = key.substring(11);
-            newObj[newKey] = `${metaObj[key]}`;
+            newObj[newKey] = string(metaObj[key]);
         });
         return newObj;
     },

package.json

@@ -88,7 +88,7 @@
   "scripts": {
     "build": "tsc",
     "build_doc": "cd documentation/listingAlgos/pics; dot -Tsvg delimiterStateChart.dot > delimiterStateChart.svg; dot -Tsvg delimiterMasterV0StateChart.dot > delimiterMasterV0StateChart.svg; dot -Tsvg delimiterVersionsStateChart.dot > delimiterVersionsStateChart.svg",
-    "coverage": "export NODE_OPTIONS=\"--tls-max-v1.2\" && nyc --clean jest tests/functional/metadata/mongodb/delObject.spec.js",
+    "coverage": "export NODE_OPTIONS=\"--tls-max-v1.2\" && nyc --clean jest tests --coverage --testTimeout=120000 --forceExit --testPathIgnorePatterns tests/functional/pykmip",
     "dev": "nodemon --ext ts --ignore build --exec \"yarn build\"",
     "ft_pykmip_test": "jest tests/functional/pykmip",
     "ft_test": "jest tests/functional --testTimeout=120000 --forceExit --testPathIgnorePatterns tests/functional/pykmip",

tests/functional/kmsAWS/highlevel.spec.js

@@ -8,6 +8,7 @@ describe('KmsAWSClient', () => {
         info: () => {},
         debug: () => {},
         error: () => {},
+        warn: () => {},
     };
 
     let client;
@@ -464,7 +465,7 @@ describe('KmsAWSClient', () => {
         });
     });
 
-    it('should list keys as a health check', done => {
+    it.skip('should list keys as a health check', done => {
         const mockResponse = {
             Keys: [
                 {
@@ -482,7 +483,7 @@ describe('KmsAWSClient', () => {
         });
     });
 
-    it('should return a failed health check when list keys is unsuccessful', done => {
+    it.skip('should return a failed health check when list keys is unsuccessful', done => {
         const mockError = new Error('mock listKeys error');
         mockError.name = 'AccessDeniedException';
         mockError.$metadata = {