Clean up some invalid TLS configuration branches

Author: Hugo Osvaldo Barrera

CHANGELOG.rst

@@ -26,8 +26,6 @@ Version 0.19.0
 - A new ``asyncio`` backend is now used. So far, this shows substantial speed
   improvements in ``discovery`` and ``metasync``, but little change in `sync`.
   This will likely continue improving over time. :gh:`906`
-- Support for `md5` and `sha1` certificate fingerprints has been dropped. If
-  you're validating certificate fingerprints, use `sha256` instead.
 - The ``google`` storage types no longer require ``requests-oauthlib``, but
   require ``python-aiohttp-oauthlib`` instead.
 - Vdirsyncer no longer includes experimental support for `EteSync
@@ -41,6 +39,22 @@ Version 0.19.0
 
 .. _etesync-dav: https://github.com/etesync/etesync-dav
 
+Changes to SSL configuration
+----------------------------
+
+Support for `md5` and `sha1` certificate fingerprints has been dropped. If
+you're validating certificate fingerprints, use `sha256` instead.
+
+# XXX: just make it one arg
+
+When using a custom `verify_fingerprint`, CA validation is always disabled.
+
+If `verify_fingerprint` is unset, CA verification is always active. Disabling
+both features is insecure and no longer supported.
+
+The `verify` parameter no longer takes boolean values, it is now optional and
+only takes a string to a custom CA for verification.
+
 Version 0.18.0
 ==============
 

docs/config.rst

@@ -175,7 +175,7 @@ CalDAV and CardDAV
         url = "..."
         #username = ""
         #password = ""
-        #verify = true
+        #verify = /path/to/custom_ca.pem
         #auth = null
         #useragent = "vdirsyncer/0.16.4"
         #verify_fingerprint = null
@@ -208,12 +208,10 @@ CalDAV and CardDAV
     :param url: Base URL or an URL to a calendar.
     :param username: Username for authentication.
     :param password: Password for authentication.
-    :param verify: Verify SSL certificate, default True. This can also be a
-        local path to a self-signed SSL certificate. See :ref:`ssl-tutorial`
-        for more information.
-    :param verify_fingerprint: Optional. SHA1 or MD5 fingerprint of the
-        expected server certificate. See :ref:`ssl-tutorial` for more
-        information.
+    :param verify: Optional. Local path to a self-signed SSL certificate.
+        See :ref:`ssl-tutorial` for more information.
+    :param verify_fingerprint: Optional. SHA256 fingerprint of the expected
+        server certificate. See :ref:`ssl-tutorial` for more information.
     :param auth: Optional. Either ``basic``, ``digest`` or ``guess``. The
         default is preemptive Basic auth, sending credentials even if server
         didn't request them. This saves from an additional roundtrip per
@@ -235,7 +233,7 @@ CalDAV and CardDAV
      url = "..."
      #username = ""
      #password = ""
-     #verify = true
+     #verify = /path/to/custom_ca.pem
      #auth = null
      #useragent = "vdirsyncer/0.16.4"
      #verify_fingerprint = null
@@ -244,12 +242,10 @@ CalDAV and CardDAV
    :param url: Base URL or an URL to an addressbook.
    :param username: Username for authentication.
    :param password: Password for authentication.
-   :param verify: Verify SSL certificate, default True. This can also be a
-                  local path to a self-signed SSL certificate. See
-                  :ref:`ssl-tutorial` for more information.
-   :param verify_fingerprint: Optional. SHA1 or MD5 fingerprint of the expected
-                              server certificate. See :ref:`ssl-tutorial` for
-                              more information.
+   :param verify: Optional. Local path to a self-signed SSL certificate.
+        See :ref:`ssl-tutorial` for more information.
+   :param verify_fingerprint: Optional. SHA256 fingerprint of the expected
+        server certificate. See :ref:`ssl-tutorial` for more information.
    :param auth: Optional. Either ``basic``, ``digest`` or ``guess``. The
                 default is preemptive Basic auth, sending credentials even if
                 server didn't request them. This saves from an additional
@@ -478,12 +474,10 @@ leads to an error.
     :param url: URL to the ``.ics`` file.
     :param username: Username for authentication.
     :param password: Password for authentication.
-    :param verify: Verify SSL certificate, default True. This can also be a
-        local path to a self-signed SSL certificate. See :ref:`ssl-tutorial`
-        for more information.
-    :param verify_fingerprint: Optional. SHA1 or MD5 fingerprint of the
-        expected server certificate. See :ref:`ssl-tutorial` for more
-        information.
+    :param verify: Optional. Local path to a self-signed SSL certificate.
+        See :ref:`ssl-tutorial` for more information.
+    :param verify_fingerprint: Optional. SHA256 fingerprint of the expected
+        server certificate. See :ref:`ssl-tutorial` for more information.
     :param auth: Optional. Either ``basic``, ``digest`` or ``guess``. The
         default is preemptive Basic auth, sending credentials even if server
         didn't request them. This saves from an additional roundtrip per

docs/ssl-tutorial.rst

@@ -15,21 +15,14 @@ To pin the certificate by fingerprint::
     type = "caldav"
     ...
     verify_fingerprint = "94:FD:7A:CB:50:75:A4:69:82:0A:F8:23:DF:07:FC:69:3E:CD:90:CA"
-    #verify = false  # Optional: Disable CA validation, useful for self-signed certs
 
-SHA1-, SHA256- or MD5-Fingerprints can be used. They're detected by their
-length.
+SHA256-Fingerprints can be used. CA validation is disabled when pinning a
+fingerprint.
 
 You can use the following command for obtaining a SHA-1 fingerprint::
 
     echo -n | openssl s_client -connect unterwaditzer.net:443 | openssl x509 -noout -fingerprint
 
-Note that ``verify_fingerprint`` doesn't suffice for vdirsyncer to work with
-self-signed certificates (or certificates that are not in your trust store). You
-most likely need to set ``verify = false`` as well. This disables verification
-of the SSL certificate's expiration time and the existence of it in your trust
-store, all that's verified now is the fingerprint.
-
 However, please consider using `Let's Encrypt <https://letsencrypt.org/>`_ such
 that you can forget about all of that. It is easier to deploy a free
 certificate from them than configuring all of your clients to accept the

tests/storage/test_http.py

@@ -126,5 +126,4 @@ def test_verify_false_disallowed(aio_connector):
     with pytest.raises(ValueError) as excinfo:
         HttpStorage(url="http://example.com", verify=False, connector=aio_connector)
 
-    assert "forbidden" in str(excinfo.value).lower()
-    assert "consider setting verify_fingerprint" in str(excinfo.value).lower()
+    assert "must be a path to a pem-file." in str(excinfo.value).lower()

tests/system/utils/test_main.py

@@ -35,12 +35,15 @@ async def test_request_ssl():
             )
         assert "certificate verify failed" in str(excinfo.value)
 
-        await http.request(
-            "GET",
-            "https://self-signed.badssl.com/",
-            verify=False,
-            session=session,
-        )
+        # XXX FIXME
+
+        with pytest.raises(Exception):
+            await http.request(
+                "GET",
+                "https://self-signed.badssl.com/",
+                verify=False,
+                session=session,
+            )
 
 
 def fingerprint_of_cert(cert, hash=hashes.SHA256) -> str:
@@ -62,10 +65,12 @@ async def test_request_ssl_leaf_fingerprint(
     httpserver.expect_request("/").respond_with_data("OK")
     url = f"https://127.0.0.1:{httpserver.port}/"
 
-    await http.request("GET", url, verify_fingerprint=fingerprint, session=aio_session)
+    ssl = http.prepare_verify(None, fingerprint)
+    await http.request("GET", url, ssl=ssl, session=aio_session)
 
+    ssl = http.prepare_verify(None, bogus)
     with pytest.raises(aiohttp.ServerFingerprintMismatch):
-        await http.request("GET", url, verify_fingerprint=bogus, session=aio_session)
+        await http.request("GET", url, ssl=ssl, session=aio_session)
 
 
 @pytest.mark.xfail(reason="Not implemented")

vdirsyncer/http.py

@@ -1,4 +1,5 @@
 import logging
+from ssl import create_default_context
 
 import aiohttp
 
@@ -64,30 +65,23 @@ def prepare_auth(auth, username, password):
 
 
 def prepare_verify(verify, verify_fingerprint):
-    if isinstance(verify, (str, bytes)):
-        verify = expand_path(verify)
-    elif not isinstance(verify, bool):
+    if isinstance(verify, str):
+        return create_default_context(cafile=expand_path(verify))
+    elif verify is not None:
         raise exceptions.UserError(
-            "Invalid value for verify ({}), "
-            "must be a path to a PEM-file or boolean.".format(verify)
+            f"Invalid value for verify ({verify}), must be a path to a PEM-file."
         )
 
     if verify_fingerprint is not None:
-        if not isinstance(verify_fingerprint, (bytes, str)):
+        if not isinstance(verify_fingerprint, str):
             raise exceptions.UserError(
                 "Invalid value for verify_fingerprint "
-                "({}), must be a string or null.".format(verify_fingerprint)
+                f"({verify_fingerprint}), must be a string."
             )
-    elif not verify:
-        raise exceptions.UserError(
-            "Disabling all SSL validation is forbidden. Consider setting "
-            "verify_fingerprint if you have a broken or self-signed cert."
-        )
 
-    return {
-        "verify": verify,
-        "verify_fingerprint": verify_fingerprint,
-    }
+        return aiohttp.Fingerprint(bytes.fromhex(verify_fingerprint.replace(":", "")))
+
+    return None
 
 
 def prepare_client_cert(cert):
@@ -99,15 +93,18 @@ def prepare_client_cert(cert):
 
 
 async def request(
-    method, url, session, latin1_fallback=True, verify_fingerprint=None, **kwargs
+    method,
+    url,
+    session,
+    latin1_fallback=True,
+    **kwargs,
 ):
-    """
-    Wrapper method for requests, to ease logging and mocking. Parameters should
-    be the same as for ``requests.request``, except:
+    """Wrapper method for requests, to ease logging and mocking.
+
+    Parameters should be the same as for ``aiohttp.request``, as well as:
 
     :param session: A requests session object to use.
-    :param verify_fingerprint: Optional. SHA1 or MD5 fingerprint of the
-        expected server certificate.
+    :param verify_fingerprint: Optional. SHA256 of the expected server certificate.
     :param latin1_fallback: RFC-2616 specifies the default Content-Type of
         text/* to be latin1, which is not always correct, but exactly what
         requests is doing. Setting this parameter to False will use charset
@@ -116,13 +113,7 @@ async def request(
         https://github.com/kennethreitz/requests/issues/2042
     """
 
-    if verify_fingerprint is not None:
-        ssl = aiohttp.Fingerprint(bytes.fromhex(verify_fingerprint.replace(":", "")))
-        kwargs.pop("verify", None)
-    elif kwargs.pop("verify", None) is False:
-        ssl = False
-    else:
-        ssl = None  # TODO XXX: Check all possible values for this
+    # TODO: Support for client-side certifications.
 
     session.hooks = {"response": _fix_redirects}
 
@@ -138,29 +129,29 @@ async def request(
 
     kwargs.pop("cert", None)  # TODO XXX FIXME!
 
-    r = await session.request(method, url, ssl=ssl, **kwargs)
+    response = await session.request(method, url, **kwargs)
 
     # See https://github.com/kennethreitz/requests/issues/2042
-    content_type = r.headers.get("Content-Type", "")
+    content_type = response.headers.get("Content-Type", "")
     if (
         not latin1_fallback
         and "charset" not in content_type
         and content_type.startswith("text/")
     ):
         logger.debug("Removing latin1 fallback")
-        r.encoding = None
+        response.encoding = None
 
-    logger.debug(r.status)
-    logger.debug(r.headers)
-    logger.debug(r.content)
+    logger.debug(response.status)
+    logger.debug(response.headers)
+    logger.debug(response.content)
 
-    if r.status == 412:
-        raise exceptions.PreconditionFailed(r.reason)
-    if r.status in (404, 410):
-        raise exceptions.NotFoundError(r.reason)
+    if response.status == 412:
+        raise exceptions.PreconditionFailed(response.reason)
+    if response.status in (404, 410):
+        raise exceptions.NotFoundError(response.reason)
 
-    r.raise_for_status()
-    return r
+    response.raise_for_status()
+    return response
 
 
 def _fix_redirects(r, *args, **kwargs):

vdirsyncer/storage/dav.py

@@ -375,7 +375,7 @@ def __init__(
         url,
         username="",
         password="",
-        verify=True,
+        verify=None,
         auth=None,
         useragent=USERAGENT,
         verify_fingerprint=None,
@@ -389,7 +389,10 @@ def __init__(
         auth = prepare_auth(auth, username, password)
         if auth:
             self._settings["auth"] = auth
-        self._settings.update(prepare_verify(verify, verify_fingerprint))
+
+        ssl = prepare_verify(verify, verify_fingerprint)
+        if ssl:
+            self._settings["ssl"] = ssl
 
         self.useragent = useragent
         self.url = url.rstrip("/") + "/"

vdirsyncer/storage/http.py

@@ -27,7 +27,7 @@ def __init__(
         url,
         username="",
         password="",
-        verify=True,
+        verify=None,
         auth=None,
         useragent=USERAGENT,
         verify_fingerprint=None,
@@ -45,7 +45,10 @@ def __init__(
         auth = prepare_auth(auth, username, password)
         if auth:
             self._settings["auth"] = auth
-        self._settings.update(prepare_verify(verify, verify_fingerprint))
+
+        ssl = prepare_verify(verify, verify_fingerprint)
+        if ssl:
+            self._settings["ssl"] = ssl
 
         self.username, self.password = username, password
         self.useragent = useragent