"GOT", but the "O" is a cute, smiling pufferfish. Index | Thread | Search

From:
Stefan Sperling <stsp@stsp.name>
Subject:
Re: got-notify-http: implement basic auth
To:
Omar Polo <op@omarpolo.com>
Cc:
gameoftrees@openbsd.org
Date:
Tue, 16 Apr 2024 17:15:03 +0200

Download raw body.

Thread
On Tue, Apr 16, 2024 at 10:11:53AM +0200, Omar Polo wrote:
> 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/'

Here is an alternative suggestion that avoids noise in the test suite
and should be somewhat safer:

gotd: /home/stsp/src/got/regress/gotd/gotd.conf:7: http://localhost:8000/: \
  HTTP notifications with basic authentication over plaintext HTTP will \
  leak credentials; add the 'insecure' config keyword if this is intentional
*** Error 1 in /home/stsp/src/got/regress/gotd 

-----------------------------------------------
 make it harder to leak notification credentials over plaintext HTTP
 
diff fd1b27119fe0bafd2e0b5ee26e877c742edfeb0e 953dac4f9d80392b44212d2822adba3e2b218fd5
commit - fd1b27119fe0bafd2e0b5ee26e877c742edfeb0e
commit + 953dac4f9d80392b44212d2822adba3e2b218fd5
blob - 6b1e63e34c2c42b7aeff0c238e2a303c5efa7c2a
blob + a346ab5981dac6f616103dfdb281f5c2785c60a2
--- gotd/gotd.conf.5
+++ gotd/gotd.conf.5
@@ -326,7 +326,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 Oc
+.It Ic url Ar URL Oo Ic user Ar user Ic password Ar password Oo Ic insecure Oc Oc
 Send notifications via HTTP.
 This directive may be specified multiple times to build a list of
 HTTP servers to send notifications to.
@@ -353,6 +353,13 @@ must be specified.
 The
 .Ar password
 must not be an empty string.
+Unless the
+.Ic insecure
+option is specified the notification target
+.Ar URL
+must be a
+.Dq https://
+URL to avoid leaking of authentication credentials.
 .Pp
 The request body contains a JSON object with a
 .Dq notifications
blob - a3f897aa6bfb4b65dde83d437ee62682525ffc9f
blob + 0aa454a1c57aed05f6c61df19f663823fe60f40b
--- gotd/parse.y
+++ gotd/parse.y
@@ -110,7 +110,7 @@ static int			 conf_notify_ref_namespace(struct gotd_re
 static int			 conf_notify_email(struct gotd_repo *,
 				    char *, char *, char *, char *, char *);
 static int			 conf_notify_http(struct gotd_repo *,
-				    char *, char *, char *);
+				    char *, char *, char *, int);
 static enum gotd_procid		 gotd_proc_id;
 
 typedef struct {
@@ -127,7 +127,7 @@ typedef struct {
 %token	PATH ERROR LISTEN ON USER REPOSITORY PERMIT DENY
 %token	RO RW CONNECTION LIMIT REQUEST TIMEOUT
 %token	PROTECT NAMESPACE BRANCH TAG REFERENCE RELAY PORT
-%token	NOTIFY EMAIL FROM REPLY TO URL PASSWORD
+%token	NOTIFY EMAIL FROM REPLY TO URL PASSWORD INSECURE
 
 %token	<v.string>	STRING
 %token	<v.number>	NUMBER
@@ -600,7 +600,7 @@ notifyflags	: BRANCH STRING {
 			    gotd_proc_id == PROC_SESSION_WRITE ||
 			    gotd_proc_id == PROC_NOTIFY) {
 				if (conf_notify_http(new_repo, $2, NULL,
-				    NULL)) {
+				    NULL, 0)) {
 					free($2);
 					YYERROR;
 				}
@@ -611,7 +611,7 @@ notifyflags	: BRANCH STRING {
 			if (gotd_proc_id == PROC_GOTD ||
 			    gotd_proc_id == PROC_SESSION_WRITE ||
 			    gotd_proc_id == PROC_NOTIFY) {
-				if (conf_notify_http(new_repo, $2, $4, $6)) {
+				if (conf_notify_http(new_repo, $2, $4, $6, 0)) {
 					free($2);
 					free($4);
 					free($6);
@@ -622,6 +622,21 @@ notifyflags	: BRANCH STRING {
 			free($4);
 			free($6);
 		}
+		| URL STRING USER STRING PASSWORD STRING INSECURE {
+			if (gotd_proc_id == PROC_GOTD ||
+			    gotd_proc_id == PROC_SESSION_WRITE ||
+			    gotd_proc_id == PROC_NOTIFY) {
+				if (conf_notify_http(new_repo, $2, $4, $6, 1)) {
+					free($2);
+					free($4);
+					free($6);
+					YYERROR;
+				}
+			}
+			free($2);
+			free($4);
+			free($6);
+		}
 		;
 	
 repository	: REPOSITORY STRING {
@@ -767,6 +782,7 @@ lookup(char *s)
 		{ "deny",			DENY },
 		{ "email",			EMAIL },
 		{ "from",			FROM },
+		{ "insecure",			INSECURE },
 		{ "limit",			LIMIT },
 		{ "listen",			LISTEN },
 		{ "namespace",			NAMESPACE },
@@ -1535,7 +1551,8 @@ conf_notify_email(struct gotd_repo *repo, char *sender
 }
 
 static int
-conf_notify_http(struct gotd_repo *repo, char *url, char *user, char *password)
+conf_notify_http(struct gotd_repo *repo, char *url, char *user, char *password,
+    int insecure)
 {
 	const struct got_error *error;
 	struct gotd_notification_target *target;
@@ -1565,10 +1582,13 @@ conf_notify_http(struct gotd_repo *repo, char *url, ch
 		goto done;
 	}
 
-	if (strcmp(proto, "http") == 0 && (user != NULL || password != NULL)) {
-		log_warnx("%s: WARNING: Using basic authentication over "
-		    "plaintext http:// will leak credentials; https:// is "
-		    "recommended for URL '%s'", getprogname(), url);
+	if (!insecure && strcmp(proto, "http") == 0 &&
+	    (user != NULL || password != NULL)) {
+		yyerror("%s: HTTP notifications with basic authentication "
+		    "over plaintext HTTP will leak credentials; add the "
+		    "'insecure' config keyword if this is intentional", url);
+		ret = -1;
+		goto done;
 	}
 
 	STAILQ_FOREACH(target, &repo->notification_targets, entry) {
blob - c00bf5c27253d88367260cc43d44b187d066276e
blob + c38bd07b843aa78426c9e628f609604a4d815417
--- 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}/" user flan password "password"' >> $(PWD)/gotd.conf
+	@echo '         url "http://localhost:${GOTD_TEST_HTTP_PORT}/" user flan password "password" insecure' >> $(PWD)/gotd.conf
 	@echo "    }" >> $(PWD)/gotd.conf
 	@echo "}" >> $(PWD)/gotd.conf
 	@$(GOTD_TRAP); $(GOTD_START_CMD)