albertito / chasquid

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

Surprising interaction of drop_characters with aliases #41

Closed znerol closed 11 months ago

znerol commented 11 months ago

Given the following config:

$ grep drop_character chasquid.conf 
drop_characters: "."
$ grep myself domains/example.com/aliases 
me.myself: me

The alias me.myself@example.com will never be expanded to me. I guess that drop_characters gets applied to the recipients address before the alias lookup is performed. In order to work around this, it is currently necessary to specify the alias without any characters specified in drop_characters. I.e.:

$ grep drop_character chasquid.conf 
drop_characters: "."
$ grep myself domains/example.com/aliases 
memyself: me

Not sure whether this should be solved in code or maybe in the docs.

albertito commented 11 months ago

Thanks a lot for filing this issue!

This is a great point, you're right that this is the current behaviour and that it is not obvious at all.

Documentation should definitely be updated, I will write something up probably later today.

But I am also wondering if chasquid should apply the drop_characters and suffix_separators to the left-side aliases when loading them. It would make the problem less likely to happen, but on the other hand it can be misleading or surprising in other ways, like someone creating an alias like me+blah: ... and expecting it to be different than me.

So I think I am inclined to keep the logic as-is, and document the current behaviour in the aliases documents, because it will be the least surprising/easier to reason about. But I will think about this more, and also please let me know if you have any opinion about it too!

albertito commented 11 months ago

FYI this is taking longer than expected because I'm exploring a few different options for how to handle these cases, but I'm working on it :)

znerol commented 11 months ago

I agree, this requires careful analysis. I was digging a bit into postfix documentation in the hope to get some ideas from over there. It looks like the behavior can be changed with propagate_unmatched_extensions. There is also a whole address rewriting documentation page. But honestly I have trouble to understand any of it.

albertito commented 11 months ago

Thanks for taking a look!

This can get complicated really quickly, so I would like to avoid those kinds of complexity, and ideally I want to keep it simple and reasonably intuitive (or at worst, least surprising) to users.

I think there are three practical options.

Option 1: Clean addresses when parsing

We can clean the addresses when parsing the aliases file, so removing any drop characters and suffixes very early. That means we treat abc and a.b.c and abc+x as the same.

So if a user writes this file:

abc: xxx
a.b.c: yyy
abc+ppp: zzz

The end result is a single alias of abc -> zzz (because they are all the same, and the last one takes precedence).

Pros: if a user writes aliases like name.lastname: something, this will work as they expect, and is the least surprising behaviour.

Cons: If a user wants to explicitly have a different alias for name and name+blah, it is impossible to do, and it will be confusing if they try, because the second one will be used for everything.

Option 2: Exact matching takes precedence

We can keep the left-hand side of the alias untouched, and then when resolving do a lookup before "cleaning" the address. If a match is found, use it. Otherwise, retry with the "clean" address.

So with this aliases file:

abc: xxx
a.b.c: yyy
abc+ppp: zzz

It results in abc -> xxx, a.bc -> xxx, abc+qqq -> xxx etc. But a.b.c -> yyy and abc+ppp -> zzz.

Pros: This is easy to reason about, and probably if a user wrote multiple alias like this, it's the behaviour they wanted. This is even more likely when considering the common case for suffixes.

Cons: A user setting an alias like name.lastname: something will find that it is only matched when the . is used, this can be really confusing and not matching their intent.

Option 3: Drop characters when parsing, do exact matches for suffixes

This is a hybrid of options 1 and 2: we "clean" the drop characters when parsing the file, leaving suffixes alone. And then do two lookups: one exact, and if that fails, one removing suffixes.

So with this aliases file:

abc: xxx
a.b.c: yyy
abc+ppp: zzz

It results in abc -> yyy, a.bc -> yyy, abc+qqq -> yyy etc. But abc+ppp -> zzz.

Pros: This is the least likely to cause confusion to users writing an alias without thinking much about it, because it matches the most common use patterns.

Cons: More complex logic, and we don't let users have aliases based on how drop characters are used.

Thoughts so far

Option 1 is my least favourite: it's too easy for users to accidentally fall into a trap and get confused.

I'm torn between options 2 and 3.

Option 2 is the easiest to explain and has more "straightforward" logic and offers a lot of flexibility, but the downside is the trap for the (I imagine) common case of name.lastname: blah. That is quite annoying.

Option 3 is more complex but I think least likely to result in confusion overall.

It's also worth keeping in mind that the aliases hooks exist, and can be used if users need really specific behaviour that is different from whatever option we choose. So let's say if we go for option 3, then a user wanting very specific behaviour can still use the hook to implement it. It is more work and not ideal, obviously, but we can factor that into the decision. The hook logic will need to be adjusted a little to make this happen, but that's not a problem.

I'm going to keep looking into this and experimenting with some of it. I have a working implementation of option 2, I will do option 3 next and see how things look, and if I find any other corner cases or problematic scenarios I have not considered.

What do you think?

Thanks!

albertito commented 11 months ago

I've implemented option 3 in branch aliases-option-3.

I am inclined to go that way, in spite of the additional complexity, because I think it results in the most intuitive/least surprising behaviour for the user.

I'm still going to keep looking into this a bit more just in case, but thought I'd put it here in case you (or anyone else) has an opinion on it!

Thank you!

albertito commented 11 months ago

After thinking about this for a while, exploring some options, and a mini-poll with a couple of friends, I ended up merging option 3 to the next branch:

When parsing aliases files, drop characters will be ignored. Suffix separators are kept as-is.

When doing lookups, drop characters will also be ignored. If the address has a suffix, the lookup will include it; if there is no match, it will try again without the suffix.

Besides some preparatory commits, this was done in 74e7c960310ce307ef5ac6505d6526308bda6678. It includes an attribution line, please take a look and let me know if you want to amend it (e.g. exclude email, use a different one, include a name, or anything else!).

I will leave this in the next branch for a bit in case something comes up, so if you think there's a better alternative, or something I missed, please let me know!

Otherwise, I will eventually merge it with master so it will be included in the upcoming release.

Thanks again!

znerol commented 11 months ago

I went over the docs in 74e7c960. I feel the new behavior is a significant improvement. Thanks @albertito .

albertito commented 11 months ago

The fix is now in master and will be included in the next release.

Thanks again!