Add support for verifying ssl certificates #596

lukasstraub2 · 2020-12-03T14:44:47Z

Description

For Security reasons it may be desirable to reject invalid certificates. This adds configuration
options to configure this.

Fixes: #594

Types of changes

Replay fix (fixes a replay specific issue)
Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My change requires a change to the documentation.
I have updated the documentation accordingly.
I have added or updated tests to cover my changes.
All new and existing tests passed.

lukasstraub2 · 2020-12-03T17:18:25Z

Weird, the tests pass on my side.

lukasstraub2 · 2020-12-10T16:26:28Z

Ping...

ikreymer · 2020-12-15T20:38:31Z

Hi, apologies for delay! There appears to be an issue appveyor currently.

This looks good, though I think want to make the ca_certs_dir optional. Can you default it to empty string and also a typo in the docs.

Thanks!

ikreymer · 2020-12-15T20:39:21Z

docs/manual/configuring.rst

+By default, SSL-Certificates of websites are not verified. To enable verification, add the following to the config::
+
+  certificates:
+    cert_req: 'CERT_REQUIRED'


Suggested change

cert_req: 'CERT_REQUIRED'

cert_reqs: 'CERT_REQUIRED'

typo

ikreymer · 2020-12-15T20:39:52Z

pywb/warcserver/http.py

@@ -17,8 +17,10 @@ class PywbHttpAdapter(HTTPAdapter):
    until a better solution is found
    """

-    # todo: allow configuring this later?
-    cert_reqs = 'CERT_NONE'
+    def __init__(self, cert_reqs='CERT_NONE', ca_cert_dir='/etc/ssl/certs', **init_kwargs):


Suggested change

def __init__(self, cert_reqs='CERT_NONE', ca_cert_dir='/etc/ssl/certs', **init_kwargs):

def __init__(self, cert_reqs='CERT_NONE', ca_cert_dir=None, **init_kwargs):

ikreymer · 2020-12-15T20:40:28Z

pywb/warcserver/warcserver.py

+            certs_config = self.config['certificates']
+            DefaultAdapters.live_adapter = PywbHttpAdapter(max_retries=Retry(3),
+                                                           cert_reqs=certs_config.get('cert_reqs', 'CERT_NONE'),
+                                                           ca_cert_dir=certs_config.get('ca_cert_dir', '/etc/ssl/certs'))


Suggested change

ca_cert_dir=certs_config.get('ca_cert_dir', '/etc/ssl/certs'))

ca_cert_dir=certs_config.get('ca_cert_dir'))

Want to make this optional.

ikreymer · 2020-12-15T20:40:48Z

pywb/warcserver/warcserver.py

+                                                           ca_cert_dir=certs_config.get('ca_cert_dir', '/etc/ssl/certs'))
+            DefaultAdapters.remote_adapter = PywbHttpAdapter(max_retries=Retry(3),
+                                                             cert_reqs=certs_config.get('cert_reqs', 'CERT_NONE'),
+                                                             ca_cert_dir=certs_config.get('ca_cert_dir', '/etc/ssl/certs'))


Suggested change

ca_cert_dir=certs_config.get('ca_cert_dir', '/etc/ssl/certs'))

ca_cert_dir=certs_config.get('ca_cert_dir'))

ikreymer · 2020-12-15T20:42:03Z

docs/manual/configuring.rst

+    cert_req: 'CERT_REQUIRED'
+    ca_cert_dir: '/etc/ssl/certs'
+
+``ca_cert_dir`` should point to a directory containing the CA certificates that you trust. Most linux distributions provide CA certificates via a package called ``ca-certificates``.


Suggested change

``ca_cert_dir`` should point to a directory containing the CA certificates that you trust. Most linux distributions provide CA certificates via a package called ``ca-certificates``.

``ca_cert_dir`` can optionally point to a directory containing the CA certificates that you trust. Most linux distributions provide CA certificates via a package called ``ca-certificates``.

If omitted, the default system CA used by Python is used.

Signed-off-by: Lukas Straub <lukasstraub2@web.de>

lukasstraub2 · 2020-12-16T17:15:42Z

Thank you for your Review. I addressed your comments.

codecov · 2020-12-16T19:02:33Z

Codecov Report

Merging #596 (e9b64ec) into master (7b51101) will decrease coverage by 0.44%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #596      +/-   ##
==========================================
- Coverage   87.69%   87.24%   -0.45%     
==========================================
  Files          64       64              
  Lines        8096     8109      +13     
  Branches     1445     1446       +1     
==========================================
- Hits         7100     7075      -25     
- Misses        640      667      +27     
- Partials      356      367      +11

Impacted Files	Coverage Δ
pywb/warcserver/http.py	`100.00% <100.00%> (ø)`
pywb/warcserver/warcserver.py	`78.21% <100.00%> (+0.75%)`	⬆️
pywb/utils/geventserver.py	`68.42% <0.00%> (-7.90%)`	⬇️
pywb/rewrite/rewriteinputreq.py	`71.87% <0.00%> (-6.25%)`	⬇️
pywb/apps/frontendapp.py	`82.85% <0.00%> (-5.15%)`	⬇️
pywb/apps/rewriterapp.py	`83.22% <0.00%> (-4.68%)`	⬇️
pywb/utils/loaders.py	`91.24% <0.00%> (-1.46%)`	⬇️
pywb/recorder/multifilewarcwriter.py	`76.83% <0.00%> (-1.13%)`	⬇️
pywb/rewrite/url_rewriter.py	`87.00% <0.00%> (-1.00%)`	⬇️
pywb/warcserver/index/aggregator.py	`91.47% <0.00%> (+1.55%)`	⬆️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7b51101...e9b64ec. Read the comment docs.

ikreymer · 2020-12-16T19:12:07Z

Thanks! Could you add a quick test case, perhaps in the main tests directory?

You can just add a custom config yaml and then call a live endpoint, maybe something like:
/live/mp_/https://expired.badssl.com/ which should trigger the check?

You can look at a small test file like: https://github.com/webrecorder/pywb/blob/master/tests/test_root_coll.py
as an example.

Signed-off-by: Lukas Straub <lukasstraub2@web.de>

lukasstraub2 · 2020-12-23T13:05:45Z

I added a testcase.

ikreymer reviewed Dec 15, 2020

View reviewed changes

lukasstraub2 added 2 commits December 16, 2020 17:57

Add support for verifying ssl certificates

959d8e4

Signed-off-by: Lukas Straub <lukasstraub2@web.de>

Add documentation for new certificate configuration options

8e3ca71

Signed-off-by: Lukas Straub <lukasstraub2@web.de>

lukasstraub2 force-pushed the ssl_verify branch from eb5ee5b to 8e3ca71 Compare December 16, 2020 17:03

Add test to check the verification of ssl certificates

e9b64ec

Signed-off-by: Lukas Straub <lukasstraub2@web.de>

ikreymer merged commit f628b40 into webrecorder:master Jan 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for verifying ssl certificates #596

Add support for verifying ssl certificates #596

lukasstraub2 commented Dec 3, 2020 •

edited

Loading

lukasstraub2 commented Dec 3, 2020

lukasstraub2 commented Dec 10, 2020

ikreymer commented Dec 15, 2020

ikreymer Dec 15, 2020

ikreymer Dec 15, 2020

ikreymer Dec 15, 2020

ikreymer Dec 15, 2020

ikreymer Dec 15, 2020

lukasstraub2 commented Dec 16, 2020

codecov bot commented Dec 16, 2020 •

edited

Loading

ikreymer commented Dec 16, 2020 •

edited

Loading

lukasstraub2 commented Dec 23, 2020

	def __init__(self, cert_reqs='CERT_NONE', ca_cert_dir='/etc/ssl/certs', **init_kwargs):
	def __init__(self, cert_reqs='CERT_NONE', ca_cert_dir=None, **init_kwargs):

	ca_cert_dir=certs_config.get('ca_cert_dir', '/etc/ssl/certs'))
	ca_cert_dir=certs_config.get('ca_cert_dir'))

	``ca_cert_dir`` should point to a directory containing the CA certificates that you trust. Most linux distributions provide CA certificates via a package called ``ca-certificates``.
	``ca_cert_dir`` can optionally point to a directory containing the CA certificates that you trust. Most linux distributions provide CA certificates via a package called ``ca-certificates``.
	If omitted, the default system CA used by Python is used.

Add support for verifying ssl certificates #596

Add support for verifying ssl certificates #596

Conversation

lukasstraub2 commented Dec 3, 2020 • edited Loading

Description

Types of changes

Checklist:

lukasstraub2 commented Dec 3, 2020

lukasstraub2 commented Dec 10, 2020

ikreymer commented Dec 15, 2020

ikreymer Dec 15, 2020

Choose a reason for hiding this comment

ikreymer Dec 15, 2020

Choose a reason for hiding this comment

ikreymer Dec 15, 2020

Choose a reason for hiding this comment

ikreymer Dec 15, 2020

Choose a reason for hiding this comment

ikreymer Dec 15, 2020

Choose a reason for hiding this comment

lukasstraub2 commented Dec 16, 2020

codecov bot commented Dec 16, 2020 • edited Loading

Codecov Report

ikreymer commented Dec 16, 2020 • edited Loading

lukasstraub2 commented Dec 23, 2020

lukasstraub2 commented Dec 3, 2020 •

edited

Loading

codecov bot commented Dec 16, 2020 •

edited

Loading

ikreymer commented Dec 16, 2020 •

edited

Loading