From: Omar Polo <op@omarpolo.com>
Subject: Re: add gotd.conf connection options
To: Stefan Sperling <stsp@stsp.name>
Cc: gameoftrees@openbsd.org
Date: Mon, 02 Jan 2023 20:36:39 +0100

On 2023/01/02 16:52:15 +0100, Stefan Sperling <stsp@stsp.name> wrote:
> --- gotd/gotd.conf.5
> +++ gotd/gotd.conf.5
> @@ -31,6 +31,56 @@ The available global configuration directives are as f
>  .Sh GLOBAL CONFIGURATION
>  The available global configuration directives are as follows:
>  .Bl -tag -width Ds
> +.It Ic connection Ar option
> +Set the specified options and limits for connections to the
> +.Xr gotd 8
> +unix socket.
> +.Pp
> +The
> +.Ic connection
> +directive may be specified multiple times, and multiple
> +.Ar option
> +arguments may be specified within curly braces:
> +.Pp
> +.Ic connection Brq Ar option1 Ar option2 ...

i'd drop "option1 option2".  Other manpages (e.g. httpd.conf(5))
usually just get away with

	.Ic connection Brq ...

> +.Pp
> +Each option should only be specified once.
> +If a given option is listed multiple times, the last line which sets this
> +option wins.
> +.Pp
> +Valid connection options are:
> +.Bl -tag -width Ds
> +.It Ic authentication timeout Ar seconds
> +Specify the timeout for client authentication.
> +If this timeout is exceeded due to an unexpected failure in the authentication
> +process, such as a crash, the connection will be terminated.
> +.Pp
> +The default timeout is 5 seconds.
> +This should only be changed if legitimate connections are exceeding the
> +default timeout for some reason.

To be fair I'm not convinced about this knob.

The auth process is just a getpeereuid() + a loop over the ACLs, it
shouldn't never hit the limit and if it does it's a bug.  users should
report it rather than trying to crank up the limit.

I'm not against the internal timeout for the auth process, I just
don't think it's worth exposing it.

> +.It Ic request timeout Ar seconds
> +Specify the inactivity timeout for operations between client and server.
> +If this timeout is exceeded while a Git protocol request is being processed,
> +the request will be aborted and the connection will be terminated.
> +.Pp
> +The default timeout is 3600 seconds (1 hour).
> +This should only be changed if legitimate requests are exceeding the default
> +timeout for some reason, such as the server spending an extraordinary
> +amount of time generating a pack file.
> +.It Ic user Ar identity Ic max Ar number
> +Limit the maximum amount of concurrent connections by the user with
> +the username
> +.Ar identity
> +to
> +.Ar number .
> +Numeric user IDs are also accepted.
> +.Pp
> +The default per-user limit is 4.
> +This should only be changed if concurrent connections from a given user are
> +expected to exceed the default limit, for example if an anonymous user
> +is granted read access and many concurrent connections will share this
> +anonymous user identity.
> +.El
>  .It Ic unix_socket Ar path
>  Set the path to the unix socket which
>  .Xr gotd 8
> @@ -161,6 +211,15 @@ repository "openbsd/ports" {
>  	permit ro anonymous
>  	deny flan_hacker
>  }
> +
> +# Use a larger request timeout value:
> +connection request timeout 7200  # 2 hours
> +
> +# Some users are granted a higher concurrent connection limit:
> +connection {
> +	user flan_hacker max 16

just wondering, what about adding a `limit' keyword there too, e.g.:

	connection limit user flan_hacker max 16
	connection {
		limit user foo max 32
		limit user bar max 64
	}

It doesn't add anything, but it'd almost read as plain english then.

> +	user anonymous max 32
> +}
>  .Ed
>  .Sh SEE ALSO
>  .Xr got 1 ,
> blob - d17cc8a6c43063e47af1fb6b1ce517a12eb909c5
> blob + 5a527cb9b1db88f3a364c3ff9f274fa12f1d81e7
> --- gotd/gotd.h
> +++ gotd/gotd.h
> @@ -28,6 +28,9 @@
>  #define GOTD_FD_NEEDED		6
>  #define GOTD_FILENO_MSG_PIPE	3
>  
> +#define GOTD_DEFAULT_REQUEST_TIMEOUT	3600
> +#define GOTD_DEFAULT_AUTH_TIMEOUT	5
> +
>  /* Client hash tables need some extra room. */
>  #define GOTD_CLIENT_TABLE_SIZE (GOTD_MAXCLIENTS * 4)
>  
> @@ -109,6 +112,11 @@ struct gotd {
>  	size_t				 nids;
>  };
>  
> +struct gotd_uid_connection_limit {
> +	uid_t uid;
> +	int max_connections;
> +};
> +
>  struct gotd {
>  	pid_t pid;
>  	char unix_socket_path[PATH_MAX];
> @@ -117,6 +125,10 @@ struct gotd {
>  	struct gotd_repolist repos;
>  	int nrepos;
>  	struct gotd_child_proc listen_proc;
> +	struct timeval request_timeout;
> +	struct timeval auth_timeout;
> +	struct gotd_uid_connection_limit *connection_limits;
> +	size_t nconnection_limits;
>  
>  	char *argv0;
>  	const char *confpath;
> blob - bcc891f386e858648339da2b211be2df91530d4b
> blob + 0244dadfadb3fbe5d3d608eaee023f5df9744ce8
> --- gotd/listen.c
> +++ gotd/listen.c
> @@ -70,6 +70,8 @@ static struct {
>  	int fd;
>  	struct gotd_imsgev iev;
>  	struct gotd_imsgev pause;
> +	struct gotd_uid_connection_limit *connection_limits;
> +	size_t nconnection_limits;
>  } gotd_listen;
>  
>  static int inflight;
> @@ -178,6 +180,26 @@ static const struct got_error *
>  	return NULL;
>  }
>  
> +struct gotd_uid_connection_limit *
> +gotd_find_uid_connection_limit(struct gotd_uid_connection_limit *limits,
> +    size_t nlimits, uid_t uid)
> +{
> +	/* This array is always sorted to allow for binary search. */
> +	int i, left = 0, right = nlimits - 1;
> +
> +	while (left <= right) {
> +		i = ((left + right) / 2);
> +		if (limits[i].uid == uid)
> +			return &limits[i];
> +		if (limits[i].uid > uid)
> +			left = i + 1;
> +		else
> +			right = i - 1;
> +	}

not really an issue, but since we already have
uid_connection_limit_cmp I wonder if we could reuse it here with
bsearch(3).

> +	return NULL;
> +}
> +
>  static const struct got_error *
>  disconnect(struct gotd_listen_client *client)
>  {
> @@ -303,7 +325,16 @@ gotd_accept(int fd, short event, void *arg)
>  		counter->nconnections = 1;
>  		add_uid_connection_counter(counter);
>  	} else {
> -		if (counter->nconnections >= GOTD_MAX_CONN_PER_UID) {
> +		int max_connections = GOTD_MAX_CONN_PER_UID;
> +		struct gotd_uid_connection_limit *limit;
> +
> +		limit = gotd_find_uid_connection_limit(
> +		    gotd_listen.connection_limits,
> +		    gotd_listen.nconnection_limits, euid);
> +		if (limit)
> +			max_connections = limit->max_connections;
> +
> +		if (counter->nconnections >= max_connections) {
>  			log_warnx("maximum connections exceeded for uid %d",
>  			    euid);
>  			goto err;
> @@ -426,7 +457,9 @@ listen_main(const char *title, int gotd_socket)
>  }
>  
>  void
> -listen_main(const char *title, int gotd_socket)
> +listen_main(const char *title, int gotd_socket,
> +    struct gotd_uid_connection_limit *connection_limits,
> +    size_t nconnection_limits)
>  {
>  	struct gotd_imsgev iev;
>  	struct event evsigint, evsigterm, evsighup, evsigusr1;
> @@ -437,6 +470,8 @@ listen_main(const char *title, int gotd_socket)
>  	gotd_listen.title = title;
>  	gotd_listen.pid = getpid();
>  	gotd_listen.fd = gotd_socket;
> +	gotd_listen.connection_limits = connection_limits;
> +	gotd_listen.nconnection_limits = nconnection_limits;
>  
>  	signal_set(&evsigint, SIGINT, listen_sighdlr, NULL);
>  	signal_set(&evsigterm, SIGTERM, listen_sighdlr, NULL);
> @@ -473,6 +508,7 @@ listen_shutdown(void)
>  {
>  	log_debug("%s: shutting down", gotd_listen.title);
>  
> +	free(gotd_listen.connection_limits);
>  	if (gotd_listen.fd != -1)
>  		close(gotd_listen.fd);
>  
> blob - 5ecdb0084e77470530899068dc16cb0f553ead7b
> blob + e7a67532164502eb5ea8893b35007e6edf63ee36
> --- gotd/listen.h
> +++ gotd/listen.h
> @@ -14,4 +14,9 @@ void listen_main(const char *title, int gotd_socket);
>   * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
>   */
>  
> -void listen_main(const char *title, int gotd_socket);
> +struct gotd_uid_connection_limit *gotd_find_uid_connection_limit(
> +    struct gotd_uid_connection_limit *limits, size_t nlimits, uid_t uid);
> +
> +void listen_main(const char *title, int gotd_socket,
> +    struct gotd_uid_connection_limit *connection_limits,
> +    size_t nconnection_limits);
> blob - eb5822c199bae7d3a0ab0b75eefe7854b6409e3c
> blob + 9acd293aaafa5ef3d24f7c8bf9a7caf2a5ec0cc5
> --- gotd/parse.y
> +++ gotd/parse.y
> @@ -46,6 +46,8 @@
>  
>  #include "log.h"
>  #include "gotd.h"
> +#include "auth.h"
> +#include "listen.h"
>  
>  TAILQ_HEAD(files, file)		 files = TAILQ_HEAD_INITIALIZER(files);
>  static struct file {
> @@ -94,6 +96,7 @@ typedef struct {
>  	union {
>  		long long	 number;
>  		char		*string;
> +		struct timeval	 tv;
>  	} v;
>  	int lineno;
>  } YYSTYPE;
> @@ -101,11 +104,12 @@ typedef struct {
>  %}
>  
>  %token	PATH ERROR ON UNIX_SOCKET UNIX_GROUP USER REPOSITORY PERMIT DENY
> -%token	RO RW
> +%token	RO RW CONNECTION MAXIMUM AUTHENTICATION REQUEST TIMEOUT
>  
>  %token	<v.string>	STRING
>  %token	<v.number>	NUMBER
>  %type	<v.number>	boolean
> +%type	<v.tv>		timeout
>  
>  %%
>  
> @@ -135,6 +139,17 @@ main		: UNIX_SOCKET STRING {
>  		| NUMBER { $$ = $1; }
>  		;
>  
> +timeout		: NUMBER
> +		{

style nit: I'd keep { in the same line as all the other rules do.

> +			if ($1 < 0) {
> +				yyerror("invalid timeout: %lld", $1);
> +				YYERROR;
> +			}
> +			$$.tv_sec = $1;
> +			$$.tv_usec = 0;
> +		}
> +		;
> +
>  main		: UNIX_SOCKET STRING {
>  			if (gotd_proc_id == PROC_LISTEN) {
>  				if (strlcpy(gotd->unix_socket_path, $2,
> @@ -169,8 +184,34 @@ main		: UNIX_SOCKET STRING {
>  			}
>  			free($2);
>  		}
> +		| connection
>  		;
>  
> +connection	: CONNECTION '{' optnl conflags_l '}'
> +		| CONNECTION conflags
> +
> +conflags_l	: conflags optnl conflags_l
> +		| conflags optnl
> +		;
> +
> +conflags	: REQUEST TIMEOUT timeout		{
> +			memcpy(&gotd->request_timeout, &$3,
> +			    sizeof(struct timeval));

nit: I'd use sizeof(gotd->request_timeout).  I don't think we'll ever
change the type, but if we do it'd be less churn.

> +		}
> +		| AUTHENTICATION TIMEOUT timeout	{
> +			memcpy(&gotd->auth_timeout, &$3,
> +			    sizeof(struct timeval));

ditto.

> +		}
> +		| USER STRING MAXIMUM NUMBER	{
> +			if (gotd_proc_id == PROC_LISTEN &&
> +			    conf_new_user_max_connections($2, $4) == -1) {
> +				free($2);
> +				YYERROR;
> +			}
> +			free($2);
> +		}
> +		;
> +
>  repository	: REPOSITORY STRING {
>  			struct gotd_repo *repo;
>  
> @@ -276,13 +317,18 @@ lookup(char *s)
>  {
>  	/* This has to be sorted always. */
>  	static const struct keywords keywords[] = {
> +		{ "authentication",		AUTHENTICATION },
> +		{ "connection",			CONNECTION },
>  		{ "deny",			DENY },
> +		{ "max",			MAXIMUM },
>  		{ "on",				ON },
>  		{ "path",			PATH },
>  		{ "permit",			PERMIT },
>  		{ "repository",			REPOSITORY },
> +		{ "request",			REQUEST },
>  		{ "ro",				RO },
>  		{ "rw",				RW },
> +		{ "timeout",			TIMEOUT },
>  		{ "unix_group",			UNIX_GROUP },
>  		{ "unix_socket",		UNIX_SOCKET },
>  		{ "user",			USER },
> @@ -637,6 +683,11 @@ parse_config(const char *filename, enum gotd_procid pr
>  		return -1;
>  	}
>  
> +	gotd->request_timeout.tv_sec = GOTD_DEFAULT_REQUEST_TIMEOUT;
> +	gotd->request_timeout.tv_usec = 0;
> +	gotd->auth_timeout.tv_sec = GOTD_DEFAULT_AUTH_TIMEOUT;
> +	gotd->auth_timeout.tv_usec = 0;
> +
>  	file = newfile(filename, 0);
>  	if (file == NULL) {
>  		/* just return, as we don't require a conf file */
> @@ -674,6 +725,67 @@ static struct gotd_repo *
>  	return (0);
>  }
>  
> +static int
> +uid_connection_limit_cmp(const void *pa, const void *pb)
> +{
> +	const struct gotd_uid_connection_limit *a, *b;
> +
> +	a = (const struct gotd_uid_connection_limit *)pa;
> +	b = (const struct gotd_uid_connection_limit *)pb;

not an issue, but the cast is not needed.  could just

+	const struct gotd_uid_connection_limit *a = pa, *b = pb;

> +	if (a->uid < b->uid)
> +		return -1;
> +	else if (a->uid > b->uid);
> +		return 1;
> +
> +	return 0;
> +}
> +
> +static int
> +conf_new_user_max_connections(const char *user, int maximum)
> +{
> +	uid_t uid;
> +	struct gotd_uid_connection_limit *limit;
> +	size_t nlimits;
> +
> +	if (maximum < 1) {
> +		yyerror("max connections cannot be smaller 1");
> +		return -1;
> +	}
> +	if (maximum > GOTD_MAXCLIENTS) {
> +		yyerror("max connections must be <= %d", GOTD_MAXCLIENTS);
> +		return -1;
> +	}
> +
> +	if (gotd_auth_parseuid(user, &uid) == -1) {
> +		yyerror("%s: no such user", user);
> +		return -1;
> +	}
> +
> +	limit = gotd_find_uid_connection_limit(gotd->connection_limits,
> +	    gotd->nconnection_limits, uid);
> +	if (limit) {
> +		limit->max_connections = maximum;
> +		return 0;
> +	}
> +
> +	limit = gotd->connection_limits;
> +	nlimits = gotd->nconnection_limits + 1;
> +	limit = reallocarray(limit, nlimits, sizeof(*limit));
> +	if (limit == NULL)
> +		fatal("reallocarray");
> +
> +	limit[nlimits - 1].uid = uid;
> +	limit[nlimits - 1].max_connections = maximum;
> +
> +	gotd->connection_limits = limit;
> +	gotd->nconnection_limits = nlimits;
> +	qsort(gotd->connection_limits, gotd->nconnection_limits,
> +	    sizeof(gotd->connection_limits[0]), uid_connection_limit_cmp);
> +
> +	return 0;
> +}
> +
>  static struct gotd_repo *
>  conf_new_repo(const char *name)
>  {