Merge pull request #197 from untitaker/mustverify

Disallow verify=false

CHANGELOG.rst

@@ -19,6 +19,8 @@ Version 0.5.0
   output.
 - Fix compatibility with iCloud again.
 - Use only one worker if debug mode is activated.
+- ``verify=false`` is now disallowed in vdirsyncer, please use
+  ``verify_fingerprint`` instead.
 
 Version 0.4.4
 =============

docs/ssl-tutorial.rst

@@ -4,57 +4,49 @@
 SSL and certificate validation
 ==============================
 
-Vdirsyncer uses the requests_ library for all its HTTP and SSL interaction.
+All SSL configuration is done per-storage.
 
-All SSL configuration is done per-storage. Storages that have anything to do
+Pinning by fingerprint
-with SSL have two parameters: ``verify`` and ``verify_fingerprint``.
+----------------------
 
-- The ``verify`` parameter determines whether to verify SSL certificates the
+To pin the certificate by SHA1- or MD5-fingerprint::
-  way browsers do: By comparing against a trust store, and by checking the
-  certificate's expiration date.
 
-  1. The default, ``true``, means that certificates will be validated against a
+    [storage foo]
-     set of trusted CAs. See :ref:`ssl-cas`.
+    type = caldav
+    ...
+    verify_fingerprint = "94:FD:7A:CB:50:75:A4:69:82:0A:F8:23:DF:07:FC:69:3E:CD:90:CA"
 
-  2. The value ``false`` will disable both trusted-CA-validation and the
+This disables :ref:`validation against root CAs <ssl-cas>`, since
-     validation of the certificate's expiration date. Unless combined with
+``verify_fingerprint`` is much stricter anyway.
-     ``verify_fingerprint``, **you should not use this value at all, because
-     it's a security risk**.
-
-  3. You can also set ``verify`` to a path of the server's certificate in PEM
-     format, instead of relying on the default root CAs::
-
-         [storage foo]
-         type = caldav
-         ...
-         verify = "/path/to/cert.pem"
-
-- The ``verify_fingerprint`` parameter can be used to compare the SSL
-  fingerprint to a fixed value. The value can be either a SHA1-fingerprint or
-  an MD5 one::
-
-      [storage foo]
-      type = caldav
-      ...
-      verify_fingerprint = "94:FD:7A:CB:50:75:A4:69:82:0A:F8:23:DF:07:FC:69:3E:CD:90:CA"
-
-  Using it will implicitly set ``verify=False``, which means that the pinned
-  certificate doesn't have to be by a trusted CA to be accepted by vdirsyncer.
 
 .. _ssl-cas:
 
-Trusted CAs
+Custom root CAs
------------
+---------------
 
-As said, vdirsyncer uses the requests_ library, which, by default, `uses its
+To point vdirsyncer to a custom set of root CAs::
-own set of trusted CAs
+
+    [storage foo]
+    type = caldav
+    ...
+    verify = "/path/to/cert.pem"
+
+Vdirsyncer uses the requests_ library, which, by default, `uses its own set of
+trusted CAs
 <http://www.python-requests.org/en/latest/user/advanced/#ca-certificates>`_.
 
 However, the actual behavior depends on how you have installed it. Some Linux
 distributions, such as Debian, patch their ``python-requests`` package to use
 the system certificate CAs. Normally these two stores are similar enough for
-you not to care. If the behavior on your system is somehow confusing, your best
+you to not care.
-bet is explicitly setting the SSL options above.
+
+But there are cases where certificate validation fails even though you can
+access the server fine through e.g. your browser. This usually indicates that
+your installation of the ``requests`` library is somehow broken. In such cases,
+it makes sense to explicitly set ``verify`` or ``verify_fingerprint`` as shown
+above.
+
+.. _requests: http://www.python-requests.org/
 
 .. _ssl-client-certs:
 
@@ -75,5 +67,3 @@ If the key and certificate are separate, a list may be used::
    type = caldav
    ...
    auth_cert = ["/path/to/certificate.crt", "/path/to/key.key"]
-
-.. _requests: http://www.python-requests.org/

tests/storage/test_http.py

@@ -6,6 +6,7 @@ from requests import Response
 
 from tests import normalize_item
 
+from vdirsyncer.exceptions import UserError
 from vdirsyncer.storage.http import HttpStorage, prepare_auth
 
 
@@ -107,7 +108,15 @@ def test_prepare_auth_guess(monkeypatch, auth):
 
     monkeypatch.delattr(requests_toolbelt.auth.guess, 'GuessAuth')
 
-    with pytest.raises(RuntimeError) as excinfo:
+    with pytest.raises(UserError) as excinfo:
         prepare_auth(auth, 'user', 'pwd')
 
     assert 'requests_toolbelt is too old' in str(excinfo.value).lower()
+
+
+def test_verify_false_disallowed():
+    with pytest.raises(ValueError) as excinfo:
+        HttpStorage(url='http://example.com', verify=False)
+
+    assert 'forbidden' in str(excinfo.value).lower()
+    assert 'consider setting verify_fingerprint' in str(excinfo.value).lower()

vdirsyncer/cli/utils.py

@@ -495,8 +495,9 @@ def handle_storage_init_error(cls, config):
             .format(cls.storage_name, u', '.join(invalid)))
 
     if not problems:
-        cli_logger.exception('')
+        if not isinstance(e, exceptions.UserError):
-        problems.append('Internal error, see above.')
+            cli_logger.exception('')
+        problems.append(str(e))
 
     raise CliError(u'Failed to initialize {}'.format(config['instance_name']),
                    problems=problems)

vdirsyncer/exceptions.py

@@ -17,6 +17,11 @@ class Error(Exception):
         super(Error, self).__init__(*args)
 
 
+class UserError(Error, ValueError):
+    '''Wrapper exception to be used to signify the traceback should not be
+    shown to the user.'''
+
+
 class CollectionNotFound(Error):
     '''Collection not found'''
 

vdirsyncer/storage/base.py

@@ -77,7 +77,7 @@ class Storage(with_metaclass(StorageMeta)):
         if read_only is None:
             read_only = self.read_only
         if self.read_only and not read_only:
-            raise ValueError('This storage can only be read-only.')
+            raise exceptions.UserError('This storage can only be read-only.')
         self.read_only = bool(read_only)
 
         if collection and instance_name:

vdirsyncer/storage/dav.py

@@ -215,11 +215,11 @@ class DavSession(object):
             password = get_password(username, url)
 
         self._settings = {
-            'verify': prepare_verify(verify),
             'auth': prepare_auth(auth, username, password),
-            'verify_fingerprint': verify_fingerprint,
             'cert': prepare_client_cert(auth_cert),
         }
+        self._settings.update(prepare_verify(verify, verify_fingerprint))
+
         self.useragent = useragent
         self.url = url.rstrip('/') + '/'
         self.parsed_url = utils.compat.urlparse.urlparse(self.url)
@@ -591,12 +591,12 @@ class CaldavStorage(DavStorage):
                  item_types=(), **kwargs):
         super(CaldavStorage, self).__init__(**kwargs)
         if not isinstance(item_types, (list, tuple)):
-            raise ValueError('item_types must be a list.')
+            raise exceptions.UserError('item_types must be a list.')
 
         self.item_types = tuple(item_types)
         if (start_date is None) != (end_date is None):
-            raise ValueError('If start_date is given, '
+            raise exceptions.UserError('If start_date is given, '
-                             'end_date has to be given too.')
+                                       'end_date has to be given too.')
         elif start_date is not None and end_date is not None:
             namespace = dict(datetime.__dict__)
             namespace['start_date'] = self.start_date = \

vdirsyncer/storage/http.py

@@ -1,7 +1,7 @@
 # -*- coding: utf-8 -*-
 
 from .base import Item, Storage
-from ..exceptions import NotFoundError
+from .. import exceptions
 from ..utils import expand_path
 from ..utils.compat import iteritems, text_type, urlparse
 from ..utils.http import request
@@ -22,24 +22,46 @@ def prepare_auth(auth, username, password):
             try:
                 from requests_toolbelt.auth.guess import GuessAuth
             except ImportError:
-                raise RuntimeError('Your version of requests_toolbelt is too '
+                raise exceptions.UserError(
-                                   'old for `guess` authentication. At least '
+                    'Your version of requests_toolbelt is too '
-                                   'version 0.4.0 is required.')
+                    'old for `guess` authentication. At least '
+                    'version 0.4.0 is required.'
+                )
             else:
                 return GuessAuth(username, password)
         else:
-            raise ValueError('Unknown authentication method: {}'.format(auth))
+            raise exceptions.UserError('Unknown authentication method: {}'
+                                       .format(auth))
     elif auth:
-        raise ValueError('For {} authentication, you need to specify username '
+        raise exceptions.UserError('You need to specify username and password '
-                         'and password.'.format(auth))
+                                   'for {} authentication.'.format(auth))
     else:
         return None
 
 
-def prepare_verify(verify):
+def prepare_verify(verify, verify_fingerprint):
     if isinstance(verify, (text_type, bytes)):
-        return expand_path(verify)
+        verify = expand_path(verify)
-    return verify
+    elif not isinstance(verify, bool):
+        raise exceptions.UserError('Invalid value for verify ({}), '
+                                   'must be a path to a PEM-file or boolean.'
+                                   .format(verify))
+
+    if verify_fingerprint is not None:
+        if not isinstance(verify_fingerprint, str):
+            raise exceptions.UserError('Invalid value for verify_fingerprint '
+                                       '({}), must be a string or null.'
+                                       .format(verify_fingerprint))
+        verify = False
+    elif not verify:
+        raise exceptions.UserError('verify = false is forbidden. Consider '
+                                   'setting verify_fingerprint instead, which '
+                                   'also disables validation against CAs.')
+
+    return {
+        'verify': verify,
+        'verify_fingerprint': verify_fingerprint,
+    }
 
 
 def prepare_client_cert(cert):
@@ -105,12 +127,12 @@ class HttpStorage(Storage):
             password = get_password(username, url)
 
         self._settings = {
-            'verify': prepare_verify(verify),
-            'verify_fingerprint': verify_fingerprint,
             'auth': prepare_auth(auth, username, password),
             'cert': prepare_client_cert(auth_cert),
             'latin1_fallback': False,
         }
+        self._settings.update(prepare_verify(verify, verify_fingerprint))
+
         self.username, self.password = username, password
         self.useragent = useragent
 
@@ -142,4 +164,4 @@ class HttpStorage(Storage):
         try:
             return self._items[href]
         except KeyError:
-            raise NotFoundError(href)
+            raise exceptions.NotFoundError(href)

vdirsyncer/storage/memory.py

@@ -18,7 +18,8 @@ class MemoryStorage(Storage):
 
     def __init__(self, fileext='', **kwargs):
         if kwargs.get('collection') is not None:
-            raise ValueError('MemoryStorage does not support collections.')
+            raise exceptions.UserError('MemoryStorage does not support '
+                                       'collections.')
         self.items = {}  # href => (etag, item)
         self.fileext = fileext
         super(MemoryStorage, self).__init__(**kwargs)

vdirsyncer/storage/singlefile.py

@@ -77,8 +77,10 @@ class SingleFileStorage(Storage):
 
         collection = kwargs.get('collection')
         if collection is not None:
-            raise ValueError('collection is not a valid argument for {}'
+            raise exceptions.UserError(
-                             .format(type(self).__name__))
+                'collection is not a valid argument for {}'
+                .format(type(self).__name__)
+            )
 
         checkfile(path, create=False)
 

vdirsyncer/sync.py

@@ -278,8 +278,8 @@ def _action_conflict_resolve(ident):
             sync_logger.info('...{} wins.'.format(b.storage))
             _action_update(ident, b, a)(a, b, conflict_resolution)
         else:
-            raise ValueError('Invalid conflict resolution mode: {}'
+            raise exceptions.UserError('Invalid conflict resolution mode: {}'
-                             .format(conflict_resolution))
+                                       .format(conflict_resolution))
 
     return inner