On Tue, Nov 12, 2019 at 02:16:25PM +0100, Quentin Rameau wrote:
Hello
> Hello,
>
> Here's a patch for smtpctl spf resolution, adding support for target
> specified as a hostname + cidr.
>
> Yes, SPF lets you specify targets like a:example.com/24.
>
> Due to the async and recursive nature of DNS resolution in spfwalk.c,
> it's kind of hard to pass data around without too much memory
> allocation, so I decided to just pass cidr(s) as integer value instead
> of keeping the original string and passing pointers around.
>
> Comments welcome, thanks!
I find it confusing that everything is stuffed into the sender struct,
including the callback. I don't really see the point. If you don't
need to pass the target value over to the callback, there is no need
to have it in the struct either, especially since the pointer is only
valid at the time you call the lookup function.
Another point, I don't understand how the parse_sender() function is
supposed to work. Can you give examples with cidr4 and cidr6?
Eric.
>
> Index: usr.sbin/smtpd/spfwalk.c
> ===================================================================
> RCS file: /var/cvs/src/usr.sbin/smtpd/spfwalk.c,v
> retrieving revision 1.15
> diff -u -r1.15 spfwalk.c
> --- usr.sbin/smtpd/spfwalk.c 11 Nov 2019 17:20:25 -0000 1.15
> +++ usr.sbin/smtpd/spfwalk.c 12 Nov 2019 12:43:25 -0000
> @@ -40,15 +40,24 @@
> #include "unpack_dns.h"
> #include "parser.h"
>
> +struct sender {
> + void (*cb)(struct dns_rr *, struct sender *);
> + char *target;
> + int cidr4;
> + int cidr6;
> +};
> +
> +void *xmalloc(size_t);
> int spfwalk(int, struct parameter *);
>
> -static void dispatch_txt(struct dns_rr *);
> -static void dispatch_mx(struct dns_rr *);
> -static void dispatch_a(struct dns_rr *);
> -static void dispatch_aaaa(struct dns_rr *);
> -static void lookup_record(int, const char *, void (*)(struct dns_rr *));
> +static void dispatch_txt(struct dns_rr *, struct sender *);
> +static void dispatch_mx(struct dns_rr *, struct sender *);
> +static void dispatch_a(struct dns_rr *, struct sender *);
> +static void dispatch_aaaa(struct dns_rr *, struct sender *);
> +static void lookup_record(int, struct sender *);
> static void dispatch_record(struct asr_result *, void *);
> static ssize_t parse_txt(const char *, size_t, char *, size_t);
> +static int parse_sender(struct sender *);
>
> int ip_v4 = 0;
> int ip_v6 = 0;
> @@ -59,6 +68,7 @@
> int
> spfwalk(int argc, struct parameter *argv)
> {
> + struct sender sdr;
> const char *ip_family = NULL;
> char *line = NULL;
> size_t linesize = 0;
> @@ -81,12 +91,17 @@
> dict_init(&seen);
> event_init();
>
> + sdr.cidr4 = sdr.cidr6 = -1;
> + sdr.cb = dispatch_txt;
> +
> while ((linelen = getline(&line, &linesize, stdin)) != -1) {
> while (linelen-- > 0 && isspace(line[linelen]))
> line[linelen] = '\0';
>
> - if (linelen > 0)
> - lookup_record(T_TXT, line, dispatch_txt);
> + if (linelen > 0) {
> + sdr.target = line;
> + lookup_record(T_TXT, &sdr);
> + }
> }
>
> free(line);
> @@ -100,20 +115,23 @@
> }
>
> void
> -lookup_record(int type, const char *record, void (*cb)(struct dns_rr *))
> +lookup_record(int type, struct sender *sdr)
> {
> struct asr_query *as;
> + struct sender *nsdr;
>
> - as = res_query_async(record, C_IN, type, NULL);
> + as = res_query_async(sdr->target, C_IN, type, NULL);
> if (as == NULL)
> err(1, "res_query_async");
> - event_asr_run(as, dispatch_record, cb);
> + nsdr = xmalloc(sizeof(*nsdr));
> + *nsdr = *sdr;
> + event_asr_run(as, dispatch_record, (void *)nsdr);
> }
>
> void
> dispatch_record(struct asr_result *ar, void *arg)
> {
> - void (*cb)(struct dns_rr *) = arg;
> + struct sender *sdr = arg;
> struct unpack pack;
> struct dns_header h;
> struct dns_query q;
> @@ -121,7 +139,7 @@
>
> /* best effort */
> if (ar->ar_h_errno && ar->ar_h_errno != NO_DATA)
> - return;
> + goto end;
>
> unpack_init(&pack, ar->ar_data, ar->ar_datalen);
> unpack_header(&pack, &h);
> @@ -130,19 +148,22 @@
> for (; h.ancount; h.ancount--) {
> unpack_rr(&pack, &rr);
> /**/
> - cb(&rr);
> + sdr->cb(&rr, sdr);
> }
> +end:
> + free(sdr);
> }
>
> void
> -dispatch_txt(struct dns_rr *rr)
> +dispatch_txt(struct dns_rr *rr, struct sender *sdr)
> {
> + char buf[4096];
> + char *argv[512];
> + char buf2[512];
> + struct sender lsdr;
> struct in6_addr ina;
> - char buf[4096];
> - char buf2[512];
> - char *in = buf;
> - char *argv[512];
> - char **ap = argv;
> + char **ap = argv;
> + char *in = buf;
> char *end;
> ssize_t n;
>
> @@ -173,6 +194,8 @@
> if (**ap == '+' || **ap == '?')
> (*ap)++;
>
> + lsdr.cidr4 = lsdr.cidr6 = -1;
> +
> if (strncasecmp("ip4:", *ap, 4) == 0) {
> if ((ip_v4 == 1 || ip_both == 1) &&
> inet_net_pton(AF_INET, *(ap) + 4,
> @@ -190,35 +213,55 @@
> if (strcasecmp("a", *ap) == 0) {
> print_dname(rr->rr_dname, buf2, sizeof(buf2));
> buf2[strlen(buf2) - 1] = '\0';
> - lookup_record(T_A, buf2, dispatch_a);
> - lookup_record(T_AAAA, buf2, dispatch_aaaa);
> + lsdr.target = buf2;
> + lsdr.cb = dispatch_a;
> + lookup_record(T_A, &lsdr);
> + lsdr.cb = dispatch_aaaa;
> + lookup_record(T_AAAA, &lsdr);
> continue;
> }
> if (strncasecmp("a:", *ap, 2) == 0) {
> - lookup_record(T_A, *(ap) + 2, dispatch_a);
> - lookup_record(T_AAAA, *(ap) + 2, dispatch_aaaa);
> + lsdr.target = *(ap) + 2;
> + if (parse_sender(&lsdr) < 0)
> + continue;
> + lsdr.cb = dispatch_a;
> + lookup_record(T_A, &lsdr);
> + lsdr.cb = dispatch_aaaa;
> + lookup_record(T_AAAA, &lsdr);
> continue;
> }
> if (strncasecmp("exists:", *ap, 7) == 0) {
> - lookup_record(T_A, *(ap) + 7, dispatch_a);
> + lsdr.target = *(ap) + 7;
> + lsdr.cb = dispatch_a;
> + lookup_record(T_A, &lsdr);
> continue;
> }
> if (strncasecmp("include:", *ap, 8) == 0) {
> - lookup_record(T_TXT, *(ap) + 8, dispatch_txt);
> + lsdr.target = *(ap) + 8;
> + lsdr.cb = dispatch_txt;
> + lookup_record(T_TXT, &lsdr);
> continue;
> }
> if (strncasecmp("redirect=", *ap, 9) == 0) {
> - lookup_record(T_TXT, *(ap) + 9, dispatch_txt);
> + lsdr.target = *(ap) + 9;
> + lsdr.cb = dispatch_txt;
> + lookup_record(T_TXT, &lsdr);
> continue;
> }
> if (strcasecmp("mx", *ap) == 0) {
> print_dname(rr->rr_dname, buf2, sizeof(buf2));
> buf2[strlen(buf2) - 1] = '\0';
> - lookup_record(T_MX, buf2, dispatch_mx);
> + lsdr.target = buf2;
> + lsdr.cb = dispatch_mx;
> + lookup_record(T_MX, &lsdr);
> continue;
> }
> if (strncasecmp("mx:", *ap, 3) == 0) {
> - lookup_record(T_MX, *(ap) + 3, dispatch_mx);
> + lsdr.target = *(ap) + 3;
> + if (parse_sender(&lsdr) < 0)
> + continue;
> + lsdr.cb = dispatch_mx;
> + lookup_record(T_MX, &lsdr);
> continue;
> }
> }
> @@ -226,9 +269,10 @@
> }
>
> void
> -dispatch_mx(struct dns_rr *rr)
> +dispatch_mx(struct dns_rr *rr, struct sender *sdr)
> {
> char buf[512];
> + struct sender lsdr;
>
> if (rr->rr_type != T_MX)
> return;
> @@ -237,12 +281,16 @@
> buf[strlen(buf) - 1] = '\0';
> if (buf[strlen(buf) - 1] == '.')
> buf[strlen(buf) - 1] = '\0';
> - lookup_record(T_A, buf, dispatch_a);
> - lookup_record(T_AAAA, buf, dispatch_aaaa);
> + lsdr.target = buf;
> + lsdr.cidr4 = lsdr.cidr6 = -1;
> + lsdr.cb = dispatch_a;
> + lookup_record(T_A, &lsdr);
> + lsdr.cb = dispatch_aaaa;
> + lookup_record(T_AAAA, &lsdr);
> }
>
> void
> -dispatch_a(struct dns_rr *rr)
> +dispatch_a(struct dns_rr *rr, struct sender *sdr)
> {
> char buffer[512];
> const char *ptr;
> @@ -251,12 +299,16 @@
> return;
>
> if ((ptr = inet_ntop(AF_INET, &rr->rr.in_a.addr,
> - buffer, sizeof buffer)))
> - printf("%s\n", ptr);
> + buffer, sizeof buffer))) {
> + if (sdr->cidr4 >= 0)
> + printf("%s/%d\n", ptr, sdr->cidr4);
> + else
> + printf("%s\n", ptr);
> + }
> }
>
> void
> -dispatch_aaaa(struct dns_rr *rr)
> +dispatch_aaaa(struct dns_rr *rr, struct sender *sdr)
> {
> char buffer[512];
> const char *ptr;
> @@ -265,11 +317,15 @@
> return;
>
> if ((ptr = inet_ntop(AF_INET6, &rr->rr.in_aaaa.addr6,
> - buffer, sizeof buffer)))
> - printf("%s\n", ptr);
> + buffer, sizeof buffer))) {
> + if (sdr->cidr6 >= 0)
> + printf("%s/%d\n", ptr, sdr->cidr6);
> + else
> + printf("%s\n", ptr);
> + }
> }
>
> -static ssize_t
> +ssize_t
> parse_txt(const char *rdata, size_t rdatalen, char *dst, size_t dstsz)
> {
> size_t len;
> @@ -302,4 +358,31 @@
> }
>
> return r;
> +}
> +
> +int
> +parse_sender(struct sender *sdr)
> +{
> + const char *err;
> + char *m4, *m6;
> +
> + m4 = sdr->target;
> + strsep(&m4, "/");
> + if (m4 == NULL)
> + return 0;
> + m6 = m4;
> + strsep(&m6, "/");
> +
> + sdr->cidr4 = strtonum(m4, 0, 32, &err);
> + if (err)
> + return sdr->cidr4 = -1;
> +
> + if (m6 == NULL)
> + return 0;
> +
> + sdr->cidr6 = strtonum(m6, 0, 128, &err);
> + if (err)
> + return sdr->cidr6 = -1;
> +
> + return 1;
> }
>
>
