you need to change len to be sizeof(saddr) instead of sizeof(struct sockaddr)
in unprivproc_pop. likewise, the memcpy in source_addresses needs to use
res->ai_addrlen instead of sizeof(struct sockaddr).
with those fixes its ok by me.
On 20 Dec 2013, at 9:12 pm, Florian Obser <flor...@openbsd.org> wrote:
> On Fri, Dec 20, 2013 at 01:17:08PM +1000, David Gwynne wrote:
>> im glad you wrote a diff rather than simply complain that nat and tftp
>> doesnt work. the moving parts generally look good to me apart from the
>> struct src_addr and getopt chunks.
>>
>> please use sockaddr_storage instead of sockaddr in the src_addr struct.
>>
>> could you split the resolution of the argument to -a out into a separate
>> function and call it after the getopt loop, and pass the value of the family
>> variable to it too?
>>
>> you might want to use sockaddr_storage in unprivproc_pop too.
>>
>> cheers,
>> dlg
>>
>
> done, I also nuked this from the man page, that was a brain-fart:
> +This has to be the IP to which an accompanying nat-to
> +.Xr pf 4
> +rule translates outgoing packets.
>
> Additionally a pass out rule is generated, so it actually does work.
>
> ( I was wondering why the return traffic was passed and I thought it was
> the nat-to rule. Turns out I had a stall pass out rule from a previous
> tftp-proxy run where it created pass rules instead of rdr rules and
> then crashed and thus the anchor was never cleared. Since I never
> restarted the tftp client those rules still matched. )
>
> Thanks,
> Florian
>
> diff --git tftp-proxy.8 tftp-proxy.8
> index f0dc9a4..fb06ba04 100644
> --- tftp-proxy.8
> +++ tftp-proxy.8
> @@ -34,6 +34,7 @@
> .Sh SYNOPSIS
> .Nm tftp-proxy
> .Op Fl 46dv
> +.Op Fl a Ar address
> .Op Fl l Ar address
> .Op Fl p Ar port
> .Op Fl w Ar transwait
> @@ -51,7 +52,7 @@ a rule with divert-reply set.
> .Pp
> The proxy inserts
> .Xr pf 4
> -pass rules using the
> +pass and / or rdr rules using the
> .Ar anchor
> facility to allow payload packets between the client and the server.
> Once the rules are inserted,
> @@ -76,6 +77,10 @@ to use IPv4 addresses only.
> Forces
> .Nm
> to use IPv6 addresses only.
> +.It Fl a Ar address
> +The proxy will use this as the source address for the initial request from
> +the client to the server for nat traversal.
> +Instead of a pass in rule a rdr rule will be generated.
> .It Fl d
> Do not daemonize.
> If this option is specified,
> diff --git tftp-proxy.c tftp-proxy.c
> index bcbecd7..7a85646 100644
> --- tftp-proxy.c
> +++ tftp-proxy.c
> @@ -141,7 +141,7 @@ __dead void
> usage(void)
> {
> extern char *__progname;
> - fprintf(stderr, "usage: %s [-46dv] [-l address] [-p port]"
> + fprintf(stderr, "usage: %s [-46dv] [-a address] [-l address] [-p port]"
> " [-w transwait]\n", __progname);
> exit(1);
> }
> @@ -179,6 +179,15 @@ struct proxy_child {
> struct proxy_child *child = NULL;
> TAILQ_HEAD(, proxy_listener) proxy_listeners;
>
> +struct src_addr {
> + TAILQ_ENTRY(src_addr) entry;
> + struct sockaddr_storage addr;
> + socklen_t addrlen;
> +};
> +TAILQ_HEAD(, src_addr) src_addrs;
> +
> +void source_addresses(const char*, int);
> +
> int
> main(int argc, char *argv[])
> {
> @@ -190,12 +199,15 @@ main(int argc, char *argv[])
> struct passwd *pw;
>
> char *addr = "localhost";
> + char *saddr = NULL;
> char *port = "6969";
> int family = AF_UNSPEC;
>
> int pair[2];
>
> - while ((c = getopt(argc, argv, "46dvl:p:w:")) != -1) {
> + TAILQ_INIT(&src_addrs);
> +
> + while ((c = getopt(argc, argv, "46a:dvl:p:w:")) != -1) {
> switch (c) {
> case '4':
> family = AF_INET;
> @@ -203,6 +215,9 @@ main(int argc, char *argv[])
> case '6':
> family = AF_INET6;
> break;
> + case 'a':
> + saddr = optarg;
> + break;
> case 'd':
> verbose = debug = 1;
> break;
> @@ -239,6 +254,9 @@ main(int argc, char *argv[])
> if (pw == NULL)
> lerrx(1, "no %s user", NOPRIV_USER);
>
> + if (saddr != NULL)
> + source_addresses(saddr, family);
> +
> switch (fork()) {
> case -1:
> lerr(1, "fork");
> @@ -312,6 +330,30 @@ main(int argc, char *argv[])
> return(0);
> }
>
> +void
> +source_addresses(const char* name, int family)
> +{
> + struct addrinfo hints, *res, *res0;
> + struct src_addr *saddr;
> + int error;
> +
> + memset(&hints, 0, sizeof(hints));
> + hints.ai_family = family;
> + hints.ai_socktype = SOCK_DGRAM;
> + hints.ai_flags = AI_PASSIVE;
> + error = getaddrinfo(name, "0", &hints, &res0);
> + if (error)
> + lerrx(1, "%s:%s: %s", name, "0", gai_strerror(error));
> + for (res = res0; res != NULL; res = res->ai_next) {
> + if ((saddr = calloc(1, sizeof(struct src_addr))) == NULL)
> + lerrx(1, "calloc");
> + memcpy(&(saddr->addr), res->ai_addr,
> + sizeof(struct sockaddr));
> + saddr->addrlen = res->ai_addrlen;
> + TAILQ_INSERT_TAIL(&src_addrs, saddr, entry);
> + }
> + freeaddrinfo(res0);
> +}
>
> void
> proxy_privproc(int s, struct passwd *pw)
> @@ -360,7 +402,8 @@ privproc_pop(int fd, short events, void *arg)
> struct addr_pair req;
> struct privproc *p = arg;
> struct fd_reply *rep;
> - int add = 0;
> + struct src_addr *saddr;
> + int add = 0, found = 0;
>
> switch (evbuffer_read(p->buf, fd, sizeof(req))) {
> case 0:
> @@ -407,9 +450,24 @@ privproc_pop(int fd, short events, void *arg)
> &on, sizeof(on)) == -1)
> lerr(1, "privproc setsockopt(REUSEPORT)");
>
> - if (bind(rep->fd, (struct sockaddr *)&req.src,
> - req.src.ss_len) == -1)
> - lerr(1, "privproc bind");
> + if (TAILQ_EMPTY(&src_addrs)) {
> + if (bind(rep->fd, (struct sockaddr *)&req.src,
> + req.src.ss_len) == -1)
> + lerr(1, "privproc bind");
> + } else {
> + TAILQ_FOREACH(saddr, &src_addrs, entry)
> + if (saddr->addr.ss_family ==
> + req.src.ss_family) {
> + if (bind(rep->fd, (struct sockaddr*)
> + &saddr->addr, saddr->addrlen) == -1)
> + lerr(1, "privproc bind");
> + found = 1;
> + break;
> + }
> +
> + if (found == 0)
> + lerr(1, "no source address found");
> + }
>
> if (TAILQ_EMPTY(&p->replies))
> add = 1;
> @@ -727,9 +785,13 @@ unprivproc_pop(int fd, short events, void *arg)
> } cmsgbuf;
> struct cmsghdr *cmsg;
> struct iovec iov;
> + struct sockaddr_storage saddr;
> + socklen_t len;
> int result;
> int s;
>
> + len = sizeof(struct sockaddr);
> +
> do {
> memset(&msg, 0, sizeof(msg));
> iov.iov_base = &result;
> @@ -787,17 +849,34 @@ unprivproc_pop(int fd, short events, void *arg)
> if (prepare_commit(r->id) == -1)
> lerr(1, "%s: prepare_commit", __func__);
>
> - if (add_filter(r->id, PF_IN, (struct sockaddr *)&r->addrs.dst,
> - (struct sockaddr *)&r->addrs.src,
> - ntohs(((struct sockaddr_in *)&r->addrs.src)->sin_port),
> - IPPROTO_UDP) == -1)
> - lerr(1, "%s: couldn't add pass in", __func__);
> -
> - if (add_filter(r->id, PF_OUT, (struct sockaddr *)&r->addrs.dst,
> - (struct sockaddr *)&r->addrs.src,
> - ntohs(((struct sockaddr_in *)&r->addrs.src)->sin_port),
> - IPPROTO_UDP) == -1)
> - lerr(1, "%s: couldn't add pass out", __func__);
> + if (TAILQ_EMPTY(&src_addrs)) {
> + if (add_filter(r->id, PF_IN, (struct sockaddr *)
> + &r->addrs.dst, (struct sockaddr *)&r->addrs.src,
> + ntohs(((struct sockaddr_in *)&r->addrs.src)
> + ->sin_port), IPPROTO_UDP) == -1)
> + lerr(1, "%s: couldn't add pass in", __func__);
> +
> + if (add_filter(r->id, PF_OUT, (struct sockaddr *)
> + &r->addrs.dst, (struct sockaddr *)&r->addrs.src,
> + ntohs(((struct sockaddr_in *)&r->addrs.src)
> + ->sin_port), IPPROTO_UDP) == -1)
> + lerr(1, "%s: couldn't add pass out", __func__);
> + } else {
> + if (getsockname(s, (struct sockaddr*)&saddr, &len) ==
> -1)
> + lerr(1, "%s: getsockname", __func__);
> + if (add_rdr(r->id, (struct sockaddr *)&r->addrs.dst,
> + (struct sockaddr*)&saddr,
> + ntohs(((struct sockaddr_in *)&saddr)->sin_port),
> + (struct sockaddr *)&r->addrs.src,
> + ntohs(((struct sockaddr_in *)&r->addrs.src)->
> + sin_port), IPPROTO_UDP ) == -1)
> + lerr(1, "%s: couldn't add rdr rule", __func__);
> + if (add_filter(r->id, PF_OUT, (struct sockaddr *)
> + &r->addrs.dst, (struct sockaddr *)&r->addrs.src,
> + ntohs(((struct sockaddr_in *)&r->addrs.src)
> + ->sin_port), IPPROTO_UDP) == -1)
> + lerr(1, "%s: couldn't add pass out", __func__);
> + }
>
> if (do_commit() == -1)
> lerr(1, "%s: couldn't commit rules", __func__);
>
>
> --
> I'm not entirely sure you are real.
