Summary: Mostly empty lines removed and added. A few bugfixes on excessive line splitting.
Reviewed By: quark-zju
Differential Revision: D8199128
fbshipit-source-id: 90c1616061bfd7cfbba0b75f03f89683340374d5
Summary:
Turned on the auto formatter. Ran `arc lint --apply-patches --take BLACK **/*.py`.
Then run `arc lint` again so some other autofixers like spellchecker etc. looked
at the code base. Manually accept the changes whenever they make sense, or use
a workaround (ex. changing "dict()" to "dict constructor") where autofix is false
positive. Disabled linters on files that are hard (i18n/polib.py) to fix, or less
interesting to fix (hgsubversion tests), or cannot be fixed without breaking
OSS build (FBPYTHON4).
Conflicted linters (test-check-module-imports.t, part of test-check-code.t,
test-check-pyflakes.t) are removed or disabled.
Duplicated linters (test-check-pyflakes.t, test-check-pylint.t) are removed.
An issue of the auto-formatter is lines are no longer guarnateed to be <= 80
chars. But that seems less important comparing with the benefit auto-formatter
provides.
As we're here, also remove test-check-py3-compat.t, as it is currently broken
if `PYTHON3=/bin/python3` is set.
Reviewed By: wez, phillco, simpkins, pkaush, singhsrb
Differential Revision: D8173629
fbshipit-source-id: 90e248ae0c5e6eaadbe25520a6ee42d32005621b
This is done by:
sed -i "s/pycompat\.sysplatform == 'darwin'/pycompat.isdarwin/" **/*.py
Plus a manual change to `sslutil.py` which involves indentation change that
cannot be done by `sed`.
Differential Revision: https://phab.mercurial-scm.org/D1035
This is a Windows only thing. Unfortunately, the socket is closed at this point
(so the certificate is unavailable to check the chain). That means it's printed
out when verification fails as a guess, on the assumption that 1) most of the
time verification won't fail, and 2) sites using expired or certs that are too
new will be rare. Maybe this is an argument for adding more functionality to
debugssl, to test for problems and print certificate info. Or maybe it's an
argument for bundling certificates with the Windows builds. That idea was set
aside when the enhanced SSL code went in last summer, and it looks like there
were issues with using certifi on Windows anyway[1].
This was tested by deleting the certificate out of certmgr.msc > "Third-Party
Root Certification Authorities" > "Certificates", seeing `hg pull` fail (with
the new message), trying this command, and then successfully performing the pull
command.
[1] https://www.mercurial-scm.org/pipermail/mercurial-devel/2016-October/089573.html
Currently, sslutil._hostsettings() performs validation that web.cacerts
exists. However, client certificates are passed in to the function
and not all callers may validate them. This includes
httpconnection.readauthforuri(), which loads the [auth] section.
If a missing file is specified, the ssl module will raise a generic
IOException. And, it doesn't even give us the courtesy of telling
us which file is missing! Mercurial then prints a generic
"abort: No such file or directory" (or similar) error, leaving users
to scratch their head as to what file is missing.
This commit introduces explicit validation of all paths passed as
arguments to wrapsocket() and wrapserversocket(). Any missing file
is alerted about explicitly.
We should probably catch missing files earlier - as part of loading
the [auth] section. However, I think the sslutil functions should
check for file presence regardless of what callers do because that's
the only way to be sure that missing files are always detected.
I ran into this python issue with an incomplete certificate chain on Windows
recently, and this is the clarification that came from that experimenting. The
comment I left on the bug tracker [1] with a reference to the CPython code [2]
indicates that the original problem I had is a different bug, but happened to
be mentioned under issue20916 on the Python bug tracker.
[1] https://bz.mercurial-scm.org/show_bug.cgi?id=5313#c7
[2] https://hg.python.org/cpython/file/v2.7.12/Modules/_ssl.c#l628
Mercurial 3.9 added the [hostsecurity] section, which is better
than [hostfingerprints] in every way.
One of the ways that [hostsecurity] is better is that it supports
SHA-256 and SHA-512 fingerprints, not just SHA-1 fingerprints.
The world is moving away from SHA-1 because it is borderline
secure. Mercurial should be part of that movement.
This patch adds a warning when a valid SHA-1 fingerprint from
the [hostfingerprints] section is being used. The warning informs
users to switch to [hostsecurity]. It even prints the config
option they should set. It uses the SHA-256 fingerprint because
recommending a SHA-1 fingerprint in 2017 would be ill-advised.
The warning will print itself on every connection to a server until
it is fixed. There is no way to suppress the warning. I admit this
is annoying. But given the security implications of sticking with
SHA-1, I think this is justified. If this patch is accepted,
I'll likely send a follow-up to start warning on SHA-1
certificates in [hostsecurity] as well. Then sometime down
the road, we can drop support for SHA-1 fingerprints.
Credit for this idea comes from timeless in issue 5466.
sys.platform returns unicode on python 3 world. Our code base has most of the
things bytes because of the transformer. So we have a bytes version of this as
pycompat.sysplatform. This series of 2 patches replaces occurences of
sys.platform with pycompat.sysplatform.
os.name returns unicodes on py3 and we have pycompat.osname which returns
bytes. This series of 2 patches will change every ocurrence of os.name with
pycompat.osname.
Certifi is currently incompatible with py2exe; the Python code for certifi gets
included in library.zip, but not the cacert.pem file - and even if it were
included, SSLContext can't load a cacert.pem file from library.zip.
This currently makes it impossible to build a standalone Windows version of
Mercurial.
Guard against this, and possibly other situations where a module with the name
"certifi" exists, but is not usable.
The SSLError exception is a bit different with pypy (message is the first
argument, not the second) This led the certificate error handling to crash when
trying to extract the ssl error message. We now handle this different and
'test-https.t' is green again.
SSLContext.get_ca_certs() can raise
"ssl.SSLError: unknown error (_ssl.c:636)" on Windows. See
https://bugs.python.org/issue20916 for more info.
We add a try..except that swallows the exception to work around
this bug. If we encounter the bug, we won't print a warning
message about attempting to load CA certificates. This is
unfortunate. But there appears to be little we can do :/
There are various causes for the inability to negotiate common SSL/TLS
protocol between client and server. Previously, we had a single, not
very actionable warning message for all of them.
As people encountered TLS 1.0 servers in real life, it was quickly
obvious that the existing messaging was inadequate to help users
rectify the situation.
This patch makes the warning messages much more verbose in hopes of
making them more actionable while simultaneously encouraging users
and servers to adopt better security practices.
This messaging flirts with the anti-pattern of "never blame the
user" by signaling out poorly-configured servers. But if we're going to
disallow TLS 1.0 by default, I think we need to say *something* or
people are just going to blame Mercurial for not being able to connect.
The messaging tries to exonerate Mercurial from being the at fault
party by pointing out the server is the entity that doesn't support
proper security (when appropriate, of course).
--insecure is our psuedo-supported footgun for disabling connection
security.
The flag already disables CA verification. I think allowing the use of
TLS 1.0 when specified is appropriate.
The Python ssl module conditionally sets the TLS 1.1 and TLS 1.2
constants depending on whether HAVE_TLSv1_2 is defined. Yes, these
are both tied to the same constant (I would think there would be
separate constants for each version). Perhaps support for TLS 1.1
and 1.2 were added at the same time and the assumption is that
OpenSSL either has neither or both. I don't know.
As part of developing this patch, it was discovered that Apple's
/usr/bin/python2.7 does not support TLS 1.1 and 1.2 (only TLS 1.0)!
On OS X 10.11, Apple Python has the modern ssl module including
SSLContext, but it doesn't appear to negotiate TLS 1.1+ nor does
it expose the constants related to TLS 1.1+. Since this code is
doing more robust feature detection (and not assuming modern ssl
implies TLS 1.1+ support), we now get TLS 1.0 warnings when running
on Apple Python. Hence the test changes.
I'm not super thrilled about shipping a Mercurial that always
whines about TLS 1.0 on OS X. We may want a follow-up patch to
suppress this warning.
Python 2.7 supports specifying a custom cipher list to TLS sockets.
Advanced users may wish to specify a custom cipher list to increase
security. Or in some cases they may wish to prefer weaker ciphers
in order to increase performance (e.g. when doing stream clones
of very large repositories).
This patch introduces a [hostsecurity] config option for defining
the cipher list. The help documentation states that it is for
advanced users only.
Honestly, I'm a bit on the fence about providing this because
it is a footgun and can be used to decrease security. However,
there are legitimate use cases for it, so I think support should
be provided.
Mercurial now requires TLS 1.1+ when TLS 1.1+ is supported by the
client. Since we made the decision to require TLS 1.1+ when running
with modern Python versions, it makes sense to do something for
legacy Python versions that only support TLS 1.0.
Feature parity would be to prevent TLS 1.0 connections out of the
box and require a config option to enable them. However, this is
extremely user hostile since Mercurial wouldn't talk to https://
by default in these installations! I can easily see how someone
would do something foolish like use "--insecure" instead - and
that would be worse than allowing TLS 1.0!
This patch takes the compromise position of printing a warning when
performing TLS 1.0 connections when running on old Python
versions. While this warning is no more annoying than the
CA certificate / fingerprint warnings in Mercurial 3.8, we provide
a config option to disable the warning because to many people
upgrading Python to make the warning go away is not an available
recourse (unlike pinning fingerprints is for the CA warning).
The warning appears as optional output in a lot of tests.
Currently, Mercurial will use TLS 1.0 or newer when connecting to
remote servers, selecting the highest TLS version supported by both
peers. On older Pythons, only TLS 1.0 is available. On newer Pythons,
TLS 1.1 and 1.2 should be available.
Security professionals recommend avoiding TLS 1.0 if possible.
PCI DSS 3.1 "strongly encourages" the use of TLS 1.2.
Known attacks like BEAST and POODLE exist against TLS 1.0 (although
mitigations are available and properly configured servers aren't
vulnerable).
I asked Eric Rescorla - Mozilla's resident crypto expert - whether
Mercurial should drop support for TLS 1.0. His response was
"if you can get away with it." Essentially, a number of servers on
the Internet don't support TLS 1.1+. This is why web browsers
continue to support TLS 1.0 despite desires from security experts.
This patch changes Mercurial's default behavior on modern Python
versions to require TLS 1.1+, thus avoiding known security issues
with TLS 1.0 and making Mercurial more secure by default. Rather
than drop TLS 1.0 support wholesale, we still allow TLS 1.0 to be
used if configured. This is a compromise solution - ideally we'd
disallow TLS 1.0. However, since we're not sure how many Mercurial
servers don't support TLS 1.1+ and we're not sure how much user
inconvenience this change will bring, I think it is prudent to ship
an escape hatch that still allows usage of TLS 1.0. In the default
case our users get better security. In the worst case, they are no
worse off than before this patch.
This patch has no effect when running on Python versions that don't
support TLS 1.1+.
As the added test shows, connecting to a server that doesn't
support TLS 1.1+ will display a warning message with a link to
our wiki, where we can guide people to configure their client to
allow less secure connections.
Currently, Mercurial will use TLS 1.0 or newer when connecting to
remote servers, selecting the highest TLS version supported by both
peers. On older Pythons, only TLS 1.0 is available. On newer Pythons,
TLS 1.1 and 1.2 should be available.
Security-minded people may want to not take any risks running
TLS 1.0 (or even TLS 1.1). This patch gives those people a config
option to explicitly control which TLS versions Mercurial should use.
By providing this option, one can require newer TLS versions
before they are formally deprecated by Mercurial/Python/OpenSSL/etc
and lower their security exposure. This option also provides an
easy mechanism to change protocol policies in Mercurial. If there
is a 0-day and TLS 1.0 is completely broken, we can act quickly
without changing much code.
Because setting the minimum TLS protocol is something you'll likely
want to do globally, this patch introduces a global config option under
[hostsecurity] for that purpose.
wrapserversocket() has been taught a hidden config option to define
the explicit protocol to use. This is queried in this function and
not passed as an argument because I don't want to expose this dangerous
option as part of the Python API. There is a risk someone could footgun
themselves. But the config option is a devel option, has a warning
comment, and I doubt most people are using `hg serve` to run a
production HTTPS server (I would have something not Mercurial/Python
handle TLS). If this is problematic, we can go back to using a
custom extension in tests to coerce the server into bad behavior.
ssl.create_default_context() disables compression on the TLS channel
in order to prevent CRIME. I think we should follow CPython's lead
and attempt to disable channel compression in order to help prevent
information leakage.
Sadly, I don't think there is anything we can do on Python versions
that don't have an SSLContext, as there is no way to set channel
options with the limited ssl API.
While ssl.create_default_context() creates a SSLContext with
reasonable default options, we can't use it because it conflicts with
our CA loading controls. So replace the comment with reality.
(FWIW the comment was written before the existing CA loading code
was in place.)
wrapsocket() is heavily tailored towards client use. In preparation
for converting the built-in server to use sslutil (as opposed to
the ssl module directly), we add wrapserversocket() for wrapping
a socket to be used on servers.
Yuya suggested we add this check to ensure we don't accidentally try
to load user-writable paths on Windows if we change the control
flow of this function later.
Many Linux distros and other Nixen have CA certificates in well-defined
locations. Rather than potentially fail to load any CA certificates at
all (which will always result in a certificate verification failure),
we scan for paths to known CA certificate files and load one if seen.
Because a proper Mercurial install will have the path to the CA
certificate file defined at install time, we print a warning that
the install isn't proper and provide a URL with instructions to
correct things.
We only perform path-based fallback on Pythons that don't know
how to call into OpenSSL to load the default verify locations. This
is because we trust that Python/OpenSSL is properly configured
and knows better than Mercurial. So this new code effectively only
runs on Python <2.7.9 (technically Pythons without the modern ssl
module).
Previously, failure to load system certificates on OS X would lead
to a certificate verify failure and that's it. We now print a warning
message with a URL that will contain information on how to configure
certificates on OS X.
As the inline comment states, there is room to improve here. I think
we could try harder to detect Homebrew and MacPorts installed
certificate files, for example. It's worth noting that Homebrew's
openssl package uses `security find-certificate -a -p` during package
installation to export the system keychain root CAs to
etc/openssl/cert.pem. This is something we could consider adding
to setup.py. We could also encourage packagers to do this. For now,
I'd just like to get this warning (which matches Windows behavior)
landed. We should have time to improve things before release.
I should have ran the entire test suite on Python 2.6. Since the
hostname matching tests are implemented in Python (not .t tests),
it didn't uncover this warning. I'm not sure why - warnings should
be printed regardless. This is possibly a bug in the test runner.
But that's for another day...
sslutil contains its own hostname matching logic. CPython has code
for the same intent. However, it is only available to Python 2.7.9+
(or distributions that have backported 2.7.9's ssl module
improvements).
This patch effectively imports CPython's hostname matching code
from its ssl.py into sslutil.py. The hostname matching code itself
is pretty similar. However, the DNS name matching code is much more
robust and spec conformant.
As the test changes show, this changes some behavior around
wildcard handling and IDNA matching. The new behavior allows
wildcards in the middle of words (e.g. 'f*.com' matches 'foo.com')
This is spec compliant according to RFC 6125 Section 6.5.3 item 3.
There is one test where the matcher is more strict. Before,
'*.a.com' matched '.a.com'. Now it doesn't match. Strictly speaking
this is a security vulnerability.
See the inline comment for what's going on here.
There is magic built into the "ssl" module that ships with modern
CPython that knows how to load the system CA certificates on
Windows. Since we're not shipping a CA bundle with Mercurial,
if we're running on legacy CPython there's nothing we can do
to load CAs on Windows, so it makes sense to print a warning.
I don't anticipate many people will see this warning because
the official (presumed popular) Mercurial distributions on
Windows bundle Python and should be distributing a modern Python
capable of loading system CA certs.
The "certifi" Python package provides a distribution of the
Mozilla trusted CA certificates as a Python package. If it is
present, we assume the user intends it to be used and we use
it to provide the default CA certificates when certificates
are otherwise not configured.
It's worth noting that this behavior roughly matches the popular
"requests" package, which also attempts to use "certifi" if
present.
Before, devel.disableloaddefaultcerts only impacted the loading of
default certs via SSLContext. After this patch, the config option also
prevents sslutil._defaultcacerts() from being called.
This config option is meant to be used by tests to force no CA certs
to be loaded. Future patches will enable _defaultcacerts() to have
success more often. Without this change we can't reliably test the
failure to load CA certs. (This patch also likely fixes test failures
on some OS X configurations.)
Future patches will change _defaultcacerts() to do something
on platforms that aren't OS X. Change the comment and logged
message to reflect the future.
