242 respect ssl ca info #411

vdye · 2021-08-11T20:32:36Z

Implements #242

Changes

Add parsing for sslCAInfo git config parameter (and corresponding environment variable)
- Ignored if sslBackend is schannel and schannelUseSSLCAInfo is false or not set
If sslCAInfo is set, custom cert validation callback is called for HTTP client
- If there are any non-certificate chain errors with the server cert, fail (see RemoteCertificateNameMismatch and RemoteCertificateNotAvailable here)
- If there are any SSL chain errors other than untrusted root, fail (see flags listed here)
- Check the root of the server certificate
  - If the server cert root matches the cert pointed to by sslCAInfo, success
  - If the certs do not match, fail
Unit tests for the sslCAInfo setting
Minor whitespace fixes from my last pull request 😜

Testing

For all tests, ran the get command as described in "Development and Debugging" (changing the protocol from "http" to "https" for github.com)

Before starting, ran sudo dpkg-reconfigure ca-certificates to interactively (and temporarily) disable the root cert used by github.com

Do not set GIT_SSL_CAINFO

Result:

fatal: The SSL connection could not be established, see inner exception.
fatal: The remote certificate is invalid because of errors in the certificate chain: UntrustedRoot

Set GIT_SSL_CAINFO to path to trusted root certificate
- Result: successful get execution

Set GIT_SSL_CAINFO to path to an unrelated trusted root certificate for GitHub

Result:

fatal: The SSL connection could not be established, see inner exception.
fatal: The remote certificate was rejected by the provided RemoteCertificateValidationCallback.

Set GIT_SSL_CAINFO to path to a non-existent file
- Result: failure with the following printout (caused by uncaught exception in callback)
```
fatal: Custom certificate bundle not found at path: <path to fake cert>
```

Questions

Should this check whether the sslCAInfo file exists, or let the program fail?
- Answer: Check first!
Should the other X509ChainStatusFlags be allowed to continue on to the cert match check?
- Answer: nope! be conservative with errors
- Because the HTTP client first validates the cert against your local root store (and I didn't remove GitHub's actual root CA from my System Roots when testing), I wasn't able to verify that the Untrusted Root error is the only one to come through with an otherwise-valid self-signed cert

Whitespace cleanup

mjcheetham

A couple of questions and changes regarding validation and what ISettings should return.

Should this check whether the sslCAInfo file exists, or let the program fail?

HttpClientFactory should probably test if the file exists before setting the callback. It should then either warn or die (throw an exception), depending on what Git's behaviour is when http.sslCAInfo points to an invalid path.

Should the other X509ChainStatusFlags be allowed to continue on to the cert match check?

We should be conservative and fail for any certificate error we encounter (and manually check the root matches the supplied one.

Because the HTTP client first validates the cert against your local root store (and I didn't remove GitHub's actual root CA from my System Roots when testing), I wasn't able to verify that the Untrusted Root error is the only one to come through with an otherwise-valid self-signed cert

This is an interesting behaviour. So setting that callback means the "normal" .NET validation still happens before us? That probably deserves a comment in the code for future readers.

src/shared/Microsoft.Git.CredentialManager/HttpClientFactory.cs

src/shared/Microsoft.Git.CredentialManager/Settings.cs

mjcheetham · 2021-08-12T09:32:13Z

src/shared/Microsoft.Git.CredentialManager/Settings.cs

+            {
+                // First, verify that, if schannel is the HTTP backend, sslCaInfo must be explicitly enabled to be used
+                if (TryGetSetting(null, KnownGitCfg.Http.SectionName, KnownGitCfg.Http.SslBackend, out string backend) &&
+                    StringComparer.OrdinalIgnoreCase.Equals(backend, "schannel") &&


Should these constants ("schannel" and "openssl") be const strings in Constants.cs?

Yes for schannel, but would you also want openssl included (even though it's not used anywhere here)?

I'd add openssl too for good measure, it doesn't hurt anyone :)

Because this setting is just a passthrough to libcurl, there are more options than schannel and openssl (depending on platform). ~~If schannel is the only one used here (so far, at least), I think I'd prefer to only include that for now (it keeps us from needing to keep up with mirroring all of libcurl's available options).~~

EDIT: I ended up creating a (nullable) enum anyway - it looks like schannel and openssl are the most commonly used, so they're included in that. I included an "Other" value as well (to handle the rest and to distinguish from the "not specified" case - if there's a better way to handle that, I'm happy to change it).

If openssl is the default backend used by Git, would it be more appropriate to use a non-nullable TlsBackend for the setting, setting the value to openssl if it's unspecified in the user's config? I've updated the implementation to do this, but I can revert if you'd like.

src/shared/Microsoft.Git.CredentialManager/HttpClientFactory.cs

vdye · 2021-08-12T15:23:00Z

Because the HTTP client first validates the cert against your local root store (and I didn't remove GitHub's actual root CA from my System Roots when testing), I wasn't able to verify that the Untrusted Root error is the only one to come through with an otherwise-valid self-signed cert

This is an interesting behaviour. So setting that callback means the "normal" .NET validation still happens before us? That probably deserves a comment in the code for future readers.

The official documentation is pretty unclear, but I think the .NET source code confirms it (assuming I traced the through it properly). I'll add a comment explicitly mentioning that in the callback.

dscho · 2021-08-14T20:59:11Z

src/shared/Microsoft.Git.CredentialManager/Settings.cs

+                // First, verify that, if schannel is the HTTP backend, sslCaInfo must be explicitly enabled to be used
+                if (TryGetSetting(null, KnownGitCfg.Http.SectionName, KnownGitCfg.Http.SslBackend, out string backend) &&
+                    StringComparer.OrdinalIgnoreCase.Equals(backend, "schannel") &&
+                    (!TryGetSetting(null, KnownGitCfg.Http.SectionName, KnownGitCfg.Http.SchannelUseSslCaInfo, out string schannelUseSslCaInfo) ||


Is there any chance that we can call git config --type=path http.sslCAInfo? The --type=path option would ensure that we benefit from Git's own path interpolation.

This would involve some changes to GitConfiguration.cs (like adding an optional isPath argument to TryGet(...) or creating a TryGetPath(...) method), but I really like the benefits of using Git's path interpolation logic (and not replicating it here). @mjcheetham what do you think?

Does this handle the environment variable version of the config as well (in this case, GIT_SSL_CAINFO)? Or is %(prefix) (and any other "magic" path interpolation) not supported with environment variables?

This does not handle the environment variables (but neither does Git interpolate prefix-relative paths in environment variables, so I think we're good).

@mjcheetham what do you think?

I think this should be discussed in a separate PR/issue, so let's leave this as is today for this change.

vdye · 2021-08-16T14:40:28Z

I figured out a way to test this more accurately on Ubuntu (manually disabling the root certificate used by GitHub, then copying that cert to a custom location to use it as a "self-signed" cert) - results have been updated in the "Testing" section of the PR description.

mjcheetham

This looks good to me! Good job @vdye! Just one nit (if you want to fix.. don't worry if not!) and a question to @dscho about default values for the http.sslBackend.

src/shared/Microsoft.Git.CredentialManager/HttpClientFactory.cs

src/shared/Microsoft.Git.CredentialManager/Settings.cs

Replicate the behavior of `http.sslCAInfo` (internally, cURL's `cainfo`/ OpenSSL's `cafile` arguments). Implemented by manually comparing the server certificate with the config-specified CA file.

fixup! linux: add option for GCM_GPG_PATH environment variable

03bf5d1

Whitespace cleanup

mjcheetham requested review from dscho and mjcheetham August 12, 2021 09:23

mjcheetham requested changes Aug 12, 2021

View reviewed changes

dscho reviewed Aug 14, 2021

View reviewed changes

vdye force-pushed the 242-respect-sslCAInfo branch 4 times, most recently from 8778c15 to 8a7b46e Compare August 15, 2021 00:55

vdye requested a review from mjcheetham August 15, 2021 01:09

vdye force-pushed the 242-respect-sslCAInfo branch 2 times, most recently from e87c754 to 36fd743 Compare August 16, 2021 14:22

mjcheetham approved these changes Aug 17, 2021

View reviewed changes

src/shared/Microsoft.Git.CredentialManager/HttpClientFactory.cs Outdated Show resolved Hide resolved

src/shared/Microsoft.Git.CredentialManager/HttpClientFactory.cs Show resolved Hide resolved

src/shared/Microsoft.Git.CredentialManager/Settings.cs Outdated Show resolved Hide resolved

http: respect http.sslCAInfo setting(s) in Git

d58bf23

Replicate the behavior of `http.sslCAInfo` (internally, cURL's `cainfo`/ OpenSSL's `cafile` arguments). Implemented by manually comparing the server certificate with the config-specified CA file.

vdye force-pushed the 242-respect-sslCAInfo branch from d7f77c4 to d58bf23 Compare August 18, 2021 15:41

vdye merged commit 770eeeb into git-ecosystem:main Aug 18, 2021

vdye deleted the 242-respect-sslCAInfo branch August 18, 2021 16:41

mjcheetham mentioned this pull request Oct 8, 2021

Cut new GCM release #482

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

242 respect ssl ca info #411

242 respect ssl ca info #411

vdye commented Aug 11, 2021 •

edited

Loading

mjcheetham left a comment

mjcheetham Aug 12, 2021

vdye Aug 12, 2021

mjcheetham Aug 12, 2021

vdye Aug 12, 2021 •

edited

Loading

vdye Aug 17, 2021 •

edited

Loading

vdye commented Aug 12, 2021 •

edited

Loading

dscho Aug 14, 2021

vdye Aug 15, 2021

dscho Aug 16, 2021

mjcheetham Aug 18, 2021

vdye commented Aug 16, 2021

mjcheetham left a comment

242 respect ssl ca info #411

242 respect ssl ca info #411

Conversation

vdye commented Aug 11, 2021 • edited Loading

Changes

Testing

Questions

mjcheetham left a comment

Choose a reason for hiding this comment

mjcheetham Aug 12, 2021

Choose a reason for hiding this comment

vdye Aug 12, 2021

Choose a reason for hiding this comment

mjcheetham Aug 12, 2021

Choose a reason for hiding this comment

vdye Aug 12, 2021 • edited Loading

Choose a reason for hiding this comment

vdye Aug 17, 2021 • edited Loading

Choose a reason for hiding this comment

vdye commented Aug 12, 2021 • edited Loading

dscho Aug 14, 2021

Choose a reason for hiding this comment

vdye Aug 15, 2021

Choose a reason for hiding this comment

dscho Aug 16, 2021

Choose a reason for hiding this comment

mjcheetham Aug 18, 2021

Choose a reason for hiding this comment

vdye commented Aug 16, 2021

mjcheetham left a comment

Choose a reason for hiding this comment

vdye commented Aug 11, 2021 •

edited

Loading

vdye Aug 12, 2021 •

edited

Loading

vdye Aug 17, 2021 •

edited

Loading

vdye commented Aug 12, 2021 •

edited

Loading