On 1 December 2013 20:37, Antoine Pitrou <solipsis@pitrou.net> wrote:
On Sun, 01 Dec 2013 02:53:32 +0100 Christian Heimes <christian@python.org> wrote:
Am 30.11.2013 23:51, schrieb Antoine Pitrou:
Small nit: what happens if the server_hostname is None (i.e. wasn't passed to context.wrap_socket())?
The code will raise an exception. My patch already implements a more verbose ValueError that explains the cause of the problem. It's flaw in code, that calls context.wrap_socket. Erroneous code will no longer pass silently.
The patch also ensures a valid combination of verify_mode and check_hostname:
context = ssl.SSLContext(ssl.PROTOCOL_TLSv1) context.check_hostname = True Traceback (most recent call last): File "<stdin>", line 1, in <module> ValueError: check_hostname needs a SSL context with either CERT_OPTIONAL or CERT_REQUIRED context.verify_mode = ssl.CERT_REQUIRED context.check_hostname = True context.verify_mode = ssl.CERT_NONE Traceback (most recent call last): File "<stdin>", line 1, in <module> ValueError: Cannot set verify_mode to CERT_NONE when check_hostname is enabled.
So I have to set attributes in a given order? I find this silly.
Perhaps a cleaner option would be to make check_hostname read only, and add a secure-by-default method that allows all verification related settings to be adjusted at once: def set_verify_mode(mode=ssl.CERT_REQUIRED, check_hostname=True): ... That way the consistency check would be directly on the method arguments, rather than involving the current context state. Any future settings that required a verified cert could then use a similar model. If we don't do that, then I think Christian's approach is a reasonable compromise given the late stage of the release cycle - it ensures the context can't get into the inconsistent verify_mode=CERT_NONE and check_hostname=True state, and leaves our options completely open for 3.5: - add a "configure all security related settings at once" set_verify_mode method as suggested above - allow the context to get into an inconsistent state, check it in wrap_socket (bad due to point of exception != point of error) - have setting check_hostname potentially affect verify_mode and vice-versa (bad due to being implicit magic) We wouldn't be able to make check_hostname read-only the way we would if we added the configuration method now, but that doesn't bother me that much as long as the setting consistency invariant is enforced. Cheers, Nick. -- Nick Coghlan | ncoghlan@gmail.com | Brisbane, Australia