| Message ID | 25cc0f6d91a9d23eb1b755e1463d672e4958a4e9.1667245639.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | t5516/t5601: avoid using localhost for failing HTTPS requests | expand |
On Mon, Oct 31 2022, Johannes Schindelin via GitGitGadget wrote: > From: Johannes Schindelin <johannes.schindelin@gmx.de> > > In 6dcbdc0d6616 (remote: create fetch.credentialsInUrl config, > 2022-06-06), we added four test cases that validate various behavior > around passing credentials as part of the URL (which is considered > unsafe in general). > > These tests do not _actually_ try to connect anywhere, but have to use > the https:// protocol in order to validate the intended code paths. > > However, using `localhost` for such a connection causes several > problems: > > - There might be a web server running on localhost, and we do not > actually want to connect to that. > > - The DNS resolver, or the local firewall, might take a substantial > amount of time (or forever, whichever comes first) to fail to connect, > slowing down the test cases unnecessarily. > > Let's instead use an IPv4 address that is guaranteed never to offer a > web server: 224.0.0.1 (which is part of the IP multicast range). > > Incidentally, this seems to fix an issue where the tests fail in the > Windows jobs of Git's CI builds. > [...] > diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh > index 45f0803ed4d..0b386c74818 100755 > --- a/t/t5601-clone.sh > +++ b/t/t5601-clone.sh > @@ -72,19 +72,19 @@ test_expect_success 'clone respects GIT_WORK_TREE' ' > ' > > test_expect_success LIBCURL 'clone warns or fails when using username:password' ' > - message="URL '\''https://username:<redacted>@localhost/'\'' uses plaintext credentials" && > - test_must_fail git -c transfer.credentialsInUrl=allow clone https://username:password@localhost attempt1 2>err && > + message="URL '\''https://username:<redacted>@224.0.0.1/'\'' uses plaintext credentials" && > + test_must_fail git -c transfer.credentialsInUrl=allow clone https://username:password@224.0.0.1 attempt1 2>err && > ! grep "$message" err && > > - test_must_fail git -c transfer.credentialsInUrl=warn clone https://username:password@localhost attempt2 2>err && > + test_must_fail git -c transfer.credentialsInUrl=warn clone https://username:password@224.0.0.1 attempt2 2>err && > grep "warning: $message" err >warnings && > test_line_count = 2 warnings && > > - test_must_fail git -c transfer.credentialsInUrl=die clone https://username:password@localhost attempt3 2>err && > + test_must_fail git -c transfer.credentialsInUrl=die clone https://username:password@224.0.0.1 attempt3 2>err && > grep "fatal: $message" err >warnings && > test_line_count = 1 warnings && > > - test_must_fail git -c transfer.credentialsInUrl=die clone https://username:@localhost attempt3 2>err && > + test_must_fail git -c transfer.credentialsInUrl=die clone https://username:@224.0.0.1 attempt3 2>err && > grep "fatal: $message" err >warnings && > test_line_count = 1 warnings > ' For this one one at least, it eventually gets around to setting up an actual httpd server with cloning etc. from $HTTPD_URL. Can't we just do that for both of these tests rather than the the 224.0.0.0 hack? I.e. the root cause is that we're cleverly faking a not-a-server here, and now we're going to add another somewhat clever hack on top. but since the test coverage is for https:// anyway, and we have other https tests against an actual server...
On Mon, Oct 31, 2022 at 07:47:17PM +0000, Johannes Schindelin via GitGitGadget wrote: > In 6dcbdc0d6616 (remote: create fetch.credentialsInUrl config, > 2022-06-06), we added four test cases that validate various behavior > around passing credentials as part of the URL (which is considered > unsafe in general). > > These tests do not _actually_ try to connect anywhere, but have to use > the https:// protocol in order to validate the intended code paths. By "actually" here, I assume you mean "they do not expect to succeed". But I think the first one (with credentialsInUrl=allow), does try to make a connection. > However, using `localhost` for such a connection causes several > problems: > > - There might be a web server running on localhost, and we do not > actually want to connect to that. > > - The DNS resolver, or the local firewall, might take a substantial > amount of time (or forever, whichever comes first) to fail to connect, > slowing down the test cases unnecessarily. Right. I think we assume that DNS resolution of localhost is fast-ish, as we use it in other https tests. But I could certainly imagine a local firewall causing issues (especially as this is real port 443, whereas our other tests are usually high ports). > Let's instead use an IPv4 address that is guaranteed never to offer a > web server: 224.0.0.1 (which is part of the IP multicast range). This feels pretty magical. I think it would be pretty unlikely for it to have a web server, but I wouldn't be surprised if there are systems where we get similar IP-routing hangs. Is there a reason not to move all of these tests into t5550 or t5551, where we have a real http server? That would be less magical, and then this first test: > test_expect_success LIBCURL 'fetch warns or fails when using username:password' ' > - message="URL '\''https://username:<redacted>@localhost/'\'' uses plaintext credentials" && > - test_must_fail git -c transfer.credentialsInUrl=allow fetch https://username:password@localhost 2>err && > + message="URL '\''https://username:<redacted>@224.0.0.1/'\'' uses plaintext credentials" && > + test_must_fail git -c transfer.credentialsInUrl=allow fetch https://username:password@224.0.0.1 2>err && > ! grep "$message" err && could be more robust. It would actually check that we succeeded in using the URL. -Peff
On Mon, Oct 31, 2022 at 07:20:11PM -0400, Jeff King wrote: > > Let's instead use an IPv4 address that is guaranteed never to offer a > > web server: 224.0.0.1 (which is part of the IP multicast range). > > This feels pretty magical. I think it would be pretty unlikely for it to > have a web server, but I wouldn't be surprised if there are systems > where we get similar IP-routing hangs. > > Is there a reason not to move all of these tests into t5550 or t5551, > where we have a real http server? That would be less magical, and then > this first test: > > > test_expect_success LIBCURL 'fetch warns or fails when using username:password' ' > > - message="URL '\''https://username:<redacted>@localhost/'\'' uses plaintext credentials" && > > - test_must_fail git -c transfer.credentialsInUrl=allow fetch https://username:password@localhost 2>err && > > + message="URL '\''https://username:<redacted>@224.0.0.1/'\'' uses plaintext credentials" && > > + test_must_fail git -c transfer.credentialsInUrl=allow fetch https://username:password@224.0.0.1 2>err && > > ! grep "$message" err && > > could be more robust. It would actually check that we succeeded in using > the URL. It is magical, indeed. If it's significantly more difficult to move these into t5550 or t5551, then I'm OK with this in the meantime (since GitHub Actions really is our primary CI target that we care about this not hanging on). But assuming that moving these around isn't that difficult, I would be slightly happier to see these tests in one of the aforementioned spots. Thanks, Taylor
On Mon, Oct 31, 2022 at 07:20:11PM -0400, Jeff King wrote: > > - The DNS resolver, or the local firewall, might take a substantial > > amount of time (or forever, whichever comes first) to fail to connect, > > slowing down the test cases unnecessarily. > > Right. I think we assume that DNS resolution of localhost is fast-ish, > as we use it in other https tests. But I could certainly imagine a local > firewall causing issues (especially as this is real port 443, whereas > our other tests are usually high ports). Actually, I am wrong about DNS here. We use a bare 127.0.0.1 in lib-httpd.sh, so DNS may indeed be the source of the issue here. That _might_ mean that using the bare IP would be safe here, but I would not want to bet on it. Using different port numbers, plus not expecting a listener on the other end, might cause different outcomes than we see in the other tests. I do think moving these tests to use a real http server is a better solution, though. I'll provide a patch in a moment. -Peff
On Mon, Oct 31, 2022 at 10:03:58PM -0400, Jeff King wrote: > I do think moving these tests to use a real http server is a better > solution, though. I'll provide a patch in a moment. Here that is. I hope this isn't co-opting your series; my goal was to do the work so that you wouldn't have to, but I admit it is assuming you agree with my approach. ;) The first patch is the interesting one. The second one is just your 2/2 rebased onto the new location. [1/2]: t5516: move plaintext-password tests from t5601 and t5516 [2/2]: t5516/t5601: be less strict about the number of credential warnings t/t5516-fetch-push.sh | 31 --------------- t/t5551-http-fetch-smart.sh | 77 +++++++++++++++++++++++++++++++++++++ t/t5601-clone.sh | 23 ----------- 3 files changed, 77 insertions(+), 54 deletions(-) -Peff
On Mon, Oct 31, 2022 at 08:59:21PM -0400, Taylor Blau wrote: > > > test_expect_success LIBCURL 'fetch warns or fails when using username:password' ' > > > - message="URL '\''https://username:<redacted>@localhost/'\'' uses plaintext credentials" && > > > - test_must_fail git -c transfer.credentialsInUrl=allow fetch https://username:password@localhost 2>err && > > > + message="URL '\''https://username:<redacted>@224.0.0.1/'\'' uses plaintext credentials" && > > > + test_must_fail git -c transfer.credentialsInUrl=allow fetch https://username:password@224.0.0.1 2>err && > > > ! grep "$message" err && > > > > could be more robust. It would actually check that we succeeded in using > > the URL. > > It is magical, indeed. If it's significantly more difficult to move > these into t5550 or t5551, then I'm OK with this in the meantime (since > GitHub Actions really is our primary CI target that we care about this > not hanging on). > > But assuming that moving these around isn't that difficult, I would be > slightly happier to see these tests in one of the aforementioned spots. I don't think it was too difficult to move them. I just sent in patches (which I just realized you were not cc'd on). -Peff
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh index 79dc470c014..8dd4610a8c2 100755 --- a/t/t5516-fetch-push.sh +++ b/t/t5516-fetch-push.sh @@ -1854,32 +1854,32 @@ test_expect_success 'refuse to push a hidden ref, and make sure do not pollute t ' test_expect_success LIBCURL 'fetch warns or fails when using username:password' ' - message="URL '\''https://username:<redacted>@localhost/'\'' uses plaintext credentials" && - test_must_fail git -c transfer.credentialsInUrl=allow fetch https://username:password@localhost 2>err && + message="URL '\''https://username:<redacted>@224.0.0.1/'\'' uses plaintext credentials" && + test_must_fail git -c transfer.credentialsInUrl=allow fetch https://username:password@224.0.0.1 2>err && ! grep "$message" err && - test_must_fail git -c transfer.credentialsInUrl=warn fetch https://username:password@localhost 2>err && + test_must_fail git -c transfer.credentialsInUrl=warn fetch https://username:password@224.0.0.1 2>err && grep "warning: $message" err >warnings && test_line_count = 3 warnings && - test_must_fail git -c transfer.credentialsInUrl=die fetch https://username:password@localhost 2>err && + test_must_fail git -c transfer.credentialsInUrl=die fetch https://username:password@224.0.0.1 2>err && grep "fatal: $message" err >warnings && test_line_count = 1 warnings && - test_must_fail git -c transfer.credentialsInUrl=die fetch https://username:@localhost 2>err && + test_must_fail git -c transfer.credentialsInUrl=die fetch https://username:@224.0.0.1 2>err && grep "fatal: $message" err >warnings && test_line_count = 1 warnings ' test_expect_success LIBCURL 'push warns or fails when using username:password' ' - message="URL '\''https://username:<redacted>@localhost/'\'' uses plaintext credentials" && - test_must_fail git -c transfer.credentialsInUrl=allow push https://username:password@localhost 2>err && + message="URL '\''https://username:<redacted>@224.0.0.1/'\'' uses plaintext credentials" && + test_must_fail git -c transfer.credentialsInUrl=allow push https://username:password@224.0.0.1 2>err && ! grep "$message" err && - test_must_fail git -c transfer.credentialsInUrl=warn push https://username:password@localhost 2>err && + test_must_fail git -c transfer.credentialsInUrl=warn push https://username:password@224.0.0.1 2>err && grep "warning: $message" err >warnings && - test_must_fail git -c transfer.credentialsInUrl=die push https://username:password@localhost 2>err && + test_must_fail git -c transfer.credentialsInUrl=die push https://username:password@224.0.0.1 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 45f0803ed4d..0b386c74818 100755 --- a/t/t5601-clone.sh +++ b/t/t5601-clone.sh @@ -72,19 +72,19 @@ test_expect_success 'clone respects GIT_WORK_TREE' ' ' test_expect_success LIBCURL 'clone warns or fails when using username:password' ' - message="URL '\''https://username:<redacted>@localhost/'\'' uses plaintext credentials" && - test_must_fail git -c transfer.credentialsInUrl=allow clone https://username:password@localhost attempt1 2>err && + message="URL '\''https://username:<redacted>@224.0.0.1/'\'' uses plaintext credentials" && + test_must_fail git -c transfer.credentialsInUrl=allow clone https://username:password@224.0.0.1 attempt1 2>err && ! grep "$message" err && - test_must_fail git -c transfer.credentialsInUrl=warn clone https://username:password@localhost attempt2 2>err && + test_must_fail git -c transfer.credentialsInUrl=warn clone https://username:password@224.0.0.1 attempt2 2>err && grep "warning: $message" err >warnings && test_line_count = 2 warnings && - test_must_fail git -c transfer.credentialsInUrl=die clone https://username:password@localhost attempt3 2>err && + test_must_fail git -c transfer.credentialsInUrl=die clone https://username:password@224.0.0.1 attempt3 2>err && grep "fatal: $message" err >warnings && test_line_count = 1 warnings && - test_must_fail git -c transfer.credentialsInUrl=die clone https://username:@localhost attempt3 2>err && + test_must_fail git -c transfer.credentialsInUrl=die clone https://username:@224.0.0.1 attempt3 2>err && grep "fatal: $message" err >warnings && test_line_count = 1 warnings '