Yet another ecdhe PR

[amluto]

This is continuation of #57 and #82. Sigh.

[amluto]

Github's little picture of the change order is creatively broken. The two new patches are shown backwards, which is important, since tests fail if they're out of order.

[hynek]

Above, you write EC_NotAvailable but here Unsupported_EllipticCurve.

UnsupportedECCurve would be more consistent I guess.

[amluto]

Or UnsupportedEC. :)

[hynek]

You’ll have to decide what EC stands for and use it consistently. Apparently it’s used as a synonym for elliptic curve cryptography, so using it as an abbreviation for elliptic curve would be confusing (how would you differentiate between ECNotAvailable and UnsupportedEC?). Do you see the problem?.

[amluto]

Before I go too far down this rabbit hole, let me try one more counterargument:

This fancy exception code is going to end up being the majority of the entire change. To me, this suggests that it's growing out of control.

What if, instead, the semantics were that set_tmp_ecdh_curve threw ValueError if:

curve_name not in ELLIPTIC_CURVE_DESCRIPTIONS

This is already more or less the case. All other failures would be considered internal errors that should never happen. They could throw the same type, a different type, or fail an assertion -- I don't think it matters all that much.

The point being that there is already a list of the legal values for curve_name. This list exists and should be correct regardless of what hacked-up OpenSSL you're using, and I still can't see why any user would legitimately care why a given curve_name is not in ELLIPTIC_CURVE_DESCRIPTIONS.

IOW, the error handling in callers should be:

try:
    ctx.set_tmp_ecdh_curve(curve)
except ValueError:
    print('Your curve %r is not supported; legal values are %s' % (curve, ', '.join(ELLIPTIC_CURVE_DESCRIPTIONS.keys()))

Because no application should write actual code to deal with the fact that Fedora and RHEL ship OpenSSL versions that remove the EC code for certain curves but don't bother to remove them from the sn/nid list.

[exarkun]

I wonder if there's a good reason to expose the descriptions here. They don't seem to have particularly meaningful values. If someone wants to learn more about a particular curve they'll probably have better luck looking around for info on it on their own? So maybe this should just be a set.

Also I'm not sure this is the right API to expose even the names of the supported curves. It seems like an increase in the disorder of the module interface. Curves actually seems more crypto related than SSL. In the interest of getting some ECDHE support into pyOpenSSL though, maybe the right thing to do for now is just make this private and then sort out the rest of the API details relating to it later.

[amluto]

I have no problem with making it be a set. It would also be easy to move it into OpenSSL.crypto, or to rename it to _ELLIPTIC_CURVES or _ELLIPTIC_CURVE_DESCRIPTIONS, or to remove it entirely.

[exarkun]

I've started addressing my own review comments in pyca:ecdhe. To prevent wasted effort I'm signaling development should proceed there by closing this pull request. Sorry if this is not optimal but it's the best way I know how to use github (feel free to tell me to do something else).