* [RFC/PATCH] push: deny policy to prevent pushes to unwanted remotes.
@ 2016-06-04 14:51 Antoine Queru
2016-06-06 11:39 ` Remi Galan Alfonso
2016-06-06 15:18 ` Matthieu Moy
0 siblings, 2 replies; 10+ messages in thread
From: Antoine Queru @ 2016-06-04 14:51 UTC (permalink / raw)
To: git
Cc: william.duclot, simon.rabourg, francois.beutin, larsxschneider,
rsbecker, aaron, Matthieu.Moy, peff
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
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [RFC/PATCH] push: deny policy to prevent pushes to unwanted remotes.
2016-06-04 14:51 [RFC/PATCH] push: deny policy to prevent pushes to unwanted remotes Antoine Queru
@ 2016-06-06 11:39 ` Remi Galan Alfonso
2016-06-06 13:43 ` Antoine Queru
2016-06-06 15:18 ` Matthieu Moy
1 sibling, 1 reply; 10+ messages in thread
From: Remi Galan Alfonso @ 2016-06-06 11:39 UTC (permalink / raw)
To: Antoine Queru
Cc: git, william duclot, simon rabourg, francois beutin,
larsxschneider, rsbecker, aaron, Matthieu Moy, peff
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC/PATCH] push: deny policy to prevent pushes to unwanted remotes.
2016-06-06 11:39 ` Remi Galan Alfonso
@ 2016-06-06 13:43 ` Antoine Queru
2016-06-15 19:37 ` Lars Schneider
0 siblings, 1 reply; 10+ messages in thread
From: Antoine Queru @ 2016-06-06 13:43 UTC (permalink / raw)
To: Remi Galan Alfonso
Cc: Antoine Queru, git, william duclot, simon rabourg,
francois beutin, larsxschneider, rsbecker, aaron, Matthieu Moy,
peff
----- Mail original -----
> 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 !
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC/PATCH] push: deny policy to prevent pushes to unwanted remotes.
2016-06-04 14:51 [RFC/PATCH] push: deny policy to prevent pushes to unwanted remotes Antoine Queru
2016-06-06 11:39 ` Remi Galan Alfonso
@ 2016-06-06 15:18 ` Matthieu Moy
2016-06-06 16:13 ` Junio C Hamano
2016-06-06 16:37 ` Matthieu Moy
1 sibling, 2 replies; 10+ messages in thread
From: Matthieu Moy @ 2016-06-06 15:18 UTC (permalink / raw)
To: Antoine Queru
Cc: git, william.duclot, simon.rabourg, francois.beutin,
larsxschneider, rsbecker, aaron, peff
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 ?
> 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)
> 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.
> +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
> +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.
> +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.
> +/*
> + *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.
> +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).
> + 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).
> + 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 */
> 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.
> --- /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/
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC/PATCH] push: deny policy to prevent pushes to unwanted remotes.
2016-06-06 15:18 ` Matthieu Moy
@ 2016-06-06 16:13 ` Junio C Hamano
2016-06-06 16:37 ` Matthieu Moy
1 sibling, 0 replies; 10+ messages in thread
From: Junio C Hamano @ 2016-06-06 16:13 UTC (permalink / raw)
To: Matthieu Moy
Cc: Antoine Queru, git, william.duclot, simon.rabourg,
francois.beutin, larsxschneider, rsbecker, aaron, peff
Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
> 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).
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC/PATCH] push: deny policy to prevent pushes to unwanted remotes.
2016-06-06 15:18 ` Matthieu Moy
2016-06-06 16:13 ` Junio C Hamano
@ 2016-06-06 16:37 ` Matthieu Moy
1 sibling, 0 replies; 10+ messages in thread
From: Matthieu Moy @ 2016-06-06 16:37 UTC (permalink / raw)
To: Antoine Queru
Cc: git, william.duclot, simon.rabourg, francois.beutin,
larsxschneider, rsbecker, aaron, peff
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/
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC/PATCH] push: deny policy to prevent pushes to unwanted remotes.
2016-06-06 13:43 ` Antoine Queru
@ 2016-06-15 19:37 ` Lars Schneider
2016-06-20 22:25 ` Antoine Queru
0 siblings, 1 reply; 10+ messages in thread
From: Lars Schneider @ 2016-06-15 19:37 UTC (permalink / raw)
To: Antoine Queru
Cc: Remi Galan Alfonso, Antoine Queru, git, william duclot,
simon rabourg, francois beutin, rsbecker, aaron, Matthieu Moy,
peff
>> ...
>>
>
> 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC/PATCH] push: deny policy to prevent pushes to unwanted remotes.
2016-06-15 19:37 ` Lars Schneider
@ 2016-06-20 22:25 ` Antoine Queru
2016-06-21 6:20 ` Matthieu Moy
0 siblings, 1 reply; 10+ messages in thread
From: Antoine Queru @ 2016-06-20 22:25 UTC (permalink / raw)
To: Lars Schneider
Cc: Remi Galan Alfonso, Antoine Queru, git, william duclot,
simon rabourg, francois beutin, rsbecker, aaron, Matthieu Moy,
peff
>
> >> ...
> >>
> >
> > 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC/PATCH] push: deny policy to prevent pushes to unwanted remotes.
2016-06-20 22:25 ` Antoine Queru
@ 2016-06-21 6:20 ` Matthieu Moy
2016-06-21 7:45 ` Lars Schneider
0 siblings, 1 reply; 10+ messages in thread
From: Matthieu Moy @ 2016-06-21 6:20 UTC (permalink / raw)
To: Antoine Queru
Cc: Lars Schneider, Remi Galan Alfonso, Antoine Queru, git,
william duclot, simon rabourg, francois beutin, rsbecker, aaron,
peff
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
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC/PATCH] push: deny policy to prevent pushes to unwanted remotes.
2016-06-21 6:20 ` Matthieu Moy
@ 2016-06-21 7:45 ` Lars Schneider
0 siblings, 0 replies; 10+ messages in thread
From: Lars Schneider @ 2016-06-21 7:45 UTC (permalink / raw)
To: Matthieu Moy
Cc: Antoine Queru, Remi Galan Alfonso, Antoine Queru, git,
william duclot, simon rabourg, francois beutin, rsbecker, aaron,
peff
> 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
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2016-06-21 7:46 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-06-04 14:51 [RFC/PATCH] push: deny policy to prevent pushes to unwanted remotes Antoine Queru
2016-06-06 11:39 ` Remi Galan Alfonso
2016-06-06 13:43 ` Antoine Queru
2016-06-15 19:37 ` Lars Schneider
2016-06-20 22:25 ` Antoine Queru
2016-06-21 6:20 ` Matthieu Moy
2016-06-21 7:45 ` Lars Schneider
2016-06-06 15:18 ` Matthieu Moy
2016-06-06 16:13 ` Junio C Hamano
2016-06-06 16:37 ` Matthieu Moy
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).