fixups post reviews

Author: benzekrimaha

lib/auth/v2/getCanonicalizedAmzHeaders.ts

@@ -1,4 +1,7 @@
-export default function getCanonicalizedAmzHeaders(headers: Record<string, string>, clientType: string) {
+import type { ArsenalRequestHeaders } from '../../types/ArsenalRequest';
+import { normalizeHeaderValue } from '../../utils/normalizeHeaders';
+
+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
@@ -11,12 +14,7 @@ export default function getCanonicalizedAmzHeaders(headers: Record<string, strin
         .filter(filterFn)
         .map(val => {
             const headerValue = headers[val];
-            // 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(',') 
-                : String(headerValue);
+            const stringValue = normalizeHeaderValue(headerValue);
             return [val.trim(), stringValue.trim()];
         });
     /*

lib/auth/v4/createCanonicalRequest.ts

@@ -1,6 +1,7 @@
 import * as crypto from 'crypto';
 import * as queryString from 'querystring';
 import awsURIencode from './awsURIencode';
+import { normalizeHeaderValue } from '../../utils/normalizeHeaders';
 
 /**
  * createCanonicalRequest - creates V4 canonical request
@@ -77,9 +78,7 @@ export default function createCanonicalRequest(
             // 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 v4, multiple values are joined with commas
-            const stringValue = Array.isArray(headerValue) 
-                ? headerValue.join(',') 
-                : String(headerValue);
+            const stringValue = normalizeHeaderValue(headerValue);
             const trimmedHeader = stringValue
                 .trim().replace(/\s+/g, ' ');
             return `${signedHeader}:${trimmedHeader}\n`;

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,23 +279,29 @@ 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);
@@ -300,4 +311,5 @@ export default class Client implements KMSInterface {
             cb(error);
         });
     }
+    */
 }

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