feat: support s/mime encryption #1946
Conversation
schwigri
requested review from
acasajus,
cquintana92 and
nguyenkims
as code owners
| except Exception as exceptasdf: | ||
| LOG.w( | ||
| "Cannot S/MIME encrypt message %s -> %s. %s %s", | ||
| contact, | ||
| alias, | ||
| mailbox, | ||
| user, | ||
| ) | ||
| LOG.w(exceptasdf) |
There was a problem hiding this comment.
Oops, sorry; I can clean up this variable name and logging
|
Hi thanks a lot for making this PR and sorry for the late response! I'll ask our team to review the PR and come back to you at the beginning of 2024 (as most of the team is on holiday during Christmas :) ) |
|
Hey, thanks for the contribution. As a general rule we'd like to have tests for new code that's merged into the repo to make sure that:
Would you mind adding tests to it? |
| ) | ||
|
|
||
| # smime | ||
| smime_public_key = sa.Column(sa.Text, nullable=True) |
There was a problem hiding this comment.
Can you move this into a different model like MailboxSMimeKey ? We load mailboxes all the time from the db and the lighter they are, the better. We only need to load the key when we're about to use it.
There was a problem hiding this comment.
If I understand correctly, this would mean to create new table for S/MIME certificates, and then add the public key contents to the new table instead of as a field in the mailbox. Then, reference the entry when we want to sign the emails so that the cert is not loaded from the DB as often?
Want to clarify as I think it will be a bit more complex logic, so I would need to take more time to understand these models and the DB structure.
| url_for("dashboard.mailbox_detail_route", mailbox_id=mailbox_id) | ||
| ) | ||
|
|
||
| mailbox.smime_public_key = request.form.get("smime") |
There was a problem hiding this comment.
Can you:
- Verify that this is a valid PEM file.
- Add a check that the key is not bigger than 15KiB? A PEM chain with several certs shouldn't be bigger than that.
There was a problem hiding this comment.
These two seem fairly straightforward. Would solution like so make sense?
from pem import parse
from sys import getsizeof
...
raw_smime_cert_input = request.form.get("smime")
if getsizeof(raw_smime_cert_input) > 15360:
flash("S/MIME certificate is too big! Must be smaller than 15 KiB")
return redirect(
url_for("dashboard.mailbox_detail_route", mailbox_id=mailbox_id)
)
smime_certs = parse(raw_smime_cert_input)
if not smime_certs[0]:
flash("Unable to validate S/MIME certificate")
return redirect(
url_for("dashboard.mailbox_detail_route", mailbox_id=mailbox_id)
)
mailbox.smime_public_key = str(smime_certs[0])
|
Thanks for reviewing and commenting. I’ll take some time to address them due to limited bandwidth. Appreciate it! |
|
@schwigri Any updates on this? :) |
|
This is a highly desired feature especially for iOS users as discussed in #2174. Thanks @schwigri for tackling the implementation! The author of this PR seems to not have the capacity at the moment to follow up the code review. @acasajus, is there anything, to be done to pick up this pull request again? Would it be worth for me (new to the code base) to spend some of my free time on this, i.e. does this PR stand a chance to get merged in the near future? |
|
@szapp (and everyone else here) — my apologies for opening this PR and not following up. I've had it running like this on a VPS and it has met all my needs, and due to my lack of familiarity with Docker and Python, I have not had enough time to dedicate to making this PR meeting high quality bar. I think it is a good foundation to build off of, but I can't commit to any updates soon. I don't want to take my current instance offline due to it handling all of my email, and I don't have the time to spin up a new VPS for testing, etc. I see some useful comments, such like:
Without further research I'm not sure how to implement these simply, as well as add proper unit testing etc. Really appreciate this product, the people that work on it, and glad to support via my Proton subscription separately :) |
|
@schwigri thanks for your response! I am glad you took the time to reply. It's good to hear that your implementation seems stable. If you don't mind, I can try and bring this PR to completion - although I cannot promise. In order to continue this very PR, would you be willing to invite me as collaborator on your fork? Alternatively, I have no issue with making my own fork, but that would result in a new PR, leaving this one to die/possibly undermining your work. Cheers! Update: Because of limited time, I won't be able to contribute on this in the near future after all. Sorry about that! |
|
I got a little bandwidth and recently spent some time to upgrade my self-hosted SL to latest (has been on 3.x since this PR was originally opened). I proposed a solution to the comment about validating the PEM file and its size, and left a clarification question in response to comment about the mailbox model. In meantime I'll try out what I've proposed for the PEM validation and learn more about how the models work. |
|
As brief follow up, I tested the PEM validation and file size validation and it works on my instance. Will take some time to investigate the models as requested. |
6 participants
Hi, I use S/MIME to enable reading encrypted email using iOS and macOS Mail apps :) I've made this change and had it deployed on my own self-hosted instance, but thought I would share here. I've read through the contributing guide, but this is my first real PR so please let me know if there is anything I missed.
Also not sure if there should be other features considered (e.g. disable S/MIME for specific aliases like PGP, or some kind of fallback for S/MIME fails but fallback to PGP, etc.), so I don't really expect this will be merged immediately but thought it may be useful for others who may be interested in this feature. It could also be expanded to include signing, but for now it didn't seem necessary given the dynamic nature of the sending address.
Related to issue #527 and discussion #535!