Sign outgoing messages

[ivan-ushakov]

close#274

---

Tests(delete all except exactly one):

• Tests will be added later (issue create unit tests for composeEmail with signing key #659)

---

To be filled by reviewers

I have reviewed that this PR... (tick whichever items you personally focused on during this review):

• addresses the issue it closes (if any)
• code is readable and understandable
• is accompanied with tests, or tests are not needed
• is free of vulnerabilities
• is documented clearly and usefully, or doesn't need documentation

[ivan-ushakov]

Test are broken because I need to know if I'm moving in the right direction first. Please take a look.

[tomholub]

when it's a WIP you should be marking it as a draft:

[tomholub]

Roughly - see comments

[tomholub]

Does makePassPhraseAlert also check that the pass phrase is correct for the key? (I don't have the code in front of me)

[ivan-ushakov]

No, only requires pass phrase from user.

[tomholub]

Got it. In that case I will need to give this a closer review, to make sure makes sense as implemented.

[ivan-ushakov]

Do you want to check signing key's pass phrase before sending it to TypeScript side?

[tomholub]

Whether we check ahead or not, the important thing here is that it behaves properly (asks user to retry) when the user enters wrong pass phrase. So it's the UX that matters here. Have you tried that scenario?

[ivan-ushakov]

@tomholub
How to know if signature was added to message? I did some debug and Message.prototype.sign is invoked when I provide signingPrv, but I don't see signature in result MIME message. Maybe because we use default compression.

[ivan-ushakov]

@tomholub
We have following code:

 private static cryptoMsgGetSignedBy = async (msg: OpenpgpMsgOrCleartext, keys: SortedKeysForDecrypt) => {
keys.signedBy = Value.arr.unique(await PgpKey.longids(msg.getSigningKeyIds ? msg.getSigningKeyIds() : []));
if (keys.signedBy.length && typeof Store.dbContactGet === 'function') {
const verificationContacts = await Store.dbContactGet(undefined, keys.signedBy);
keys.verificationContacts = verificationContacts.filter(contact => contact && contact.pubkey) as Contact[];
keys.forVerification = [];
for (const contact of keys.verificationContacts) {
const { keys: keysForVerification } = await openpgp.key.readArmored(contact.pubkey!);
keys.forVerification.push(...keysForVerification);
}
}
}

but Store.dbContactGet always empty:

 static dbContactGet = async (db: void, emailOrLongid: string[]): Promise<(Contact | undefined)[]> => {
return [];
}

[tomholub]

@tomholub How to know if signature was added to message? I did some debug and Message.prototype.sign is invoked when I provide signingPrv, but I don't see signature in result MIME message. Maybe because we use default compression.

It's because the message is armored, so you wouldn't see the signature be human-readable.

Open the message in browser extension, and see if it's showing as signed.

[tomholub]

@tomholub We have following code:

 private static cryptoMsgGetSignedBy = async (msg: OpenpgpMsgOrCleartext, keys: SortedKeysForDecrypt) => {
keys.signedBy = Value.arr.unique(await PgpKey.longids(msg.getSigningKeyIds ? msg.getSigningKeyIds() : []));
if (keys.signedBy.length && typeof Store.dbContactGet === 'function') {
const verificationContacts = await Store.dbContactGet(undefined, keys.signedBy);
keys.verificationContacts = verificationContacts.filter(contact => contact && contact.pubkey) as Contact[];
keys.forVerification = [];
for (const contact of keys.verificationContacts) {
const { keys: keysForVerification } = await openpgp.key.readArmored(contact.pubkey!);
keys.forVerification.push(...keysForVerification);
}
}
}

but Store.dbContactGet always empty:

 static dbContactGet = async (db: void, emailOrLongid: string[]): Promise<(Contact | undefined)[]> => {
return [];
}

This is for signature verification - you shouldn't concern yourself with that. Ivan P. is working on that in #274 . For this issue, only signing of messages will be addressed.

[ivan-ushakov]

This is for signature verification - you shouldn't concern yourself with that. Ivan P. is working on that in #274 . For this issue, only signing of messages will be addressed.

OK, so I don't need to write test for this at TypeScript side? Because now I can not test if message was signed.

[ivan-ushakov]

Open the message in browser extension, and see if it's showing as signed.

Here is how signed message looks like in Chrome browser with extension:

[tomholub]

This is for signature verification - you shouldn't concern yourself with that. Ivan P. is working on that in #274 . For this issue, only signing of messages will be addressed.

OK, so I don't need to write test for this at TypeScript side? Because now I can not test if message was signed.

Please file an issue to write a test for this later. After #609 is done, it will be a lot easier to write the test. Thank you.

[tomholub]

Whether we check ahead or not, the important thing here is that it behaves properly (asks user to retry) when the user enters wrong pass phrase. So it's the UX that matters here. Have you tried that scenario?

[tomholub]

Looks correctly implemented, see comments for code improvements.

[tomholub]

There is no need to create a KeyParser type and mock this: this mock can be avoided by instead generating the actual keys with desired properties. That will be a much more direct test of the functionality, and it will simplify production code by removing KeyService.KeyParser which is there only for these tests.

[ivan-ushakov]

Idea was to have pure unit test not integration one. Since we have separate test for the parsing inside Core class there is no need to test it again. Also this makes test slower because parsing and executing TS code takes time. But I can use your approach if you want.

[tomholub]

Thank you - I'm not too worried between a 5ms test and 30ms test. What worries me is this:

https://external-content.duckduckgo.com/iu/?u=https%3A%2F%2Fmedia.giphy.com%2Fmedia%2FsYz9N4FHOLlxC%2Fgiphy.gif&f=1&nofb=1

And also complex production code.

For our codebase, mocks are more appropriate for things that are really heavy like network calls, or situations where it's not possible to easily induce the desired test behavior without a mock.

But for a bit of parsing, it's better to let the production code do its job, even if it's already covered by other tests.

[ivan-ushakov]

I tried to use Core.generateKey but private key always have only one KeyDetails for there is no way to check algorithm completely like I did in my original test. Maybe it is not possible to have several KeyDetails for the one armored private key?

[tomholub]

One armored private key gives you a single KeyDetails, but nothing stops you from generating several private keys. After all, in the app storage, there is one private key per database record, and they all get parsed separately, each producing one KeyDetails structure.

[ivan-ushakov]

OK, looks like my understanding of this was wrong. I changed algorithm once again and modified unit test to work with real keys.

[ivan-ushakov]

Parsed private key could have only one KeyDetails. Am I right here?

[tomholub]

If the private key came from our own storage, then yes - we only store one key per record.

[tomholub]

This looks good. I understand it's just a different coding philosophy, but the sooner we unite this, the better. Thanks!

[tomholub]

If the private key came from our own storage, then yes - we only store one key per record.