From: Stefan Sperling <stsp@stsp.name>
Subject: Re: HMAC digest for gotd HTTP notifications
To: Omar Polo <op@omarpolo.com>
Cc: gameoftrees@openbsd.org
Date: Mon, 19 Aug 2024 18:38:26 +0200

On Mon, Aug 19, 2024 at 05:54:48PM +0200, Omar Polo wrote:
> we have a very similar text for wireguard in ifconfig(8) so I guess it's
> fine :)
> 
> I'm still a bit worried that we keep these secrets in the config file
> which is installed world-readable (and has to be for gotsh).  Not a
> problem with this diff, just reiterating so that I don't forget :P

Oh right, that's a bit of a problem.

However, we'll need one key per notification target, and there can
be many of those. An smtpd.conf-style table approach might work,
where we'll stub values out if the table isn't accessible, such that
other information can still be parsed by regular users. Will handle
this later.

> > +The signature uses HMAC-SHA256 and will be sent in the HTTP header
> > +.Dq HTTP_X_GOTD_SIGNATURE_256 .
> 
> shouldn't this be X-Gotd-Signature-256?  Actually, even the trailing
> -256 seem superfluous, since we set the value to sha256=abc, so I'd go
> with just X-Gotd-Signature.
> 
> (MDN[0] and W3C notwithstandig, X- is still and will still be used)

Sure, I'm happy to change the name.

> > +	ctx = HMAC_CTX_new();
> > +	if (ctx == NULL) {
> > +		log_warn("HMAC_CTX_new");
> 
> I'm not sure how useful errno is after calls to libcrypto functions.  I
> guess that the indended way to deal with them is to ERR_get_error(),
> see[1], but it may be a bit overkill here.  Just using
> 
> 	log_warnx("$function has failed");
> 
> should be enough.

Fine, will do.

> > +			log_warnx("reading HMAC payload: %s",
> > +			    strerror(ferror(payload)));
> 
> I don't think ferror() returns the errno value, it should just a
> "boolean" to indicate whether it was an EOF or some failure.  `errno'
> should still be consulted.  So, I think we should use
> 
> +			log_warn("failed to read HMAC payload");

Ah yes, that's a mistake. Thanks for catching it.

> > @@ -193,6 +193,11 @@ run_notification_helper(const char *prog, const char *
> >  			setenv("GOT_NOTIFY_HTTP_PASS", pass, 1);
> >  		}
> >  
> > +		if (hmac_secret)
> > +			setenv("GOT_NOTIFY_HTTP_HMAC_SECRET", hmac_secret, 1);
> > +		else
> > +			unsetenv("GOT_NOTIFY_HTTP_HMAC_SECRET");
> 
> do we really need unsetenv() here?  This env is only set in in the
> child, so it shouldn't leak in the parent too.  (otherwise we'd have to
> unset the other variables as well.)

I was worried that some random value could be inherited if the variable
happens to exist in the parent's environment for some reason.

> > +if (defined $hmac_signature) {
> > +	die "no HMAC secret provided" if not (defined $hmac_secret);
> 
> nit: could just be
> 
>   +	die "no Hmac secret provided" unless defined $hmac_secret;
> 
> almost the same number of chars but i think it's slightly more nice :)

Agreed, thanks.