fixups post review

Author: benzekrimaha

.github/pykmip/Dockerfile

@@ -16,7 +16,6 @@ RUN apk add --no-cache \
     git clone https://github.com/openkmip/pykmip.git && \
     cd pykmip && \
     pip3 install . && \
-    cd / && \
     apk del .build-deps && \
     rm -rf /var/cache/apk/* /pykmip && \
     mkdir /pykmip

lib/routes/routeVeeam.js

@@ -32,8 +32,19 @@ const apiToAction = {
 const allowedSdkQueryKeys = new Set([
     'x-id',
     'x-amz-user-agent',
-    'x-amz-sdk-invocation-id',
-    'x-amz-sdk-request',
+]);
+
+// Allowed query parameters for SigV4 presigned URLs (lower-cased).
+const allowedPresignQueryKeys = new Set([
+    'x-amz-algorithm',
+    'x-amz-credential',
+    'x-amz-date',
+    'x-amz-expires',
+    'x-amz-signedheaders',
+    'x-amz-signature',
+    'x-amz-security-token',
+    // Used by Veeam UI for delete operations.
+    'tagging',
 ]);
 
 const routeMap = {
@@ -71,38 +82,27 @@ function checkBucketAndKey(bucketName, objectKey, requestQueryParams, method, lo
     }
     if (method !== 'LIST') {
         // Reject any unsupported request, but allow downloads and deletes from UI
-        // Download relies on GETs calls with auth in query parameters, that can be
-        // checked if 'X-Amz-Credential' is included.
-        // Deletion requires that the tags of the object are returned.
+        // Download relies on GETs calls with auth in query parameters, and delete
+        // requires that the tags of the object are returned.
         const originalQuery = requestQueryParams || {};
-        const normalizedQueryKeys = new Map(
-            Object.keys(originalQuery).map(key => [key.toLowerCase(), key])
-        );
-        const isPresignedGet = method === 'GET'
-            && (normalizedQueryKeys.has('x-amz-credential')
-                || normalizedQueryKeys.has('tagging'));
 
-        if (!isPresignedGet && normalizedQueryKeys.size > 0) {
-            const hasUnsupportedKeys = Array.from(normalizedQueryKeys.keys())
-                .some(lowerKey => {
-                    if (allowedSdkQueryKeys.has(lowerKey)) {
-                        return false;
-                    }
-                    if (lowerKey.startsWith('x-amz-sdk-')) {
-                        return false;
-                    }
-                    return true;
-                });
-            if (hasUnsupportedKeys) {
+        for (const [key, value] of Object.entries(originalQuery)) {
+            const normalizedKey = key.toLowerCase();
+
+            // Ensure x-id, when present, matches the expected action for the method.
+            if (normalizedKey === 'x-id' && value !== apiToAction[method]) {
+                return errorInstances.InvalidRequest
+                    .customizeDescription('The Veeam SOSAPI folder does not support this action.');
+            }
+
+            const isAllowedSdkKey = allowedSdkQueryKeys.has(normalizedKey)
+                || normalizedKey.startsWith('x-amz-sdk-');
+            const isAllowedPresignKey = allowedPresignQueryKeys.has(normalizedKey);
+
+            if (!isAllowedSdkKey && !isAllowedPresignKey) {
                 return errorInstances.InvalidRequest
                     .customizeDescription('The Veeam SOSAPI folder does not support this action.');
             }
-        }
-        const xIdOriginalKey = normalizedQueryKeys.get('x-id');
-        const xIdValue = xIdOriginalKey ? originalQuery[xIdOriginalKey] : undefined;
-        if (xIdValue && xIdValue !== apiToAction[method]) {
-            return errorInstances.InvalidRequest
-                .customizeDescription('The Veeam SOSAPI folder does not support this action.');
         }
         if (typeof objectKey !== 'string' || !validObjectKeys.includes(objectKey)) {
             log.debug('invalid object name', { objectKey });

tests/unit/internal/routeVeeam.js

@@ -78,6 +78,57 @@ describe('RouteVeeam: checkBucketAndKey', () => {
         });
     });
 
+    it('should allow SigV4 presigned GET query parameters in mixed case', () => {
+        const err = routeVeeam.checkBucketAndKey(
+            'test',
+            '.system-d26a9498-cb7c-4a87-a44a-8ae204f5ba6c/system.xml',
+            {
+                'X-Amz-Algorithm': 'AWS4-HMAC-SHA256',
+                'X-Amz-Credential': 'cred',
+                'X-Amz-Date': '20240101T000000Z',
+                'X-Amz-Expires': '900',
+                'X-Amz-SignedHeaders': 'host',
+                'X-Amz-Signature': 'signature',
+                'X-Amz-Security-Token': 'token',
+            },
+            'GET',
+            log,
+        );
+        assert.strictEqual(err, undefined);
+    });
+
+    it('should allow SigV4-style query parameters on non-GET when they are presigned', () => {
+        const err = routeVeeam.checkBucketAndKey(
+            'test',
+            '.system-d26a9498-cb7c-4a87-a44a-8ae204f5ba6c/system.xml',
+            {
+                'x-amz-algorithm': 'AWS4-HMAC-SHA256',
+                'x-amz-credential': 'cred',
+                'x-amz-date': '20240101T000000Z',
+                'x-amz-expires': '900',
+                'x-amz-signedheaders': 'host',
+                'x-amz-signature': 'signature',
+            },
+            'DELETE',
+            log,
+        );
+        assert.strictEqual(err, undefined);
+    });
+
+    it('should reject unexpected query parameters even when presigned GET keys are present', () => {
+        const err = routeVeeam.checkBucketAndKey(
+            'test',
+            '.system-d26a9498-cb7c-4a87-a44a-8ae204f5ba6c/system.xml',
+            {
+                'X-Amz-Credential': 'a',
+                extra: 'not-allowed',
+            },
+            'GET',
+            log,
+        );
+        assert.strictEqual(err.is.InvalidRequest, true);
+    });
+
     it('should allow AWS SDK x-id=PutObject query on PUT for system.xml', () => {
         const err = routeVeeam.checkBucketAndKey(
             'test',
@@ -115,6 +166,28 @@ describe('RouteVeeam: checkBucketAndKey', () => {
         );
         assert.strictEqual(err.is.InvalidRequest, true);
     });
+
+    it('should reject mismatched x-id value on GET for system.xml', () => {
+        const err = routeVeeam.checkBucketAndKey(
+            'test',
+            '.system-d26a9498-cb7c-4a87-a44a-8ae204f5ba6c/system.xml',
+            { 'x-id': 'PutObject' },
+            'GET',
+            log,
+        );
+        assert.strictEqual(err.is.InvalidRequest, true);
+    });
+
+    it('should accept x-id with different casing when value matches action', () => {
+        const err = routeVeeam.checkBucketAndKey(
+            'test',
+            '.system-d26a9498-cb7c-4a87-a44a-8ae204f5ba6c/system.xml',
+            { 'X-Id': 'GetObject' },
+            'GET',
+            log,
+        );
+        assert.strictEqual(err, undefined);
+    });
 });
 
 describe('RouteVeeam: checkBucketAndKey', () => {