On Wed, Jul 24, 2019 at 12:18:05AM +0200, Klemens Nanni wrote:
> On Tue, Jul 23, 2019 at 09:06:33AM +0200, Gilles Chehade wrote:
> > On Tue, Jul 23, 2019 at 08:51:54AM +0200, Sebastien Marie wrote:
> > > it seems to me this url is wrong. the '@' in username should be
> > > urlencoded.
> > >
> > > smtps://klemens%40posteo...@posteo.de:465.
> OK, according to this it is indeed "wrong", but as gilles already noted
> we do not comply with any specification in this regard anyway.
>
No, I didn't say we don't comply with any specification, I said I didn't
have the RFC in mind because I tend to think of the relay url as if it's
an internal format for smtpd and nothing more. It doesn't mean that this
format wasn't chosen because it is standardized. Sorry, I wasn't clear.
If you take any tool or library that parses an url into components, that
will parse our relay url correctly. Similarly, if you use one that build
an url based on components, it will create a valid relay url if you skip
the (optional) password and the username is a valid label in smtpd.
>>> o = urlparse('smtp+tls://la...@mail.poolp.org:587')
>>> o.scheme, o.username, o.hostname, o.port
('smtp+tls', 'label', 'mail.poolp.org', 587)
>>> b = urlunparse(o)
>>> b
'smtp+tls://la...@mail.poolp.org:587'
>>>
At the very least we should make sure that changes to the relay url will
not break this and I'm not so sure after reading semarie@'s mail.
> > Just to clarify one thing, the "username" is not really a username, it is
> > the key used to lookup the username/password pair in a table.
> The current documentation does not impose any limitation on labels.
>
> > - maybe using @ in a label is not too practical
> Why not? It is indeed quite practical as my use case shows.
>
Yes, but you are only seeing YOUR very specific use case here.
The more generic use-case is that you are using an e-mail address as the
username and by allowing @ in labels you are allowing not only yours but
other users too, and @ is far from being the only annoying character you
can find in an e-mail address.
I said using @ in a label might not be practical because we're currently
able to hide behind the fact that we make rules about what a valid label
is, whereas when we add @ any e-mail address should be usable so we need
to make sure they can be expressed without breaking relay url.
As an example, if we allow @ and consider an e-mail acceptable for label
then we need to also allow _at least_ these ones:
#define MAILADDR_ALLOWED "!#$%&'*/?^`{|}~+-=_"
Maybe they aren't an issue, but it needs to be assessed.
> > - maybe it should be urlencoded indeed
> I agree with benno that this is the job of the user.
>
And it isn't today because we have the indirection.
> > I agree that since we have a format that looks somehow standard, we
> > should at least adhere to it.
> Adhere to what exactly?
>
Well very simple:
Today the urls smtpd deal with are RFC-compliant and we can parse, build
and validate them easily with various tools.
Does the change you propose affect that ?
Does it allow smtpd to load url that can't parse, build or validate ?
> As far as I can tell, my diff does not break anything. Existing
> `relay-url' values containing more than one "@" are invalid, hence
> cannot be in use.
>
Yes, and this is a side-effect of what you're proposing, which is adding
a new use-case. Your change does not break existing setups which is good
but doesn't mean that the change in itself is correct.
> Also, to.c:text_to_mailaddr() already uses
>
> 106 username = buffer;
> 107 hostname = strrchr(username, '@');
>
In this case only one @ can be found because a mailaddr is built from
localpart where @ is disallowed and domainpart where @ is disallowed,
the two joined by a @.
See below:
> where the username may contain "@". Consider the following example to
> demonstrate why my use case should be no different:
>
> # cat smtpd.conf
> table secrets file:/etc/mail/secrets
> action "relay" relay host smtps://klem...@posteo.de@posteo.de:465 auth
> <secrets>
> ...
> # cat secrets
> klem...@posteo.de mysecret
> ...
>
No it may not contain @, if it does it means you have found a bug and we
are missing a call to valid_localpart() somehwere because a mailaddr, as
the name implies, is an e-mail address and two @ are not valid.
> From table(5) "Credentials tables"
>
> In a relay context, the credentials are a mapping of labels and
> username:password pairs, where the username may be omitted if identical
> to the label:
>
> label1 user:password
> label2 password
>
> So I am simply putting the username into the label so I don't have to
> come up with an extra label in the first place.
>
This is where the misunderstanding arises: username != e-mail address.
So you are not simply putting a username into the label, you are putting
an e-mail address into the label, which is a whole different story ... a
username is not an e-mail address, the allowed character set differ, the
length differ, they are not the same thing.
That you may use e-mail address in place of username when authenticating
at a distant MX is a thing indeed, but what it means is that credentials
consist of a username OR an e-mail, not that username and e-mail are the
same thing.
And finally, you're not "putting" the username into the label really but
using a shortcut so the label has the same value as the username. It has
an implication: what you see in the relay url is a label, not a username
nor an e-mail address, even if it looks the same. It is a label used for
an indirection into a table.
> Is that too far fetched? I think this should just work.
>
Well I'm not saying it shouldn't.
First of all, if we didn't provide the "optional" username shortcut, you
wouldn't have even raised that use-case so maybe we shouldn't even allow
that in the first place.
Then, if we do allow username AND e-mail as labels, we need to assess if
this causes issues with relay url. Is @ the only character that causes a
parsing issue (hint: no) ? Can they all be solved by a strchr -> strrchr
switch (I have no idea) ? Will it break url parsers (I have no idea) ?
Finally, if you _rely_ on the label being == your username, it means you
are considering the label as the username. If we no longer hide behind a
label that is != username and plain assume that the label is a username,
shouldn't we already plan for encoding passwords in the relay url ?
--
Gilles Chehade @poolpOrg
https://www.poolp.org patreon: https://www.patreon.com/gilles
