Re: smtpd: fix ipv6 table lookups [was: Re: IPv6 and addrname SNAFU]
• Kirill Miazine [2023-12-15 10:11]: In my previous mail I failed to realize that the colon is needed for aliases(5) so we can't "just" get rid of it. (thanks Kirill for making me noticing it -- I completely forgot that file tables split on : too.) I don't have any other ideas. Maybe we can try to be 'smart' and not split on : if the line starts with a "[" character, or maybe that's just too magic. split on : only if it is followed by whitespace would work, no? or maybe don't split as long as key is between [ and ]? and make sure ipv6 addresses are enclosed in [], to avoid confusion with addresses ending in :
Re: smtpd: fix ipv6 table lookups [was: Re: IPv6 and addrname SNAFU]
• Omar Polo [2023-12-15 10:04]: Hi, On 2023/12/14 23:15:53 +, gil...@poolp.org wrote: hello, fyi, if the first line of a file used to feed a table is @list, then it forces parsing to consider content as a serie of values, not a serie of key-value. Yeah, the @list special comment is handy to avoid ambiguities between key-value and list tables. However, in this specific case, the helo-src table, we can't use a list table, we need a kp-table. In my previous mail I failed to realize that the colon is needed for aliases(5) so we can't "just" get rid of it. (thanks Kirill for making me noticing it -- I completely forgot that file tables split on : too.) I don't have any other ideas. Maybe we can try to be 'smart' and not split on : if the line starts with a "[" character, or maybe that's just too magic. split on : only if it is followed by whitespace would work, no? and make sure ipv6 addresses are enclosed in [], to avoid confusion with addresses ending in :: P.S.: [...] After a closer look I noticed that there might be something wrong with my diff, tracing table_static_priv_add() i noticed a weird 'pattern': : adding [fe80::fce1:baff:fed3:6e35%tap0] -> (null) : adding [[fe80::fce1:baff:fed3:6e35%tap0]] -> (null) : adding [ipv6:[fe80::fce1:baff:fed3:6e35%tap0]] -> (null) that I'm not sure was present also before, so I'll have to take a closer look. This is due to config.c:set_localaddrs wrapping the result from ss_to_text() in brackets. Do you remember why it is needed? ss_to_text() already wraps ipv6 addresses in [...] (without the ipv6: "prefix" though.) Thanks, Omar Polo diff /home/op/tmp/smtpd commit - 59829af3c4da38e511c4f8e3e4a38e45fcf3b082 path + /home/op/tmp/smtpd blob - a93e09cb6cf315d1e518de697912d1e43d8695da file + config.c --- config.c +++ config.c @@ -171,7 +171,6 @@ set_localaddrs(struct smtpd *conf, struct table *local struct sockaddr_in *sain; struct sockaddr_in6 *sin6; struct table*t; - char buf[NI_MAXHOST + 5]; t = table_create(conf, "static", "", NULL); table_add(t, "local", NULL); @@ -194,8 +193,6 @@ set_localaddrs(struct smtpd *conf, struct table *local sain->sin_len = sizeof(struct sockaddr_in); table_add(t, ss_to_text(), NULL); table_add(localnames, ss_to_text(), NULL); - (void)snprintf(buf, sizeof buf, "[%s]", ss_to_text()); - table_add(localnames, buf, NULL); break; case AF_INET6: @@ -215,10 +212,6 @@ set_localaddrs(struct smtpd *conf, struct table *local #endif table_add(t, ss_to_text(), NULL); table_add(localnames, ss_to_text(), NULL); - (void)snprintf(buf, sizeof buf, "[%s]", ss_to_text()); - table_add(localnames, buf, NULL); - (void)snprintf(buf, sizeof buf, "[ipv6:%s]", ss_to_text()); - table_add(localnames, buf, NULL); break; } }
Re: smtpd: fix ipv6 table lookups [was: Re: IPv6 and addrname SNAFU]
Hi, On 2023/12/14 23:15:53 +, gil...@poolp.org wrote: > hello, > > fyi, > if the first line of a file used to feed a table is @list, then > it forces parsing to consider content as a serie of values, not > a serie of key-value. Yeah, the @list special comment is handy to avoid ambiguities between key-value and list tables. However, in this specific case, the helo-src table, we can't use a list table, we need a kp-table. In my previous mail I failed to realize that the colon is needed for aliases(5) so we can't "just" get rid of it. (thanks Kirill for making me noticing it -- I completely forgot that file tables split on : too.) I don't have any other ideas. Maybe we can try to be 'smart' and not split on : if the line starts with a "[" character, or maybe that's just too magic. P.S.: > > [...] > > After a closer look I noticed that there might be something wrong with > > my diff, tracing table_static_priv_add() i noticed a weird 'pattern': > > > > : adding [fe80::fce1:baff:fed3:6e35%tap0] -> (null) > > : adding [[fe80::fce1:baff:fed3:6e35%tap0]] -> (null) > > : adding [ipv6:[fe80::fce1:baff:fed3:6e35%tap0]] -> (null) > > > > that I'm not sure was present also before, so I'll have to take a closer > > look. This is due to config.c:set_localaddrs wrapping the result from ss_to_text() in brackets. Do you remember why it is needed? ss_to_text() already wraps ipv6 addresses in [...] (without the ipv6: "prefix" though.) Thanks, Omar Polo diff /home/op/tmp/smtpd commit - 59829af3c4da38e511c4f8e3e4a38e45fcf3b082 path + /home/op/tmp/smtpd blob - a93e09cb6cf315d1e518de697912d1e43d8695da file + config.c --- config.c +++ config.c @@ -171,7 +171,6 @@ set_localaddrs(struct smtpd *conf, struct table *local struct sockaddr_in *sain; struct sockaddr_in6 *sin6; struct table*t; - char buf[NI_MAXHOST + 5]; t = table_create(conf, "static", "", NULL); table_add(t, "local", NULL); @@ -194,8 +193,6 @@ set_localaddrs(struct smtpd *conf, struct table *local sain->sin_len = sizeof(struct sockaddr_in); table_add(t, ss_to_text(), NULL); table_add(localnames, ss_to_text(), NULL); - (void)snprintf(buf, sizeof buf, "[%s]", ss_to_text()); - table_add(localnames, buf, NULL); break; case AF_INET6: @@ -215,10 +212,6 @@ set_localaddrs(struct smtpd *conf, struct table *local #endif table_add(t, ss_to_text(), NULL); table_add(localnames, ss_to_text(), NULL); - (void)snprintf(buf, sizeof buf, "[%s]", ss_to_text()); - table_add(localnames, buf, NULL); - (void)snprintf(buf, sizeof buf, "[ipv6:%s]", ss_to_text()); - table_add(localnames, buf, NULL); break; } }
Re: smtpd: fix ipv6 table lookups [was: Re: IPv6 and addrname SNAFU]
• Omar Polo [2023-12-14 23:33]: > On 2023/12/14 21:36:44 +0100, Kirill Miazine wrote: > > [...] > > cvs checkout took looong time, so I took src.tar.gz from 7.4, applied > > patch there. > > > > diff worked for static maps, at least. tested on this config: > > thanks for testing the diff > > > [...] > > > > but didn't work with "table helo-names file:/etc/mail/helo-names", > > however, which contained: > > > > 127.0.0.1 localhost1 > > ::1 localhost2 > > [::1] localhost3 > > ipv6:::1 localhost4 > > Unfortunately this is expected, You mentioned that you didn't think makemap as-is supports ipv6 addresses, but that diff should at least fix the usage in other tables, so I gave plain file-based tables a try, too. > see table_static.c around line 172. File tables are split on ":" too > so "::1 localhost2" becomes "" -> ":1 localhost2" > > It's not clearly documented, but table(5) says > > : A file table can be converted to a Berkeley database using the makemap(8) > : utility with no syntax change. > > and makemap(8) then specifies > > : The database key and value may optionally be separated by the colon > : character. > > It sucks, it should be documented in table(5) too at least. (and even > then, I'm failing to see the value of splitting on : too given that it > breaks ipv6 addresses.) Yup, I noticed that too, and mentioned in my original email. Until being bitten by the bug, I was sure that keys and values in a file based table are separated by spaces: A mapping will be written with each key and value on a line, whitespaces separating both columns. Actually, here table(5) also contradicts with aliases(5): In a primary domain context, the key is the user part of the recipient address, whilst the value is one or many recipients as described in aliases(5): user1 otheruser user2 otheruser1,otheruser2 user3 otheru...@example.com No colons... But in aliases(5) Within the file, ‘#’ is a comment delimiter; anything placed after it is discarded. The file consists of key/value mappings of the form: key: value1, value2, value3, ... Default /etc/aliases has colons, so colon can't be removed for aliases easily... > After a closer look I noticed that there might be something wrong with > my diff, tracing table_static_priv_add() i noticed a weird 'pattern': > > : adding [fe80::fce1:baff:fed3:6e35%tap0] -> (null) > : adding [[fe80::fce1:baff:fed3:6e35%tap0]] -> (null) > : adding [ipv6:[fe80::fce1:baff:fed3:6e35%tap0]] -> (null) > > that I'm not sure was present also before, so I'll have to take a closer > look. > -- -- Kirill Miazine
Re: smtpd: fix ipv6 table lookups [was: Re: IPv6 and addrname SNAFU]
hello, fyi, if the first line of a file used to feed a table is @list, then it forces parsing to consider content as a serie of values, not a serie of key-value. gilles December 14, 2023 11:33 PM, "Omar Polo" wrote: > On 2023/12/14 21:36:44 +0100, Kirill Miazine wrote: > >> [...] >> cvs checkout took looong time, so I took src.tar.gz from 7.4, applied >> patch there. >> >> diff worked for static maps, at least. tested on this config: > > thanks for testing the diff > >> [...] >> >> but didn't work with "table helo-names file:/etc/mail/helo-names", >> however, which contained: >> >> 127.0.0.1 localhost1 >> ::1 localhost2 >> [::1] localhost3 >> ipv6:::1 localhost4 > > Unfortunately this is expected, see table_static.c around line 172. > File tables are split on ":" too so "::1 localhost2" becomes > "" -> ":1 localhost2" > > It's not clearly documented, but table(5) says > > : A file table can be converted to a Berkeley database using the makemap(8) > : utility with no syntax change. > > and makemap(8) then specifies > > : The database key and value may optionally be separated by the colon > : character. > > It sucks, it should be documented in table(5) too at least. (and even > then, I'm failing to see the value of splitting on : too given that it > breaks ipv6 addresses.) > > After a closer look I noticed that there might be something wrong with > my diff, tracing table_static_priv_add() i noticed a weird 'pattern': > > : adding [fe80::fce1:baff:fed3:6e35%tap0] -> (null) > : adding [[fe80::fce1:baff:fed3:6e35%tap0]] -> (null) > : adding [ipv6:[fe80::fce1:baff:fed3:6e35%tap0]] -> (null) > > that I'm not sure was present also before, so I'll have to take a closer > look.
Re: smtpd: fix ipv6 table lookups [was: Re: IPv6 and addrname SNAFU]
On 2023/12/14 21:36:44 +0100, Kirill Miazine wrote: > [...] > cvs checkout took looong time, so I took src.tar.gz from 7.4, applied > patch there. > > diff worked for static maps, at least. tested on this config: thanks for testing the diff > [...] > > but didn't work with "table helo-names file:/etc/mail/helo-names", > however, which contained: > > 127.0.0.1 localhost1 > ::1 localhost2 > [::1] localhost3 > ipv6:::1 localhost4 Unfortunately this is expected, see table_static.c around line 172. File tables are split on ":" too so "::1 localhost2" becomes "" -> ":1 localhost2" It's not clearly documented, but table(5) says : A file table can be converted to a Berkeley database using the makemap(8) : utility with no syntax change. and makemap(8) then specifies : The database key and value may optionally be separated by the colon : character. It sucks, it should be documented in table(5) too at least. (and even then, I'm failing to see the value of splitting on : too given that it breaks ipv6 addresses.) After a closer look I noticed that there might be something wrong with my diff, tracing table_static_priv_add() i noticed a weird 'pattern': : adding [fe80::fce1:baff:fed3:6e35%tap0] -> (null) : adding [[fe80::fce1:baff:fed3:6e35%tap0]] -> (null) : adding [ipv6:[fe80::fce1:baff:fed3:6e35%tap0]] -> (null) that I'm not sure was present also before, so I'll have to take a closer look.
Re: smtpd: fix ipv6 table lookups [was: Re: IPv6 and addrname SNAFU]
Helo, Omar
Thank you for the patch!
• Omar Polo [2023-12-14 20:01]:
[moving to tech@]
On 2023/12/13 20:37:09 +0100, Kirill Miazine wrote:
I've spent several hours debugging an issue.
table(5) specifies addrname format as a mapping from inet4 or inet6
addresses to hostnames:
::1 localhost
127.0.0.1 localhost
88.190.23.165 www.opensmtpd.org
But I can't get IPv6 mappings to work. So I've given up, and use helo
instead of helo-src.
But helo-src is useful (not on this particular setup, though).
For a month or so I had a static addrname table like this:
table helo-names { \
65.108.153.125 = com.krot.org, \
2a01:4f9:c010:9411::1 = com.krot.org
}
I suppose this was the cause of the frequent "Failed to retrieve helo
string" errors I was getting -- smtpd would not get helo name for IPv6
address.
At first I thought it was space around equal signs, but table(5) uses
space in the example. So I had to dig further.
Doing tracing, I saw that for IPv6 it -- apparently -- would be looking
up IPv6 address enclosed in brackets:
lookup: lookup "[2a01:4f9:c010:9411::1]" as ADDRNAME in table
static:helo-names -> none
So, I went ahead and did:
table helo-names { \
65.108.153.125 = com.krot.org, \
"[2a01:4f9:c010:9411::1]" = com.krot.org \
}
(by the way, in -current the \ are no longer needed.)
This didn't help, either:
lookup: lookup "[2a01:4f9:c010:9411::1]" as ADDRNAME in table
static:helo-names -> "com.krot.org"
warn: failure during helo lookup helo-names:[2a01:4f9:c010:9411::1]
Now it did find "com.krot.org", but why did it report "failure during
helo lookup"? It found a match, but still reported failure...
The issue is that smtpd fails to parse the address.
First, the sockaddr is turned into a string using sa_to_text() that
wraps ipv6 addresses in brackets. The resulting string is then searched
in the table and (if found) passed to inet_pton to get back a sockaddr,
but the [] are left in there, and so it fails.
I don't think it's safe to change sa_to_text() to not wrap ipv6
addresses in brackets, but we can adjust parse_sockaddr().
See diff below.
Thinking that it could be a static-map issue, I did a file: map. First
as documented in the man page:
2a01:4f9:c010:9411::1 com.krot.org
65.108.153.125 com.krot.org
This didn't work:
lookup: lookup "[2a01:4f9:c010:9411::1]" as ADDRNAME in table
static:helo-names -> none
Then with square brackets:
65.108.153.125 com.krot.org
[2a01:4f9:c010:9411::1] com.krot.org
And this time square bracked didn't help.
lookup: lookup "[2a01:4f9:c010:9411::1]" as ADDRNAME in table
static:helo-names -> none
New try with db:
makemap -d hash -o helo-names.db -t set helo-names
When reading makemap(8), I noticed this:
In all cases, makemap reads lines consisting of words separated by
whitespace. The first word of a line is the database key; the remainder
represents the mapped value. The database key and value may optionally
be separated by the colon character.
Colon character! In makemap.c there's:
strsep(, " \t:");
So it would be splitting IPv6 addresses, IIUC.
table(5) doesn't say anything about colons.
So I did a dump, and it indeed does separate the IPv6 address:
# makemap -U helo-names.db
65.108.153.125 com.krot.org
[2a01 4f9:c010:9411::1] com.krot.org
How would one put IPv6 addresses in such a map?
I don't think makemap as-is supports ipv6 addresses, but this should at
least fix the usage in other tables.
Can you please test it and report back if it works? I have to admit I
failed to test helo-src locally so I hacked up something in smtpd.c to
perform a lookup in order to test this.
(I *believe* the diff should apply to -portable as well as-is.)
cvs checkout took looong time, so I took src.tar.gz from 7.4, applied
patch there.
diff worked for static maps, at least. tested on this config:
table local-interfaces { ::1 127.0.0.1 }
table helo-names { \
127.0.0.1 = localhost, \
::1 = localhost, \
"[::1]" = localhost, \
ipv6:::1 = localhost \
}
listen on socket
listen on lo0
action mail_relay relay host smtp://[::1] \
src \
helo-src
match from local for any action mail_relay
from logs:
mproc: dispatcher -> lka : 56 IMSG_MTA_LOOKUP_HELO
imsg: lka <- dispatcher: IMSG_MTA_LOOKUP_HELO (len=56)
lookup: lookup "[::1]" as ADDRNAME in table static:helo-names -> "localhost"
mproc: lka -> dispatcher : 23 IMSG_MTA_LOOKUP_HELO
imsg: dispatcher <- lka: IMSG_MTA_LOOKUP_HELO (len=23)
[...]
d147a0751842134 smtp connected address=[::1] host=localhost.localdomain
smtp: 0x1d0c969: >>> 220 build ESMTP OpenSMTPD
smtp: 0x1d0c969: IO_LOWAT ib=0 ob=0>
mta: 0x1d084c9d6d0: IO_DATAIN ib=27 ob=0>
mta: 0x1d084c9d6d0: <<< 220 build ESMTP OpenSMTPD
mta: 0x1d084c9d6d0: MTA_BANNER -> MTA_EHLO
mta: 0x1d084c9d6d0: >>> EHLO localhost
so this is good!
but didn't work with "table helo-names
