On 2022/12/27 16:02:12 +0100, Stefan Sperling <stsp@stsp.name> wrote:
> Run authentication in a separate child process, removing the "getpw"
> pledge promise from the parent.
>  
> This patch applies on top of my previous patch to fork repo children
> on demand.

nice to see the changes to wait_for_children.  OK op@, some comments below

> diff f015a20bf2366e80c0d25eb8448eed577c148784 a4b6708ab0e570f77974656765a7767f03cdd125
> commit - f015a20bf2366e80c0d25eb8448eed577c148784
> commit + a4b6708ab0e570f77974656765a7767f03cdd125
> blob - ed5bbd87051a58c687b26b3585c2a1e3714d65f3
> blob + 57b17a1dd26385f232f8f989e4fa041babfeafe3
> --- gotd/auth.c
> +++ gotd/auth.c
> @@ -25,18 +25,52 @@
>  #include <pwd.h>
>  #include <grp.h>
>  #include <sha1.h>
> +#include <signal.h>
>  #include <stdint.h>
>  #include <stdio.h>
>  #include <stdlib.h>
> +#include <string.h>
>  #include <imsg.h>
>  #include <unistd.h>
>  
>  #include "got_error.h"
> +#include "got_path.h"
>  
>  #include "gotd.h"
>  #include "log.h"
>  #include "auth.h"
>  
> +static struct gotd_auth {
> +	pid_t pid;
> +	const char *title;
> +	struct gotd_repo *repo;
> +} gotd_auth;
> +
> +static void auth_shutdown(void);
> +
> +static void
> +auth_sighdlr(int sig, short event, void *arg)
> +{
> +	/*
> +	 * Normal signal handler rules don't apply because libevent
> +	 * decouples for us.
> +	 */
> +
> +	switch (sig) {
> +	case SIGHUP:
> +		break;
> +	case SIGUSR1:
> +		break;

just for curiosity, what's the point of catching SIGUSR1 too?  the
listener process and gotwebd do that too (but gotwebd due to proc.c I
guess), but otherwise they don't do anything with it.

> [...]
> blob - 3040877532eed07c6a4bd0d310bf4b9564ceab06
> blob + 047f9d4ecb86187d413c4cf02096cccd7398217f
> --- gotd/gotd.c
> +++ gotd/gotd.c
> [...]
> @@ -1906,6 +1933,120 @@ gotd_dispatch_repo_child(int fd, short event, void *ar
>  }
>  
>  static void
> +gotd_dispatch_auth_child(int fd, short event, void *arg)
> +{
> +	const struct got_error *err = NULL;
> +	struct gotd_imsgev *iev = arg;
> +	struct imsgbuf *ibuf = &iev->ibuf;
> +	struct gotd_client *client;
> +	struct gotd_repo *repo = NULL;
> +	ssize_t n;
> +	int shut = 0;
> +	struct imsg imsg;
> +	uint32_t client_id = 0;
> +	int do_disconnect = 0;
> +	enum gotd_procid proc_type;
> +
> +	client = find_client_by_proc_fd(fd);
> +	if (client == NULL)
> +		fatalx("cannot find client for fd %d", fd);
> +
> +	if (client->auth == NULL)
> +		fatalx("cannot find auth child process for fd %d", fd);
> +
> +	if (event & EV_READ) {
> +		if ((n = imsg_read(ibuf)) == -1 && errno != EAGAIN)
> +			fatal("imsg_read error");
> +		if (n == 0) {
> +			/* Connection closed. */
> +			shut = 1;
> +			goto done;
> +		}
> +	}
> +
> +	if (event & EV_WRITE) {
> +		n = msgbuf_write(&ibuf->w);
> +		if (n == -1 && errno != EAGAIN)
> +			fatal("msgbuf_write");
> +		if (n == 0) {
> +			/* Connection closed. */
> +			shut = 1;
> +		}
> +		goto done;
> +	}
> +
> +	if (client->auth->iev.ibuf.fd != fd)
> +		fatalx("%s: unexpected fd %d", __func__, fd);

it's a bit redundant to check the fd, since find_client_by_proc_fd
should return a client with that fd.  what i'm missing?

> +	if ((n = imsg_get(ibuf, &imsg)) == -1)
> +		fatal("%s: imsg_get error", __func__);
> +	if (n == 0)	/* No more messages. */
> +		return;

any reason not to do the usual loop here around imsg_get?  it works
like this, but if we ever grow the set of messages it could lead to
issues, and looping would reduce the diff with the other dispatch
functions.

(well, not that the auth process should do more, so it's fine like it
is already)

> +	evtimer_del(&client->tmo);
> +
> +	switch (imsg.hdr.type) {
> +	case GOTD_IMSG_ERROR:
> +		do_disconnect = 1;
> +		err = gotd_imsg_recv_error(&client_id, &imsg);
> +		break;
> +	case GOTD_IMSG_ACCESS_GRANTED:
> +		break;
> +	default:
> +		do_disconnect = 1;
> +		log_debug("unexpected imsg %d", imsg.hdr.type);
> +		break;
> +	}
> +
> +	if (!verify_imsg_src(client, client->auth, &imsg)) {
> +		do_disconnect = 1;
> +		log_debug("dropping imsg type %d from PID %d",
> +		    imsg.hdr.type, client->auth->pid);
> +	}
> +	imsg_free(&imsg);
> +
> +	if (do_disconnect) {
> +		if (err)
> +			disconnect_on_error(client, err);
> +		else
> +			disconnect(client);
> +		goto done;
> +	}
> +
> +	repo = find_repo_by_name(client->auth->repo_name);
> +	if (repo == NULL) {
> +		err = got_error(GOT_ERR_NOT_GIT_REPO);
> +		goto done;
> +	}
> +	kill_auth_proc(client);
> +
> +	log_info("authenticated uid %d for repository %s\n",
> +	    client->euid, repo->name);
> +
> +	if (client->required_auth & GOTD_AUTH_WRITE)
> +		proc_type = PROC_REPO_WRITE;
> +	else
> +		proc_type = PROC_REPO_READ;
> +
> +	err = start_repo_child(client, proc_type, repo,
> +		gotd.argv0, gotd.confpath, gotd.daemonize,
> +		gotd.verbosity);
> +done:
> +	if (err)
> +		log_warnx("uid %d: %s", client->euid, err->msg);
> +
> +	/* We might have killed the auth process by now. */
> +	if (client->auth != NULL) {
> +		if (!shut) {
> +			gotd_imsg_event_add(iev);
> +		} else {
> +			/* This pipe is dead. Remove its event handler */
> +			event_del(&iev->ev);
> +		}
> +	}
> +}
> [...]