Enable https checking using a test server by cjmayo · Pull Request #338 · linkchecker/linkchecker

cjmayo · 2019-11-05T19:54:29Z

Previously disabled because it was using Amazon. Verification is disabled because we generate our own certificate.

Also use the generated certificate to test httputil.x509_to_dict() and the fix required for Python 3.

anarcat

lgtm

mgedmin

This is awesome!

mgedmin · 2019-11-06T07:03:50Z

tests/checker/test_https.py

+        # Strip pass phrase from the key, else it is requested at server start
+        subprocess.check_output(["openssl", "pkey",
+            "-passin", "pass:linkchecker",
+            "-in", temp_key_file, "-out", get_file("https_key.pem")])


I think this belongs in setUp, not __init__

Surely there's a way of generating a self-signed cert without a password

Generating a new SSL key/cert for every test seems expensive, can we not do that when the key already exists? (And maybe set expiration date to something long -- a decade, a thousand years -- to prevent problems from cert reuse.)

I wonder if pyopenssl (or cryptogtaphy) have APIs for generating SSL keys/self-signed certs, to avoid shelling out.

Re 2: I believe the -nodes argument to openssl req suppresses the password prompt (it means "don't encrypt the key" because openssl's command-line UX is terrible).

See also https://unix.stackexchange.com/a/104305/89841

Re 4: https://skippylovesmalorie.wordpress.com/2010/02/12/how-to-generate-a-self-signed-certificate-using-pyopenssl/

Generating a new SSL key/cert for every test seems expensive, can we not do that when the key already exists? (And maybe set expiration date to something long -- a decade, a thousand years -- to prevent problems from cert reuse.)

There should be a way to generate SSL keys with garbage entropy or /dev/urandom so that it's fast. There's such a flag in GnuPG for sure, so maybe there's something similar in openssl?

Seems like a reasonable request anyways.

1. I think this belongs in `setUp`, not `__init__`

Is setUp called for each method, the docs aren't really clear? I did think about setUpClass, saw New in 3.2 and didn't use it - however it is in 2.7 and seems to work.

Re 2: I believe the -nodes argument to openssl req suppresses the password prompt (it means "don't encrypt the key" because openssl's command-line UX is terrible).

That works.

Re 4: https://skippylovesmalorie.wordpress.com/2010/02/12/how-to-generate-a-self-signed-certificate-using-pyopenssl/

I will have a go. That link suggests you can set the notAfter time which could make for a better test than just True.

Yes, setUp is called before each test method.

Note that __init__ is called once for every test method too, while constructing the test suite!

The unittest API requires that each instance of a test case has a runTest() method that runs the test. When you define a test class, the default test loader invokes unittest.MakeSuite(YourTestClass) which creates an empty TestSuite(), enumerates all the test methods of the class, and then instantiates the class by passing the test method name to __init__ to get a test case instance that will run that particular method from its runTest().

So if you wanted to avoid creating new SSL keys for every test method, __init__ is the wrong way to go about it.

(Caveat: I don't know for sure that pytest's unittest compatibility mode also works the same way, but I think it's very likely.)

mgedmin · 2019-11-06T07:04:28Z

tests/checker/httpserver.py

-            conn = HTTPConnection("localhost:%d" % port)
+            if https:
+                conn = HTTPSConnection("localhost:%d" % port,
+                        context=ssl._create_unverified_context())


Is that a documented API method? I worry because it starts with an underscore.

_create_unverified_context() is not mentioned in https://docs.python.org/3/library/ssl.html. Let's not use it.

We can inline the implementation from the stdlib and eliminate all the constants from default arguments, which will give us

context = ssl.SSLContext(ssl.PROTOCOL_TLS) context.check_hostname = False context.verify_mode = ssl.CERT_NONE

and the docs say that check_hostname is False by default except in one specific circumstance that doesn't apply to us, so this assignment can be omitted. The default value of verify_mode is not documented. I'm pretty sure it is CERT_NONE in all current versions of Python, so we might simplify this to

conn = HTTPSConnection(..., context=ssl.SSLContext(ssl.PROTOCOL_TLS))

but for readability I'd prefer a top-level constant in the test file

UNVERIFIED_SSL_CONTEXT_FOR_TESTS = ssl.SSLContext(ssl.PROTOCOL_TLS) ... conn = HTTPSConnection(..., context=UNVERIFIED_SSL_CONTEXT_FOR_TESTS)

_create_unverified_context() is not mentioned in https://docs.python.org/3/library/ssl.html. Let's not use it.

It came from the http.client.HTTPSConnection documentation:
https://docs.python.org/3/library/http.client.html?highlight=_create_unverified_context#http.client.HTTPSConnection

but I don't mind replacing it.

That makes me feel much better, but also makes me want to SCREAM at Python developers who did this.

mgedmin · 2019-11-06T09:55:50Z

tests/checker/httpserver.py

-    conn = HTTPConnection("localhost:%d" % port)
+    if https:
+        conn = HTTPSConnection("localhost:%d" % port,
+                               context=ssl._create_unverified_context())


(the above applies here also)

dev-requirements.txt

mgedmin identified issues so i retract my approval until those are fixed

cjmayo · 2019-11-06T20:06:56Z

tests/checker/test_https.py

+        super(TestHttps, self).__init__(methodName=methodName)
+        self.handler = CookieRedirectHttpRequestHandler
+        temp_key_file = get_file("key.pem")
+        subprocess.check_output(["openssl", "req", "-x509", "-newkey",


That should have been subprocess.check_call rather than subprocess.check_output.

Verification has to be turned off because we are using a self-signed certificate.

File "/usr/lib/python3.7/site-packages/linkcheck/httputil.py", line 68, in asn1_generaltime_to_seconds line: res = datetime.strptime(timestr, timeformat + 'Z') locals: res = <local> None datetime = <global> <class 'datetime.datetime'> datetime.strptime = <global> <built-in method strptime of type object at 0x7fa39064dda0> timestr = <local> b'20191106202117Z' timeformat = <local> '%Y%m%d%H%M%S' TypeError: strptime() argument 1 must be str, not bytes pyOpenSSL OpenSSL.crypto.X509.get_notAfter() returns bytes: https://www.pyopenssl.org/en/stable/api/crypto.html#OpenSSL.crypto.X509.get_notAfter

cjmayo · 2019-11-11T20:18:18Z

OK, now with key and certificate generation by pyOpenSSL which enables a better check for x509_to_dict:

self.assertEqual(httputil.x509_to_dict(cert)["notAfter"],
                 "Jan 02 03:04:05 2119 GMT")

mgedmin · 2019-11-12T07:42:32Z

(a reminder that force-pushes force me to review the entire thing again, since I cannot see just the changes since the last time)

mgedmin

LGTM, ~~with one minor change requested~~!

tests/checker/test_https.py

mgedmin · 2019-11-12T07:45:12Z

tests/checker/test_https.py

+        cert.get_subject().CN = "localhost"
+        cert.set_serial_number(1000)
+        cert.gmtime_adj_notBefore(0)
+        cert.set_notAfter(b"21190102030405Z")


One hundred years, haha!

cjmayo mentioned this pull request Nov 5, 2019

Change HtmlParser to use Beautiful Soup #337

Merged

cjmayo force-pushed the https branch from 2f094f8 to d190806 Compare November 5, 2019 20:04

anarcat previously approved these changes Nov 5, 2019

View reviewed changes

mgedmin reviewed Nov 6, 2019

View reviewed changes

dev-requirements.txt Show resolved Hide resolved

cjmayo commented Nov 6, 2019

View reviewed changes

cjmayo added 2 commits November 11, 2019 20:12

Enable https checking using a test server

dee4be4

Verification has to be turned off because we are using a self-signed certificate.

cjmayo force-pushed the https branch from d190806 to ecd0677 Compare November 11, 2019 20:13

mgedmin approved these changes Nov 12, 2019

View reviewed changes

anarcat merged commit c92ab72 into linkchecker:master Nov 14, 2019

cjmayo deleted the https branch November 21, 2019 19:48

Conversation

cjmayo commented Nov 5, 2019

Uh oh!

anarcat left a comment

Choose a reason for hiding this comment

Uh oh!

mgedmin left a comment

Choose a reason for hiding this comment

Uh oh!

mgedmin Nov 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mgedmin Nov 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mgedmin Nov 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjmayo commented Nov 11, 2019

Uh oh!

mgedmin commented Nov 12, 2019

Uh oh!

mgedmin left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mgedmin Nov 6, 2019 •

edited

Loading

mgedmin Nov 6, 2019 •

edited

Loading

mgedmin Nov 6, 2019 •

edited

Loading

mgedmin left a comment •

edited

Loading