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

ssl context #23

Merged
merged 5 commits into from
Oct 2, 2020
Merged

ssl context #23

merged 5 commits into from
Oct 2, 2020

Conversation

rashley-iqt
Copy link
Contributor

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

@rashley-iqt
Copy link
Contributor Author

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

@vog
Copy link
Member

vog commented Sep 2, 2020

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

@vog
Copy link
Member

vog commented Sep 4, 2020

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
Copy link
Contributor Author

fixed that typo

@vog
Copy link
Member

vog commented Sep 4, 2020

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
Copy link
Contributor Author

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:

  • open the file for writing
  • write to the file
  • flush the buffer
  • re-open for reading
    I was worried that if i just opened the file in read/write mode i wouldn't necessarily get clean data out of it, so it seemed less brittle to close and re-open, enforcing the deletion in a finally block.

@rashley-iqt
Copy link
Contributor Author

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

@vog
Copy link
Member

vog commented Oct 2, 2020

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 vog merged commit d7bf523 into m-click:master Oct 2, 2020
@vog
Copy link
Member

vog commented Oct 2, 2020

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.)

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

Successfully merging this pull request may close these issues.

None yet

2 participants