fix: stop signing a package from allocating several times its size

server/src/main/java/org/eclipse/openvsx/publish/ExtensionVersionIntegrityService.java

@@ -25,8 +25,10 @@
 import org.bouncycastle.asn1.x509.SubjectPublicKeyInfo;
 import org.bouncycastle.crypto.params.AsymmetricKeyParameter;
 import org.bouncycastle.crypto.params.Ed25519PrivateKeyParameters;
+import org.bouncycastle.crypto.params.Ed25519PublicKeyParameters;
 import org.bouncycastle.crypto.signers.Ed25519Signer;
 import org.bouncycastle.crypto.util.PublicKeyFactory;
+import org.bouncycastle.math.ec.rfc8032.Ed25519;
 import org.bouncycastle.openssl.PEMParser;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -72,22 +74,47 @@ public boolean verifyExtensionVersion(TempFile extensionFile, TempFile signature
             var publicKeyInfo = (SubjectPublicKeyInfo) pemParser.readObject();
             publicKeyParameters = PublicKeyFactory.createKey(publicKeyInfo);
         } catch (IOException e) {
-            throw new ErrorResultException("Failed to read private key file", e);
+            throw new ErrorResultException("Failed to read public key file", e);
+        }
+
+        // The PEM above can carry any key type; Ed25519Signer would have thrown a ClassCastException from
+        // inside init() for anything else, which says nothing useful about the file that was handed in.
+        if (!(publicKeyParameters instanceof Ed25519PublicKeyParameters ed25519PublicKey)) {
+            throw new ErrorResultException("Public key file does not hold an Ed25519 public key");
         }
 
         boolean verified;
         try {
-            var signer = new Ed25519Signer();
-            signer.init(false, publicKeyParameters);
-            try (var in = Files.newInputStream(extensionFile.getPath())) {
-                int len;
-                var buffer = new byte[1024];
-                while ((len = in.read(buffer)) > 0) {
-                    signer.update(buffer, 0, len);
-                }
+            // The signature first, and by size before contents: the low-level verify reads a fixed 64
+            // bytes from the offset it is handed, so a truncated signature would come back out of
+            // BouncyCastle as an index out of bounds and a padded one would verify on its first 64 bytes
+            // with the rest ignored. Ed25519Signer made this check itself and answered false, which is
+            // the honest answer - a signature of the wrong length does not verify - and is what the one
+            // caller, the mirror, already refuses the package on.
+            //
+            // Ahead of the package, so that a signature this can already tell is not one costs nothing.
+            // Both files arrive here from a remote registry, and there is no point reading a few hundred
+            // megabytes to check something that has already failed - nor reading a "signature" that is
+            // a few hundred megabytes to find out it is not 64 bytes.
+            var signatureSize = Files.size(signatureFile.getPath());
+            if (signatureSize != Ed25519PrivateKeyParameters.SIGNATURE_SIZE) {
+                // The file, not the version it belongs to: the only caller downloads into a bare
+                // TempFile that carries no FileResource, so there is no extension to name here.
+                logger.warn(
+                        "Signature file {} is {} bytes, expected {}",
+                        signatureFile.getPath(),
+                        signatureSize,
+                        Ed25519PrivateKeyParameters.SIGNATURE_SIZE);
+                return false;
             }
 
-            verified = signer.verifySignature(Files.readAllBytes(signatureFile.getPath()));
+            var signature = Files.readAllBytes(signatureFile.getPath());
+            // One array rather than a read loop into Ed25519Signer, for the reason spelled out on
+            // createSignatureFile below: the streaming signer buffers the whole message anyway, and does
+            // it in a structure that doubles as it grows.
+            var message = Files.readAllBytes(extensionFile.getPath());
+            verified = ed25519PublicKey
+                    .verify(Ed25519.Algorithm.Ed25519, null, message, 0, message.length, signature, 0);
         } catch (IOException e) {
             throw new ErrorResultException("Failed to verify extension file", e);
         }
@@ -144,20 +171,33 @@ public TempFile generateSignature(TempFile extensionFile, SignatureKeyPair keyPa
         return sigzipFile;
     }
 
-    private TempFile createSignatureFile(TempFile extensionFile, SignatureKeyPair keyPair) throws IOException {
+    /**
+     * Signs the package with the key pair's private key.
+     * <p>
+     * Reads it into one array rather than streaming it through {@link Ed25519Signer}, which reads as the
+     * memory-safe option and is the opposite of one. Pure Ed25519 (RFC 8032) hashes the message twice -
+     * once for the nonce, once for the challenge - so a signer cannot discard what it has been fed, and
+     * BouncyCastle's keeps it in a {@code ByteArrayOutputStream} that doubles its capacity as it fills.
+     * Feeding that a 300 MB package peaks at three quarters of a gigabyte, because the 256 MB array and
+     * the 512 MB one being copied into are both live during the last growth - which is why publishing
+     * one used to need a 2 GB heap (see #1450). Read in one go, the cost is the size of the package.
+     * <p>
+     * It is still the size of the package, so an instance running with the integrity service enabled
+     * wants a heap comfortably above {@code ovsx.publishing.max-content-size}. Constant memory is not
+     * reachable from here: every {@code sign} overload BouncyCastle exposes for pure Ed25519 takes a
+     * {@code byte[]}, and its one streaming signer implements Ed25519ph, whose signatures are a
+     * different scheme that nothing verifying these packages today would accept.
+     * <p>
+     * Package-private so that a test can measure what it allocates against the streaming alternative.
+     */
+    TempFile createSignatureFile(TempFile extensionFile, SignatureKeyPair keyPair) throws IOException {
         var privateKeyParameters = new Ed25519PrivateKeyParameters(keyPair.getPrivateKey(), 0);
-        var signer = new Ed25519Signer();
-        signer.init(true, privateKeyParameters);
-        try (var in = Files.newInputStream(extensionFile.getPath())) {
-            int len;
-            var buffer = new byte[1024];
-            while ((len = in.read(buffer)) > 0) {
-                signer.update(buffer, 0, len);
-            }
-        }
+        var message = Files.readAllBytes(extensionFile.getPath());
+        var signature = new byte[Ed25519PrivateKeyParameters.SIGNATURE_SIZE];
+        privateKeyParameters.sign(Ed25519.Algorithm.Ed25519, null, message, 0, message.length, signature, 0);
 
         var signatureFile = new TempFile("signature", ".sig");
-        Files.write(signatureFile.getPath(), signer.generateSignature());
+        Files.write(signatureFile.getPath(), signature);
         return signatureFile;
     }
 

server/src/test/java/org/eclipse/openvsx/publish/ExtensionVersionIntegrityServiceTest.java

@@ -10,10 +10,16 @@
 package org.eclipse.openvsx.publish;
 
 import java.io.IOException;
+import java.lang.management.ManagementFactory;
 import java.nio.file.Files;
+import java.util.Arrays;
+import java.util.Random;
 import java.util.zip.ZipFile;
 
+import com.sun.management.ThreadMXBean;
 import jakarta.persistence.EntityManager;
+import org.bouncycastle.crypto.params.Ed25519PrivateKeyParameters;
+import org.bouncycastle.crypto.signers.Ed25519Signer;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.ExtendWith;
 import org.springframework.beans.factory.annotation.Autowired;
@@ -27,13 +33,15 @@
 import org.eclipse.openvsx.entities.ExtensionVersion;
 import org.eclipse.openvsx.entities.FileResource;
 import org.eclipse.openvsx.entities.Namespace;
+import org.eclipse.openvsx.entities.SignatureKeyPair;
 import org.eclipse.openvsx.migration.GenerateKeyPairJobService;
 import org.eclipse.openvsx.repositories.RepositoryService;
 import org.eclipse.openvsx.util.ArchiveUtil;
 import org.eclipse.openvsx.util.TempFile;
 import org.eclipse.openvsx.util.UUIDService;
 
 import static org.junit.jupiter.api.Assertions.*;
+import static org.junit.jupiter.api.Assumptions.assumeTrue;
 import static org.mockito.Mockito.when;
 
 @ExtendWith(SpringExtension.class)
@@ -104,6 +112,192 @@ void testGenerateSignature() throws IOException {
         }
     }
 
+    /**
+     * A package big enough for the growth pattern below to dominate the measurement, and deliberately not
+     * a power of two: a ByteArrayOutputStream filling up with it ends on a 16 MiB array, having allocated
+     * every smaller one on the way.
+     */
+    private static final int PACKAGE_SIZE = 9 * 1024 * 1024;
+
+    /** How the signing used to work, kept here as the thing the shipped implementation is measured against. */
+    private static byte[] signByStreaming(TempFile packageFile, SignatureKeyPair keyPair) throws IOException {
+        var signer = new Ed25519Signer();
+        signer.init(true, new Ed25519PrivateKeyParameters(keyPair.getPrivateKey(), 0));
+        try (var in = Files.newInputStream(packageFile.getPath())) {
+            int len;
+            var buffer = new byte[1024];
+            while ((len = in.read(buffer)) > 0) {
+                signer.update(buffer, 0, len);
+            }
+        }
+
+        return signer.generateSignature();
+    }
+
+    private static TempFile givenPackageOfSize(int size) throws IOException {
+        var packageFile = new TempFile("package", ".vsix");
+        var content = new byte[size];
+        // Not zeros: an array of them compresses and deduplicates in ways that could flatter one of the
+        // two measurements below, and signing hashes the bytes either way.
+        new Random(1450).nextBytes(content);
+        Files.write(packageFile.getPath(), content);
+        return packageFile;
+    }
+
+    private static long allocatedBy(ThrowingRunnable body) throws Exception {
+        var threads = (ThreadMXBean) ManagementFactory.getThreadMXBean();
+        var id = Thread.currentThread().threadId();
+        var before = threads.getThreadAllocatedBytes(id);
+        body.run();
+        var after = threads.getThreadAllocatedBytes(id);
+        // -1 is what the counter reports when it is not measuring. Skipping beats letting a negative
+        // reading through, which would satisfy the "allocated little" assertion for the wrong reason.
+        assumeTrue(before >= 0 && after >= 0, "JVM stopped reporting per-thread allocation");
+        return after - before;
+    }
+
+    private interface ThrowingRunnable {
+        void run() throws Exception;
+    }
+
+    /**
+     * The signature is the format's, not an implementation detail: reading the package in one go rather
+     * than streaming it into the signer has to produce the very same bytes, or every signature already
+     * published stops matching the one a re-sign would produce.
+     */
+    @Test
+    void signsExactlyAsTheStreamingSignerDid() throws Exception {
+        var keyPair = keyPairService.generateKeyPair();
+        try (var packageFile = givenPackageOfSize(64 * 1024)) {
+            try (var signatureFile = integrityService.createSignatureFile(packageFile, keyPair)) {
+                assertArrayEquals(
+                        signByStreaming(packageFile, keyPair),
+                        Files.readAllBytes(signatureFile.getPath()));
+            }
+        }
+    }
+
+    /**
+     * A signature of the wrong length does not verify - it does not blow up.
+     * <p>
+     * The low-level verify reads a fixed 64 bytes from the offset it is handed, so a truncated file
+     * reaches BouncyCastle as an index out of bounds and a padded one verifies on its first 64 bytes with
+     * the remainder ignored. {@code Ed25519Signer}, which this replaced, checked the length itself and
+     * answered false; the mirror - the only caller - refuses the package on exactly that answer.
+     */
+    @Test
+    void refusesASignatureOfTheWrongLength() throws Exception {
+        var keyPair = keyPairService.generateKeyPair();
+        try (
+                var packageFile = givenPackageOfSize(64 * 1024);
+                var publicKeyFile = givenPublicKeyFile(keyPair);
+                var truncated = new TempFile("signature", ".sig")
+        ) {
+            try (var signatureFile = integrityService.createSignatureFile(packageFile, keyPair)) {
+                var signature = Files.readAllBytes(signatureFile.getPath());
+                Files.write(truncated.getPath(), Arrays.copyOf(signature, signature.length - 1));
+            }
+
+            assertFalse(integrityService.verifyExtensionVersion(packageFile, truncated, publicKeyFile));
+        }
+    }
+
+    /**
+     * And it refuses it without reading the package, which is the order the check has to be in: both
+     * files come from a remote registry, and there is nothing to be gained by pulling a few hundred
+     * megabytes into memory to check something that has already failed.
+     * <p>
+     * Asserted by pointing it at a package that does not exist. Reading it would throw rather than merely
+     * waste the memory, which turns "did not read it" into something a test can see.
+     */
+    @Test
+    void refusesAWrongLengthSignatureWithoutReadingThePackage() throws Exception {
+        var keyPair = keyPairService.generateKeyPair();
+        try (
+                var publicKeyFile = givenPublicKeyFile(keyPair);
+                var truncated = new TempFile("signature", ".sig");
+                var missingPackage = new TempFile("package", ".vsix")
+        ) {
+            Files.write(truncated.getPath(), new byte[63]);
+            Files.delete(missingPackage.getPath());
+
+            assertFalse(integrityService.verifyExtensionVersion(missingPackage, truncated, publicKeyFile));
+        }
+    }
+
+    @Test
+    void verifiesASignatureItProduced() throws Exception {
+        var keyPair = keyPairService.generateKeyPair();
+        try (
+                var packageFile = givenPackageOfSize(64 * 1024);
+                var publicKeyFile = givenPublicKeyFile(keyPair);
+                var signatureFile = integrityService.createSignatureFile(packageFile, keyPair)
+        ) {
+            assertTrue(integrityService.verifyExtensionVersion(packageFile, signatureFile, publicKeyFile));
+        }
+    }
+
+    private static TempFile givenPublicKeyFile(SignatureKeyPair keyPair) throws IOException {
+        var publicKeyFile = new TempFile("public", ".pem");
+        Files.writeString(publicKeyFile.getPath(), keyPair.getPublicKeyText());
+        return publicKeyFile;
+    }
+
+    /**
+     * Why the implementation looks the way it does (#1450). Streaming a package into
+     * {@link Ed25519Signer} looks like the memory-safe choice, but pure Ed25519 hashes the message twice,
+     * so the signer keeps every byte handed to it - in a {@code ByteArrayOutputStream} that doubles as it
+     * fills, allocating each intermediate array on the way. What it costs is therefore a multiple of the
+     * package rather than the package, and for the ~300 MB one in #1450 that was the difference between
+     * fitting in a 1 GB heap and not.
+     * <p>
+     * Measured with per-thread allocation counters, so this is bytes allocated rather than peak live set -
+     * the doubling shows up in both, and only the former can be read off without a heap dump.
+     */
+    @Test
+    void allocatesThePackageOnceWhereStreamingAllocatedItSeveralTimesOver() throws Exception {
+        var threads = ManagementFactory.getThreadMXBean();
+        assumeTrue(
+                threads instanceof ThreadMXBean sunThreads && sunThreads.isThreadAllocatedMemorySupported(),
+                "JVM does not report per-thread allocation");
+
+        // Supported does not mean switched on, and a counter that is off reports -1 rather than failing.
+        // HotSpot enables it by default; a JVM started with it disabled gets it turned on here.
+        var sunThreads = (ThreadMXBean) threads;
+        if (!sunThreads.isThreadAllocatedMemoryEnabled()) {
+            sunThreads.setThreadAllocatedMemoryEnabled(true);
+        }
+        assumeTrue(sunThreads.isThreadAllocatedMemoryEnabled(), "per-thread allocation reporting is off");
+
+        var keyPair = keyPairService.generateKeyPair();
+        try (var packageFile = givenPackageOfSize(PACKAGE_SIZE)) {
+            // Once through each first: class loading and the JIT's own allocations belong to nobody's
+            // measurement, and they only happen the first time round.
+            signByStreaming(packageFile, keyPair);
+            integrityService.createSignatureFile(packageFile, keyPair).close();
+
+            var streaming = allocatedBy(() -> signByStreaming(packageFile, keyPair));
+            var readInOneGo = allocatedBy(() -> integrityService.createSignatureFile(packageFile, keyPair).close());
+
+            System.out.printf(
+                    "signing a %d byte package allocated %d bytes streaming, %d bytes read in one go (%.1fx)%n",
+                    PACKAGE_SIZE,
+                    streaming,
+                    readInOneGo,
+                    (double) streaming / readInOneGo);
+
+            // The shipped path pays for the package and little else.
+            assertTrue(
+                    readInOneGo < PACKAGE_SIZE * 1.5,
+                    "reading in one go allocated " + readInOneGo + " for a " + PACKAGE_SIZE + " byte package");
+            // The streaming one pays for it several times over. Asserted well below the ~3.5x that the
+            // doubling actually costs, so that this fails on a regression rather than on JVM noise.
+            assertTrue(
+                    streaming > PACKAGE_SIZE * 2.0,
+                    "streaming allocated only " + streaming + " for a " + PACKAGE_SIZE + " byte package");
+        }
+    }
+
     @TestConfiguration
     static class TestConfig {
         @Bean