lfshttp: let Go find macOS root CAs for TLS certs

In commit 8009d17 of PR git-lfs#1787, which
was then merged into our main development branch in PR git-lfs#1839, we added
support for the use of custom TLS/SSL certificates as may be specified
with the "http.<url>.sslCAInfo" and "http.sslCAPath" Git configuration
options or the GIT_SSL_CAINFO and GIT_SSL_CAPATH environment variables.

In the same commit we also introduced logic to ensure that on macOS,
TLS/SSL certificates were validated against a set of root certificate
authorities which included those registered in the System keychain
and not just those in the SystemRootCertificates keychain, as these
may contain distinct sets of root certificate authorities.

The HttpClient() method of our Client structure in our "lfshttp" package
initializes a Client structure from the Go library's "net/http" package,
which we then use to make HTTP requests to a given remote host.  We
populate the Transport element of the "net/http" package's Client
structure with a Transport structure we initialize in the Transport()
method of our own Client structure.

Our Transport() method sets the TLSClientConfig element of the new
Transport structure to a new Config structure, from the Go library's
"crypto/tls" package, which we also initialize in our Transport()
method.  This TLS configuration structure then controls how the TLS/SSL
verification is performed for the HTTP requests we make to a given
URL with the "net/http" package's Client structure.

In particular, the RootCAs element of the "crypto/tls" package's Config
structure defines the pool of root certificate authorities which will be
used to verify the TLS/SSL certificate presented by the HTTP server.
Our Transport() method sets the RootCAs element to the value returned
by the getRootCAsForHost() function, which in turn calls two functions,
appendRootCAsForHostFromGitconfig() and appendRootCAsForHostFromPlatform().

The first of these two functions, appendRootCAsForHostFromGitconfig(),
searches for valid TLS/SSL certificate files in any of the locations
specified by the GIT_SSL_CAINFO environment variable, the
"http.<url>.sslCAInfo" Git configuration option, the GIT_SSL_CAPATH
environment variable, and the "http.sslCAPath" Git configuration option,
in that order.  If one of these settings has a defined value, then it
preempts the others, even if no valid certificate files are found at
the location it specifies.  If valid certificates are found, their
contents are returned in PEM format, which our getRootCAsForHost()
function adds to the pool of certificate it will return.

In addition to the foregoing, however, the getRootCAsForHost() function
in our "lfshttp" package then calls appendRootCAsForHostFromPlatform(),
which may add other certificates to the pool that will be returned to
the Transport() method.  As noted, these will be used as the value of
the RootCAs element of the Config structure from the "crypto/tls" package,
and are therefore included in the set of root certificate authorities
allowed to verify the TLS/SSL certificate offered by the server in
response to our HTTP requests.

The appendRootCAsForHostFromPlatform() function is platform-specific,
with three different versions, but only the one for macOS systems is
non-trivial; the others simply return their input without alteration.

On macOS, the appendRootCAsForHostFromPlatform() function runs the
"/usr/bin/security list-keychains" command, extracts the path to the
System keychain, and the runs the "/usr/bin/security find-certificate"
command to look for certificates in the System keychain whose names
match the hostname from the URL for which we are constructing an
HTTP client.  If any are found, their PEM data is appended to that
found by the appendRootCAsForHostFromGitconfig() function (if any),
and so these certificates are also included among those used to
verify the server certificate in our HTTP requests.

This extra step, which is unique to macOS, was included in commit
8009d17 because at the time of PRs git-lfs#1787
and git-lfs#1839 we built the Git LFS client with Go v1.7.4, and depending on
the version of macOS in use and how Go was compiled, the Verify() method
of the Certificate structure in the "crypto/x509" package did not always
read root certificates from the System keychain.

The Go standard library's "crypto/tls" package performs the required
steps in a TLS/SSL handshake when an HTTP request is made with a
Client from the "net/http" package.  One of these steps involves the
verification of the server's certificate, which is done with a call
to the Verify() method of the "crypto/x509" package's Certificate
structure, passing in the root certificate authorities from the
RootCAs element of the Config structure.  The Verify() method, in
turn, depends on the pool of root certificates loaded by the internal
loadSystemRoots() function:

  https://github.com/golang/go/blob/go1.7.4/src/crypto/tls/handshake_client.go#L287-L304
  https://github.com/golang/go/blob/go1.7.4/src/crypto/x509/verify.go#L247
  https://github.com/golang/go/blob/go1.7.4/src/crypto/x509/root.go#L15-L22

Until Go v1.16, the "crypto/x509" package contained two separate
implementations of its loadSystemRoots() function.  When Go was compiled
with "cgo" support, so as to permit the use of C code, the function called
a C language function named FetchPEMRoots(), which could retrieve
certificates from the SystemRootCertificates keychain but also the
System and "login" keychains (known as the System, Admin, and User
trust setting domains, respectively), but only on systems with macOS
10.9 (Mavericks) and higher installed:

  https://github.com/golang/go/blob/go1.7.4/src/crypto/x509/root_cgo_darwin.go#L212
  https://github.com/golang/go/blob/go1.7.4/src/crypto/x509/root_cgo_darwin.go#L85-L90

On older macOS systems, the FetchPEMRoots() function simply called the
FetchPEMRoots_MountainLion() function, which only returned certificates
from the SystemRootCertificates keychain:

  https://github.com/golang/go/blob/go1.7.4/src/crypto/x509/root_cgo_darwin.go#L81-L83
  https://github.com/golang/go/blob/go1.7.4/src/crypto/x509/root_cgo_darwin.go#L19-L55

Alternatively, if Go was compiled without "cgo" support, then the
loadSystemRoots() function called an execSecurityRoots() function, which
invoked the "/usr/bin/security find-certificate" command on the
SystemRootCertificates keychain:

  https://github.com/golang/go/blob/go1.7.4/src/crypto/x509/root_nocgo_darwin.go#L10
  https://github.com/golang/go/blob/go1.7.4/src/crypto/x509/root_darwin.go#L31

These divergent implementations emerged from initial efforts to support
the verification of server TLS/SSL certificates against the System and
"login" keychains as well as the SystemRootCertificates keychain.  The
golang/go#14514 and golang/go#16532 issues reported the lack of support
for the System and "login" keychains in Go v1.6.x, and so in commit
golang/go@6cd698d the "cgo" implementation
was updated to load certificates from all three keychains.

Unfortunately, as reported in golang/go#16473, this led to crashes on
older macOS systems, and so the FetchPEMRoots_MountainLion() function was
introduced in commit golang/go@ff60da6
to restore the previous behaviour on systems with macOS 10.8 (Mountain
Lion) or earlier versions of macOS.  As a result, on these systems,
even if Go was compiled with "cgo" enabled, only root certificate
authorities from the SystemRootCertificates keychain would be checked
when attempting to verify a server's TLS/SSL certificate.

Both changes to the "cgo" version of the "crypto/x509" package were
included in Go v1.7.  However, the non-"cgo" version of the package still
only called the "/usr/bin/security find-certificate" command on the
SystemRootCertificates keychain.  The execSecurityRoots() function was
not updated to also run the "/usr/bin/security find-certificate" command
on the System and "login" keychains until commit
golang/go@4dbcacd, which was included
in the Go v1.9 release.

Subsequently, these two versions were refactored extensively in commit
golang/go@6f52790, which was part
of the Go v1.15 release.  The non-"cgo" version was removed entirely,
meaning the Go standard library no longer called the
"/usr/bin/security find-certificate" command at all.  The "cgo"
implementation was retained for testing purposes but ultimately also
removed in commit golang/go@5e18135 of
Go v1.16.  In this revised implementation, the loadSystemRoots() function
continued to load certificates from all three keychains, as before.

Further refinements have been made to this code since then, including
the consolidation of AMD64 and ARM64 versions, and another major
refactoring in commit golang/go@feb024f,
which followed the design from golang/go#46287.  In that commit, the
loadSystemRoots() function to a simple wrapper, the Verify() method was
altered to just run the systemVerify() method for macOS systems when no
custom root certificates were supplied by the caller, as was already the
case for Windows systems.  As well, the systemVerify() method was
expanded from a no-op call into the method which performs the principal
certificate verification steps, making use of the new TLS/SSL verification
APIs available with all the versions of macOS supported by Go at the time.
One of these API calls, in particular, is the SecTrustEvaluateWithError()
function, which ultimately determines if a given certificate is accepted
or not, per Apple's documentation:

  https://developer.apple.com/documentation/security/sectrustevaluatewitherror(_:_:)

We now build the Git LFS client using at least Go v1.23.x, and in
most cases we benefit from these improvements to the Go standard
library's TLS/SSL verification logic on macOS systems.  If the
user provides their own certificates to be used as root certificate
authorities with either the GIT_SSL_* environment variables or the
"http.<url>.sslCAInfo" or "http.sslCAPath" configuration options,
these will be checked by the Verify() method of the "crypto/x509"
package's Certificate structure in preference to any certificates
provided by the system, which is our desired behaviour.

In this case, as noted above, because we have defined the RootCAs element
of the Config structure from the "crypto/tls" package that we set as the
TLSClientConfig element of the Transport structure for our HTTP client,
the RootCAs element will be passed to the Verify() method of the
"crypto/x509" package's Certificate structure as the Roots element
of the VerifyOptions structure.  However, this Roots element, which is
a CertPool structure, will have its internal systemPool flag set to
"false".  As a consequence, because the VerifyOptions structure's Roots
element is non-nil but that Roots element's systemPool flag is "false",
the Verify() method will not call the platform-specific systemVerify()
method at all, so only the certificates the user provided will be used
to verify the server's TLS/SSL certificate:

  https://github.com/golang/go/blob/go1.23.0/src/crypto/tls/handshake_client.go#L1099-L1114
  https://github.com/golang/go/blob/go1.23.0/src/crypto/x509/cert_pool.go#L31-L35
  https://github.com/golang/go/blob/go1.23.0/src/crypto/x509/verify.go#L771-L785

If the user does not specify any of the GIT_SSL_* environment variables
or the "http.<url>.sslCAInfo" or "http.sslCAPath" configuration options,
then in general we will not define the RootCAs element of the "crypto/tls"
package's Config structure, and the Verify() method of the "crypto/x509"
package's Certificate structure will proceed to use its platform-specific
systemVerify() method on macOS and Windows.

Even if a macOS user's System keychain contains certificates, as is
common in enterprise environments, so long as none of them contain a
certificate whose name matches the server hostname from the HTTP
request's URL, our appendRootCAsForHostFromPlatform() function will
not append any of the keychain's certificates to the CertPool it returns,
and so we will not define the RootCAs element, and hence the "crypto/x509"
package's platform- specific certificate verification logic will be used.

However, as reported in git-lfs#6036, if a macOS user's System keychain does
happen to contain a certificate which matches the server hostname from
the HTTP request's URL, our appendRootCAsForHostFromPlatform() function
will append that certificate to the CertPool it returns, which will
then cause the Verify() method of the "crypto/x509" package's Certificate
structure to skip the platform-specific verification logic and try to
verify the server's TLS/SSL certificate against just the one we have
provided from the System keychain.  Should that not actually be a
root certificate authority which can verify the server's certificate,
the HTTP request will be rejected, which is not the behaviour we
expect or intend.

To resolve this problem and also simplify our HTTP client setup code
on macOS, we can just remove the workaround introduced in commit
8009d17 back in 2016.  We first
eliminate our appendRootCAsForHostFromPlatform() functions entirely,
including both the no-op versions for Linux and Windows, and the
macOS version which invoked the "/usr/bin/security list-keychains"
and "/usr/bin/security find-certificate" commands.

We then drop the getRootCAsForHost() function in our "lfshttp" package,
rename the appendRootCAsForHostFromGitconfig() function to
getRootCAsForHostFromGitconfig(), and revise its input arguments to
match those of the former getRootCAsForHost() function.

As well as making our code simpler, these changes mean that we will
also no longer run the "/usr/bin/security" command at all on macOS
systems, which should improve our performance on those systems.

Our Go tests which validate our support of the GIT_SSL_* environment
variables and the "http.<url>.sslCAInfo" or "http.sslCAPath" configuration
options still pass as expected, once we adjust the name of the function
they call.

Designing a test which could validate the Git LFS client's behaviour
on macOS when certificates are installed in the System keychain,
though, would be very challenging.  To quote from commit
golang/go@feb024f, which implemented
the Go language's proposal from golang/go#46287:

  Unfortunately there is not a clean way to programmatically add test
  roots to the system trust store that the builders would tolerate. The
  most obvious solution, using 'security add-trusted-cert' requires human
  interaction for authorization.

Instead, we rely on the Go project's own testing of their support for
system verification of HTTP server's TLS/SSL certificates, which should
be sufficient for our project as well.

Author: chrisd8088

lfshttp/certs.go

@@ -115,24 +115,17 @@ func getClientCertForHost(c *Client, host string) (*tls.Certificate, error) {
 	return &certobj, nil
 }
 
-// getRootCAsForHost returns a certificate pool for that specific host (which may
-// be "host:port" loaded from either the gitconfig or from a platform-specific
-// source which is not included by default in the golang certificate search)
+// getRootCAsForHostFromGitconfig returns a certificate pool for that
+// specific host (which may be "host:port" or just "host") loaded from the
+// Git configuration and environment settings, if any are defined.
 // May return nil if it doesn't have anything to add, in which case the default
 // RootCAs will be used if passed to TLSClientConfig.RootCAs
-func getRootCAsForHost(c *Client, host string) *x509.CertPool {
+func getRootCAsForHostFromGitconfig(c *Client, host string) *x509.CertPool {
 	// don't init pool, want to return nil not empty if none found; init only on successful add cert
 	var pool *x509.CertPool
 
-	// gitconfig first
-	pool = appendRootCAsForHostFromGitconfig(c.osEnv, c.gitEnv, pool, host)
-	// Platform specific
-	return appendRootCAsForHostFromPlatform(pool, host)
-}
-
-func appendRootCAsForHostFromGitconfig(osEnv, gitEnv config.Environment, pool *x509.CertPool, host string) *x509.CertPool {
 	url := fmt.Sprintf("https://%v/", host)
-	uc := config.NewURLConfig(gitEnv)
+	uc := config.NewURLConfig(c.gitEnv)
 
 	backend, _ := uc.Get("http", url, "sslbackend")
 	schannelUseSslCaInfoStrValue, _ := uc.Get("http", url, "schannelusesslcainfo")
@@ -145,19 +138,19 @@ func appendRootCAsForHostFromGitconfig(osEnv, gitEnv config.Environment, pool *x
 	// Accumulate certs from all these locations:
 
 	// GIT_SSL_CAINFO first
-	if cafile, _ := osEnv.Get("GIT_SSL_CAINFO"); len(cafile) > 0 {
+	if cafile, _ := c.osEnv.Get("GIT_SSL_CAINFO"); len(cafile) > 0 {
 		return appendCertsFromFile(pool, cafile)
 	}
 	// http.<url>/.sslcainfo or http.<url>.sslcainfo
 	if cafile, ok := uc.Get("http", url, "sslcainfo"); ok {
 		return appendCertsFromFile(pool, cafile)
 	}
 	// GIT_SSL_CAPATH
-	if cadir, _ := osEnv.Get("GIT_SSL_CAPATH"); len(cadir) > 0 {
+	if cadir, _ := c.osEnv.Get("GIT_SSL_CAPATH"); len(cadir) > 0 {
 		return appendCertsFromFilesInDir(pool, cadir)
 	}
 	// http.sslcapath
-	if cadir, ok := gitEnv.Get("http.sslcapath"); ok {
+	if cadir, ok := c.gitEnv.Get("http.sslcapath"); ok {
 		return appendCertsFromFilesInDir(pool, cadir)
 	}
 

lfshttp/certs_darwin.go

@@ -73,7 +73,7 @@ func TestCertFromSSLCAInfoConfig(t *testing.T) {
 		assert.Nil(t, err)
 
 		for _, matchedHostTest := range sslCAInfoMatchedHostTests {
-			pool := getRootCAsForHost(c, matchedHostTest.hostName)
+			pool := getRootCAsForHostFromGitconfig(c, matchedHostTest.hostName)
 
 			var shouldOrShouldnt string
 			if matchedHostTest.shouldMatch {
@@ -96,7 +96,7 @@ func TestCertFromSSLCAInfoConfig(t *testing.T) {
 
 	// Should match any host at all
 	for _, matchedHostTest := range sslCAInfoMatchedHostTests {
-		pool := getRootCAsForHost(c, matchedHostTest.hostName)
+		pool := getRootCAsForHostFromGitconfig(c, matchedHostTest.hostName)
 		assert.NotNil(t, pool)
 	}
 }
@@ -117,7 +117,7 @@ func TestCertFromSSLCAInfoEnv(t *testing.T) {
 
 	// Should match any host at all
 	for _, matchedHostTest := range sslCAInfoMatchedHostTests {
-		pool := getRootCAsForHost(c, matchedHostTest.hostName)
+		pool := getRootCAsForHostFromGitconfig(c, matchedHostTest.hostName)
 		assert.NotNil(t, pool)
 	}
 }
@@ -140,7 +140,7 @@ func TestCertFromSSLCAInfoEnvIsIgnoredForSchannelBackend(t *testing.T) {
 
 	// Should match any host at all
 	for _, matchedHostTest := range sslCAInfoMatchedHostTests {
-		pool := getRootCAsForHost(c, matchedHostTest.hostName)
+		pool := getRootCAsForHostFromGitconfig(c, matchedHostTest.hostName)
 		assert.Nil(t, pool)
 	}
 }
@@ -164,7 +164,7 @@ func TestCertFromSSLCAInfoEnvWithSchannelBackend(t *testing.T) {
 
 	// Should match any host at all
 	for _, matchedHostTest := range sslCAInfoMatchedHostTests {
-		pool := getRootCAsForHost(c, matchedHostTest.hostName)
+		pool := getRootCAsForHostFromGitconfig(c, matchedHostTest.hostName)
 		assert.NotNil(t, pool)
 	}
 }
@@ -183,7 +183,7 @@ func TestCertFromSSLCAPathConfig(t *testing.T) {
 
 	// Should match any host at all
 	for _, matchedHostTest := range sslCAInfoMatchedHostTests {
-		pool := getRootCAsForHost(c, matchedHostTest.hostName)
+		pool := getRootCAsForHostFromGitconfig(c, matchedHostTest.hostName)
 		assert.NotNil(t, pool)
 	}
 }
@@ -201,7 +201,7 @@ func TestCertFromSSLCAPathEnv(t *testing.T) {
 
 	// Should match any host at all
 	for _, matchedHostTest := range sslCAInfoMatchedHostTests {
-		pool := getRootCAsForHost(c, matchedHostTest.hostName)
+		pool := getRootCAsForHostFromGitconfig(c, matchedHostTest.hostName)
 		assert.NotNil(t, pool)
 	}
 }

lfshttp/certs_nix.go

@@ -482,7 +482,7 @@ func (c *Client) Transport(u *url.URL, access creds.AccessMode) (http.RoundTripp
 	if isCertVerificationDisabledForHost(c, host) {
 		tr.TLSClientConfig.InsecureSkipVerify = true
 	} else {
-		tr.TLSClientConfig.RootCAs = getRootCAsForHost(c, host)
+		tr.TLSClientConfig.RootCAs = getRootCAsForHostFromGitconfig(c, host)
 	}
 
 	if err := c.configureProtocols(u, tr); err != nil {