Address ObjectStore quality findings

TESTS.md

@@ -28,7 +28,7 @@ The script exits nonzero on failure. The test programs use temporary local direc
 
 ## Disposable Docker cluster tests
 
-Requires Docker with Compose, Python 3, `curl`, and a free local port 9001. Make a test-only environment file from `.env.cluster.example` and fill in all five blank credentials with test-only values. Keep that file private and out of Git.
+Requires Docker with Compose, Python 3.9 or newer, `curl`, and a free local port 9001. Make a test-only environment file from `.env.cluster.example` and fill in all five blank credentials with test-only values. Keep that file private and out of Git.
 
 ```sh
 cp .env.cluster.example /tmp/objectstore-cluster-tests.env

client/examples/awt-images/ImageManager.java

@@ -371,7 +371,9 @@ public final class ImageManager extends Frame {
                     Object value = event.getTransferable().getTransferData(DataFlavor.javaFileListFlavor);
                     List<?> dropped = (List<?>) value;
                     List<java.io.File> files = new ArrayList<>();
-                    for (Object item : dropped) if (item instanceof java.io.File file) files.add(file);
+                    for (Object item : dropped) {
+                        if (item instanceof java.io.File file) files.add(file);
+                    }
                     event.dropComplete(true);
                     EventQueue.invokeLater(() -> uploadImages(files));
                 } catch (Exception error) {
@@ -391,7 +393,10 @@ public final class ImageManager extends Frame {
         Button cancel = new Button("Cancel");
         Button proceed = new Button("Continue");
         cancel.addActionListener(event -> dialog.dispose());
-        proceed.addActionListener(event -> { accepted[0] = true; dialog.dispose(); });
+        proceed.addActionListener(event -> {
+            accepted[0] = true;
+            dialog.dispose();
+        });
         buttons.add(cancel);
         buttons.add(proceed);
         dialog.add(buttons, BorderLayout.SOUTH);

client/src/main/java/cloud/lunarsky/objectstore/client/Json.java

@@ -124,7 +124,10 @@ final class Json {
                     return result.toString();
                 }
                 if (current < 0x20) throw new ProtocolException("Unescaped control character in JSON string");
-                if (current != '\\') { result.append(current); continue; }
+                if (current != '\\') {
+                    result.append(current);
+                    continue;
+                }
                 if (index >= source.length()) throw new ProtocolException("Incomplete JSON escape");
                 char escaped = source.charAt(index++);
                 switch (escaped) {
@@ -159,31 +162,42 @@ final class Json {
         private BigDecimal number() throws ProtocolException {
             int start = index;
             if (take('-') && index >= source.length()) throw new ProtocolException("Invalid JSON number");
+            integerDigits();
+            if (take('.')) requireDigits("Invalid JSON fraction");
+            if (take('e') || take('E')) {
+                if (!take('+')) take('-');
+                requireDigits("Invalid JSON exponent");
+            }
+            try { return new BigDecimal(source.substring(start, index)); }
+            catch (NumberFormatException e) { throw new ProtocolException("Invalid JSON number", e); }
+        }
+
+        private void integerDigits() throws ProtocolException {
             if (take('0')) {
                 if (index < source.length() && Character.isDigit(source.charAt(index)))
                     throw new ProtocolException("Invalid JSON number");
             } else {
                 if (index >= source.length() || source.charAt(index) < '1' || source.charAt(index) > '9')
                     throw new ProtocolException("Invalid JSON number");
-                while (index < source.length() && source.charAt(index) >= '0' && source.charAt(index) <= '9') index++;
+                scanDigits();
             }
-            if (take('.')) {
+        }
+
+        private void requireDigits(String message) throws ProtocolException {
             int first = index;
-                while (index < source.length() && source.charAt(index) >= '0' && source.charAt(index) <= '9') index++;
+            scanDigits();
-                if (first == index) throw new ProtocolException("Invalid JSON fraction");
+            if (first == index) throw new ProtocolException(message);
         }
-            if (take('e') || take('E')) {
+
-                if (!take('+')) take('-');
+        private void scanDigits() {
-                int first = index;
             while (index < source.length() && source.charAt(index) >= '0' && source.charAt(index) <= '9') index++;
-                if (first == index) throw new ProtocolException("Invalid JSON exponent");
-            }
-            try { return new BigDecimal(source.substring(start, index)); }
-            catch (NumberFormatException e) { throw new ProtocolException("Invalid JSON number", e); }
         }
 
         private boolean take(char value) {
-            if (index < source.length() && source.charAt(index) == value) { index++; return true; }
+            if (index < source.length() && source.charAt(index) == value) {
+                index++;
+                return true;
+            }
             return false;
         }
 

scripts/test-cluster-http.py

@@ -22,12 +22,31 @@ port = int(values.get("CLUSTER_HOST_PORT", "9001"))
 if not 1 <= port <= 65535:
     raise ValueError("CLUSTER_HOST_PORT must be between 1 and 65535")
 host = f"127.0.0.1:{port}"
+MAX_RESPONSE_BYTES = 1024 * 1024
 
 
 def sign(key, message):
     return hmac.new(key, message.encode(), hashlib.sha256).digest()
 
 
+def parse_xml(content):
+    if len(content) > MAX_RESPONSE_BYTES:
+        raise ValueError("Unsafe XML response from test server")
+    text = content.decode("utf-8")
+    if "<!DOCTYPE" in text or "<!ENTITY" in text:
+        raise ValueError("Unsafe XML response from test server")
+    parser = ET.XMLParser()
+    parser.feed(text)
+    return parser.close()
+
+
+def read_response(response):
+    content = response.read(MAX_RESPONSE_BYTES + 1)
+    if len(content) > MAX_RESPONSE_BYTES:
+        raise ValueError("Oversized response from test server")
+    return content
+
+
 def request(method, path, body=b"", extra=None):
     extra = extra or {}
     date = datetime.datetime.now(datetime.timezone.utc).strftime("%Y%m%dT%H%M%SZ")
@@ -50,7 +69,7 @@ def request(method, path, body=b"", extra=None):
     try:
         connection.request(method, path, body=body if method in ("PUT", "POST") else None, headers=headers)
         response = connection.getresponse()
-        return response.status, response.read(), response.headers
+        return response.status, read_response(response), response.headers
     finally:
         connection.close()
 
@@ -60,7 +79,7 @@ def anonymous(method, path):
     try:
         connection.request(method, path)
         response = connection.getresponse()
-        return response.status, response.read()
+        return response.status, read_response(response)
     finally:
         connection.close()
 
@@ -75,7 +94,7 @@ if len(sys.argv) > 2 and sys.argv[2] == "acl":
     path = f"/{bucket}/cluster-test/acl-multipart"
     status, content, _ = request("POST", path + "?uploads", extra={"x-amz-acl": "public-read"})
     assert status == 200, (status, content)
-    upload_id = ET.fromstring(content).findtext("UploadId")
+    upload_id = parse_xml(content).findtext("UploadId")
     status, _, headers = request("PUT", path + f"?partNumber=1&uploadId={upload_id}", b"public part")
     assert status == 200, status
     completion = ("<CompleteMultipartUpload><Part><PartNumber>1</PartNumber><ETag>" +
@@ -102,9 +121,9 @@ if len(sys.argv) > 4 and sys.argv[2] == "status":
 if len(sys.argv) > 2 and sys.argv[2] == "version-survivor":
     status, listing, _ = request("GET", "/version-bucket?versions")
     assert status == 200, status
-    root = ET.fromstring(listing)
+    root = parse_xml(listing)
     namespace = {"s3": "http://s3.amazonaws.com/doc/2006-03-01/"}
-    expected_etag = '"' + hashlib.md5(b"older cluster version").hexdigest() + '"'
+    expected_etag = '"' + hashlib.md5(b"older cluster version", usedforsecurity=False).hexdigest() + '"'
     versions = [version for version in root.findall("s3:Version", namespace)
                 if version.findtext("s3:ETag", namespaces=namespace) == expected_etag]
     assert len(versions) == 1, listing
@@ -166,7 +185,7 @@ copy_source = f"/{bucket}/cluster-test/copy-source.txt"
 copy_target = f"/{bucket}/cluster-test/copied.txt"
 body = b"cluster copy and checksum test"
 crc32 = base64.b64encode(zlib.crc32(body).to_bytes(4, "big")).decode()
-md5 = base64.b64encode(hashlib.md5(body).digest()).decode()
+md5 = base64.b64encode(hashlib.md5(body, usedforsecurity=False).digest()).decode()
 status, _, headers = request("PUT", copy_source, body,
                              {"content-type": "text/plain", "content-md5": md5,
                               "x-amz-checksum-crc32": crc32,
@@ -257,7 +276,7 @@ assert status == 200 and b"<DeleteMarker>" in content and old_version.encode() i
 versioned_multipart = "/version-bucket/multipart.txt"
 status, content, _ = request("POST", versioned_multipart + "?uploads")
 assert status == 200, (status, content)
-versioned_upload = ET.fromstring(content).findtext("UploadId")
+versioned_upload = parse_xml(content).findtext("UploadId")
 assert versioned_upload, content
 versioned_part = b"retained multipart version"
 status, _, headers = request("PUT", versioned_multipart +

src/cloud/lunarsky/store/AwsChunkedInputStream.java

@@ -105,25 +105,26 @@ final class AwsChunkedInputStream extends FilterInputStream {
         catch (NumberFormatException error) { throw invalid("Invalid signed chunk size"); }
         if (chunkLeft > decodedLength - decoded) throw invalid("Signed chunks exceed decoded length");
         chunkHash.reset();
-        if (chunkLeft == 0) {
+        if (chunkLeft == 0) finishPayload();
+    }
+
+    private void finishPayload() throws IOException {
         finishChunk();
         if (decoded != decodedLength) throw invalid("Decoded length mismatch");
         if (trailerName == null) {
             if (!line().isEmpty()) throw invalid("Invalid signed chunk ending");
         } else {
+            verifyTrailer();
+        }
+        if (in.read() != -1) throw invalid("Extra bytes after signed payload");
+        finished = true;
+    }
+
+    private void verifyTrailer() throws IOException {
         String trailer = line();
         if (!trailer.startsWith(trailerName + ":")) throw invalid("Missing signed checksum trailer");
         trailerValue = trailer.substring(trailerName.length() + 1);
-                byte[] actual;
+        if (!Base64.getEncoder().encodeToString(trailerChecksum()).equals(trailerValue))
-                if (trailerCrc != null) {
-                    long value = trailerCrc.getValue();
-                    actual = new byte[trailerName.equals("x-amz-checksum-crc64nvme") ? 8 : 4];
-                    for (int i = actual.length - 1; i >= 0; i--) {
-                        actual[i] = (byte) value;
-                        value >>>= 8;
-                    }
-                } else actual = trailerXxhash != null ? trailerXxhash.digest() : trailerHash.digest();
-                if (!Base64.getEncoder().encodeToString(actual).equals(trailerValue))
             throw new StoreException(400, "BadDigest", "Checksum trailer mismatch");
         String signature = line();
         if (!signature.matches("x-amz-trailer-signature=[0-9a-f]{64}"))
@@ -138,9 +139,17 @@ final class AwsChunkedInputStream extends FilterInputStream {
             throw invalid("Trailer signature mismatch");
         if (!line().isEmpty()) throw invalid("Invalid trailer ending");
     }
-            if (in.read() != -1) throw invalid("Extra bytes after signed payload");
+
-            finished = true;
+    private byte[] trailerChecksum() {
+        if (trailerCrc == null)
+            return trailerXxhash != null ? trailerXxhash.digest() : trailerHash.digest();
+        long value = trailerCrc.getValue();
+        byte[] actual = new byte[trailerName.equals("x-amz-checksum-crc64nvme") ? 8 : 4];
+        for (int i = actual.length - 1; i >= 0; i--) {
+            actual[i] = (byte) value;
+            value >>>= 8;
         }
+        return actual;
     }
 
     private void finishChunk() throws IOException {

src/cloud/lunarsky/store/ClientLimits.java

@@ -114,9 +114,21 @@ final class ClientLimits {
         if (exchange.getRemoteAddress().getAddress().isLoopbackAddress() &&
             exchange.getRequestHeaders().get("X-Real-IP") == null &&
             path.equals("/health")) return null;
-        String address = address(exchange);
+        Client client = admit(address(exchange));
+        if (bytesPerSecond > 0) {
+            try {
+                exchange.setStreams(new LimitedInput(exchange.getRequestBody(), client),
+                    new LimitedOutput(exchange.getResponseBody(), client));
+            } catch (RuntimeException error) {
+                leave(client);
+                throw error;
+            }
+        }
+        return client;
+    }
+
+    private synchronized Client admit(String address) {
         Client client;
-        synchronized (this) {
         long now = System.nanoTime();
         if (++admissions % 1024 == 0 || clients.size() >= MAX_CLIENTS)
             clients.entrySet().removeIf(entry -> entry.getValue().inFlight == 0 &&
@@ -136,16 +148,6 @@ final class ClientLimits {
             throw new StoreException(503, "SlowDown", "Client request rate exceeded");
         if (requestsPerSecond > 0) client.requestTokens--;
         client.inFlight++;
-        }
-        if (bytesPerSecond > 0) {
-            try {
-                exchange.setStreams(new LimitedInput(exchange.getRequestBody(), client),
-                    new LimitedOutput(exchange.getResponseBody(), client));
-            } catch (RuntimeException error) {
-                leave(client);
-                throw error;
-            }
-        }
         return client;
     }
 

src/cloud/lunarsky/store/Main.java

@@ -63,41 +63,59 @@ public final class Main {
             admitted = slots.tryAcquire();
             if (!admitted) throw new StoreException(503, "SlowDown", "Too many concurrent requests");
             if (handleStatus(exchange)) return;
+            dispatch(exchange);
+        } catch (StoreException error) {
+            sendStoreError(exchange, error, requestId);
+        } catch (Exception error) {
+            System.err.println("ObjectStore request failed: " + requestId + " " + error.getClass().getSimpleName());
+            sendError(exchange, 500, "InternalError", "Storage operation failed", requestId);
+        } finally {
+            try { exchange.close(); }
+            finally {
+                if (admitted) slots.release();
+                clientLimits.leave(client);
+            }
+        }
+    }
+
+    private void dispatch(HttpExchange exchange) throws IOException {
         SigV4.Verified verified = anonymousRead(exchange)
             ? new SigV4.Verified("UNSIGNED-PAYLOAD", exchange.getRequestURI().getRawQuery(),
                 null, null, null, null, null)
             : authentication.verifyRequest(exchange.getRequestMethod(),
                 exchange.getRequestURI(), exchange.getRequestHeaders());
-            String hash = verified.payload();
-            String principal = verified.principal();
         String path = SigV4.decode(exchange.getRequestURI().getRawPath());
         Map<String, String> query = query(verified.applicationQuery());
         if (path.equals(CAPABILITIES_PATH)) {
-                requireOwner(principal);
+            requireOwner(verified.principal());
             if (!exchange.getRequestMethod().equals("GET")) unsupported("Capability operation");
             if (!query.isEmpty())
                 throw new StoreException(400, "InvalidArgument", "Capability request has unsupported query parameters");
-                requireEmptyBody(exchange, hash);
+            requireEmptyBody(exchange, verified.payload());
             capabilities(exchange);
-            } else if (path.equals("/")) {
+            return;
-                requireOwner(principal);
+        }
+        if (path.equals("/")) {
+            requireOwner(verified.principal());
             if (!exchange.getRequestMethod().equals("GET") ||
                 !(query.isEmpty() || query.size() == 1 && "ListBuckets".equals(query.get("x-id"))))
                 unsupported("Service operation");
-                requireEmptyBody(exchange, hash);
+            requireEmptyBody(exchange, verified.payload());
             listBuckets(exchange);
-            } else {
+            return;
+        }
         int slash = path.indexOf('/', 1);
         String requestedBucket = slash < 0 ? path.substring(1) : path.substring(1, slash);
         if (requestedBucket.isEmpty()) throw new StoreException(404, "NoSuchBucket", "Bucket not found");
         if (slash < 0 || slash == path.length() - 1) {
-                    handleBucket(exchange, query, hash, requestedBucket, principal);
+            handleBucket(exchange, query, verified.payload(), requestedBucket, verified.principal());
         } else {
             store.bucket(requestedBucket);
             handleObject(exchange, path.substring(slash + 1), query, verified, requestedBucket);
         }
     }
-        } catch (StoreException error) {
+
+    private void sendStoreError(HttpExchange exchange, StoreException error, String requestId) throws IOException {
         if (error.status == 503 && error.code.equals("SlowDown"))
             exchange.getResponseHeaders().set("Retry-After", "1");
         if (anonymousRead(exchange) && error.status == 404)
@@ -111,17 +129,6 @@ public final class Main {
         }
         sendError(exchange, error.status, error.code, error.getMessage(), requestId);
     }
-        catch (Exception error) {
-            System.err.println("ObjectStore request failed: " + requestId + " " + error.getClass().getSimpleName());
-            sendError(exchange, 500, "InternalError", "Storage operation failed", requestId);
-        } finally {
-            try { exchange.close(); }
-            finally {
-                if (admitted) slots.release();
-                clientLimits.leave(client);
-            }
-        }
-    }
 
     private static boolean anonymousRead(HttpExchange exchange) {
         if (!exchange.getRequestMethod().equals("GET") &&

src/cloud/lunarsky/store/SigV4.java

@@ -74,10 +74,36 @@ final class SigV4 {
 
     private Verified verifyPresigned(String method, URI uri, Headers headers) {
         if (headers.containsKey("authorization")) denied("Use one authentication method");
+        PresignedQuery query = presignedQuery(uri.getRawQuery());
+        Map<String, String> fields = query.fields();
+        if (!fields.keySet().equals(Set.of("X-Amz-Algorithm", "X-Amz-Credential", "X-Amz-Date",
+                "X-Amz-Expires", "X-Amz-SignedHeaders", "X-Amz-Signature")) ||
+            !"AWS4-HMAC-SHA256".equals(fields.get("X-Amz-Algorithm")))
+            denied("Invalid presigned parameters");
+        String[] credential = credentialScope(fields.get("X-Amz-Credential"));
+        String date = fields.get("X-Amz-Date");
+        validatePresignedTime(date, credential[1], fields.get("X-Amz-Expires"));
+        String signedHeaders = fields.get("X-Amz-SignedHeaders");
+        String canonicalHeaders = canonicalHeaders(headers, signedHeaders, Set.of("host"));
+        String scope = String.join("/", Arrays.copyOfRange(credential, 1, 5));
+        String canonical = method + "\n" + encode(decode(uri.getRawPath()), true) + "\n"
+            + canonicalQuery(query.signed()) + "\n" + canonicalHeaders + "\n"
+            + signedHeaders + "\nUNSIGNED-PAYLOAD";
+        String toSign = "AWS4-HMAC-SHA256\n" + date + "\n" + scope + "\n"
+            + hex(hash(canonical.getBytes(StandardCharsets.UTF_8)));
+        String signature = fields.get("X-Amz-Signature");
+        byte[] key = signingKey(secret(credential[0]), credential[1], region);
+        if (!HEX.matcher(signature).matches() ||
+            !MessageDigest.isEqual(hmac(key, toSign), HexFormat.of().parseHex(signature)))
+            denied("Signature mismatch");
+        return new Verified("UNSIGNED-PAYLOAD", query.application(), key, date, scope, signature, credential[0]);
+    }
+
+    private static PresignedQuery presignedQuery(String rawQuery) {
         Map<String, String> fields = new TreeMap<>();
         StringBuilder application = new StringBuilder();
         StringBuilder signed = new StringBuilder();
-        for (String part : uri.getRawQuery().split("&", -1)) {
+        for (String part : rawQuery.split("&", -1)) {
             String[] pair = part.split("=", 2);
             String name = decode(pair[0]);
             String value = decode(pair.length == 2 ? pair[1] : "");
@@ -89,17 +115,19 @@ final class SigV4 {
                 appendQuery(signed, part);
             }
         }
-        if (!fields.keySet().equals(Set.of("X-Amz-Algorithm", "X-Amz-Credential", "X-Amz-Date",
+        return new PresignedQuery(fields, application.toString(), signed.toString());
-                "X-Amz-Expires", "X-Amz-SignedHeaders", "X-Amz-Signature")) ||
+    }
-            !"AWS4-HMAC-SHA256".equals(fields.get("X-Amz-Algorithm")))
+
-            denied("Invalid presigned parameters");
+    private void validatePresignedTime(String date, String scopeDate, String rawExpires) {
-        String[] credential = credentialScope(fields.get("X-Amz-Credential"));
+        if (!date.matches("[0-9]{8}T[0-9]{6}Z") || !date.startsWith(scopeDate))
-        String date = fields.get("X-Amz-Date");
-        if (!date.matches("[0-9]{8}T[0-9]{6}Z") || !date.startsWith(credential[1]))
             denied("Invalid signing date");
         long expires;
-        try { expires = Long.parseLong(fields.get("X-Amz-Expires")); }
+        try {
-        catch (NumberFormatException error) { denied("Invalid presigned expiry"); return null; }
+            expires = Long.parseLong(rawExpires);
+        } catch (NumberFormatException error) {
+            denied("Invalid presigned expiry");
+            return;
+        }
         if (expires < 1 || expires > 604800) denied("Invalid presigned expiry");
         try {
             Instant start = Instant.from(DATE.parse(date));
@@ -107,22 +135,10 @@ final class SigV4 {
             if (now.isBefore(start.minus(Duration.ofMinutes(5))) || now.isAfter(start.plusSeconds(expires)))
                 denied("Presigned URL has expired or is not yet valid");
         } catch (java.time.DateTimeException error) { denied("Invalid signing date"); }
-        String signedHeaders = fields.get("X-Amz-SignedHeaders");
-        String canonicalHeaders = canonicalHeaders(headers, signedHeaders, Set.of("host"));
-        String scope = String.join("/", Arrays.copyOfRange(credential, 1, 5));
-        String canonical = method + "\n" + encode(decode(uri.getRawPath()), true) + "\n"
-            + canonicalQuery(signed.toString()) + "\n" + canonicalHeaders + "\n"
-            + signedHeaders + "\nUNSIGNED-PAYLOAD";
-        String toSign = "AWS4-HMAC-SHA256\n" + date + "\n" + scope + "\n"
-            + hex(hash(canonical.getBytes(StandardCharsets.UTF_8)));
-        String signature = fields.get("X-Amz-Signature");
-        byte[] key = signingKey(secret(credential[0]), credential[1], region);
-        if (!HEX.matcher(signature).matches() ||
-            !MessageDigest.isEqual(hmac(key, toSign), HexFormat.of().parseHex(signature)))
-            denied("Signature mismatch");
-        return new Verified("UNSIGNED-PAYLOAD", application.toString(), key, date, scope, signature, credential[0]);
     }
 
+    private record PresignedQuery(Map<String, String> fields, String application, String signed) { }
+
     private static boolean hasPresignedQuery(String raw) {
         return raw != null && (raw.startsWith("X-Amz-Algorithm=") || raw.contains("&X-Amz-Algorithm="));
     }