[bpo-28414] Make all hostnames in SSL module IDN A-labels #5128

tiran · 2018-01-07T15:41:43Z

Alternative approach to PR #3010

Really all server_hostname attributes are IDN A-labels
SSLContext.set_servername_callback argument is IDN A-label, too
Replace all externally replaced certs with proper certs from internal test helper

https://bugs.python.org/issue28414

alex · 2018-01-07T15:56:56Z

Doc/whatsnew/3.7.rst

+expected hostname in A-label form (``"xn--pythn-mua.org"``), rather
+than the U-label form (``"pythön.org"``). The server name argument to
+:meth:`ssl.SSLContext.set_servername_callback`` is an A-label instead of
+an U-labe, too. (Contributed by Nathaniel J. Smith and Christian Heimes


(also not sure what the deal with github rendering all this with underlines is; potential reST issue?)

It's caused by a supernumerous backtick. I have fixed both issues.

alex · 2018-01-07T15:58:42Z

Lib/test/test_asyncio/test_events.py

@@ -1181,6 +1181,8 @@ def test_create_unix_server_ssl_verified(self):
        server.close()
        self.loop.run_until_complete(proto.done)

+    maxDiff = None


Remnants of debugging failing tests

njsmith · 2018-01-10T22:21:01Z

I did a quick skim and the code seems fine to me, but... I do think that for set_servername_callback, instead of just breaking things we maybe we need to rename it and add a compatibility shim. Quoting from https://mail.python.org/pipermail/python-dev/2017-December/151527.html:

On the server, the obvious fix would be to start passing
A-label-encoded names to the servername_callback, instead of
U-label-encoded names. Unfortunately, this is a bit trickier, because
this has historically worked (AFAIK) for IDNA names, so long as they
didn't use one of the four magic characters who changed meaning
between IDNA 2003 and IDNA 2008. But we do still need to do something.
For example, right now, it's impossible to use the ssl module to
implement a web server at https://straße.de, because incoming
connections will use SNI to say that they expect a cert for
"xn--strae-oqa.de", and then the ssl module will freak out and throw
an exception instead of invoking the servername callback.

It's ugly, but probably the simplest thing is to add a new function
like set_servername_callback2 that uses the A-label, and then redefine
set_servername_callback as a deprecated compatibility shim:
def set_servername_callback(self, cb):
    def shim_cb(sslobj, servername, sslctx):
        if servername is not None:
            servername = servername.encode("ascii").decode("idna")
        return cb(sslobj, servername, sslctx)
    self.set_servername_callback2(shim_cb)
We can bikeshed what the new name should be. Maybe set_sni_callback?
or set_server_hostname_callback, since the corresponding client-mode
argument is server_hostname?

tiran · 2018-01-13T19:45:33Z

Meh! :(

Let's make it PEP 543 compatible and add sni_callback property. The Python subclass can provide your shim cb. Thanks for proposing it!

Historically, our handling of international domain names (IDNs) in the ssl module has been very broken. The flow went like: 1. User passes server_hostname= to the SSLSocket/SSLObject constructor. This gets normalized to an A-label by using the PyArg_Parse "et" mode: bytes objects get passed through unchanged (assumed to already be A-labels); str objects get run through .encode("idna") to convert them into A-labels. 2. newPySSLSocket takes this A-label, and for some reason decodes it *back* to a U-label, and stores that as the object's server_hostname attribute. 3. Later, this U-label server_hostname attribute gets passed to match_hostname, to compare against the hostname seen in the certificate. But certificates contain A-labels, and match_hostname expects to be passed an A-label, so this doesn't work at all. This PR fixes the problem by removing the pointless decoding at step 2, so that internally we always use A-labels, which matches how internet protocols are designed in general: A-labels are used everywhere internally and on-the-wire, and U-labels are basically just for user interfaces. This also matches the general advice to handle encoding/decoding once at the edges, though for backwards-compatibility we continue to use 'str' objects to store A-labels, even though they're now always ASCII. Technically there is a minor compatibility break here: if a user examines the .server_hostname attribute of an ssl-wrapped socket, then previously they would have seen a U-label like "pythön.org", and now they'll see an A-label like "xn--pythn-mua.org". But this only affects non-ASCII domain names, which have never worked in the first place, so it seems unlikely that anyone is relying on the old behavior. This PR also adds an end-to-end test for IDN hostname validation. Previously there were no tests for this functionality. Fixes bpo-28414.

All test certs must be generated by CPython's own test helper. Signed-off-by: Christian Heimes <christian@python.org>

Signed-off-by: Christian Heimes <christian@python.org>

njsmith

I pushed some prose changes, and here are some comments on the code. Overall looks good, but there are a few small cleanups to consider, and a few points where I have questions.

njsmith · 2018-02-22T22:50:55Z

Lib/ssl.py

+            self.sni_callback = None
+        else:
+            if not hasattr(server_name_callback, '__call__'):
+                raise TypeError("not a callable object")


Why not if not callable(server_name_callback)?

Ah right, callable was re-added in Python 3.2. I'll change it.

njsmith · 2018-02-22T22:51:37Z

Lib/ssl.py

+
+            def shim_cb(sslobj, servername, sslctx):
+                if servername is not None:
+                    servername = servername.encode("ascii").decode("idna")


It would be simpler to use _encode_hostname here, no?

I don't mind to use the method here, too.

njsmith · 2018-02-22T22:52:29Z

Lib/ssl.py

@@ -355,13 +355,20 @@ def __new__(cls, protocol=PROTOCOL_TLS, *args, **kwargs):
        self = _SSLContext.__new__(cls, protocol)
        return self

-    def __init__(self, protocol=PROTOCOL_TLS):
-        self.protocol = protocol
+    def _encode_hostname(self, hostname):


Maybe make this a top-level utility function, instead of a method?

I'd prefer to keep this a method. It makes it easier to modify the behavior in subclasses or in tests.

njsmith · 2018-02-23T01:17:23Z

Modules/_ssl.c

    {"options", (getter) get_options,
                (setter) set_options, NULL},
+    {"protocol", (getter) get_protocol,
+                 NULL, NULL},


Why is this new protocol attribute in this PR? Should it be documented or anything?

It's not new: https://docs.python.org/3/library/ssl.html#ssl.SSLContext.protocol

I just moved it from Python code into C code so I can access the value from C methods. The new setter for the callback uses the value to prevent setting a SNI callback for a client-only context.

Ah, gotcha.

njsmith · 2018-02-23T01:18:29Z

Modules/_ssl.c

+                Py_INCREF(result);
+                Py_SETREF(ssl->ctx, (PySSLContext *)result);
+                SSL_set_SSL_CTX(ssl->ssl, ssl->ctx->ctx);
+            }


The docs just say that sni_callback has to return None. What's this extra context setting stuff about? Should it be removed?

I added the code for future compatibility with PEP 543. Rather than modifying socket.context, the callback can return a new context and let the API take care of the rest.

By the way it doesn't have to be None. Other values signal an error:

The server_name_callback function must return None to allow the TLS negotiation to continue. If a TLS failure is required, a constant ALERT_DESCRIPTION_* can be returned. Other return values will result in a TLS fatal error with ALERT_DESCRIPTION_INTERNAL_ERROR.

Ok yeah, I think adding new undocumented features in b2 for partial compatibility with an unfinished PEP is probably a bad plan, so thanks for removing that :-). It'll be easy to add back later.

Drop extra code for PEP 543 future compatibility in sni callback Use callable() and encode_hostname in shim for old SNI callback. Signed-off-by: Christian Heimes <christian@python.org>

tiran · 2018-02-24T01:33:20Z

The context special handling is pining for the fjords.

miss-islington · 2018-02-24T01:35:10Z

Thanks @tiran for the PR, and @njsmith for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

) Previously, the ssl module stored international domain names (IDNs) as U-labels. This is problematic for a number of reasons -- for example, it made it impossible for users to use a different version of IDNA than the one built into Python. After this change, we always convert to A-labels as soon as possible, and use them for all internal processing. In particular, server_hostname attribute is now an A-label, and on the server side there's a new sni_callback that receives the SNI servername as an A-label rather than a U-label. (cherry picked from commit 11a1493) Co-authored-by: Christian Heimes <christian@python.org>

bedevere-bot · 2018-02-24T01:35:27Z

GH-5843 is a backport of this pull request to the 3.7 branch.

…H-5843) Previously, the ssl module stored international domain names (IDNs) as U-labels. This is problematic for a number of reasons -- for example, it made it impossible for users to use a different version of IDNA than the one built into Python. After this change, we always convert to A-labels as soon as possible, and use them for all internal processing. In particular, server_hostname attribute is now an A-label, and on the server side there's a new sni_callback that receives the SNI servername as an A-label rather than a U-label. (cherry picked from commit 11a1493) Co-authored-by: Christian Heimes <christian@python.org>

mattip · 2020-12-15T09:14:39Z

Why are there no tests for the new function name, the tests in the stdlib all refer to set_servername_callback ?

tiran requested review from 1st1 and asvetlov as code owners January 7, 2018 15:41

the-knights-who-say-ni added the CLA signed label Jan 7, 2018

bedevere-bot added the awaiting merge label Jan 7, 2018

tiran force-pushed the die-idna-die-hard branch from 134072e to eacb9e9 Compare January 7, 2018 15:42

tiran mentioned this pull request Jan 7, 2018

[bpo-28414] In SSL module, store server_hostname as an A-label #3010

Closed

alex reviewed Jan 7, 2018

View reviewed changes

tiran force-pushed the die-idna-die-hard branch from eacb9e9 to 9bd40df Compare January 7, 2018 16:06

asvetlov approved these changes Jan 9, 2018

View reviewed changes

tiran force-pushed the die-idna-die-hard branch from 9bd40df to 31b862e Compare January 13, 2018 19:19

tiran force-pushed the die-idna-die-hard branch from 31b862e to 0b512f1 Compare January 14, 2018 00:18

tiran mentioned this pull request Jan 19, 2018

bpo-32598: Use autoconf to detect usable OpenSSL #5242

Merged

tiran force-pushed the die-idna-die-hard branch from 0b512f1 to 428265e Compare January 20, 2018 14:23

tiran added the DO-NOT-MERGE label Jan 26, 2018

tiran force-pushed the die-idna-die-hard branch from 428265e to 5ec02e4 Compare January 28, 2018 19:50

njsmith and others added 3 commits February 22, 2018 18:48

Drop trustme certs

f985559

All test certs must be generated by CPython's own test helper. Signed-off-by: Christian Heimes <christian@python.org>

Fix IDN SAN handling

1ec0678

Signed-off-by: Christian Heimes <christian@python.org>

tiran force-pushed the die-idna-die-hard branch from 5ec02e4 to 1ec0678 Compare February 22, 2018 18:19

tiran added needs backport to 3.7 and removed DO-NOT-MERGE labels Feb 22, 2018

tiran changed the title ~~[bpo-28414][WIP] Make all hostnames in SSL module IDN A-labels~~ [bpo-28414] Make all hostnames in SSL module IDN A-labels Feb 22, 2018

tiran requested a review from njsmith February 22, 2018 18:29

njsmith added 2 commits February 22, 2018 17:14

Small doc updates

a0e42d4

Update blurb to match what this PR does now

e5073b5

njsmith reviewed Feb 23, 2018

View reviewed changes

Address review comments

8e3c14f

Drop extra code for PEP 543 future compatibility in sni callback Use callable() and encode_hostname in shim for old SNI callback. Signed-off-by: Christian Heimes <christian@python.org>

njsmith approved these changes Feb 24, 2018

View reviewed changes

njsmith merged commit 11a1493 into python:master Feb 24, 2018

bedevere-bot removed the awaiting merge label Feb 24, 2018

bedevere-bot removed the needs backport to 3.7 label Feb 24, 2018

stratakis mentioned this pull request Aug 28, 2020

[3.6] bpo-41183: Skip ssl tests for disabled versions (GH-16427) #21882

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[bpo-28414] Make all hostnames in SSL module IDN A-labels #5128

[bpo-28414] Make all hostnames in SSL module IDN A-labels #5128

tiran commented Jan 7, 2018 •

edited by bedevere-bot

Loading

alex Jan 7, 2018

alex Jan 7, 2018

tiran Jan 7, 2018

alex Jan 7, 2018

tiran Jan 7, 2018

njsmith commented Jan 10, 2018

tiran commented Jan 13, 2018

njsmith left a comment

njsmith Feb 22, 2018

tiran Feb 23, 2018

njsmith Feb 22, 2018

tiran Feb 23, 2018

njsmith Feb 22, 2018

tiran Feb 23, 2018

njsmith Feb 23, 2018

tiran Feb 23, 2018

njsmith Feb 24, 2018

njsmith Feb 23, 2018

tiran Feb 23, 2018

njsmith Feb 24, 2018

tiran commented Feb 24, 2018

miss-islington commented Feb 24, 2018

bedevere-bot commented Feb 24, 2018

mattip commented Dec 15, 2020

[bpo-28414] Make all hostnames in SSL module IDN A-labels #5128

[bpo-28414] Make all hostnames in SSL module IDN A-labels #5128

Conversation

tiran commented Jan 7, 2018 • edited by bedevere-bot Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

njsmith commented Jan 10, 2018

tiran commented Jan 13, 2018

njsmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiran commented Feb 24, 2018

miss-islington commented Feb 24, 2018

bedevere-bot commented Feb 24, 2018

mattip commented Dec 15, 2020

tiran commented Jan 7, 2018 •

edited by bedevere-bot

Loading