Send via SMTP support for aliases #869

sahilph · 2022-04-02T02:43:05Z

This PR add ability to send via SMTP for aliases. With this emails can be sent directly from aliases via SMTP without the need for creation of reverse-alias.

If enabled, Every alias will have a unique password (generated client side) for SMTP access. Username will be same as alias.

This adds a global option in Settings to enable this feature, without enabling this option, SMTP access will remain disabled for the user.

Note: Even after enabling this option in settings, you would still need to enable SMTP individually per alias. This will also disable receiving via reverse-alias for that particular alias. i.e. Sender's original address will be displayed in "From:".

SMTP server address should ideally be the same where your app is hosted (but this depends on your configuration)
SMTP port: 465
This would need SSL cert/key file as SMTP would be over (implicit) SSL.

This should resolve #679

The screenshots below will make it more clear on how it would operate:

First enable "SMTP for Aliases" from Settings Page:

Then on the Aliases page, select "More" on an alias and a new toggle, to enable SMTP, will have appeared below "Pin this alias":

After toggling it, SMTP credential will be generated for that alias:

This will also disable receiving via reverse-alias, for that particular alias. i.e. Sender's original address will be displayed in "From:"

Also change way of importing nanoid

This reverts commit 42c4c80.

developStorm · 2022-04-02T03:20:49Z

Great job! Thanks @sahilph. I think this is definitely a valuable feature. The only point I currently find somewhat awkward is that each alias would get their own credentials. My thought for this feature is that users should be able to connect to the SMTP server using a generic credential, and the SMTP server would automatically detect which specific alias should be used to send the email by looking at the address the user is replying to, and other information that available. This way, users would only have to add their credentials once to Gmail's send-as feature or local email client, and reply to emails sent to any alias with the generic credentials.

sahilph · 2022-04-02T04:11:28Z

@developStorm Thanks for you feedback.

One of the main reasons I developed this was to use Gmail "Send mail as:" feature. And It requires users to add "From:", and username and password for every alias.
So even if there was a single generic credential, users will have to manually add the alias in Gmail anyways. So I thought of adding separate credentials, which would make the authentication handling easier.

Also consider one more scenario. Suppose the aliases are getting generated on-the-fly and I want to give a email to website say non-existent-alias@mycustom.domain.
Now non-existent-alias@mycustom.domain is not the alias which I ever send email from, but for some reason the website needs replying from that email, even if there was generic credential then I would have to again add that alias in Gmail's "Send mail as:" anyways.

In the current scenario, since we can enable SMTP per-alias-basis, a reverse-alias will automatically get generated and replying will be easier as I can just reply via reverse-alias.

Also I'm not sure how to authenticate and validate, if there was only a single generic credential present. If you have any ideas please let me know and I can implement it.

developStorm · 2022-04-02T04:22:38Z

@sahilph The idea is that the SMTP server does not need to honor the From specified in "Send mail as:" feature. So we can just put a dummy From there and let the SMTP server figrue out which From (alias) should be used based on the mail content that is being sent. How to determine right alias to use is a problem though, I'm thinking that we can use the recipent's address but I'm not sure if that's necessarily accurate. Any thoughts on this?

sahilph · 2022-04-02T04:46:39Z

@developStorm While it would be very convenient to have such a feature, I can't really think of a way of implementing this without reading From: field 😅.

developStorm · 2022-04-02T04:52:12Z

@sahilph We just have to guess ;-) For example, if the incoming email to SL SMTP server looks like:

MAIL FROM: dummy@simplelogin.co
RCPT TO: user@example.com

Then now SL SMTP server can guess: "hmm the authenticated user has a recent interaction with user@example.com on alias alias1@simplelogin.co, so we should change the From to alias1@simplelogin.co". SMTP server then delivers the reply on behalf of alias1@simplelogin.co to user@example.com to example.com's SMTP server.

sahilph · 2022-04-02T05:35:11Z

@developStorm I don't think we should rely on guesswork while sending emails.

Also what about sending to someone who we have never sent before ?

developStorm · 2022-04-02T05:39:29Z

@sahilph Yeah guess is not a perfect idea, but I think it's worse to have user add every single alias they have, when they have let's say 50+ aliases and need to be able to reply as all those aliases.

For new recipients, user can manually add them on SL portal before they proceed, as what they have to do now to obtain reply address.

sahilph · 2022-04-02T06:30:48Z

Yeah guess is not a perfect idea, but I think it's worse to have user add every single alias they have, when they have let's say 50+ aliases and need to be able to reply as all those aliases.

User don't have to add every single alias. They only need to add the ones they usually send from. For other aliases those who dont have SMTP enabled, sending via reverse-alias is still possible

For new recipients, user can manually add them on SL portal before they proceed, as what they have to do now to obtain reply address.

Then what's the point of SMTP? users can do that already do that using reverse-alias.

The whole point of SMTP access (and this PR) is that, if users want to send emails from the aliases they most often send from, there would be no need of creating reverse-alias. Users can just the emails using their aliases via SMTP using their favorite email client. No need to manually login to SL portal and create reverse aliases everytime.

For other non-frequently used aliases, use the reverse-alias method as that would still work.

TL;DR: For sending via most-frequently used alias -> Use SMTP ; otherwise -> use the regular-reverse alias method.

sahilph · 2022-04-02T06:36:52Z

@developStorm
Check out #679 as to what I mean. I was facing similar issues.
Here are a few comments highlighting the issue:
#679 (reply in thread)
#679 (reply in thread)

nguyenkims

The PR looks really good! I haven't run it though, it'd be great if you can include how to test this new service in the CONTRBUTING file.

I left some comments, lots of them are rather nits though.

README.md

app/api/views/alias.py

nguyenkims · 2022-04-02T08:19:47Z

app/config.py

@@ -444,3 +444,7 @@ def setup_nameservers():


 NAMESERVERS = setup_nameservers()
+
+# For SMTP support
+SMTP_SSL_CERT_FILE = os.environ.get("SMTP_SSL_CERT_FILE") or "/smtp_ssl_cert.pem"


Can we add PATH suffix to be explicit, so SMTP_SSL_CERT_FILEPATH instead of SMTP_SSL_CERT_FILE?

And as existing installations don't have the certificates and keys file, should this new service disabled unless the env variables are set?

Can we add PATH suffix to be explicit, so SMTP_SSL_CERT_FILEPATH instead of SMTP_SSL_CERT_FILE?

I have changed this.

And as existing installations don't have the certificates and keys file, should this new service disabled unless the env variables are set?

The service won't start at all, if the files cert/key files do not exist and thus would remain disabled.

SMTP_handler.py

nguyenkims · 2022-04-02T11:11:48Z

SMTP_handler.py

+                return [(False, status.E515)]
+
+    user = alias.user
+    mailbox = Mailbox.get_by(id=alias.mailbox_id)


the mailbox should come from the envelope.mail_from as an alias can have several mailboxes.

Well, if user can have multiple inboxes, it won't be relevant to include mailbox here.
I added it only for logging as I thought Email Log requires it. But it doesn't, so it can be removed. What are your thoughts. ?

The mailbox_id is needed by EmailLog. In this reply phase, this is the mailbox (or a mailbox's authorized address) that sends the email.

But for SMTP phase, the envelope.mail_from, will be the alias itself.

So how to get the relevant mailbox in case of multiple mailboxes ?

I think it just just be removed as we are already setting is_SMTP=True

SMTP_handler.py

nguyenkims

I just answered some questions.

README.md

app/api/views/alias.py

app/models.py

nguyenkims · 2022-04-08T16:15:20Z

email_handler.py

-        add_or_replace_header(msg, "Reply-To", new_reply_to_header)
-        LOG.d("Reply-To header, new:%s, old:%s", new_reply_to_header, reply_to_header)
+    # Don't Change Headers when SMTP is enabled for alias
+    if not is_SMTP_enabled_for_alias:


I did not get this part. Can you please explain this ?

Currently SMTP handler receives an email from an alias to a contact and send the email to the contact.

What I meant is in addition to that flow, SMTP handler can also handle the case user sends an email from an alias to a reverse alias. So when SMTP handler receives an email from an alias to a reverse alias, it extracts the contact address from the reverse alias and continue the normal process.

sahilph

@nguyenkims I have replied to some of the comments.

app/api/views/alias.py

README.md

app/models.py

sahilph · 2022-04-10T02:52:35Z

email_handler.py

-        add_or_replace_header(msg, "Reply-To", new_reply_to_header)
-        LOG.d("Reply-To header, new:%s, old:%s", new_reply_to_header, reply_to_header)
+    # Don't Change Headers when SMTP is enabled for alias
+    if not is_SMTP_enabled_for_alias:


Oh alright, now I kinda get it.

Say a user is trying to send via SMTP, but instead of sending to the actual contact address, they send to the contact's reverse alias.

So you are saying that, the SMTP handler should handle this case as well, right?

Also, in this scenario, the reverse-alias, should belong to the alias, correct ?

nguyenkims

Hey just left a thought. I think we can merge the PR and it can be used as-is for the self hosting. For SimpleLogin.io version, I think we need to figure out a way to have Postfix protecting this new service.

README.md

sahilph · 2022-04-13T06:37:24Z

I think we can merge the PR and it can be used as-is for the self hosting.

Oh that's nice.

For SimpleLogin.io version, I think we need to figure out a way to have Postfix protecting this new service.

Yes, if we could figure out a way to make postfix talk to python, that would be great as that is the only thing preventing this from happening.

sahilph · 2022-04-13T06:39:26Z

Also, I have replied to the comments.

nguyenkims

I just left 2 comments.

nguyenkims · 2022-04-14T15:33:12Z

app/models.py

@@ -2557,6 +2575,45 @@ class AliasMailbox(Base, ModelMixin):
    alias = orm.relationship(Alias)


+class SMTPCredentials(Base, ModelMixin, PasswordOracle):
+    __tablename__ = "SMTP_credentials"
+    __table_args__ = (sa.UniqueConstraint("alias_id", name="uq_alias"),)


@sahilph re in case you haven't seen this comment.

nguyenkims · 2022-04-14T15:36:37Z

email_handler.py

-        add_or_replace_header(msg, "Reply-To", new_reply_to_header)
-        LOG.d("Reply-To header, new:%s, old:%s", new_reply_to_header, reply_to_header)
+    # Don't Change Headers when SMTP is enabled for alias
+    if not is_SMTP_enabled_for_alias:


As I mentioned earlier, the email will fail dmarc alignement so we can't keep the original header. We cannot keep original header at the moment. This code part needs to be reverted in order for the PR to be merged.

ARC is another topic that should probably be addressed in another PR.

nguyenkims

I realized that I haven't done the follow up on the discussion on SMTP_handler file.

nguyenkims · 2022-04-14T15:38:39Z

SMTP_handler.py

+
+    contact = Contact.get_by(website_email=website_email)
+    if not contact:
+        LOG.w(f"No contact with {website_email} as website email")


What I meant is this is a rather normal case so we shouldn't use logging warning level here. info or debug level is more suitable.

nguyenkims · 2022-04-14T15:43:19Z

SMTP_handler.py

+        LOG.w(f"No contact with {website_email} as website email")
+    try:
+        LOG.d("Create or get contact for website_email:%s", website_email)
+        contact = get_or_create_contact("", website_email, alias, msg)


I mean the current get_or_create_contact handles a lot of cases that aren't necessary for this usage. I think it's better to have a smaller method, which is a stripped down version of the get_or_create_contact to be used here.

nguyenkims · 2022-04-14T15:44:40Z

SMTP_handler.py

+                return [(False, status.E515)]
+
+    user = alias.user
+    mailbox = Mailbox.get_by(id=alias.mailbox_id)


The mailbox_id is needed by EmailLog. In this reply phase, this is the mailbox (or a mailbox's authorized address) that sends the email.

nguyenkims · 2022-04-14T15:46:40Z

SMTP_handler.py

+        LOG.e("%s domain isn't known", alias)
+        return False, status.E503
+
+    contact = Contact.get_by(website_email=website_email)


As mentioned in another comment, I think we can also handle the case where website_email is a reverse alias here. This should be relatively easy IMO.

I have added some code to handle this case.

- Also create a stripped down function `get_or_create_contact_for_SMTP_phase` - Also change the log to debug instead of warning

This reverts commit 25a56ec.

sahilph

@nguyenkims I have made some changes and left some comments.

sahilph · 2022-04-15T03:20:57Z

SMTP_handler.py

+        LOG.e("%s domain isn't known", alias)
+        return False, status.E503
+
+    contact = Contact.get_by(website_email=website_email)


I have added some code to handle this case.

sahilph · 2022-04-15T03:21:21Z

SMTP_handler.py

+
+    contact = Contact.get_by(website_email=website_email)
+    if not contact:
+        LOG.w(f"No contact with {website_email} as website email")


I have changed this to debug level.

sahilph · 2022-04-15T03:22:33Z

SMTP_handler.py

+        LOG.w(f"No contact with {website_email} as website email")
+    try:
+        LOG.d("Create or get contact for website_email:%s", website_email)
+        contact = get_or_create_contact("", website_email, alias, msg)


I have created a similar stripped down version get_or_create_contact_for_SMTP_phase please check if its fine.

sahilph · 2022-04-15T03:23:49Z

SMTP_handler.py

+                return [(False, status.E515)]
+
+    user = alias.user
+    mailbox = Mailbox.get_by(id=alias.mailbox_id)


But for SMTP phase, the envelope.mail_from, will be the alias itself.

So how to get the relevant mailbox in case of multiple mailboxes ?

I think it just just be removed as we are already setting is_SMTP=True

sahilph · 2022-04-15T03:25:24Z

app/models.py

@@ -2557,6 +2575,45 @@ class AliasMailbox(Base, ModelMixin):
    alias = orm.relationship(Alias)


+class SMTPCredentials(Base, ModelMixin, PasswordOracle):
+    __tablename__ = "SMTP_credentials"
+    __table_args__ = (sa.UniqueConstraint("alias_id", name="uq_alias"),)


I have removed the __table_args__

sahilph · 2022-04-15T03:26:10Z

email_handler.py

-        add_or_replace_header(msg, "Reply-To", new_reply_to_header)
-        LOG.d("Reply-To header, new:%s, old:%s", new_reply_to_header, reply_to_header)
+    # Don't Change Headers when SMTP is enabled for alias
+    if not is_SMTP_enabled_for_alias:


I have reverted this part.

acasajus · 2022-04-19T10:50:23Z

app/api/views/alias.py

+        else:
+            SMTPCredentials.delete_by_alias_id(alias_id=alias.id)
+        alias.enable_SMTP = enable_SMTP
+        changed = True


Instead of generating the password in the client it'd be better to generate it server-side and show it once to the client on successful activation:

No need to add extra dependencies in python and in js (ie. nanoid)

No need to have js code in the front

No strange 21 character validation

Other clients have no need to add nanoid dependencies.

Just generate a random secret, store it in the db and in the next page display show the credentials warning the user that the credentials won't be able to be displayed any more times.

@nguyenkims Is this really needed at this stage ?

@sahilph generally I think it's better to generate secret on server as we can't control client environment. Adding this change should be quite simple right as the server generates the password and send this back to the client to display?

acasajus · 2022-04-19T10:54:21Z

Hey, nice PR. But as a general comment, there aren't any tests added. I would suggest to add some:

Creating the SMTPCredentials does what it's supposed to and that passwords can be validated
The SMTP handler is able to receive connections, authenticate/fail, and deliver the messages to where it's expected.

sahilph · 2022-04-24T03:51:07Z

@acasajus Thanks for the feedback.
Yes, I understand that there are no tests, and I can certainly write some if needed, but I believe, ideally, tests should be written by someone else, because if I also write tests for this, it will be highly biased. (I certainly don't want my one code to break now do I ? 😁 )

nguyenkims · 2022-04-24T17:32:54Z

@sahilph @acasajus Having tests for every PR is indeed a good idea. We actually set a limit in terms of code coverage under which a PR won't pass the CI. And tests should be written by the PR author :). It happens several times that writing tests help discover cases that we don't realize during the development. We are not extreme in terms of test coverage though but there should be enough tests to cover the most frequent cases.

Robin-Sch · 2022-07-03T09:59:31Z

any update? I really need this asap

EvilAaron · 2022-07-03T15:53:40Z

Yeah, I am following this with great interest as well.

I purchased a year of SL and love much about the service and have brought 2 of my domain on board.

After using for a couple of weeks though and mucking with the reverse alias system and clogging my contacts up with extra redundant emails in their profiles, this is the one part that feels clunky.

Really hoping this is solved with this above proposed solution. If no way to "send as" SL aliases from email client arrives in a year when renewal arrives, I would have to consider pulling my domains and finding a service for email support and relegate SL to just throwaway emails on their domains.

I am hoping a way will be found, as I love SL so far and would love for it to fit.

c0nfigurati0n · 2022-08-04T13:00:10Z

I'm not sure i get the point of this function, seen as SimpleLogin is indeed not meant to be a transactional email service. But if there are other use cases for this function, then i would say, "why not".

nephalemsec · 2022-09-30T15:13:57Z

What exactly is holding this back? This has been sitting in a completed state for so long, how can we get it over the line?

edsimpsons83 · 2022-09-30T15:51:44Z

Agreed, disappointing to see such a valuable PR just sitting here.

nguyenkims · 2022-10-02T17:25:00Z

@nephalemsec @edsimpsons83 the PR isn't complete. We asked for adding tests into it (among other things) and haven't received any news yet. And please keep in mind that this PR is rather for self hosting. To make it available on SL.io, we need to adapt the infra and work on an anti-abuse system first.

nephalemsec · 2022-10-03T01:58:57Z

@nephalemsec @edsimpsons83 the PR isn't complete. We asked for adding tests into it (among other things) and haven't received any news yet. And please keep in mind that this PR is rather for self hosting. To make it available on SL.io, we need to adapt the infra and work on an anti-abuse system first.

Understood. As @sahilph has stated he believes there would be a bias with the PR author making unit tests is there a plan for SL team to assist?

nguyenkims · 2022-10-03T08:11:57Z

@nephalemsec providing unit tests should actually be part of the PR and it is up to the author to write tests.

c0nfigurati0n · 2022-10-09T15:42:45Z

I'm not sure i get the point of this function, seen as SimpleLogin is indeed not meant to be a transactional email service. But if there are other use cases for this function, then i would say, "why not".

Sadly i didn't get a reply from someone explaining the purpose of this function to me. :(

sugarfunk · 2022-10-10T12:05:45Z

I'm not sure i get the point of this function, seen as SimpleLogin is indeed not meant to be a transactional email service. But if there are other use cases for this function, then i would say, "why not".

Sadly i didn't get a reply from someone explaining the purpose of this function to me. :(

To be able to send from an alias without needing to log into the SL interface comes to mind.

jasonhe54 · 2023-04-24T03:34:35Z

So, is this PR just dead now after 6 months? Is there any intent on supporting this feature? Would be nice to actually be able to use SL as a way to send emails from an alias similar to the way iCloud Plus currently lets you do it.

Haulien · 2023-11-09T13:13:56Z

It's a shame this PR has been left to die. Time to look for a better alternative to SL.

sahilph added 18 commits March 30, 2022 21:41

Create initial SMTP handler

ca245b2

Add helper functions to SMTPCredentials class

a93696e

Modify Front-end for SMTP support

fbfc714

Fix smtp-for-aliases-section checkbox issue

334ab93

Change script type to module

d07af51

Also change way of importing nanoid

Improve SMTP password logic

dfdbf0d

Add __repr__ to SMTP Credentials class

26fdf52

Add decode to auth_data in SMTP_handler

fb65e0c

Add auto-generated migrations

42c4c80

Change Default port to 465 for SMTP_handler

7b5181c

Format code with: poetry run black

7673644

Revert "Add auto-generated migrations"

25b8ee8

This reverts commit 42c4c80.

Fix email logging

8387ecd

Re-enable is_SMTP in EmailLog

c512e77

Fix filtering for SMTP enabled aliases

73f3d09

Add auto-generated migrations

ba2a047

Add SMTP variables to example.env

1af6fcf

Add about SMTP handler to README

84ec9e2

nguyenkims reviewed Apr 2, 2022

View reviewed changes

sahilph added 2 commits April 2, 2022 21:15

Add command to generate self-signed certs in README

48da27a

change SMTP_password to lowercase.

b717f79

nguyenkims reviewed Apr 8, 2022

View reviewed changes

sahilph commented Apr 10, 2022

View reviewed changes

nguyenkims reviewed Apr 12, 2022

View reviewed changes

README.md Show resolved Hide resolved

nguyenkims reviewed Apr 14, 2022

View reviewed changes

sahilph added 5 commits April 14, 2022 21:51

Remove __table_args__ when unique=True in models.py

b8ec8aa

Revert keeping original header in email_handler.py

18c0a02

Handle sending to reverse alias in SMTP_handler

a2901e2

- Also create a stripped down function `get_or_create_contact_for_SMTP_phase` - Also change the log to debug instead of warning

Revert "Add auto-generated migrations"

e0b7fc3

This reverts commit 25a56ec.

Add auto-generated migrations

32d6a73

sahilph commented Apr 15, 2022

View reviewed changes

acasajus reviewed Apr 19, 2022

View reviewed changes

sahilph requested a review from cquintana92 as a code owner May 10, 2022 09:25

Send via SMTP support for aliases #869

Are you sure you want to change the base?

Send via SMTP support for aliases #869

Conversation

sahilph commented Apr 2, 2022

developStorm commented Apr 2, 2022

sahilph commented Apr 2, 2022

developStorm commented Apr 2, 2022

sahilph commented Apr 2, 2022

developStorm commented Apr 2, 2022 • edited

sahilph commented Apr 2, 2022

developStorm commented Apr 2, 2022

sahilph commented Apr 2, 2022

sahilph commented Apr 2, 2022

nguyenkims left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nguyenkims left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sahilph left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nguyenkims left a comment

Choose a reason for hiding this comment

sahilph commented Apr 13, 2022

sahilph commented Apr 13, 2022

nguyenkims left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nguyenkims left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sahilph left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acasajus commented Apr 19, 2022

sahilph commented Apr 24, 2022

nguyenkims commented Apr 24, 2022

Robin-Sch commented Jul 3, 2022

EvilAaron commented Jul 3, 2022

c0nfigurati0n commented Aug 4, 2022 • edited

nephalemsec commented Sep 30, 2022

edsimpsons83 commented Sep 30, 2022

nguyenkims commented Oct 2, 2022 • edited

nephalemsec commented Oct 3, 2022

nguyenkims commented Oct 3, 2022

c0nfigurati0n commented Oct 9, 2022

sugarfunk commented Oct 10, 2022

jasonhe54 commented Apr 24, 2023

Haulien commented Nov 9, 2023

developStorm commented Apr 2, 2022 •

edited

c0nfigurati0n commented Aug 4, 2022 •

edited

nguyenkims commented Oct 2, 2022 •

edited