Implemented support for symmetric encryption and decryption#322
Conversation
|
@mattosaurus FYI I bumped the dotnet version in the unit tests project to v10 (since that's the only SDK that I have installed on my machine), but let me know if I should revert that if you'd prefer staying on v8. |
…::AddVersionHeader (set default to true to retain previous PgpCore's behaviour). This is associated with mattosaurus#193.
|
Great. Thanks for this, I'll take a look later. I think as long as it's targeted at .NET 10 and standard 2 then we should be good so happy to increase the version |
There was a problem hiding this comment.
Pull request overview
This pull request adds support for symmetric encryption and decryption to PgpCore, a significant new feature allowing password-based encryption without asymmetric keys. The PR also includes spelling corrections ("algorithim" → "algorithm") and introduces an AddVersionHeader property to control version header emission in armored output.
Changes:
- Added symmetric key encryption/decryption support via new EncryptionKeys constructors and updated encryption/decryption logic
- Introduced AddVersionHeader public property in PGP class for controlling version headers in armored output
- Fixed spelling of "algorithm" throughout the codebase (previously misspelled as "algorithim")
- Updated version to 6.6.0.0 with corresponding release notes
Reviewed changes
Copilot reviewed 43 out of 43 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| PgpCore/PGP.cs | Added AddVersionHeader property; updated ArmoredOutputStream calls to use it |
| PgpCore/Models/EncryptionKeys.cs | Added SymmetricKey property, multiple new constructors for symmetric key support, changed passphrase storage to byte[] |
| PgpCore/PGP.EncryptSync.cs, PGP.EncryptAsync.cs | Added symmetric key encryption via AddMethodRaw calls |
| PgpCore/PGP.DecryptSync.cs, PGP.DecryptAsync.cs | Added symmetric key decryption handling for PgpPbeEncryptedData |
| PgpCore/PGP.SignSync.cs, PGP.SignAsync.cs | Updated to use AddVersionHeader parameter |
| PgpCore/PGP.KeySync.cs, PGP.KeyAsync.cs | Fixed parameter spelling (preferredSymetricKeyAlgorithms → preferredSymmetricKeyAlgorithms) |
| PgpCore/Helpers/Utilities.cs | Changed FindBestSigningKey to return null instead of throwing |
| PgpCore.Tests/* | Added KeyType.Symmetric enum value, updated all test constructors to pass symmetricKey parameter, added symmetric-specific tests |
| PgpCore.Tests/PgpCore.Tests.csproj | Changed from multi-targeting to net10.0 only |
| README.md | Fixed algorithm spelling in documentation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| _secretKeys = | ||
| new Lazy<PgpSecretKeyRingBundle>(() => Utilities.ReadSecretKeyRingBundle(privateKey.GetStream())); | ||
|
|
||
| _passPhrase = Encoding.Default.GetBytes(passPhrase); |
There was a problem hiding this comment.
Encoding.Default.GetBytes() uses the system's default encoding which varies by platform and locale. This is inappropriate for cryptographic keys which should use a consistent, well-defined encoding. Consider using Encoding.UTF8.GetBytes() instead for consistent cross-platform behavior, as is done in the test code at PgpCore.Tests/TestFactory.cs:98 and line 160.
| throw new ArgumentException("Could not find any signing keys in keyring"); | ||
| return secretKeys.First(); | ||
|
|
||
| return secretKeys.FirstOrDefault(); |
There was a problem hiding this comment.
The change from throwing an exception when no signing keys are found to returning null could cause null reference exceptions in calling code. The previous behavior was defensive, explicitly throwing when no keys could be found. Now calling code must handle the null case, but this is not documented and could lead to runtime errors. Consider adding null checks in the code that calls FindBestSigningKey or documenting this behavior change.
| if (symmetricKey != null && symmetricKey.Length > 0) | ||
| { | ||
| _symmetricKey = symmetricKey; | ||
| } | ||
| else | ||
| { | ||
| if (string.IsNullOrEmpty(publicKey)) | ||
| throw new ArgumentException("PublicKeyFilePath"); | ||
| if (string.IsNullOrEmpty(privateKey)) | ||
| throw new ArgumentException("PrivateKeyFilePath"); | ||
| if (passPhrase == null) | ||
| throw new ArgumentNullException(nameof(passPhrase), "Invalid Pass Phrase."); | ||
| } | ||
|
|
||
| var keyRings = Utilities.ReadAllKeyRings(publicKey.GetStream()); | ||
|
|
||
| _secretKeys = | ||
| new Lazy<PgpSecretKeyRingBundle>(() => Utilities.ReadSecretKeyRingBundle(privateKey.GetStream())); | ||
|
|
||
| _passPhrase = Encoding.Default.GetBytes(passPhrase); | ||
| InitializeKeys(keyRings); | ||
| } |
There was a problem hiding this comment.
When a symmetric key is provided, the validation logic skips checking for public/private keys and passphrases. However, the code then still attempts to read key rings and secret key bundles from the provided public/private key parameters which will be null or empty strings. This will likely cause exceptions at runtime. The InitializeKeys call should be conditional or handle the symmetric-only case differently.
mattosaurus
changed the base branch from
master
to
feature/add-symmetric-enc-dec
mattosaurus
changed the base branch from
feature/add-symmetric-enc-dec
to
master
mattosaurus
changed the base branch from
master
to
feature/add-symmetric-enc-dec
Thought this was missing in this otherwise awesome codebase, so I gave it a shot :)
Tests still succeed and I've added an additional key type in the test data's KeyType enum, but I also tested it manually using Kleopatra: works on my machine 🥲
Cheers <3