cpython

cpython copied to clipboard

• Issue #109945

Co-authored-by: Martin Schmatz

This change makes it possible to allow multiple groups to be specified in a colon separated string. See issues for more detail

The implementation choice is to modify the existing function rather than introduce a new one.

Initially opening as draft to check contributor guidelines, code layout etc.

• Issue: gh-109945

All commit authors signed the Contributor License Agreement.

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

FYI am aware of the test failure. Will be working on update for that. (apologies for delay) + arranging CLA.

The test failure occurs within test_set_ecdh_curve which tests a follows:

    @unittest.skipUnless(ssl.HAS_ECDH, "ECDH disabled on this OpenSSL build")
    def test_set_ecdh_curve(self):
        ctx = ssl.SSLContext(ssl.PROTOCOL_TLS_SERVER)
        ctx.set_ecdh_curve("prime256v1")
        ctx.set_ecdh_curve(b"prime256v1")
        self.assertRaises(TypeError, ctx.set_ecdh_curve)
        self.assertRaises(TypeError, ctx.set_ecdh_curve, None)
        self.assertRaises(ValueError, ctx.set_ecdh_curve, "foo")
        self.assertRaises(ValueError, ctx.set_ecdh_curve, b"foo")

This raises two issues

a) If the change in this PR is to be included, the unit tests should be updated to also include lists of curves - no problem

More relevant is

b) The current tests assert that an exception is raised if an invalid curve is specified (which is not in the initial code)

There are a few ways of handling this

1. I could parse the list of curves, checking each one for validity and raising an exception either on first failure, or after finding all errors. (ie strtok loop, OBJ_snnid checks). This would keep the behaviour effectively the same as the current version, with precisely reported errors, but is the most complex. (and some concern about any loop iterating through the list ad implementation questions, such as is using of string.h / strtok_r ok?)
2. There are lots of places in the ssl code in general where we use _setSSLError and just return NULL without formatting a python error - even in the original code this occurs. The test could be changed to permit this -- but this is a change in behaviour
3. In addition to _setSSLError when we fail to set the curves, I could add something like PyErr_Format(PyExc_ValueError,"unknown elliptic curves %R", name);. This keeps the behaviour closest for the test case, and is the simplest implementation, but does mean other ssl failures now raise an exception where they didn't previously.

Not being very familiar with the code I'm interested in feedback as to what the most appropriate way of handling this would be? Is the additional complexity of the first option the most appropriate & desirable?

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

The current PR proposal builds cleanly. However is an area that needs discussion

Previously an invalid (no NID) curve would result in a value exception, whilst any other kind of SSL error would not, though it would set the ssl error. This passed tests cleanly.

Since we now can have multiple curves, one option is to simple call the underlying SSL function to set the curves. This makes it difficult to distinguish between the two cases, meaning that the code returns a valueerror in both cases (for invalid curves)

In working through this, I also noticed that the thread sanitizer checks FAIL when _setSSLError() is called. Before this code change we still did call this, just in less cases, and via a code path that the unix tests didn't check. With the code change the new code (minus the most recent commit) would set this on every exception (as above). My conclusion is that this function isn't thread safe? So is that a more general bug that needs investigation?

For now the _setSSLError() is removed, however I think it should be reinstated once understood.

Rebased. Would very much appreciate any review comments . Thanks!

Aware of one doc validation error from my news blurb

Adds support for multiple curves to be specified in SSLContext.set_ecdh_curve() for OpenSSL 3.0 and above by setting curve_name to a colon separated list of curves. This allows multiple curves to be passed on a TLS client hello.

/home/runner/work/cpython/cpython/build/NEWS:70: WARNING: py:func reference target not found: warnings.filterswarnings

FYI - I'm looping in @sethmlarson for additional eyeballs on whether or not they think this makes sense given it is a _ssl module change.

Going to also ping @woodruffw and @jvdprng since they're working on an adjacent project.

Going to also ping @woodruffw and @jvdprng since they're working on an adjacent project.

Thank you for the ping!

I'd like to understand the motivation a little better here: is the current cipher suite configuration insufficient? And is there a reason why ssl needs an explicit configuration for this, instead of allowing the client/server to negotiate a compatible cipher suite?

I'd like to understand the motivation a little better here: is the current cipher suite configuration insufficient? And is there a reason why ssl needs an explicit configuration for this, instead of allowing the client/server to negotiate a compatible cipher suite?

This API is for configuring the curves to offer during ECDH key exchange, not the ciphers.

This API is for configuring the curves to offer during ECDH key exchange, not the ciphers.

Whoops!

That makes more sense, although I'm still curious about the intended application here -- IME it's not common for user-level TLS APIs (with the notable exception of OpenSSL) to expose curve selection as a configurable parameter, instead preferring to keep that as a implementation detail of negotiation (with the assumption that the TLS implementation either ensures a baseline level of security or the user configures a higher level "security margin" setting that influences the curve selection.)

TL;DR: My basic concern is that this exposes an API that's uncommon among TLS APIs, is a low level option that is typically only influenced by higher level APIs, and is a potential footgun. But one could easily argue that many of OpenSSL's APIs are footguns and thus there is ample precedent for exposing this 🙂

Quick update - thanks for the review comments - apologies for the delay: just back from a few week's vacation, so will work through & respond specifically. Appreciated.