Conversation
sslKey and sslCert in http subsection
|
I have accepted the "Contributor License Agreement" now... |
technoweenie
left a comment
There was a problem hiding this comment.
This looks good (minus my really minor style issues), but I really want an integration test for this too. Can you provide instructions on building a sample cert for this?
Our integration tests (the bash files in the test dir) do run against a local SSL web server, which is configured in test/cmd/lfstest-gitserver.go, add in #1067.
It'd be great if you could take a stab at this. But I understand if you can't. Can you just make sure the "Allow edits from maintainers" checkbox is checked for this PR?
lfsapi/certs.go
Outdated
| // isClientCertEnabledForHost returns whether client certificate | ||
| // are configured for the given host | ||
| func isClientCertEnabledForHost(c *Client, host string) bool { | ||
|
|
There was a problem hiding this comment.
I apologize for the nitpicky review comments, but white space after { and before } really bugs me.
lfsapi/certs.go
Outdated
| // getClientCertForHost returns a client certificate for a specific host (which may | ||
| // be "host:port" loaded from the gitconfig | ||
| func getClientCertForHost(c *Client, host string) tls.Certificate { | ||
|
|
There was a problem hiding this comment.
Another unnecessary blank line.
lfsapi/client.go
Outdated
|
|
||
| if isClientCertEnabledForHost(c, host) { | ||
|
|
||
| tracerx.Printf("using certs ...") |
There was a problem hiding this comment.
Two things:
- Can you remove the blank space above the tracerx line, and below the
BuildNameToCertificate()line? - Can you change the tracerx message to:
tracerx.Printf("http: client cert for %s", host)
ttaylorr
left a comment
There was a problem hiding this comment.
Pending mine and @technoweenie's comments, this looks 👍
lfsapi/certs.go
Outdated
| _, hostSslCertOk := c.gitEnv.Get(fmt.Sprintf("http.https://%v/.sslCert", host)) | ||
| _, hostSslCaInfoOk := c.gitEnv.Get(fmt.Sprintf("http.https://%v/.sslCaInfo", host)) | ||
|
|
||
| if hostSslKeyOk && hostSslCertOk && hostSslCaInfoOk { |
There was a problem hiding this comment.
I would recommend simplifying this to:
return hostSslKeyOk && hostSslCertOk && hostSslCaInfoOk| // be "host:port" loaded from the gitconfig | ||
| func getClientCertForHost(c *Client, host string) tls.Certificate { | ||
|
|
||
| hostSslKey, _ := c.gitEnv.Get(fmt.Sprintf("http.https://%v/.sslKey", host)) |
There was a problem hiding this comment.
Before fetching these values, do you think it would be good to call isClientCertEnabledForHost with the c and host args to bail out early if certificates aren't enabled for that given host?
There was a problem hiding this comment.
That happens in lfsapi/client.go already. I don't think we need to call isClientCertEnabledForHost() twice.
There was a problem hiding this comment.
Ah, great catch -- thanks!
|
I added an integration test based on this work: |
|
I have seen this problem before at our real server.I had to go back to release version 1.5.5.
|
|
What are you referring to specifically?
You can ignore this. It has to do with file locking. More info at #1884
This should be covered by this PR. I'll do some digging. Thanks for adding the integration test! |
|
I'm unable to get this working locally: This exception is coming from Good news is that this fails exactly the same way in Linux (the Travis CI builds) and Windows (AppVeyor). The clone test you added is a good one, because that command is testing that both git and git-lfs are reading the same git config keys to setup the ssl client cert. |
|
on os x you have to install homebrew openssl git and curl versions. |
|
Not much luck on my end. I did write a test using the new locking API: diff --git a/test/test-lock.sh b/test/test-lock.sh
index c5bf135e..93cc8695 100755
--- a/test/test-lock.sh
+++ b/test/test-lock.sh
@@ -17,6 +17,21 @@ begin_test "creating a lock"
)
end_test
+begin_test "create lock with server using client cert"
+(
+ set -e
+ reponame="lock_create_client_cert"
+ setup_remote_repo_with_file "$reponame" "a.dat"
+
+ git config lfs.url "$CLIENTCERTGITSERVER/$reponame.git/info/lfs"
+ GITLFSLOCKSENABLED=1 git lfs lock "a.dat" | tee lock.log
+ grep "'a.dat' was locked" lock.log
+
+ id=$(grep -oh "\((.*)\)" lock.log | tr -d "()")
+ assert_server_lock "$reponame" "$id"
+)
+end_test
+
begin_test "creating a lock (--json)"
(
set -eThe difference between this and the |
|
I added your locking-test, but it fails:
|
|
Ah, can you change the filename from |
technoweenie
left a comment
There was a problem hiding this comment.
It's interesting that this requires git config --global http.$LFS_CLIENT_CERT_URL/.sslVerify "false". Since it only affects $LFS_CLIENT_CERT_URL, I suppose it's alright. I'm assuming that the config value is disabling server SSL verification in git/git-lfs, but still sending the client cert to $LFS_CLIENT_CERT_URL. Tests seem to fail if I take out the sslKey or sslCert config values.
I'd really like to get rid of the sslVerify setting, but I'm confident that it only affects the 2 tests added in this PR.
ttaylorr
left a comment
There was a problem hiding this comment.
I'd really like to get rid of the
sslVerifysetting, but I'm confident that it only affects the 2 tests added in this PR.
I agree. I think this looks good to be merged into master, thanks again for your contribution @schuCriX. 👍
| Subject: pkix.Name{Organization: []string{"Yhat, Inc."}}, | ||
| SignatureAlgorithm: x509.SHA256WithRSA, | ||
| NotBefore: time.Now(), | ||
| NotAfter: time.Now().Add(time.Hour), // valid for an hour |
There was a problem hiding this comment.
This could cause issues with DST transitions, but I'm not worried about it since the likelihood of it happening is so low.
|
the go http test server uses a hardcoded certificate for https. it is defined in the internal module. the certificate is issued to an empty server name. the newest git client on travis ci test fails on validating this empty name against 127.0.0.1. so we have several ways to handle this:
thanks.... |
|
@schuCriX: I've merged this into master. The EDIT: See #1893 for the actual merge. Thanks again for your work on this! |
In commit 9a00eb3 of PR git-lfs#1893 we altered our "clone ClientCert" test of the Git LFS client's support for TLS/SSL client certificates to deal with an exception raised by the version of libcurl provided by macOS at the time. As was reported in curl/curl#1105, the libcurl installed as part of macOS 10.12 ("Sierra") failed to find some TLS/SSL client certificates and instead raised an NSInvalidArgumentException. See the notes in our PR git-lfs#1869 for more details: git-lfs#1869 (comment) To work around this issue, we skip the "clone ClientCert" test if any NSInvalidArgumentException messages are found in the output of the Git clone operation performed by the clone_repo_clientcert() function of our t/testhelpers.sh shell library. The same workaround was then copied into the "clone ClientCert with homedir certs" test when it was added to our t/t-clone.sh test script in commit c54c9da of PR git-lfs#5657. The underlying problem in libcurl was resolved by the changes in commit curl/curl@7c9b9ad, which was included with libcurl version 7.52.0 and so is available in all contemporary versions of macOS. For instance, by the time of macOS 10.14 ("Mojave"), the version of libcurl packaged with the OS was v7.54.0. We can therefore drop our workarounds, as they are no longer applicable to any supported versions of macOS. (Note, too, that the GitHub Actions runners we now use for our CI jobs provide a libcurl version installed by Homebrew, which takes precedence over the one supplied by macOS.)
allow for sslKey and sslCert in gitconfig http subsection