◐ Shell
reader mode source ↗
Skip to content

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

Merged
njsmith merged 6 commits into
python:masterfrom
tiran:die-idna-die-hard
Feb 24, 2018
Merged

[bpo-28414] Make all hostnames in SSL module IDN A-labels#5128
njsmith merged 6 commits into
python:masterfrom
tiran:die-idna-die-hard

Conversation

@tiran

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

Copy link
Copy Markdown
Member

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

@tiran tiran requested review from 1st1 and asvetlov as code owners January 7, 2018 15:41
@tiran tiran force-pushed the die-idna-die-hard branch from 134072e to eacb9e9 Compare January 7, 2018 15:42
@tiran tiran mentioned this pull request Jan 7, 2018
Closed
@tiran tiran force-pushed the die-idna-die-hard branch from eacb9e9 to 9bd40df Compare January 7, 2018 16:06
@njsmith

njsmith commented Jan 10, 2018

Copy link
Copy Markdown
Contributor

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 tiran force-pushed the die-idna-die-hard branch from 9bd40df to 31b862e Compare January 13, 2018 19:19
@tiran

tiran commented Jan 13, 2018

Copy link
Copy Markdown
Member Author

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!

@tiran tiran force-pushed the die-idna-die-hard branch from 31b862e to 0b512f1 Compare January 14, 2018 00:18
@tiran tiran mentioned this pull request Jan 19, 2018
Merged
@tiran tiran force-pushed the die-idna-die-hard branch from 0b512f1 to 428265e Compare January 20, 2018 14:23
@tiran 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
@njsmith @tiran
[bpo-28414] In SSL module, store server_hostname as an A-label …
b116364 
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.
@tiran
Drop trustme certs …
f985559 
All test certs must be generated by CPython's own test helper.

Signed-off-by: Christian Heimes <christian@python.org>
@tiran
Fix IDN SAN handling …
1ec0678 
Signed-off-by: Christian Heimes <christian@python.org>
@tiran tiran force-pushed the die-idna-die-hard branch from 5ec02e4 to 1ec0678 Compare February 22, 2018 18:19
@tiran tiran changed the title [bpo-28414][WIP] Make all hostnames in SSL module IDN A-labels Feb 22, 2018
@tiran tiran requested a review from njsmith February 22, 2018 18:29
njsmith
njsmith reviewed Feb 23, 2018
View reviewed changes

@njsmith njsmith left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide comment

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.

@tiran
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>
23 hidden items Load more…
@tiran

tiran commented Feb 24, 2018

Copy link
Copy Markdown
Member Author

The context special handling is pining for the fjords.

@njsmith njsmith merged commit 11a1493 into python:master Feb 24, 2018
@miss-islington

miss-islington commented Feb 24, 2018

Copy link
Copy Markdown
Contributor

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

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Feb 24, 2018
@tiran @miss-islington
[bpo-28414] Make all hostnames in SSL module IDN A-labels (pythonGH-5128 …
db1dcdf 
)

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

bedevere-bot commented Feb 24, 2018

Copy link
Copy Markdown

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

njsmith pushed a commit that referenced this pull request Feb 24, 2018
@miss-islington @tiran @njsmith
[bpo-28414] Make all hostnames in SSL module IDN A-labels (GH-5128) (G… …
1c37e27 
…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>
@stratakis stratakis mentioned this pull request Aug 28, 2020
Closed
@mattip

mattip commented Dec 15, 2020

Copy link
Copy Markdown
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alex alex alex left review comments

@asvetlov asvetlov asvetlov approved these changes

@njsmith njsmith njsmith approved these changes

@1st1 1st1 Awaiting requested review from 1st1

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@tiran @njsmith @miss-islington @bedevere-bot @mattip @alex @asvetlov @the-knights-who-say-ni