[Mime] S/MIME Support #30981

sstok · 2019-04-07T12:56:22Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	no
Fixed tickets	#30875
License	MIT
Doc PR	TODO

This is a heavy work in progress and far from working, I tried to finish this before the end of FOSSA but due to the large complexity of working with raw Mime data it will properly take a week or so (at least I hope so..) to completely finish this. I'm sharing it here for the statistics.

This adds the S/MIME Signer and Encryptor, unlike the Swiftmailer implementation both these functionalities have been separated. When a transporter doesn't support the Raw MIME entity information it will fallback to the original message (without signing/encryption). In any case using a secure connection is always the best guarantee against modification or information disclosure.

src/Symfony/Component/Mime/Security/SMimeSigner.php

sstok · 2019-04-07T13:04:48Z

Any suggestions to clean-up the Header parsing is welcome btw.

I found https://github.com/zbateson/mail-mime-parser but we can't use it as-is, it depends on Guzzle streams and PSR-7. But maybe we can use it's code logic (not this actual library) for the Mime component (in the long run) to parse Mime messages.

sstok · 2019-04-08T17:55:28Z

Good news, due to the way the Mime component handles messages this will be actually easier than I expected 😄

After the message is signed we need to extract the boundary of the envelope and then use this to find the signature, then we can extract the signature, and create a new message (SMimeMesage) which holds the original message (the wrapped message) and signature as a SignedPart (multipart/signed) body. The original message does not have to be reconstructed as this remains unchanged when wrapped by the signing process. It was actually easier than expected, and it only took me two weeks to discover this 🙃

linaori

I might have not been looking properly, is there a test case that shows what happens when the crt/key files do not exist or contain invalid data? It's important to give feedback to the developer if this is the case

src/Symfony/Component/Mime/Security/SMime.php

src/Symfony/Component/Mime/Security/SMimeSigner.php

src/Symfony/Component/Mime/Tests/Security/SMimeSignerTest.php

ostrolucky

You are not deleting these "temp files".
Adding symfony/filesystem dependency is overkill IMHO. tmpfile() and stream_get_meta_data(tmpfile())['uri'] is all you need

src/Symfony/Component/Mime/Security/SMimeEncryptor.php

src/Symfony/Component/Mime/Tests/Security/SMimeEncryptorTest.php

sstok · 2019-04-27T09:53:33Z

What would be safest way to remove these temp files? When you remove them to soon the application will crash, at least for the final message.

The intermediate files can be safely removed after processing as they are no longer needed.

ostrolucky · 2019-04-27T11:56:43Z

in case of tmpfile(), those are removed automatically when there is no longer any reference to it

sstok · 2019-04-28T11:52:42Z

Using tmpfile() seems to work perfectly well, at least on my system 😝

sstok · 2019-05-02T09:51:25Z

Status: Needs review

src/Symfony/Component/Mime/RawMessage.php

fabpot · 2019-05-02T16:03:29Z

src/Symfony/Component/Mime/Security/SMime.php

+    protected function normalizeFilePath(string $path): string
+    {
+        if (!file_exists($path)) {
+            throw new RuntimeException(sprintf('File does not exist: %s', $path));


Could be File "%s" does not exist. (notice the dot at the end of the exception message)

src/Symfony/Component/Mime/WrappedMessage.php

fabpot · 2019-05-02T16:16:52Z

src/Symfony/Component/Mime/WrappedMessage.php

+     */
+    public function __serialize(): array
+    {
+        $this->toString();


Why is it needed?

The SMIME signer/encryptor uses a generator that cannot be serialized. In fact, tmpfile() removes the file after a successful shutdown. Keeping theses files for a longer period would mean extensive growth of temp files.

Alternatively we can store the S/MIME as string, but this will require more memory.

src/Symfony/Component/Mime/WrappedMessage.php

sstok · 2019-06-04T15:24:34Z

There seem to be some changes in the way the SmtpEnvelope is handled, now it expects a Message but the SMimeMessage extends from RawMessage.

Now, given we don't keep the original message it should (as I have said to many times) be possible to use a Message object:

Take the whole message (including attachments) (without the Envelope headers), and convert this to a string (generator), as a SMimePart object.
Create a new Message object with the SMimePart as body and original message Headers

When you sign and then encrypt (or vice-versa), take the Body (SMimePart) and encrypt this, then assign the new SMimePart (containing the encrypted body) as the Message body.

swiftmailer/swiftmailer#1023 introduced "Support for full MIME message wrapping (message/rfc822) as described in RFC5751 section 3.1."

BUT! Interesting enough, no mail-client even supports the enveloped format 😐 (Thunderbird complains the message has no Subject or Sender or To), or I'm doing something wrong in the current implementation? Note that Headers are never encrypted, but signing the headers doesn't work as expected with previous attempts.

Edit. OK, the message/rfc822 header part is in-fact missing, which was causing these "errors" 🤦‍♂

I'm going to experiment a little more with this 10 headed monster, should be done before the feature freeze of Symfony 4.4 right? 🙃

Edit2. message/rfc822 takes the entire message wraps this in a message/rfc822 mime-entity and signs that, the envelope headers are always present in the root entity (Message) but oping an enveloped message shows the message separately. Given this the only difference leys within the body content I'm going to focus on the default version (attached), and letting message/rfc822 be something for another pr.

fabpot · 2019-06-05T02:23:20Z

Right, this would be part of 4.4 :)

sstok · 2019-06-23T15:06:27Z

I finally managed to make this work! I tested it manually with Thunderbird and checked the contents by hand. Everything seems to be good.

In this implementation only the message body is signed/encrypted, the headers are ignored for the S/MIME process itself. The new message has all the "previous" envelope headers and has the S/MIME entity as it's body content (in a raw string format).

To use a fully wrapped message (message/rfc822), you first replace the body with a MessagePart object (taking the message itself) and then pass this for signing. The signer will sign the body (MessagePart) as normal, making the whole message signed 👍

Status: Needs review

fabpot

Looks great to me. I've made a few comments. Also, can you add a note in the CHANGELOG file?

src/Symfony/Component/Mime/Part/SMimePart.php

src/Symfony/Component/Mime/Security/SMime.php

fabpot · 2019-06-29T08:36:05Z

Looks great now. Apart from the small comments I've just made, I think it's ready. Thank you @sstok for your work on this. Can you take the comments into account? I will merge then.

sstok · 2019-06-29T08:53:42Z

Should be good now 👍

fabpot · 2019-06-29T09:04:51Z

Thank you @sstok.

This PR was merged into the 4.4 branch. Discussion ---------- [Mime] S/MIME Support | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes  | BC breaks? | no | Deprecations? | no | Tests pass? | no | Fixed tickets | #30875 | License | MIT | Doc PR | TODO ~~This is a heavy work in progress and far from working, I tried to finish this before the end of FOSSA but due to the large complexity of working with raw Mime data it will properly take a week or so (at least I hope so..) to completely finish this. I'm sharing it here for the statistics.~~ This adds the S/MIME Signer and Encryptor, unlike the Swiftmailer implementation both these functionalities have been separated. When a transporter doesn't support the Raw MIME entity information it will fallback to the original message (without signing/encryption). In any case using a secure connection is always the best guarantee against modification or information disclosure. Commits ------- 6e70d12 [Mime] Added SMimeSigner and Encryptor

nicolas-grekas · 2019-06-30T07:56:30Z

It looks like tests on Windows could need some love (@requires ... annotations maybe):
https://ci.appveyor.com/project/fabpot/symfony/builds/25640752

This PR was squashed before being merged into the 4.4 branch (closes #32317). Discussion ---------- [Mime] Updated some PHPDoc contents | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | no | New feature? | no  | BC breaks? | no  | Deprecations? | no  | Tests pass? | yes  | Fixed tickets | - | License | MIT | Doc PR | not needed While documenting #30981 I found some PHPDoc info not clear enough. This "improves" it, but if you don't like it, it's OK to close this. Thanks. Commits ------- 9432f1f [Mime] Updated some PHPDoc contents

This PR was merged into the 5.4 branch. Discussion ---------- [Mime] regenerate test certificates | Q | A | ------------- | --- | Branch? | 5.4 | Bug fix? | no | New feature? | no | Deprecations? | no | Tickets | - | License | MIT | Doc PR | - Linked to #30981. They will now expire in 2423. Commits ------- 0e5e875 [Mime] regenerate test certificates

carsonbot added Status: Needs Review Feature labels Apr 7, 2019

michaelcullum added the ⭐️ EUFOSSA Hackathon label Apr 7, 2019

fabpot reviewed Apr 7, 2019

View reviewed changes

src/Symfony/Component/Mime/Security/SMimeSigner.php Outdated Show resolved Hide resolved

nicolas-grekas added this to the next milestone Apr 7, 2019

javiereguiluz added the Mime label Apr 8, 2019

sstok force-pushed the 30875-mailer-s-mime-support branch from e1b3613 to 09b5341 Compare April 18, 2019 18:28

sstok marked this pull request as ready for review April 18, 2019 18:28

sstok force-pushed the 30875-mailer-s-mime-support branch from 09b5341 to 86cc9dc Compare April 18, 2019 18:40

carsonbot removed the Status: Needs Review label Apr 19, 2019

linaori suggested changes Apr 19, 2019

View reviewed changes

src/Symfony/Component/Mime/Security/SMime.php Outdated Show resolved Hide resolved

src/Symfony/Component/Mime/Security/SMimeSigner.php Outdated Show resolved Hide resolved

src/Symfony/Component/Mime/Tests/Security/SMimeSignerTest.php Outdated Show resolved Hide resolved

carsonbot added the Status: Needs Work label Apr 19, 2019

sstok force-pushed the 30875-mailer-s-mime-support branch from 86cc9dc to 770a9f8 Compare April 19, 2019 14:34

ostrolucky suggested changes Apr 19, 2019

View reviewed changes

teohhanhui reviewed Apr 25, 2019

View reviewed changes

src/Symfony/Component/Mime/Security/SMimeEncryptor.php Outdated Show resolved Hide resolved

teohhanhui reviewed Apr 25, 2019

View reviewed changes

src/Symfony/Component/Mime/Tests/Security/SMimeEncryptorTest.php Outdated Show resolved Hide resolved

sstok force-pushed the 30875-mailer-s-mime-support branch 2 times, most recently from c994fdf to 37033a3 Compare April 28, 2019 11:52

carsonbot added Status: Needs Review and removed Status: Needs Work labels May 2, 2019

fabpot requested changes May 2, 2019

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels May 2, 2019

fabpot requested changes May 2, 2019

View reviewed changes

sstok force-pushed the 30875-mailer-s-mime-support branch 2 times, most recently from 147d1b7 to cbdd2c6 Compare June 23, 2019 14:23

carsonbot added Status: Needs Review and removed Status: Needs Work labels Jun 23, 2019

fabpot requested changes Jun 29, 2019

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels Jun 29, 2019

sstok force-pushed the 30875-mailer-s-mime-support branch 2 times, most recently from 961a68b to 271cf77 Compare June 29, 2019 08:52

[Mime] Added SMimeSigner and Encryptor

6e70d12

sstok force-pushed the 30875-mailer-s-mime-support branch from 271cf77 to 6e70d12 Compare June 29, 2019 08:53

fabpot mentioned this pull request Jun 29, 2019

Add docs for: [Mime] S/MIME Support symfony/symfony-docs#11850

Closed

fabpot merged commit 6e70d12 into symfony:4.4 Jun 29, 2019

sstok deleted the 30875-mailer-s-mime-support branch June 29, 2019 09:18

javiereguiluz mentioned this pull request Jul 2, 2019

[Mime] Updated some PHPDoc contents #32317

Merged

nicolas-grekas modified the milestones: next, 4.4 Oct 27, 2019

This was referenced Nov 12, 2019

Release v4.4.0-BETA1 #34338

Merged

Release v5.0.0-BETA1 #34339

Merged

nicolas-grekas mentioned this pull request Apr 19, 2023

[Mime] regenerate test certificates #50060

Merged

Uh oh!

[Mime] S/MIME Support #30981

[Mime] S/MIME Support #30981

Uh oh!

Conversation

sstok commented Apr 7, 2019 • edited by javiereguiluz Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

sstok commented Apr 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sstok commented Apr 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

linaori left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ostrolucky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sstok commented Apr 27, 2019

Uh oh!

ostrolucky commented Apr 27, 2019

Uh oh!

sstok commented Apr 28, 2019

Uh oh!

sstok commented May 2, 2019

Uh oh!

Uh oh!

fabpot May 2, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fabpot May 2, 2019

Choose a reason for hiding this comment

Uh oh!

sstok May 8, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sstok commented Jun 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fabpot commented Jun 5, 2019

Uh oh!

sstok commented Jun 23, 2019

Uh oh!

fabpot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fabpot commented Jun 29, 2019

Uh oh!

sstok commented Jun 29, 2019

Uh oh!

fabpot commented Jun 29, 2019

Uh oh!

nicolas-grekas commented Jun 30, 2019

Uh oh!

Uh oh!

sstok commented Apr 7, 2019 •

edited by javiereguiluz

Loading

sstok commented Apr 7, 2019 •

edited

Loading

sstok commented Apr 8, 2019 •

edited

Loading

sstok commented Jun 4, 2019 •

edited

Loading