classification
Title: SSLContext.load_cert_chain() should accept a password argument
Type: enhancement Stage: resolved
Components: Library (Lib) Versions: Python 3.3
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: eric.araujo, giampaolo.rodola, janssen, pitrou, python-dev, simpkins
Priority: normal Keywords: patch

Created on 2011-08-21 07:28 by simpkins, last changed 2011-08-25 12:46 by pitrou. This issue is now closed.

Files
File name Uploaded Description Edit
ssl-password.patch simpkins, 2011-08-21 07:28 Patch to add a password argument to load_cert_chain() review
ssl-password.2.patch simpkins, 2011-08-21 19:42 Updated patch to accept any callable review
ssl-password.3.patch simpkins, 2011-08-24 18:34 Updated patch to fix missing decref review
ssl-password.4.patch simpkins, 2011-08-25 05:55 Patch with updated documentation review
Messages (11)
msg142595 - (view) Author: Adam Simpkins (simpkins) Date: 2011-08-21 07:28
The SSLContext.load_cert_chain() method should accept a password argument to use if the private key is encrypted.  Currently it always uses OpenSSL's default password callback, which prompts the user interactively for a password.

I've attached a patch that adds an extra password argument, which can be either a string, bytes, bytearray, or a function to call to get the password.
msg142611 - (view) Author: √Čric Araujo (eric.araujo) * (Python committer) Date: 2011-08-21 11:36
> I've attached a patch that adds an extra password argument, which can be
> either a string, bytes, bytearray, or a function to call to get the password.

It seems a bit strange to me to accept string types or callable in the same argument.  If it just supported strings, people could still write password=somefunction(), right?
msg142637 - (view) Author: Adam Simpkins (simpkins) Date: 2011-08-21 18:24
> It seems a bit strange to me to accept string types or callable in the 
> same argument.  If it just supported strings, people could still write
> password=somefunction(), right?

The function is only called if the private key is encrypted and a 
password is necessary.  This allows the code to only prompt for a 
password if it is actually needed.

This also parallels the OpenSSL C API, which only accepts a function to
get the password.  I added support for plain strings as a convenience.  
The string argument will be ignored if the file is not encrypted.
msg142640 - (view) Author: Adam Simpkins (simpkins) Date: 2011-08-21 19:42
Here's a new patch that accepts any callable.  The old patch only accepted
actual function objects.
msg142884 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-08-24 13:46
Thanks for the patch. This is a generally useful functionality and the patch looks mostly good.
I have a couple comments:

- in _pwinfo_set(), you need to decref password_bytes when you're finished
- you check the password size in _password_callback() but not in _pwinfo_set(), is it expected? is the size limit only meaningful with a callback rather than a predefined password?
msg142893 - (view) Author: Adam Simpkins (simpkins) Date: 2011-08-24 18:34
Good catch.  Here's an updated patch to fix the missing decref in
_pwinfo_set()

The length check in _password_callback() applies to both callback
functions and predefined strings.  The C API always uses a callback,
so _password_callback() is used even when the python caller has passed
in a string.  We can't check the length in _pwinfo_set() since it depends
on the buffer length argument that openssl will pass to
_password_callback().
msg142924 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-08-24 21:20
I have one last concern: what is the character set of an OpenSSL password? I see you are using PyUnicode_AsEncodedString(x, NULL, NULL), which basically returns a utf8-encoded bytestring. Since the OpenSSL doc don't specify anything, we could accept it as a best-effort thing.
Or perhaps we should use PyUnicode_EncodeFSDefault(), which uses the "filesystem encoding", which is usually the same as the current terminal (assuming the password is typed on a terminal).
msg142955 - (view) Author: Adam Simpkins (simpkins) Date: 2011-08-25 05:43
OpenSSL doesn't appear to do any special handling for i18n, and just
treats the strings as binary data.  It uses fgets() to read the password
from the terminal, so it will receive it however the terminal encodes
it.

It's not clear to me that PyUnicode_EncodeFSDefault() is quite the right
thing to do.  The initfsencoding() routine seems different from how
initstdio() works (which checks the PYTHONIOENCODING environment
variable, and then appears to fall back to os.device_encoding()).

I'm leaning towards just updating the docs to specify that if a string
is supplied it will be encoded using UTF-8.  I'm happy to do either way
you recommend, though.
msg142956 - (view) Author: Adam Simpkins (simpkins) Date: 2011-08-25 05:55
Here's a patch with updates to the documentation to more fully specify the
behavior of the password field, including specifying that strings will be
encoded using UTF-8.
msg142967 - (view) Author: Roundup Robot (python-dev) Date: 2011-08-25 12:45
New changeset cdc6c1b072a5 by Antoine Pitrou in branch 'default':
Issue #12803: SSLContext.load_cert_chain() now accepts a password argument
http://hg.python.org/cpython/rev/cdc6c1b072a5
msg142968 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-08-25 12:46
Your latest patch was committed, thank you!
History
Date User Action Args
2011-08-25 12:46:34pitrousetstatus: open -> closed
resolution: fixed
messages: + msg142968

stage: patch review -> resolved
2011-08-25 12:45:48python-devsetnosy: + python-dev
messages: + msg142967
2011-08-25 05:55:51simpkinssetfiles: + ssl-password.4.patch

messages: + msg142956
2011-08-25 05:43:27simpkinssetmessages: + msg142955
2011-08-24 21:20:48pitrousetmessages: + msg142924
2011-08-24 18:34:58simpkinssetfiles: + ssl-password.3.patch

messages: + msg142893
2011-08-24 13:46:47pitrousetmessages: + msg142884
2011-08-21 19:42:51simpkinssetfiles: + ssl-password.2.patch

messages: + msg142640
2011-08-21 18:24:36simpkinssetmessages: + msg142637
2011-08-21 11:36:36eric.araujosetnosy: + eric.araujo
messages: + msg142611
2011-08-21 07:45:33pitrousetstage: patch review
versions: + Python 3.3, - Python 3.1, Python 3.2
2011-08-21 07:42:54ezio.melottisetnosy: + janssen, pitrou, giampaolo.rodola
2011-08-21 07:28:26simpkinscreate