m-click / requests_pkcs12

Add PKCS#12 support to the Python requests library in a clean way, without monkey patching or temporary files
ISC License
124 stars 33 forks source link

ssl context #23

Closed rashley-iqt closed 4 years ago

rashley-iqt commented 4 years ago

Adds a method allowing users to create an ssl.SSLContext in addition to a PyOpenSSL.SSLContext as discussed in issue #20.

rashley-iqt commented 4 years ago

@vog have you had a chance to review this yet?

vog commented 4 years ago

Sorry for the delay, I'll review your contribution as soon as possible.

vog commented 4 years ago

I believe there's a typo in line 98, where the function is called under its first name instead of its renamed name.

rashley-iqt commented 4 years ago

fixed that typo

vog commented 4 years ago

Thanks for fixing!

Another question: Why do you suppress the automatic deletion of the temporary file, just to re-establish it afterwards?

with NamedTemporaryFile(delete=False) as c:
    try:
       FOO(c)
    finally:
        os.remove(c.name)

Can't this be simplified to this?

with NamedTemporaryFile() as c:
    FOO(c)

Moreover, a small coding style issue. Please add the missing space after the last comma:

from OpenSSL.crypto import load_pkcs12, dump_certificate, dump_privatekey,FILETYPE_PEM

New:

from OpenSSL.crypto import load_pkcs12, dump_certificate, dump_privatekey, FILETYPE_PEM
rashley-iqt commented 4 years ago

The suppression of automatic deletion of the temp file was done because closing the file will trigger that deletion. For the case here i needed to be able to:

rashley-iqt commented 4 years ago

@vog do I need to make any other changes here? I think I got everything unless you disagree about the implicit delete.

vog commented 4 years ago

I'm pretty sure that flushing (without closing) would have been sufficient to get a clean read afterwards.

However, I don't see a flaw in your approach, either, apart from 4 extra lines of code (try, finally, close, delete).

So I'm merging this as-is. Thanks again for your contribution!

vog commented 4 years ago

I just published a new release requests-pkcs12 1.9 containing your improvement.

Feel free to add something to the README, if you feel that we should make the additional function more visible (e.g. I'm not sure what other projects like httpx demand from using an additional library. Right now, to an outsider that function migh look like an undocumented purely internal implementation detail, on whose existence one should not rely.)