-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[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
base: 7.3
Are you sure you want to change the base?
[MimePgp] Add the component #59372
Conversation
src/Symfony/Component/Pgp/Exception/PgpBadPassphraseException.php
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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.
src/Symfony/Component/Pgp/Exception/PgpBadPassphraseException.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Pgp/Exception/PgpKeyNotFoundException.php
Outdated
Show resolved
Hide resolved
@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); |
There was a problem hiding this 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)
src/Symfony/Component/Pgp/Mime/Part/Multipart/PgpEncryptedPart.php
Outdated
Show resolved
Hide resolved
@stof Yes, I came across this issue while testing. But I didn't find a workaround. Your suggested approach should be the solution. |
There was a problem hiding this 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this 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.
src/Symfony/Component/MimePgp/Mime/Part/Multipart/PgpSignedPart.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/MimePgp/Mime/Part/Multipart/PgpSignedPart.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/MimePgp/Mime/Part/Multipart/PgpSignedPart.php
Outdated
Show resolved
Hide resolved
With the latest commits I refactored the |
118454c
to
86d072a
Compare
Hi there, @the-pulli and I spent time on this PR today and we pushed some modifications.
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. |
e94f54e
to
b67f00a
Compare
671dab3
to
15774f1
Compare
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 But the encrypted emails are a bit different: you need to selectively choose the recipient public keys. 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. |
@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) |
53db3ba
to
2cd488a
Compare
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.
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. |
@Spomky, no problem. I'll wait this one out 🤓 |
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 🤓