On Tue, Apr 16, 2024 at 10:11:53AM +0200, Omar Polo wrote:
> On 2024/04/16 01:25:04 +0200, Omar Polo <op@omarpolo.com> wrote:
> > with this the authentication is properly implemented.  I'm unsure how to
> > plug it in the regress, so for the moment no tests.
> 
> here's an improved diff without extra include and with the regress too.
> It makes the log output more verbose as a drawback though:
> 
> : gotd: gotd: WARNING: Using basic authentication over plaintext http://
> : will leak credentials; https:// is recommended for URL
> : 'http://localhost:8000/'

Looks good to me, ok stsp@

One thing I don't quite understand is how our regress verifies
authentication. The HTTP server seems to compare the auth token
to a known base64 encoded value?  Could we use some Perl module
to check whether a real web server will accept the token we send?

> diff /home/op/w/got
> commit - c1003102a22a77d068a14f9ffa7877f67c28e95d
> path + /home/op/w/got
> blob - 9c8a90e392c27e03eb1dd32a88414aa62a355012
> file + gotd/libexec/got-notify-http/got-notify-http.c
> --- gotd/libexec/got-notify-http/got-notify-http.c
> +++ gotd/libexec/got-notify-http/got-notify-http.c
> @@ -730,17 +730,70 @@ jsonify(FILE *fp, const char *repo)
>  	return 0;
>  }
>  
> +static char
> +sixet2ch(int c)
> +{
> +	c &= 0x3F;
> +
> +	if (c < 26)
> +		return 'A' + c;
> +	c -= 26;
> +	if (c < 26)
> +		return 'a' + c;
> +	c -= 26;
> +	if (c < 10)
> +		return '0' + c;
> +	c -= 10;
> +	if (c == 0)
> +		return '+';
> +	if (c == 1)
> +		return '/';
> +
> +	errx(1, "invalid sixet 0x%x", c);
> +}
> +
>  static char *
>  basic_auth(const char *username, const char *password)
>  {
> -	char	*tmp;
> -	int	 len;
> +	char	*str, *tmp, *end, *s, *p;
> +	char	 buf[3];
> +	int	 len, i, r;
>  
> -	len = asprintf(&tmp, "%s:%s", username, password);
> -	if (len == -1)
> +	r = asprintf(&str, "%s:%s", username, password);
> +	if (r == -1)
>  		err(1, "asprintf");
>  
> -	/* XXX base64-ify */
> +	/*
> +	 * Will need 4 * r/3 bytes to encode the string, plus a
> +	 * rounding to the next multiple of 4 for padding, plus NUL.
> +	 */
> +	len = 4 * r / 3;
> +	len = (len + 3) & ~3;
> +	len++;
> +
> +	tmp = calloc(1, len);
> +	if (tmp == NULL)
> +		err(1, "malloc");
> +
> +	s = str;
> +	p = tmp;
> +	while (*s != '\0') {
> +		memset(buf, 0, sizeof(buf));
> +		for (i = 0; i < 3 && *s != '\0'; ++i, ++s)
> +			buf[i] = *s;
> +
> +		*p++ = sixet2ch(buf[0] >> 2);
> +		*p++ = sixet2ch((buf[1] >> 4) | (buf[0] << 4));
> +		if (i > 1)
> +			*p++ = sixet2ch((buf[1] << 2) | (buf[2] >> 6));
> +		if (i > 2)
> +			*p++ = sixet2ch(buf[2]);
> +	}
> +
> +	for (end = tmp + len - 1; p < end; ++p)
> +		*p = '=';
> +
> +	free(str);
>  	return tmp;
>  }
>  
> blob - 682368c09afd8f26371f3c0eb5b63f700ac39026
> file + gotd/notify.c
> --- gotd/notify.c
> +++ gotd/notify.c
> @@ -161,7 +161,8 @@ gotd_notify_sighdlr(int sig, short event, void *arg)
>  }
>  
>  static void
> -run_notification_helper(const char *prog, const char **argv, int fd)
> +run_notification_helper(const char *prog, const char **argv, int fd,
> +    const char *user, const char *pass)
>  {
>  	const struct got_error *err = NULL;
>  	pid_t pid;
> @@ -185,6 +186,11 @@ run_notification_helper(const char *prog, const char *
>  
>  		closefrom(STDERR_FILENO + 1);
>  
> +		if (user != NULL && pass != NULL) {
> +			setenv("GOT_NOTIFY_HTTP_USER", user, 1);
> +			setenv("GOT_NOTIFY_HTTP_PASS", pass, 1);
> +		}
> +
>  		if (execv(prog, (char *const *)argv) == -1) {
>  			fprintf(stderr, "%s: exec %s: %s\n", getprogname(),
>  			    prog, strerror(errno));
> @@ -249,7 +255,8 @@ notify_email(struct gotd_notification_target *target, 
>  
>  	argv[i] = NULL;
>  
> -	run_notification_helper(GOTD_PATH_PROG_NOTIFY_EMAIL, argv, fd);
> +	run_notification_helper(GOTD_PATH_PROG_NOTIFY_EMAIL, argv, fd,
> +	    NULL, NULL);
>  }
>  
>  static void
> @@ -273,7 +280,8 @@ notify_http(struct gotd_notification_target *target, c
>  
>  	argv[argc] = NULL;
>  
> -	run_notification_helper(GOTD_PATH_PROG_NOTIFY_HTTP, argv, fd);
> +	run_notification_helper(GOTD_PATH_PROG_NOTIFY_HTTP, argv, fd,
> +	    target->conf.http.user, target->conf.http.password);
>  }
>  
>  static const struct got_error *
> blob - 0dee7475233829d5d5fc2cc3f028cfc7789ce428
> file + regress/gotd/Makefile
> --- regress/gotd/Makefile
> +++ regress/gotd/Makefile
> @@ -188,7 +188,7 @@ start_gotd_http_notification: ensure_root
>  	@echo '    path "$(GOTD_TEST_REPO)"' >> $(PWD)/gotd.conf
>  	@echo '    permit rw $(GOTD_DEVUSER)' >> $(PWD)/gotd.conf
>  	@echo '    notify {' >> $(PWD)/gotd.conf
> -	@echo '         url "http://localhost:${GOTD_TEST_HTTP_PORT}/"' >> $(PWD)/gotd.conf
> +	@echo '         url "http://localhost:${GOTD_TEST_HTTP_PORT}/" user flan password "password"' >> $(PWD)/gotd.conf
>  	@echo "    }" >> $(PWD)/gotd.conf
>  	@echo "}" >> $(PWD)/gotd.conf
>  	@$(GOTD_TRAP); $(GOTD_START_CMD)
> blob - 5cbff3264bfe341856058856515d41c34ed996bf
> file + regress/gotd/http-server
> --- regress/gotd/http-server
> +++ regress/gotd/http-server
> @@ -18,10 +18,11 @@ use v5.36;
>  use IPC::Open2;
>  use Getopt::Long qw(:config bundling);
>  
> +my $auth;
>  my $port = 8000;
>  
> -GetOptions("p:i" => \$port)
> -    or die("usage: $0 [-p port]\n");
> +GetOptions("a:s" => \$auth, "p:i" => \$port)
> +    or die("usage: $0 [-a auth] [-p port]\n");
>  
>  my $pid = open2(my $out, my $in, 'nc', '-l', 'localhost', $port);
>  
> @@ -61,6 +62,15 @@ while (<$out>) {
>  		    unless m/Connection: close$/;
>  		next;
>  	}
> +
> +	if (m/Authorization/) {
> +		die "bad authorization header"
> +		    unless m/Authorization: basic (.*)$/;
> +		my $t = $1;
> +		die "wrong authorization; got $t want $auth"
> +		    if not defined($auth) or $auth ne $t;
> +		next;
> +	}
>  }
>  
>  die "no Content-Length header" unless defined $clen;
> blob - 38661743a189d324f50db59d65ebfb278ed84eff
> file + regress/gotd/http_notification.sh
> --- regress/gotd/http_notification.sh
> +++ regress/gotd/http_notification.sh
> @@ -17,6 +17,9 @@
>  . ../cmdline/common.sh
>  . ./common.sh
>  
> +# flan:password encoded in base64
> +AUTH="ZmxhbjpwYXNzd29yZA=="
> +
>  test_file_changed() {
>  	local testroot=`test_init file_changed 1`
>  
> @@ -41,7 +44,7 @@ test_file_changed() {
>  	local commit_id=`git_show_head $testroot/repo-clone`
>  	local author_time=`git_show_author_time $testroot/repo-clone`
>  
> -	timeout 5 ./http-server -p $GOTD_TEST_HTTP_PORT \
> +	timeout 5 ./http-server -a $AUTH -p $GOTD_TEST_HTTP_PORT \
>  	    > $testroot/stdout &
>  
>  	got send -b main -q -r $testroot/repo-clone
> @@ -134,7 +137,7 @@ test_bad_utf8() {
>  	local commit_id=`git_show_head $testroot/repo-clone`
>  	local author_time=`git_show_author_time $testroot/repo-clone`
>  
> -	timeout 5 ./http-server -p $GOTD_TEST_HTTP_PORT \
> +	timeout 5 ./http-server -a $AUTH -p $GOTD_TEST_HTTP_PORT \
>  	    > $testroot/stdout &
>  
>  	got send -b main -q -r $testroot/repo-clone
> @@ -229,7 +232,7 @@ test_many_commits_not_summarized() {
>  		set -- "$@" "$commit_id $d"
>  	done
>  
> -	timeout 5 ./http-server -p "$GOTD_TEST_HTTP_PORT" \
> +	timeout 5 ./http-server -a $AUTH -p "$GOTD_TEST_HTTP_PORT" \
>  	    > $testroot/stdout &
>  
>  	got send -b main -q -r $testroot/repo-clone
> @@ -334,7 +337,7 @@ test_many_commits_summarized() {
>  		set -- "$@" "$short_commit_id $d"
>  	done
>  
> -	timeout 5 ./http-server -p "$GOTD_TEST_HTTP_PORT" \
> +	timeout 5 ./http-server -a $AUTH -p "$GOTD_TEST_HTTP_PORT" \
>  	    > $testroot/stdout &
>  
>  	got send -b main -q -r $testroot/repo-clone
> @@ -414,7 +417,7 @@ test_branch_created() {
>  	local commit_id=`git_show_branch_head $testroot/repo-clone newbranch`
>  	local author_time=`git_show_author_time $testroot/repo-clone $commit_id`
>  
> -	timeout 5 ./http-server -p "$GOTD_TEST_HTTP_PORT" \
> +	timeout 5 ./http-server -a $AUTH -p "$GOTD_TEST_HTTP_PORT" \
>  	    > $testroot/stdout &
>  
>  	got send -b newbranch -q -r $testroot/repo-clone
> @@ -501,7 +504,7 @@ test_branch_removed() {
>  		return 1
>  	fi
>  
> -	timeout 5 ./http-server -p "$GOTD_TEST_HTTP_PORT" \
> +	timeout 5 ./http-server -a $AUTH -p "$GOTD_TEST_HTTP_PORT" \
>  	    > $testroot/stdout &
>  
>  	local commit_id=`git_show_branch_head $testroot/repo-clone newbranch`
> @@ -556,7 +559,7 @@ test_tag_created() {
>  	local commit_id=`git_show_head $testroot/repo-clone`
>  	local tagger_time=`git_show_tagger_time $testroot/repo-clone 1.0`
>  
> -	timeout 5 ./http-server -p "$GOTD_TEST_HTTP_PORT" \
> +	timeout 5 ./http-server -a $AUTH -p "$GOTD_TEST_HTTP_PORT" \
>  	    >$testroot/stdout &
>  
>  	got send -t 1.0 -q -r $testroot/repo-clone
> @@ -634,7 +637,7 @@ test_tag_changed() {
>  	got tag -r $testroot/repo-clone -m "new tag" 1.0 > /dev/null
>  	local tagger_time=`git_show_tagger_time $testroot/repo-clone 1.0`
>  
> -	timeout 5 ./http-server -p "$GOTD_TEST_HTTP_PORT" \
> +	timeout 5 ./http-server -a $AUTH -p "$GOTD_TEST_HTTP_PORT" \
>  	    > $testroot/stdout &
>  
>  	got send -f -t 1.0 -q -r $testroot/repo-clone
> 
>