Let git-lfs HTTPS transport send cookies #3825

kupson · 2019-09-19T09:38:44Z

This allows Git LFS to use the same cookies as configured for Git
(http.cookieFile). Those cookies may be needed for e.g. Gcloud Identity-Aware
Proxy.

Cookies are sent only over HTTPS.

Fixes #3824

bk2204

Hey,,

Thanks for sending in this patch, and welcome to Git LFS! I think this is a great start. I'd like to see an integration test in the t/ directory so we don't regress this case, and I think there are some test failures on Windows that should be addressed.

For writing the test, it may be useful to add a special repository name to t/cmd/lfstest-gitserver.go that handles the special requirements of persisting a cookie properly. If you have any questions, I'm happy to answer them.

bk2204 · 2019-09-19T20:22:08Z

lfshttp/cookies.go

+}
+
+func getCookieJarForHost(c *Client, host string) (http.CookieJar, error) {
+cookieFile, _ := c.uc.Get("http", fmt.Sprintf("https://%v", host), "cookieFile")


This line doesn't seem to be indented properly. Could you run goimports or go fmt to make sure your code is tidy?

Should be fixed now.

bk2204 · 2019-09-19T20:24:32Z

lfshttp/cookies.go

+	"fmt"
+	"net/http"
+
+	"github.com/google/slothfs/cookie"


I assume we need this dependency because the builtin Go cookie jar can't read from a file?

Yes. Git-lfs should read the same file as git and libcurl - a parser for Netscape cookie jar file format is needed (https://curl.haxx.se/docs/http-cookies.html).

The format is quite simple and the parser can be added directly to lfshttp/cookies.go if you would like to avoid additional dependencies.

bk2204

Looking good; I found just one more thing that may be worth cleaning up.

I suspect you'll want to wait until #3833 is merged and then squash rebase them on top so that we can verify that the tests do indeed pass on Windows.

bk2204 · 2019-09-23T17:38:02Z

t/t-clone.sh

+
+  # golang net.http.Cookie ignores cookies with IP instead of domain/hostname
+  GITSERVER_SAVED="$GITSERVER"
+  GITSERVER=$(echo "$GITSERVER" | sed 's/127\.0\.0\.1/localhost/')


This is in a subshell, so you need not save the old version and restore it. Any variables you modify here are not persisted in other tests.

This should be fixed now.

slonopotamus · 2019-09-26T20:17:18Z

@kupson Please, rebase onto latest master to make sure tests pass on Windows

This allows Git LFS to use the same cookies as configured for Git (http.cookieFile). Those cookies may be needed for e.g. Gcloud Identity-Aware Proxy.

The script has to overwrite GITSERVER because golang http library refuses to accept cookies for 127.0.0.1.

The `t-credentials.sh` requires no credentials for localhost

bk2204

This looks good. Thanks again for the patch.

The original parser that was used in git-lfs#3825 brings in a lot of dependencies that complicate packaging git-lfs. This replaces it with a small parser I wrote with almost no dependencies. I've tested this as extensively as i can and it seems to work correctly.

In commit b9602f3 of PR git-lfs#5616 we resolved a problem which caused flaky test results in our CI jobs by ensuring that the t/t-credential.sh test script used a unique directory for the credential record files we provide to our git-credential-lfstest helper program. This change ensured that when the "clone (HTTP server/proxy require cookies)" test in our t/t-clone.sh script creates a record file named "localhost" in the common directory specified by the CREDSDIR environment variable, as a copy of the "127.0.0.1" file our setup_creds() shell library function creates, this does not conflict with the "credentials from netrc" test and other related tests in the t/t-credential.sh test script, which depend on there not existing a credential for the "localhost" hostname. However, a previous attempt to resolve this problem was already made in commits bb87944 and bcb9eb3 of PR git-lfs#3825, but assumed that our test scripts would always run sequentially rather than in parallel. In that PR, the "clone (HTTP server/proxy require cookies)" was added, and at the end of the test, it removes the "localhost" credential record file it creates at the beginning of the test. Of course, if the test ever failed, then it would leave the "localhost" file in place, so even if our test scripts always ran sequentially there would still be a conflict with the tests in the t/t-credential.sh script. We can more comprehensively ensure that these two test scripts do not conflict again in the future by using the same technique applied in commit b9602f3 to the t/t-credential.sh script in the t/t-clone.sh script as well. We therefore add a call to setup_creds() at the start of the t/t-clone.sh script, so it will also use a separate copy of the credential record files in the CREDSDIR directory. This means that the "localhost" file will only exist in a credential record directory used by the t/t-clone.sh script. We can then also drop the attempt to clean up the "localhost" file from the end of the "clone (HTTP server/proxy require cookies)" test. To make this change, we also need to slightly refactor our t/testhelpers.sh library so that the setup_creds() function can generate the appropriate credential record filenames on its own. At the moment, it depends on some local variables defined in the setup() function, but these are used nowhere else except in the setup_creds() function, so we can just move their values directly into the strings concatenated and passed to the write_creds_path() function by the setup_creds() function.

In commit b9602f3 of PR git-lfs#5616 we resolved a problem which caused flaky test results in our CI jobs by ensuring that the t/t-credential.sh test script used a unique directory for the credential record files we provide to our git-credential-lfstest helper program. This change ensured that when the "clone (HTTP server/proxy require cookies)" test in our t/t-clone.sh script creates a record file named "localhost" in the common directory specified by the CREDSDIR environment variable, specifically a copy of the "127.0.0.1" file created by the setup_creds() shell library function, this does not conflict with the "credentials from netrc" test and other related tests in the t/t-credential.sh test script, which depend on there being no credential records associated with the "localhost" hostname. However, we already made a previous attempt to resolve this problem in commits bb87944 and bcb9eb3 of PR git-lfs#3825, but that change assumes that our test scripts always run sequentially rather than in parallel. In PR git-lfs#3825 the "clone (HTTP server/proxy require cookies)" test was added, and at the end of the test, it removes the "localhost" credential record file it creates at the beginning of the test. Of course, if the test ever fails, then it leaves the "localhost" record file in place, so even if our test scripts always ran sequentially there would still be a potential conflict with the tests in the t/t-credential.sh script. We can more comprehensively ensure that these two test scripts do not conflict again in the future by using the same technique applied in commit b9602f3 to the t/t-credential.sh script in the t/t-clone.sh script as well. First, we add a call to setup_creds() at the start of the t/t-clone.sh script after setting the CREDSDIR environment variable to a path unique to the t/t-clone.sh script. This ensures that a separate copy of the default credential record file for the 127.0.0.1 hostname is available for the exclusive use of the tests in the script. Next, we move the creation of the credential record files associated with the TLS/SSL client certificate used in the "clone ClientCert" test from the generic setup_creds() function into the test itself. We would otherwise need to define the "certpath" and "keypath" variables, as their values are interpolated by the function into the names of these credential record files. The TLS/SSL client certificate and key files are generated by our lfstest-gitserver utility when it is first executed. Their locations are defined by the LFSTEST_CLIENT_* environment variables passed by the setup() function in our t/testhelpers.sh shell library to the lfstest-count-tests utility, which then runs the lfstest-gitserver program. The values of these environment variables are set from the LFS_CLIENT_CERT_FILE and LFS_CLIENT_KEY_FILE* variables defined by our t/testenv.sh script. We now use these variables in the "clone ClientCert" test when we call the write_creds_file() function directly, instead of allowing the setup_creds() function to perform those calls. We could define the "certpath" and "keypath" variables (using the LFS_CLIENT_CERT_FILE and LFS_CLIENT_KEY_FILE_ENCRYPTED variables) prior to calling setup_creds() at the start of the t/t-clone.sh script. However, the "clone ClientCert" test is the only one which actually makes use of these credential record files, as this is the only test which actively checks the use of an encrypted TLS/SSL client certificate, and so is the only place where we need to create these record files. Further, we can also move into the test the "git config" commands that set the "http.<url>.sslCert" and "http.<url>.sslKey" Git configuration options with the locations of the TLS/SSL client certificate files. Previously, we set these configuration options in the setup() function in our t/testhelpers.sh shell library, so they were configured for all tests in all test scripts, although only the "clone ClientCert" test makes use of them. Finally, the "clone (HTTP server/proxy require cookies)" test no longer needs to attempt to delete the credential record file for the "localhost" hostname after the test is complete, so we can simply remove that code. Note that the "clone ClientCert" test was first introduced in commit daba49a of PR git-lfs#1893, at which time the "git config" commands to set the "http.<url>.sslCert" and "http.<url>.sslKey" options were added to the setup() function, and the LFS_CLIENT_CERT_FILE and LFS_CLIENT_KEY_FILE variables were defined in what was then the test/testenv.sh script. Later, in commit 52f94c2 of PR git-lfs#3270, the LFS_CLIENT_KEY_FILE_ENCRYPTED variable was added along with support in the lfstest-gitserver program to generate an encrypted certificate key file. In another commit in PR git-lfs#3270, commit 706beca, the "clone ClientCert" test was then expanded to validate use of the encrypted key file with the "git lfs clone" command. (Note too that the following test, the "clone ClientCert with homedir certs" test, appears to also depend on credential record files for the TLS/SSL client certificate files it creates in the dedicated home directory used by our tests. However, because this test does not set the "http.sslCertPasswordProtected" Git configuration option or the GIT_SSL_CERT_PASSWORD_PROTECTED environment variable, Git does not attempt to retrieve a passphrase for the certificate files, and so the associated credential record files are never actually used. We will address this issue in a subsequent commit in this PR.)

In commit b9602f3 of PR git-lfs#5616 we resolved a problem which caused flaky test results in our CI jobs by ensuring that the t/t-credential.sh test script used a unique directory for the credential record files we provide to our git-credential-lfstest helper program. This change ensured that when the "clone (HTTP server/proxy require cookies)" test in our t/t-clone.sh script creates a record file named "localhost" in the common directory specified by the CREDSDIR environment variable, specifically a copy of the "127.0.0.1" file created by the setup_creds() shell library function, this does not conflict with the "credentials from netrc" test and other related tests in the t/t-credential.sh test script, which depend on there being no credential records associated with the "localhost" hostname. However, we already made a previous attempt to resolve this problem in commits bb87944 and bcb9eb3 of PR git-lfs#3825, but that change assumes that our test scripts always run sequentially rather than in parallel. In PR git-lfs#3825 the "clone (HTTP server/proxy require cookies)" test was added, and at the end of the test, it removes the "localhost" credential record file it creates at the beginning of the test. Of course, if the test ever fails, then it leaves the "localhost" record file in place, so even if our test scripts always ran sequentially there would still be a potential conflict with the tests in the t/t-credential.sh script. We can more comprehensively ensure that these two test scripts do not conflict again in the future by using the same technique applied in commit b9602f3 to the t/t-credential.sh script in the t/t-clone.sh script as well. First, we add a call to setup_creds() at the start of the t/t-clone.sh script after setting the CREDSDIR environment variable to a path unique to the t/t-clone.sh script. This ensures that a separate copy of the default credential record file for the 127.0.0.1 hostname is available for the exclusive use of the tests in the script. Next, we move the creation of the credential record files associated with the TLS/SSL client certificate used in the "clone ClientCert" test from the generic setup_creds() function into the test itself. We would otherwise need to define the "certpath" and "keypath" variables, as their values are interpolated by the function into the names of these credential record files. The TLS/SSL client certificate and key files are generated by our lfstest-gitserver utility when it is first executed. Their locations are defined by the LFSTEST_CLIENT_* environment variables passed by the setup() function in our t/testhelpers.sh shell library to the lfstest-count-tests utility, which then runs the lfstest-gitserver program. The values of these environment variables are set from the LFS_CLIENT_CERT_FILE and LFS_CLIENT_KEY_FILE* variables defined by our t/testenv.sh script. We now use these variables in the "clone ClientCert" test when we call the write_creds_file() function directly, instead of allowing the setup_creds() function to perform those calls. We could define the "certpath" and "keypath" variables (using the LFS_CLIENT_CERT_FILE and LFS_CLIENT_KEY_FILE_ENCRYPTED variables) prior to calling setup_creds() at the start of the t/t-clone.sh script. However, the "clone ClientCert" test is the only one which actually makes use of these credential record files, as this is the only test which actively checks the use of an encrypted TLS/SSL client certificate, and so is the only place where we need to create these record files. Further, we can also move into the test the "git config" commands that set the "http.<url>.sslCert" and "http.<url>.sslKey" Git configuration options with the locations of the TLS/SSL client certificate files, and we do the same for the "create lock with server using client cert" test in the t/t-lock.sh test script. Previously, we set these configuration options in the setup() function in our t/testhelpers.sh shell library, so they were configured for all tests in all test scripts, although only the "clone ClientCert" test and the "create lock with server using client cert" test make use of them. Finally, the "clone (HTTP server/proxy require cookies)" test no longer needs to attempt to delete the credential record file for the "localhost" hostname after the test is complete, so we can simply remove that code. Note that the "clone ClientCert" test was first introduced in commit daba49a of PR git-lfs#1893, at which time the "git config" commands to set the "http.<url>.sslCert" and "http.<url>.sslKey" options were added to the setup() function, and the LFS_CLIENT_CERT_FILE and LFS_CLIENT_KEY_FILE variables were defined in what was then the test/testenv.sh script. Later, in commit 52f94c2 of PR git-lfs#3270, the LFS_CLIENT_KEY_FILE_ENCRYPTED variable was added along with support in the lfstest-gitserver program to generate an encrypted certificate key file. In another commit in PR git-lfs#3270, commit 706beca, the "clone ClientCert" test was then expanded to validate use of the encrypted key file with the "git lfs clone" command. (Note too that the following test, the "clone ClientCert with homedir certs" test, appears to also depend on credential record files for the TLS/SSL client certificate files it creates in the dedicated home directory used by our tests. However, because this test does not set the "http.sslCertPasswordProtected" Git configuration option or the GIT_SSL_CERT_PASSWORD_PROTECTED environment variable, Git does not attempt to retrieve a passphrase for the certificate files, and so the associated credential record files are never actually used. We will address this issue in a subsequent commit in this PR.)

bk2204 requested changes Sep 19, 2019

View reviewed changes

kupson requested a review from bk2204 September 23, 2019 06:40

bk2204 reviewed Sep 23, 2019

View reviewed changes

Rafal Kupka and others added 5 commits September 28, 2019 19:02

Let git-lfs HTTP transport send cookies

aa7ac8f

This allows Git LFS to use the same cookies as configured for Git (http.cookieFile). Those cookies may be needed for e.g. Gcloud Identity-Aware Proxy.

Fix indentation

9b521d9

Add http.cookieFile test

bb87944

The script has to overwrite GITSERVER because golang http library refuses to accept cookies for 127.0.0.1.

Don't ignore errors from getCookieJarForHost()

a1a38cb

Allow tilde expansion in http.cookieFile config option

bb0adf4

kupson force-pushed the add-cookiefile branch from fd91e1e to bb0adf4 Compare September 28, 2019 18:02

kupson added 2 commits September 29, 2019 09:01

It'a subshell - no need to save GITSERVER

a07cd38

Add comment about "rm $CREDSDIR/localhost"

bcb9eb3

The `t-credentials.sh` requires no credentials for localhost

kupson requested a review from bk2204 September 29, 2019 09:03

bk2204 approved these changes Sep 30, 2019

View reviewed changes

bk2204 merged commit e132c34 into git-lfs:master Sep 30, 2019

ssgelm mentioned this pull request Oct 22, 2019

Possible to use github.com/google/slothfs alternative? #3870

Closed

ssgelm mentioned this pull request Oct 27, 2019

Use different parser for cookiejar files #3886

Merged

axel3rd mentioned this pull request Nov 29, 2019

Improve "Basic Authentication" behind reverse-proxy #3935

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Let git-lfs HTTPS transport send cookies #3825

Let git-lfs HTTPS transport send cookies #3825

kupson commented Sep 19, 2019

bk2204 left a comment

bk2204 Sep 19, 2019

kupson Sep 23, 2019

bk2204 Sep 19, 2019

kupson Sep 23, 2019

bk2204 left a comment

bk2204 Sep 23, 2019

kupson Sep 29, 2019

slonopotamus commented Sep 26, 2019

bk2204 left a comment

Let git-lfs HTTPS transport send cookies #3825

Let git-lfs HTTPS transport send cookies #3825

Conversation

kupson commented Sep 19, 2019

bk2204 left a comment

Choose a reason for hiding this comment

bk2204 Sep 19, 2019

Choose a reason for hiding this comment

kupson Sep 23, 2019

Choose a reason for hiding this comment

bk2204 Sep 19, 2019

Choose a reason for hiding this comment

kupson Sep 23, 2019

Choose a reason for hiding this comment

bk2204 left a comment

Choose a reason for hiding this comment

bk2204 Sep 23, 2019

Choose a reason for hiding this comment

kupson Sep 29, 2019

Choose a reason for hiding this comment

slonopotamus commented Sep 26, 2019

bk2204 left a comment

Choose a reason for hiding this comment