Download raw body.
add gotd.conf connection options
On Mon, Jan 02, 2023 at 08:36:39PM +0100, Omar Polo wrote: > 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 ... I felt the ... was a bit unclear, but sure, we can keep things consistent. > > +.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. You're right, I added this for symmetry to request timeout but there is probably no good reason to expose this to users. I've dropped the user-facing parts in the new patch below. We can always put them back in if they ever turn out to be needed (e.g. because someone runs yellow pages and passwd lookups take forever for some reason). > > +.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. And then "limit" is a bit redundant with "max" isn't it? So I have dropped "max" in the new patch below and syntax now reads: connection limit user flan_hacker 16 > > +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). Oh, interesting. I did not know that bsearch(3) existed. There are more cases where it could be used. The above search loop is based on code from lib/pack.c, which could probably also be adjusted. I'd leave this change for later. > > + 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. Sure. This was stolen from httpd's parse.y. > > > + 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. Agreed. > > @@ -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; Sure. New patch with above tweaks applied: diff 3bf00f2542ea6e7825f52c155e5f3f5fecb136e3 4b3a750f5eb97ce1225ce9c421a298725ff1204b commit - 3bf00f2542ea6e7825f52c155e5f3f5fecb136e3 commit + 4b3a750f5eb97ce1225ce9c421a298725ff1204b blob - fd7f8c11aa319b18aa9f13289797a9809af40003 blob + 01a705f61574e2653156dbe6ccb60035616515ac --- gotd/auth.c +++ gotd/auth.c @@ -72,8 +72,8 @@ static int } } -static int -parseuid(const char *s, uid_t *uid) +int +gotd_auth_parseuid(const char *s, uid_t *uid) { struct passwd *pw; const char *errstr; @@ -95,7 +95,7 @@ uidcheck(const char *s, uid_t desired) { uid_t uid; - if (parseuid(s, &uid) != 0) + if (gotd_auth_parseuid(s, &uid) != 0) return -1; if (uid != desired) return -1; blob - fe78e2c9301e0c052e89de790159fb9afaeb5094 blob + 87cd12560d438c7e4b48d2f7a762ff678b51d92e --- gotd/auth.h +++ gotd/auth.h @@ -14,6 +14,6 @@ void * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */ -void -auth_main(const char *title, struct gotd_repolist *repos, +int gotd_auth_parseuid(const char *, uid_t *); +void auth_main(const char *title, struct gotd_repolist *repos, const char *repo_path); blob - 0f2528c97db20b779a500b205d84cfcdb238b7ad blob + 68f77c0970813d13f62fbcfba19833ca3a1efca1 --- gotd/gotd.c +++ gotd/gotd.c @@ -93,7 +93,6 @@ static struct timeval timeout = { 3600, 0 }; static struct gotd_clients gotd_clients[GOTD_CLIENT_TABLE_SIZE]; static SIPHASH_KEY clients_hash_key; volatile int client_cnt; -static struct timeval timeout = { 3600, 0 }; static struct timeval auth_timeout = { 5, 0 }; static struct gotd gotd; @@ -1204,7 +1203,7 @@ gotd_request(int fd, short events, void *arg) if (client->state == GOTD_STATE_EXPECT_LIST_REFS) evtimer_add(&client->tmo, &auth_timeout); else - evtimer_add(&client->tmo, &timeout); + evtimer_add(&client->tmo, &gotd.request_timeout); } } @@ -2586,7 +2585,8 @@ main(int argc, char **argv) if (pledge("stdio sendfd unix", NULL) == -1) err(1, "pledge"); #endif - listen_main(title, fd); + listen_main(title, fd, gotd.connection_limits, + gotd.nconnection_limits); /* NOTREACHED */ break; case PROC_AUTH: blob - 612eac16fcff173392e5868ae2bfa04f9cee4b0f blob + ba00f127e7a704c7c95a42f2210fd109af6de5a3 --- gotd/gotd.conf.5 +++ gotd/gotd.conf.5 @@ -31,6 +31,48 @@ 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 ... +.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 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 limit Ic user Ar identity 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 +203,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 { + limit user flan_hacker 16 + limit user anonymous 32 +} .Ed .Sh SEE ALSO .Xr got 1 , blob - d17cc8a6c43063e47af1fb6b1ce517a12eb909c5 blob + 82485863fa9ea61c5334cb5ecd4b70eac7128cb4 --- gotd/gotd.h +++ gotd/gotd.h @@ -28,6 +28,8 @@ #define GOTD_FD_NEEDED 6 #define GOTD_FILENO_MSG_PIPE 3 +#define GOTD_DEFAULT_REQUEST_TIMEOUT 3600 + /* Client hash tables need some extra room. */ #define GOTD_CLIENT_TABLE_SIZE (GOTD_MAXCLIENTS * 4) @@ -109,6 +111,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 +124,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; + } + + 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 + 580f43381ccf163eac2c738800e7efd25c7562e4 --- 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 LIMIT REQUEST TIMEOUT %token <v.string> STRING %token <v.number> NUMBER %type <v.number> boolean +%type <v.tv> timeout %% @@ -135,6 +139,16 @@ main : UNIX_SOCKET STRING { | NUMBER { $$ = $1; } ; +timeout : NUMBER { + 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 +183,30 @@ 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(gotd->request_timeout)); + } + | LIMIT USER STRING NUMBER { + if (gotd_proc_id == PROC_LISTEN && + conf_limit_user_connections($3, $4) == -1) { + free($3); + YYERROR; + } + free($3); + } + ; + repository : REPOSITORY STRING { struct gotd_repo *repo; @@ -276,13 +312,17 @@ lookup(char *s) { /* This has to be sorted always. */ static const struct keywords keywords[] = { + { "connection", CONNECTION }, { "deny", DENY }, + { "limit", LIMIT }, { "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 +677,9 @@ 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; + file = newfile(filename, 0); if (file == NULL) { /* just return, as we don't require a conf file */ @@ -674,6 +717,64 @@ 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 = pa, *b = pb; + + if (a->uid < b->uid) + return -1; + else if (a->uid > b->uid); + return 1; + + return 0; +} + +static int +conf_limit_user_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) {
add gotd.conf connection options