Expose support for using ecdhe with SSL connections #9

alex · 2014-01-17T18:09:13Z

Needs pyca/cryptography@16168d0 to be in a release
how to test
documentation somewhere?

hynek · 2014-01-17T18:11:06Z

OpenSSL/SSL.py

“constants”

hynek · 2014-01-17T18:13:31Z

As for tests, can’t you have a conditional test that runs if HAVE_EC is true and tries to get some common curve?

alex · 2014-01-17T18:56:57Z

Tests are failing because this isn't in a release of Cryptography yet.

hynek · 2014-01-17T19:01:24Z

OpenSSL/SSL.py

Can we make this positive please? OPENSSL_HAS_EC sounds more descriptive anyway.

hynek · 2014-01-17T19:38:46Z

Just to add a bit of context, this is how ECDHE works in nginx: http://trac.nginx.org/nginx/browser/nginx/src/event/ngx_event_openssl.c#L680

alex · 2014-01-17T19:39:36Z

Do you think the APIs we expose should be based on the strings, instead of
NIDs?

On Fri, Jan 17, 2014 at 1:38 PM, Hynek Schlawack
notifications@github.comwrote:

Just to add a bit of context, this is how ECDHE works in nginx:
http://trac.nginx.org/nginx/browser/nginx/src/event/ngx_event_openssl.c#L680

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/9#issuecomment-32639384
.

"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

hynek · 2014-01-17T19:41:56Z

Is there something like curve_name_to_nid? Could we write it if not?

alex · 2014-01-17T19:45:58Z

If you look at the nginx code, that's what it does.

On Fri, Jan 17, 2014 at 1:41 PM, Hynek Schlawack
notifications@github.comwrote:

Is there something like curve_name_to_nid? Could we write it if not?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/9#issuecomment-32639667
.

"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

hynek · 2014-01-17T19:54:31Z

I know, I just wasn’t sure if it’s an internal nginx function or not. I’m generally opposed to use convenience data types as arguments if all it takes is to write f(g(x)) instead of f(x). This also means that the name would have to be looked up for each call – that doesn’t seem like a great idea for such a low-level API.

Conflicts: .gitignore OpenSSL/test/test_ssl.py

lvh · 2014-02-27T15:47:56Z

I think crossbar does a lot that we want in a decent way, but it's AGPL: https://github.com/crossbario/crossbar/blob/master/crossbar/crossbar/tlsctx.py

I think it's pretty confusing that "set_tmp_ecdh_by_curve_name" takes a NID instead of a string name. I think we should have a method that takes a NID, an API method takes a name and uses a simple dict lookup to translate between the two. (Basically exactly like that source file I linked.)

I am also at a loss on how to test this properly beyond "it appears to have worked".

hynek · 2014-02-27T15:56:11Z

I imagine @oberstet might let talk himself into donating that code to pyOpenSSL?

lvh · 2014-02-27T15:57:42Z

That would be very nice of him; it would certainly save us some time :)

oberstet · 2014-02-27T16:01:54Z

@hynek @lvh sure: I am the only author of https://github.com/crossbario/crossbar/blob/master/crossbar/crossbar/tlsctx.py and I hereby relicense the former file under the pyOpenSSL's own license. And it would be awesome if the code could dissolve into pyopenssl. It's soley concerned with TLS, and that shouldn't be in an app.

amluto · 2014-02-27T21:01:37Z

I have a patch for the old pyOpenSSL C code here:

https://bugs.launchpad.net/pyopenssl/+bug/1233810/+attachment/3907155/+files/ecdh_v2.patch

What do people think about the API? I can try to port it over, but:

EC_get_builtin_curves appears to be missing
A lot of NIDs are missing. I'm a but vague on how this is supposed to work in the hazmat binding code.

exarkun · 2014-03-01T22:17:48Z

OpenSSL/SSL.py

I think that the idiom thus far in pyOpenSSL is to wrap the kind of stateful object that a ... EC_KEY* (what SSL_CTX_set_tmp_ecdh takes) is in a thin object and unwrap it as necessary when going down to OpenSSL.

What do you think about an ... EllipticalCurveKey class that wraps this EC_KEY* and changing this method so that it is merely a wrapper around SSL_CTX_set_tmp_ecdh? EllipcitalCurveKey.new_by_curve_name(nid) might be the way the other half of this functionality is accessed.

Just thinking out loud here, let me know if this sounds dumb.

amluto · 2014-03-05T00:09:13Z

(Is this issue intentionally closed? I don't think anything actually got merged.)

I think that exposing actual EC objects is silly -- loading a non-named ECDH curve is pointless, since as far as I know essentially no TLS implementation actually supports non-named curves. (There's a mechanism in the spec, though.) So the only real use case (for TLS contexts anyway) is to load a curve by name.

alex · 2014-03-05T00:10:26Z

(I believe JP has been closing PRs as a means of marking them reviewed).

I don't have a strong opinion about EC objects vs. just nids vs.
referencing them by strings with their names. Can someone feel very
strongly about this and tell me what to do it :-)

On Tue, Mar 4, 2014 at 4:09 PM, Andrew Lutomirski
notifications@github.comwrote:

(Is this issue intentionally closed? I don't think anything actually got
merged.)

I think that exposing actual EC objects is silly -- loading a non-named
ECDH curve is pointless, since as far as I know essentially no TLS
implementation actually supports non-named curves. (There's a mechanism in
the spec, though.) So the only real use case (for TLS contexts anyway) is
to load a curve by name.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/9#issuecomment-36695584
.

"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 · 2014-03-05T00:18:40Z

(aha)

TBH, I think that using magic numbers is silly here, and OpenSSL's idea that NID_xyz is a "name" is daft.

That being said, using NID_xyz constants is nice because the error you get if you misspell them is obvious. (And you can tab-complete it.) Also, the implementation is simpler. So do that :)

See pyca/cryptography#732, too, though -- exposing all the constants would be nice.

hynek · 2014-03-05T09:38:32Z

It would be great if the proposed API could take into account the following new feature in 1.0.2:

*) Support for automatic EC temporary key parameter selection. If enabled the most preferred EC parameters are automatically used instead of hardcoded fixed parameters. Now a server just has to call: SSL_CTX_set_ecdh_auto(ctx, 1) and the server will automatically support ECDH and use the most appropriate parameters.

(I’m not saying that you should support it out of the box, just to take it into account when making API decisions.)

amluto · 2014-03-05T23:24:04Z

One option for supporting set_ecdh_auto: implement it on all OpenSSL versions, but have it automatically fall back to just selecting P-256 on pre-1.0.2 OpenSSL.

(I still think that this or #57 should be merged, since I can't see any reason not to support selecting an explicit curve.)

Expose support for using ecdhe with SSL connections

7b8d57a

hynek reviewed Jan 17, 2014
View reviewed changes

OpenSSL/SSL.py Outdated

Copy link

Contributor

hynek Jan 17, 2014

Choose a reason for hiding this comment

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

“constants”

alex added 3 commits January 17, 2014 12:45

Corrected a typo

a683fc0

Added tests

12dc084

Added a gitignore

2c04e86

hynek reviewed Jan 17, 2014
View reviewed changes

OpenSSL/SSL.py Outdated

Copy link

Contributor

hynek Jan 17, 2014

Choose a reason for hiding this comment

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

Can we make this positive please? OPENSSL_HAS_EC sounds more descriptive anyway.

Use the internal name

807853c

alex added 3 commits January 19, 2014 21:03

Added documentation

d5419e2

Merge branch 'master' into ecdhe-support

479878d

Conflicts: .gitignore OpenSSL/test/test_ssl.py

Merge branch 'master' into ecdhe-support

32f016f

exarkun reviewed Mar 1, 2014
View reviewed changes

exarkun closed this Mar 1, 2014

amluto mentioned this pull request Mar 5, 2014

Add fancier ECDHE support #57

Closed

exarkun mentioned this pull request Apr 19, 2014

Ecdhe #101

Merged

tiran mentioned this pull request May 28, 2015

Segfault in X509 object from verify callback #273

Closed

alex deleted the ecdhe-support branch July 7, 2017 03:15

github-actions bot locked as resolved and limited conversation to collaborators Aug 17, 2020

Expose support for using ecdhe with SSL connections #9

Expose support for using ecdhe with SSL connections #9

Uh oh!

Conversation

alex commented Jan 17, 2014

Uh oh!

hynek Jan 17, 2014

Choose a reason for hiding this comment

Uh oh!

hynek commented Jan 17, 2014

Uh oh!

alex commented Jan 17, 2014

Uh oh!

hynek Jan 17, 2014

Choose a reason for hiding this comment

Uh oh!

hynek commented Jan 17, 2014

Uh oh!

alex commented Jan 17, 2014

Uh oh!

hynek commented Jan 17, 2014

Uh oh!

alex commented Jan 17, 2014

Uh oh!

hynek commented Jan 17, 2014

Uh oh!

lvh commented Feb 27, 2014

Uh oh!

hynek commented Feb 27, 2014

Uh oh!

lvh commented Feb 27, 2014

Uh oh!

oberstet commented Feb 27, 2014

Uh oh!

amluto commented Feb 27, 2014

Uh oh!

exarkun Mar 1, 2014

Choose a reason for hiding this comment

Uh oh!

amluto commented Mar 5, 2014

Uh oh!

alex commented Mar 5, 2014

Uh oh!

amluto commented Mar 5, 2014

Uh oh!

hynek commented Mar 5, 2014

Uh oh!

amluto commented Mar 5, 2014

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

6 participants