Skip to content

Conversation

exarkun
Copy link
Member

@exarkun exarkun commented Apr 19, 2014

Update of #89, #82, #71, #57, #9

This changes the interface in a small but significant way - curves are now represented by an explicit object with a name attribute and some other private attributes. This approach eliminates nearly all of the error conditions that the previous PRs tried to deal with (and thus eliminates the question of what the exceptions for those error conditions should look like).

It also tweaks some naming to bring it in line with the underlying OpenSSL APIs which is generally how pyOpenSSL naming has been done up to this point.

alex and others added 30 commits January 17, 2014 12:08
Conflicts:
	.gitignore
	OpenSSL/test/test_ssl.py
It would be great if there were a clean way to enumerate them rather
than just listing them like this, but I don't know of one.
…rves

Different OpenSSL builds support different curves.  Determine the
supported curves at startup and expose the list.
Using NIDs is awkward and requires updating pyOpenSSL every time a new
curve is added.  This approach avoids needing to update pyOpenSSL
each time a new curve is added, and it results in more readable code
and a more readable dict ELLIPTIC_CURVE_DESCRIPTIONS.
This also ninja-fixes the typo that was in the previous comment.
This sneakily fixes some test cases typos, too.
…t" does not install them in a way that makes them available later.
@sholsapp
Copy link
Contributor

Would it make sense to use the same/similar abstraction to preserve the interface being changed in #96? That PR seems to change the method signature because a curve has some extra bits of information required to generate keys.

I think we'd also need to add trivial abstractions for RSA/DSA type keys, but I think it would lead to a more OO implementation of generate_key and possibly more.

Thoughts?

@amluto
Copy link
Contributor

amluto commented Apr 29, 2014

Looks like a reasonable interface. Here are a couple of things I don't like, though:

  • get_elliptic_curves returns a set that doesn't act like a set. For example, OpenSSL.crypto.get_elliptic_curve('prime256v1') in OpenSSL.crypto.get_elliptic_curves() returns False. It might be nice to implement hash and equality on _EllipticCurve objects.
  • get_elliptic_curve is gratuitously slow. Maybe this doesn't matter, but IMO it's silly. There's an easy speedup: use the OpenSSL APIs.

@exarkun
Copy link
Member Author

exarkun commented Apr 29, 2014

Thanks for the review!

It might be nice to implement hash and equality on _EllipticCurve objects.

This seems reasonable.

use the OpenSSL APIs.

It already uses OpenSSL APIs. Lots of them. 😄 Can you be more specific? As far as I know, there isn't an API for retrieving a curve by name. The APIs for looking up a NID for a name and a curve for a NID introduce extra failure modes that I was happy to eliminate in the latest refactoring.

If performance ends up being important here (I doubt it will) then maybe just some style of memoization of this work would be good enough?

@alex
Copy link
Member

alex commented Apr 29, 2014

If this is going to be performance sensitive, it definitely seems like a
simple dict cache is the right solution. That said, this seems like the
type of thing called, at most, once per outgoing connection, so performance
isn't super critical.

On Tue, Apr 29, 2014 at 12:52 PM, Jean-Paul Calderone <
notifications@github.com> wrote:

Thanks for the review!

It might be nice to implement hash and equality on _EllipticCurve objects.

This seems reasonable.

use the OpenSSL APIs.

It already uses OpenSSL APIs. Lots of them. [image: 😄] Can you be
more specific? As far as I know, there isn't an API for retrieving a curve
by name. The APIs for looking up a NID for a name and a curve for a NID
introduce extra failure modes that I was happy to eliminate in the latest
refactoring.

If performance ends up being important here (I doubt it will) then maybe
just some style of memoization of this work would be good enough?


Reply to this email directly or view it on GitHubhttps://github.com//pull/101#issuecomment-41723706
.

"I disapprove of what you say, but I will defend to the death your right to
say it." -- Evelyn Beatrice Hall (summarizing Voltaire)
"The people's good is the highest law." -- Cicero
GPG Key fingerprint: 125F 5C67 DFE9 4084

@amluto
Copy link
Contributor

amluto commented Apr 29, 2014

I think that the extra failure modes for converting name to NID don't matter in practice. If you feed a name of a curve that get_elliptic_curves returns into OBJ_sn2nid and OBJ_sn2nid fails, then your OpenSSL implementation is just broken.

@exarkun
Copy link
Member Author

exarkun commented Apr 29, 2014

then your OpenSSL implementation is just broken.

... and this has never happened to anyone. 😉

exarkun added a commit that referenced this pull request May 1, 2014
Add basic support for using ECDHE.
@exarkun exarkun merged commit 1a3d211 into master May 1, 2014
@exarkun exarkun deleted the ecdhe branch February 17, 2015 17:49
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 23, 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.

5 participants