-
-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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.
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
All: Add new encryption methods based on native crypto libraries #10696
All: Add new encryption methods based on native crypto libraries #10696
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for working on this!
I've left a few comments.
packages/app-mobile/services/e2ee/NativeEncryption.react-native.ts
Outdated
Show resolved
Hide resolved
import { promisify } from 'util'; | ||
import crypto = require('crypto'); | ||
|
||
class NativeEncryption implements NativeEncryptionInterface { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure the use of a class makes sense here. Is it just to group these functions together? In that case a file crypto.ts
might be preferable with plain functions inside.
Also the fact that it is "native" doesn't really matter, so it should not appear in the name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also the fact that it is "native" doesn't really matter, so it should not appear in the name.
I will remove it.
Is it just to group these functions together? In that case a file
crypto.ts
might be preferable with plain functions inside.
I guess this is the grouped plain functions, right? If so I can rewrite these functions in that way.
https://github.com/laurent22/joplin/blob/v3.0.1/packages/lib/services/e2ee/RSA.node.ts#L11
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have removed the Native
in the name and renamed these modules to Crypto
in commit a2be9f1.
However, I just noticed that the name Crypto
has been used in the Web Crypto API. I guess it's better to use a different name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have removed the class definition in commit 17aa0fd.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As @tessus suggested, I've replaced the name to Krypto
@laurent22 @personalizedrefrigerator I thinks it's ready to be merged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the pull request. I understand this is preparation work, but have you been able to use this for E2EE?
import crypto from 'react-native-quick-crypto'; | ||
import { HashAlgorithm } from 'react-native-quick-crypto/lib/typescript/keys'; | ||
|
||
const Krypto: KryptoInterface = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
In general we don't "brand" filenames to go around naming conflicts. TypeScript provides ways to deal with this when importing.
-
The name should be all lowercase because it's a library of functions (see coding style guide) and should be "crypto.ts"
-
It should default-export this constant. Name it cryptoLib if you want, and then
export default cryptoLib
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Krypto
was my suggestion, since I didn't know whether ts/js had namespaces. Sorry about that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK I will rename them.
return crypto.getHashes(); | ||
}, | ||
|
||
// eslint-disable-next-line @typescript-eslint/no-explicit-any -- TODO: remove the type "any" here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mentioned that the pull request is ready to be merged but surely we can't merge this. You're correct that all "any" should be replaced by proper types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it's good to add react-native-quick-crypto
(or craftzdog/react-native-buffer
) in the dependencies of the whole project, so I can import the Buffer
type in it as RNBuffer
and make a new type NodeBuffer | RNBuffer
for the interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and make a new type NodeBuffer | RNBuffer for the interface.
Ideally the interface would not mention any RN-specific type because it's going to be shared with non-RN code. Don't NodeBuffer and RNBuffer share the same interface? Are there differences? If it's the same, maybe create a new interface that declares just the methods and properties that you need
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've created type CryptoBuffer
for it.
@@ -26,6 +26,8 @@ function shimInit() { | |||
shim.Geolocation = GeolocationReact; | |||
shim.sjclModule = require('@joplin/lib/vendor/sjcl-rn.js'); | |||
|
|||
shim.Krypto = require('../services/e2ee/Krypto.react-native').default; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For new code, all import statements should be at the top.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it.
packages/lib/services/e2ee/types.ts
Outdated
@@ -25,3 +25,14 @@ export interface RSA { | |||
publicKey(rsaKeyPair: RSAKeyPair): string; | |||
privateKey(rsaKeyPair: RSAKeyPair): string; | |||
} | |||
|
|||
export interface KryptoInterface { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please name it "Crypto". We know from the type that it's an interface so it's not necessary to append this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK
@laurent22 @tessus @personalizedrefrigerator Please help review the changes. Thanks! |
7788bc0
to
832e589
Compare
}, | ||
|
||
pbkdf2Raw: async (password: string, salt: CryptoBuffer, iterations: number, keylen: number, digest: string): Promise<CryptoBuffer> => { | ||
const digestMap: { [key: string]: HashAlgorithm } = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again please read the coding style guide, we explicitly ask not to set types this way. See https://joplinapp.org/help/dev/coding_style#avoid-inline-types
I really shouldn't have to keep reminding you to read it. It's understandable to miss a few details here and there but for this PR that's what, the third or fourth time that I remind you to read it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I only checked the method declaration. I will change it later.
'sha512': 'SHA-512', | ||
'ripemd160': 'RIPEMD-160', | ||
}; | ||
const digestAlgorithm: string = digestMap[digest.toLowerCase()] || digest; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also not recommended. See https://joplinapp.org/help/dev/coding_style#dont-set-the-type-when-it-can-be-inferred
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed.
packages/lib/services/e2ee/crypto.ts
Outdated
const digestMap: { [key: string]: string } = { | ||
'sha-1': 'sha1', | ||
'sha-224': 'sha224', | ||
'sha-256': 'sha256', | ||
'sha-384': 'sha384', | ||
'sha-512': 'sha512', | ||
'ripemd-160': 'ripemd160', | ||
}; | ||
const digestAlgorithm: string = digestMap[digest.toLowerCase()] || digest; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really get this part - are the digestMap the list of supported digests? In that case shouldn't that be exposed to the library user? In other words, it seems there should be an enum that tells the list of supported digests.
And digest
should probably have this type.
Also please don't use toLowerCase()
for this - the input digest should be strictly correct and checked at compile time using a type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The digestMap
of pbkdf2Raw()
is for mapping the name according to the implementations. For Node.js
, the hash algorithms are named like sha1
, sha256
while for react-native-quick-crypto
they are named like SHA-1
, SHA-256
. I planned to use the string type for the name, just like how Node.js
does. But now I will change it to a enum because react-native-quick-crypto
only supports a limited set of them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, still a few details but we're getting there. In general please try to use consistent names for things.
For example a map of Cars is a CarMap, not a VehicleMap or an AutomobileMap. OR else you rename car to Vehicle or Automobile too.
Consistent names help with reading the code and searching for strings.
packages/lib/services/e2ee/types.ts
Outdated
export type HashAlgorithm = | ||
| 'SHA-1' | ||
| 'SHA-224' | ||
| 'SHA-256' | ||
| 'SHA-384' | ||
| 'SHA-512' | ||
| 'RIPEMD-160'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you mind using an actual enum for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to use enum but the enum member name cannot contain -
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here are the enum member name styles I tried:
SHA-512
: The style used in react-native-quick-crypto
, but the enum member name cannot contain -
.
sha512
: The style used in node:crypto
, but it is not PascalCase so the linter fails.
Sha512
: Valid enum member name and follows linter rules, but not used by node:crypto
or react-native-quick-crypto
and looks strange.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just use the type used in node:crypto and add the exceptions to eslintrc.js, in the "ENUM" section
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finished in commit f2ed473.
packages/lib/services/e2ee/crypto.ts
Outdated
pbkdf2 as nodePbkdf2, | ||
} from 'crypto'; | ||
|
||
type NodeDigestNameMap = Record<HashAlgorithm, string>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is it actually? A digest or a hash algorithm? Please pick one name. Also everything is "NodeSomething" in this file but that doesn't mean we should prefix all types and variables. It's implied that it's NodeSomething.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is it actually? A digest or a hash algorithm? Please pick one name.
They are almost identical in this context. For node:crypto
and react-native-quick-crypto
, the argument name in pbkdf2()
is digest
but the type of it is HashAlgorithm
. Considering the digest is the result of a hash algorithm I will use I will use digest for all.hashxx
for all cases except the argument name in pbkdf2Raw()
(to match the argument name in node:crypto
and react-native-quick-crypto
).
Also everything is "NodeSomething" in this file but that doesn't mean we should prefix all types and variables. It's implied that it's NodeSomething.
I use the prefix node
to indicate the function is from the node:crypto
rather than my own implementation. If I remove the prefix, the name getCiphers
could refers to the one in node:crypto
and the one in my interface Crypto
. In the upcoming draft I didn't add the node
prefix to functions like createCipheriv
because I won't implement a wrapper of it in the interface Crypto
.
Is this acceptable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finished in commit e4eb58c.
Yes I did. |
@laurent22 I think we have shown that these new encryption methods ( |
@personalizedrefrigerator Hi. I just found that I cannot build the project after merging the current
I tried merging several commits in the |
The error seems to be coming from the Does changing these lines in yarn.lock from "@types/node@npm:^18.0.0":
version: 18.19.33
resolution: "@types/node@npm:18.19.33" to "@types/node@npm:^18.0.0":
version: 18.19.34
resolution: "@types/node@npm:18.19.34" then re-running |
Co-authored-by: Henry Heino <[email protected]>
It works. Thanks |
I think for Case 1~4 they have nothing to do with a specific encryption method. They might be tested elsewhere, but I'm not sure about it. @personalizedrefrigerator I guess I've checked the cases you mentioned. |
@laurent22 @personalizedrefrigerator I thinks it's ready to be merged. |
[EncryptionMethod.SJCL4]: 5000, | ||
[EncryptionMethod.Custom]: 5000, | ||
[EncryptionMethod.KeyV1]: 5000, // Master key is not encrypted by chunks so this value will not be used. | ||
[EncryptionMethod.FileV1]: 131072, // 128k |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(comment because of #9852, my joplin-desktop
now contains 4G data + 8G encrypted data, feel free to ignore!)
for such large chunk size, do you think the encrypted bytes might benefit from a quick zstd compression? i.e. tentatively compress the plaintext before encryptRaw
then set result.compressed = true; result.ct = encryptedCompressed
if smaller, the same for decrypt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if the extra compression step will slow down the encryption/decryption speed. The E2EE on mobile platform is not very fast, so the speed should be taken into consideration.
With that said, even if we don't use compression now, the encrypted file size is decreased in the new encryption methods. (report).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
zstd can reach around 500mb/s with default compression level on desktop, while I can't find a benchmark on mobile, its overhead(a slight regression over huge improvements you brought) should remain manageable
I'm still reading the full report, overhead decrease in file size and master key are promising, thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The speed overhead could be much greater than expected.
For example, the raw AES encryption speed on desktop is over a few GB/s
https://discourse.joplinapp.org/t/performance-test-of-node-js/38762#aes-encryptiondecryption-performance-2
But in the final implementation, the encryption speed is less than 100MB/s
https://discourse.joplinapp.org/t/final-report-of-the-native-encryption-project/40171#p-128759-based-on-commit-edd2a9002a31b0fee4f11ded0a37951543e289ab-release-build-7
Things could be worse on React Native. The raw AES encryption speed could reach 100MB/s. But in the final implementation, the encryption speed is less than 2MB/s
https://discourse.joplinapp.org/t/performance-test-of-react-native-quick-crypto/38622#aes-encryptiondecryption-performance-2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, I almost never edit notes on my phone, but the decryption slowness on first sync was already concerningly impressive..
Ok I think we can merge this then. Many thanks @wh201906 for implementing this important feature, and thanks @personalizedrefrigerator for reviewing and providing support! |
@wh201906, we're getting this random test error on CI:
Do you know what could be the reason? It certainly looks like a value somewhere is stored as a byte, but it's checked here as if it was something else, an integer maybe. What I'm wondering though is could it mean that encryption is faulty due to an integer overflow error? Or is that just the test that is not correct? And I wonder in terms of security if there could be a flaw - for example a number is expected to be a large integer, but it's in fact just a byte. |
@laurent22 I will check it right away. |
The implementation of
It’s just an issue with the test. I'll submit a PR to fix it and add test cases for specific timestamps.
This could happen when dealing with
|
This PR adds some new encryption methods for improved performance and security. Users need to turn on
Use beta encryption
in the sync settings to enable this feature.Code Changes:
react-native-quick-crypto
to theapp-mobile
package.crypto
module, including its type definition and implementations for bothNode.js
andReact Native
.StringV1
,FileV1
andKeyV1
) with AES-256-GCM cipher and better performance.featureFlag
in the sync settings for the new methods.5000
to65536
for string and131072
for files.defaultFileEncryptionMethod_
to handle JS strings and base64 file content separately.EncryptionService.encrypt()
to support async functions.EncryptionService.test.ts
andSynchronizer.e2ee.test.ts
to test the new encryption methods in some test cases.