diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 748fc4e..c2c078a 100644 --- a/CHANGELOG.rst +++ b/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 ============= diff --git a/docs/ssl-tutorial.rst b/docs/ssl-tutorial.rst index a74dc5a..d784b36 100644 --- a/docs/ssl-tutorial.rst +++ b/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 -with SSL have two parameters: ``verify`` and ``verify_fingerprint``. +Pinning by fingerprint +---------------------- -- The ``verify`` parameter determines whether to verify SSL certificates the - way browsers do: By comparing against a trust store, and by checking the - certificate's expiration date. +To pin the certificate by SHA1- or MD5-fingerprint:: - 1. The default, ``true``, means that certificates will be validated against a - set of trusted CAs. See :ref:`ssl-cas`. + [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" - 2. The value ``false`` will disable both trusted-CA-validation and the - validation of the certificate's expiration date. Unless combined with - ``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. +This disables :ref:`validation against root CAs `, since +``verify_fingerprint`` is much stricter anyway. .. _ssl-cas: -Trusted CAs ------------ +Custom root CAs +--------------- -As said, vdirsyncer uses the requests_ library, which, by default, `uses its -own set of trusted CAs +To point vdirsyncer to a custom set of root 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 `_. 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 -bet is explicitly setting the SSL options above. +you to not care. + +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/ diff --git a/tests/storage/test_http.py b/tests/storage/test_http.py index ecae7b0..f2a5c8b 100644 --- a/tests/storage/test_http.py +++ b/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() diff --git a/vdirsyncer/cli/utils.py b/vdirsyncer/cli/utils.py index 15d9ea8..4c92df5 100644 --- a/vdirsyncer/cli/utils.py +++ b/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('') - problems.append('Internal error, see above.') + if not isinstance(e, exceptions.UserError): + cli_logger.exception('') + problems.append(str(e)) raise CliError(u'Failed to initialize {}'.format(config['instance_name']), problems=problems) diff --git a/vdirsyncer/exceptions.py b/vdirsyncer/exceptions.py index a4d960e..a5e541b 100644 --- a/vdirsyncer/exceptions.py +++ b/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''' diff --git a/vdirsyncer/storage/base.py b/vdirsyncer/storage/base.py index 040467d..48222e0 100644 --- a/vdirsyncer/storage/base.py +++ b/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: diff --git a/vdirsyncer/storage/dav.py b/vdirsyncer/storage/dav.py index 6182928..ca77b44 100644 --- a/vdirsyncer/storage/dav.py +++ b/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, ' - 'end_date has to be given too.') + raise exceptions.UserError('If start_date is given, ' + '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 = \ diff --git a/vdirsyncer/storage/http.py b/vdirsyncer/storage/http.py index 61d8551..b4283fa 100644 --- a/vdirsyncer/storage/http.py +++ b/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 ' - 'old for `guess` authentication. At least ' - 'version 0.4.0 is required.') + raise exceptions.UserError( + 'Your version of requests_toolbelt is too ' + '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 ' - 'and password.'.format(auth)) + raise exceptions.UserError('You need to specify username and password ' + '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) - return verify + verify = expand_path(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) diff --git a/vdirsyncer/storage/memory.py b/vdirsyncer/storage/memory.py index 90d0767..8a32f3d 100644 --- a/vdirsyncer/storage/memory.py +++ b/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) diff --git a/vdirsyncer/storage/singlefile.py b/vdirsyncer/storage/singlefile.py index 76027aa..45ca28f 100644 --- a/vdirsyncer/storage/singlefile.py +++ b/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 {}' - .format(type(self).__name__)) + raise exceptions.UserError( + 'collection is not a valid argument for {}' + .format(type(self).__name__) + ) checkfile(path, create=False) diff --git a/vdirsyncer/sync.py b/vdirsyncer/sync.py index f04c3bd..f57a821 100644 --- a/vdirsyncer/sync.py +++ b/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: {}' - .format(conflict_resolution)) + raise exceptions.UserError('Invalid conflict resolution mode: {}' + .format(conflict_resolution)) return inner