Desktop,Mobile,Cli: Support accepting shares with a new key format#12829
Conversation
…eb, add more tests
This removes the need for a separate "algorithm" property (which would require changes to Joplin Server).
See inline comments -- this should improve UX during migration if: 1. Some sharer shares a notebook with client A, targeting client A's PPK. 2. Client A's PPK is migrated. 3. Client A tries to accept the share.
5f777f6 to
50c6139
Compare
laurent22
left a comment
There was a problem hiding this comment.
Thanks Henry, that looks good I think. I've left a few comments so please take a look
| generateKeyPair: async () => { | ||
| const keys = new NodeRSA(); | ||
| keys.setOptions(legacyRSAOptions); | ||
| const keySize = 2048; |
There was a problem hiding this comment.
Any reason not to choose a large key size to make it more future proof? I don't assume that would accept performance much?
There was a problem hiding this comment.
Note that this is in the legacyRsa object — it should match the behavior of the old RSA logic. Previously, generateKeyPair took a keySize that was always 2048.
The old generateKeyPair looked like this:
generateKeyPair: async (keySize: number): Promise<RSAKeyPair> => {
const keys = new NodeRSA();
keys.setOptions(nodeRSAOptions);
keys.generateKeyPair(keySize, 65537);
// Sanity check
if (!keys.isPrivate()) throw new Error('No private key was generated');
if (!keys.isPublic()) throw new Error('No public key was generated');
return keys;
},The keySize was inlined into generateKeyPair to allow the new RSA logic to use a keySize of 4096 (while keeping the legacy behavior the same).
| return text.match(/^[a-f0-9]+$/) && text.length % 2 === 0; | ||
| }; | ||
|
|
||
| const modulusLengthBits = 4096; |
There was a problem hiding this comment.
Have you checked the performance of RSA 4096 on mobile? As I understand it can be very slow? And it looks like RSA 2048 is still considered ok till 2030: https://community.rsa.com/s/article/RSA-cryptography-and-NIST-guidance
But it would be good to use 4096 already, provided it doesn't massively impact mobile performance (in my case the app already freezes on startup for 3 seconds, maybe because of encryption/decryption, so I wouldn't want that to becomes 10 seconds+)
There was a problem hiding this comment.
Thank you for pointing this out! That would certainly be good to check.
Regarding using a key size of 2048,
- 2048 results in a plaintext size smaller than Joplin's master keys. According to https://crypto.stackexchange.com/a/42100, a key size of 2048 results in a plaintext size of 190 bytes. Joplin's master keys are 512 bytes hex-encoded, 256 bytes when decoded.
- Note: This was also an issue with the old RSA padding format, at least on mobile. Previously, NodeRSA handled the larger input by breaking the input into chunks (in a way that seems to match ECB mode), and encrypting each chunk individually. On mobile, we previously had support for decrypting input data provided in this format, but not encrypting. As a result, the mobile app could not encrypt Joplin master keys (and thus could not create encrypted shares).
There was a problem hiding this comment.
provided it doesn't massively impact mobile performance
I've added performance logging to ppkTestUtils:
- On an Android emulator, with a key size of 4096, the
loadKeys/encrypt/decryptoperations seem to be reasonably fast (<30ms each).
I plan to test on a physical device later today.
Update:
On a physical device, the previously-tested operations have similar performance.
Performance markers from an Android 13 device, with Joplin in development mode:
:38:27: Performance: ppkTestUtils/loadKeys/shortData--legacy: Start at 7.7s
Logger.js:261 12:38:27: Performance: ppkTestUtils/loadKeys/shortData--legacy: End at 7.7s (took 1.92ms)
Logger.js:261 12:38:27: Performance: ppkTestUtils/decrypt.0/shortData--legacy: Start at 7.7s
Logger.js:261 12:38:27: Performance: ppkTestUtils/decrypt.0/shortData--legacy: End at 7.77s (took 67.48ms)
Logger.js:261 12:38:27: Performance: ppkTestUtils/encrypt/shortData--legacy: Start at 7.77s
Logger.js:261 12:38:27: Performance: ppkTestUtils/encrypt/shortData--legacy: End at 7.79s (took 18.61ms)
Logger.js:261 12:38:27: Performance: ppkTestUtils/decrypt.1/shortData--legacy: Start at 7.79s
Logger.js:261 12:38:28: Performance: ppkTestUtils/decrypt.1/shortData--legacy: End at 7.82s (took 24.11ms)
Logger.js:261 12:38:28: Performance: ppkTestUtils/loadKeys/longData--legacy: Start at 7.82s
Logger.js:261 12:38:28: Performance: ppkTestUtils/loadKeys/longData--legacy: End at 7.82s (took 1.1ms)
Logger.js:261 12:38:28: Performance: ppkTestUtils/decrypt.0/longData--legacy: Start at 7.82s
Logger.js:261 12:38:28: Performance: ppkTestUtils/decrypt.0/longData--legacy: End at 7.91s (took 88.55ms)
Logger.js:261 12:38:28: Performance: ppkTestUtils/encrypt/longData--legacy: Start at 7.91s
Logger.js:261 12:38:28: Performance: ppkTestUtils/encrypt/longData--legacy: End at 7.92s (took 5.55ms)
Logger.js:261 12:38:28: Performance: ppkTestUtils/decrypt.1/longData--legacy: Start at 7.92s
Logger.js:261 12:38:28: Performance: ppkTestUtils/decrypt.1/longData--legacy: End at 7.94s (took 26.69ms)
Logger.js:261 12:38:28: Performance: ppkTestUtils/loadKeys/blockAlignedData--legacy: Start at 7.95s
Logger.js:261 12:38:28: Performance: ppkTestUtils/loadKeys/blockAlignedData--legacy: End at 7.95s (took 1.04ms)
Logger.js:261 12:38:28: Performance: ppkTestUtils/decrypt.0/blockAlignedData--legacy: Start at 7.96s
Logger.js:261 12:38:28: Performance: ppkTestUtils/decrypt.0/blockAlignedData--legacy: End at 8.02s (took 65.98ms)
Logger.js:261 12:38:28: Performance: ppkTestUtils/encrypt/blockAlignedData--legacy: Start at 8.02s
Logger.js:261 12:38:28: Performance: ppkTestUtils/encrypt/blockAlignedData--legacy: End at 8.03s (took 6.73ms)
Logger.js:261 12:38:28: Performance: ppkTestUtils/decrypt.1/blockAlignedData--legacy: Start at 8.03s
Logger.js:261 12:38:28: Performance: ppkTestUtils/decrypt.1/blockAlignedData--legacy: End at 8.06s (took 24.14ms)
Logger.js:261 12:38:28: Performance: ppkTestUtils/loadKeys/shortKey--legacy: Start at 8.06s
Logger.js:261 12:38:28: Performance: ppkTestUtils/loadKeys/shortKey--legacy: End at 8.06s (took 1.05ms)
Logger.js:261 12:38:28: Performance: ppkTestUtils/decrypt.0/shortKey--legacy: Start at 8.06s
Logger.js:261 12:38:28: Performance: ppkTestUtils/decrypt.0/shortKey--legacy: End at 8.07s (took 8.21ms)
Logger.js:261 12:38:28: Performance: ppkTestUtils/encrypt/shortKey--legacy: Start at 8.07s
Logger.js:261 12:38:28: Performance: ppkTestUtils/encrypt/shortKey--legacy: End at 8.07s (took 5.78ms)
Logger.js:261 12:38:28: Performance: ppkTestUtils/decrypt.1/shortKey--legacy: Start at 8.08s
Logger.js:261 12:38:28: Performance: ppkTestUtils/decrypt.1/shortKey--legacy: End at 8.08s (took 8.44ms)
Logger.js:261 12:38:28: Performance: ppkTestUtils/loadKeys/legacyData--legacy: Start at 8.09s
Logger.js:261 12:38:28: Performance: ppkTestUtils/loadKeys/legacyData--legacy: End at 8.09s (took 1ms)
Logger.js:261 12:38:28: Performance: ppkTestUtils/decrypt.0/legacyData--legacy: Start at 8.09s
Logger.js:261 12:38:28: Performance: ppkTestUtils/decrypt.0/legacyData--legacy: End at 8.11s (took 25.31ms)
Logger.js:261 12:38:28: Performance: ppkTestUtils/encrypt/legacyData--legacy: Start at 8.12s
Logger.js:261 12:38:28: Performance: ppkTestUtils/encrypt/legacyData--legacy: End at 8.12s (took 6.15ms)
Logger.js:261 12:38:28: Performance: ppkTestUtils/decrypt.1/legacyData--legacy: Start at 8.12s
Logger.js:261 12:38:28: Performance: ppkTestUtils/decrypt.1/legacyData--legacy: End at 8.15s (took 23.17ms)
Logger.js:261 12:38:28: Performance: ppkTestUtils/loadKeys/v2Data--rsa-v2: Start at 8.15s
Logger.js:261 12:38:28: Performance: ppkTestUtils/loadKeys/v2Data--rsa-v2: End at 8.16s (took 14.11ms)
Logger.js:261 12:38:28: Performance: ppkTestUtils/decrypt.0/v2Data--rsa-v2: Start at 8.17s
Logger.js:261 12:38:28: Performance: ppkTestUtils/decrypt.0/v2Data--rsa-v2: End at 8.21s (took 47.62ms)
Logger.js:261 12:38:28: Performance: ppkTestUtils/encrypt/v2Data--rsa-v2: Start at 8.21s
Logger.js:261 12:38:28: Performance: ppkTestUtils/encrypt/v2Data--rsa-v2: End at 8.22s (took 6.1ms)
Logger.js:261 12:38:28: Performance: ppkTestUtils/decrypt.1/v2Data--rsa-v2: Start at 8.22s
Logger.js:261 12:38:28: Performance: ppkTestUtils/decrypt.1/v2Data--rsa-v2: End at 8.25s (took 29.57ms)
Logger.js:261 12:38:28: Performance: ppkTestUtils/loadKeys/v2DataWebHexadecimal--rsa-v2: Start at 8.25s
Logger.js:261 12:38:28: Performance: ppkTestUtils/loadKeys/v2DataWebHexadecimal--rsa-v2: End at 8.26s (took 1.67ms)
Logger.js:261 12:38:28: Performance: ppkTestUtils/decrypt.0/v2DataWebHexadecimal--rsa-v2: Start at 8.26s
Logger.js:261 12:38:28: Performance: ppkTestUtils/decrypt.0/v2DataWebHexadecimal--rsa-v2: End at 8.29s (took 34.74ms)
Logger.js:261 12:38:28: Performance: ppkTestUtils/encrypt/v2DataWebHexadecimal--rsa-v2: Start at 8.29s
Logger.js:261 12:38:28: Performance: ppkTestUtils/encrypt/v2DataWebHexadecimal--rsa-v2: End at 8.3s (took 2.26ms)
Logger.js:261 12:38:28: Performance: ppkTestUtils/decrypt.1/v2DataWebHexadecimal--rsa-v2: Start at 8.3s
Logger.js:261 12:38:28: Performance: ppkTestUtils/decrypt.1/v2DataWebHexadecimal--rsa-v2: End at 8.33s (took 29.65ms)
Logger.js:261 12:38:33: Performance: ppkTestUtils/loadKeys/local data--rsa-v2: Start at 13.22s
Logger.js:261 12:38:33: Performance: ppkTestUtils/loadKeys/local data--rsa-v2: End at 13.22s (took 2.43ms)
Logger.js:261 12:38:33: Performance: ppkTestUtils/decrypt.0/local data--rsa-v2: Start at 13.22s
Logger.js:261 12:38:33: Performance: ppkTestUtils/decrypt.0/local data--rsa-v2: End at 13.25s (took 33.58ms)
Logger.js:261 12:38:33: Performance: ppkTestUtils/encrypt/local data--rsa-v2: Start at 13.25s
Logger.js:261 12:38:33: Performance: ppkTestUtils/encrypt/local data--rsa-v2: End at 13.26s (took 2.14ms)
Logger.js:261 12:38:33: Performance: ppkTestUtils/decrypt.1/local data--rsa-v2: Start at 13.26s
Logger.js:261 12:38:33: Performance: ppkTestUtils/decrypt.1/local data--rsa-v2: End at 13.29s (took 29.41ms)
However, at least in development mode, the generateKey operation (which runs during sync, when syncInfo.ppk is unset and when resetting the master key password) is very slow (trials: 12.69s, 3.46s, 8.16s, 7.11s).
Update 2:
generateKey also seems to be slow in release mode on the same Android device (trials: 3.9s, 2.66s, 4.15s, 7.26s, 4.66s, 4.45, 8.64, 9.86).
| private textToBuffer_(textUtf8: string) { | ||
| // To avoid data loss when restoring (whether everything is capital or lowercase), work only | ||
| // with lowercase hexadecimal. | ||
| const isHex = isLowercaseHexadecimalString(textUtf8); |
There was a problem hiding this comment.
I'm not sure I understand the logic here - is that for compatibility with the previous version? Which means some data is hex and some is not?
There was a problem hiding this comment.
Currently, Joplin seems to only provide non-hex-encoded data during tests (assuming that Joplin master keys are always hex-encoded).
An alternative to this might be to:
throwwhen provided non-hex-encoded data, both here and in the legacy RSA implementation. This would help ensure that the input data remains hexadecimal encoded in the future.- Add tests and/or runtime assertions elsewhere to help ensure that Joplin master keys remain hex-encoded in the future (even if the legacy RSA implementation is still in-use).
- Without hexadecimal decoding, Joplin master keys are larger than the maximum plaintext size (see notes above).
- Rename RsaLegacy -> RsaV1 - Add note that RsaV2 can block the mobile app UI when generating keys, but might not after migrating to the New Architecture. - Disable key generation with RsaV2 in the mobile app startup tests. - Verify that key generation is tested with whatever public key algorithm is the default in the mobile app startup tests.
During pull request review, it was suggested that an RSA-OAEP-based encryption option be provided that uses a length-2048 key (due to the performance overhead of generating length-4096 keys). This commit adds such an encryption scheme as "RsaV2". The old RsaV2 has been renamed to "RsaV3". As before, migrations to both RsaV2 and RsaV3 are disabled. Possible concerns: - See comment at the top of "LongDataWrapper.ts". - MDN, referring to the key size: "Some organizations are now recommending that it should be 4096." - https://developer.mozilla.org/en-US/docs/Web/API/RsaHashedKeyGenParams#moduluslength
…/node version" This reverts commit d0335f5.
|
I think we can go ahead and merge this, as soon as the conflicts are fixed |
Summary
This pull request:
ppkfield ininfo.jsonto a new key format (RSA-OAEP-4096). Note: This migration logic has automated tests, but is disabled.New key formats (currently disabled)
This pull request allows sharing folders with accounts that use RSA-OAEP for public key encryption. Logic is also added to migrate existing users to this new key format, but is currently disabled (requires a migration step — see note above).
OWASP recommends using elliptical curve cryptography (ECC), where possible, over RSA for asymmetric encryption. However, Joplin’s current mobile cryptographic dependencies (e.g. react-native-quick-crypto) support RSA, but don't seem to support ECC for data encryption/decryption. OWASP also states that “if ECC is not available and RSA must be used, then ensure that the key is at least 2048 bits.”
The parameters for RSA-OAEP can be found in
WebCryptoRsa.ts:.RsaV2: 2048.RsaV3: 4096[1, 0, 1]).These parameters are based on the RSA DOM example and parameters suggested in the MDN web documentation, which recommend a key size of either at least 2048 or 4096 bits, a public exponent of 65537, and a
hashof one ofsha-256,sha-384, orsha-512.Key size notes:
.RsaV2, this pull request breaks the input data into blocks and encrypts/decrypts each block individually. This is the same as the approach taken by NodeRSA's long data handler. Unlike NodeRSA, this pull request limits the maximum number of blocks to two. See comments at the top ofLongDataWrapper.tsfor possible concerns with this approach.This part of the pull request should be reviewed very carefully before being globally enabled (which would be done in a separate pull request).
Testing plan
self@localhost,self2@localhost, andself3@localhost.self@localhosttoself2@localhost.self2@localhostand verify that a share invitation banner is visible, then quit Joplin.RSAv2migration and recompile.self3@localhost(in a different profile), sign in to Joplin Server if not done so already.self2@localhost, share a folder withself3@localhostand sync.self1@localhost, share a folder withself3@localhostand sync.self3@localhost, accept the share fromself2@localhost.RSAv2migration and recompile.It has been verified that the integration tests (see ppkTestUtils.ts) pass on Android 13 and on web (in Chrome).
Footnotes
https://crypto.stackexchange.com/a/42100 ↩