From fbe6b4dbc1111b37a609cb3f1071cb1911902227 Mon Sep 17 00:00:00 2001 From: Thomas Neidhart Date: Sat, 5 Sep 2026 15:24:43 +0200 Subject: [PATCH 1/3] fix: stop signing a package from allocating several times its size Signing streamed the package into BouncyCastle's Ed25519Signer in 1 KiB chunks, which reads as the memory-safe way to do it and is the opposite. Pure Ed25519 hashes the message twice - once for the nonce, once for the challenge - so a signer cannot discard what it is fed, and BouncyCastle's holds it in a ByteArrayOutputStream that doubles its capacity as it fills. The ~300 MB package in #1450 therefore ended up in a 512 MB array with the 256 MB one it was copied from still live: three quarters of a gigabyte to sign 300 MB, which is why that publish needed the heap raised to 2 GB. The reported OutOfMemoryError comes straight out of that growth, at ByteArrayOutputStream.write -> ensureCapacity -> Arrays.copyOf. Read the file into one exactly sized array and sign that instead. Measured on a 9 MiB package: 33,560,752 bytes allocated streaming, 9,445,000 read in one go - 3.6x down to 1.0x. The signature is unchanged. Ed25519Signer does no more than accumulate and hand the message to Ed25519PrivateKeyParameters.sign, which is what this now calls directly, and a test asserts the two produce identical bytes so that signatures already published keep matching. Verification had the same defect the other way round, on the mirroring path, and is fixed the same way. It also now says so when handed a PEM holding something other than an Ed25519 key, rather than letting a ClassCastException out of Ed25519Signer.init. This is a smaller heap, not a bounded one: it still scales with the package, so an instance with the integrity service enabled wants a heap above ovsx.publishing.max-content-size. Constant memory is not reachable through BouncyCastle - every pure-Ed25519 sign overload takes a byte[], and the one streaming signer implements Ed25519ph, a different scheme. Co-Authored-By: Claude Opus 5 (1M context) --- .../ExtensionVersionIntegrityService.java | 61 ++++++---- .../ExtensionVersionIntegrityServiceTest.java | 115 ++++++++++++++++++ 2 files changed, 154 insertions(+), 22 deletions(-) diff --git a/server/src/main/java/org/eclipse/openvsx/publish/ExtensionVersionIntegrityService.java b/server/src/main/java/org/eclipse/openvsx/publish/ExtensionVersionIntegrityService.java index 9f0592868..19f3178d1 100644 --- a/server/src/main/java/org/eclipse/openvsx/publish/ExtensionVersionIntegrityService.java +++ b/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; @@ -75,19 +77,21 @@ public boolean verifyExtensionVersion(TempFile extensionFile, TempFile signature throw new ErrorResultException("Failed to read private 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); - } - } - - verified = signer.verifySignature(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()); + var signature = Files.readAllBytes(signatureFile.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 +148,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. + *

+ * 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. + *

+ * 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. + *

+ * 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; } diff --git a/server/src/test/java/org/eclipse/openvsx/publish/ExtensionVersionIntegrityServiceTest.java b/server/src/test/java/org/eclipse/openvsx/publish/ExtensionVersionIntegrityServiceTest.java index 651f24672..58427ce37 100644 --- a/server/src/test/java/org/eclipse/openvsx/publish/ExtensionVersionIntegrityServiceTest.java +++ b/server/src/test/java/org/eclipse/openvsx/publish/ExtensionVersionIntegrityServiceTest.java @@ -10,10 +10,15 @@ package org.eclipse.openvsx.publish; import java.io.IOException; +import java.lang.management.ManagementFactory; import java.nio.file.Files; +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,6 +32,7 @@ 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; @@ -34,6 +40,7 @@ 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 +111,114 @@ 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(); + return threads.getThreadAllocatedBytes(id) - 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())); + } + } + } + + /** + * 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. + *

+ * 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"); + + 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 From 11d782eb7ec2d000206a57284c3059c9f9031792 Mon Sep 17 00:00:00 2001 From: Thomas Neidhart Date: Mon, 7 Sep 2026 08:23:08 +0200 Subject: [PATCH 2/3] review: refuse a signature of the wrong length instead of throwing Reading the whole signature and handing it to the low-level verify lost a check the streaming signer was making. Ed25519Signer.verifySignature compares the length against SIGNATURE_SIZE and answers false; the low-level Ed25519PublicKeyParameters.verify reads a fixed 64 bytes from the offset it is given, so a truncated signature came back out of BouncyCastle as "ArrayIndexOutOfBoundsException: last source index 64 out of bounds for byte[63]" and a padded one would have verified on its first 64 bytes with the rest ignored. Check the length and answer false, which is both the honest answer and what the one caller already refuses the package on. A test covers it, and fails with that same exception when the check is taken out. Also from the review: the failure of reading the public key PEM reported "Failed to read private key file", and the allocation test assumed a supported per-thread counter is a running one. It reports -1 when it is not, which would have satisfied the "allocated little" assertion for entirely the wrong reason; the counter is now switched on if it is off, and a negative reading skips rather than passes. Co-Authored-By: Claude Opus 5 (1M context) --- .../ExtensionVersionIntegrityService.java | 19 +++++- .../ExtensionVersionIntegrityServiceTest.java | 58 ++++++++++++++++++- 2 files changed, 75 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/org/eclipse/openvsx/publish/ExtensionVersionIntegrityService.java b/server/src/main/java/org/eclipse/openvsx/publish/ExtensionVersionIntegrityService.java index 19f3178d1..201984b54 100644 --- a/server/src/main/java/org/eclipse/openvsx/publish/ExtensionVersionIntegrityService.java +++ b/server/src/main/java/org/eclipse/openvsx/publish/ExtensionVersionIntegrityService.java @@ -74,7 +74,7 @@ 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 @@ -90,6 +90,23 @@ public boolean verifyExtensionVersion(TempFile extensionFile, TempFile signature // it in a structure that doubles as it grows. var message = Files.readAllBytes(extensionFile.getPath()); var signature = Files.readAllBytes(signatureFile.getPath()); + // Checked here because the low-level verify reads a fixed 64 bytes from the offset it is + // given: 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 used to make this check itself and answer 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. + if (signature.length != 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(), + signature.length, + Ed25519PrivateKeyParameters.SIGNATURE_SIZE); + return false; + } + verified = ed25519PublicKey .verify(Ed25519.Algorithm.Ed25519, null, message, 0, message.length, signature, 0); } catch (IOException e) { diff --git a/server/src/test/java/org/eclipse/openvsx/publish/ExtensionVersionIntegrityServiceTest.java b/server/src/test/java/org/eclipse/openvsx/publish/ExtensionVersionIntegrityServiceTest.java index 58427ce37..ee5e43bcb 100644 --- a/server/src/test/java/org/eclipse/openvsx/publish/ExtensionVersionIntegrityServiceTest.java +++ b/server/src/test/java/org/eclipse/openvsx/publish/ExtensionVersionIntegrityServiceTest.java @@ -12,6 +12,7 @@ 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; @@ -148,7 +149,11 @@ private static long allocatedBy(ThrowingRunnable body) throws Exception { var id = Thread.currentThread().threadId(); var before = threads.getThreadAllocatedBytes(id); body.run(); - return threads.getThreadAllocatedBytes(id) - before; + 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 { @@ -172,6 +177,49 @@ void signsExactlyAsTheStreamingSignerDid() throws Exception { } } + /** + * A signature of the wrong length does not verify - it does not blow up. + *

+ * 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)); + } + } + + @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, @@ -190,6 +238,14 @@ void allocatesThePackageOnceWhereStreamingAllocatedItSeveralTimesOver() throws E 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 From 2fce580fdb7bfa2ef39886ad86f4962721884b23 Mon Sep 17 00:00:00 2001 From: Thomas Neidhart Date: Mon, 7 Sep 2026 09:09:41 +0200 Subject: [PATCH 3/3] review: check the signature before reading the package The length check sat after the package had already been pulled into memory, which in a change about not allocating needlessly is the wrong way round. Read the signature first, and check its size rather than its contents: both files arrive from a remote registry, so 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. The new test points verification at a package that does not exist, so reading it throws rather than merely wasting the memory. That turns "did not read it" into something a test can see: with the package read first it fails with NoSuchFileException. Co-Authored-By: Claude Opus 5 (1M context) --- .../ExtensionVersionIntegrityService.java | 32 +++++++++++-------- .../ExtensionVersionIntegrityServiceTest.java | 23 +++++++++++++ 2 files changed, 42 insertions(+), 13 deletions(-) diff --git a/server/src/main/java/org/eclipse/openvsx/publish/ExtensionVersionIntegrityService.java b/server/src/main/java/org/eclipse/openvsx/publish/ExtensionVersionIntegrityService.java index 201984b54..2136c1c81 100644 --- a/server/src/main/java/org/eclipse/openvsx/publish/ExtensionVersionIntegrityService.java +++ b/server/src/main/java/org/eclipse/openvsx/publish/ExtensionVersionIntegrityService.java @@ -85,28 +85,34 @@ public boolean verifyExtensionVersion(TempFile extensionFile, TempFile signature boolean verified; try { - // 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()); - var signature = Files.readAllBytes(signatureFile.getPath()); - // Checked here because the low-level verify reads a fixed 64 bytes from the offset it is - // given: 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 used to make this check itself and answer 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. - if (signature.length != Ed25519PrivateKeyParameters.SIGNATURE_SIZE) { + // 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(), - signature.length, + signatureSize, Ed25519PrivateKeyParameters.SIGNATURE_SIZE); return false; } + 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) { diff --git a/server/src/test/java/org/eclipse/openvsx/publish/ExtensionVersionIntegrityServiceTest.java b/server/src/test/java/org/eclipse/openvsx/publish/ExtensionVersionIntegrityServiceTest.java index ee5e43bcb..b8cde1dfd 100644 --- a/server/src/test/java/org/eclipse/openvsx/publish/ExtensionVersionIntegrityServiceTest.java +++ b/server/src/test/java/org/eclipse/openvsx/publish/ExtensionVersionIntegrityServiceTest.java @@ -202,6 +202,29 @@ void refusesASignatureOfTheWrongLength() throws Exception { } } + /** + * 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. + *

+ * 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();