From dee8a1455c8ad443ef59e0d5b7c168886e50b9ea Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Tue, 19 Jul 2022 18:32:13 +0000 Subject: [PATCH 1/5] daemon: clarify directory arguments The undecorated arguments to the 'git-daemon' command provide a list of directories. When at least one directory is specified, then 'git-daemon' only serves requests that are within that directory list. The boolean '--strict-paths' option makes the list more explicit in that subdirectories are no longer included. The existing documentation and error messages around this directory list refer to it and its behavior as a "whitelist". The word "whitelist" has cultural implications that are not inclusive. Thankfully, it is not difficult to reword and avoid its use. In the process, we can define the purpose of this directory list directly. In Documentation/git-daemon.txt, rewrite the OPTIONS section around the '' option. Add additional clarity to the other options that refer to these directories. Some error messages can also be improved in daemon.c. The '--strict-paths' option requires '' arguments, so refer to that section of the documentation directly. A logerror() call points out that a requested directory is not in the specified directory list. We can use "list" here without any loss of information. Signed-off-by: Derrick Stolee Signed-off-by: Junio C Hamano --- Documentation/git-daemon.txt | 21 +++++++++++---------- daemon.c | 8 ++++---- 2 files changed, 15 insertions(+), 14 deletions(-) diff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt index fdc28c041c..236df516c7 100644 --- a/Documentation/git-daemon.txt +++ b/Documentation/git-daemon.txt @@ -32,8 +32,8 @@ that service if it is enabled. It verifies that the directory has the magic file "git-daemon-export-ok", and it will refuse to export any Git directory that hasn't explicitly been marked for export this way (unless the `--export-all` parameter is specified). If you -pass some directory paths as 'git daemon' arguments, you can further restrict -the offers to a whitelist comprising of those. +pass some directory paths as 'git daemon' arguments, the offers are limited to +repositories within those directories. By default, only `upload-pack` service is enabled, which serves 'git fetch-pack' and 'git ls-remote' clients, which are invoked @@ -50,7 +50,7 @@ OPTIONS Match paths exactly (i.e. don't allow "/foo/repo" when the real path is "/foo/repo.git" or "/foo/repo/.git") and don't do user-relative paths. 'git daemon' will refuse to start when this option is enabled and no - whitelist is specified. + directory arguments are provided. --base-path=:: Remap all the path requests as relative to the given path. @@ -73,7 +73,7 @@ OPTIONS %IP for the server's IP address, %P for the port number, and %D for the absolute path of the named repository. After interpolation, the path is validated against the directory - whitelist. + list. --export-all:: Allow pulling from all directories that look like Git repositories @@ -218,9 +218,11 @@ standard output to be sent to the requestor as an error message when it declines the service. :: - A directory to add to the whitelist of allowed directories. Unless - --strict-paths is specified this will also include subdirectories - of each named directory. + The remaining arguments provide a list of directories. If any + directories are specified, then the `git-daemon` process will + serve a requested directory only if it is contained in one of + these directories. If `--strict-paths` is specified, then the + requested directory must match one of these directories exactly. SERVICES -------- @@ -264,9 +266,8 @@ git 9418/tcp # Git Version Control System 'git daemon' as inetd server:: To set up 'git daemon' as an inetd service that handles any - repository under the whitelisted set of directories, /pub/foo - and /pub/bar, place an entry like the following into - /etc/inetd all on one line: + repository within `/pub/foo` or `/pub/bar`, place an entry like + the following into `/etc/inetd` all on one line: + ------------------------------------------------ git stream tcp nowait nobody /usr/bin/git diff --git a/daemon.c b/daemon.c index 58f1077885..0ae7d12b5c 100644 --- a/daemon.c +++ b/daemon.c @@ -279,7 +279,7 @@ static const char *path_ok(const char *directory, struct hostinfo *hi) /* The validation is done on the paths after enter_repo * appends optional {.git,.git/.git} and friends, but * it does not use getcwd(). So if your /pub is - * a symlink to /mnt/pub, you can whitelist /pub and + * a symlink to /mnt/pub, you can include /pub and * do not have to say /mnt/pub. * Do not say /pub/. */ @@ -298,7 +298,7 @@ static const char *path_ok(const char *directory, struct hostinfo *hi) return path; } - logerror("'%s': not in whitelist", path); + logerror("'%s': not in directory list", path); return NULL; /* Fallthrough. Deny by default */ } @@ -403,7 +403,7 @@ static int run_service(const char *dir, struct daemon_service *service, * a "git-daemon-export-ok" flag that says that the other side * is ok with us doing this. * - * path_ok() uses enter_repo() and does whitelist checking. + * path_ok() uses enter_repo() and checks for included directories. * We only need to make sure the repository is exported. */ @@ -1444,7 +1444,7 @@ int cmd_main(int argc, const char **argv) cred = prepare_credentials(user_name, group_name); if (strict_paths && (!ok_paths || !*ok_paths)) - die("option --strict-paths requires a whitelist"); + die("option --strict-paths requires '' arguments"); if (base_path && !is_directory(base_path)) die("base-path '%s' does not exist or is not a directory", From acc5e287f2cce46402abd614b53de5f096ee349f Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Tue, 19 Jul 2022 18:32:14 +0000 Subject: [PATCH 2/5] git-cvsserver: clarify directory list The documentation and error messages for git-cvsserver include some references to a "whitelist" that is not otherwise included in the documentation. When different parts of the documentation do not use common language, this can lead to confusion as to how things are meant to operate. Further, the word "whitelist" has cultural implications that make its use non-inclusive. Thankfully, we can remove it while increasing clarity. Update Documentation/git-cvsserver.txt in a similar way to the previous change to Documentation/git-daemon.txt. The optional '...' list can specify a list of allowed directories. We refer to that list directly inside of the documentation for the GIT_CVSSERVER_ROOT environment variable. While modifying this documentation, update the environment variables to use a list format. We use the modern way of tabbing the description of each variable in this section. We do _not_ update the description of '...' to use tabs this way since the rest of the items in the OPTIONS list do not use this modern formatting. A single error message in the actual git-cvsserver.perl code refers to the whitelist during argument parsing. Instead, refer to the directory list that has been clarified in the documentation. Signed-off-by: Derrick Stolee Signed-off-by: Junio C Hamano --- Documentation/git-cvsserver.txt | 19 ++++++++++--------- git-cvsserver.perl | 2 +- t/t9400-git-cvsserver-server.sh | 2 +- 3 files changed, 12 insertions(+), 11 deletions(-) diff --git a/Documentation/git-cvsserver.txt b/Documentation/git-cvsserver.txt index 4dc57ed254..53f111bc0a 100644 --- a/Documentation/git-cvsserver.txt +++ b/Documentation/git-cvsserver.txt @@ -63,11 +63,10 @@ Print version information and exit Print usage information and exit :: -You can specify a list of allowed directories. If no directories -are given, all are allowed. This is an additional restriction, gitcvs -access still needs to be enabled by the `gitcvs.enabled` config option -unless `--export-all` was given, too. - +The remaining arguments provide a list of directories. If no directories +are given, then all are allowed. Repositories within these directories +still require the `gitcvs.enabled` config option, unless `--export-all` +is specified. LIMITATIONS ----------- @@ -311,11 +310,13 @@ ENVIRONMENT These variables obviate the need for command-line options in some circumstances, allowing easier restricted usage through git-shell. -GIT_CVSSERVER_BASE_PATH takes the place of the argument to --base-path. +GIT_CVSSERVER_BASE_PATH:: + This variable replaces the argument to --base-path. -GIT_CVSSERVER_ROOT specifies a single-directory whitelist. The -repository must still be configured to allow access through -git-cvsserver, as described above. +GIT_CVSSERVER_ROOT:: + This variable specifies a single directory, replacing the + `...` argument list. The repository still requires the + `gitcvs.enabled` config option, unless `--export-all` is specified. When these environment variables are set, the corresponding command-line arguments may not be used. diff --git a/git-cvsserver.perl b/git-cvsserver.perl index 4c8118010a..7b757360e2 100755 --- a/git-cvsserver.perl +++ b/git-cvsserver.perl @@ -152,7 +152,7 @@ $state->{allowed_roots} = [ @ARGV ]; # don't export the whole system unless the users requests it if ($state->{'export-all'} && !@{$state->{allowed_roots}}) { - die "--export-all can only be used together with an explicit whitelist\n"; + die "--export-all can only be used together with an explicit '...' list\n"; } # Environment handling for running under git-shell diff --git a/t/t9400-git-cvsserver-server.sh b/t/t9400-git-cvsserver-server.sh index 210ddf09e3..379b19f2f8 100755 --- a/t/t9400-git-cvsserver-server.sh +++ b/t/t9400-git-cvsserver-server.sh @@ -221,7 +221,7 @@ test_expect_success 'req_Root (export-all)' \ 'cat request-anonymous | git-cvsserver --export-all pserver "$WORKDIR" >log 2>&1 && sed -ne \$p log | grep "^I LOVE YOU\$"' -test_expect_success 'req_Root failure (export-all w/o whitelist)' \ +test_expect_success 'req_Root failure (export-all w/o directory list)' \ '! (cat request-anonymous | git-cvsserver --export-all pserver >log 2>&1 || false)' test_expect_success 'req_Root (everything together)' \ From 559c2c3d2a34c4d8c24265e118175f55771112a2 Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Tue, 19 Jul 2022 18:32:15 +0000 Subject: [PATCH 3/5] git.txt: remove redundant language The documentation for GIT_ALLOW_PROTOCOL has a sentence that adds no value, since it repeats the meaning from the previous sentence (twice!). The word "whitelist" has cultural implications that are not inclusive, which brought attention to this sentence. Helped-by: Jeff King Helped-by: Junio C Hamano Signed-off-by: Derrick Stolee Signed-off-by: Junio C Hamano --- Documentation/git.txt | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/Documentation/git.txt b/Documentation/git.txt index 302607a496..47a6095ff4 100644 --- a/Documentation/git.txt +++ b/Documentation/git.txt @@ -885,9 +885,7 @@ for full details. If set to a colon-separated list of protocols, behave as if `protocol.allow` is set to `never`, and each of the listed protocols has `protocol..allow` set to `always` - (overriding any existing configuration). In other words, any - protocol not mentioned will be disallowed (i.e., this is a - whitelist, not a blacklist). See the description of + (overriding any existing configuration). See the description of `protocol.allow` in linkgit:git-config[1] for more details. `GIT_PROTOCOL_FROM_USER`:: From 0011f94a4f05d5311a4f3223da8caf3bfbb9e293 Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Tue, 19 Jul 2022 18:32:16 +0000 Subject: [PATCH 4/5] t: avoid "whitelist" The word "whitelist" has cultural implications that are not inclusive. Thankfully, it is not difficult to reword and avoid its use. Focus on changes in the test scripts, since most of the changes are in comments and test names. The renamed test_allow_var helper is only used once inside the widely-used test_proto helper. Signed-off-by: Derrick Stolee Signed-off-by: Junio C Hamano --- t/lib-proto-disable.sh | 6 +++--- t/t5812-proto-disable-http.sh | 2 +- t/t5815-submodule-protos.sh | 4 ++-- t/test-lib-functions.sh | 3 +-- 4 files changed, 7 insertions(+), 8 deletions(-) diff --git a/t/lib-proto-disable.sh b/t/lib-proto-disable.sh index 83babe57d9..890622be81 100644 --- a/t/lib-proto-disable.sh +++ b/t/lib-proto-disable.sh @@ -1,7 +1,7 @@ # Test routines for checking protocol disabling. -# Test clone/fetch/push with GIT_ALLOW_PROTOCOL whitelist -test_whitelist () { +# Test clone/fetch/push with GIT_ALLOW_PROTOCOL environment variable +test_allow_var () { desc=$1 proto=$2 url=$3 @@ -183,7 +183,7 @@ test_config () { # $2 - machine-readable name of the protocol # $3 - the URL to try cloning test_proto () { - test_whitelist "$@" + test_allow_var "$@" test_config "$@" } diff --git a/t/t5812-proto-disable-http.sh b/t/t5812-proto-disable-http.sh index af8772fada..d8da5f58d1 100755 --- a/t/t5812-proto-disable-http.sh +++ b/t/t5812-proto-disable-http.sh @@ -16,7 +16,7 @@ test_expect_success 'create git-accessible repo' ' test_proto "smart http" http "$HTTPD_URL/smart/repo.git" -test_expect_success 'curl redirects respect whitelist' ' +test_expect_success 'http(s) transport respects GIT_ALLOW_PROTOCOL' ' test_must_fail env GIT_ALLOW_PROTOCOL=http:https \ GIT_SMART_HTTP=0 \ git clone "$HTTPD_URL/ftp-redir/repo.git" 2>stderr && diff --git a/t/t5815-submodule-protos.sh b/t/t5815-submodule-protos.sh index 06f55a1b8a..4d5956cc18 100755 --- a/t/t5815-submodule-protos.sh +++ b/t/t5815-submodule-protos.sh @@ -1,6 +1,6 @@ #!/bin/sh -test_description='test protocol whitelisting with submodules' +test_description='test protocol filtering with submodules' . ./test-lib.sh . "$TEST_DIRECTORY"/lib-proto-disable.sh @@ -36,7 +36,7 @@ test_expect_success 'update of ext not allowed' ' test_must_fail git -C dst submodule update ext-module ' -test_expect_success 'user can override whitelist' ' +test_expect_success 'user can filter protocols with GIT_ALLOW_PROTOCOL' ' GIT_ALLOW_PROTOCOL=ext git -C dst submodule update ext-module ' diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh index 6da7273f1d..8c44856eae 100644 --- a/t/test-lib-functions.sh +++ b/t/test-lib-functions.sh @@ -651,8 +651,7 @@ test_set_prereq () { # test_unset_prereq() !*) ;; - # (Temporary?) whitelist of things we can't easily - # pretend not to support + # List of things we can't easily pretend to not support SYMLINKS) ;; # Inspecting whether GIT_TEST_FAIL_PREREQS is on From f5adaa5cc31006ad6a2a62d5be008e3453a365e4 Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Tue, 19 Jul 2022 18:32:17 +0000 Subject: [PATCH 5/5] transport.c: avoid "whitelist" The word "whitelist" has cultural implications that are not inclusive. Thankfully, it is not difficult to reword and avoid its use. The GIT_ALLOW_PROTOCOL environment variable was referred to as a "whitelist", but the word "allow" is already part of the variable. Replace "whitelist" with "allow_list" in these cases to demonstrate that we are processing a list of allowed protocols. Signed-off-by: Derrick Stolee Signed-off-by: Junio C Hamano --- transport.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/transport.c b/transport.c index 52db7a3cb0..b51e991e44 100644 --- a/transport.c +++ b/transport.c @@ -940,7 +940,7 @@ static int external_specification_len(const char *url) return strchr(url, ':') - url; } -static const struct string_list *protocol_whitelist(void) +static const struct string_list *protocol_allow_list(void) { static int enabled = -1; static struct string_list allowed = STRING_LIST_INIT_DUP; @@ -1020,9 +1020,9 @@ static enum protocol_allow_config get_protocol_config(const char *type) int is_transport_allowed(const char *type, int from_user) { - const struct string_list *whitelist = protocol_whitelist(); - if (whitelist) - return string_list_has_string(whitelist, type); + const struct string_list *allow_list = protocol_allow_list(); + if (allow_list) + return string_list_has_string(allow_list, type); switch (get_protocol_config(type)) { case PROTOCOL_ALLOW_ALWAYS: