remote: create fetch.credentialsInUrl config

Users sometimes provide a "username:password" combination in their plaintext URLs. Since Git stores these URLs in plaintext in the .git/config file, this is a very insecure way of storing these credentials. Credential managers are a more secure way of storing this information. System administrators might want to prevent this kind of use by users on their machines. Create a new "fetch.credentialsInUrl" config option and teach Git to warn or die when seeing a URL with this kind of information. The warning anonymizes the sensitive information of the URL to be clear about the issue. This change currently defaults the behavior to "allow" which does nothing with these URLs. We can consider changing this behavior to "warn" by default if we wish. At that time, we may want to add some advice about setting fetch.credentialsInUrl=ignore for users who still want to follow this pattern (and not receive the warning). An earlier version of this change injected the logic into url_normalize() in urlmatch.c. While most code paths that parse URLs eventually normalize the URL, that normalization does not happen early enough in the stack to avoid attempting connections to the URL first. By inserting a check into the remote validation, we identify the issue before making a connection. In the old code path, this was revealed by testing the new t5601-clone.sh test under --stress, resulting in an instance where the return code was 13 (SIGPIPE) instead of 128 from the die(). However, we can reuse the parsing information from url_normalize() in order to benefit from its well-worn parsing logic. We can use the struct url_info that is created in that method to replace the password with "<redacted>" in our error messages. This comes with a slight downside that the normalized URL might look slightly different from the input URL (for instance, the normalized version adds a closing slash). This should not hinder users figuring out what the problem is and being able to fix the issue. As an attempt to ensure the parsing logic did not catch any unintentional cases, I modified this change locally to to use the "die" option by default. Running the test suite succeeds except for the explicit username:password URLs used in t5550-http-fetch-dumb.sh and t5541-http-push-smart.sh. This means that all other tested URLs did not trigger this logic. The tests show that the proper error messages appear (or do not appear), but also count the number of error messages. When only warning, each process validates the remote URL and outputs a warning. This happens twice for clone, three times for fetch, and once for push. Helped-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: Derrick Stolee <derrickstolee@github.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Derrick Stolee committed Jun 6, 2022 at 14:36 UTC 6dcbdc0d6616d7fbd2445aa2237b22e3c172ea85
4 files changed +117
Documentation/config/fetch.txt
+14
@@ -95,3 +95,17 @@ fetch.writeCommitGraph::
95 merge and the write may take longer. Having an updated commit-graph
96 file helps performance of many Git commands, including `git merge-base`,
97 `git push -f`, and `git log --graph`. Defaults to false.
98 +
99 +fetch.credentialsInUrl::
100 + A URL can contain plaintext credentials in the form
101 + `<protocol>://<user>:<password>@<domain>/<path>`. Using such URLs
102 + is not recommended as it exposes the password in multiple ways,
103 + including Git storing the URL as plaintext in the repository config.
104 + The `fetch.credentialsInUrl` option provides instruction for how Git
105 + should react to seeing such a URL, with these values:
106 ++
107 +* `allow` (default): Git will proceed with its activity without warning.
108 +* `warn`: Git will write a warning message to `stderr` when parsing a URL
109 + with a plaintext credential.
110 +* `die`: Git will write a failure message to `stderr` when parsing a URL
111 + with a plaintext credential.
remote.c
+48
@@ -1,6 +1,7 @@
1 #include "cache.h"
2 #include "config.h"
3 #include "remote.h"
4 +#include "urlmatch.h"
5 #include "refs.h"
6 #include "refspec.h"
7 #include "object-store.h"
@@ -613,6 +614,50 @@ const char *remote_ref_for_branch(struct branch *branch, int for_push)
614 return NULL;
615 }
616
617 +static void validate_remote_url(struct remote *remote)
618 +{
619 + int i;
620 + const char *value;
621 + struct strbuf redacted = STRBUF_INIT;
622 + int warn_not_die;
623 +
624 + if (git_config_get_string_tmp("fetch.credentialsinurl", &value))
625 + return;
626 +
627 + if (!strcmp("warn", value))
628 + warn_not_die = 1;
629 + else if (!strcmp("die", value))
630 + warn_not_die = 0;
631 + else if (!strcmp("allow", value))
632 + return;
633 + else
634 + die(_("unrecognized value fetch.credentialsInURL: '%s'"), value);
635 +
636 + for (i = 0; i < remote->url_nr; i++) {
637 + struct url_info url_info = { 0 };
638 +
639 + if (!url_normalize(remote->url[i], &url_info) ||
640 + !url_info.passwd_off)
641 + goto loop_cleanup;
642 +
643 + strbuf_reset(&redacted);
644 + strbuf_add(&redacted, url_info.url, url_info.passwd_off);
645 + strbuf_addstr(&redacted, "<redacted>");
646 + strbuf_addstr(&redacted,
647 + url_info.url + url_info.passwd_off + url_info.passwd_len);
648 +
649 + if (warn_not_die)
650 + warning(_("URL '%s' uses plaintext credentials"), redacted.buf);
651 + else
652 + die(_("URL '%s' uses plaintext credentials"), redacted.buf);
653 +
654 +loop_cleanup:
655 + free(url_info.url);
656 + }
657 +
658 + strbuf_release(&redacted);
659 +}
660 +
661 static struct remote *
662 remotes_remote_get_1(struct remote_state *remote_state, const char *name,
663 const char *(*get_default)(struct remote_state *,
@@ -638,6 +683,9 @@ remotes_remote_get_1(struct remote_state *remote_state, const char *name,
683 add_url_alias(remote_state, ret, name);
684 if (!valid_remote(ret))
685 return NULL;
686 +
687 + validate_remote_url(ret);
688 +
689 return ret;
690 }
691
t/t5516-fetch-push.sh
+32
@@ -12,6 +12,7 @@ This test checks the following functionality:
12 * --porcelain output format
13 * hiderefs
14 * reflogs
15 +* URL validation
16 '
17
18 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
@@ -1809,4 +1810,35 @@ test_expect_success 'refuse fetch to current branch of bare repository worktree'
1810 git -C bare.git fetch -u .. HEAD:wt
1811 '
1812
1813 +test_expect_success 'fetch warns or fails when using username:password' '
1814 + message="URL '\''https://username:<redacted>@localhost/'\'' uses plaintext credentials" &&
1815 + test_must_fail git -c fetch.credentialsInUrl=allow fetch https://username:password@localhost 2>err &&
1816 + ! grep "$message" err &&
1817 +
1818 + test_must_fail git -c fetch.credentialsInUrl=warn fetch https://username:password@localhost 2>err &&
1819 + grep "warning: $message" err >warnings &&
1820 + test_line_count = 3 warnings &&
1821 +
1822 + test_must_fail git -c fetch.credentialsInUrl=die fetch https://username:password@localhost 2>err &&
1823 + grep "fatal: $message" err >warnings &&
1824 + test_line_count = 1 warnings &&
1825 +
1826 + test_must_fail git -c fetch.credentialsInUrl=die fetch https://username:@localhost 2>err &&
1827 + grep "fatal: $message" err >warnings &&
1828 + test_line_count = 1 warnings
1829 +'
1830 +
1831 +
1832 +test_expect_success 'push warns or fails when using username:password' '
1833 + message="URL '\''https://username:<redacted>@localhost/'\'' uses plaintext credentials" &&
1834 + test_must_fail git -c fetch.credentialsInUrl=allow push https://username:password@localhost 2>err &&
1835 + ! grep "$message" err &&
1836 +
1837 + test_must_fail git -c fetch.credentialsInUrl=warn push https://username:password@localhost 2>err &&
1838 + grep "warning: $message" err >warnings &&
1839 + test_must_fail git -c fetch.credentialsInUrl=die push https://username:password@localhost 2>err &&
1840 + grep "fatal: $message" err >warnings &&
1841 + test_line_count = 1 warnings
1842 +'
1843 +
1844 test_done
t/t5601-clone.sh
+23
@@ -71,6 +71,29 @@ test_expect_success 'clone respects GIT_WORK_TREE' '
71
72 '
73
74 +test_expect_success 'clone warns or fails when using username:password' '
75 + message="URL '\''https://username:<redacted>@localhost/'\'' uses plaintext credentials" &&
76 + test_must_fail git -c fetch.credentialsInUrl=allow clone https://username:password@localhost attempt1 2>err &&
77 + ! grep "$message" err &&
78 +
79 + test_must_fail git -c fetch.credentialsInUrl=warn clone https://username:password@localhost attempt2 2>err &&
80 + grep "warning: $message" err >warnings &&
81 + test_line_count = 2 warnings &&
82 +
83 + test_must_fail git -c fetch.credentialsInUrl=die clone https://username:password@localhost attempt3 2>err &&
84 + grep "fatal: $message" err >warnings &&
85 + test_line_count = 1 warnings &&
86 +
87 + test_must_fail git -c fetch.credentialsInUrl=die clone https://username:@localhost attempt3 2>err &&
88 + grep "fatal: $message" err >warnings &&
89 + test_line_count = 1 warnings
90 +'
91 +
92 +test_expect_success 'clone does not detect username:password when it is https://username@domain:port/' '
93 + test_must_fail git -c fetch.credentialsInUrl=warn clone https://username@localhost:8080 attempt3 2>err &&
94 + ! grep "uses plaintext credentials" err
95 +'
96 +
97 test_expect_success 'clone from hooks' '
98
99 test_create_repo r0 &&