Repository navigation
ssl: post-commit review of the new memory BIO API #66754
Description
| BPO | 22564 |
|---|---|
| Nosy | @pitrou, @vstinner, @alex |
| Files |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
assignee = None
closed_at = <Date 2014-10-10.11:12:42.987>
created_at = <Date 2014-10-06.10:14:25.224>
labels = []
title = 'ssl: post-commit review of the new memory BIO API'
updated_at = <Date 2014-10-10.11:12:42.985>
user = 'https://github.com/vstinner'bugs.python.org fields:
activity = <Date 2014-10-10.11:12:42.985>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2014-10-10.11:12:42.987>
closer = 'vstinner'
components = []
creation = <Date 2014-10-06.10:14:25.224>
creator = 'vstinner'
dependencies = []
files = ['36850']
hgrepos = []
issue_num = 22564
keywords = ['patch']
message_count = 11.0
messages = ['228653', '228664', '228687', '228693', '228741', '228753', '228763', '228869', '228981', '228983', '228984']
nosy_count = 5.0
nosy_names = ['geertj', 'pitrou', 'vstinner', 'alex', 'python-dev']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = None
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue22564'
versions = ['Python 3.5']
Activity
vstinner commentedon Oct 6, 2014
Hi,
I just saw that the changeset a79003f25a41 was merged and it adds support for memory BIO to the Python ssl module, great! It's a nice addition, especially because it becomes possible to implement SSL for IOCP event loop in asyncio (bpo-22560).
I read the changeset and I have a few comments. Sorry, I didn't have time before to review the patch. I prefer to open a new issue. I'm interested to work on patches, but I would prefer a first feedback before working on a patch.
+.. method:: SSLSocket.read(len=0, buffer=None)
Hum, I fear that some people will call read() with two parameters by mistake. I would prefer a keyword-only parameter: put a "$" in the call to PyArg_ParseTuple() in PySSL_SSLread().
The "read()" method is quite generic, many Python functions expect a "file-like" object with a read() method which only take one indexed parameter.
An alternative would be to implemented readinto() and drop the buffer parameter from read().
Same remark for SSLObject.read() and _ssl._SSLSocket.read().
+.. attribute:: SSLSocket.server_hostname
+
bytesinstance (...)Oh, I would prefer to have a Unicode string here, but I see that the attribute is *not* new, it's already present in Python 3.4.
The versionadded field is wrong. It looks like the attribute was introduced in Python 3.2:
https://docs.python.org/dev/whatsnew/3.2.html#ssl
Maybe the change is that the attribute now also exists in the _ssl module? But the _ssl module is not documented.
+.. method:: MemoryBIO.write_eof()
+
+ Write an EOF marker to the memory BIO. After this method has been called, it
+ is illegal to call :meth:`~MemoryBIO.write`.
What does it mean, "illegal"? An exception is raised? Maybe it would be more explicit to say that an exception is raised.
+- All IO on an :class:`SSLObject` is non-blocking. This means that for example
+ :meth:`~SSLSocket.read` will raise an :exc:`SSLWantReadError` if it needs
+ more data than the incoming BIO has available.
It would be nice to repeat this information in the documentation of the SSLObject.read() method.
+.. method:: SSLContext.wrap_bio(incoming, outgoing, server_side=False, \
Why not puting the documentation of this method where the SSLContext is documented?
+Some notes related to the use of :class:`SSLObject`:
I suggest to move these notes just after the documentation of the SSLObject class.
+The following methods are available from :class:`SSLSocket`:
methods and *attributes*.
+class SSLObject:
SSLContext.wrap_bio. This class is typically used by framework authorsSSLSocket, this object lacks the following features:This documentation is useful. It should be copied in the documentation of the SSLObject class.
+.. class:: MemoryBIO
+
+ A memory buffer that can be used to pass data between Python and an SSL
+ protocol instance.
+
+.. attribute:: MemoryBIO.pending
+
+ Return the number of bytes currently in the memory buffer.
I prefer when class methods and attributes are part of the "class" block in the documentation, the documentation is rendered differently and it's more readable IMO.
Compare:
https://docs.python.org/dev/library/ssl.html#ssl.MemoryBIO
with:
https://docs.python.org/dev/library/io.html#io.IOBase
But the choice how to document methods and attributes was not made in the memory BIO changeset, it is older. I suggest to "upgrade" to style to use the same style than the io module (put everything in the ".. class:" block).
What do you think?
geertj commentedon Oct 6, 2014
Hi Victor,
see below my comments:
Both SSLSocket.read() and _SSLSocket.read() already accepted two arguments so I went for consistency. The former has been publicly documented in prior releases so I don't think it can be changed?
This is when it was first documented. If it's more correct to specify when it was first implemented then I can put it to 3.2.
Agreed that it should be changes to unicode.
Currently SSLSocket.server_hostname is whatever was passed to the constructor, which can be unicode or an already encoded bytes instance. SSLObject.server_hostname on the other hand is always a bytes instance.
Should SSLSocket.server_hostname also be changed to always return unicode even if a bytes was passed to the constructor? I'd tend to say yes especially because the attribute was not documented before. But it would be a change in behavior.
Now that I think of it - since SSLSocket now uses SSLObject to check the hostname, and SSLObject exposes server_hostname as a bytes instance, is hostname checking currently broken for non-ascii hostnames?
Mostly make sense. I will have a look.
pitrou commentedon Oct 6, 2014
+.. attribute:: SSLSocket.server_hostname
+
bytesinstance (...)Ah, this is a mistake. It's actually always a str instance (on SSLObject as well).
geertj commentedon Oct 6, 2014
It is indeed, I stand corrected. I was confused by the decode -> encode roundtrip that happens in _ssl.
However I think that in theory SSLSocket.server_hostname could be a bytes, if a bytes was passed into the constructor. _ssl parses this argument with the "et" format which means it will let a correctly encoded byte string through. Not sure if anybody is using this though.
vstinner commentedon Oct 6, 2014
newPySSLSocket() expects a char* string and use PyUnicode_Decode() to decode bytes.
geertj commentedon Oct 7, 2014
Yup, and this value is available as SSLSocket._sslobj.server_hostname. But SSLSocket.server_hostname is not this, it is what was passed to the constructor which can be a bytes instance.
For total cleanness maybe the constructor should raise a TypeError if server_hostname is passes as a bytes.
vstinner commentedon Oct 7, 2014
Oh. So there are two attributes? SSLSocket.server_hostname and
SSLSocket._sslobj.server_hostname? The constructor should make sure
that both are consistent (ex: initialize SSLSocket.server_hostname
from sslobj.server_hostname), or SSLSocket.server_hostname should be a
property reading SSLSocket._sslobj.server_hostname (or a getter/setter
if we should be able to modify it).
vstinner commentedon Oct 9, 2014
Here is a first patch for SSL documentation. If the patch looks fine, I will first apply revelant parts to Python 3.4 documentation.
python-dev commentedon Oct 10, 2014
New changeset 54402b25f0b9 by Victor Stinner in branch '3.4':
Issue bpo-22564: ssl doc: fix typos
https://hg.python.org/cpython/rev/54402b25f0b9
New changeset 61e52fda1006 by Victor Stinner in branch '3.4':
Issue bpo-22564: ssl doc: document read(), write(), pending, server_side and
https://hg.python.org/cpython/rev/61e52fda1006
New changeset 63386eda0cb7 by Victor Stinner in branch '3.4':
Issue bpo-22564: ssl doc: use "class" marker to document the SSLSocket class
https://hg.python.org/cpython/rev/63386eda0cb7
New changeset 8338ae599931 by Victor Stinner in branch '3.4':
Issue bpo-22564: ssl doc: mention how SSLSocket are usually created
https://hg.python.org/cpython/rev/8338ae599931
python-dev commentedon Oct 10, 2014
New changeset 61fbd3d5c307 by Victor Stinner in branch '3.4':
Issue bpo-22564: ssl doc: mention asyncio in the non-blocking section
https://hg.python.org/cpython/rev/61fbd3d5c307
New changeset 11ad670ca663 by Victor Stinner in branch 'default':
Issue bpo-22564: ssl doc: reorganize and reindent documentation of SSLObject and
https://hg.python.org/cpython/rev/11ad670ca663
New changeset 28fcf6c01bd9 by Victor Stinner in branch 'default':
Issue bpo-22564: ssl doc, add more links to the non-blocking section
https://hg.python.org/cpython/rev/28fcf6c01bd9
New changeset 5f773540e2ef by Victor Stinner in branch 'default':
Issue bpo-22564: cleanup SSLObject doc
https://hg.python.org/cpython/rev/5f773540e2ef
vstinner commentedon Oct 10, 2014
Ok, I think I addressed most of my remarks and I consider the issue as done.
Please open a new issue to address this point.