Skip to content
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

Create an ssl.SSLContext #20

Closed
kafonek opened this issue May 4, 2020 · 15 comments
Closed

Create an ssl.SSLContext #20

kafonek opened this issue May 4, 2020 · 15 comments

Comments

@kafonek
Copy link

kafonek commented May 4, 2020

httpx is growing in popularity as the "next generation" requests.py. It can accept custom ssl.SSLContext objects but not PyOpenSSLContext, encode/httpx#924.

create_ssl_context returns a PyOpenSSLContext. Could that optionally return an ssl.SSLContext or another function added for that support?

Thanks.

@rashley-iqt
Copy link
Contributor

@vog is this a change you would be open to? If so I'd be happy to handle the actual implementation.

@vog
Copy link
Member

vog commented May 26, 2020

Yes, I'm open to that change, but would like to propose an even more preferable solution:

Please try to get our code included directly in httpx.

As stated in our README, this requests_pkcs12 library is meant to be a transitional solution until its functionality is provided by requests directly. However, that will take some time. See the corresponding issue for more details.

So, please open an issue in httpx, feel free to link to this issue from here (and reverse), and let's hope this feature will have a better fate in httpx than it had in requests.

@rashley-iqt
Copy link
Contributor

I like that plan. I will open an issue there and reference this thread. If you are amenable I'd still like to do a PR here to show a reference implementation to help streamline the discussion.

@vog
Copy link
Member

vog commented May 26, 2020

Of course, feel free to propose a pull request. I'd prefer a separate function next to create_ssl_context, perhaps moving common code into a third internal function. Feel free to rename the functions as you see fit (e.g. create_pyopenssl_context(), create_ssl_sslcontext() or similar)

@rashley-iqt
Copy link
Contributor

@vog and @kafonek , I wanted to get a quick opinion. The existing implementation of the Pkcs12Adapter is looking for a bytes object as an input, but the ssl standard library requires a file path for the cert file. My inclination is to take a bytes object and use a temp file created from that object because:

  • it maintains consistency between the calls
  • i can ensure that any cert files that i am responsible for are properly disposed of

Thoughts?

@kafonek
Copy link
Author

kafonek commented Jun 10, 2020

@rashley-iqt - If I'm reading right, Pkcs12Adapter takes either pkcs12_data or pkcs12_filename and a pkcs12_password kwargs. create_ssl_context (returns PyOpenSSLContext) takes pkcs12_data as bytes. The Adapter handles reading in the file as bytes if a file is passed in when it calls create_ssl_context.

I do think it's a good idea to have any new function that returns an ssl.SSLContext take the same type of arguments as the current create_ssl_context, which would be data and password as bytes right now?

httpx doesn't use Adapters like requests, so I don't see any need to change the current Pkcs12Adapter; all we need is a function that returns a ssl.SSLContext object. httpx.Client is analogous to requests.Session, but the Client can take in verify=<ssl.SSLContext object> as an init kwarg whereas Session's mount adapters after instantiation.

@rashley-iqt
Copy link
Contributor

yes so the current method, which i think should be renamed to create_pyopen_ssl_context, has a signature of create_pyopen_ssl_context( bytes pkcs_data, bytes password_data). So the logical equivalent would then be create_ssl_ssl_context( bytes pkcs_data, bytes password_data) but that has to call one of the load_cert_chain methods which only accept a file path. Thats why I would need the temp file.

@kafonek
Copy link
Author

kafonek commented Jun 11, 2020

Got it, thanks @rashley-iqt. If I have an encrypted .p12 file on disk already, would the workflow then look like the following?

data = open('/my/bundle.p12', 'rb').read()
pw = input('pki password: ').encode('utf8')

ctx = create_ssl_ssl_context(data, pw) 
### ^^ that step creates and destroys a tempfile with the decrypted p12 or converted to pem?
client = httpx.Client(verify=ctx)
resp = client.get(pki_required_url)

That sounds reasonable to me. I believe pypki2 takes more or less that approach when it creates ssl.SSLContext from pkcs12 right now. @gershwinlabs would have to double check me on that.

@vog
Copy link
Member

vog commented Jun 11, 2020

I strongly advice against creating temporary files.

However, if there's absolutely no way around it, I'd like to propose two things:

  1. Provide an API without the temporary file, i.e. if the p12 file already exists on disk, it makes no sense to copy it over to a temporary file. Please offer a second function to use the file directly.
  2. Make sure that the temporary file has permissions like "0600", is always deleted (e.g. via with ...:), and delete it as soon as possible.
    • If I remember correctly, it is sufficient that the temporary file only exists during creation of the SSL context object. As soon as it is created, it has all data and doesn't need to access the temporary file anymore. Thus, the temporary file would exist only for a few milliseconds.
    • In particular, don't keep the temporary file during the whole HTTPS request or session.

@vog
Copy link
Member

vog commented Aug 19, 2020

@kafonek, @rashley-iqt: It there anything left to do for the requests_pkcs12 project here?

@kafonek
Copy link
Author

kafonek commented Aug 19, 2020

@vog I still think it is a good idea for requests_pkcs12 to have a method just like create_ssl_context(pkcs12_data, pkcs12_password_bytes) that returns an ssl.SSLContext instead of urllib3.contrib.pyopenssl.PyOpenSSLContext. My use-case for that is integrating with httpx not requests though, so I understand if you think it is outside the scope of this project. I am not working on a PR.

@vog
Copy link
Member

vog commented Aug 19, 2020

I don't think this is outside the scope of this project. This library is very small, so making it fit for httpx in addition to requests is fine.

(I just noted in previous comments that inclusion in the httpx library would be preferable, but as long as this is not the case, I think this project can provide helper functions for them as well.)

In other words: Patches and/or pull requests regarding httpx are welcome.

@rashley-iqt
Copy link
Contributor

I have some code to turn into a PR for this use case. I just need to block out time to get it tested. I apologize for the delay, I don't want this to languish.

@rashley-iqt rashley-iqt mentioned this issue Aug 21, 2020
@rashley-iqt
Copy link
Contributor

just sent you a PR @vog

@kafonek
Copy link
Author

kafonek commented Nov 4, 2020

Sorry I just noticed this is still open while pointing someone else to this project. @rashley-iqt thank you for adding this! @vog I am good with this being closed if you are.

@vog vog closed this as completed May 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants