Re: bgpd strict community negotiation

2021-05-14 Thread Sebastian Benoit
Claudio Jeker(cje...@diehard.n-r-g.com) on 2021.05.12 19:49:08 +0200:
> RFC5492 is fairly explicit when a capability should be enabled on a
> session:
> 
>A BGP speaker that supports a particular capability may use this
>capability with its peer after the speaker determines (as described
>above) that the peer supports this capability.  Simply put, a given
>capability can be used on a peering if that capability has been
>advertised by both peers.  If either peer has not advertised it, the
>capability cannot be used.
> 
> Adjust capa_neg_calc() to follow this strict model.
> This affects route refersh and graceful restart. For graceful restart this
> requires to flush the RIBs immediatly.
> 
> Also ignore and warn about RREFRESH messages that are received on a
> session where route refesh is disabled.

ok.

This might actually fix some strange behaviour re graceful restart i've seen in 
the
past.

/Benno


> 
> -- 
> :wq Claudio
> 
> Index: session.c
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/session.c,v
> retrieving revision 1.414
> diff -u -p -r1.414 session.c
> --- session.c 6 May 2021 09:18:54 -   1.414
> +++ session.c 12 May 2021 16:33:42 -
> @@ -1636,7 +1636,7 @@ session_neighbor_rrefresh(struct peer *p
>  {
>   u_int8_ti;
>  
> - if (!p->capa.peer.refresh)
> + if (!p->capa.neg.refresh)
>   return (-1);
>  
>   for (i = 0; i < AID_MAX; i++) {
> @@ -2257,6 +2257,11 @@ parse_refresh(struct peer *peer)
>   return (0);
>   }
>  
> + if (!peer->capa.neg.refresh) {
> + log_peer_warnx(>conf, "peer sent unexpected refresh");
> + return (0);
> + }
> +
>   if (imsg_rde(IMSG_REFRESH, peer->conf.id, , sizeof(aid)) == -1)
>   return (-1);
>  
> @@ -2546,16 +2551,15 @@ capa_neg_calc(struct peer *p)
>  {
>   u_int8_ti, hasmp = 0;
>  
> - /* refresh: does not realy matter here, use peer setting */
> - p->capa.neg.refresh = p->capa.peer.refresh;
> + /* a capability is accepted only if both sides announced it */
>  
> - /* as4byte: both side must announce capability */
> - if (p->capa.ann.as4byte && p->capa.peer.as4byte)
> - p->capa.neg.as4byte = 1;
> - else
> - p->capa.neg.as4byte = 0;
> + p->capa.neg.refresh =
> + (p->capa.ann.refresh && p->capa.peer.refresh) != 0;
> +
> + p->capa.neg.as4byte =
> + (p->capa.ann.as4byte && p->capa.peer.as4byte) != 0;
>  
> - /* MP: both side must announce capability */
> + /* MP: both side must agree on the AFI,SAFI pair */
>   for (i = 0; i < AID_MAX; i++) {
>   if (p->capa.ann.mp[i] && p->capa.peer.mp[i])
>   p->capa.neg.mp[i] = 1;
> @@ -2569,18 +2573,21 @@ capa_neg_calc(struct peer *p)
>   p->capa.neg.mp[AID_INET] = 1;
>  
>   /*
> -  * graceful restart: only the peer capabilities are of interest here.
> +  * graceful restart: the peer capabilities are of interest here.
>* It is necessary to compare the new values with the previous ones
>* and act acordingly. AFI/SAFI that are not part in the MP capability
>* are treated as not being present.
> +  * Also make sure that a flush happens if the session stopped
> +  * supporting graceful restart.
>*/
>  
>   for (i = 0; i < AID_MAX; i++) {
>   int8_t  negflags;
>  
>   /* disable GR if the AFI/SAFI is not present */
> - if (p->capa.peer.grestart.flags[i] & CAPA_GR_PRESENT &&
> - p->capa.neg.mp[i] == 0)
> + if (p->capa.ann.grestart.restart == 0 ||
> + (p->capa.peer.grestart.flags[i] & CAPA_GR_PRESENT &&
> + p->capa.neg.mp[i] == 0))
>   p->capa.peer.grestart.flags[i] = 0; /* disable */
>   /* look at current GR state and decide what to do */
>   negflags = p->capa.neg.grestart.flags[i];
> @@ -2600,6 +2607,8 @@ capa_neg_calc(struct peer *p)
>   }
>   p->capa.neg.grestart.timeout = p->capa.peer.grestart.timeout;
>   p->capa.neg.grestart.restart = p->capa.peer.grestart.restart;
> + if (p->capa.ann.grestart.restart == 0)
> + p->capa.neg.grestart.restart = 0;
>  
>   return (0);
>  }
> 



Re: rsync fix file handling in uploader

2021-05-14 Thread Sebastian Benoit
Claudio Jeker(cje...@diehard.n-r-g.com) on 2021.05.07 17:12:16 +0200:
> So yesterday I committed a change to simplify file handling. This removed
> the O_NONBLOCK flag from openat() but today I realized that this was a bit
> premature. The code at that point does not know if the file is actually a
> regular file and so if you put a fifo in place of a regular file it will
> hang rsync.

right :/

> 
> Anyway doing a blind open of any file here is far from ideal. There may be
> involuntary side-effects especially on device nodes. So better change the
> code to do the fstatat() first and only open the file if it is indeed a
> regular file.
> 
> While there change log_link to log_symlink which is more precise and also
> fix a comment that missed some context.
> 
> OK?

yes.

> -- 
> :wq Claudio
> 
> Index: uploader.c
> ===
> RCS file: /cvs/src/usr.bin/rsync/uploader.c,v
> retrieving revision 1.25
> diff -u -p -r1.25 uploader.c
> --- uploader.c6 May 2021 07:35:22 -   1.25
> +++ uploader.c7 May 2021 15:03:03 -
> @@ -79,7 +79,7 @@ log_dir(struct sess *sess, const struct 
>   * operator that we're a link.
>   */
>  static void
> -log_link(struct sess *sess, const struct flist *f)
> +log_symlink(struct sess *sess, const struct flist *f)
>  {
>  
>   if (!sess->opts->server)
> @@ -181,7 +181,7 @@ pre_link(struct upload *p, struct sess *
>   return 0;
>   }
>   if (sess->opts->dry_run) {
> - log_link(sess, f);
> + log_symlink(sess, f);
>   return 0;
>   }
>  
> @@ -259,7 +259,7 @@ pre_link(struct upload *p, struct sess *
>   free(temp);
>   }
>  
> - log_link(sess, f);
> + log_symlink(sess, f);
>   return 0;
>  }
>  
> @@ -645,6 +645,7 @@ pre_file(const struct upload *p, int *fi
>  struct sess *sess)
>  {
>   const struct flist *f;
> + int rc;
>  
>   f = >fl[p->idx];
>   assert(S_ISREG(f->st.mode));
> @@ -661,51 +662,43 @@ pre_file(const struct upload *p, int *fi
>   /*
>* For non dry-run cases, we'll write the acknowledgement later
>* in the rsync_uploader() function.
> -  * If the call to openat() fails with ENOENT, there's a
> -  * fast-path between here and the write function.
>*/
>  
> - *filefd = openat(p->rootfd, f->path,
> - O_RDONLY | O_NOFOLLOW, 0);
> + *filefd = -1;
> + rc = fstatat(p->rootfd, f->path, st, AT_SYMLINK_NOFOLLOW);
>  
> - if (*filefd == -1 && errno != ENOENT) {
> - ERR("%s: openat", f->path);
> + if (rc == -1) {
> + if (errno == ENOENT)
> + return 1;
> +
> + ERR("%s: fstatat", f->path);
>   return -1;
>   }
> - if (*filefd == -1)
> + if (rc != -1 && !S_ISREG(st->st_mode)) {
> + if (S_ISDIR(st->st_mode) &&
> + unlinkat(p->rootfd, f->path, AT_REMOVEDIR) == -1) {
> + ERR("%s: unlinkat", f->path);
> + return -1;
> + }
>   return 1;
> -
> - /*
> -  * Check if the file is already up to date, in which case close it
> -  * and go to the next one.
> -  */
> -
> - if (fstat(*filefd, st) == -1) {
> - ERR("%s: fstat", f->path);
> - close(*filefd);
> - *filefd = -1;
> - return -1;
> - } else if (!S_ISREG(st->st_mode)) {
> - ERRX("%s: not regular", f->path);
> - close(*filefd);
> - *filefd = -1;
> - return -1;
>   }
>  
>   if (st->st_size == f->st.size &&
>   st->st_mtime == f->st.mtime) {
>   LOG3("%s: skipping: up to date", f->path);
> - if (!rsync_set_metadata(sess, 0, *filefd, f, f->path)) {
> + if (!rsync_set_metadata_at(sess, 0, p->rootfd, f, f->path)) {
>   ERRX1("rsync_set_metadata");
> - close(*filefd);
> - *filefd = -1;
>   return -1;
>   }
> - close(*filefd);
> - *filefd = -1;
>   return 0;
>   }
>  
> + *filefd = openat(p->rootfd, f->path, O_RDONLY | O_NOFOLLOW, 0);
> + if (*filefd == -1 && errno != ENOENT) {
> + ERR("%s: openat", f->path);
> + return -1;
> + }
> +
>   /* file needs attention */
>   return 1;
>  }
> @@ -785,8 +778,7 @@ rsync_uploader(struct upload *u, int *fi
>   off_t   offs;
>   int c;
>  
> - /* This should never get called. */
> -
> + /* Once finished this should never get called again. */
>   assert(u->state != UPLOAD_FINISHED);
>  
>   /*
> 



Re: rsync exit code and error cleanup

2021-05-14 Thread Sebastian Benoit
Claudio Jeker(cje...@diehard.n-r-g.com) on 2021.05.07 12:16:26 +0200:
> Currently our rsync does not follow the exit codes from rsync. Also the
> error handling is complex because ERR() and ERRX() are not terminating the
> process.
> 
> This diff tries to start cleaning up the mess a bit. Introduce some exit
> codes to use and apply them in places where it is obvious.
> 
> The rsync server process should normally communicate errors back via the
> rsync socket but there are some errors where this will most probably fail
> as well. A good example are memory failures.
> 
> I used ERR_IPC as a kind of catchall for any system error that pops up
> (fork, socketpair but also pledge, unveil).
> 
> The goal is to reduce the amount of ERR(), ERRX() and ERRX1() from rsync.
> This should simplify the code base.
> 
> I also synced the mkpath() call with the one from mkdir and handle the
> error now outside of the call. Again I think the result is cleaner.
> 
> OK?

ok benno

> -- 
> :wq Claudio
> 
> Index: client.c
> ===
> RCS file: /cvs/src/usr.bin/rsync/client.c,v
> retrieving revision 1.15
> diff -u -p -r1.15 client.c
> --- client.c  8 May 2019 20:00:25 -   1.15
> +++ client.c  7 May 2021 08:29:10 -
> @@ -43,7 +43,7 @@ rsync_client(const struct opts *opts, in
>  
>   if (pledge("stdio unix rpath wpath cpath dpath fattr chown getpw 
> unveil",
>   NULL) == -1)
> - err(1, "pledge");
> + err(ERR_IPC, "pledge");
>  
>   memset(, 0, sizeof(struct sess));
>   sess.opts = opts;
> Index: extern.h
> ===
> RCS file: /cvs/src/usr.bin/rsync/extern.h,v
> retrieving revision 1.36
> diff -u -p -r1.36 extern.h
> --- extern.h  31 Mar 2021 19:45:16 -  1.36
> +++ extern.h  7 May 2021 08:22:32 -
> @@ -42,6 +42,19 @@
>  #define  CSUM_LENGTH_PHASE2 (16)
>  
>  /*
> + * Rsync error codes.
> + */
> +#define ERR_SYNTAX   1
> +#define ERR_PROTOCOL 2
> +#define ERR_SOCK_IO  10
> +#define ERR_FILE_IO  11
> +#define ERR_WIREPROTO12
> +#define ERR_IPC  14  /* catchall for any kind of syscall 
> error */
> +#define ERR_TERMIMATED   16
> +#define ERR_WAITPID  21
> +#define ERR_NOMEM22
> +
> +/*
>   * Use this for --timeout.
>   * All poll events will use it and catch time-outs.
>   */
> Index: fargs.c
> ===
> RCS file: /cvs/src/usr.bin/rsync/fargs.c,v
> retrieving revision 1.17
> diff -u -p -r1.17 fargs.c
> --- fargs.c   8 May 2019 20:00:25 -   1.17
> +++ fargs.c   7 May 2021 08:19:29 -
> @@ -17,6 +17,7 @@
>  #include 
>  
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -51,7 +52,7 @@ fargs_cmdline(struct sess *sess, const s
>   if (sess->opts->ssh_prog) {
>   ap = strdup(sess->opts->ssh_prog);
>   if (ap == NULL)
> - goto out;
> + err(ERR_NOMEM, NULL);
>  
>   while ((arg = strsep(, " \t")) != NULL) {
>   if (arg[0] == '\0') {
> @@ -127,8 +128,4 @@ fargs_cmdline(struct sess *sess, const s
>   addargs(, "%s", f->sink);
>  
>   return args.list;
> -out:
> - freeargs();
> - ERR("calloc");
> - return NULL;
>  }
> Index: main.c
> ===
> RCS file: /cvs/src/usr.bin/rsync/main.c,v
> retrieving revision 1.53
> diff -u -p -r1.53 main.c
> --- main.c31 Mar 2021 19:45:16 -  1.53
> +++ main.c7 May 2021 08:30:18 -
> @@ -83,18 +83,18 @@ fargs_parse(size_t argc, char *argv[], s
>   /* Allocations. */
>  
>   if ((f = calloc(1, sizeof(struct fargs))) == NULL)
> - err(1, "calloc");
> + err(ERR_NOMEM, NULL);
>  
>   f->sourcesz = argc - 1;
>   if ((f->sources = calloc(f->sourcesz, sizeof(char *))) == NULL)
> - err(1, "calloc");
> + err(ERR_NOMEM, NULL);
>  
>   for (i = 0; i < argc - 1; i++)
>   if ((f->sources[i] = strdup(argv[i])) == NULL)
> - err(1, "strdup");
> + err(ERR_NOMEM, NULL);
>  
>   if ((f->sink = strdup(argv[i])) == NULL)
> - err(1, "strdup");
> + err(ERR_NOMEM, NULL);
>  
>   /*
>* Test files for its locality.
> @@ -109,15 +109,16 @@ fargs_parse(size_t argc, char *argv[], s
>   if (fargs_is_remote(f->sink)) {
>   f->mode = FARGS_SENDER;
>   if ((f->host = strdup(f->sink)) == NULL)
> - err(1, "strdup");
> + err(ERR_NOMEM, NULL);
>   }
>  
>   if (fargs_is_remote(f->sources[0])) {
>   if (f->host != NULL)
> - errx(1, "both source and destination cannot be remote 
> files");
> + 

Re: limit concurrent RTR connects in bgpd

2021-05-14 Thread Sebastian Benoit
Claudio Jeker(cje...@diehard.n-r-g.com) on 2021.05.14 11:21:11 +0200:
> I think it is a good idea to limit the number of concurrent connects in
> bgpd. I used 32 as the limit since that is way enough for the number of
> RTR sessions people will configure.
> 
> If the limit is hit the request will be dropped and the rtr process will
> retry the connect after the retry timeout. Hopefully by then the number of
> connections is down again.

ok benno

but i suggest to s/inflight/concurrent/, and keep the word inflight for log
messages about tracking connections for purpose of not running out of file
descriptors?

> -- 
> :wq Claudio
> 
> Index: bgpd.c
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/bgpd.c,v
> retrieving revision 1.236
> diff -u -p -r1.236 bgpd.c
> --- bgpd.c11 May 2021 07:57:24 -  1.236
> +++ bgpd.c11 May 2021 08:00:25 -
> @@ -74,6 +74,7 @@ struct connect_elm {
>  TAILQ_HEAD( ,connect_elm)connect_queue = \
>   TAILQ_HEAD_INITIALIZER(connect_queue);
>  u_intconnect_cnt;
> +#define MAX_CONNECT_CNT  32
>  
>  void
>  sighdlr(int sig)
> @@ -1303,6 +1304,12 @@ bgpd_rtr_connect(struct rtr_config *r)
>   struct connect_elm *ce;
>   struct sockaddr *sa;
>   socklen_t len;
> +
> + if (connect_cnt >= MAX_CONNECT_CNT) {
> + log_warnx("rtr %s: too many inflight connection requests",
> + r->descr);
> + return;
> + }
>  
>   if ((ce = calloc(1, sizeof(*ce))) == NULL) {
>   log_warn("rtr %s", r->descr);
> 



Re: httpd(8): don't try to chunk-encode an empty body

2021-05-14 Thread Sebastian Benoit
Florian Obser(flor...@openbsd.org) on 2021.05.14 19:13:49 +0200:
> As found out by Chris Narkiewicz the hard way, trying to chunk encode an
> empty body makes the nextclown app stop working. (see "Nextcloud stopped
> working after upgrade to 6.9" on ports@).
> 
> I don't think there is a valid way to do this, so don't try to.
> 
> This is kinda maybe a hack since there might be other stuff that isn't
> sending a body. So if anyone has a long list of status codes that should
> be in the same list, let me know.

https://datatracker.ietf.org/doc/html/rfc7230#page-32
gives the list as 

   ... any response with a 1xx
   (Informational), 204 (No Content), or 304 (Not Modified) status
   code is always terminated by the first empty line after the
   header fields, regardless of the header fields present in the
   message, and thus cannot contain a message body.

see the text for other responses that must have an empty body (for other
reasons than the status code).

/Benno

> 
> We can't easily find out if the fcgi server is going to send us an
> empty body without reading everything from the server until we hit the
> body. I'm happy to entertain a diff that does that.
> 
> In the meantime, this scratches my itch.
> OK?

ok benno@

> 
> diff --git server_fcgi.c server_fcgi.c
> index 8d3b581568f..0ac80c27d11 100644
> --- server_fcgi.c
> +++ server_fcgi.c
> @@ -615,6 +615,10 @@ server_fcgi_header(struct client *clt, unsigned int code)
>   if (kv_add(>http_headers, "Server", HTTPD_SERVERNAME) == NULL)
>   return (-1);
>  
> + /* we cannot chunk-encode no-content */
> + if (code == 204)
> + clt->clt_fcgi.chunked = 0;
> +
>   /* Set chunked encoding */
>   if (clt->clt_fcgi.chunked) {
>   /* XXX Should we keep and handle Content-Length instead? */
> 
> 
> -- 
> I'm not entirely sure you are real.
> 



Re: iked(8): support for intermediate CAs and multiple CERT payloads

2021-05-14 Thread Stuart Henderson
On 2021/05/14 21:14, Tobias Heider wrote:
> On Thu, May 13, 2021 at 02:39:37PM +0900, Katsuhiro Ueno wrote:
> > Hi,
> > 
> > I would be happy if iked(8) supports intermediate CAs and sends the
> > entire certificate chain to the clients. The diff attached adds
> > supports for intermediate CAs and multiple CERT payloads to iked(8).
> > 
> > What I would like to do is to use a LetsEncrypt certificate as a
> > server certificate of IKEv2 EAP and establish VPN connections with
> > Windows clients. However, I could not complete it because of the
> > following reasons.
> > * LetsEncrypt server certificate is issued by an intermediate CA
> >   and therefore the certificate of the intermediate CA is needed to
> >   check the validity of the server certificate.
> > * Windows expects the IKEv2 server to send the intermediate CA's
> >   certificate in addition to the server certificate to check the
> >   validity.
> > * On the other hand, iked(8) is not capable of dealing with
> >   certificate chains and sending multiple certificates (multiple
> >   CERT payloads) to the clients.
> > Consequently, Windows fails to verify the certificate and therefore
> > VPN connection cannot be established.
> > 
> > To overcome this, I added an (ad-hoc) support for certificate chain
> > and multiple CERT payloads. The diff attached is the changes that I
> > made. It works fine for me but I am not sure whether or not it works
> > for everyone and everywhere. Tests and comments are greatly
> > appreciated.
> > 
> > Many thanks,
> > Katsuhiro Ueno
> 
> Hi Katsuhiro,
> 
> thank you for the diff!
> As Stuart said this is a very welcome addition and the diff looks good to me.
> 
> Unfortunately I don't have a Windows machine here to test with, so it would be
> nice if anyone reading this could give it a try on their Windows setup.
> 
> I will try running a few more tests with Strongswan clients and commit it
> once I'm sure everything still works.
> 
> - Tobias
> 

Tested with Android strongswan. If I write the intermediate and
root certificates to ca/ca.crt and the server certificate to
certs/$HOSTNAME.crt then I'm able to connect.

I haven't tested Windows yet, I'll try to locate a machine to test with
at the weekend.

The certificate arrangement is a little awkward to work with typical
ACME infrastructure used with standard TLS servers:

For a standard server the root certificate would not normally need to
be present at all; only server and intermediate certs are needed (though
possibly this may be different for IKEv2).  As such, ACME clients don't
normally fetch this certificate so it must be retrieved separately.

Also typically for a TLS server, any intermediates would be stored
alongside the server certificate (fullchain.pem in the usual acme-client
config). Storing that in a separate file isn't difficult ("chain" rather
than "full chain" for acme-client) but, combined with the above need
to store the CA in that same file, it does mean some extra fiddling is
required.

So to use this diff as-is with acme-client, something like this is needed
(I haven't tested the whole thing put together but I think it's right),

domain key "/etc/iked/private/local.key"
domain certificate "/etc/iked/certs/hostname.crt"
domain chain certificate "/etc/iked/ca/chain.crt"



ftp -o /etc/iked/ca/ca.p7c $(openssl x509 -in /etc/iked/ca/chain.crt -text 
-noout | grep 'CA Issuers' | cut -d: -f2-)

(cat /etc/iked/ca/chain.crt; openssl pkcs7 -inform DER -in /etc/iked/ca/ca.p7c 
-print_certs) > /etc/iked/ca/ca.crt

At least it's going to need some doc to show what goes where. But it
would be much simpler to work with if it would at least accept "full
chain" in the single certs/hostname.crt file.



Re: iked(8): support for intermediate CAs and multiple CERT payloads

2021-05-14 Thread Tobias Heider
On Thu, May 13, 2021 at 02:39:37PM +0900, Katsuhiro Ueno wrote:
> Hi,
> 
> I would be happy if iked(8) supports intermediate CAs and sends the
> entire certificate chain to the clients. The diff attached adds
> supports for intermediate CAs and multiple CERT payloads to iked(8).
> 
> What I would like to do is to use a LetsEncrypt certificate as a
> server certificate of IKEv2 EAP and establish VPN connections with
> Windows clients. However, I could not complete it because of the
> following reasons.
> * LetsEncrypt server certificate is issued by an intermediate CA
>   and therefore the certificate of the intermediate CA is needed to
>   check the validity of the server certificate.
> * Windows expects the IKEv2 server to send the intermediate CA's
>   certificate in addition to the server certificate to check the
>   validity.
> * On the other hand, iked(8) is not capable of dealing with
>   certificate chains and sending multiple certificates (multiple
>   CERT payloads) to the clients.
> Consequently, Windows fails to verify the certificate and therefore
> VPN connection cannot be established.
> 
> To overcome this, I added an (ad-hoc) support for certificate chain
> and multiple CERT payloads. The diff attached is the changes that I
> made. It works fine for me but I am not sure whether or not it works
> for everyone and everywhere. Tests and comments are greatly
> appreciated.
> 
> Many thanks,
> Katsuhiro Ueno

Hi Katsuhiro,

thank you for the diff!
As Stuart said this is a very welcome addition and the diff looks good to me.

Unfortunately I don't have a Windows machine here to test with, so it would be
nice if anyone reading this could give it a try on their Windows setup.

I will try running a few more tests with Strongswan clients and commit it
once I'm sure everything still works.

- Tobias



httpd(8): don't try to chunk-encode an empty body

2021-05-14 Thread Florian Obser
As found out by Chris Narkiewicz the hard way, trying to chunk encode an
empty body makes the nextclown app stop working. (see "Nextcloud stopped
working after upgrade to 6.9" on ports@).

I don't think there is a valid way to do this, so don't try to.

This is kinda maybe a hack since there might be other stuff that isn't
sending a body. So if anyone has a long list of status codes that should
be in the same list, let me know.

We can't easily find out if the fcgi server is going to send us an
empty body without reading everything from the server until we hit the
body. I'm happy to entertain a diff that does that.

In the meantime, this scratches my itch.
OK?

diff --git server_fcgi.c server_fcgi.c
index 8d3b581568f..0ac80c27d11 100644
--- server_fcgi.c
+++ server_fcgi.c
@@ -615,6 +615,10 @@ server_fcgi_header(struct client *clt, unsigned int code)
if (kv_add(>http_headers, "Server", HTTPD_SERVERNAME) == NULL)
return (-1);
 
+   /* we cannot chunk-encode no-content */
+   if (code == 204)
+   clt->clt_fcgi.chunked = 0;
+
/* Set chunked encoding */
if (clt->clt_fcgi.chunked) {
/* XXX Should we keep and handle Content-Length instead? */


-- 
I'm not entirely sure you are real.



smtp(1): protocols and ciphers

2021-05-14 Thread Eric Faurot
Hello.

This diff allows to specify protcols and ciphers in smtp(1).
I thought it was cleaner to added a generic -O option flag for this.

Eric.

Index: smtp.1
===
RCS file: /cvs/src/usr.sbin/smtpd/smtp.1,v
retrieving revision 1.9
diff -u -p -r1.9 smtp.1
--- smtp.1  13 Feb 2021 08:07:48 -  1.9
+++ smtp.1  14 May 2021 13:25:59 -
@@ -26,6 +26,7 @@
 .Op Fl a Ar authfile
 .Op Fl F Ar from
 .Op Fl H Ar helo
+.Op Fl O Ar option
 .Op Fl s Ar server
 .Op Ar recipient ...
 .Sh DESCRIPTION
@@ -63,6 +64,31 @@ Display usage.
 Do not actually execute a transaction,
 just try to establish an SMTP session and quit.
 When this option is given, no message is read from the standard input.
+.It Fl O Ar option
+Set additional configuration options that don't have a specific flag.
+The
+.Ar option
+string has the form
+.Sm off
+.Ar name No = Ar value
+.Sm on
+where
+.Ar name
+is one of the following:
+.Bl -tag -width "protocols"
+.It protocols
+Specify the protocols to use for tls connections.
+Refer to
+.Xr tls_config_parse_protocols 3
+for
+.Ar value .
+.It ciphers
+Specify the allowed ciphers for tls connections.
+Refer to
+.Xr tls_config_set_ciphers 3
+for
+.Ar value .
+.El
 .It Fl s Ar server
 Specify the server to connect to and connection parameters.
 The format is
Index: smtpc.c
===
RCS file: /cvs/src/usr.sbin/smtpd/smtpc.c,v
retrieving revision 1.15
diff -u -p -r1.15 smtpc.c
--- smtpc.c 10 Apr 2021 10:19:19 -  1.15
+++ smtpc.c 14 May 2021 13:31:45 -
@@ -48,22 +48,45 @@ static struct smtp_mail mail;
 static const char *servname = NULL;
 static struct tls_config *tls_config;
 
+static const char *protocols = NULL;
+static const char *ciphers = NULL;
+
 static void
 usage(void)
 {
extern char *__progname;
 
fprintf(stderr, "usage: %s [-Chnv] [-a authfile] [-F from] [-H helo] "
+   "[-O option] "
"[-s server] [recipient ...]\n", __progname);
exit(1);
 }
 
+static void
+parse_option(char *opt)
+{
+   char *v;
+
+   v = strchr(opt, '=');
+   if (v == NULL)
+   fatalx("invalid option string \"%s\"", opt);
+   *v++ = '\0';
+
+   if (!strcmp(opt, "ciphers"))
+   ciphers = v;
+   else if (!strcmp(opt, "protocols"))
+   protocols = v;
+   else
+   fatalx("unknown option \"%s\"", opt);
+}
+
 int
 main(int argc, char **argv)
 {
char hostname[256];
FILE *authfile;
int ch, i;
+   uint32_t protos;
char *server = "localhost";
char *authstr = NULL;
size_t alloc = 0;
@@ -91,7 +114,7 @@ main(int argc, char **argv)
memset(, 0, sizeof(mail));
mail.from = pw->pw_name;
 
-   while ((ch = getopt(argc, argv, "CF:H:S:a:hns:v")) != -1) {
+   while ((ch = getopt(argc, argv, "CF:H:O:S:a:hns:v")) != -1) {
switch (ch) {
case 'C':
params.tls_verify = 0;
@@ -102,6 +125,9 @@ main(int argc, char **argv)
case 'H':
params.helo = optarg;
break;
+   case 'O':
+   parse_option(optarg);
+   break;
case 'S':
servname = optarg;
break;
@@ -159,6 +185,17 @@ main(int argc, char **argv)
tls_config = tls_config_new();
if (tls_config == NULL)
fatal("tls_config_new");
+
+   if (protocols) {
+   if (tls_config_parse_protocols(, protocols) == -1)
+   fatalx("failed to parse protocol '%s'", protocols);
+   if (tls_config_set_protocols(tls_config, protos) == -1)
+   fatalx("tls_config_set_protocols: %s",
+   tls_config_error(tls_config));
+   }
+   if (ciphers && tls_config_set_ciphers(tls_config, ciphers) == -1)
+   fatalx("tls_config_set_ciphers: %s",
+   tls_config_error(tls_config));
if (tls_config_set_ca_file(tls_config, tls_default_ca_cert_file()) == 
-1)
fatal("tls_set_ca_file");
if (!params.tls_verify) {



Re: mpe.4: properly refer to ioctl(2)

2021-05-14 Thread Klemens Nanni
My bad;  fixed, thanks.

On Fri, May 14, 2021 at 12:07:14PM +0200, Caspar Schutijser wrote:
> Index: mpe.4
> ===
> RCS file: /cvs/src/share/man/man4/mpe.4,v
> retrieving revision 1.11
> diff -u -p -r1.11 mpe.4
> --- mpe.4 18 Mar 2021 14:22:04 -  1.11
> +++ mpe.4 14 May 2021 10:01:58 -
> @@ -41,7 +41,7 @@ The interface itself can be configured w
>  see its manual page for more information.
>  .Sh IOCTLS
>  The following
> -.Nm ioctl 2
> +.Xr ioctl 2
>  calls are specific to
>  .Nm ,
>  .Xr mpip 4
> 



Re: running network stack forwarding in parallel

2021-05-14 Thread Martin Pieuchot
On 13/05/21(Thu) 14:50, Vitaliy Makkoveev wrote:
> On Thu, May 13, 2021 at 01:15:05PM +0200, Hrvoje Popovski wrote:
> > On 13.5.2021. 1:25, Vitaliy Makkoveev wrote:
> > > It seems this lock order issue is not parallel diff specific.
> > 
> > 
> > 
> > Yes,  you are right ... it seemed familiar but i couldn't reproduce it
> > on lapc trunk or without this diff so i thought that parallel diff is
> > one to blame ..
> > 
> > 
> > sorry for noise ..
> >
> 
> Timeout thread and interface destroy thread are both serialized by
> kernel lock so it's hard to catch this issue. So your report is
> useful :)

The use of the NET_LOCK() in *clone_destroy() is problematic.  tpmr(4)
has a similar problem as reported by Hrvoje in a different thread.  I
don't know what it is serializing, hopefully David can tell us more.



mpe.4: properly refer to ioctl(2)

2021-05-14 Thread Caspar Schutijser
Index: mpe.4
===
RCS file: /cvs/src/share/man/man4/mpe.4,v
retrieving revision 1.11
diff -u -p -r1.11 mpe.4
--- mpe.4   18 Mar 2021 14:22:04 -  1.11
+++ mpe.4   14 May 2021 10:01:58 -
@@ -41,7 +41,7 @@ The interface itself can be configured w
 see its manual page for more information.
 .Sh IOCTLS
 The following
-.Nm ioctl 2
+.Xr ioctl 2
 calls are specific to
 .Nm ,
 .Xr mpip 4



limit concurrent RTR connects in bgpd

2021-05-14 Thread Claudio Jeker
I think it is a good idea to limit the number of concurrent connects in
bgpd. I used 32 as the limit since that is way enough for the number of
RTR sessions people will configure.

If the limit is hit the request will be dropped and the rtr process will
retry the connect after the retry timeout. Hopefully by then the number of
connections is down again.
-- 
:wq Claudio

Index: bgpd.c
===
RCS file: /cvs/src/usr.sbin/bgpd/bgpd.c,v
retrieving revision 1.236
diff -u -p -r1.236 bgpd.c
--- bgpd.c  11 May 2021 07:57:24 -  1.236
+++ bgpd.c  11 May 2021 08:00:25 -
@@ -74,6 +74,7 @@ struct connect_elm {
 TAILQ_HEAD( ,connect_elm)  connect_queue = \
TAILQ_HEAD_INITIALIZER(connect_queue);
 u_int  connect_cnt;
+#define MAX_CONNECT_CNT32
 
 void
 sighdlr(int sig)
@@ -1303,6 +1304,12 @@ bgpd_rtr_connect(struct rtr_config *r)
struct connect_elm *ce;
struct sockaddr *sa;
socklen_t len;
+
+   if (connect_cnt >= MAX_CONNECT_CNT) {
+   log_warnx("rtr %s: too many inflight connection requests",
+   r->descr);
+   return;
+   }
 
if ((ce = calloc(1, sizeof(*ce))) == NULL) {
log_warn("rtr %s", r->descr);



Re: Fix mbuf leaks in re_rxeof()

2021-05-14 Thread Claudio Jeker
On Thu, May 13, 2021 at 02:40:31PM +, Visa Hankala wrote:
> It looks that re_rxeof() might leak mbufs in two cases. The first case
> happens if the controller returns an incomplete frame when frames are
> expected to be non-fragmented. Note that in this instance the fragment
> list sc->rl_head should be empty and does not need clearing.
> 
> The second leak happens if a frame has a reception error. The code has
> cleared any preceding fragments but leaks the list's final mbuf.
> 
> Index: dev/ic/re.c
> ===
> RCS file: src/sys/dev/ic/re.c,v
> retrieving revision 1.210
> diff -u -p -r1.210 re.c
> --- dev/ic/re.c   7 May 2021 09:13:19 -   1.210
> +++ dev/ic/re.c   13 May 2021 14:33:20 -
> @@ -1280,6 +1280,8 @@ re_rxeof(struct rl_softc *sc)
>   if ((sc->rl_flags & RL_FLAG_JUMBOV2) != 0 &&
>   (rxstat & (RL_RDESC_STAT_SOF | RL_RDESC_STAT_EOF)) !=
>   (RL_RDESC_STAT_SOF | RL_RDESC_STAT_EOF)) {
> + ifp->if_ierrors++;
> + m_freem(m);
>   continue;
>   } else if (!(rxstat & RL_RDESC_STAT_EOF)) {
>   m->m_len = RL_FRAMELEN(sc->rl_max_mtu);
> @@ -1328,6 +1330,7 @@ re_rxeof(struct rl_softc *sc)
>   m_freem(sc->rl_head);
>   sc->rl_head = sc->rl_tail = NULL;
>   }
> + m_freem(m);
>   continue;
>   }
>  
> 

Looks correct to me. OK claudio@

-- 
:wq Claudio



Re: ftpd(8): add pledge(2)

2021-05-14 Thread Sebastien Marie
On Fri, May 14, 2021 at 07:29:48AM +0200, Matthias Pressfreund wrote:
> Interesting. How do I figure the correct order of keywords? So far I thought 
> it
> didn't matter.

for the kernel, the order doesn't matter.

for people reviewing code, it matters.
 
> On 2021-05-13 18:40, Theo de Raadt wrote:
> > +   if (pledge("stdio rpath inet recvfd sendfd "
> > +   "wpath cpath proc tty getpw", NULL) == 
> > -1)
> > 
> > Please change the order:
> > 
> > stdio rpath wpath cpath inet recvfd sendfd proc tty getpw
> > 
> > (It remains extremely permissive).
> > 
> 

-- 
Sebastien Marie