Skip to content

Conversation

@untitaker
Copy link
Member

@untitaker
Copy link
Member Author

Sorry for the spam. So i guess any changes can be filed against this branch.

@untitaker
Copy link
Member Author

Maybe https://pypi.python.org/pypi/pytest-localserver can be used.

@t-8ch
Copy link
Collaborator

t-8ch commented Aug 20, 2014

pytest-localserver depends on pyOpenSSL, are you fine with this?

@untitaker
Copy link
Member Author

I don't care that much about the deps of the testsuite, so i'm fine with that.

On Wed, Aug 20, 2014 at 12:25:13PM -0700, Thomas Weißschuh wrote:

pytest-localserver depends on pyOpenSSL, are you fine with this?


Reply to this email directly or view it on GitHub:
#106 (comment)

@untitaker
Copy link
Member Author

@t-8ch Should i add you as a collaborator so you can push directly to this? (Please don't merge yet though)

@t-8ch
Copy link
Collaborator

t-8ch commented Aug 20, 2014

@untitaker It turns out, that the current fingerprint logic only checks fingerprints if it also validates the certficate itself (If it is trusted by a CA, not if the name matches, which is rather broken).
This has to be fixed in urllib3 and pulled into requests... I'll prepare a pull there.

@untitaker
Copy link
Member Author

@t-8ch I added you as a collaborator.

I'd like to make this work regardless of the version of requests installed. If you have created a PR for requests, please post here then.

@t-8ch
Copy link
Collaborator

t-8ch commented Aug 20, 2014

Blocked on urllib3/urllib3#444

@untitaker
Copy link
Member Author

So the end user has to set verify to None and tls_fingerprint to something? Is setting verify to True and also setting tls_fingerprint still an option?

@untitaker
Copy link
Member Author

I renamed this to verify_fingerprint because i think the name shows better that these params are related.

@t-8ch
Copy link
Collaborator

t-8ch commented Aug 20, 2014

currently verify=None also disables fingerprint verification.
settings verify_fingerprint verifies the fingerprint instead of the hostname.
The certificate still needs to be trusted by a CA in the trust chain.
This is rather broken, hence the PR to urllib3

@untitaker
Copy link
Member Author

I mean, assuming we are running a urllib3 version with your PR merged.

@t-8ch
Copy link
Collaborator

t-8ch commented Aug 20, 2014

Yes, then you would set verify=None and verify_fingerprint=abc. We could also set verify=None when we get an fingerprint

@untitaker
Copy link
Member Author

We could also set verify=None when we get an fingerprint

I'd rather keep as strict defaults as possible.

@untitaker
Copy link
Member Author

I added some tests, but they fail on my machine. I am able to start a local server using the CLI of pytest-localserver and get some SSLErrors through manual testing, but in the test env requests seems to be happy with the certificate. I don't see any indication that the pytest plugin monkeypatches requests for cert verification.

@t-8ch
Copy link
Collaborator

t-8ch commented Aug 20, 2014

I confused verify=None with verify=False in my posts. At the moment verify=False also disables fingerprint verification.

@untitaker
Copy link
Member Author

My fault -- a different test didn't properly clean up after monkeypatching.

@t-8ch
Copy link
Collaborator

t-8ch commented Aug 29, 2014

The issue in urllib3 has been fixed and released in requests 2.4.0. We can depend on the old version of werkzeug requiring pyopenssl and later switch to the new one.

@t-8ch t-8ch closed this Aug 29, 2014
@t-8ch t-8ch reopened this Aug 29, 2014
@t-8ch
Copy link
Collaborator

t-8ch commented Aug 29, 2014

The testsuite is now blocked on kennethreitz/requests#2192

@untitaker
Copy link
Member Author

We can't depend on a old version of Werkzeug, neither work with Py3.

@untitaker
Copy link
Member Author

Ah nvm i see you already commited something. Well, that's a solution too.

@untitaker
Copy link
Member Author

Actually i fixed the issue with requests by simply catching more errors: vdirsyncer/owncloud-testserver@6499b76

@untitaker
Copy link
Member Author

Is there anything stopping this PR now?

@t-8ch
Copy link
Collaborator

t-8ch commented Aug 30, 2014

:shipit:

untitaker added a commit that referenced this pull request Aug 30, 2014
@untitaker untitaker merged commit 686441b into master Aug 30, 2014
@untitaker untitaker deleted the tls_fingerprints branch September 14, 2014 10:20
jsonn pushed a commit to jsonn/pkgsrc that referenced this pull request Oct 2, 2014
Version 0.3.0
=============

*released on 20 September 2014*

- Add ``verify_fingerprint`` parameter to
  :py:class:`vdirsyncer.storage.HttpStorage`,
  :py:class:`vdirsyncer.storage.CaldavStorage` and
  :py:class:`vdirsyncer.storage.CarddavStorage`,
  see issue `#99`_ and pull request `#106`_.

- Add ``passwordeval`` parameter to :ref:`general_config`, see issue `#108`_
  and pull request `#117`_.

- Emit warnings (instead of exceptions) about certain invalid responses from
  the server, see issue `#113`_.  This is apparently required for compatibility
  with Davmail.

.. _`#99`: pimutils/vdirsyncer#99
.. _`#106`: pimutils/vdirsyncer#106
.. _`#108`: pimutils/vdirsyncer#108
.. _`#113`: pimutils/vdirsyncer#113
.. _`#117`: pimutils/vdirsyncer#117
jsonn pushed a commit to jsonn/pkgsrc that referenced this pull request Oct 11, 2014
Version 0.3.0
=============

*released on 20 September 2014*

- Add ``verify_fingerprint`` parameter to
  :py:class:`vdirsyncer.storage.HttpStorage`,
  :py:class:`vdirsyncer.storage.CaldavStorage` and
  :py:class:`vdirsyncer.storage.CarddavStorage`,
  see issue `#99`_ and pull request `#106`_.

- Add ``passwordeval`` parameter to :ref:`general_config`, see issue `#108`_
  and pull request `#117`_.

- Emit warnings (instead of exceptions) about certain invalid responses from
  the server, see issue `#113`_.  This is apparently required for compatibility
  with Davmail.

.. _`#99`: pimutils/vdirsyncer#99
.. _`#106`: pimutils/vdirsyncer#106
.. _`#108`: pimutils/vdirsyncer#108
.. _`#113`: pimutils/vdirsyncer#113
.. _`#117`: pimutils/vdirsyncer#117
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants