Re: [PATCH] urlmatch: do not allow passwords in URLs by default

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Fri, Apr 30 2021, Derrick Stolee via GitGitGadget wrote:

Now for a comment on the direction...:

> From: Derrick Stolee <dstolee@microsoft.com>
>
> Git allows URLs of the following pattern:
>
>   https://username:password@domain/route
>
> These URLs are then parsed to pull out the username and password for use
> when authenticating with the URL. Git is careful to anonymize the URL in
> status messages with transport_anonymize_url(), but it stores the URL as
> plaintext in the .git/config file. The password may leak in other ways.
>
> This is not a recommended way to store credentials, especially
> authentication tokens that do more than simply allow access to a
> repository.
>
> Users should be aware that when they provide a URL in this form that
> they are not being incredibly secure. It is much better to use a

"Incredibly secure"? Security so good you wouldn't believe it if you saw
it ? :)

> credential manager to securely store passwords. Even better, some
> credential managers use more sophisticated authentication strategies
> including multi-factor authentication. This does not stop users from
> continuing to do this.
>
> Some Git hosting providers are working to completely drop
> username/password credential strategies, which will make URLs of this
> form stop working. However, that requires certain changes to credential
> managers that need to be released and sufficiently adopted before making
> such a server-side change.
>
> In the meantime, it might be helpful to alert users that they are doing
> something insecure with these URLs.
>
> Create a new config option, core.allowUsernamePasswordUrls, which is
> disabled by default. If Git attempts to parse a password from a URL in
> this form, it will die() if this config is not enabled. This affects a
> few test scripts, but enabling the config in those places is relatively
> simple.
>
> This will cause a significant change in behavior for users who rely upon
> this username:password pattern. The error message describes the config
> that they must enable to continue working with these URLs. This has a
> significant chance of breaking some automated workflows that use URLs in
> this fashion, but even those workflows would be better off using a
> different mechanism for credentials.

...

> I cannot understate the care in which we should consider this change.
> The impact of this change in a Git release could be significant. We
> should advertise this very clearly in the release notes.

Seems like something more for below-the-"---" than a commit
message. If/when it lands the "consider this change" has already
happened.

> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>     Reject passwords in URLs
>     
>     I received multiple messages "alerting" me to the issue of users
>     supplying server-side tokens into the username:password field of a URL.
>     This is not a secure way to handle these tokens.
>     
>     On the one hand, this is user error: Users should not supply a token to
>     a location where they do not know what will happen to it. In Git's
>     defense, its behavior is completely open about storing the URL in the
>     .git/config file as a plain-text string and users should know that when
>     using this feature.
>     
>     However, users just. keep. doing it.
>     
>     There is some expectation that since this portion of the URL is a
>     password, then Git is responsible for tracking that password securely.
>     I'm not sure we should venture down that road, since we already have a
>     pretty good solution by using the credential helper interface.
>     
>     Here is my best effort to find a compromise here: start failing when
>     parsing a password from a URL like this, with a config option to
>     re-enable the existing behavior.
>     
>     I completely understand if this is too much of a breaking change. I
>     wonder if there is anything we can do to assist users into being more
>     careful with their secrets.

I think a good staring compromise would not be to make long-standing
supported behavior an error, but to use the advise() system to emit some
notice about this.

While I get what problem you're trying to solve in practice, I disagree
with the strong assertion that a user is "doing something insecure" by
definition by using such URLs.

If you trust your FS permissions, and as a local user, ultimately if you
didn't a credential manager wouldn't be much use anyway, and/or
use/don't care (e.g. closed network) about transport security then
having a password in your config is just fine, and doesn't otherwise
compromise security.

Unlike SSH this is a thing that the HTTP protocol supports, so I don't
think it's the place of git to go out of its way by default to break
working URL schemas by making hard breaking assumptions about the user's
workflow.

I think a much better way to address this issue, if it's an issue that
needs to be addressed, is on the server-side. The service operator is in
a much better position to know if URL passwords are a bad idea in their
case (e.g. is the software frequently used in certain ways, is there no
transport security, are we using debug URL tracing etc).

You allude to some of that in your commit message. "Some Git hosting
providers". Some? GitHub? In any case, to me that's further reason we
should hold off on such a change in git.git. Let the big hosting
providers experiment with this, and if e.g. they all decide to stop
supporting this and it therefore becomes less common practice we'll be
better informed about if/what to change in git.git.