Skip to content

Allow to load SSL certfile and keyfile from a file-like object #129216

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.

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

Open
comsyspro opened this issue Jan 23, 2025 · 4 comments
Open

Allow to load SSL certfile and keyfile from a file-like object #129216

comsyspro opened this issue Jan 23, 2025 · 4 comments
Labels
extension-modules C modules in the Modules dir topic-SSL type-feature A feature request or enhancement

Comments

@comsyspro
Copy link

comsyspro commented Jan 23, 2025

Feature or enhancement

Proposal:

# Have a look in commit 7c3ddb5
# It should be possible to also load certfile and keyfile from embedded certificate strings
cert_file = io.BytesIO(SERVER_CERT_STRING.encode('utf-8'))
key_file = io.BytesIO(SERVER_KEY_ENCRYPTED_STRING.encode('utf-8'))
context.load_cert_chain(certfile=cert_file, keyfile=key_file, password=key_pass)

7c3ddb5

It should be possible to also load certfile and keyfile from embedded certificate strings. At the moment the code only reads the certificates from files (filepath). But when you don't want to have the saved/written certificates on the storage you should be able to load them from embedded strings.

Would it possible to include this code to the latest cpython? I tested this code from commit 7c3ddb5 with cpython v3.8.10 and here it almost has worked directly and only few lines had to be adapted. But I also tried it with the latest cpython version and here in Visual Studio there are some warnings. I don't know why this functionality isn't included in the master branch because it adds useful and advanced functionalities.

Has this already been discussed elsewhere?

#60691

Links to previous discussion of this feature:

main...jgehrcke:cpython:jgehrcke/address-issue-16487-sept17

@comsyspro comsyspro added the type-feature A feature request or enhancement label Jan 23, 2025
@picnixz picnixz changed the title proposal for advanced context.load_cert_chain() function: load certfile and keyfile from embedded certificate strings Allow to load SSL certfile and keyfile from a file-like object Jan 23, 2025
@picnixz picnixz added extension-modules C modules in the Modules dir topic-SSL labels Jan 23, 2025
@picnixz
Copy link
Member

picnixz commented Jan 23, 2025

I don't know why this functionality isn't included in the master branch because it adds useful and advanced functionalities.

The rationale for the previous rejected feature was explained here: #2449 (comment). Please read the entire thread before reposing a new feature request in the future so to avoid duplicated discussions.

Relevant other comments:

There has been a recipe as an alternative (#2449 (comment)):

#!/usr/bin/env python3

import tempfile
import ssl

def load_cert_memory(context, certdata, keydata, password):
    if not password:
        raise ValueError("Insecure key")
    with tempfile.NamedTemporaryFile(mode="wt") as f:
        if keydata is not None:
            f.write(keydata)
            if not keydata.endswith("\n"):
                f.write("\n")
        f.write(certdata)
        f.flush()
        context.load_cert_chain(f.name, password=password)


def test():
    passphrase = "somepass"
    with open("Lib/test/ssl_cert.pem") as f:
        testcert = f.read()
    with open("Lib/test/ssl_key.passwd.pem") as f:
        testkey = f.read()
    with open("Lib/test/keycert.passwd.pem") as f:
        combined = f.read()

    ctx = ssl.create_default_context(ssl.Purpose.CLIENT_AUTH)
    load_cert_memory(ctx, testcert, testkey, passphrase)

    ctx = ssl.create_default_context(ssl.Purpose.CLIENT_AUTH)
    load_cert_memory(ctx, combined, None, passphrase)


if __name__ == "__main__":
    test()

Now, it was suggested loading certificates and so from any object implementing the Buffer protocol. The problem, as highlighted by #60691 (comment) is that, while it wouldn't matter for the certificate, it opens a possible security hole if you allow to load the key from a memory buffer. I personally don't want to endorse that kind of change (as this change is likely non-trivial and should be carefully reviewed) but maybe @gpshead has some thought on this. Otherwise, I'd recommend closing this one as a duplicate of #60691.

@comsyspro
Copy link
Author

I already used this "tempfile" approach to write out the embedded strings and securely delete the created tempfiles afterwards. But here you have to write the certificate files to disk (and this is not wanted in some use cases). I want to have a python application with embedded certificate files which can read them from inside the code without writing them out. This python application is then protected and embedded inside a c++ executable, so the certs are not visible.

@gpshead
Copy link
Member

gpshead commented Feb 5, 2025

The old PR #2449 looked far more complicated than I expected, but might contain what is needed. However it was created back when OpenSSL 1.1.0 was dominant. Have things changed since? Now we're supporting OpenSSL 3.x and have some major users on the fork-duck AWS-LC & BoringSSL 1.1.1 API subset +extras as needed API trains.

This function https://github.com/python/cpython/pull/2449/files#diff-89879be484d86da4e77c90d5408b2e10190ee27cc86337cd0f8efc3520d60621R3451
is a bit annoying to have to carry around, but isn't that complicated from a maintainability standpoint.

But down in https://github.com/python/cpython/pull/2449/files#diff-89879be484d86da4e77c90d5408b2e10190ee27cc86337cd0f8efc3520d60621R3521-R3525 is where it got too complicated. I would not bother supporting the concept of a password callback. That was jumping the shark.

it opens a possible security hole if you allow to load the key from a memory buffer.

This is a documentation issue. keys and passwords stored in memory might be considered a "risk" to some... but lets be realistic. They're stored in process memory anyways, inside the SSL library.

I realize having them ever touch Python object memory increases risk of being able to avoid unwanted copies lingering in freed memory floating around the process (our VM can never make any guarantees there) but this is a choice people are making by wanting to pass them directly in anyways.

However, implementing an interface that accepts Python duck typed file-like objects is pointless complication. If you've got one of those, you can read it yourself and pass it in as a str/bytes. We don't need to have code doing that for users.

The simple thing would be to add the ability to pass them all as str/bytes in addition to the existing file paths interface.

@comsyspro
Copy link
Author

comsyspro commented Feb 5, 2025

I'm asking me if this could be similarily done like in this function context.load_verify_locations(cadata=ca_string). Here you can also pass a CA certificate as an embedded string. And this should also be possible for context.load_cert_chain(certdata=cert_string, keydata=key_string, password=password_string). Would this be possible to implement it like that?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
extension-modules C modules in the Modules dir topic-SSL type-feature A feature request or enhancement
Projects
None yet
Development

No branches or pull requests

3 participants