From 4a169da280aa6d22bdf0cf5baea65f47bd363a3a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Wed, 15 Jun 2022 12:44:11 +0200 Subject: [PATCH 1/2] fetch doc: note "pushurl" caveat about "credentialsInUrl", elaborate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Amend the documentation and release notes entry for the "fetch.credentialsInUrl" feature added in 6dcbdc0d661 (remote: create fetch.credentialsInUrl config, 2022-06-06), it currently doesn't detect passwords in `remote..pushurl` configuration. We shouldn't lull users into a false sense of security, so we need to mention that prominently. This also elaborates and clarifies the "exposes the password in multiple ways" part of the documentation. As noted in [1] a user unfamiliar with git's implementation won't know what to make of that scary claim, e.g. git hypothetically have novel git-specific ways of exposing configured credentials. The reality is that this configuration is intended as an aid for users who can't fully trust their OS's or system's security model, so lets say that's what this is intended for, and mention the most common ways passwords stored in configuration might inadvertently get exposed. 1. https://lore.kernel.org/git/220524.86ilpuvcqh.gmgdl@evledraar.gmail.com/ Signed-off-by: Ævar Arnfjörð Bjarmason Acked-by: Derrick Stolee Signed-off-by: Junio C Hamano --- Documentation/RelNotes/2.37.0.txt | 4 +++- Documentation/config/fetch.txt | 34 +++++++++++++++++++++++++------ 2 files changed, 31 insertions(+), 7 deletions(-) diff --git a/Documentation/RelNotes/2.37.0.txt b/Documentation/RelNotes/2.37.0.txt index 8f1ff3a596..39ca3606de 100644 --- a/Documentation/RelNotes/2.37.0.txt +++ b/Documentation/RelNotes/2.37.0.txt @@ -55,7 +55,9 @@ UI, Workflows & Features * Update the doctype written in gitweb output to xhtml5. * The "fetch.credentialsInUrl" configuration variable controls what - happens when a URL with embedded login credential is used. + happens when a URL with embedded login credential is used on either + "fetch" or "push". Credentials are currently only detected in + `remote..url` config, not `remote..pushurl`. Performance, Internal Implementation, Development Support etc. diff --git a/Documentation/config/fetch.txt b/Documentation/config/fetch.txt index 0db7fe85bb..827961059f 100644 --- a/Documentation/config/fetch.txt +++ b/Documentation/config/fetch.txt @@ -98,12 +98,34 @@ fetch.writeCommitGraph:: `git push -f`, and `git log --graph`. Defaults to false. fetch.credentialsInUrl:: - A URL can contain plaintext credentials in the form - `://:@/`. Using such URLs - is not recommended as it exposes the password in multiple ways, - including Git storing the URL as plaintext in the repository config. - The `fetch.credentialsInUrl` option provides instruction for how Git - should react to seeing such a URL, with these values: + A configured URL can contain plaintext credentials in the form + `://:@/`. You may want + to warn or forbid the use of such configuration (in favor of + using linkgit:git-credential[1]). ++ +Note that this is currently limited to detecting credentials in +`remote..url` configuration, it won't detect credentials in +`remote..pushurl` configuration. ++ +You might want to enable this to prevent inadvertent credentials +exposure, e.g. because: ++ +* The OS or system where you're running git may not provide way way or + otherwise allow you to configure the permissions of the + configuration file where the username and/or password are stored. +* Even if it does, having such data stored "at rest" might expose you + in other ways, e.g. a backup process might copy the data to another + system. +* The git programs will pass the full URL to one another as arguments + on the command-line, meaning the credentials will be exposed to oher + users on OS's or systems that allow other users to see the full + process list of other users. On linux the "hidepid" setting + documented in procfs(5) allows for configuring this behavior. ++ +If such concerns don't apply to you then you probably don't need to be +concerned about credentials exposure due to storing that sensitive +data in git's configuration files. If you do want to use this, set +`fetch.credentialsInUrl` to one of these values: + * `allow` (default): Git will proceed with its activity without warning. * `warn`: Git will write a warning message to `stderr` when parsing a URL From 7281c196b1166f1c00df33948c67b0ef81ba63a9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Wed, 15 Jun 2022 12:44:12 +0200 Subject: [PATCH 2/2] transfer doc: move fetch.credentialsInUrl to "transfer" config namespace MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rename the "fetch.credentialsInUrl" configuration variable introduced in 6dcbdc0d661 (remote: create fetch.credentialsInUrl config, 2022-06-06) to "transfer". There are existing exceptions, but generally speaking the "." configuration should only apply to command described in the "namespace" (and its sub-commands, so e.g. "clone.*" or "fetch.*" might also configure "git-remote-https"). But in the case of "fetch.credentialsInUrl" we've got a configuration variable that configures the behavior of all of "clone", "push" and "fetch", someone adjusting "fetch.*" configuration won't expect to have the behavior of "git push" altered, especially as we have the pre-existing "{transfer,fetch,receive}.fsckObjects", which configures different parts of the transfer dialog. So let's move this configuration variable to the "transfer" namespace before it's exposed in a release. We could add all of "{transfer,fetch,pull}.credentialsInUrl" at some other time, but once we have "fetch" configure "pull" such an arrangement would would be a confusing mess, as we'd at least need to have "fetch" configure "push" (but not the other way around), or change existing behavior. Signed-off-by: Ævar Arnfjörð Bjarmason Acked-by: Derrick Stolee Signed-off-by: Junio C Hamano --- Documentation/RelNotes/2.37.0.txt | 2 +- Documentation/config/fetch.txt | 36 ----------------------------- Documentation/config/transfer.txt | 38 +++++++++++++++++++++++++++++++ remote.c | 4 ++-- t/t5516-fetch-push.sh | 14 ++++++------ t/t5601-clone.sh | 10 ++++---- 6 files changed, 53 insertions(+), 51 deletions(-) diff --git a/Documentation/RelNotes/2.37.0.txt b/Documentation/RelNotes/2.37.0.txt index 39ca3606de..9902a74f34 100644 --- a/Documentation/RelNotes/2.37.0.txt +++ b/Documentation/RelNotes/2.37.0.txt @@ -54,7 +54,7 @@ UI, Workflows & Features * Update the doctype written in gitweb output to xhtml5. - * The "fetch.credentialsInUrl" configuration variable controls what + * The "transfer.credentialsInUrl" configuration variable controls what happens when a URL with embedded login credential is used on either "fetch" or "push". Credentials are currently only detected in `remote..url` config, not `remote..pushurl`. diff --git a/Documentation/config/fetch.txt b/Documentation/config/fetch.txt index 827961059f..cd65d236b4 100644 --- a/Documentation/config/fetch.txt +++ b/Documentation/config/fetch.txt @@ -96,39 +96,3 @@ fetch.writeCommitGraph:: merge and the write may take longer. Having an updated commit-graph file helps performance of many Git commands, including `git merge-base`, `git push -f`, and `git log --graph`. Defaults to false. - -fetch.credentialsInUrl:: - A configured URL can contain plaintext credentials in the form - `://:@/`. You may want - to warn or forbid the use of such configuration (in favor of - using linkgit:git-credential[1]). -+ -Note that this is currently limited to detecting credentials in -`remote..url` configuration, it won't detect credentials in -`remote..pushurl` configuration. -+ -You might want to enable this to prevent inadvertent credentials -exposure, e.g. because: -+ -* The OS or system where you're running git may not provide way way or - otherwise allow you to configure the permissions of the - configuration file where the username and/or password are stored. -* Even if it does, having such data stored "at rest" might expose you - in other ways, e.g. a backup process might copy the data to another - system. -* The git programs will pass the full URL to one another as arguments - on the command-line, meaning the credentials will be exposed to oher - users on OS's or systems that allow other users to see the full - process list of other users. On linux the "hidepid" setting - documented in procfs(5) allows for configuring this behavior. -+ -If such concerns don't apply to you then you probably don't need to be -concerned about credentials exposure due to storing that sensitive -data in git's configuration files. If you do want to use this, set -`fetch.credentialsInUrl` to one of these values: -+ -* `allow` (default): Git will proceed with its activity without warning. -* `warn`: Git will write a warning message to `stderr` when parsing a URL - with a plaintext credential. -* `die`: Git will write a failure message to `stderr` when parsing a URL - with a plaintext credential. diff --git a/Documentation/config/transfer.txt b/Documentation/config/transfer.txt index b49429eb4d..b4475c0690 100644 --- a/Documentation/config/transfer.txt +++ b/Documentation/config/transfer.txt @@ -1,3 +1,41 @@ +transfer.credentialsInUrl:: + A configured URL can contain plaintext credentials in the form + `://:@/`. You may want + to warn or forbid the use of such configuration (in favor of + using linkgit:git-credential[1]). This will be used on + linkgit:git-clone[1], linkgit:git-fetch[1], linkgit:git-push[1], + and any other direct use of the configured URL. ++ +Note that this is currently limited to detecting credentials in +`remote..url` configuration, it won't detect credentials in +`remote..pushurl` configuration. ++ +You might want to enable this to prevent inadvertent credentials +exposure, e.g. because: ++ +* The OS or system where you're running git may not provide way way or + otherwise allow you to configure the permissions of the + configuration file where the username and/or password are stored. +* Even if it does, having such data stored "at rest" might expose you + in other ways, e.g. a backup process might copy the data to another + system. +* The git programs will pass the full URL to one another as arguments + on the command-line, meaning the credentials will be exposed to oher + users on OS's or systems that allow other users to see the full + process list of other users. On linux the "hidepid" setting + documented in procfs(5) allows for configuring this behavior. ++ +If such concerns don't apply to you then you probably don't need to be +concerned about credentials exposure due to storing that sensitive +data in git's configuration files. If you do want to use this, set +`transfer.credentialsInUrl` to one of these values: ++ +* `allow` (default): Git will proceed with its activity without warning. +* `warn`: Git will write a warning message to `stderr` when parsing a URL + with a plaintext credential. +* `die`: Git will write a failure message to `stderr` when parsing a URL + with a plaintext credential. + transfer.fsckObjects:: When `fetch.fsckObjects` or `receive.fsckObjects` are not set, the value of this variable is used instead. diff --git a/remote.c b/remote.c index 9b9bbfe80e..42c891d44f 100644 --- a/remote.c +++ b/remote.c @@ -623,7 +623,7 @@ static void validate_remote_url(struct remote *remote) struct strbuf redacted = STRBUF_INIT; int warn_not_die; - if (git_config_get_string_tmp("fetch.credentialsinurl", &value)) + if (git_config_get_string_tmp("transfer.credentialsinurl", &value)) return; if (!strcmp("warn", value)) @@ -633,7 +633,7 @@ static void validate_remote_url(struct remote *remote) else if (!strcmp("allow", value)) return; else - die(_("unrecognized value fetch.credentialsInURL: '%s'"), value); + die(_("unrecognized value transfer.credentialsInURL: '%s'"), value); for (i = 0; i < remote->url_nr; i++) { struct url_info url_info = { 0 }; diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh index dedca106a7..79d8a7b367 100755 --- a/t/t5516-fetch-push.sh +++ b/t/t5516-fetch-push.sh @@ -1836,18 +1836,18 @@ test_expect_success 'refuse to push a hidden ref, and make sure do not pollute t test_expect_success 'fetch warns or fails when using username:password' ' message="URL '\''https://username:@localhost/'\'' uses plaintext credentials" && - test_must_fail git -c fetch.credentialsInUrl=allow fetch https://username:password@localhost 2>err && + test_must_fail git -c transfer.credentialsInUrl=allow fetch https://username:password@localhost 2>err && ! grep "$message" err && - test_must_fail git -c fetch.credentialsInUrl=warn fetch https://username:password@localhost 2>err && + test_must_fail git -c transfer.credentialsInUrl=warn fetch https://username:password@localhost 2>err && grep "warning: $message" err >warnings && test_line_count = 3 warnings && - test_must_fail git -c fetch.credentialsInUrl=die fetch https://username:password@localhost 2>err && + test_must_fail git -c transfer.credentialsInUrl=die fetch https://username:password@localhost 2>err && grep "fatal: $message" err >warnings && test_line_count = 1 warnings && - test_must_fail git -c fetch.credentialsInUrl=die fetch https://username:@localhost 2>err && + test_must_fail git -c transfer.credentialsInUrl=die fetch https://username:@localhost 2>err && grep "fatal: $message" err >warnings && test_line_count = 1 warnings ' @@ -1855,12 +1855,12 @@ test_expect_success 'fetch warns or fails when using username:password' ' test_expect_success 'push warns or fails when using username:password' ' message="URL '\''https://username:@localhost/'\'' uses plaintext credentials" && - test_must_fail git -c fetch.credentialsInUrl=allow push https://username:password@localhost 2>err && + test_must_fail git -c transfer.credentialsInUrl=allow push https://username:password@localhost 2>err && ! grep "$message" err && - test_must_fail git -c fetch.credentialsInUrl=warn push https://username:password@localhost 2>err && + test_must_fail git -c transfer.credentialsInUrl=warn push https://username:password@localhost 2>err && grep "warning: $message" err >warnings && - test_must_fail git -c fetch.credentialsInUrl=die push https://username:password@localhost 2>err && + test_must_fail git -c transfer.credentialsInUrl=die push https://username:password@localhost 2>err && grep "fatal: $message" err >warnings && test_line_count = 1 warnings ' diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh index d2f046b4b9..e6a248bbdc 100755 --- a/t/t5601-clone.sh +++ b/t/t5601-clone.sh @@ -73,24 +73,24 @@ test_expect_success 'clone respects GIT_WORK_TREE' ' test_expect_success 'clone warns or fails when using username:password' ' message="URL '\''https://username:@localhost/'\'' uses plaintext credentials" && - test_must_fail git -c fetch.credentialsInUrl=allow clone https://username:password@localhost attempt1 2>err && + test_must_fail git -c transfer.credentialsInUrl=allow clone https://username:password@localhost attempt1 2>err && ! grep "$message" err && - test_must_fail git -c fetch.credentialsInUrl=warn clone https://username:password@localhost attempt2 2>err && + test_must_fail git -c transfer.credentialsInUrl=warn clone https://username:password@localhost attempt2 2>err && grep "warning: $message" err >warnings && test_line_count = 2 warnings && - test_must_fail git -c fetch.credentialsInUrl=die clone https://username:password@localhost attempt3 2>err && + test_must_fail git -c transfer.credentialsInUrl=die clone https://username:password@localhost attempt3 2>err && grep "fatal: $message" err >warnings && test_line_count = 1 warnings && - test_must_fail git -c fetch.credentialsInUrl=die clone https://username:@localhost attempt3 2>err && + test_must_fail git -c transfer.credentialsInUrl=die clone https://username:@localhost attempt3 2>err && grep "fatal: $message" err >warnings && test_line_count = 1 warnings ' test_expect_success 'clone does not detect username:password when it is https://username@domain:port/' ' - test_must_fail git -c fetch.credentialsInUrl=warn clone https://username@localhost:8080 attempt3 2>err && + test_must_fail git -c transfer.credentialsInUrl=warn clone https://username@localhost:8080 attempt3 2>err && ! grep "uses plaintext credentials" err '