From: Omar Polo <op@omarpolo.com>
Subject: Re: resolve account name of users connecting to gotd
To: Stefan Sperling <stsp@stsp.name>
Cc: gameoftrees@openbsd.org
Date: Wed, 27 Dec 2023 14:11:27 +0100

On 2023/12/26 17:54:30 +0100, Stefan Sperling <stsp@stsp.name> wrote:
> Pass the account name of successfully authenticated users from the
> gotd auth process to the gotd parent process.
> 
> This can be used to make log messages more readable. With this patch
> we display the username instead of a UID when reporting successful
> authentication in the logs. We could expand upon this later and tweak
> other messages which currently display the UID. But I actually wrote
> this patch because account name information will be required to
> provide meaningful commit notifications.

It is already useful on its own, I'm happy not to have to grep
/etc/passwd to remember which user is which uid :)

> One quirk which deviates from our usual style is that auth_check() will
> not free the *username pointer on failure. I chose to free it in the
> caller instead to keep the existing auth_check() implementation mostly
> as it is, since the early returns are an important part of the code flow.
> Alternatively, we could do another getpwuid() call in the caller but a
> call to strdup() on the existing passwd object is likely cheaper.

I think it's fine.  Alternatively, we could just use a stack-allocated
buffer and avoid all the headaches with managing another allocation.

> There is a small drive-by fix in here which ensures that the message
> GOTD_IMSG_ACCESS_GRANTED will only be accepted once.
> 
> ok?

ok op@

two nits below.

> [...]
> @@ -200,18 +209,23 @@ recv_authreq(struct imsg *imsg, struct gotd_imsgev *ie
> [...]
>  	if (gotd_imsg_compose_event(iev, GOTD_IMSG_ACCESS_GRANTED,
> -	    PROC_AUTH, -1, NULL, 0) == -1)
> -		return got_error_from_errno("imsg compose ACCESS_GRANTED");
> -
> -	return NULL;
> +	    PROC_AUTH, -1, (void *)username, len) == -1)

the cast to void here is superfluous.

> +		err = got_error_from_errno("imsg compose ACCESS_GRANTED");
> +done:
> +	free(username);
> +	return err;
>  }
>  
>  static void
> blob - 892a54d71ecfa0e0dc9b036bdc334a9cbb1e4225
> blob + 0c6944a273b5b1885488a4dd1d44002cea5cb70e
> --- gotd/gotd.c
> +++ gotd/gotd.c
> @@ -1188,14 +1196,25 @@ gotd_dispatch_auth_child(int fd, short event, void *ar
> [...]
> +	} else {
> +		client->state = GOTD_CLIENT_STATE_ACCESS_GRANTED;
> +		if (datalen > 0)
> +			client->username = strndup(imsg.data, datalen);
> +		if (client->username == NULL &&
> +		    asprintf(&client->username, "uid %d", client->euid) == -1) {
> +			err = got_error_from_errno("asprintf");
> +			imsg_free(&imsg);
> +			goto done;
> +		}
> +		imsg_free(&imsg);

I think it would be clearer without the else block since if we hit
do_disconnect we return early.

>  	}
>  
>  	repo = gotd_find_repo_by_name(client->auth->repo_name, &gotd);