search

albertito / chasquid

SMTP (email) server with a focus on simplicity, security, and ease of operation [mirror]
https://blitiri.com.ar/p/chasquid/
Other
870 stars 58 forks source link

introduce catch-all alias resolution #24

Closed aellwein closed 2 years ago

aellwein commented 2 years ago

A "catch-all" alias is an optional, special form of an alias that applies in a very last step of resolution (i.e. after running aliases hooks), if no aliases could be found for the user name so far.

Addresses GitHub issue #23.

albertito commented 2 years ago

Thank you for sending this! And sorry for the delay in review, as I mentioned in the bug, I have been afk.

The implementation is totally sound, and I appreciate the doc and tests too, but I think there's a problem with the logic. Basically, I think the catch-all should be used only when the local user does not exist, not when there's no matching alias.

Imagine that a@domain exists as a normal user (not an alias). And there's a catch-all alias pointing to c@domain. In that case, emails to a@domain will be sent to c@domain because of the catch-all. But they should arrive to a@domain since it's a valid user.

One possibility would be to not change how aliases are resolved, just track the catch-all separately. Then, change internal/smtpsrv/conn.go's userExists function so that if the user doesn't exist, the catch-all (if any) is returned. I haven't looked at this in detail though, it might have some problems too.

I will look into this a bit more, but thought I'd mention it just in case you also want to give it a try or have any thoughts :)

Thanks again!

aellwein commented 2 years ago

Ok, i think you are right, the catch-all fallback should be done on the user level. I will try to rework this accordingly.

aellwein commented 2 years ago

@albertito sorry for the delay. The last commit reflects my current progress, please tell me what you think about this solution. Some thoughts on this:

albertito commented 2 years ago

Thanks for following up on this!

I think the catch-all makes sense in the aliases file from a configuration perspective (since you're redirecting something, which is what aliases do), plus is more flexible since you can redirect to multiple locations, etc.

But implementation wise, I think it is exposing the trickiness of keeping aliases and userdb completely agnostic from each other as they are today.

Your patch does the right thing on that particular "user exists" check, but there's a problem in that it doesn't affect the alias resolution done at queue processing time. To do that properly, I think aliases need to know about users, but it requires the change to be much more intrusive.

I have a draft implementation that is functional, but I don't like how the tight coupling between aliases and userdb is ending up. I'd like to spend more time on it, to see if I can make the code organization a bit better and avoid cluttering things too much.

Give me a few days to see if I can find something to propose. I really appreciate your patches, but unfortunately this is quite tricky to get right and with maintainable results :S

albertito commented 2 years ago

Just FYI this will probably be on hold until January, but I haven't forgotten about it! :)

albertito commented 2 years ago

Just a status update: I've resumed work on this and have a preliminary patch. There's still some rough edges, and there are a lot of moving parts and edge cases to consider carefully, so it's going to take a bit more time.

I'll post another update once I have something reasonable to share.

Thanks!

albertito commented 2 years ago

Sorry for the delay, this took a while!

As I worked on this, I ended up doing a bunch of cleanups and improvements to the aliases code but unrelated to this particular feature (67d0064f57595228494fd4b77c260992988b67b8, feb10299be1315081dc983f1f17efac45f61eec6, d7ca50c3e0721223c6c09d72f80cf0edd5a329f1). Those are already in next, and will probably end up in master sometime soon.

As for the catch-all, I have a temporary aliases-catchall branch with patch 1973e94d146eaf90077aade52758d8c45af1de0a, which I think is reasonably ready for review, if you want to take a look and/or give it a try.

I'm still not 100% convinced about the approach, and want to be careful in case there are unintended consequences, but it's the best I can come up with so far :)

Let me know what you think if you take a look or try it out.

Thanks!

aellwein commented 2 years ago

Hello @albertito , thank you so much for doing this improvement, i appreciate that! Please allow me some time for testing this feature, i will come back to you in the next few days.

aellwein commented 2 years ago

Sorry, i've been busy last weeks with other stuff, but i didn't forgot about this. Hopefully i can do testing on this weekend.

ThinkChaos commented 2 years ago

I gave the aliases-catchall branch a spin and it works for me.

Tests I did:

aellwein commented 2 years ago

@albertito, just wanted to write you back. I also tested it with my server setup and it works as expected (i've tested for the same domain, as i only have one in my setup). Thanks!

albertito commented 2 years ago

Thanks a lot to both for testing this branch! It's great to hear things work as expected :)

I'll be merging the patch to the next branch this week/weekend, and expect it to be included in the next release.

albertito commented 2 years ago

This has been merged in the next branch, see commit f303e43!

I'm going to close this PR since I think we're done with this part of the discussion, and we can keep tracking this in the related issue #23.

Thanks a lot again!