bpo-40990: Add HTTPS support to http.server.HTTPServer #20923
Conversation
There was a problem hiding this comment.
- ssl module is optional and may not be available. The http.server and its tests should still work when ssl is not present.
- You have to deal with ALPN correctly and refuse connections when ALPN is present and does not contain correct protocol indicator
- keys are usually protected and wrapped with PKCS#8 PBE2
tlsis technically correct but we don't use it in arguments yet- SSLSocket may raise exceptions that are not yet handled by HTTPServer
- HTTPSServer should be a separate class
| .. class:: HTTPServer(server_address, RequestHandlerClass, \ | ||
| bind_and_activate=True, *, tls=None) |
There was a problem hiding this comment.
This should be a separate class, not an argument.
| @@ -101,6 +101,7 @@ | |||
| import socket # For gethostbyaddr() | |||
| import socketserver | |||
| import sys | |||
| import ssl | |||
There was a problem hiding this comment.
ssl support is optional, you have to wrap the import in a try/except block.
| """Wrap the socket in SSLSocket if TLS is enabled""" | ||
| super().server_activate() | ||
| if self.tls_cert and self.tls_key: | ||
| context = ssl.SSLContext(ssl.PROTOCOL_TLS_SERVER) |
There was a problem hiding this comment.
The default SSL context requires a TLS 1.3 certificate so it doesn't work with ssl_cert.pem:
ssl.SSLError: [SSL: TLSV13_ALERT_CERTIFICATE_REQUIRED] tlsv13 alert certificate required
Is this something that we want to force for a development server?
There was a problem hiding this comment.
There is no such thing as a TLS 1.3 certificate. This is a TLS alert with message certificate required. Looks like the certificate does not have correct extensions to handle TLS 1.3.
You cannot use any file from test directory any way. The directory is often not installed or shipped to reduce disk space usage in containers or on small systems.
| def __init__(self, server_address, RequestHandlerClass, | ||
| bind_and_activate=True, *, tls=None): | ||
| if tls is None: | ||
| self.tls_cert = self.tls_key = None |
There was a problem hiding this comment.
It's not a cert file, it's a cert chain file.
| parser.add_argument('--tls-cert', | ||
| help='Specify the path to a TLS certificate') |
| elif not args.tls_cert or not args.tls_key: | ||
| parser.error('Both --tls-cert and --tls-key must be provided to enable TLS') | ||
| else: | ||
| tls = (args.tls_cert, args.tls_key) |
There was a problem hiding this comment.
It's common to have the private key and the cert chain in the same file. The key argument should be optional.
| @@ -21,6 +21,7 @@ | |||
| import html | |||
| import http.client | |||
| import urllib.parse | |||
| import ssl | |||
| # BaseHTTPServerTestCase. | ||
|
|
||
| # We have to use the correct path from the folder created by regtest | ||
| tls = ('../../Lib/test/ssl_cert.pem', '../../Lib/test/ssl_key.pem') |
There was a problem hiding this comment.
os.path.join(os.path.dirname(__file__), 'ssl_cert.pem')
| @@ -291,6 +299,31 @@ def test_head_via_send_error(self): | |||
| self.assertEqual(b'', data) | |||
|
|
|||
|
|
|||
| class BaseHTTPSServerTestCase(BaseTestCase): | |||
There was a problem hiding this comment.
Skip the test if ssl module is not supported
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase And if you don't make the requested changes, you will be put in the comfy chair! |
|
@remilapeyre I'm happy to continue this PR if you are not able to continue it |
Hi @jaswdr, I've been quite busy but should be able to finish it this week :) |
|
|
||
| .. warning:: | ||
|
|
||
| The HTTPS support is for development and test puposes and must not be used |
|
closing this PR as abandoned as there's a new one up. |
https://bugs.python.org/issue40990