Use cert_reqs=CERT_REQUIRED by default #1507
Conversation
|
Yay excited about this one. :) Should we bolt on more docs to cert failure exceptions so people know what to do? |
Only if those docs are actually helpful |
|
I'll take a look at the docs as well tonight. |
Codecov Report
@@ Coverage Diff @@
## master #1507 +/- ##
==========================================
- Coverage 99.94% 99.94% -0.01%
==========================================
Files 22 22
Lines 1809 1806 -3
==========================================
- Hits 1808 1805 -3
Misses 1 1
Continue to review full report at Codecov.
|
docs/user-guide.rst
Outdated
| @@ -202,8 +202,13 @@ recommended to set the ``Content-Type`` header:: | |||
| Certificate verification | |||
| ------------------------ | |||
|
|
|||
| .. note:: *New in version 1.25* | |||
|
|
|||
| HTTPS connections are now verified by default | |||
There was a problem hiding this comment.
IMO
HTTPS connections are now verified by default (
cert_reqs = 'CERT_REQUIRED').
is sufficient.
The "regardless of CA certs are specified" might add some unnecessary confusion.
(Also s/of/if/).
docs/user-guide.rst
Outdated
| HTTPS connections are now verified by default | ||
| (``cert_reqs = 'CERT_REQUIRED'``) regardless of whether | ||
| CA certs are specified. | ||
|
|
||
| It is highly recommended to always use SSL certificate verification. |
There was a problem hiding this comment.
Could probably remove this sentence now.
There was a problem hiding this comment.
Let's reword it, maybe "While you can disable certification verification, it is highly recommend to leave it on."
docs/user-guide.rst
Outdated
| HTTPS connections are now verified by default | ||
| (``cert_reqs = 'CERT_REQUIRED'``) regardless of whether | ||
| CA certs are specified. | ||
|
|
||
| It is highly recommended to always use SSL certificate verification. |
There was a problem hiding this comment.
Let's reword it, maybe "While you can disable certification verification, it is highly recommend to leave it on."
docs/user-guide.rst
Outdated
| It is highly recommended to always use SSL certificate verification. | ||
| **By default, urllib3 does not verify HTTPS requests**. | ||
|
|
||
| In order to enable verification you will need a set of root certificates. The easiest |
There was a problem hiding this comment.
I don't know if this was ever completely true? You could always set cert_reqs to REQUIRED and we'd load the default verification locations.
So now by default urllib3 tries to load the default certificate locations via SSLContext.load_default_certs() which apparently works well on Windows and on non-Windows uses SSLContext.set_default_verify_locations() which is also reliable in my experience (Available in Python 2.7.9+? and Python 3.4+).
The only platforms that will have to use certifi or their own bundle are platforms that don't have SSLContext.load_default_certs() or don't define SSLContext at all because our implementation of SSLContext doesn't define load_default_certs() (Should our implementation try to implement this?).
As for contrib/: PyOpenSSLContext defines a functioning load_default_certs() and SecureTransportContext defines the method but does absolutely nothing with it so maybe it will fail cert verification unless SecureTransport loads certs by default? Will have to look at docs.
To sum it up, it's a mixed bag of whether we load system certs if no certificate locations are given. :) But now we'll fail loudly by default instead of emitting a warning.
There was a problem hiding this comment.
Then we should probs remove this, yeah?
There was a problem hiding this comment.
When you say this you mean the whole section on certifi? I'm not sure what your comment is directed at. If we remove that subsection then the section as a whole is pretty bare and the last part mentions system certificates as well. Maybe this whole section needs a rewrite?
|
Thanks for the comments, I've updated the documentation in the areas mentioned. What do we think about the wordings now? |
docs/user-guide.rst
Outdated
|
|
||
| While you can disable certification verification, it is highly recommend to leave it on. | ||
|
|
||
| For certificate verification to work properly you will need a set of root certificates. Unless otherwise |
There was a problem hiding this comment.
For certificate verification to work properly you will need a set of root certificates. Unless otherwise specified urllib3 will try to load the default system certificate stores.
Aren't these two sentences contradictory? The first one sounds like you have to do something additional for urllib3 to work at all, then the second sentence sounds like it does something sensible by default and you don't need to do anything.
IMO remove the first sentence, or rephrase.
There was a problem hiding this comment.
Sounds good to me :)
|
🍮 |
|
We can change the documentation in a separate PR, would like to merge this change into the TLS 1.3 PR so we can finally land that monster. Thanks all for reviewing :) |
resolve_cert_reqs was changed in pull urllib3#1507 but the docstring was left with its old default: update to new CERT_REQUIRED default.
Initial implementation of requiring and validating certificates when using HTTPS by default.
I'm sure I'll have to rewrite some tests, will work on that after I see all the failures. :)
The
InsecureRequestWarningwill still be emitted when explicitly usingCERT_NONEorCERT_OPTIONAL.Closes #1454.