-
-
Notifications
You must be signed in to change notification settings - Fork 847
core: handle inbox item sync & decryption #8733
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
core: handle inbox item sync & decryption #8733
Conversation
Signed-off-by: 01zulfi <[email protected]>
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.
Looks good overall.
packages/core/src/api/sync/index.ts
Outdated
{ | ||
alg: item.alg, | ||
iv: item.iv, | ||
cipher: item.cipher, | ||
format: "base64", | ||
length: item.length, | ||
salt: "" | ||
} |
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.
Why not pass the item as-is?
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.
salt
is not present in SyncInboxItem
Signed-off-by: 01zulfi <[email protected]>
packages/core/src/api/sync/merger.ts
Outdated
cipher: item.cipher, | ||
format: "base64", | ||
length: item.length, | ||
salt: "" |
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.
Use the salt from the key then.
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 inbox keys? They don't have a salt. Or, do you mean the salt from user's database encryption key?
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.
No from the decryptedKey
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.
decryptedKey
is just a string, I'm not sure how to derive salt from there
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.
How are you decrypting without the salt? We need the salt.
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.
salt
is not passed to the decryption functions at all, that's why I'm getting away with just passing an empty string 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.
Ah I see, you modified KeyUtils.transform
. I don't think that's a good idea. We will need to include the salt from the inbox-api-server
so it can be used 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.
PR to send salt streetwriters/notesnook-sync-server#57
Ah I see, you modified KeyUtils.transform. I don't think that's a good idea.
This is done to accommodate the absence of salt in the key, not sure how to handle this.
Signed-off-by: 01zulfi <[email protected]>
219c4bb
to
53d7ccb
Compare
Signed-off-by: 01zulfi <[email protected]>
Related #8526