Skip to content

gh-60691: allow certificates to be specified from memory #2449

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
jgehrcke wants to merge 7 commits into from
Closed

gh-60691: allow certificates to be specified from memory #2449

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
7c3ddb5
bpo-16487: allow certificates to be specified from memory
jgehrcke Jun 27, 2017
34c1f0c
Merge remote-tracking branch 'origin/master' into address-issue-16487
jgehrcke Sep 30, 2017
f3048d9
_ssl.c: address 'dereferencing pointer to incomplete type' errors
jgehrcke Sep 30, 2017
56c1654
test_ssl: make it pass on OpenSSL 1.1.0
jgehrcke Sep 30, 2017
990e1c4
_ssl.c: improve comments
jgehrcke Sep 30, 2017
4f6671e
Lib/ssl.py: fix indendation
jgehrcke Oct 1, 2017
bf6df1f
bpo-16487: add entry to NEWS.d/next/Library
jgehrcke Oct 2, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 10 additions & 6 deletions Doc/library/ssl.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1270,13 +1270,13 @@ to speed up repeated connections from the same clients.
.. method:: SSLContext.load_cert_chain(certfile, keyfile=None, password=None)

Load a private key and the corresponding certificate. The *certfile*
string must be the path to a single file in PEM format containing the
string must be the path to a single file or a file object containing the
certificate as well as any number of CA certificates needed to establish
the certificate's authenticity. The *keyfile* string, if present, must
point to a file containing the private key in. Otherwise the private
key will be taken from *certfile* as well. See the discussion of
:ref:`ssl-certificates` for more information on how the certificate
is stored in the *certfile*.
the certificate's authenticity, in the PEM format. The *keyfile* string, if
present, must be the path to a file or a file object containing the private
key in PEM format. Otherwise the private key will be taken from *certfile*
as well. See the discussion of :ref:`ssl-certificates` for more information
on how the certificate is stored in *certfile*.

The *password* argument may be a function to call to get the password for
decrypting the private key. It will only be called if the private key is
Expand All @@ -1287,6 +1287,10 @@ to speed up repeated connections from the same clients.
as the *password* argument. It will be ignored if the private key is not
encrypted and no password is needed.

If *certfile* is provided as a file path, *keyfile* (if given) must be
provided as a file path as well (mixing file path and file object for these
arguments is not allowed).

If the *password* argument is not specified and a password is required,
OpenSSL's built-in password prompting mechanism will be used to
interactively prompt the user for a password.
Expand Down
46 changes: 46 additions & 0 deletions Lib/ssl.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@
import re
import sys
import os
import io
from collections import namedtuple
from enum import Enum as _Enum, IntEnum as _IntEnum, IntFlag as _IntFlag

Expand Down Expand Up @@ -450,6 +451,51 @@ def load_default_certs(self, purpose=Purpose.SERVER_AUTH):
self._load_windows_store_certs(storename, purpose)
self.set_default_verify_paths()

def load_cert_chain(self, certfile, keyfile=None, password=None):
# If `certfile` is bytes or string, treat it as file path.
if isinstance(certfile, str) or isinstance(certfile, bytes):
certfile_path = certfile

# If no `keyfile` is given, read private key from `certfile`.
if keyfile is None:
keyfile_path = certfile_path
else:
# If `certfile` is bytes or string, expect `keyfile` to be
# a bytes or string file path, too.
keyfile_path = keyfile

# Pre CPython 3.7 behavior: let OpenSSL consume the files via
# SSL_CTX_use_certificate_chain_file().

@pitrou pitrou Jun 29, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why not read the files in Python and fall through below? This would allow deleting a bunch of C code.

@jgehrcke jgehrcke Jun 30, 2017

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

It's intriguing. You are asking this now and I feel like "mhm, I think there was a pretty strong reason to not do it, because I tried hard, but what was the reason again?".

I am glad that I wrote a rather complete summary one year ago. It revived my memories... this is an excerpt from it:


[...]

An important note on backwards compatibility:

>> Antoine, are you suggesting that we remove the current c-level
>> capability to use file system files (using open()) and just go with
>> raw bytes data at the C api level, and then do the 'filename or
>> filelike object' in Python land?
>
> Yes, I think that's reasonable. Hopefully it won't break any existing uses.

I absolutely agree that Python code should be responsible for doing the
distinction between file paths and file objects. However, I do not agree with

> remove the current c-level capability to use file system files

After following the code I am rather sure that we should leave the current C
implementation (`_ssl__SSLContext_load_cert_chain_impl()` in Modules/_ssl.c)
as untouched as possible, and not try to integrate the new logic into that
very same function.

[...]

The central argument for having two separate C code paths is that old code
must proceed using OpenSSL's SSL_CTX_use_certificate_chain_file().

[...]

A bit of background on this can be found in the summary I linked to. I tried hard to understand the OpenSSL docs and the corresponding C code in the CPython repository, and tried hard to come up with what you were suggesting but I failed achieving the level of confidence that's required here. I was unable to achieve the desired level of confidence as of inherently bad OpenSSL API docs, and (this is what I vaguely remember) a few obvious mismatches between code and docs. In my summary I also wrote

When inspecting the OpenSSL code for SSL_CTX_use_certificate_chain_file(), some
of the above statements are not entirely correct.

(referring to statements from API docs)

So I could not rely on doc. Then I probably thought that the only way to gain confidence was via a proper test suite. In this particular case here, however, we probably agree that a test suite that actually matters does not really exist. It's comprised of all the applications in the world that use this code path. As of certificate diversity and pure combinatorics the input space into this function is super large.

From here it was rather easy to conclude for me that the old code path is better left intact, and that having a switch/shunt at a higher level is much safer in terms of backwards compatibility.

return self._load_cert_chain_pem_from_file_paths(
certfile_path, keyfile_path, password)

# Expect `certfile` to be a file object, expect `keyfile` to be `None`
# or a file object. Read file(s) and prepare OpenSSL memory BIO
# objects. If file objects return text, encode it to bytes.

certdata = certfile.read()
if isinstance(certdata, str):
certdata = certdata.encode('utf-8')

if keyfile is not None:
keydata = keyfile.read()
if isinstance(keydata, str):
keydata = keydata.encode('utf-8')
else:
# Expect that `certdata` contains the private key, too.
keydata = certdata

certbio = MemoryBIO()
certbio.write(certdata)
certbio.write_eof()

keybio = MemoryBIO()
keybio.write(keydata)
keybio.write_eof()

return self._load_cert_chain_pem_from_bio(certbio, keybio, password)


@property
def options(self):
return Options(super().options)
Expand Down
Loading