Download raw body.
HMAC digest for gotd HTTP notifications
On 2024/08/19 12:08:58 +0200, Stefan Sperling <stsp@stsp.name> wrote:
> Add support for HMAC digests to allow authenticating and
> verifying gotd HTTP notifications.
>
> HMAC is used by several Git forges to authenticate their webhooks,
> rather than using basic auth. It makes sense for gotd to provide
> a similar scheme in addition to basic authentication. Even though
> at present none of the forges support gotd notifications, having
> this feature available might help with having such support in
> some forges in the future.
>
> The way it works is based on forgejo's docs:
> https://forgejo.org/docs/latest/user/webhooks/
> We provide a custom HTTP header which contains a sha256 HMAC digest,
> which can be verified by the recipient if they know the shared secret.
> A new regression test verifies the digest in Perl, which can also
> serve as a general example of how to implement verification.
>
> Our manual page suggests using openssl rand to generate secrets.
> I hope this is enough to ensure that good secrets will be used,
> though it's entirely up to the admin of gotd to set this up securely.
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
> Feature addition suggested by dch@freebsd.org in private mail.
>
> ok?
ok op with some minor nits inline
> M gotd/gotd.conf.5 | 14+ 1-
> M gotd/gotd.h | 1+ 0-
> M gotd/libexec/got-notify-http/Makefile | 2+ 2-
> M gotd/libexec/got-notify-http/got-notify-http.c | 89+ 2-
> M gotd/notify.c | 9+ 3-
> M gotd/parse.y | 64+ 6-
> M regress/gotd/Makefile | 21+ 1-
> M regress/gotd/README | 1+ 1-
> M regress/gotd/http-server | 31+ 2-
> A regress/gotd/http_notification_hmac.sh | 114+ 0-
>
> 10 files changed, 346 insertions(+), 18 deletions(-)
>
> commit - bc7dcd6603946972172d2e08412488029d34759a
> commit + 5dba089cd36617a108a1b7b28b178db09084af06
> blob - fc2b64876b2b20537494f7fa72e0f2a19756ca54
> blob + 4638c10ed322c7d77ce9a38b39a234f80a172047
> --- gotd/gotd.conf.5
> +++ gotd/gotd.conf.5
> @@ -333,7 +333,7 @@ The
> and
> .Ic port
> directives can be used to specify a different SMTP server address and port.
> -.It Ic url Ar URL Oo Ic user Ar user Ic password Ar password Oo Ic insecure Oc Oc
> +.It Ic url Ar URL Oo Ic user Ar user Ic password Ar password Oo Ic insecure Oc Oc Oo Ic hmac Ar secret Oc
> Send notifications via HTTP.
> This directive may be specified multiple times to build a list of
> HTTP servers to send notifications to.
> @@ -368,6 +368,19 @@ must be a
> .Dq https://
> URL to avoid leaking of authentication credentials.
> .Pp
> +If a
> +.Ic hmac
> +.Ar secret
> +is provided, the request body will be signed using HMAC, allowing the
> +receiver to verify the notification message's authenticity and integrity.
> +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)
[0]: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers
> +Suitable secrets can be generated with
> +.Xr openssl 1
> +as follows:
> +.Pp
> +.Dl $ openssl rand -base64 32
> +.Pp
> The request body contains a JSON object with a
> .Dq notifications
> property containing an array of notification objects.
> [...]
> --- gotd/libexec/got-notify-http/got-notify-http.c
> +++ gotd/libexec/got-notify-http/got-notify-http.c
> [...]
> +static unsigned char *
> +compute_hmac_sha256(FILE *payload, off_t paylen, const char *hmac_secret,
> + size_t secret_len, unsigned char *hmac_sig_buf, unsigned int *hmac_siglen)
> +{
> + HMAC_CTX *ctx;
> + char buf[4096];
> + off_t n;
> + ssize_t r;
> +
> + *hmac_siglen = 0;
> +
> + 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.
[1]: https://git.omarpolo.com/?action=blob&commit=2922e3f14f02dfd00440e5b528fb7c38dab81050&file=utils.c&folder=&path=gmid.git#line382
> + return NULL;
> + }
> +
> + if (!HMAC_Init_ex(ctx, hmac_secret, secret_len, EVP_sha256(), NULL)) {
> + log_warn("HMAC_Init_ex");
ditto
> + goto fail;
> + }
> +
> + n = paylen;
> + while (n > 0) {
> + r = fread(buf, 1, n > sizeof(buf) ? sizeof(buf) : n, payload);
> + if (r == 0) {
> + if (feof(payload)) {
> + log_warnx("HMAC payload truncated");
> + goto fail;
> + }
> + 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");
> + goto fail;
> + }
> + if (!HMAC_Update(ctx, buf, r)) {
ditto about errno and libcrypto
> + log_warn("HMAC_Update");
> + goto fail;
> + }
> + n -= r;
> + }
> +
> + if (!HMAC_Final(ctx, hmac_sig_buf, hmac_siglen)) {
ditto
> + log_warn("HMAC_Final");
> + goto fail;
> + }
> +
> + *hmac_siglen = HMAC_size(ctx);
> +
> + HMAC_CTX_free(ctx);
> + return hmac_sig_buf;
> +fail:
> + HMAC_CTX_free(ctx);
> + return NULL;
> +}
> +
> [...]
> @@ -964,10 +1046,15 @@ main(int argc, char **argv)
> "Content-Type: application/json\r\n"
> "Content-Length: %lld\r\n"
> "User-Agent: %s\r\n"
> - "Connection: close\r\n",
> + "Connection: close\r\n"
> + "%s%s%s%s",
> path, host,
> nonstd ? ":" : "", nonstd ? port : "",
> - (long long)paylen, USERAGENT);
> + (long long)paylen, USERAGENT,
> + hmac_sig ? "HTTP_X_GOTD_SIGNATURE_256: " : "",
ditto about the name of the header.
> + hmac_sig ? "sha256=" : "",
> + hmac_sig ? hex : "",
> + hmac_sig ? "\r\n" : "");
> if (ret == -1)
> fatal("bufio_compose_fmt");
>
> blob - 20958912de97f4002cb2fa805ef1624b48a47647
> blob + 804d5eca66fbe09fb4b30ded4f78f34438caf7d9
> --- gotd/notify.c
> +++ gotd/notify.c
> @@ -164,7 +164,7 @@ gotd_notify_sighdlr(int sig, short event, void *arg)
>
> static void
> run_notification_helper(const char *prog, const char **argv, int fd,
> - const char *user, const char *pass)
> + const char *user, const char *pass, const char *hmac_secret)
> {
> const struct got_error *err = NULL;
> pid_t pid;
> @@ -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.)
> if (execv(prog, (char *const *)argv) == -1) {
> fprintf(stderr, "%s: exec %s: %s\n", getprogname(),
> prog, strerror(errno));
> [...]
> --- regress/gotd/http-server
> +++ regress/gotd/http-server
> [...]
> @@ -71,10 +76,23 @@ while (<$out>) {
> if not defined($auth) or $auth ne $t;
> next;
> }
> +
> + if (m/HTTP_X_GOTD_SIGNATURE_256/) {
ditto about the header name
> + die "bad hmac signature header"
> + unless m/HTTP_X_GOTD_SIGNATURE_256: sha256=(.*)$/;
> + $hmac_signature = $1;
> + next;
> + }
> }
>
> die "no Content-Length header" unless defined $clen;
>
> +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 :)
> + my $sha256 = Digest->new("SHA-256");
> + $hmac = Digest::HMAC->new($hmac_secret, $sha256);
> +}
> +
> while ($clen != 0) {
> my $len = $clen;
> $len = 512 if $clen > 512;
HMAC digest for gotd HTTP notifications