feat(#448): support encrypting submission payload#766
feat(#448): support encrypting submission payload#766garethbowen wants to merge 23 commits intomainfrom
Conversation
🦋 Changeset detectedLatest commit: 2e9c7a3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| // ); | ||
| // el3.textContent = this.signature; | ||
| // manifest.appendChild(el3); | ||
| // } |
There was a problem hiding this comment.
This part is still waiting on a decision. I suspect it's going to get removed completely because it's a) optional, and b) not implemented in central.
|
@latin-panda This is ready for review. I'm still waiting on a couple of answers and it'll need a fair amount of QA testing, but as I'm AFK for the next week I thought I'd send it over for review in the meantime. Thanks! |
latin-panda
left a comment
latin-panda
left a comment
There was a problem hiding this comment.
Really interesting! I left some comments below
| readonly instanceId: string, | ||
| readonly symmetricKey: Uint8Array<ArrayBuffer> | ||
| ) { | ||
| const key = CryptoJS.lib.WordArray.create(symmetricKey); |
There was a problem hiding this comment.
Does it make sense to use this function here too?
| const key = CryptoJS.lib.WordArray.create(symmetricKey); | |
| const key = arrayBufferToWordArray(symmetricKey); |
There was a problem hiding this comment.
In this case we don't want to convert the chars to 32-bit because that would corrupt the key, right?
There was a problem hiding this comment.
The cryptoJS algorithm requires the key to be formatted as an array of 32-bit words, right?
If we pass the 8-bit array directly, cryptoJS misinterprets the length and compensates by adding zeros. The arrayBufferToWordArray handles it by grouping the 32 bytes into 8 blocks of 32-bit words without changing the bits. Were you thinking this might cause a mismatch with Central?
There was a problem hiding this comment.
What do you think about adding Alex as a second pair of eyes on these PRs? I think he has evaluated security across the Central layers. Maybe he has some feedback to share 🤔
There was a problem hiding this comment.
Were you thinking this might cause a mismatch with Central?
Yes exactly. We send the symmetric key through to central so it can decrypt the message. I made this change without changing the asymmetric code and the decryption no longer worked. It's fiddly because the asymmetric side cannot be supported by crypto-js so it's using native crypto instead which has a completely different API, so it's very fragile. Do you think I should endeavour to update the asymmetric code to match, or leave this as is?
What do you think about adding Alex as a second pair of eyes on these PRs?
Yes, good idea. I was hoping someone who had implemented encryption in Collect or Enketo would be able to help out but that code hasn't changed for ~7 years and I don't recognise any of the GH usernames. Talking with LN this morning she also recommended Alex so I'll reach out to him.
There was a problem hiding this comment.
Do you think I should endeavour to update the asymmetric code to match, or leave this as is?
Ah, I see. Updating the asymmetric code with extreme care feels like the right thing to do, otherwise it's a big piece of code debt that might sit there for another couple of years. But on the other side, we would need to add backward compatibility for Central (versioning the encryption), right? :(
If you think the risk and time aren't worth it for this PR, maybe we can leave it for later? We should probably add a comment to document it so the context isn't lost
There was a problem hiding this comment.
Ok, I tried again a bit more extensively, and got it to work against central. Thanks!
There was a problem hiding this comment.
There doesn't seem to be any test here that the encrypted content can be de-crypted. Is that expected?
There was a problem hiding this comment.
Yes that is the glaring hole in this PR... The reason is that WF doesn't currently have the testing infrastructure to start a Central instance to test against. I think we should build that out for this and many other tests that would benefit from full e2e testing. However if the code ends up in the central-frontend repo a lot of this work is already done, so I'm delaying it till then. Issue raised to capture this: #775
There was a problem hiding this comment.
What about copy/pasting the central code here? Or cloning the central repo, and running the relevant code directly with node?
There was a problem hiding this comment.
Is there a "submission manifest" for non-encrypted submissions?
There was a problem hiding this comment.
No there's no submission manifest.
Non-encrypted submissions send the instance xml as the payload. Encrypted submissions encrypt the instance xml and attach it as the first attachment, and then use the submission manifest as the payload.
There was a problem hiding this comment.
Got it, thanks 👍
I wonder if that'll be obvious to future readers, or if it would be helpful to mark this file more clearly, e.g. rename as EncryptedSubmissionManifestDefinition, or put in an encryption-specific subdirectory.
| wordArrayToArrayBuffer, | ||
| } from '../../../../../src/lib/client-reactivity/instance-state/quarantine/wordArrayUtils'; | ||
|
|
||
| describe('symmetric encryption', () => { |
There was a problem hiding this comment.
Is it worth adding something like:
describe('encryptAttachment()', () => {
// Currently not tested because X...
});
|
|
||
| it('should produce different ciphertexts for the same input', async () => { | ||
| // ensures attacker cannot figure out what the plaintext is by looking up known encrypted submissions | ||
| const result1 = await getEncryptedSymmetricKey(publicKeyBase64, symmetricKey); |
There was a problem hiding this comment.
| it('should produce different ciphertexts for the same input', async () => { | |
| // ensures attacker cannot figure out what the plaintext is by looking up known encrypted submissions | |
| const result1 = await getEncryptedSymmetricKey(publicKeyBase64, symmetricKey); | |
| // See: | |
| // * https://en.wikipedia.org/wiki/Optimal_asymmetric_encryption_padding | |
| // * https://en.wikipedia.org/wiki/Probabilistic_encryption | |
| it('should produce different ciphertexts for the same input', async () => { | |
| const result1 = await getEncryptedSymmetricKey(publicKeyBase64, symmetricKey); |
There was a problem hiding this comment.
Not sure if this is over the top - perhaps this is all obvious to anyone who should be touching this code?
| }); | ||
|
|
||
| describe('functions are symmetrical', () => { | ||
| [[][0], [0, 1], [0, 1, 2], [0, 1, 2, 3], [0x80], [0xff, 0xff, 0xff, 0xff]].forEach( |
There was a problem hiding this comment.
| [[][0], [0, 1], [0, 1, 2], [0, 1, 2, 3], [0x80], [0xff, 0xff, 0xff, 0xff]].forEach( | |
| [[], [0], [0, 1], [0, 1, 2], [0, 1, 2, 3], [0x80], [0xff, 0xff, 0xff, 0xff]].forEach( |
alxndrsn
left a comment
alxndrsn
left a comment
There was a problem hiding this comment.
As discussed, this code is critical - user data could be lost if there are bugs now or in future. Any bugfixes or other changes are at high risk of introducing regressions. Best to wait until tests can be included in this PR.
3 participants
Closes #448
I have verified this PR works in these browsers (latest versions):
What else has been done to verify that this works as intended?
Minimal testing. I'd really like to add an e2e test one day that actually verifies that the submission can be decrypted, but that requires actually starting up backend. If we end up in the same repo as frontend then there already is a framework to do this so postponing it until we figure out where we're going to go!
Why is this the best possible solution? Were any other approaches considered?
How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?
Do we need any specific form for testing your changes? If so, please attach one.
No specific form but it only works in central (not demo) and only if you set up encryption as laid out in the test plan.
What's changed