feat: handle email encryption via x25519 asymmetric keys #2248
Conversation
Carbonhell
requested review from
Mark-Simulacrum,
emilyalbini,
jdno,
marcoieni and
ubiratansoares
as code owners
Carbonhell
force-pushed
the
feature/asymmetric-encryption-emails
branch
from
b3380c4 to
a72b95f
Compare
Carbonhell
force-pushed
the
feature/asymmetric-encryption-emails
branch
from
a72b95f to
6288097
Compare
There was a problem hiding this comment.
Hi @Carbonhell, thanks for rising this PR! Great to have this contribution 🚀
I'll leave two considerations regarding your proposal
Encryption scheme
I'm by no means a PKI expert, but when I first evaluated what I'd propose myself to implement this feature, I definetly recollected rage, the Rust implementation for age encryption. I use age (the Go CLI) to encrypt some of my personal stuff 😄
rage builds on top of the some dependencies you mentioned, but the implementation for small recipients using the very same x25519 looks super straightforward as per available examples, and it also seems the right fit for our use case.
My initial take is that having less code attached to this feature make things simpler to grasp and maintain* for the long run. That said : would you mind giving a try on rage and bring some feedback to us, without commiting to it all?
I think the implementation will become shorter in terms of code, but we could definetly learn from a comparison on binary sizes, private/public key sizes and total dependencies we'd bring to our dep graph.
After that, perhaps we can make a decision regarding which crypto approach we want to use, perhaps gathering input from other infra folks too 🙂
Migrating existing encrypted emails and setting credentials
As soon as we have an agreement on the PKI approach - either yours, rage or something else - I'd be happy to generate the key, add it to CI, decrypt existing emails and leave a comment to additional changes you can add in a single additional commit to finish this PR 🙂
Just wanted to let you know that this particular feature is not a priority for infra right now, hence it may take some time between reviews, feedback and action from our side.
I'll leave some additional comments in the code itself, just to highlight a few things that came to mind
| return Err(Error::WrongKeyLength); | ||
| } | ||
| let key = Key::from_slice(key.as_bytes()); | ||
| fn init_cipher(key: &[u8]) -> Result<XChaCha20Poly1305, Error> { |
There was a problem hiding this comment.
Why the original validation is no longer needed?
There was a problem hiding this comment.
Because the shared key generated via the KDF is already guaranteed to be of 32 bytes as per the docs: https://docs.rs/blake3/latest/blake3/struct.Hash.html
Although this can also easily be enforced by specifically accepting slices of 32 bytes instead of generic u8 slices. Will do that
There was a problem hiding this comment.
Ah ok! Got it!
Asked mostly to understand whether should we return an instance of XChaCha20Poly1305 directly here
There was a problem hiding this comment.
Oh, my bad, you're right. There's no need for the Result enum here (it's a leftover from the previous signature of this function) - will also remove it. Thanks for spotting this issue!
| The production private key is accessible to select Infrastructure Team members, and | ||
| it is stored in the following parameter on AWS SSM Parameter Store: |
There was a problem hiding this comment.
Considering the change we are proposing here, I think we could store the private key somewhere else. Ofc we'll have it as CI secret, but eventually 1password could be an option for infra admins as well, just in case we need to decrypt existing emails and get in touch with people for whatever reason.
Hence, I do think we could simplify a bit this approach. Not sure what other infra admins think about this 👀
There was a problem hiding this comment.
I'm not familiar with 1password and how you use it, but I think a relevant point is that I can't seem to find the Terraform definition of this parameter on simpleinfra - I assume it was created directly via the AWS dashboard. This might be an extra point in favor of using 1password to remove this parameter that isn't currently managed under IaC
Thank you for the feedback! To be fair, I wasn't aware of Edit: technically, x25519-dalek isn't on blessed.rs, but the ed25519-dalek is, and they're both based on the same core from what I gathered |
2 participants
Closes #2245.
Note: The PR requires two changes by an infra-team admin before being ready to be merged (cc @marcoieni):
generate-keys;email_encryption,rsmust be replaced with the newly generated one;EMAIL_PRIVATE_KEY, which is passed as an environment variable in the same way as the now deprecatedEMAIL_ENCRYPTION_KEYsecret/env var;decrypt-emailcommand on the mai branch), encrypting them via this branch'sencrypt-emailutility, and adding them as changes to this PR;There's a section at the end of the README which refers to a SSM parameter - for simplicity, I haven't changed its syntax (even though it'd be more correct to rename it to "email-private-key" instead of encryption key).
The encryption scheme uses x25519-dalek to generate a shared secret via Diffie-Hellman, with blake3 (recommended in RustCrypto) used as a key derivation function to get the key to use with XChaCha20Poly1305 (which was the library used previously for symmetric encryption/decryption).