From: Omar Polo Subject: Re: smtpd/makemap vs ipv6 addresses in table(5)s To: tech@openbsd.org Cc: Gilles Chehade Date: Fri, 01 Mar 2024 22:37:07 +0100 friendly ping, anyone has an opinion? Omar Polo wrote: > On Tue Feb 20, 2024 at 10:59 PM CET, Gilles Chehade wrote: > > On Tue, Feb 20, 2024 at 05:10:52PM +0100, Omar Polo wrote: > > > [...] > > > So, I'm proposing to require ipv6 addresses to be wrapped in braces in > > > table(5) files, and to amend the parser to skip over to the matching ] > > > if the line starts with [. Is it enough or should it be more strict? > > > > > > > I dunno if it should be more strict but that would already be a good > > start and an improvement over today. > > One downside I realized only later is that it still allows gibberish > after the ip addr, as in "[::1]foo". > > > > [...] > > > > > > thoughs? > > > > > > > Generally ok with this but also starting to wonder if makemap and static > > tables parsing would not benefit from a regex(3)-based line parser to be > > honest. It's not used anywhere an expensive regex would be an issue, and > > it would be more maintainable than having to read it every ten year when > > someone hits a case we didn't hit before. > > > > Dunno, just opening the discussion > > Not sure if a regexp is needed here, but for sure it could be > interesting to eventually settle on some other details of the parser. > For example, I these confusing: > > foo :: bar becomes "foo" -> ":: bar" > foo::bar becomes "foo" -> ":bar" > foo:: bar becomes "foo" -> "bar" > > so, some details should be agreed upon regardless of the implementation > of the parser. > > my diff also introduces another ambiguity: what to do when there > is no closing ']'? does it makes sense for keys to include '[' > outside of ipv6 addresses? (I guess no, but don't know for sure.) > > Here's another try, where I may have gone too far, which considers > a parse error if there is no closing ] and also improves IMHO the > splitting of key-value. Using the same example table as before, > now it becomes: > > suzaku% cat t > [::1]: bar > [::2] bar > foo1 bar > foo2: bar > foo3:bar > foo4 : bar > foo5 :: bar > foo6::bar > foo7:: bar > suzaku% ./makemap t && ./makemap -U t.db | sort > [::1] bar > [::2] bar > foo1 bar > foo2 bar > foo3 bar > foo4 : bar > foo5 :: bar > foo6 :bar > foo7 : bar > > i.e. it splits on the first space or colon, and then trims the > spaces. Multiple colons are not trimmed. Does anybody have aliases > lines with "foo : bar"? > > > diffstat /home/op/w/smtpd > M table.5 | 10+ 7- > M util.c | 15+ 23- > > 2 files changed, 25 insertions(+), 30 deletions(-) > > diff /home/op/w/smtpd > commit - 0e1879aafc8d52781267651a6455236cadaf2b54 > path + /home/op/w/smtpd > blob - 65dca359f38d257db5d8c948374a8433c15e1a9d > file + table.5 > --- table.5 > +++ table.5 > @@ -173,10 +173,11 @@ of addresses in the table until a match is found. > A netaddr table can contain exact addresses or netmasks, and looks as follow: > .Bd -literal -offset indent > 192.168.1.1 > -::1 > -ipv6:::1 > +[::1] > 192.168.1.0/24 > .Ed > +.Pp > +IPv6 addresses must be enclosed in square brackets. > .Ss Userinfo tables > Userinfo tables are used in rule context to specify an alternate userbase, > mapping virtual users to local system users by UID, GID and home directory. > @@ -214,11 +215,11 @@ A source table looks as follow: > .Bd -literal -offset indent > 192.168.1.2 > 192.168.1.3 > -::1 > -::2 > -ipv6:::3 > -ipv6:::4 > +[::1] > +[::2] > .Ed > +.Pp > +IPv6 address must be enclosed in square brackets. > .Ss Mailaddr tables > Mailaddr tables are lists of email addresses. > They can be used in the following contexts: > @@ -254,10 +255,12 @@ outgoing connection. > .Pp > The format is a mapping from inet4 or inet6 addresses to hostnames: > .Bd -literal -offset indent > -::1 localhost > +[::1] localhost > 127.0.0.1 localhost > 88.190.23.165 www.opensmtpd.org > .Ed > +.Pp > +IPv6 addresses must be enclosed in square brackets. > .Sh SEE ALSO > .Xr smtpd.conf 5 , > .Xr makemap 8 , > blob - d481162c8973a288e6470ba1ef1367ad487b8d2d > file + util.c > --- util.c > +++ util.c > @@ -855,7 +855,7 @@ int > parse_table_line(FILE *fp, char **line, size_t *linesize, > int *type, char **key, char **val, int *malformed) > { > - char *keyp, *valp, *p; > + char *keyp, *valp; > ssize_t linelen; > > *key = NULL; > @@ -885,37 +885,29 @@ parse_table_line(FILE *fp, char **line, size_t *linesi > return 0; > } > > - if (*type == T_NONE) { > - for (p = keyp; *p; p++) { > - if (*p == ' ' || *p == '\t' || *p == ':') { > - *type = T_HASH; > - break; > - } > + if (*keyp == '[') { > + if ((valp = strchr(keyp, ']')) == NULL) { > + *malformed = 1; > + return (0); > } > - if (*type == T_NONE) > - *type = T_LIST; > - } > + valp++; > + } else > + valp = keyp + strcspn(keyp, " \t:"); > > + if (*type == T_NONE) > + *type = (*valp == '\0') ? T_LIST : T_HASH; > + > if (*type == T_LIST) { > *key = keyp; > return (0); > } > > /* T_HASH */ > - valp = keyp; > - strsep(&valp, " \t:"); > - if (valp) { > - while (*valp) { > - if (!isspace((unsigned char)*valp) && > - !(*valp == ':' && > - isspace((unsigned char)*(valp + 1)))) > - break; > - ++valp; > - } > - if (*valp == '\0') > - valp = NULL; > + if (*valp != '\0') { > + *valp++ = '\0'; > + valp += strspn(valp, " \t"); > } > - if (valp == NULL) > + if (*valp == '\0') > *malformed = 1; > > *key = keyp;