Re: smtpd: Allow labels containing "@"

2019-07-24 Thread Gilles Chehade
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 
> 
>   ...
>   # 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 

Re: smtpd: Allow labels containing "@"

2019-07-24 Thread Gilles Chehade
On Tue, Jul 23, 2019 at 10:20:10PM +0200, Sebastian Benoit wrote:
> Gilles Chehade(gil...@poolp.org) on 2019.07.23 09:06:33 +0200:
> > On Tue, Jul 23, 2019 at 08:51:54AM +0200, Sebastien Marie wrote:
> > > On Mon, Jul 22, 2019 at 11:26:28PM +0200, Klemens Nanni wrote:
> > > > My mail is klem...@posteo.de and the provider expects this full address
> > > > as username, so that makes for the following perfectly
> > > > valid SMTP URL smtps://klem...@posteo.de@posteo.de:465.
> > > 
> > > it seems to me this url is wrong. the '@' in username should be 
> > > urlencoded.
> > > 
> > >   smtps://klemens%40posteo...@posteo.de:465.
> > > 
> > > RFC3986 Uniform Resource Identifier (URI): Generic Syntax
> > > 
> > >   3.2.1.  User Information
> > > 
> > >   The userinfo subcomponent may consist of a user name and, optionally,
> > >   scheme-specific information about how to gain authorization to access
> > >   the resource.  The user information, if present, is followed by a
> > >   commercial at-sign ("@") that delimits it from the host.
> > > 
> > >   userinfo= *( unreserved / pct-encoded / sub-delims / ":" )
> > > 
> > 
> > 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.
> > 
> > that being said:
> > 
> > - maybe using @ in a label is not too practical
> > - maybe it should be urlencoded indeed
> > 
> > I didn't think about RFC3986 because we're really using the relay url as
> > an internal format between smtpd processes, not as a standardized format
> > so it could as well be schema=smtp,label=klem...@posteo.de,host=posteo.de
> > 
> > I agree that since we have a format that looks somehow standard, we
> > should at least adhere to it.
> 
> Why should i, when typing something in the config file have to care about
> url-encoding? Should this not just be done in the parser?
> 

There are two things that are being mixed here.

1- you don't need to care about url-encoding, no one has ever had to use
   url-encoding in credentials and as of today you can use whatever user
   and password without relying on url-encoding ... because they are not
   part of the relay url but rely on an indirection to a table. keep not
   caring about url-encoding as much as I don't.

2- kn@ is bringing a NEW use-case where he wants to use the username for
   the label, this raises questions because unlike the indirection, more
   characters than just @ need to be escaped.

I see kn@ has sent another mail so I will explain further in my reply to
him.

-- 
Gilles Chehade @poolpOrg

https://www.poolp.orgpatreon: https://www.patreon.com/gilles



Re: smtpd: Allow labels containing "@"

2019-07-23 Thread Klemens Nanni
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.
 
> 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.

> - maybe it should be urlencoded indeed
I agree with benno that this is the job of the user.

> I didn't think about RFC3986 because we're really using the relay url as
> an internal format between smtpd processes, not as a standardized format
> so it could as well be schema=smtp,label=klem...@posteo.de,host=posteo.de
Looks fine to me per se but this would be a much bigger and breaking
change I do not want to persue.

> I agree that since we have a format that looks somehow standard, we
> should at least adhere to it.
Adhere to what exactly?

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.

Also, to.c:text_to_mailaddr() already uses

106 username = buffer;
107 hostname = strrchr(username, '@');

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 

...
# cat secrets
klem...@posteo.de   mysecret
...

>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.

Is that too far fetched?  I think this should just work.



Re: smtpd: Allow labels containing "@"

2019-07-23 Thread Sebastian Benoit
Gilles Chehade(gil...@poolp.org) on 2019.07.23 09:06:33 +0200:
> On Tue, Jul 23, 2019 at 08:51:54AM +0200, Sebastien Marie wrote:
> > On Mon, Jul 22, 2019 at 11:26:28PM +0200, Klemens Nanni wrote:
> > > My mail is klem...@posteo.de and the provider expects this full address
> > > as username, so that makes for the following perfectly
> > > valid SMTP URL smtps://klem...@posteo.de@posteo.de:465.
> > 
> > it seems to me this url is wrong. the '@' in username should be urlencoded.
> > 
> > smtps://klemens%40posteo...@posteo.de:465.
> > 
> > RFC3986 Uniform Resource Identifier (URI): Generic Syntax
> > 
> >   3.2.1.  User Information
> > 
> > The userinfo subcomponent may consist of a user name and, optionally,
> > scheme-specific information about how to gain authorization to access
> > the resource.  The user information, if present, is followed by a
> > commercial at-sign ("@") that delimits it from the host.
> > 
> > userinfo= *( unreserved / pct-encoded / sub-delims / ":" )
> > 
> 
> 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.
> 
> that being said:
> 
> - maybe using @ in a label is not too practical
> - maybe it should be urlencoded indeed
> 
> I didn't think about RFC3986 because we're really using the relay url as
> an internal format between smtpd processes, not as a standardized format
> so it could as well be schema=smtp,label=klem...@posteo.de,host=posteo.de
> 
> I agree that since we have a format that looks somehow standard, we
> should at least adhere to it.

Why should i, when typing something in the config file have to care about
url-encoding? Should this not just be done in the parser?

/Benno

> 
> my previous ok is withdrawned, sorry kn@



Re: smtpd: Allow labels containing "@"

2019-07-23 Thread Gilles Chehade
On Tue, Jul 23, 2019 at 08:51:54AM +0200, Sebastien Marie wrote:
> On Mon, Jul 22, 2019 at 11:26:28PM +0200, Klemens Nanni wrote:
> > My mail is klem...@posteo.de and the provider expects this full address
> > as username, so that makes for the following perfectly
> > valid SMTP URL smtps://klem...@posteo.de@posteo.de:465.
> 
> it seems to me this url is wrong. the '@' in username should be urlencoded.
> 
>   smtps://klemens%40posteo...@posteo.de:465.
> 
> RFC3986 Uniform Resource Identifier (URI): Generic Syntax
> 
>   3.2.1.  User Information
> 
>   The userinfo subcomponent may consist of a user name and, optionally,
>   scheme-specific information about how to gain authorization to access
>   the resource.  The user information, if present, is followed by a
>   commercial at-sign ("@") that delimits it from the host.
> 
>   userinfo= *( unreserved / pct-encoded / sub-delims / ":" )
> 

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.

that being said:

- maybe using @ in a label is not too practical
- maybe it should be urlencoded indeed

I didn't think about RFC3986 because we're really using the relay url as
an internal format between smtpd processes, not as a standardized format
so it could as well be schema=smtp,label=klem...@posteo.de,host=posteo.de

I agree that since we have a format that looks somehow standard, we
should at least adhere to it.

my previous ok is withdrawned, sorry kn@



> > I've been doing that with mutt(1) for ages.
> > 
> > smtpd.conf(5) has the following syntax:
> > 
> > host relay-url
> > Do not perform MX lookups but relay messages to the relay
> > host described by relay-url.  The format for relay-url is
> > [proto://[label@]]host[:port].  The following protocols
> > are available:
> > 
> > yielding the following config in my case:
> > 
> > action "relay" relay host smtps://klem...@posteo.de@posteo.de:465 auth 
> > 
> > 
> > However, when parsing the label in the `relay-url', smtpd(8) stops at
> > the first "@" sign, not expecting labels to contain it.  The following
> > diff fixes this spanning the label to the last occurence of "@" as is
> > already done in other code places.
> > 
> > Feedback? Objections?
> > OK?
> > 
> > Index: to.c
> > ===
> > RCS file: /cvs/src/usr.sbin/smtpd/to.c,v
> > retrieving revision 1.35
> > diff -u -p -r1.35 to.c
> > --- to.c30 Dec 2018 23:09:58 -  1.35
> > +++ to.c22 Jul 2019 21:02:48 -
> > @@ -352,7 +352,7 @@ text_to_relayhost(struct relayhost *rela
> > relay->port = 0;
> >  
> > /* first, we extract the label if any */
> > -   if ((q = strchr(p, '@')) != NULL) {
> > +   if ((q = strrchr(p, '@')) != NULL) {
> > *q = 0;
> > if (strlcpy(relay->authlabel, p, sizeof (relay->authlabel))
> > >= sizeof (relay->authlabel))
> > 
> 
> -- 
> Sebastien Marie
> 

-- 
Gilles Chehade @poolpOrg

https://www.poolp.orgpatreon: https://www.patreon.com/gilles



Re: smtpd: Allow labels containing "@"

2019-07-23 Thread Sebastien Marie
On Mon, Jul 22, 2019 at 11:26:28PM +0200, Klemens Nanni wrote:
> My mail is klem...@posteo.de and the provider expects this full address
> as username, so that makes for the following perfectly
> valid SMTP URL smtps://klem...@posteo.de@posteo.de:465.

it seems to me this url is wrong. the '@' in username should be urlencoded.

smtps://klemens%40posteo...@posteo.de:465.

RFC3986 Uniform Resource Identifier (URI): Generic Syntax

  3.2.1.  User Information

The userinfo subcomponent may consist of a user name and, optionally,
scheme-specific information about how to gain authorization to access
the resource.  The user information, if present, is followed by a
commercial at-sign ("@") that delimits it from the host.

userinfo= *( unreserved / pct-encoded / sub-delims / ":" )


> I've been doing that with mutt(1) for ages.
> 
> smtpd.conf(5) has the following syntax:
> 
>   host relay-url
>   Do not perform MX lookups but relay messages to the relay
>   host described by relay-url.  The format for relay-url is
>   [proto://[label@]]host[:port].  The following protocols
>   are available:
> 
> yielding the following config in my case:
> 
>   action "relay" relay host smtps://klem...@posteo.de@posteo.de:465 auth 
> 
> 
> However, when parsing the label in the `relay-url', smtpd(8) stops at
> the first "@" sign, not expecting labels to contain it.  The following
> diff fixes this spanning the label to the last occurence of "@" as is
> already done in other code places.
> 
> Feedback? Objections?
> OK?
> 
> Index: to.c
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/to.c,v
> retrieving revision 1.35
> diff -u -p -r1.35 to.c
> --- to.c  30 Dec 2018 23:09:58 -  1.35
> +++ to.c  22 Jul 2019 21:02:48 -
> @@ -352,7 +352,7 @@ text_to_relayhost(struct relayhost *rela
>   relay->port = 0;
>  
>   /* first, we extract the label if any */
> - if ((q = strchr(p, '@')) != NULL) {
> + if ((q = strrchr(p, '@')) != NULL) {
>   *q = 0;
>   if (strlcpy(relay->authlabel, p, sizeof (relay->authlabel))
>   >= sizeof (relay->authlabel))
> 

-- 
Sebastien Marie



Re: smtpd: Allow labels containing "@"

2019-07-22 Thread Gilles Chehade
On Mon, Jul 22, 2019 at 11:26:28PM +0200, Klemens Nanni wrote:
> My mail is klem...@posteo.de and the provider expects this full address
> as username, so that makes for the following perfectly
> valid SMTP URL smtps://klem...@posteo.de@posteo.de:465.
> 
> I've been doing that with mutt(1) for ages.
> 
> smtpd.conf(5) has the following syntax:
> 
>   host relay-url
>   Do not perform MX lookups but relay messages to the relay
>   host described by relay-url.  The format for relay-url is
>   [proto://[label@]]host[:port].  The following protocols
>   are available:
> 
> yielding the following config in my case:
> 
>   action "relay" relay host smtps://klem...@posteo.de@posteo.de:465 auth 
> 
> 
> However, when parsing the label in the `relay-url', smtpd(8) stops at
> the first "@" sign, not expecting labels to contain it.  The following
> diff fixes this spanning the label to the last occurence of "@" as is
> already done in other code places.
> 
> Feedback? Objections?
> OK?
> 

no objection, ok


> Index: to.c
> ===
> RCS file: /cvs/src/usr.sbin/smtpd/to.c,v
> retrieving revision 1.35
> diff -u -p -r1.35 to.c
> --- to.c  30 Dec 2018 23:09:58 -  1.35
> +++ to.c  22 Jul 2019 21:02:48 -
> @@ -352,7 +352,7 @@ text_to_relayhost(struct relayhost *rela
>   relay->port = 0;
>  
>   /* first, we extract the label if any */
> - if ((q = strchr(p, '@')) != NULL) {
> + if ((q = strrchr(p, '@')) != NULL) {
>   *q = 0;
>   if (strlcpy(relay->authlabel, p, sizeof (relay->authlabel))
>   >= sizeof (relay->authlabel))
> 

-- 
Gilles Chehade @poolpOrg

https://www.poolp.orgpatreon: https://www.patreon.com/gilles



smtpd: Allow labels containing "@"

2019-07-22 Thread Klemens Nanni
My mail is klem...@posteo.de and the provider expects this full address
as username, so that makes for the following perfectly
valid SMTP URL smtps://klem...@posteo.de@posteo.de:465.

I've been doing that with mutt(1) for ages.

smtpd.conf(5) has the following syntax:

host relay-url
Do not perform MX lookups but relay messages to the relay
host described by relay-url.  The format for relay-url is
[proto://[label@]]host[:port].  The following protocols
are available:

yielding the following config in my case:

action "relay" relay host smtps://klem...@posteo.de@posteo.de:465 auth 


However, when parsing the label in the `relay-url', smtpd(8) stops at
the first "@" sign, not expecting labels to contain it.  The following
diff fixes this spanning the label to the last occurence of "@" as is
already done in other code places.

Feedback? Objections?
OK?

Index: to.c
===
RCS file: /cvs/src/usr.sbin/smtpd/to.c,v
retrieving revision 1.35
diff -u -p -r1.35 to.c
--- to.c30 Dec 2018 23:09:58 -  1.35
+++ to.c22 Jul 2019 21:02:48 -
@@ -352,7 +352,7 @@ text_to_relayhost(struct relayhost *rela
relay->port = 0;
 
/* first, we extract the label if any */
-   if ((q = strchr(p, '@')) != NULL) {
+   if ((q = strrchr(p, '@')) != NULL) {
*q = 0;
if (strlcpy(relay->authlabel, p, sizeof (relay->authlabel))
>= sizeof (relay->authlabel))