threads / rfc / 42528

RFC patchpush: deny policy to prevent pushes to unwanted remotes.

Subject: [RFC/PATCH] push: deny policy to prevent pushes to unwanted remotes.

## tl;dr

10 messages between Jun 4, 2016 and Jun 21, 2016. Diffs are folded; open one to read it.

replies: 9people: 6as markdown or json

Antoine Queru· Jun 4, 2016, 14:51 UTC · lore

Currently, a user wanting to prevent accidental pushes to the wrong remote has to create a pre-push hook. The feature offers a configuration to allow users to prevent accidental pushes to the wrong remote. The user may define a list of whitelisted remotes, a list of blacklisted remotes and a default policy ("allow" or "deny"). A push is denied if the remote is explicitely blacklisted or if it isn't whitelisted and the default policy is "deny".

This feature is intended as a safety net more than a real security, the user will always be able to modify the config if he wants to. It is here for him to consciously restrict his push possibilities. For example, it may be useful for an unexperimented user fearing to push to the wrong remote, or for companies wanting to avoid unintentionnal leaking of private code on public repositories.

By default the whitelist/blacklist feature is disabled since the default policy is "allow".

Signed-off-by: Antoine Queru <antoine.queru@grenoble-inp.org>
Signed-off-by: Francois Beutin <francois.beutin@grenoble-inp.org>
Signed-off-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
---

Changes since v1 : Scheme handled. Documentation fixed and moved. Option "-no-verify" added to dismiss this process.

Right now, one function "longest_prefix_size" is pretty awful because url_normalize treat local path with "file://..." as an url but not local path without this prefix. So a local path and a repo without scheme are seen likewise. Forbid local path without "file://.." could make the code easier and faster. What do you think ?

User, password and port are not treated. Should it be ?
 Documentation/config.txt            |  17 ++++++
 Documentation/urls-remotes.txt      |  55 ++++++++++++++++++
 builtin/push.c                      | 110 ++++++++++++++++++++++++++++++++++--
 t/t5544-push-whitelist-blacklist.sh |  51 +++++++++++++++++
 4 files changed, 228 insertions(+), 5 deletions(-)
 create mode 100755 t/t5544-push-whitelist-blacklist.sh
Show changes to 4 files +228 −5

Documentation/config.txt, Documentation/urls-remotes.txt, builtin/push.c, t/t5544-push-whitelist-blacklist.sh

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 53f00db..7fe88f5 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2517,6 +2517,23 @@ remote.pushDefault::
 	`branch.<name>.remote` for all branches, and is overridden by
 	`branch.<name>.pushRemote` for specific branches.
 
+remote.pushBlacklist::
+	The list of remotes the user is forbidden to push to.
+	See linkgit:git-push[1]
+
+remote.pushWhitelist::
+	The list of remotes the user is allowed to push to.
+	See linkgit:git-push[1]
+
+remote.pushDefaultPolicy::
+	Can be set to either 'allow' or 'deny', defines the policy to
+	adopt when the user tries to push to a remote not in the
+	whitelist or the blacklist. Defaults to 'allow'.
+
+remote.pushDenyMessage::
+	The message printed when pushing to a forbidden remote.
+	Defaults to "Pushing to this remote has been forbidden".
+
 remote.<name>.url::
 	The URL of a remote repository.  See linkgit:git-fetch[1] or
 	linkgit:git-push[1].
diff --git a/Documentation/urls-remotes.txt b/Documentation/urls-remotes.txt
index bd184cd..705914d 100644
--- a/Documentation/urls-remotes.txt
+++ b/Documentation/urls-remotes.txt
@@ -89,6 +89,61 @@ git push uses:
 	HEAD:refs/heads/<head>
 ------------
 
+Denying pushes to the wrong remotes
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+If you fear accidental pushes to the wrong remotes, you can use the
+blacklist/whitelist feature. The goal is to catch pushes to unwanted
+remotes before they happen.
+
+The simplest way to forbid pushes to a remote is to add its url in the
+config file under the 'remote.pushBlacklist' variable.  A more
+restrictive way is to change 'remote.pushDefaultPolicy' to 'deny',
+thus denying pushes to every remote not whitelisted. You can then add
+the right remotes under 'remote.pushWhitelist'.
+
+You can also configure more advanced acceptation/denial behavior
+following this rule: the more the url in the config prefixes the asked
+url the more priority it has.
+
+For example, if we set up the configuration variables like this:
+
+-------------------------------
+git config --add remote.pushBlacklist repository.com
+git config --add remote.pushWhitelist repository.com/Special_Folder
+-------------------------------
+
+Pushes like this  will be accepted:
+-------------------------------
+git push repository.com/Special_Path/*
+-------------------------------
+
+While this one for example will be denied:
+-------------------------------
+git push repository.com/Other_Path/
+-------------------------------
+
+Remember to use '--add' with 'git config' to add more than one
+'remote.pushBlacklist'.
+
+Specific schemes can also be denied. For example :
+
+-------------------------------
+git config --add remote.pushBlacklist http://repo.com
+-------------------------------
+
+By doing so, only pushes to repo.com using 'http' will be denied,
+whereas other will be allowed, like 'https'. If you want to deny
+all pushes to a certain repo, don't put scheme at the beginning.
+
+Additionally, you can configure the message printed when a push is
+denied with the 'remote.pushDenyMessage' configuration variable.
+
+You can dismiss this process by using '--no-verify'.
+
+An error will be raised if the url is blacklisted and whitelisted at
+the same time.
+
 
 
 
diff --git a/builtin/push.c b/builtin/push.c
index 4e9e4db..bf20257 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -11,6 +11,8 @@
 #include "submodule.h"
 #include "submodule-config.h"
 #include "send-pack.h"
+#include "urlmatch.h"
+#include "url.h"
 
 static const char * const push_usage[] = {
 	N_("git push [<options>] [<repository> [<refspec>...]]"),
@@ -353,6 +355,101 @@ static int push_with_options(struct transport *transport, int flags)
 	return 1;
 }
 
+static void check_length_prefix(const int whitelist,
+				const int blacklist,
+				const char *repo,
+				const char *deny_message,
+				const char *default_policy)
+{
+	if (whitelist < blacklist)
+		die(_("%s"), deny_message);
+	if (whitelist == blacklist) {
+		if (whitelist)
+			die(_("%s cannot be whitelisted and blacklisted"
+			      "at the same time"), repo);
+		if (!strcmp(default_policy, "deny"))
+			die(_("%s"), deny_message);
+		if (strcmp(default_policy, "allow"))
+			die(_("Unrecognized value for remote.pushDefaultPolicy,"
+			      "should be 'allow' or 'deny'"));
+	}
+
+}
+
+static void find_longest_prefix(const char *str, const char *pre, int *max)
+{
+	if (starts_with(str, pre)) {
+		int tmp = strlen(pre);
+		if (*max < tmp)
+			*max = tmp;
+	}
+}
+
+/*
+ *NEEDSWORK: Ugly because file://... is recognized as an url, and we
+ *may want to compare it to local path without this scheme. Forcing
+ *the user to put file:// before every local path would make the code
+ *easier and avoid confusion with a distant repo like 'github.com'
+ *which is not an url.
+ */
+static int longest_prefix_size(const char* target_str,
+				const struct string_list *list)
+{
+	struct string_list_item *curr_item;
+	struct url_info target_url;
+	int max_size = 0;
+	if (!list)
+		return 0;
+
+	skip_prefix(target_str, "file://", &target_str);
+	url_normalize(target_str, &target_url);
+
+	for_each_string_list_item(curr_item, list) {
+		struct url_info curr_url;
+		const char *curr_str = curr_item->string;
+		skip_prefix(curr_str, "file://", &curr_str);
+		url_normalize(curr_str, &curr_url);
+		if (target_url.url &&
+		    curr_url.url &&
+		    target_url.scheme_len == curr_url.scheme_len &&
+		    !strncmp(target_url.url,curr_url.url,curr_url.scheme_len))
+			find_longest_prefix(target_url.url + target_url.host_off,
+					    curr_url.url + curr_url.host_off,
+					    &max_size);
+		else if (target_url.url && !curr_url.url)
+			find_longest_prefix(target_url.url + target_url.host_off,
+					    curr_str,
+					    &max_size);
+
+		else if(!target_url.url && !curr_url.url)
+			find_longest_prefix(target_str, curr_str, &max_size);
+	}
+	return max_size;
+}
+
+static void block_unauthorized_url(const char *repo)
+{
+	struct url_info target_url;
+	const char *default_policy, *deny_message;
+	const struct string_list *whitelist, *blacklist;
+	int whitelist_size, blacklist_size;
+
+	url_normalize(repo, &target_url);
+
+	if (git_config_get_value("remote.pushDefaultPolicy", &default_policy))
+		default_policy = "allow";
+	if (git_config_get_value("remote.pushDenyMessage", &deny_message))
+		deny_message = "Pushing to this remote using this protocol is forbidden";
+
+	whitelist = git_config_get_value_multi("remote.pushWhitelist");
+	blacklist = git_config_get_value_multi("remote.pushBlacklist");
+
+	whitelist_size = longest_prefix_size(repo, whitelist);
+	blacklist_size = longest_prefix_size(repo, blacklist);
+
+	check_length_prefix(whitelist_size, blacklist_size, repo, deny_message, default_policy);
+}
+
 static int do_push(const char *repo, int flags)
 {
 	int i, errs;
@@ -404,15 +501,18 @@ static int do_push(const char *repo, int flags)
 	url_nr = push_url_of_remote(remote, &url);
 	if (url_nr) {
 		for (i = 0; i < url_nr; i++) {
-			struct transport *transport =
-				transport_get(remote, url[i]);
+			struct transport *transport;
+			if (!(flags & TRANSPORT_PUSH_NO_HOOK))
+				block_unauthorized_url(url[i]);
+			transport = transport_get(remote, url[i]);
 			if (push_with_options(transport, flags))
 				errs++;
 		}
 	} else {
-		struct transport *transport =
-			transport_get(remote, NULL);
-
+		struct transport *transport;
+		if (!(flags & TRANSPORT_PUSH_NO_HOOK))
+			block_unauthorized_url(remote->url[0]);
+		transport = transport_get(remote, NULL);
 		if (push_with_options(transport, flags))
 			errs++;
 	}
diff --git a/t/t5544-push-whitelist-blacklist.sh b/t/t5544-push-whitelist-blacklist.sh
new file mode 100755
index 0000000..8f9f056
--- /dev/null
+++ b/t/t5544-push-whitelist-blacklist.sh
@@ -0,0 +1,51 @@
+#!/bin/sh
+
+test_description='blacklist for push'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+	git init --bare blacklist/ &&
+	git init --bare whitelist/ &&
+	git init --bare blacklist/allow &&
+	test_commit commit &&
+	echo "fatal: Pushing to this remote using this protocol is forbidden" > forbidden
+'
+
+test_expect_success 'basic case' '
+	git config --add remote.pushBlacklist http://blacklist.com &&
+	test_must_fail git push http://blacklist.com HEAD 2> result &&
+	test_cmp result forbidden
+'
+
+test_expect_success 'no scheme and no path' '
+	git config remote.pushBlacklist blacklist.com &&
+	test_must_fail git push http://blacklist.com/foo HEAD 2> result &&
+	test_cmp result forbidden
+'
+
+test_expect_success 'local path' '
+	git config remote.pushBlacklist blacklist &&
+	test_must_fail git push blacklist HEAD 2> result &&
+	test_cmp result forbidden
+'
+
+test_expect_success 'local path with file://' '
+	git config remote.pushBlacklist file://blacklist &&
+	test_must_fail git push blacklist HEAD 2> result &&
+	test_cmp result forbidden
+'
+test_expect_success 'only one scheme allowed' '
+	git config remote.pushDefaultPolicy deny &&
+	git config remote.pushWhitelist http://blacklist.com &&
+	test_must_fail git push https://blacklist.com HEAD 2> result &&
+	test_cmp result forbidden
+'
+
+test_expect_success 'denied repo in allowed repo' '
+	git config remote.pushBlacklist blacklist &&
+	git config --add remote.pushWhitelist blacklist/allow &&
+	git push blacklist/allow HEAD
+'
+
+test_done
-- 
2.8.2.625.g2f87638.dirty
Remi Galan Alfonso· Jun 6, 2016, 11:39 UTC · re: Antoine Queru · lore

Re: [RFC/PATCH] push: deny policy to prevent pushes to unwanted remotes.

Hi Antoine,
Antoine Queru <Antoine.Queru@grenoble-inp.org> writes:
Show 12 quoted lines
> [...]
> +For example, if we set up the configuration variables like this:
> +
> +-------------------------------
> +git config --add remote.pushBlacklist repository.com
> +git config --add remote.pushWhitelist repository.com/Special_Folder
> +-------------------------------
> +
> +Pushes like this  will be accepted:
> +-------------------------------
> +git push repository.com/Special_Path/*
> +-------------------------------
According to your previous `git config`, it should be:
	git push repository.com/Special_Folder/*
Show 14 quoted lines
> +
> +While this one for example will be denied:
> +-------------------------------
> +git push repository.com/Other_Path/
> +-------------------------------
> +
> [...]
> +/*
> + *NEEDSWORK: Ugly because file://... is recognized as an url, and we
> + *may want to compare it to local path without this scheme. Forcing
> + *the user to put file:// before every local path would make the code
> + *easier and avoid confusion with a distant repo like 'github.com'
> + *which is not an url.
> + */
Style: space after '*' (when there is text after), meaning:
	/*
	 * NEEDSWORK: Ugly because file://... is recognized [...]
	 * [...]
	 */
> +static int longest_prefix_size(const char* target_str,
> +                                const struct string_list *list)
That might just be my mailer but this line is not properly lined up
with the previous one (one space too much).
It should be:
static int longest_prefix_size(const char* target_str,
			       const struct string_list *list)
Show 8 quoted lines
> [...]
> +        for_each_string_list_item(curr_item, list) {
> +                struct url_info curr_url;
> +                const char *curr_str = curr_item->string;
> +                skip_prefix(curr_str, "file://", &curr_str);
> +                url_normalize(curr_str, &curr_url);
> +                if (target_url.url &&
> +                    curr_url.url &&
You can put target_url.url and curr_url.url on the same line.
> +                    target_url.scheme_len == curr_url.scheme_len &&
> +                    !strncmp(target_url.url,curr_url.url,curr_url.scheme_len))
Style: space after ','.
With those two things, the condition would look like this:
		if (target_url.url && curr_url.url &&
		    target_url.scheme_len == curr_url.scheme_len &&
		    !strncmp(target_url.url, curr_url.url, curr_url.scheme_len))
Show 5 quoted lines
> [...]
> +        whitelist_size = longest_prefix_size(repo, whitelist);
> +        blacklist_size = longest_prefix_size(repo, blacklist);
> +
> +        check_length_prefix(whitelist_size, blacklist_size, repo, deny_message, default_policy);
This line is above 80 characters, so:
	check_length_prefix(whitelist_size, blacklist_size, repo, deny_message,
			    default_policy);
Show 11 quoted lines
> [...]
> +test_expect_success 'setup' '
> +        git init --bare blacklist/ &&
> +        git init --bare whitelist/ &&
> +        git init --bare blacklist/allow &&
> +        test_commit commit &&
> +        echo "fatal: Pushing to this remote using this protocol is forbidden" > forbidden
> +'
> +
> +test_expect_success 'basic case' '
> +        git config --add remote.pushBlacklist http://blacklist.com &&

You use `git config` instead of `test_config`, meaning that the configuration you introduce will persist after the test.

It is not really a problem here since for the other tests you don't use `git config --add` so the configuration will be overwritten. However I still think you should use `test_config` to avoid causing trouble to potential future tests that would use `--add` and expect a clean state.

Show 6 quoted lines
> [...]
> +test_expect_success 'local path with file://' '
> +        git config remote.pushBlacklist file://blacklist &&
> +        test_must_fail git push blacklist HEAD 2> result &&
> +        test_cmp result forbidden
> +'
(you forgot a new line here)
Show 8 quoted lines
> +test_expect_success 'only one scheme allowed' '
> +        git config remote.pushDefaultPolicy deny &&
> +        git config remote.pushWhitelist http://blacklist.com &&
> +        test_must_fail git push https://blacklist.com HEAD 2> result &&
> +        test_cmp result forbidden
> +'
> +
> +test_expect_success 'denied repo in allowed repo' '

'allowed repo in denied remote'? In any case the current title is misleading for me.

> +        git config remote.pushBlacklist blacklist &&
> +        git config --add remote.pushWhitelist blacklist/allow &&
> +        git push blacklist/allow HEAD
> +'

Thanks, Rémi

Antoine Queru· Jun 6, 2016, 13:43 UTC · re: Remi Galan Alfonso · lore

Re: [RFC/PATCH] push: deny policy to prevent pushes to unwanted remotes.

----- Mail original -----
Show 135 quoted lines
> Hi Antoine,
> 
> Antoine Queru <Antoine.Queru@grenoble-inp.org> writes:
> > [...]
> > +For example, if we set up the configuration variables like this:
> > +
> > +-------------------------------
> > +git config --add remote.pushBlacklist repository.com
> > +git config --add remote.pushWhitelist repository.com/Special_Folder
> > +-------------------------------
> > +
> > +Pushes like this  will be accepted:
> > +-------------------------------
> > +git push repository.com/Special_Path/*
> > +-------------------------------
> 
> According to your previous `git config`, it should be:
> 	git push repository.com/Special_Folder/*
> 
> > +
> > +While this one for example will be denied:
> > +-------------------------------
> > +git push repository.com/Other_Path/
> > +-------------------------------
> > +
> > [...]
> > +/*
> > + *NEEDSWORK: Ugly because file://... is recognized as an url, and we
> > + *may want to compare it to local path without this scheme. Forcing
> > + *the user to put file:// before every local path would make the code
> > + *easier and avoid confusion with a distant repo like 'github.com'
> > + *which is not an url.
> > + */
> 
> Style: space after '*' (when there is text after), meaning:
> 	/*
> 	 * NEEDSWORK: Ugly because file://... is recognized [...]
> 	 * [...]
> 	 */
> 
> > +static int longest_prefix_size(const char* target_str,
> > +                                const struct string_list *list)
> 
> That might just be my mailer but this line is not properly lined up
> with the previous one (one space too much).
> It should be:
> static int longest_prefix_size(const char* target_str,
> 			       const struct string_list *list)
> 
> > [...]
> > +        for_each_string_list_item(curr_item, list) {
> > +                struct url_info curr_url;
> > +                const char *curr_str = curr_item->string;
> > +                skip_prefix(curr_str, "file://", &curr_str);
> > +                url_normalize(curr_str, &curr_url);
> > +                if (target_url.url &&
> > +                    curr_url.url &&
> 
> You can put target_url.url and curr_url.url on the same line.
> 
> > +                    target_url.scheme_len == curr_url.scheme_len &&
> > +
> > !strncmp(target_url.url,curr_url.url,curr_url.scheme_len))
> 
> Style: space after ','.
> 
> With those two things, the condition would look like this:
> 
> 		if (target_url.url && curr_url.url &&
> 		    target_url.scheme_len == curr_url.scheme_len &&
> 		    !strncmp(target_url.url, curr_url.url, curr_url.scheme_len))
> 
> > [...]
> > +        whitelist_size = longest_prefix_size(repo, whitelist);
> > +        blacklist_size = longest_prefix_size(repo, blacklist);
> > +
> > +        check_length_prefix(whitelist_size, blacklist_size, repo,
> > deny_message, default_policy);
> 
> This line is above 80 characters, so:
> 
> 	check_length_prefix(whitelist_size, blacklist_size, repo, deny_message,
> 			    default_policy);
> 
> > [...]
> > +test_expect_success 'setup' '
> > +        git init --bare blacklist/ &&
> > +        git init --bare whitelist/ &&
> > +        git init --bare blacklist/allow &&
> > +        test_commit commit &&
> > +        echo "fatal: Pushing to this remote using this protocol is
> > forbidden" > forbidden
> > +'
> > +
> > +test_expect_success 'basic case' '
> > +        git config --add remote.pushBlacklist http://blacklist.com &&
> 
> You use `git config` instead of `test_config`, meaning that the
> configuration you introduce will persist after the test.
> 
> It is not really a problem here since for the other tests you don't
> use `git config --add` so the configuration will be
> overwritten. However I still think you should use `test_config` to
> avoid causing trouble to potential future tests that would use
> `--add` and expect a clean state.
> 
> > [...]
> > +test_expect_success 'local path with file://' '
> > +        git config remote.pushBlacklist file://blacklist &&
> > +        test_must_fail git push blacklist HEAD 2> result &&
> > +        test_cmp result forbidden
> > +'
> 
> (you forgot a new line here)
> 
> > +test_expect_success 'only one scheme allowed' '
> > +        git config remote.pushDefaultPolicy deny &&
> > +        git config remote.pushWhitelist http://blacklist.com &&
> > +        test_must_fail git push https://blacklist.com HEAD 2> result &&
> > +        test_cmp result forbidden
> > +'
> > +
> > +test_expect_success 'denied repo in allowed repo' '
> 
> 'allowed repo in denied remote'? In any case the current title is
> misleading for me.
> 
> > +        git config remote.pushBlacklist blacklist &&
> > +        git config --add remote.pushWhitelist blacklist/allow &&
> > +        git push blacklist/allow HEAD
> > +'
> 
> Thanks,
> Rémi
> 

Hello Rémi, thanks you for your input ! I'll make the appropriate changes and send a new version as soon as i can !

Lars Schneider· Jun 15, 2016, 19:37 UTC · re: Antoine Queru · lore

Re: [RFC/PATCH] push: deny policy to prevent pushes to unwanted remotes.

Show 5 quoted lines
>> ...
>> 
> 
> Hello Rémi, thanks you for your input ! I'll make the appropriate changes 
> and send a new version as soon as i can !
Hi Antoine,

do you have an updated version already or is this the one I should look at? http://article.gmane.org/gmane.comp.version-control.git/296445

Thanks, Lars

Antoine Queru· Jun 20, 2016, 22:25 UTC · re: Lars Schneider · lore

Re: [RFC/PATCH] push: deny policy to prevent pushes to unwanted remotes.

Show 14 quoted lines
> 
> >> ...
> >> 
> > 
> > Hello Rémi, thanks you for your input ! I'll make the appropriate changes
> > and send a new version as soon as i can !
> 
> Hi Antoine,
> 
> do you have an updated version already or is this the one I should look at?
> http://article.gmane.org/gmane.comp.version-control.git/296445
> 
> Thanks,
> Lars

Hello Lars ! I'm actually in holidays, but I will try to send a new version by the end of the week, it's nearly done. The code has been refactored with what Matthieu and Remi told, so don't bother to read the last version.

However, in the last version, if we want to deny an website, including all schemes, we can blacklist the url without the scheme. For example, "pushBlacklist = github.com". By doing so, this remote is not an url anymore, and it can't be differenced with a local relative path. It's a problem because these two have a different treatement. The choice we made to solve this is to force the user to put the scheme "file://" before any local relative path. What do you think ?

Antoine
Matthieu Moy· Jun 21, 2016, 06:20 UTC · re: Antoine Queru · lore

Re: [RFC/PATCH] push: deny policy to prevent pushes to unwanted remotes.

Antoine Queru <antoine.queru@ensimag.grenoble-inp.fr> writes:
Show 8 quoted lines
> However, in the last version, if we want to deny an website,
> including all schemes, we can blacklist the url without the
> scheme. For example, "pushBlacklist = github.com". By doing so, this
> remote is not an url anymore, and it can't be differenced with a local
> relative path. It's a problem because these two have a different
> treatement. The choice we made to solve this is to force the user to
> put the scheme "file://" before any local relative path. What do you
> think ?

file:// URL can not be relative (well, you can invent a syntax where they are, but that would be weird).

I think you can just forbid relative path in whitelist/blacklist, hence consider that anything that is neither a full URL nor an absolute path is a protocol-less URL:

* http://github.com = github.com with HTTP protocol
* github.com = github.com with any protocol
* /path/to/file or file:///path/to/file = local path
-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
Lars Schneider· Jun 21, 2016, 07:45 UTC · re: Matthieu Moy · lore

Re: [RFC/PATCH] push: deny policy to prevent pushes to unwanted remotes.

Show 25 quoted lines
> On 21 Jun 2016, at 08:20, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:
> 
> Antoine Queru <antoine.queru@ensimag.grenoble-inp.fr> writes:
> 
>> However, in the last version, if we want to deny an website,
>> including all schemes, we can blacklist the url without the
>> scheme. For example, "pushBlacklist = github.com". By doing so, this
>> remote is not an url anymore, and it can't be differenced with a local
>> relative path. It's a problem because these two have a different
>> treatement. The choice we made to solve this is to force the user to
>> put the scheme "file://" before any local relative path. What do you
>> think ?
> 
> file:// URL can not be relative (well, you can invent a syntax where
> they are, but that would be weird).
> 
> I think you can just forbid relative path in whitelist/blacklist, hence
> consider that anything that is neither a full URL nor an absolute path
> is a protocol-less URL:
> 
> * http://github.com = github.com with HTTP protocol
> 
> * github.com = github.com with any protocol
> 
> * /path/to/file or file:///path/to/file = local path

I agree. Ignoring relative paths (and mentioning that in the docs) sounds reasonable to me. I don't think that would be a limitation as I can't think of a white/blacklist use case for relative URLs.

Thanks, Lars

Matthieu Moy· Jun 6, 2016, 15:18 UTC · re: Antoine Queru · lore

Re: [RFC/PATCH] push: deny policy to prevent pushes to unwanted remotes.

Antoine Queru <Antoine.Queru@grenoble-inp.org> writes:
> Currently, a user wanting to prevent accidental pushes to the wrong
> remote has to create a pre-push hook.  The feature

It's not clear what "The feature" refers to. Given the context, I read it as "pre-push hook", but I think this is not what you meant.

> User, password and port are not treated. Should it be ?

Please elaborate on "not treated". What happens if github.com is blacklisted by the user pushes to http://me@github.com ?

Show 18 quoted lines
>  Documentation/config.txt            |  17 ++++++
>  Documentation/urls-remotes.txt      |  55 ++++++++++++++++++
>  builtin/push.c                      | 110 ++++++++++++++++++++++++++++++++++--
>  t/t5544-push-whitelist-blacklist.sh |  51 +++++++++++++++++
>  4 files changed, 228 insertions(+), 5 deletions(-)
>  create mode 100755 t/t5544-push-whitelist-blacklist.sh
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 53f00db..7fe88f5 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -2517,6 +2517,23 @@ remote.pushDefault::
>  	`branch.<name>.remote` for all branches, and is overridden by
>  	`branch.<name>.pushRemote` for specific branches.
>  
> +remote.pushBlacklist::
> +	The list of remotes the user is forbidden to push to.
> +	See linkgit:git-push[1]

This is only true when remote.pushDefaultPolicy is "allow". It makes sense to say it explicitly here, if only to have a pointer to pushDefaultPolicy.

	The list of remotes the user is forbidden to push to when
	`remote.pushDefaultPolicy` is 'allow'
?

This doesn't document the "longest prefix wins", so the complete sentence could be

	The list of remotes the user is forbidden to push to when
	`remote.pushDefaultPolicy` is 'allow', or the list of exceptions
	to `remote.pushWhitelist` when it is 'deny'.
Or perhaps it's just overkill.
> +remote.pushWhitelist::
> +	The list of remotes the user is allowed to push to.
> +	See linkgit:git-push[1]
Likewise.
> +remote.pushDenyMessage::
> +	The message printed when pushing to a forbidden remote.
> +	Defaults to "Pushing to this remote has been forbidden".

Perhaps add something like "Please check your Git configuration files for details" to give a hint to the user about what's going on? (and give a chance to read the comment next to the configuration hopefully)

Show 13 quoted lines
>  remote.<name>.url::
>  	The URL of a remote repository.  See linkgit:git-fetch[1] or
>  	linkgit:git-push[1].
> diff --git a/Documentation/urls-remotes.txt b/Documentation/urls-remotes.txt
> index bd184cd..705914d 100644
> --- a/Documentation/urls-remotes.txt
> +++ b/Documentation/urls-remotes.txt
> @@ -89,6 +89,61 @@ git push uses:
>  	HEAD:refs/heads/<head>
>  ------------
>  
> +Denying pushes to the wrong remotes
> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2 '~' missing above.
Show 5 quoted lines
> +The simplest way to forbid pushes to a remote is to add its url in the
> +config file under the 'remote.pushBlacklist' variable.  A more
> +restrictive way is to change 'remote.pushDefaultPolicy' to 'deny',
> +thus denying pushes to every remote not whitelisted. You can then add
> +the right remotes under 'remote.pushWhitelist'.

A few meters from you are people working on guidelines for formatting of configuration variables. I guess they read this patch already but they're too angry to tell you ;-).

> +You can also configure more advanced acceptation/denial

A native may confirm/infirm, but it seems "acceptation" is essentially the word french people use to mean "acceptance".

> +Pushes like this  will be accepted:
> +-------------------------------
> +git push repository.com/Special_Path/*
> +-------------------------------

The '*' is misleading to me. As-is, it's a shell metacaracter that would be expanded locally by my shell. I guess you don't mean it like this, but rather "type anything here". Since you're already giving a concrete example, I'd make it completely concrete like

git push repository.com/Special_Path/subdirectory
Show 7 quoted lines
> +While this one for example will be denied:
> +-------------------------------
> +git push repository.com/Other_Path/
> +-------------------------------
> +
> +Remember to use '--add' with 'git config' to add more than one
> +'remote.pushBlacklist'.
I'd write:

`remote.pushBlacklist` and `remote.pushWhitelist` are multi-valued variables, hence you can more than one element using `git config --add`.

Now, a real question about the behavior: what happens when multiple patterns match on the same side (white or black), like

pushBlacklist = example.com pushBlacklist = example.com/a/b pushWhitelist = example.com/a

?
I think this deserves a test.
> +Specific schemes can also be denied. For example :
No space before :.
> +-------------------------------
> +git config --add remote.pushBlacklist http://repo.com
> +-------------------------------

Avoid using real domain names (or domain names that could become real) in examples. example.com is made precisely for this (rfc2606).

> +By doing so, only pushes to repo.com using 'http' will be denied,
`...` around `repo.com`.
> +static void check_length_prefix(const int whitelist,
> +				const int blacklist,

On the caller side, these are called *_size, and you dropped the _size part here. I find it much less readable.

Show 8 quoted lines
> +static void find_longest_prefix(const char *str, const char *pre, int *max)
> +{
> +	if (starts_with(str, pre)) {
> +		int tmp = strlen(pre);
> +		if (*max < tmp)
> +			*max = tmp;
> +	}
> +}
>From the name of the function, I'd have expected it to find the longest

common prefix of two strings or so, but it doesn't do that at all. Please find a better name.

Show 7 quoted lines
> +/*
> + *NEEDSWORK: Ugly because file://... is recognized as an url, and we
> + *may want to compare it to local path without this scheme. Forcing
> + *the user to put file:// before every local path would make the code
> + *easier and avoid confusion with a distant repo like 'github.com'
> + *which is not an url.
> + */

Not sure what you mean. In the text above, which instance refers to "the path the user is pushing to" and which one is "the path that appears in the whitelist/blacklist"?

I think you have to forbid relative local path here if you want to allow strings like "github.com", which can both be a relative path and a hostname.

If you do, then you can apply the rule
int is_local = 0;
if (is_absolute_path(curr_url))
	is_local = 1;
else if (skip_prefix(curr_url, "file://", &curr_url))
	is_local = 1

to know if the current item is local or not. OTOH, for target_str, the URL scheme has to be there for remote accesses so there's no ambiguity.

Show 13 quoted lines
> +static int longest_prefix_size(const char* target_str,
> +				const struct string_list *list)
> +{
> +	struct string_list_item *curr_item;
> +	struct url_info target_url;
> +	int max_size = 0;
> +	if (!list)
> +		return 0;
> +
> +	skip_prefix(target_str, "file://", &target_str);
> +	url_normalize(target_str, &target_url);
> +
> +	for_each_string_list_item(curr_item, list) {

curr_item just says it's the current item, but not of what. Perhaps curr_prefix or curr_pattern (not 100% convinced myself actually).

Show 8 quoted lines
> +		struct url_info curr_url;
> +		const char *curr_str = curr_item->string;
> +		skip_prefix(curr_str, "file://", &curr_str);
> +		url_normalize(curr_str, &curr_url);
> +		if (target_url.url &&
> +		    curr_url.url &&
> +		    target_url.scheme_len == curr_url.scheme_len &&
> +		    !strncmp(target_url.url,curr_url.url,curr_url.scheme_len))

This comparison is quite obscure if you don't have the whole picture in mind. It really deserves a comment (explanation and/or example).

Show 9 quoted lines
> +			find_longest_prefix(target_url.url + target_url.host_off,
> +					    curr_url.url + curr_url.host_off,
> +					    &max_size);
> +		else if (target_url.url && !curr_url.url)
> +			find_longest_prefix(target_url.url + target_url.host_off,
> +					    curr_str,
> +					    &max_size);
> +
> +		else if(!target_url.url && !curr_url.url)
Space after if.
> +			find_longest_prefix(target_str, curr_str, &max_size);

When no condition match, there's no "else" so you're doing nothing. This also deserves a comment like

/* else, either:
 * - !target_url.url && curr_url.url: ...
 * - target_url.url && curr_url.url but both use different schemes: ...
 */

Actually, I'd even write actual if/else branches for these cases, with an empty statement like

if (target_url.url && curr_url.url)
	if (target_url.scheme_len == curr_url.scheme_len &&
            !strncmp(target_url.url,curr_url.url,curr_url.scheme_len))
        	find_longest_prefix(...)
        else
        	; /* explanation of why you do nothing here */
...
else
	; /* likewise */
Show 27 quoted lines
>  static int do_push(const char *repo, int flags)
>  {
>  	int i, errs;
> @@ -404,15 +501,18 @@ static int do_push(const char *repo, int flags)
>  	url_nr = push_url_of_remote(remote, &url);
>  	if (url_nr) {
>  		for (i = 0; i < url_nr; i++) {
> -			struct transport *transport =
> -				transport_get(remote, url[i]);
> +			struct transport *transport;
> +			if (!(flags & TRANSPORT_PUSH_NO_HOOK))
> +				block_unauthorized_url(url[i]);
> +			transport = transport_get(remote, url[i]);
>  			if (push_with_options(transport, flags))
>  				errs++;
>  		}
>  	} else {
> -		struct transport *transport =
> -			transport_get(remote, NULL);
> -
> +		struct transport *transport;
> +		if (!(flags & TRANSPORT_PUSH_NO_HOOK))
> +			block_unauthorized_url(remote->url[0]);
> +		transport = transport_get(remote, NULL);
>  		if (push_with_options(transport, flags))
>  			errs++;
>  	}

I think it's time to introduce a helper function doing either the body of the "for" loop or the "else" branch: you're starting to get too much code duplication IMHO.

Show 50 quoted lines
> --- /dev/null
> +++ b/t/t5544-push-whitelist-blacklist.sh
> @@ -0,0 +1,51 @@
> +#!/bin/sh
> +
> +test_description='blacklist for push'
> +
> +. ./test-lib.sh
> +
> +test_expect_success 'setup' '
> +	git init --bare blacklist/ &&
> +	git init --bare whitelist/ &&
> +	git init --bare blacklist/allow &&
> +	test_commit commit &&
> +	echo "fatal: Pushing to this remote using this protocol is forbidden" > forbidden
> +'
> +
> +test_expect_success 'basic case' '
> +	git config --add remote.pushBlacklist http://blacklist.com &&
> +	test_must_fail git push http://blacklist.com HEAD 2> result &&
> +	test_cmp result forbidden
> +'
> +
> +test_expect_success 'no scheme and no path' '
> +	git config remote.pushBlacklist blacklist.com &&
> +	test_must_fail git push http://blacklist.com/foo HEAD 2> result &&
> +	test_cmp result forbidden
> +'
> +
> +test_expect_success 'local path' '
> +	git config remote.pushBlacklist blacklist &&
> +	test_must_fail git push blacklist HEAD 2> result &&
> +	test_cmp result forbidden
> +'
> +
> +test_expect_success 'local path with file://' '
> +	git config remote.pushBlacklist file://blacklist &&
> +	test_must_fail git push blacklist HEAD 2> result &&
> +	test_cmp result forbidden
> +'
> +test_expect_success 'only one scheme allowed' '
> +	git config remote.pushDefaultPolicy deny &&
> +	git config remote.pushWhitelist http://blacklist.com &&
> +	test_must_fail git push https://blacklist.com HEAD 2> result &&
> +	test_cmp result forbidden
> +'
> +
> +test_expect_success 'denied repo in allowed repo' '
> +	git config remote.pushBlacklist blacklist &&
> +	git config --add remote.pushWhitelist blacklist/allow &&

This is not self-contained as it relies on the configuration set by the previous test. Actually, I think you don't want the --add here.

I think
git push -c remote.pushBlacklist=blacklist \
         -c remote.pushWhitelist=http://blacklist.com \
         -c remote.pushWhitelist=blacklist/allow \
	blacklist/allow HEAD

would be even clearer: there's a bit more syntactical disturbance (-c, \), but the whole relevant config is under the reviwer's eyes at the right time.

> +	git push blacklist/allow HEAD

This is the only positive case, and many others are missing. For example "push to https://example.com" is allowed even when "http://example.com" is blacklisted.

It's harder to test without doing an actual remote push, but I think you can still manage using fake URL scheme like foo://example.com and check that git-remote-foo is actually called.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
Junio C Hamano· Jun 6, 2016, 16:13 UTC · re: Matthieu Moy · lore

Re: [RFC/PATCH] push: deny policy to prevent pushes to unwanted remotes.

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
Show 21 quoted lines
> Antoine Queru <Antoine.Queru@grenoble-inp.org> writes:
>
>> Currently, a user wanting to prevent accidental pushes to the wrong
>> remote has to create a pre-push hook.  The feature
>
> It's not clear what "The feature" refers to. Given the context, I read
> it as "pre-push hook", but I think this is not what you meant.
>
>> User, password and port are not treated. Should it be ?
>
> Please elaborate on "not treated". What happens if github.com is
> blacklisted by the user pushes to http://me@github.com ?
> ...
>> +remote.pushBlacklist::
>> +	The list of remotes the user is forbidden to push to.
>> +	See linkgit:git-push[1]
>
> This is only true when remote.pushDefaultPolicy is "allow". It makes
> sense to say it explicitly here, if only to have a pointer to
> pushDefaultPolicy.
> ...

All good comments and suggestions, but I am bothered a lot more by this being called "Policy" and uses words like "Prevent" and "Forbidden" as if there is a real enforcement mechanism, when in reality what is proposed is only advisory.

It is a good design balance to leave such a client-side mechanism advisory and go no further than that; a truly draconian client side mechanism has no value, as the Git protocol is open, well documented and you can write and use a more liberal client to work around such a mechanism. I do not think selling such an advisory mechanism as if it is a policy enforcement feature.

"user wanting to avoid accidental pushes" the log message mentions sets the expectation at the right level, I would think. I do not think this is about a "Policy enforcement"; where its value lies in is accident prevention (and possibly self discipline enforcement).

Matthieu Moy· Jun 6, 2016, 16:37 UTC · re: Matthieu Moy · lore

Re: [RFC/PATCH] push: deny policy to prevent pushes to unwanted remotes.

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>> + *easier and avoid confusion with a distant repo like 'github.com'
Forgotten nit in previous message: s/distant/remote/.
-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

← back to recent threads