[moving to tech@]
On 2023/12/13 20:37:09 +0100, Kirill Miazine <k...@krot.org> 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(&valp, " \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.)
diff /tmp/smtpd
commit - 59829af3c4da38e511c4f8e3e4a38e45fcf3b082
path + /tmp/smtpd
blob - 7328cf5df6eb0034e9bb5c91b59c74bd3d402db4
file + table.c
--- table.c
+++ table.c
@@ -628,7 +628,8 @@ parse_sockaddr(struct sockaddr *sa, int family, const
struct in6_addr in6a;
struct sockaddr_in *sin;
struct sockaddr_in6 *sin6;
- char *cp, *str2;
+ char *cp;
+ char addr[NI_MAXHOST];
const char *errstr;
switch (family) {
@@ -649,23 +650,21 @@ parse_sockaddr(struct sockaddr *sa, int family, const
return (0);
case PF_INET6:
- if (strncasecmp("ipv6:", str, 5) == 0)
+ if (*str == '[')
+ str++;
+ if (!strncasecmp("ipv6:", str, 5))
str += 5;
- cp = strchr(str, SCOPE_DELIMITER);
- if (cp) {
- str2 = strdup(str);
- if (str2 == NULL)
- return (-1);
- str2[cp - str] = '\0';
- if (inet_pton(PF_INET6, str2, &in6a) != 1) {
- free(str2);
- return (-1);
- }
- cp++;
- free(str2);
- } else if (inet_pton(PF_INET6, str, &in6a) != 1)
+
+ if (strlcpy(addr, str, sizeof(addr)) >= sizeof(addr))
return (-1);
+ if ((cp = strchr(addr, ']')) != NULL)
+ *cp = '\0';
+ if ((cp = strchr(addr, SCOPE_DELIMITER)) != NULL)
+ *cp++ = '\0';
+ if (inet_pton(PF_INET6, addr, &in6a) != 1)
+ return (-1);
+
sin6 = (struct sockaddr_in6 *)sa;
memset(sin6, 0, sizeof *sin6);
sin6->sin6_len = sizeof(struct sockaddr_in6);
