Skip to content

[MimePgp] Add the component #59372

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

Open
wants to merge 1 commit into
base: 7.3
Choose a base branch
from
Open

[MimePgp] Add the component #59372

wants to merge 1 commit into from

Conversation

the-pulli
Copy link

Q A
Branch? 7.3
Bug fix? no
New feature? yes
Deprecations? no
Issues None
License MIT

Hopefully closes PR #50222 as obsolete.

I tried to refactor the old code base to match the Symfony style.

Comments and suggestions are welcome. This is my first major contribution to a large OS software. So I probably have a lot to learn 🤓

Copy link
Member

@GromNaN GromNaN left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add to the description a code example of how the component will be used. This is useful for the review and then for the documentation.

@the-pulli
Copy link
Author

the-pulli commented Jan 6, 2025

@GromNaN, here is an example:

<?php

require_once __DIR__.'/vendor/autoload.php';

use Symfony\Component\Mailer\Mailer;
use Symfony\Component\Mailer\Transport;
use Symfony\Component\Mime\Address;
use Symfony\Component\Mime\Email;
use Symfony\Component\Pgp\PgpEncrypter;

$transport = Transport::fromDsn('valid_dsn');

$mailer = new Mailer($transport);

$email = (new Email())
    ->from(new Address('the@pulli.dev', 'PuLLi'))
    ->to(new Address('fabien@symfony.com', 'Fabien Potencier'))
    ->text("Hello there!\n\nHow are you?")
    ->subject('PGP Mail')
    ->attachFromPath('SomeFile.pdf');

$email2 = (new Email())
    ->from(new Address('the@pulli.dev', 'PuLLi'))
    ->to(new Address('fabien@symfony.com', 'Fabien Potencier'))
    ->text("Hello there again!\n\nHow are you?")
    ->subject('PGP Mail II')
    ->attachFromPath('SomeFile.pdf');

$transport = Transport::fromDsn('valid_dsn');
$mailer = new Mailer($transport);

$pgpEncryptor = new PGPEncrypter();

// only needed if you have more than one key belonging to the from-email address
// for example the@pulli.dev has two keys for the same email address (not recommended):
// B23EF01A is the identifier for key 1
// B23EF01B is the identifier for key 2
$pgpEncryptor->signingKey('B23EF01A');

// method arguments: Message $message, ?string $passphrase, bool $attachKey
$messageSigned = $pgpEncryptor->sign($email, 'signing_key_passphrase', true);

// method arguments: Message $message, ?string $passphrase, bool $attachKey
$messageEncryptedAndSigned = $pgpEncryptor->encryptAndSign($email, 'signing_key_passphrase', true);

// method arguments: Message $message, bool $attachKey
$messageEncrypted = $pgpEncryptor->encrypt($email, true);

$mailer->send($messageSigned);
$mailer->send($messageEncryptedAndSigned);
$mailer->send($messageEncrypted);

// Second Mail
$messageEncrypted = $pgpEncryptor->encrypt($email2, true);

// Problem: Properties `$key`, `$signedPart`, `$signature` on PGPEncryptor are still set to the values from `$mail` or the setter method.

// Solution: as @Jean-Beru suggested, using a unique service to remove all state from `PGPEncrypter`
$pgpEncryptor = new PGPEncrypter();
$messageSigned = $pgpEncryptor->sign($email, 'B23EF01A', 'signing_key_passphrase', true);

Copy link
Member

@stof stof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be usable with TemplatedEmail, we should probably also define a listener in symfony/mailer (similar to what #58501 does for the DkimSigner, and with proper event listener priorities to make both feature work together), so that the message can be encrypted after template rendering instead of encrypting the message before the call to Mailer::send as done in your example (which won't work when using a TemplatedEmail for which the body is rendered only during sending)

@the-pulli
Copy link
Author

To be usable with TemplatedEmail, we should probably also define a listener in symfony/mailer (similar to what #58501 does for the DkimSigner, and with proper event listener priorities to make both feature work together), so that the message can be encrypted after template rendering instead of encrypting the message before the call to Mailer::send as done in your example (which won't work when using a TemplatedEmail for which the body is rendered only during sending)

@stof Yes, I came across this issue while testing. But I didn't find a workaround. Your suggested approach should be the solution.

Copy link
Contributor

@Jean-Beru Jean-Beru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice component ❤️

I'm working on a project where we need to decrypt/encrypt files using PGP. IMO, this component should propose an API to do it with simple strings.

* @throws KeyNotFoundException
* @throws GeneralException
*/
public function encrypt(Message $message, bool $attachKey = false): Message
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should encryption be limited to Message objects? It might be interesting to open it to string to allow file encryption for example.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Jean-Beru Thanks, for the compliment. It's actually not a problem to encrypt/decrypt strings/files with pgp. I've done that too in a project of mine using the pear/crypt_gpg package.

@OskarStark OskarStark changed the title First draft of symfony/pgp [Pgp] Add the component Jan 10, 2025
@GromNaN GromNaN changed the title [Pgp] Add the component [MimePgp] Add the component Jan 19, 2025
Copy link
Member

@GromNaN GromNaN left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 todo left to be decided.

@the-pulli
Copy link
Author

With the latest commits I refactored the PgpSigningTrait to contain only one method now. After finding a flaw with signing the message while attaching the key. I went down the road what is really necessary to create a valid signature. That is normalize the line endings and and add a CR LF at the end of the body to sign. What I was doing before, was adding CR LF before the boundary endings of the text parts, which isn't necessary at all.
I think now the Trait is more clear and concise. I also tried to work all your other comments in the commits. If I left something out, just let me know!

@GromNaN GromNaN requested review from GromNaN and removed request for GromNaN February 5, 2025 16:37
@stof stof mentioned this pull request Feb 18, 2025
@Spomky Spomky force-pushed the 7.3 branch 3 times, most recently from 118454c to 86d072a Compare February 20, 2025 16:22
@GromNaN GromNaN requested review from GromNaN and removed request for GromNaN February 20, 2025 16:33
@Spomky
Copy link
Contributor

Spomky commented Feb 20, 2025

Hi there,

@the-pulli and I spent time on this PR today and we pushed some modifications.

  1. This component does not need any external libraries but the symfony/process
  2. The cryptographic operations are split into the PgpSigner and PgpEncrypter classes. This allows the developpers to perform one, the other or both operations before sending the email.
  3. Similarily to [Mailer] Add configuration for dkim and smime signers #58501, there is a PgpSignerListener to globally sign all emails. The Encryption listener is not implemented though because I am not sure it is a good idea, but still possible in the end.

All classes but the signer and encrypter are marked as internal as they are not supposed to be used outside this component.

Usage:

$signer = new PGPSigner(
    '/path/to/the/secret/key.asc',
    '/path/to/the/public/key.asc', // Attached to the email if present. null by default.
    'passphrase', // If needed, null by default. 
    [] // Options (binary path, digest/cipher algorithms...
);

$signedEmail = $signer->sign($email);

$encrypter = new PGPEncrypter(
    [
        'jane@example.com' => '/path/to/first-gpg.asc',
        'john@example.com' => '/path/to/second-gpg.asc',
    ],
    [] // Options (binary path, digest/cipher algorithms...
);

$encryptedEmail = $encrypter->encrypt($signedEmail);

Framework configuration

framework:
    mailer:
        dsn: '%env(MAILER_DSN)%'
        pgp_signer:
            secret_key: '/path/to/the/secret/key.asc' # required
            public_key: '/path/to/the/public/key.asc' # null by default
            passphrase: ''' # null by default
            binary: 'gpg' # gnupg, /bin/gpg2...
            cipher_algorithm: 'AES256' # default to 'AES256'
            digest_algorithm: 'SHA512' # default to 'SHA512'

Let us know if you have any comments on this PR.
Many thanks.

@Spomky Spomky force-pushed the 7.3 branch 5 times, most recently from e94f54e to b67f00a Compare February 21, 2025 08:32
@Spomky Spomky force-pushed the 7.3 branch 3 times, most recently from 671dab3 to 15774f1 Compare February 21, 2025 09:56
@stof
Copy link
Member

stof commented Feb 21, 2025

The encryption listener is a must-have, as it is the only way to make it compatible with TemplatedEmail (as you want to perform encryption after the rendering of the body)

@Spomky
Copy link
Contributor

Spomky commented Feb 21, 2025

The encryption listener is a must-have, as it is the only way to make it compatible with TemplatedEmail (as you want to perform encryption after the rendering of the body)

I understand the problem with templated emails, but I do not understand the purpose of the smime_encrypter or the pgpmime_encrypter.
When you send a digitally signed email, you just need the sender private key to sign it (and passphrase if needed).

But the encrypted emails are a bit different: you need to selectively choose the recipient public keys.
When I read how smime_encrypter is designed, there is only one certificate meaning that only one recipient can decrypt it. Unless I missed something, this will never work. Where do you select the correct recipient public key?

Instead, I would have passed the recipient public keys to the message metadata and designed the listener in a way where, when public keys are present, the email is encrypted.
WDYT?

@stof
Copy link
Member

stof commented Feb 21, 2025

@Spomky it is totally possible that the current smime encrypter is flawed (I'm not using it myself, so I cannot check). The listener for it was not part of the initial design of the feature (it was not even released in the same version)

Introduce the new MimePgp component for encrypting MIME messages using OpenPGP. The component is marked as experimental and includes initial setup files, exceptions, tests, and a changelog.
@Spomky
Copy link
Contributor

Spomky commented Apr 18, 2025

Hi @the-pulli,

There was no consensus within the core team to include it at this time. The decision is postponed and the PR will be re-evaluated for a future version.
I hope you understand, and thanks again for your work!

@the-pulli
Copy link
Author

@Spomky, no problem. I'll wait this one out 🤓

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants