Add sending S/MIME to Secure Compose

[wiktor-k]

// tom edit: close #2124

• minor code updates (will make a separate review for myself)
• add a test - send to single s/mime
• add a test - send to mixed recipients (should err)
• add a test - send to two s/mime (sould err?)
• add a test - attachment (is this supposed to not work yet?)
• add a test - send with broken S/MIME cert - err
• add a test - send with non-S/MIME cert - err
• delete earlier dev files
• manual testing
• evaluate why shows as bytes, not attachment, on recipient end

---

This is a PR adding sending S/MIME to Secure Compose.

This is not yet ready for review.

What works:

• adding S/MIME key if it's missing,
• correctly encrypting content if only S/MIME keys are present

What doesn't work:

• MIME encoding of the encrypted message... it's probably something minor I missed,
• correctly removing own key from encrypted set if only S/MIME recipients are present,
• correctly removing calls to PGP specific methods (that are commented out currently),
• code style issues.

[tomholub]

I know you said not ready for review :)

Later you can make a file like core/smime.ts with class / methods specific to smime, whose methods could (for now, before we abstract one level up) be called from the core/*-pgp.ts files.

It's shaping up really well.

[wiktor-k]

Okay, changes from previous iteration:

• it correctly sends S/MIME encrypted e-mails if only S/MIME recipients are selected,
• for others the old logic is used (OpenPGP),
• cleaned up the bit about detecting keys (missing: Serial ID detection, I used random for now),
• minimized amount of places touched by this code, still there are too many (e.g. extension/chrome/elements/compose-modules/compose-draft-module.ts).

The goal was to seamlessly support adding S/MIME keys and sending S/MIME e-mails while leaving the rest of features un-touched. I think it's mostly done but would like to get your opinion.

Later you can make a file like core/smime.ts with class / methods specific to smime, whose methods could (for now, before we abstract one level up) be called from the core/*-pgp.ts files.

Yes, definitely. Abstracting S/MIME vs OpenPGP would definitely make working with the code more pleasant but my casual look indicated that PGP is interleaved in too many places to do it all at once :( Are there code coverage reports for test somewhere? Usually refactorings (even with static types) can introduce silent bugs and I'd like to avoid that.

[tomholub]

Are there code coverage reports for test somewhere?

Nope, I don't have any such metric. If you are familiar with a way to get such metric, I'd be happy to start tracking it.

To give you an idea, most tests are integration tests, driven through the UI (puppeteer) - testing user scenarios. What that means is that most important functionality will get hit one way or another through these tests, and "hot paths" like storage get hit a lot.

As far as code line coverage.. eh.. 60%? But it's the 60% we really care about.

[tomholub]

Adding comments. These are basically what I'd need in order to feel comfortable merging.

Plus also assuming that the new PgpMsg.Encrypt code gets moved into some sort of Smime class and just called from there.

Otherwise I don't think we need to refactor further in this PR. I could merge it and then the next few PRs would target refactoring and abstracting.

[wiktor-k]

Yep, excellent feedback, that's what I was looking for, thanks! I'll get back with the changes and then we can think of next steps.

[wiktor-k]

Nope, I don't have any such metric. If you are familiar with a way to get such metric, I'd be happy to start tracking it.

It seems the tests cover quite a few critical functions (like sending stuff with/without attachments) so maybe refactoring this part is not as risky as I initially thought... 🤔

[tomholub]

Nope, I don't have any such metric. If you are familiar with a way to get such metric, I'd be happy to start tracking it.

It seems the tests cover quite a few critical functions (like sending stuff with/without attachments) so maybe refactoring this part is not as risky as I initially thought... thinking

I indeed test the compose button quite heavily functionality wise, but only some tests actually look at what was sent - eg if it's sending something unique. Eventually we'll need to dedicate the time to carefully evaluate the messages the tests compose.

I think, after my review, it should be reasonably safe to merge.

[tomholub]

@wiktor-k sorry I managed to miss this becoming ready for review! I'll take a look. Thanks!

[wiktor-k]

No problem, it's still Easter here so I didn't want to push you (yet ;) ).

[tomholub]

Easter here too :) I don't observe holidays much, so normally a nudge doesn't hurt.

[tomholub]

..

[wiktor-k]

add a test - send to mixed recipients (should err)

Yes.

add a test - send to two s/mime (sould err?)

No, it should work. But I never tested this :)

add a test - attachment (is this supposed to not work yet?)

Yes, it won't work but should work in general. I think S/MIME is very similary to PGP/MIME so maybe if we unified this code we would have attachment support "for free".

By the way, why isn't PGP/MIME the default? I thought inline is considered bad. Is this for some kind of backwards-compatibility reasons?

[tomholub]

By the way, why isn't PGP/MIME the default? I thought inline is considered bad. Is this for some kind of backwards-compatibility reasons?

Gmail API screws up the messages, so they don't really come out as PGP/MIME.

I'll try to talk to them to fix it, haven't had the time yet.

[tomholub]

Still want to do this.. sorry for the delay!

[wiktor-k]

Don't worry Tom, when this is blocked I'm doing other stuff and will resume when you're ready 👍

[tomholub]

The notes are all for myself except one where you are mentioned

[tomholub]

I tried to give it a HTTPS cert instead, and it happily encrypts for it 🤔 is this expected behavior? The lib doesn't care? In that case, sounds like this is something we should be checking ourselves? (eg in next PR)

    // todo - unexpectedly works
    // ava.default.only('send non-S/MIME cert - err', testWithBrowser('compose', async (t, browser) => {
    //   const inboxPage = await browser.newPage(t, TestUrls.extensionInbox('test.ci.compose@org.flowcrypt.com'));
    //   let composeFrame = await InboxPageRecipe.openAndGetComposeFrame(inboxPage);
    //   await ComposePageRecipe.fillMsg(composeFrame, { to: 'smime@recipient.com' }, t.title);
    //   const httpsCert = '-----BEGIN CERTIFICATE-----\nMIIFZTCCBE2gAwIBAgISA/LOLnFAcrNSDjMi+PvkSbX1MA0GCSqGSIb3DQEBCwUA\nMEoxCzAJBgNVBAYTAlVTMRYwFAYDVQQKEw1MZXQncyBFbmNyeXB0MSMwIQYDVQQD\nExpMZXQncyBFbmNyeXB0IEF1dGhvcml0eSBYMzAeFw0yMDAzMTQxNTQ0NTVaFw0y\nMDA2MTIxNTQ0NTVaMBgxFjAUBgNVBAMTDWZsb3djcnlwdC5jb20wggEiMA0GCSqG\nSIb3DQEBAQUAA4IBDwAwggEKAoIBAQDBYeT+zyJK4VrAtpBoxnzNrgPMkeJ3WBw3\nlZrO7GXsPUUQL/2uL3NfMwQ4qWqsiJStShaTQ0UX1MQCBgdOY/Ajr5xgyCz4aE0+\nQeReGy+qFyoGE9okVdF+/uJhFTOkK8goA4rDRN3MrSuWsivc/5/8Htd/M01JFAcU\nEblrPkSBtJp8IAtr+QD8etmMd05N0oQFNFT/T7QNrEdItCKSS6jMpprR4phr792K\niQh9MzhZ3O+QEM+UKpsL0dM9C6PD9jNFjFz3EDch/VFPbBlcBfWGvYnjBlqKjhYA\nLPUVPgIF4CVQ60EoOHk1ewyoAyydYyFXppUz1eDvemUhLMWuBJ2tAgMBAAGjggJ1\nMIICcTAOBgNVHQ8BAf8EBAMCBaAwHQYDVR0lBBYwFAYIKwYBBQUHAwEGCCsGAQUF\nBwMCMAwGA1UdEwEB/wQCMAAwHQYDVR0OBBYEFMr4ERxBRtKNI67oIkJHN2QSBptE\nMB8GA1UdIwQYMBaAFKhKamMEfd265tE5t6ZFZe/zqOyhMG8GCCsGAQUFBwEBBGMw\nYTAuBggrBgEFBQcwAYYiaHR0cDovL29jc3AuaW50LXgzLmxldHNlbmNyeXB0Lm9y\nZzAvBggrBgEFBQcwAoYjaHR0cDovL2NlcnQuaW50LXgzLmxldHNlbmNyeXB0Lm9y\nZy8wKQYDVR0RBCIwIIIPKi5mbG93Y3J5cHQuY29tgg1mbG93Y3J5cHQuY29tMEwG\nA1UdIARFMEMwCAYGZ4EMAQIBMDcGCysGAQQBgt8TAQEBMCgwJgYIKwYBBQUHAgEW\nGmh0dHA6Ly9jcHMubGV0c2VuY3J5cHQub3JnMIIBBgYKKwYBBAHWeQIEAgSB9wSB\n9ADyAHcAb1N2rDHwMRnYmQCkURX/dxUcEdkCwQApBo2yCJo32RMAAAFw2e8sLwAA\nBAMASDBGAiEA7Omcf4+uFphcbEq19r4GoWi7E1qvsJTykvgH342x1d4CIQDSCJZK\n3zsVSw8I1GVfnIr/drVhgn4TJgacXx6+gBzfXQB3ALIeBcyLos2KIE6HZvkruYol\nIGdr2vpw57JJUy3vi5BeAAABcNnvK/kAAAQDAEgwRgIhAP7BbIkG/mNclZAVqgA0\nomAB/6xMwbu1ZUsHNBMkZG+QAiEAmZWCVdUfmFs3b+zDEaAF7eFDnz7qbDa5q6M0\n98r8In0wDQYJKoZIhvcNAQELBQADggEBAFaUhUkxGkHc3lxozCbozM7ffAOcK5De\nJGoTtsXw/XmMACBIIqn2Aan+zvQdK/cWV9+dYu5tA/PHZwVbfKAU2x+Fizs7uDgs\nslg16un1/DP7bmi4Ih3KDVyznzgTwWPq9CmPMIeCXBSGvGN4xdfyIf7mKPSmsEB3\ngkM8HyE27e2u8B4f/R4W+sbqx0h5Y/Kv6NFqgQlatEY2HdAQDYYL21xO1ZjaUozP\nyfHQSJwGHp3/1Xdq5mIkV7w9xxhOn64FXp4S0spVCxT3er1EEUurq+lXjyeX4Dog\n1gy3r417NPqQWuBJcA/InSaS/GUyGghp+kuGfIDqVYfQqU1297nThEA=\n-----END CERTIFICATE-----\n';
    //   await pastePublicKeyManually(composeFrame, inboxPage, 'smime@recipient.com', httpsCert);
    //   await composeFrame.waitAndClick('@action-send', { delay: 2 });
    //   await PageRecipe.waitForModalAndRespond(composeFrame, 'error', { contentToCheck: 'dunno', timeout: 40 });
    // }));

[wiktor-k]

I tried to give it a HTTPS cert instead, and it happily encrypts for it thinking is this expected behavior? The lib doesn't care? In that case, sounds like this is something we should be checking ourselves? (eg in next PR)

The library probably doesn't care but we could check it in general because S/MIME certs have "key usage flags" (simiar to E/S/C/A flags for PGP certs). I mean I didn't check if the lib exposes key flags but I hope it does... then we can reject certs that don't have correct flags. Btw interesting test case :)

[tomholub]

Btw interesting test case :)

Users!! :)

[tomholub]

When opening an s/mime message on a client without s/mime (tried Gmail), it just shows the bytes.

I would have expected it to show as an attachment (I vaguely recall trying to open such message before). Can you have a look?

Also, have you tried to open such message in an s/mime client, after integrating it?

We're getting there.

[wiktor-k]

Also, have you tried to open such message in an s/mime client, after integrating it?

Yes, I always tested it with Thunderbird that had my certificate and it just worked:

When opening an s/mime message on a client without s/mime (tried Gmail), it just shows the bytes.
I would have expected it to show as an attachment (I vaguely recall trying to open such message before). Can you have a look?

Maybe we can wrap it in an attachment. It worked in Thunderbird so I didn't bother to try out other options.

We're getting there.

Yes :)

[tomholub]

Approve, we can do sending as attachment in another PR.