Skip to content

Conversation

@Lukasa
Copy link
Member

@Lukasa Lukasa commented Mar 31, 2014

This is part of #79.

This is my attempt to remain in the spirit of PyOpenSSL by making a minimal wrapper around OpenSSL's callback logic. As you can see, minimal is not my strong suit. Feedback would be appreciated. =D

Note: this will probably fail automated testing at the moment, as it relies on features that aren't yet in the current cryptography release. Passes on my machine on 2.7.

@Lukasa Lukasa mentioned this pull request Mar 31, 2014
@exarkun
Copy link
Member

exarkun commented Mar 31, 2014

Can you add a link to the cryptography pull request that introduces the necessary features? This will make it easier to figure out what release of cryptography this will depend on.

@Lukasa
Copy link
Member Author

Lukasa commented Mar 31, 2014

Of course, it's pyca/cryptography#857.

@Lukasa
Copy link
Member Author

Lukasa commented Apr 1, 2014

So the current cryptography master appears to pass (according to Travis) except on 3.2, which fails for reasons unrelated to my change. =)

@myers
Copy link

myers commented Apr 2, 2014

@Lukasa Excited to see NPN to get into pyopenssl.

@alex
Copy link
Member

alex commented May 3, 2014

The release has happened, so this should work now.

@Lukasa
Copy link
Member Author

Lukasa commented May 3, 2014

👍 Thanks for your help @alex!

@Lukasa
Copy link
Member Author

Lukasa commented May 7, 2014

@exarkun What can I do to help move this forward now that cryptograpy has a release with the needed bindings? =)

Copy link
Member

Choose a reason for hiding this comment

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

It might be nice to make Next Protocol Negotiation a link to a document about that concept. Super nice would be a pyOpenSSL-internal doc explaining all the pieces of NPN but considering NPN is kind of obsolete already and there isn't already such a doc, an external link would probably have the best cost/benefit ratio.

@exarkun
Copy link
Member

exarkun commented May 7, 2014

Thanks for your work on this Lukasa. Sorry I didn't get back to you with feedback sooner.

In addition to my inline comments, could you also add documentation for the new methods to doc/api/ssl.rst? Doesn't need to be any different from what you wrote in the docstrings (pyOpenSSL just has two copies of its API docs atm 😢 ).

Also, if you wouldn't mind closing this PR and re-opening it using a branch with a different name than "master" that would be wonderful.

Thanks again.

@Lukasa
Copy link
Member Author

Lukasa commented May 8, 2014

@exarkun No problem. =) There wouldn't have been a great deal to gain before now anyway, since it was all writing against bindings that didn't exist.

I'll handle the markups in the next couple of days. I'm super busy at the minute but this is a high priority for me (I need it for one of my OSS projects), so I won't let this languish for long.

@Lukasa
Copy link
Member Author

Lukasa commented May 10, 2014

@exarkun Ok, I've made the changes you suggested and opened a new pull request from the npn branch. The new PR is #110. Let me know what further changes you want! =)

@exarkun exarkun closed this May 15, 2014
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants