Skip to content

Conversation

@chrisd8088
Copy link
Member

@chrisd8088 chrisd8088 commented May 5, 2025

This PR eliminates our uses of the /usr/bin/security command on macOS, which we previously invoked twice each time we established HTTP client transport settings for a new host in order to try to support the verification of TLS/SSL server certificates against custom CA (Certificate Authority) certificates in the System keychain.

Since Go v1.15, the Go standard library's crypto/x509 package has provided a more comprehensive and correct implementation of the same support, so we can simply defer to it in all cases.

This change should resolve the types of inconsistencies reported in #6036 while also simplifying our code and improving the performance of the Git LFS client on macOS systems.

Background

During the development of the Git LFS v2.0.0 major release, we refactored and expanded portions of our original code to create the lfsapi package in PR #1839, following the proposal outlined in #1783. One step in that process was the implementation in PR #1787 of the controls we offer for HTTP client transport settings, including various timeout configuration options like lfs.tlsTimeout (which had been introduced previously), along with new support for Git's TLS/SSL certificate verification configuration options.

Specifically, in commit 8009d17 of PR #1787 we implemented support for the use of custom TLS/SSL certificates identified 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 would be verified with a matching CA certificate if one was found in the System keychain, because the Go version we used at the time, namely Go v1.7.4, did not always guarantee this would be true.

As was reported in golang/go#14514 and golang/go#16532, Go v1.7.x would gather CA certificates from all three standard macOS keychains (SystemRootCertificates, System, and local) if Go was compiled with cgo support and
the macOS version was 10.9 (Mavericks) or higher. On older systems, even if cgo builds were enabled, only the SystemRootCertificates certificates would be used to verify server certificates. This behaviour was a temporarily solution, introduced in commit golang/go@ff60da6, to resolve the regressions caused when the first attempt to support all three keychains was added in commit golang/go@6cd698d.)

(Moreover, if cgo support was not enabled, there was also a separate non-cgo implementation in the crypto/x509 package which would invoke the /usr/bin/security find-certificate command. Our Git LFS client builds enabled cgo, however.)

Hence the changes in our commit 8009d17 were intended to work around the gap in the default behaviour of the crypto/x509 package on macOS 10.8 (Mountain Lion) and earlier systems. When the Transport() method in our lfshttp package called getRootCAsForHost(), that function would finish by invoking a platform-specific appendRootCAsForHostFromPlatform() function, and which on macOS systems would run the /usr/bin/security list-keychains command and then run the /usr/bin/security find-certificate command. On other operating systems, the appendRootCAsForHostFromPlatform() functions were simple no-ops.

Current Behaviour

Go v1.9 included changes to the non-cgo version of the crypto/x509 package to also gather certificates from the System and login keychains, made in commit golang/go@4dbcacd, but more significantly, the crypto/x509 package was extensively refactored for the Go v1.15 release in commit golang/go@6f52790. The non-cgo implementation was removed entirely, meaning the Go standard library no longer called the /usr/bin/security find-certificate command at all. The original cgo implementation was retained for testing purposes but ultimately also removed in commit golang/go@5e18135 of Go v1.16.

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 specifies their own certificates with either the GIT_SSL_* environment variables or the http.<url>.sslCAInfo or http.sslCAPath configuration options, we pass them as the RootCAs element of the tls.Config structure we set for the http.Transport structure's TLSClientConfig element, and so 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.

If the user does not specify any of the Git TLS/SSL certificate verification settings, then in general we will set the RootCAs element to an nil x509.CertPool value, and so the Certificate's Verify() method 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 #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 Certificate's Verify() method 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 certificate authority which can verify the server's certificate, the HTTP request will be rejected, which is not the behaviour we expect or intend.

Solution

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.

Testing

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 largely 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.

Most importantly, @stanhu has also reported that the changes in this PR successfully resolved the problem described in #6036:

With git-lfs v3.6.1, I get the tls: failed to verify certificate: x509: certificate signed by unknown authority error, but with the git-lfs in the CI artifact, it works fine.

In addition, I have performed local testing on a macOS system to confirm that HTTP requests made with the current implementation of Go's crypto/x509 package definitely verify server certificates against CA certificates found only in the System keychain, which was the original impetus for the workaround using the /usr/bin/security command that was introduced in PR #1787.

(With any luck, these tests are enough to avoid the danger posed by golang/go@6f52790, which was an upheaval sufficiently intense as to awaken Cthulhu, per that commit's description.)

Resolves #6036.
Replaces PR #6037.
/cc @stanhu as reporter and author of PR #6037.

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.
@chrisd8088 chrisd8088 requested a review from a team as a code owner May 5, 2025 16:46
@chrisd8088 chrisd8088 marked this pull request as draft May 5, 2025 16:47
@chrisd8088 chrisd8088 changed the title Use G Verify TLS/SSL certificates using default Go support for macOS system root CAs May 5, 2025
@stanhu
Copy link
Contributor

stanhu commented May 5, 2025

Thanks for this! I'm all for simplifying this logic.

@stanhu
Copy link
Contributor

stanhu commented May 21, 2025

@bk2204 Would you mind reviewing this? Thanks!

@chrisd8088
Copy link
Member Author

@bk2204 Would you mind reviewing this? Thanks!

@stanhu — Thanks for the prompt! As it happens, @bk2204 is actually one of our alumni now and not in the @git-lfs/core group anymore, so we'll need a review from @larsxschneider instead.

Copy link
Member

@larsxschneider larsxschneider left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Have you tried a build against Windows too? If not, please let me know: I am happy to test it (although based on the changes, I don't expect a problem).

(Plus, in this PR I learned about Cthulhu 😄 )

@chrisd8088
Copy link
Member Author

Have you tried a build against Windows too?

Well, our CI jobs succeed and pass the test suite, so in that regard at least, yes, definitely.

In particular, we have some Go tests in the lfshttp/certs_test.go source file which exercise our support for the GIT_SSL_CA* environment variables, including with the http.sslBackend Git configuration option set to schannel, as it might be on Windows.

Fortunately, I think we can also have a lot of confidence that Windows support is going to be unffected because on that platform the appendRootCAsForHostFromPlatform() function we're removing was just a no-op. (The function was also a no-op for Linux as well.)

I believe this PR's changes boil down to merging the appendRootCAsForHostFromGitconfig() function into the getRootCAsForHost() function (so we have to update a bunch of call sites, and tweak how the new common function accesses its arguments, but otherwise not change anything major), removing the no-op appendRootCAsForHostFromPlatform() functions on Windows and Linux, and removing the more complex and fragile version of that function on macOS.

Because that last difference is really the only substantive change, and @stanhu and I have both done some independent testing on macOS, I think we're OK to merge this PR.

Thanks to both of you for the assistance!

@chrisd8088 chrisd8088 merged commit 4920220 into git-lfs:main May 22, 2025
19 of 20 checks passed
@chrisd8088 chrisd8088 deleted the use-golang-macos-tls-roots branch May 22, 2025 21:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

darwin: Git LFS improperly adds certificates that cause SSL validation to fail

3 participants