Download raw body.
HMAC digest for gotd HTTP notifications
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.
HMAC digest for gotd HTTP notifications