| Submitter | Manuel Jacob |
|---|---|
| Date | May 30, 2020, 5:52 a.m. |
| Message ID | <9ae0e1b1a499dfce1807.1590817940@tmp> |
| Download | mbox | patch |
| Permalink | /patch/46403/ |
| State | New |
| Headers | show |
Comments
On Fri, May 29, 2020 at 11:50 PM Manuel Jacob <me@manueljacob.de> wrote: > # HG changeset patch > # User Manuel Jacob <me@manueljacob.de> > # Date 1590801838 -7200 > # Sat May 30 03:23:58 2020 +0200 > # Node ID 9ae0e1b1a499dfce1807e3c9ec5c03714c6f154a > # Parent 992db2b7bd11431df9145abc35dca2eba73b9972 > # EXP-Topic require_modern_ssl > sslutil: eliminate `_canloaddefaultcerts` by constant-folding code using it > > diff --git a/mercurial/sslutil.py b/mercurial/sslutil.py > --- a/mercurial/sslutil.py > +++ b/mercurial/sslutil.py > @@ -52,8 +52,6 @@ if util.safehasattr(ssl, b'PROTOCOL_TLSv > if util.safehasattr(ssl, b'PROTOCOL_TLSv1_2'): > supportedprotocols.add(b'tls1.2') > > -_canloaddefaultcerts = True > - > > def _hostsettings(ui, hostname): > """Obtain security settings for a hostname. > @@ -227,7 +225,7 @@ def _hostsettings(ui, hostname): > > # Require certificate validation if CA certs are being loaded and > # verification hasn't been disabled above. > - if cafile or (_canloaddefaultcerts and > s[b'allowloaddefaultcerts']): > + if cafile or (s[b'allowloaddefaultcerts']): > s[b'verifymode'] = ssl.CERT_REQUIRED > else: > # At this point we don't have a fingerprint, aren't being > @@ -721,14 +719,6 @@ def _plainapplepython(): > ) > > > -_systemcacertpaths = [ > - # RHEL, CentOS, and Fedora > - b'/etc/pki/tls/certs/ca-bundle.trust.crt', > - # Debian, Ubuntu, Gentoo > - b'/etc/ssl/certs/ca-certificates.crt', > -] > - > - > def _defaultcacerts(ui): > """return path to default CA certificates or None. > > @@ -751,23 +741,6 @@ def _defaultcacerts(ui): > except (ImportError, AttributeError): > pass > > - # On Windows, only the modern ssl module is capable of loading the > system > - # CA certificates. If we're not capable of doing that, emit a warning > - # because we'll get a certificate verification error later and the > lack > - # of loaded CA certificates will be the reason why. > - # Assertion: this code is only called if certificates are being > verified. > - if pycompat.iswindows: > - if not _canloaddefaultcerts: > - ui.warn( > - _( > - b'(unable to load Windows CA certificates; see ' > - b'https://mercurial-scm.org/wiki/SecureConnections > for ' > - b'how to configure Mercurial to avoid this message)\n' > - ) > - ) > - > - return None > - > # Apple's OpenSSL has patches that allow a specially constructed > certificate > # to load the system CA store. If we're running on Apple Python, use > this > # trick. > @@ -778,58 +751,6 @@ def _defaultcacerts(ui): > if os.path.exists(dummycert): > return dummycert > > - # The Apple OpenSSL trick isn't available to us. If Python isn't able > to > - # load system certs, we're out of luck. > - if pycompat.isdarwin: > - # FUTURE Consider looking for Homebrew or MacPorts installed certs > - # files. Also consider exporting the keychain certs to a file > during > - # Mercurial install. > - if not _canloaddefaultcerts: > - ui.warn( > - _( > - b'(unable to load CA certificates; see ' > - b'https://mercurial-scm.org/wiki/SecureConnections > for ' > - b'how to configure Mercurial to avoid this message)\n' > - ) > - ) > - return None > - > - # / is writable on Windows. Out of an abundance of caution make sure > - # we're not on Windows because paths from _systemcacerts could be > installed > - # by non-admin users. > - assert not pycompat.iswindows > - > - # Try to find CA certificates in well-known locations. We print a > warning > - # when using a found file because we don't want too much silent magic > - # for security settings. The expectation is that proper Mercurial > - # installs will have the CA certs path defined at install time and the > - # installer/packager will make an appropriate decision on the user's > - # behalf. We only get here and perform this setting as a feature of > - # last resort. > - if not _canloaddefaultcerts: > - for path in _systemcacertpaths: > - if os.path.isfile(path): > - ui.warn( > - _( > - b'(using CA certificates from %s; if you see this > ' > - b'message, your Mercurial install is not properly > ' > - b'configured; see ' > - b' > https://mercurial-scm.org/wiki/SecureConnections ' > - b'for how to configure Mercurial to avoid this ' > - b'message)\n' > - ) > - % path > - ) > - return path > - > - ui.warn( > - _( > - b'(unable to load CA certificates; see ' > - b'https://mercurial-scm.org/wiki/SecureConnections for ' > - b'how to configure Mercurial to avoid this message)\n' > - ) > - ) > - > return None > The removal of all the code scared me when reviewing this. But looking at the final state of things, I think things are reasonable. Essentially, we only now look for certifi and apply the Apple dummy cert trick and return None otherwise. The removed code was effectively about presenting platform-specific warnings and locating default cert paths on some distros. That should be safe to remove. Note that the removed messages still appear in at least test-https.t. I'm unsure if a subsequent patch in this series cleans up the tests. If not, please submit a follow-up to do that.
On 2020-05-30 18:11, Gregory Szorc wrote: > On Fri, May 29, 2020 at 11:50 PM Manuel Jacob <me@manueljacob.de> > wrote: > >> # HG changeset patch >> # User Manuel Jacob <me@manueljacob.de> >> # Date 1590801838 -7200 >> # Sat May 30 03:23:58 2020 +0200 >> # Node ID 9ae0e1b1a499dfce1807e3c9ec5c03714c6f154a >> # Parent 992db2b7bd11431df9145abc35dca2eba73b9972 >> # EXP-Topic require_modern_ssl >> sslutil: eliminate `_canloaddefaultcerts` by constant-folding code >> using it >> >> diff --git a/mercurial/sslutil.py b/mercurial/sslutil.py >> --- a/mercurial/sslutil.py >> +++ b/mercurial/sslutil.py >> @@ -52,8 +52,6 @@ if util.safehasattr(ssl, b'PROTOCOL_TLSv >> if util.safehasattr(ssl, b'PROTOCOL_TLSv1_2'): >> supportedprotocols.add(b'tls1.2') >> >> -_canloaddefaultcerts = True >> - >> >> def _hostsettings(ui, hostname): >> """Obtain security settings for a hostname. >> @@ -227,7 +225,7 @@ def _hostsettings(ui, hostname): >> >> # Require certificate validation if CA certs are being loaded >> and >> # verification hasn't been disabled above. >> - if cafile or (_canloaddefaultcerts and >> s[b'allowloaddefaultcerts']): >> + if cafile or (s[b'allowloaddefaultcerts']): >> s[b'verifymode'] = ssl.CERT_REQUIRED >> else: >> # At this point we don't have a fingerprint, aren't being >> @@ -721,14 +719,6 @@ def _plainapplepython(): >> ) >> >> >> -_systemcacertpaths = [ >> - # RHEL, CentOS, and Fedora >> - b'/etc/pki/tls/certs/ca-bundle.trust.crt', >> - # Debian, Ubuntu, Gentoo >> - b'/etc/ssl/certs/ca-certificates.crt', >> -] >> - >> - >> def _defaultcacerts(ui): >> """return path to default CA certificates or None. >> >> @@ -751,23 +741,6 @@ def _defaultcacerts(ui): >> except (ImportError, AttributeError): >> pass >> >> - # On Windows, only the modern ssl module is capable of loading >> the >> system >> - # CA certificates. If we're not capable of doing that, emit a >> warning >> - # because we'll get a certificate verification error later and >> the >> lack >> - # of loaded CA certificates will be the reason why. >> - # Assertion: this code is only called if certificates are being >> verified. >> - if pycompat.iswindows: >> - if not _canloaddefaultcerts: >> - ui.warn( >> - _( >> - b'(unable to load Windows CA certificates; see ' >> - >> b'https://mercurial-scm.org/wiki/SecureConnections >> for ' >> - b'how to configure Mercurial to avoid this >> message)\n' >> - ) >> - ) >> - >> - return None >> - >> # Apple's OpenSSL has patches that allow a specially constructed >> certificate >> # to load the system CA store. If we're running on Apple Python, >> use >> this >> # trick. >> @@ -778,58 +751,6 @@ def _defaultcacerts(ui): >> if os.path.exists(dummycert): >> return dummycert >> >> - # The Apple OpenSSL trick isn't available to us. If Python isn't >> able >> to >> - # load system certs, we're out of luck. >> - if pycompat.isdarwin: >> - # FUTURE Consider looking for Homebrew or MacPorts installed >> certs >> - # files. Also consider exporting the keychain certs to a file >> during >> - # Mercurial install. >> - if not _canloaddefaultcerts: >> - ui.warn( >> - _( >> - b'(unable to load CA certificates; see ' >> - >> b'https://mercurial-scm.org/wiki/SecureConnections >> for ' >> - b'how to configure Mercurial to avoid this >> message)\n' >> - ) >> - ) >> - return None >> - >> - # / is writable on Windows. Out of an abundance of caution make >> sure >> - # we're not on Windows because paths from _systemcacerts could be >> installed >> - # by non-admin users. >> - assert not pycompat.iswindows >> - >> - # Try to find CA certificates in well-known locations. We print a >> warning >> - # when using a found file because we don't want too much silent >> magic >> - # for security settings. The expectation is that proper Mercurial >> - # installs will have the CA certs path defined at install time >> and the >> - # installer/packager will make an appropriate decision on the >> user's >> - # behalf. We only get here and perform this setting as a feature >> of >> - # last resort. >> - if not _canloaddefaultcerts: >> - for path in _systemcacertpaths: >> - if os.path.isfile(path): >> - ui.warn( >> - _( >> - b'(using CA certificates from %s; if you see >> this >> ' >> - b'message, your Mercurial install is not >> properly >> ' >> - b'configured; see ' >> - b' >> https://mercurial-scm.org/wiki/SecureConnections ' >> - b'for how to configure Mercurial to avoid >> this ' >> - b'message)\n' >> - ) >> - % path >> - ) >> - return path >> - >> - ui.warn( >> - _( >> - b'(unable to load CA certificates; see ' >> - b'https://mercurial-scm.org/wiki/SecureConnections >> for ' >> - b'how to configure Mercurial to avoid this >> message)\n' >> - ) >> - ) >> - >> return None >> > > The removal of all the code scared me when reviewing this. But looking > at > the final state of things, I think things are reasonable. Essentially, > we > only now look for certifi and apply the Apple dummy cert trick and > return > None otherwise. The removed code was effectively about presenting > platform-specific warnings and locating default cert paths on some > distros. > That should be safe to remove. Sorry, I should have splitted the part that removed "if pycompat....: return None" into a separate patch, to make it more obvious that it's redundant. > Note that the removed messages still appear in at least test-https.t. > I'm > unsure if a subsequent patch in this series cleans up the tests. If > not, > please submit a follow-up to do that. The code appeared in a test matching optional output. I've sent a patch (once incomplete and one fixed) to clean it up. Now the message is still in i18n/, but if I understood correctly that directory should not be touched by me even for removals.
On Sat, May 30, 2020 at 10:16 AM Manuel Jacob <me@manueljacob.de> wrote: > On 2020-05-30 18:11, Gregory Szorc wrote: > > On Fri, May 29, 2020 at 11:50 PM Manuel Jacob <me@manueljacob.de> > > wrote: > > > >> # HG changeset patch > >> # User Manuel Jacob <me@manueljacob.de> > >> # Date 1590801838 -7200 > >> # Sat May 30 03:23:58 2020 +0200 > >> # Node ID 9ae0e1b1a499dfce1807e3c9ec5c03714c6f154a > >> # Parent 992db2b7bd11431df9145abc35dca2eba73b9972 > >> # EXP-Topic require_modern_ssl > >> sslutil: eliminate `_canloaddefaultcerts` by constant-folding code > >> using it > >> > >> diff --git a/mercurial/sslutil.py b/mercurial/sslutil.py > >> --- a/mercurial/sslutil.py > >> +++ b/mercurial/sslutil.py > >> @@ -52,8 +52,6 @@ if util.safehasattr(ssl, b'PROTOCOL_TLSv > >> if util.safehasattr(ssl, b'PROTOCOL_TLSv1_2'): > >> supportedprotocols.add(b'tls1.2') > >> > >> -_canloaddefaultcerts = True > >> - > >> > >> def _hostsettings(ui, hostname): > >> """Obtain security settings for a hostname. > >> @@ -227,7 +225,7 @@ def _hostsettings(ui, hostname): > >> > >> # Require certificate validation if CA certs are being loaded > >> and > >> # verification hasn't been disabled above. > >> - if cafile or (_canloaddefaultcerts and > >> s[b'allowloaddefaultcerts']): > >> + if cafile or (s[b'allowloaddefaultcerts']): > >> s[b'verifymode'] = ssl.CERT_REQUIRED > >> else: > >> # At this point we don't have a fingerprint, aren't being > >> @@ -721,14 +719,6 @@ def _plainapplepython(): > >> ) > >> > >> > >> -_systemcacertpaths = [ > >> - # RHEL, CentOS, and Fedora > >> - b'/etc/pki/tls/certs/ca-bundle.trust.crt', > >> - # Debian, Ubuntu, Gentoo > >> - b'/etc/ssl/certs/ca-certificates.crt', > >> -] > >> - > >> - > >> def _defaultcacerts(ui): > >> """return path to default CA certificates or None. > >> > >> @@ -751,23 +741,6 @@ def _defaultcacerts(ui): > >> except (ImportError, AttributeError): > >> pass > >> > >> - # On Windows, only the modern ssl module is capable of loading > >> the > >> system > >> - # CA certificates. If we're not capable of doing that, emit a > >> warning > >> - # because we'll get a certificate verification error later and > >> the > >> lack > >> - # of loaded CA certificates will be the reason why. > >> - # Assertion: this code is only called if certificates are being > >> verified. > >> - if pycompat.iswindows: > >> - if not _canloaddefaultcerts: > >> - ui.warn( > >> - _( > >> - b'(unable to load Windows CA certificates; see ' > >> - > >> b'https://mercurial-scm.org/wiki/SecureConnections > >> for ' > >> - b'how to configure Mercurial to avoid this > >> message)\n' > >> - ) > >> - ) > >> - > >> - return None > >> - > >> # Apple's OpenSSL has patches that allow a specially constructed > >> certificate > >> # to load the system CA store. If we're running on Apple Python, > >> use > >> this > >> # trick. > >> @@ -778,58 +751,6 @@ def _defaultcacerts(ui): > >> if os.path.exists(dummycert): > >> return dummycert > >> > >> - # The Apple OpenSSL trick isn't available to us. If Python isn't > >> able > >> to > >> - # load system certs, we're out of luck. > >> - if pycompat.isdarwin: > >> - # FUTURE Consider looking for Homebrew or MacPorts installed > >> certs > >> - # files. Also consider exporting the keychain certs to a file > >> during > >> - # Mercurial install. > >> - if not _canloaddefaultcerts: > >> - ui.warn( > >> - _( > >> - b'(unable to load CA certificates; see ' > >> - > >> b'https://mercurial-scm.org/wiki/SecureConnections > >> for ' > >> - b'how to configure Mercurial to avoid this > >> message)\n' > >> - ) > >> - ) > >> - return None > >> - > >> - # / is writable on Windows. Out of an abundance of caution make > >> sure > >> - # we're not on Windows because paths from _systemcacerts could be > >> installed > >> - # by non-admin users. > >> - assert not pycompat.iswindows > >> - > >> - # Try to find CA certificates in well-known locations. We print a > >> warning > >> - # when using a found file because we don't want too much silent > >> magic > >> - # for security settings. The expectation is that proper Mercurial > >> - # installs will have the CA certs path defined at install time > >> and the > >> - # installer/packager will make an appropriate decision on the > >> user's > >> - # behalf. We only get here and perform this setting as a feature > >> of > >> - # last resort. > >> - if not _canloaddefaultcerts: > >> - for path in _systemcacertpaths: > >> - if os.path.isfile(path): > >> - ui.warn( > >> - _( > >> - b'(using CA certificates from %s; if you see > >> this > >> ' > >> - b'message, your Mercurial install is not > >> properly > >> ' > >> - b'configured; see ' > >> - b' > >> https://mercurial-scm.org/wiki/SecureConnections ' > >> - b'for how to configure Mercurial to avoid > >> this ' > >> - b'message)\n' > >> - ) > >> - % path > >> - ) > >> - return path > >> - > >> - ui.warn( > >> - _( > >> - b'(unable to load CA certificates; see ' > >> - b'https://mercurial-scm.org/wiki/SecureConnections > >> for ' > >> - b'how to configure Mercurial to avoid this > >> message)\n' > >> - ) > >> - ) > >> - > >> return None > >> > > > > The removal of all the code scared me when reviewing this. But looking > > at > > the final state of things, I think things are reasonable. Essentially, > > we > > only now look for certifi and apply the Apple dummy cert trick and > > return > > None otherwise. The removed code was effectively about presenting > > platform-specific warnings and locating default cert paths on some > > distros. > > That should be safe to remove. > > Sorry, I should have splitted the part that removed "if pycompat....: > return None" into a separate patch, to make it more obvious that it's > redundant. > It isn't your fault: the old code was a bit convoluted, as it was using early return to deal with various platform variants. The diff was hard to understand but the final state makes a lot more sense than what existed before! > > > Note that the removed messages still appear in at least test-https.t. > > I'm > > unsure if a subsequent patch in this series cleans up the tests. If > > not, > > please submit a follow-up to do that. > > The code appeared in a test matching optional output. I've sent a patch > (once incomplete and one fixed) to clean it up. Now the message is still > in i18n/, but if I understood correctly that directory should not be > touched by me even for removals. > I have since seen the patch to clean up test-https.t - thank you! You are correct that i18n/ can be ignored.
Patch
diff --git a/mercurial/sslutil.py b/mercurial/sslutil.py --- a/mercurial/sslutil.py +++ b/mercurial/sslutil.py @@ -52,8 +52,6 @@ if util.safehasattr(ssl, b'PROTOCOL_TLSv if util.safehasattr(ssl, b'PROTOCOL_TLSv1_2'): supportedprotocols.add(b'tls1.2') -_canloaddefaultcerts = True - def _hostsettings(ui, hostname): """Obtain security settings for a hostname. @@ -227,7 +225,7 @@ def _hostsettings(ui, hostname): # Require certificate validation if CA certs are being loaded and # verification hasn't been disabled above. - if cafile or (_canloaddefaultcerts and s[b'allowloaddefaultcerts']): + if cafile or (s[b'allowloaddefaultcerts']): s[b'verifymode'] = ssl.CERT_REQUIRED else: # At this point we don't have a fingerprint, aren't being @@ -721,14 +719,6 @@ def _plainapplepython(): ) -_systemcacertpaths = [ - # RHEL, CentOS, and Fedora - b'/etc/pki/tls/certs/ca-bundle.trust.crt', - # Debian, Ubuntu, Gentoo - b'/etc/ssl/certs/ca-certificates.crt', -] - - def _defaultcacerts(ui): """return path to default CA certificates or None. @@ -751,23 +741,6 @@ def _defaultcacerts(ui): except (ImportError, AttributeError): pass - # On Windows, only the modern ssl module is capable of loading the system - # CA certificates. If we're not capable of doing that, emit a warning - # because we'll get a certificate verification error later and the lack - # of loaded CA certificates will be the reason why. - # Assertion: this code is only called if certificates are being verified. - if pycompat.iswindows: - if not _canloaddefaultcerts: - ui.warn( - _( - b'(unable to load Windows CA certificates; see ' - b'https://mercurial-scm.org/wiki/SecureConnections for ' - b'how to configure Mercurial to avoid this message)\n' - ) - ) - - return None - # Apple's OpenSSL has patches that allow a specially constructed certificate # to load the system CA store. If we're running on Apple Python, use this # trick. @@ -778,58 +751,6 @@ def _defaultcacerts(ui): if os.path.exists(dummycert): return dummycert - # The Apple OpenSSL trick isn't available to us. If Python isn't able to - # load system certs, we're out of luck. - if pycompat.isdarwin: - # FUTURE Consider looking for Homebrew or MacPorts installed certs - # files. Also consider exporting the keychain certs to a file during - # Mercurial install. - if not _canloaddefaultcerts: - ui.warn( - _( - b'(unable to load CA certificates; see ' - b'https://mercurial-scm.org/wiki/SecureConnections for ' - b'how to configure Mercurial to avoid this message)\n' - ) - ) - return None - - # / is writable on Windows. Out of an abundance of caution make sure - # we're not on Windows because paths from _systemcacerts could be installed - # by non-admin users. - assert not pycompat.iswindows - - # Try to find CA certificates in well-known locations. We print a warning - # when using a found file because we don't want too much silent magic - # for security settings. The expectation is that proper Mercurial - # installs will have the CA certs path defined at install time and the - # installer/packager will make an appropriate decision on the user's - # behalf. We only get here and perform this setting as a feature of - # last resort. - if not _canloaddefaultcerts: - for path in _systemcacertpaths: - if os.path.isfile(path): - ui.warn( - _( - b'(using CA certificates from %s; if you see this ' - b'message, your Mercurial install is not properly ' - b'configured; see ' - b'https://mercurial-scm.org/wiki/SecureConnections ' - b'for how to configure Mercurial to avoid this ' - b'message)\n' - ) - % path - ) - return path - - ui.warn( - _( - b'(unable to load CA certificates; see ' - b'https://mercurial-scm.org/wiki/SecureConnections for ' - b'how to configure Mercurial to avoid this message)\n' - ) - ) - return None