classification
Title: Server-side support for TLS Server Name Indication extension
Type: enhancement Stage: resolved
Components: Library (Lib) Versions: Python 3.4
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: christian.heimes, daniel-black, grooverdan, jcea, kyoshida, mpb, piotr.dobrogost, pitrou, python-dev
Priority: normal Keywords: patch

Created on 2010-03-10 15:14 by jcea, last changed 2013-06-24 19:12 by mpb. This issue is now closed.

Files
File name Uploaded Description Edit
issue8109_server_side_sni.patch daniel-black, 2012-09-14 13:16 review
issue-8109-sni-serverside.patch daniel-black, 2012-12-10 03:30 review
issue-8109-sni-serverside.patch daniel-black, 2012-12-16 03:42 review
sni.patch pitrou, 2013-01-04 21:21 review
sni2.patch pitrou, 2013-01-04 23:01 review
issue-8109.patch kyoshida, 2013-04-11 03:14 review
issue-8109.patch kyoshida, 2013-04-11 10:06 review
Messages (27)
msg100787 - (view) Author: Jesús Cea Avión (jcea) * (Python committer) Date: 2010-03-10 15:14
SSL sockets should support SNI, both as servers and clients: http://en.wikipedia.org/wiki/Server_Name_Indication

After that, libraries that support SSL/TLS should be upgraded to take advantage of it.

Any interest in supporting this?.
msg103751 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-04-20 20:47
Duplicate of issue5639.
msg125613 - (view) Author: Daniel Black (grooverdan) * Date: 2011-01-07 01:37
issue #5639 only has functionality for client side SNI. Server side SNI is still missing.

For server side SNI to be supported a server program should be able to retrieve the server name provided by the client call and alter the server certificate/key before the server completes the TLS/SSL connection.
msg125645 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-01-07 13:43
> Server side SNI is still missing.

Right, re-opening.
msg168793 - (view) Author: danblack (daniel-black) Date: 2012-08-21 16:52
test_sni not working. getpeercert() not returning a certificate.
msg170233 - (view) Author: Jesús Cea Avión (jcea) * (Python committer) Date: 2012-09-10 20:33
Daniel, your patch looks quite interesting. Please, send a contributor agreement to the PSF: http://www.python.org/psf/contrib/contrib-form-python/ . Let me know when you status have changed.

Why are you changing "Lib/test/keycert2.pem"?

Please, provide also a documentation patch.

This is a feature enhancement. Would be applied to 3.4, it is too late for 3.3 :-(. Too bad! :(
msg170474 - (view) Author: danblack (daniel-black) Date: 2012-09-14 13:16
> Daniel, your patch looks quite interesting. Please, send a contributor agreement to the PSF: http://www.python.org/psf/contrib/contrib-form-python/ . Let me know when you status have changed.

Already done. Has been accepted and I've got an acknowledgement email.

> Why are you changing "Lib/test/keycert2.pem"?
I was mistakely assuming that this was the only test that used it. Fixed now. Also added a CA key and server for validating key chains. I didn't end up using it however thought it would be handy.

> Please, provide also a documentation patch.

Done. Also improved error checking and reference counting.

> This is a feature enhancement. Would be applied to 3.4, it is too late for 3.3 :-(. Too bad! :(

Was expected. Its been 2.5 years since the bug opened. A little more won't hurt.

I've also changed SSLSocket.context to be a property. Its not quite working. The current test case as is working however using an assignment as per line 1958 of Lib/test/test_ssl.py.
msg172167 - (view) Author: danblack (daniel-black) Date: 2012-10-06 04:30
happy with this?

I'm not sure what i've done to make s._set_context(newctx) work but s.context = newctx fail. I though the code here http://bugs.python.org/review/8109/diff2/5815:5989/Lib/ssl.py effectively maps them.
msg172195 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-10-06 13:10
Daniel, I'll take a look.
msg176776 - (view) Author: danblack (daniel-black) Date: 2012-12-02 07:49
> Antoine Pitrou (pitrou) * 	Date: 2012-10-06 13:10
> Daniel, I'll take a look.

minor nag :-)
msg177258 - (view) Author: danblack (daniel-black) Date: 2012-12-10 03:30
I've added a full set of alert descriptions and cleaned up the doco some more.

The reference counting when the SNI callback comes in is my greatest worry.
msg177544 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-12-15 17:57
I've posted a few more comments.
As for cyclic garbage collection, it's explained a bit there:
http://docs.python.org/dev/extending/newtypes.html#supporting-cyclic-garbage-collection

If it isn't very clear to you, I can still handle it myself, though. Those docs aren't the best.
msg177581 - (view) Author: danblack (daniel-black) Date: 2012-12-16 08:34
> If it isn't very clear to you, I can still handle it myself, though. Those docs aren't the best.

Not clear enough. Yes I'd appreciate you handling it. Thanks.
msg179079 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-01-04 21:08
Here is an updated patch with cyclic GC support, and other small things.
msg179093 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-01-04 23:01
Updated patch after Daniel's comments.
msg179141 - (view) Author: Roundup Robot (python-dev) Date: 2013-01-05 20:22
New changeset 927afb7bca2a by Antoine Pitrou in branch 'default':
Issue #8109: The ssl module now has support for server-side SNI, thanks to a :meth:`SSLContext.set_servername_callback` method.
http://hg.python.org/cpython/rev/927afb7bca2a
msg179143 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-01-05 20:24
I've committed the latest patch. Thank you very much!
msg179145 - (view) Author: danblack (daniel-black) Date: 2013-01-05 21:27
> I've committed the latest patch. Thank you very much!

much appreciate your help.
msg179158 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2013-01-06 00:36
Coverity reports an issue in the callback function:

/Modules/_ssl.c: 2403 ( uninit_use)
   2400            /* remove race condition in this the call back while if removing the
   2401             * callback is in progress */
   2402            PyGILState_Release(gstate);
>>> CID 966640: Uninitialized scalar variable (UNINIT)
>>> Using uninitialized value "ret".
   2403            return ret;
   2404        }
   2405    
   2406        ssl = SSL_get_app_data(s);
   2407        assert(PySSLSocket_Check(ssl));

I don't know which error code should be returned in this case.
msg179173 - (view) Author: danblack (daniel-black) Date: 2013-01-06 06:18
> I don't know which error code should be returned in this case.

Thanks Christian. My fault - asked Antoine to remove the default value for it and didn't see this like.

make line 2403:

return SSL_TLSEXT_ERR_OK;
msg179182 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-01-06 14:30
Fixed in 52b4d9bfc9ea (Roundup e-mail gateway seems broken).
msg179194 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-01-06 15:51
(testing Roundup mail gateway, please ignore)
msg186539 - (view) Author: Kazuhiro Yoshida (kyoshida) Date: 2013-04-11 03:14
I am trying to use SSLContext.set_servername_callback in my program but when a callback is set, it seems that connecting to the server without providing a server name causes a segmentation fault. (e.g. 'openssl s_client -connect localhost:443 -servername foo' is OK but 'openssl s_client -connect localhost:443' crashes the server. A simple test that causes the same error is included in the patch.)

My expectation was to get None as the second argument of the callback in such cases so I modified Modules/_ssl.c (as in the patch) to make it behave as I expected.

The modification seems to work fine as far as I've tested, but I'd appreciate if an official fix is available.
msg186541 - (view) Author: Daniel Black (grooverdan) * Date: 2013-04-11 03:50
nice patch. Thanks for finding the bug. I like the solution with test case.

Just needs a small enhancement of documention to ensure other users expect this behaviour.
msg186555 - (view) Author: Kazuhiro Yoshida (kyoshida) Date: 2013-04-11 10:06
Thanks for a comment.
I've made a version that adds a line to the document.
msg186578 - (view) Author: Roundup Robot (python-dev) Date: 2013-04-11 18:49
New changeset 4ae6095b4638 by Antoine Pitrou in branch 'default':
Fix a crash when setting a servername callback on a SSL server socket and the client doesn't send a server name.
http://hg.python.org/cpython/rev/4ae6095b4638
msg186579 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-04-11 18:49
Thank you for finding this! The patch is now committed.
History
Date User Action Args
2013-06-24 19:12:59mpbsetnosy: + mpb
2013-04-11 18:49:53pitrousetmessages: + msg186579
2013-04-11 18:49:20python-devsetmessages: + msg186578
2013-04-11 10:06:45kyoshidasetfiles: + issue-8109.patch

messages: + msg186555
2013-04-11 03:50:27grooverdansetmessages: + msg186541
2013-04-11 03:14:55kyoshidasetfiles: + issue-8109.patch
nosy: + kyoshida
messages: + msg186539

2013-01-06 15:51:14pitrousetmessages: + msg179194
2013-01-06 14:30:49pitrousetstatus: open -> closed
resolution: fixed
messages: + msg179182

stage: needs patch -> resolved
2013-01-06 06:18:37daniel-blacksetmessages: + msg179173
2013-01-06 00:36:25christian.heimessetstatus: closed -> open

nosy: + christian.heimes
messages: + msg179158

resolution: fixed -> (no value)
stage: resolved -> needs patch
2013-01-05 21:27:27daniel-blacksetmessages: + msg179145
2013-01-05 20:24:42pitrousetstatus: open -> closed
resolution: fixed
messages: + msg179143

stage: patch review -> resolved
2013-01-05 20:22:49python-devsetnosy: + python-dev
messages: + msg179141
2013-01-04 23:01:51pitrousetfiles: + sni2.patch

messages: + msg179093
2013-01-04 21:21:32pitrousetfiles: + sni.patch
2013-01-04 21:21:24pitrousetfiles: - sni.patch
2013-01-04 21:08:51pitrousetfiles: + sni.patch

messages: + msg179079
components: + Library (Lib), - Extension Modules
2012-12-16 08:34:36daniel-blacksetmessages: + msg177581
2012-12-16 03:42:51daniel-blacksetfiles: + issue-8109-sni-serverside.patch
2012-12-15 17:57:39pitrousetmessages: + msg177544
2012-12-10 03:30:25daniel-blacksetfiles: + issue-8109-sni-serverside.patch

messages: + msg177258
2012-12-02 07:49:06daniel-blacksetmessages: + msg176776
2012-11-23 20:00:53piotr.dobrogostsetnosy: + piotr.dobrogost
2012-10-06 13:10:49pitrousetmessages: + msg172195
2012-10-06 04:30:16daniel-blacksetmessages: + msg172167
2012-09-14 13:16:58daniel-blacksetfiles: - issue8109_server_side_sni.patch
2012-09-14 13:16:25daniel-blacksetfiles: + issue8109_server_side_sni.patch

messages: + msg170474
2012-09-10 20:34:02jceasetversions: + Python 3.4, - Python 3.3
2012-09-10 20:33:49jceasetmessages: + msg170233
stage: needs patch -> patch review
2012-08-21 16:52:54daniel-blacksetfiles: + issue8109_server_side_sni.patch

nosy: + daniel-black
messages: + msg168793

keywords: + patch
2011-01-07 13:43:16pitrousetstatus: closed -> open

title: Support for TLS Server Name Indication extension -> Server-side support for TLS Server Name Indication extension
messages: + msg125645
versions: + Python 3.3, - Python 2.7, Python 3.2
superseder: Support TLS SNI extension in ssl module ->
resolution: duplicate -> (no value)
stage: needs patch
2011-01-07 01:37:36grooverdansetnosy: + grooverdan
messages: + msg125613
2010-04-20 20:47:50pitrousetstatus: open -> closed

nosy: + pitrou
messages: + msg103751

superseder: Support TLS SNI extension in ssl module
resolution: duplicate
2010-03-10 15:14:05jceacreate