git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
* [PATCH 0/2] For v2.37.0: rename fetch.credentialsInUrl to transfer.credentialsInUrl
@ 2022-06-15 10:44 Ævar Arnfjörð Bjarmason
  2022-06-15 10:44 ` [PATCH 1/2] fetch doc: note "pushurl" caveat about "credentialsInUrl", elaborate Ævar Arnfjörð Bjarmason
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-06-15 10:44 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Derrick Stolee,
	Ævar Arnfjörð Bjarmason

The $subject is a case where nothing is broken except potential user
expectations, but as argued in 2/2 I think we really should rename
this before we kick it out the door.

Having a "fetch" configuration configure "git push" is really
confusing, and once we need to support it backing out of it would be a
hassle.

Then 1/2 clarifies how it really works, and what it's aimed to do, and
notes the major caveat that the check doesn't cover "pushurl"
configuration, which in some setups is the only place you'll put
passwords in URLs. I think it likewise makes sense to have that in
v2.37.0 so we won't lull users into a false sense of security.

This is a relatively large change for an "rc", but the only code
changes are s/fetch\.credentialsInUrl/transfer.credentialsInUrl/g, the
rest is all docs.

There's a trivial conflict with my just-submitted
https://lore.kernel.org/git/cover-0.1-00000000000-20220615T103609Z-avarab@gmail.com:
The relevant "test_expect_success" line needs a "LIBCURL" prerequisite
added in the merge conflict.

Ævar Arnfjörð Bjarmason (2):
  fetch doc: note "pushurl" caveat about "credentialsInUrl", elaborate
  transfer doc: move fetch.credentialsInUrl to "transfer" config
    namespace

 Documentation/RelNotes/2.37.0.txt |  6 +++--
 Documentation/config/fetch.txt    | 14 ------------
 Documentation/config/transfer.txt | 38 +++++++++++++++++++++++++++++++
 remote.c                          |  4 ++--
 t/t5516-fetch-push.sh             | 14 ++++++------
 t/t5601-clone.sh                  | 10 ++++----
 6 files changed, 56 insertions(+), 30 deletions(-)

-- 
2.36.1.1239.gfba91521d90


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 1/2] fetch doc: note "pushurl" caveat about "credentialsInUrl", elaborate
  2022-06-15 10:44 [PATCH 0/2] For v2.37.0: rename fetch.credentialsInUrl to transfer.credentialsInUrl Ævar Arnfjörð Bjarmason
@ 2022-06-15 10:44 ` Ævar Arnfjörð Bjarmason
  2022-06-26 13:15   ` René Scharfe
  2022-06-15 10:44 ` [PATCH 2/2] transfer doc: move fetch.credentialsInUrl to "transfer" config namespace Ævar Arnfjörð Bjarmason
  2022-06-15 13:12 ` [PATCH 0/2] For v2.37.0: rename fetch.credentialsInUrl to transfer.credentialsInUrl Derrick Stolee
  2 siblings, 1 reply; 6+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-06-15 10:44 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Derrick Stolee,
	Ævar Arnfjörð Bjarmason

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.<name>.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 <avarab@gmail.com>
---
 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 8f1ff3a5961..39ca3606dec 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.<name>.url` config, not `remote.<name>.pushurl`.
 
 
 Performance, Internal Implementation, Development Support etc.
diff --git a/Documentation/config/fetch.txt b/Documentation/config/fetch.txt
index 0db7fe85bb8..827961059f8 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
-	`<protocol>://<user>:<password>@<domain>/<path>`. 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
+	`<protocol>://<user>:<password>@<domain>/<path>`. 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.<name>.url` configuration, it won't detect credentials in
+`remote.<name>.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
-- 
2.36.1.1239.gfba91521d90


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH 2/2] transfer doc: move fetch.credentialsInUrl to "transfer" config namespace
  2022-06-15 10:44 [PATCH 0/2] For v2.37.0: rename fetch.credentialsInUrl to transfer.credentialsInUrl Ævar Arnfjörð Bjarmason
  2022-06-15 10:44 ` [PATCH 1/2] fetch doc: note "pushurl" caveat about "credentialsInUrl", elaborate Ævar Arnfjörð Bjarmason
@ 2022-06-15 10:44 ` Ævar Arnfjörð Bjarmason
  2022-06-15 13:12 ` [PATCH 0/2] For v2.37.0: rename fetch.credentialsInUrl to transfer.credentialsInUrl Derrick Stolee
  2 siblings, 0 replies; 6+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2022-06-15 10:44 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Derrick Stolee,
	Ævar Arnfjörð Bjarmason

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
"<namespace>.<var>" 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 <avarab@gmail.com>
---
 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 39ca3606dec..9902a74f34a 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.<name>.url` config, not `remote.<name>.pushurl`.
diff --git a/Documentation/config/fetch.txt b/Documentation/config/fetch.txt
index 827961059f8..cd65d236b43 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
-	`<protocol>://<user>:<password>@<domain>/<path>`. 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.<name>.url` configuration, it won't detect credentials in
-`remote.<name>.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 b49429eb4db..b4475c06900 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
+	`<protocol>://<user>:<password>@<domain>/<path>`. 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.<name>.url` configuration, it won't detect credentials in
+`remote.<name>.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 9b9bbfe80ec..42c891d44fd 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 dedca106a7a..79d8a7b3675 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:<redacted>@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:<redacted>@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 d2f046b4b92..e6a248bbdcc 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:<redacted>@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
 '
 
-- 
2.36.1.1239.gfba91521d90


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH 0/2] For v2.37.0: rename fetch.credentialsInUrl to transfer.credentialsInUrl
  2022-06-15 10:44 [PATCH 0/2] For v2.37.0: rename fetch.credentialsInUrl to transfer.credentialsInUrl Ævar Arnfjörð Bjarmason
  2022-06-15 10:44 ` [PATCH 1/2] fetch doc: note "pushurl" caveat about "credentialsInUrl", elaborate Ævar Arnfjörð Bjarmason
  2022-06-15 10:44 ` [PATCH 2/2] transfer doc: move fetch.credentialsInUrl to "transfer" config namespace Ævar Arnfjörð Bjarmason
@ 2022-06-15 13:12 ` Derrick Stolee
  2022-06-15 18:35   ` Junio C Hamano
  2 siblings, 1 reply; 6+ messages in thread
From: Derrick Stolee @ 2022-06-15 13:12 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason, git; +Cc: Junio C Hamano

On 6/15/22 6:44 AM, Ævar Arnfjörð Bjarmason wrote:
> The $subject is a case where nothing is broken except potential user
> expectations, but as argued in 2/2 I think we really should rename
> this before we kick it out the door.
> 
> Having a "fetch" configuration configure "git push" is really
> confusing, and once we need to support it backing out of it would be a
> hassle.
> 
> Then 1/2 clarifies how it really works, and what it's aimed to do, and
> notes the major caveat that the check doesn't cover "pushurl"
> configuration, which in some setups is the only place you'll put
> passwords in URLs. I think it likewise makes sense to have that in
> v2.37.0 so we won't lull users into a false sense of security.
> 
> This is a relatively large change for an "rc", but the only code
> changes are s/fetch\.credentialsInUrl/transfer.credentialsInUrl/g, the
> rest is all docs.

Sorry that I didn't see this before looking at your other RFC.
(That RFC still has some references to "fetch.credentialsInUrl"
in your commit messages, btw.)

This rename makes sense and is good to handle before the release.
Thank you for updating the release notes.

The expanded details in the docs are good.

Thanks,
-Stolee

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 0/2] For v2.37.0: rename fetch.credentialsInUrl to transfer.credentialsInUrl
  2022-06-15 13:12 ` [PATCH 0/2] For v2.37.0: rename fetch.credentialsInUrl to transfer.credentialsInUrl Derrick Stolee
@ 2022-06-15 18:35   ` Junio C Hamano
  0 siblings, 0 replies; 6+ messages in thread
From: Junio C Hamano @ 2022-06-15 18:35 UTC (permalink / raw)
  To: Derrick Stolee; +Cc: Ævar Arnfjörð Bjarmason, git

Derrick Stolee <derrickstolee@github.com> writes:

> This rename makes sense and is good to handle before the release.
> Thank you for updating the release notes.
>
> The expanded details in the docs are good.

Generally, transfer.$name hierarchy is there to give defaults to
fetch.$name and push.$name so that the knob can be set separately
for different directions of the transfer, but for this particular
one, I do not see a reason why we want to redact fetch requests
without redacting push requests.

One thing that worries me is that this will probably burn our early
adopters, those who are willing to help our development by running
versions based on 'next' and already have fetch.* configuration.
But the breakage this change brings on them hopefully will not be
large enough to discourage them from being guinea pigs ;-)

Thanks, all.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/2] fetch doc: note "pushurl" caveat about "credentialsInUrl", elaborate
  2022-06-15 10:44 ` [PATCH 1/2] fetch doc: note "pushurl" caveat about "credentialsInUrl", elaborate Ævar Arnfjörð Bjarmason
@ 2022-06-26 13:15   ` René Scharfe
  0 siblings, 0 replies; 6+ messages in thread
From: René Scharfe @ 2022-06-26 13:15 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason, git
  Cc: Junio C Hamano, Derrick Stolee

Am 15.06.22 um 12:44 schrieb Ævar Arnfjörð Bjarmason:
> 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.<name>.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 <avarab@gmail.com>
> ---
>  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 8f1ff3a5961..39ca3606dec 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.<name>.url` config, not `remote.<name>.pushurl`.
>
>
>  Performance, Internal Implementation, Development Support etc.
> diff --git a/Documentation/config/fetch.txt b/Documentation/config/fetch.txt
> index 0db7fe85bb8..827961059f8 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
> -	`<protocol>://<user>:<password>@<domain>/<path>`. 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
> +	`<protocol>://<user>:<password>@<domain>/<path>`. You may want
> +	to warn or forbid the use of such configuration (in favor of
> +	using linkgit:git-credential[1]).

What I want is one thing, but the more interesting point here is that
this option actually allows me to achieve these goals, no?  E.g. "This
configuration variable can let git warn about or reject such URLs.".
Otherwise this reads more as a general kind of advise along the lines
of "Passwords are insecure; you may want to use multi-factor
authentication.".

> ++
> +Note that this is currently limited to detecting credentials in
> +`remote.<name>.url` configuration, it won't detect credentials in
> +`remote.<name>.pushurl` configuration.

That's a puzzling restriction.

> ++
> +You might want to enable this to prevent inadvertent credentials
> +exposure, e.g. because:

At this point in the text "this" is undefined; the values and their
meaning are explained two paragraphs down.  Perhaps reverse the order?

> ++
> +* 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.

Perhaps s/way way/a way/?  In that case: What's the difference between
"provide a way" and "allow you" in this context?  The point here is that
"you are unable to restrict access to the configuration file storing the
credentials", right?

> +* 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.

No need for scare quotes here; this is a legit term.

> +* 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.

"oher" has been fixed already.  Linux should get a capital L.

> ++
> +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:

So I'm safe as long as I don't make backups, right? ;-)

Cleartext passwords are a security risk and advising not to be concerned
about storing them in a config file is a bit scary.  A better default
would probably be to reject URLs with credentials and make credential
helpers at least as convenient to use as a plaintext file (if they
aren't already) -- but that's just a tangent from a slightly paranoid
admin type.  My point is that the tone seems a bit off.

>  +
>  * `allow` (default): Git will proceed with its activity without warning.
>  * `warn`: Git will write a warning message to `stderr` when parsing a URL


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2022-06-26 13:16 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-15 10:44 [PATCH 0/2] For v2.37.0: rename fetch.credentialsInUrl to transfer.credentialsInUrl Ævar Arnfjörð Bjarmason
2022-06-15 10:44 ` [PATCH 1/2] fetch doc: note "pushurl" caveat about "credentialsInUrl", elaborate Ævar Arnfjörð Bjarmason
2022-06-26 13:15   ` René Scharfe
2022-06-15 10:44 ` [PATCH 2/2] transfer doc: move fetch.credentialsInUrl to "transfer" config namespace Ævar Arnfjörð Bjarmason
2022-06-15 13:12 ` [PATCH 0/2] For v2.37.0: rename fetch.credentialsInUrl to transfer.credentialsInUrl Derrick Stolee
2022-06-15 18:35   ` Junio C Hamano

Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

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 read-only IMAP folder(s) and NNTP newsgroup(s).