Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Decryption of Large Files/Streams - Update documentation and Guard Test case #1462

Merged
merged 9 commits into from Jun 2, 2022
Merged

Decryption of Large Files/Streams - Update documentation and Guard Test case #1462

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
68c7c29
Illustration of Memory Explosion and Process Crash
Jan 7, 2022
29c0c84
Updated documentation for allowUnauthenticatedStream
Jan 28, 2022
42e402e
allowUnauthenticatedStream implemented on demonstration test
Jan 28, 2022
358b07b
Apply suggestions from code review
larabr Jun 1, 2022
8b17f12
Use password encryption and disable compression to reduce noise
larabr Jun 2, 2022
e8319f9
Trigger stream decryption in all streaming tests
larabr Jun 2, 2022
f3f0403
Uniform input stream generation across all tests
larabr Jun 2, 2022
3b48d7d
Use 1MB instead of 1MiB to allow comparison with previous benchmarks
larabr Jun 2, 2022
db05571
Reduce size and number of chunks to reduce variability below warning …
larabr Jun 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 10 additions & 1 deletion src/config/config.js
View file
Open in desktop
Expand Up @@ -92,8 +92,17 @@ export default {
*/
allowUnauthenticatedMessages: false,
/**
* Allow streaming unauthenticated data before its integrity has been checked.
* Allow streaming unauthenticated data before its integrity has been checked. This would allow the application to
* process very large streams by deferring the checksum integrity check to be only verified at the end of the stream.
larabr marked this conversation as resolved.
Show resolved Hide resolved
*
* This setting is **insecure** if the partially decrypted message is processed further or displayed to the user.
*
* Therefore, the client application is advised to implement at least one of the following strategies when processing
* very large streams.
* - Perform a two-phase commit -- process the data in a staged area, and when the stream finalizes, commit if no error
* occurs.
* - Read the stream twice -- read the contents to the end of the stream, and if the data is verified to be correct,
* then perform a second pass on the stream to perform the real work.
larabr marked this conversation as resolved.
Show resolved Hide resolved
* @memberof module:config
* @property {Boolean} allowUnauthenticatedStream
*/
Expand Down
32 changes: 32 additions & 0 deletions test/benchmarks/memory_usage.js
View file
Open in desktop
Expand Up @@ -213,6 +213,38 @@ class MemoryBenchamrkSuite {
await openpgp.decrypt({ message: encryptedMessage, passwords, config });
});

suite.add('openpgp.encrypt/decrypt (PGP, text @ 100MB, with streaming)', async () => {
Copy link
Collaborator

@larabr larabr Jan 28, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested locally, and as implemented, this test has too large variability in memory usage across runs, due to the key generation part and the compression.
This is why in the other tests we encrypt using passwords, and disable compression altogether.
Could you do the same here? It should suffice to take the code from the MDC, text, with streaming test, and change the input to createMessage, to pass the stream as you are already doing.

await stream.loadStreamsPonyfill();
Copy link
Collaborator

@larabr larabr Jan 31, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For benchmark stability and consistency with the other test, we need to use password encryption and disable compression here too (see other comment).
Also, I don't think we need to decrypt 6GB of data for spotting a regression in this case, 100MB will suffice, and allows comparing the benchmark with the test where allowUnauthenticatedStream is off (aside from that, we do not want to overload the CI too much).


const userID = { name: 'test', email: 'a@b.com' };
const passphrase = 'super long and hard to guess secret';
const { privateKey: rawPrivateKey, publicKey: rawPublicKey } =
await openpgp.generateKey({ userIDs: [userID], format: 'armored', passphrase });

function* largeDataGenerator() {
const chunkSize = 1024 * 1024; /*1MB*/
const numberOfChunks = 100;

for (let chunkNumber = 0; chunkNumber < numberOfChunks; chunkNumber++) {
yield 'a'.repeat(chunkSize);
}
}

const plaintextMessage = await openpgp.createMessage({ text: require('stream').Readable.from(largeDataGenerator()) });
assert(plaintextMessage.fromStream);

const publicKey = await openpgp.readKey({ armoredKey: rawPublicKey });
const armoredEncryptedMessage = await openpgp.encrypt({ message: plaintextMessage, encryptionKeys: [publicKey] });

const encryptedMessage = await openpgp.readMessage({ armoredMessage: armoredEncryptedMessage });

const privateKey = await openpgp.decryptKey({
privateKey: await openpgp.readKey({ armoredKey: rawPrivateKey }),
passphrase
});
await openpgp.decrypt({ message: encryptedMessage, decryptionKeys: privateKey });
});

const stats = await suite.run();
// Print JSON stats to stdout
console.log(JSON.stringify(stats, null, 4));
Expand Down