Skip to content

Commit 82141f3

Browse files
benzekrimahabenzekrimaha
committed
post reviews fixups
1 parent 6166515 commit 82141f3

File tree

12 files changed

+133
-95
lines changed

12 files changed

+133
-95
lines changed

‎lib/auth/v2/getCanonicalizedAmzHeaders.ts‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,6 @@
1-
export default function getCanonicalizedAmzHeaders(headers: Record<string, string>, clientType: string) {
1+
import type { ArsenalRequestHeaders } from '../../types/ArsenalRequest';
2+
3+
export default function getCanonicalizedAmzHeaders(headers: ArsenalRequestHeaders, clientType: string) {
24
/*
35
Iterate through headers and pull any headers that are x-amz headers.
46
Need to include 'x-amz-date' here even though AWS docs
@@ -14,9 +16,9 @@ export default function getCanonicalizedAmzHeaders(headers: Record<string, strin
1416
// AWS SDK v3 can pass header values as arrays (for multiple values),
1517
// strings, or other types. We need to normalize them before calling .trim()
1618
// Per HTTP spec and AWS Signature v2, multiple values are joined with commas
17-
const stringValue = Array.isArray(headerValue)
18-
? headerValue.join(',')
19-
: String(headerValue);
19+
const stringValue = Array.isArray(headerValue)
20+
? headerValue.join(',')
21+
: (headerValue ?? '');
2022
return [val.trim(), stringValue.trim()];
2123
});
2224
/*

‎lib/auth/v4/streamingV4/constructChunkStringToSign.ts‎

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -24,12 +24,10 @@ export default function constructChunkStringToSign(
2424
currentChunkHash = constants.emptyStringHash;
2525
} else {
2626
const hash = crypto.createHash('sha256');
27-
if (typeof justDataChunk === 'string') {
28-
hash.update(justDataChunk, 'binary');
29-
} else {
30-
hash.update(justDataChunk);
31-
}
32-
currentChunkHash = hash.digest('hex');
27+
const chunkHash = typeof justDataChunk === 'string'
28+
? hash.update(justDataChunk, 'binary')
29+
: hash.update(justDataChunk);
30+
currentChunkHash = chunkHash.digest('hex');
3331
}
3432
return `AWS4-HMAC-SHA256-PAYLOAD\n${timestamp}\n` +
3533
`${credentialScope}\n${lastSignature}\n` +

‎lib/network/kmsAWS/Client.ts‎

Lines changed: 22 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -9,13 +9,13 @@ import { KMSClient,
99
GenerateDataKeyCommand,
1010
EncryptCommand,
1111
DecryptCommand,
12-
ListKeysCommand,
1312
NotFoundException,
1413
KMSInvalidStateException } from '@aws-sdk/client-kms';
1514
const { NodeHttpHandler } = require('@smithy/node-http-handler');
1615
import * as werelogs from 'werelogs';
1716
import assert from 'assert';
1817
import { KMSInterface, KmsBackend, getKeyIdFromArn, KmsProtocol, KmsType, makeBackend } from '../KMSInterface';
18+
import { AwsError } from '../types';
1919

2020
type TLSVersion = 'TLSv1.3' | 'TLSv1.2' | 'TLSv1.1' | 'TLSv1';
2121

@@ -140,7 +140,7 @@ export default class Client implements KMSInterface {
140140
// or aws arn when creating the KMS Key
141141
const arn = `${this.backend.arnPrefix}${keyId}`;
142142
cb(null, keyId, arn);
143-
}).catch(err => {
143+
}).catch((err: AwsError) => {
144144
const error = arsenalErrorAWSKMS(err);
145145
logger.error('AWS KMS: failed to create master encryption key', { err });
146146
cb(error);
@@ -174,9 +174,10 @@ export default class Client implements KMSInterface {
174174
return;
175175
}
176176
cb(null);
177-
}).catch(err => {
177+
}).catch((err: AwsError) => {
178178
if (err instanceof NotFoundException || err instanceof KMSInvalidStateException) {
179-
logger.info('AWS KMS: key does not exist or is already pending deletion', { masterKeyId, error: err });
179+
// master key does not exist or is already pending deletion
180+
logger.warn('AWS KMS: key does not exist or is already pending deletion', { masterKeyId, error: err });
180181
return cb(null);
181182
}
182183
const error = arsenalErrorAWSKMS(err);
@@ -208,7 +209,7 @@ export default class Client implements KMSInterface {
208209
const isolatedPlaintext = this.safePlaintext(data.Plaintext as Buffer);
209210
logger.debug('AWS KMS: data key generated');
210211
cb(null, isolatedPlaintext, Buffer.from(data.CiphertextBlob as Uint8Array));
211-
}).catch(err => {
212+
}).catch((err: AwsError) => {
212213
const error = arsenalErrorAWSKMS(err);
213214
logger.error('AWS KMS: failed to generate data key', { err });
214215
cb(error);
@@ -242,8 +243,7 @@ export default class Client implements KMSInterface {
242243

243244
logger.debug('AWS KMS: data key ciphered');
244245
cb(null, Buffer.from(data.CiphertextBlob as Uint8Array));
245-
return;
246-
}).catch(err => {
246+
}).catch((err: AwsError) => {
247247
const error = arsenalErrorAWSKMS(err);
248248
logger.error('AWS KMS: failed to cipher data key', { err });
249249
cb(error);
@@ -278,16 +278,27 @@ export default class Client implements KMSInterface {
278278

279279
logger.debug('AWS KMS: data key deciphered');
280280
cb(null, isolatedPlaintext);
281-
}).catch(err => {
281+
}).catch((err: AwsError) => {
282282
const error = arsenalErrorAWSKMS(err);
283283
logger.error('AWS KMS: failed to decipher data key', { err });
284284
cb(error);
285285
});
286286
}
287287

288288
/**
289-
* Healthcheck function to verify KMS connectivity
289+
* NOTE1: S3C-4833 KMS healthcheck is disabled in CloudServer
290+
* NOTE2: The best approach for implementing the AWS KMS health check is still under consideration.
291+
* In the meantime, this method is commented out to prevent potential issues related to costs or permissions.
292+
*
293+
* Reasons for commenting out:
294+
* - frequent API calls can lead to increased expenses.
295+
* - access key secret key used must have `kms:ListKeys` permissions
296+
*
297+
* Future potential actions:
298+
* - implement caching mechanisms to reduce the number of API calls.
299+
* - differentiate between error types (e.g., 500 vs. 403) for more effective error handling.
290300
*/
301+
/*
291302
healthcheck(logger: werelogs.Logger, cb: (err: Error | null) => void): void {
292303
logger.debug("AWS KMS: performing healthcheck");
293304
@@ -298,10 +309,11 @@ export default class Client implements KMSInterface {
298309
this.client.send(command).then(() => {
299310
logger.debug("AWS KMS healthcheck: list keys succeeded");
300311
cb(null);
301-
}).catch(err => {
312+
}).catch(err : AwsError => {
302313
const error = arsenalErrorAWSKMS(err);
303314
logger.error("AWS KMS healthcheck: failed to list keys", { err });
304315
cb(error);
305316
});
306317
}
318+
*/
307319
}

‎lib/network/utils.ts‎

Lines changed: 33 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -33,26 +33,50 @@ export function arsenalErrorKMIP(err: string | Error) {
3333

3434
const allowedKmsErrorCodes = Object.keys(allowedKmsErrors) as unknown as (keyof typeof allowedKmsErrors)[];
3535

36-
function isAWSError(err: unknown):
37-
err is S3ServiceException | KMSServiceException | (Error & { name?: string }) {
38-
return (err instanceof S3ServiceException || err instanceof KMSServiceException ||
39-
(err instanceof Error && typeof err.name === 'string')
40-
);
36+
type AwsSdkError = (S3ServiceException | KMSServiceException | (Error & {
37+
name: string;
38+
$metadata?: { [key: string]: unknown };
39+
}));
40+
41+
function getAwsErrorCode(err: unknown): string | undefined {
42+
if (err instanceof S3ServiceException || err instanceof KMSServiceException) {
43+
return err.name;
44+
}
45+
46+
if (err instanceof Error && typeof err.name === 'string') {
47+
// AWS SDK v3 errors inherit from Error but are not always instances of the
48+
// exported Exception classes once they cross async boundaries.
49+
// They still expose metadata markers such as `$metadata` and an error `name`.
50+
const maybeAwsMetadata = (err as AwsSdkError).$metadata;
51+
if (maybeAwsMetadata && typeof maybeAwsMetadata === 'object') {
52+
return err.name;
53+
}
54+
55+
if (allowedKmsErrorCodes.includes(err.name as keyof typeof allowedKmsErrors)) {
56+
return err.name;
57+
}
58+
}
59+
60+
return undefined;
4161
}
4262

4363
export function arsenalErrorAWSKMS(err: string | Error | S3ServiceException) {
44-
if (isAWSError(err)) {
45-
const errorCode = err.name;
64+
const awsErrorCode = getAwsErrorCode(err);
65+
66+
if (awsErrorCode) {
67+
const errorCode = awsErrorCode;
68+
const errorMessage = err instanceof Error ? err.message : `${err}`;
69+
4670
if (allowedKmsErrorCodes.includes(errorCode as keyof typeof allowedKmsErrors)) {
47-
return errorInstances[`KMS.${errorCode}`].customizeDescription(err.message);
71+
return errorInstances[`KMS.${errorCode}`].customizeDescription(errorMessage);
4872
} else {
4973
// Encapsulate into a generic ArsenalError but keep the aws error code
5074
return ArsenalError.unflatten({
5175
is_arsenal_error: true,
5276
type: `KMS.${errorCode}`, // aws s3 prefix kms errors with KMS.
5377
code: 500,
5478
description: `unexpected AWS_KMS error`,
55-
stack: err.stack,
79+
stack: err instanceof Error ? err.stack : undefined,
5680
});
5781
}
5882
}

‎lib/storage/data/LocationConstraintParser.js‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
11
const { http, https } = require('httpagent');
22
const url = require('url');
3-
const { S3Client } = require('@aws-sdk/client-s3');
43
const { NodeHttpHandler } = require('@smithy/node-http-handler');
54
const { fromIni } = require('@aws-sdk/credential-providers');
65
const Sproxy = require('sproxydclient');

‎lib/storage/data/external/utils.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ const utils = {
4545
const metaObj = obj || {};
4646
Object.keys(metaObj).forEach(key => {
4747
const newKey = key.substring(11);
48-
newObj[newKey] = `${metaObj[key]}`;
48+
newObj[newKey] = string(metaObj[key]);
4949
});
5050
return newObj;
5151
},

‎package.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,7 @@
8888
"scripts": {
8989
"build": "tsc",
9090
"build_doc": "cd documentation/listingAlgos/pics; dot -Tsvg delimiterStateChart.dot > delimiterStateChart.svg; dot -Tsvg delimiterMasterV0StateChart.dot > delimiterMasterV0StateChart.svg; dot -Tsvg delimiterVersionsStateChart.dot > delimiterVersionsStateChart.svg",
91-
"coverage": "export NODE_OPTIONS=\"--tls-max-v1.2\" && nyc --clean jest tests/functional/metadata/mongodb/delObject.spec.js",
91+
"coverage": "export NODE_OPTIONS=\"--tls-max-v1.2\" && nyc --clean jest tests --coverage --testTimeout=120000 --forceExit --testPathIgnorePatterns tests/functional/pykmip",
9292
"dev": "nodemon --ext ts --ignore build --exec \"yarn build\"",
9393
"ft_pykmip_test": "jest tests/functional/pykmip",
9494
"ft_test": "jest tests/functional --testTimeout=120000 --forceExit --testPathIgnorePatterns tests/functional/pykmip",

‎tests/unit/s3routes/routesUtils/responseStreamData.spec.js‎

Lines changed: 10 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ const logger = new werelogs.Logger('test:routesUtils.responseStreamData');
88
const { responseStreamData } = require('../../../../lib/s3routes/routesUtils');
99
const AwsClient = require('../../../../lib/storage/data/external/AwsClient');
1010
const DummyObjectStream = require('../../storage/data/DummyObjectStream');
11+
const { once } = require('../../../../lib/jsutil');
1112

1213
werelogs.configure({
1314
level: 'debug',
@@ -175,16 +176,9 @@ describe('routesUtils.responseStreamData', () => {
175176
});
176177

177178
it('should not leak socket if client closes the connection before ' +
178-
'data backend starts streaming', async () => {
179-
await new Promise((resolve, reject) => {
180-
let called = false;
181-
function safeDone(err) {
182-
if (!called) {
183-
called = true;
184-
if (err) reject(err);
185-
else resolve();
186-
}
187-
}
179+
'data backend starts streaming', done => {
180+
const doneOnce = once(done);
181+
try {
188182
responseStreamData(undefined, {}, {}, [{
189183
key: 'foo',
190184
size: 10000000,
@@ -201,16 +195,14 @@ describe('routesUtils.responseStreamData', () => {
201195
emit: () => {},
202196
write: () => {},
203197
end: () => setTimeout(() => {
204-
try {
205-
const nOpenSockets = Object.keys(awsAgent.sockets).length;
206-
assert.strictEqual(nOpenSockets, 0);
207-
safeDone();
208-
} catch (err) {
209-
safeDone(err);
210-
}
198+
const nOpenSockets = Object.keys(awsAgent.sockets).length;
199+
assert.strictEqual(nOpenSockets, 0);
200+
doneOnce();
211201
}, 1000),
212202
isclosed: true,
213203
}, undefined, logger.newRequestLogger());
214-
});
204+
} catch (err) {
205+
doneOnce(err);
206+
}
215207
});
216208
});

‎tests/unit/storage/data/DummyService.js‎

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -113,18 +113,18 @@ class DummyService {
113113
this.abortMultipartUploadAsync = promisify(this.abortMultipartUpload.bind(this));
114114

115115
this.commandHandlers = new Map([
116-
[PutObjectCommand, (cmd) => this.putObjectAsync(cmd.input)],
117-
[CopyObjectCommand, (cmd) => this.copyObjectAsync(cmd.input)],
118-
[HeadObjectCommand, (cmd) => this.headObjectAsync(cmd.input)],
119-
[PutObjectTaggingCommand, (cmd) => this.putObjectTaggingAsync(cmd.input)],
120-
[DeleteObjectTaggingCommand, (cmd) => this.deleteObjectTaggingAsync(cmd.input)],
121-
[CompleteMultipartUploadCommand, (cmd) => this.completeMultipartUploadAsync(cmd.input)],
116+
[PutObjectCommand, cmd => this.putObjectAsync(cmd.input)],
117+
[CopyObjectCommand, cmd => this.copyObjectAsync(cmd.input)],
118+
[HeadObjectCommand, cmd => this.headObjectAsync(cmd.input)],
119+
[PutObjectTaggingCommand, cmd => this.putObjectTaggingAsync(cmd.input)],
120+
[DeleteObjectTaggingCommand, cmd => this.deleteObjectTaggingAsync(cmd.input)],
121+
[CompleteMultipartUploadCommand, cmd => this.completeMultipartUploadAsync(cmd.input)],
122122
[GetObjectCommand, this._handleGetObject.bind(this)], // Special case - returns stream
123-
[DeleteObjectCommand, (cmd) => this.deleteObjectAsync(cmd.input)],
124-
[CreateMultipartUploadCommand, (cmd) => this.createMultipartUploadAsync(cmd.input)],
125-
[UploadPartCommand, (cmd) => this.uploadPartAsync(cmd.input)],
126-
[ListPartsCommand, (cmd) => this.listPartsAsync(cmd.input)],
127-
[AbortMultipartUploadCommand, (cmd) => this.abortMultipartUploadAsync(cmd.input)],
123+
[DeleteObjectCommand, cmd => this.deleteObjectAsync(cmd.input)],
124+
[CreateMultipartUploadCommand, cmd => this.createMultipartUploadAsync(cmd.input)],
125+
[UploadPartCommand, cmd => this.uploadPartAsync(cmd.input)],
126+
[ListPartsCommand, cmd => this.listPartsAsync(cmd.input)],
127+
[AbortMultipartUploadCommand, cmd => this.abortMultipartUploadAsync(cmd.input)],
128128
]);
129129
}
130130

‎tests/unit/storage/data/external/ExternalClients.spec.js‎

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -150,10 +150,8 @@ describe('external backend clients', () => {
150150
dataStoreName: backend.config.dataStoreName,
151151
response: new stream.PassThrough(),
152152
}, [10000000, 10000050], '');
153-
154153
let data = '';
155154
const streamToRead = readable;
156-
157155
await new Promise((resolve, reject) => {
158156
streamToRead.on('data', (chunk) => {
159157
data += chunk.toString();
@@ -172,13 +170,9 @@ describe('external backend clients', () => {
172170
dataStoreName: backend.config.dataStoreName,
173171
response: new stream.PassThrough(),
174172
}, [10000000, 10000050], '');
175-
176-
177173
const readable = result
178-
179174
let errorHandled = false;
180-
181-
await new Promise((resolve) => {
175+
await new Promise(resolve => {
182176
readable
183177
.once('data', () => readable.emit('error', new Error('OOPS')))
184178
.on('error', err => {

0 commit comments

Comments
 (0)