Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove MD5 & SHA1 from certificate validation #1186

Closed
rugk opened this issue Sep 17, 2016 · 8 comments
Closed

Remove MD5 & SHA1 from certificate validation #1186

rugk opened this issue Sep 17, 2016 · 8 comments
Labels

Comments

@rugk
Copy link

rugk commented Sep 17, 2016

Please remove SHA-1 and MD5 as options for verifying certificates. MD5 is known to be broken for decades and SHA-1 is considered insecure and being depreciated.

Just do not offer these insecure options anymore, so that developers cannot use them. Get security by default!

@asvetlov asvetlov added good first issue Good for newcomers client labels Sep 18, 2016
@asvetlov
Copy link
Member

We cannot just suddenly remove something without very strong reason.
What can we do is:

  1. Update documentation by suggesting SHA256 only and declaring MD5/SHA-1 deprecation
  2. Raise a warning on MD5 or SHA-1 usage but keep supporting them for 1.5 years.

Volunteer is needed.

@rugk
Copy link
Author

rugk commented Sep 18, 2016

Yes this would be good. Although I would argue to drop MD5 support earlier than SHA-1. (maybe MD5: 0.5 years, SHA1: 1.5y)

@Taekyoon
Copy link
Contributor

Taekyoon commented Oct 9, 2016

Can I put some messages when developers choose 'MD5' or 'SHA-1' to the functions?
And If it's okay, should I just put comment on code or print in console?

@asvetlov
Copy link
Member

asvetlov commented Oct 9, 2016

@Taekyoon I don't follow.
From my perspective it should be warnings.warn call along with logging.warning

@jake-jake-jake
Copy link
Contributor

I've added a warning.warn call and have updated the docs to show that the older hashes are insecure and deprecated on this branch on my fork. What is your preferred method of testing this behavior? (I am not super familiar with PyTest.)

@asvetlov
Copy link
Member

These tests: https://github.com/KeepSafe/aiohttp/blob/master/tests/test_client_functional.py#L256-L308 should raise warning on deprecated fingerprints.
Warning could be caught by with pytest.warns(DeprecationWarning): ... context manager.

@asvetlov
Copy link
Member

Fixed by #1341

@lock
Copy link

lock bot commented Oct 29, 2019

This thread has been automatically locked since there has not been
any recent activity after it was closed. Please open a new issue for
related bugs.

If you feel like there's important points made in this discussion,
please include those exceprts into that new issue.

@lock lock bot added the outdated label Oct 29, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

4 participants