Raise NotImplementedError for Unimplemented Functions

[mhils]

As the title says - just silently doing nothing is really unexpected.

refs #387, refs #388

[Lukasa]

This seems reasonable, but I'll let @hynek make that call. Otherwise the patch looks good, though you'll want to improve the test coverage to account for these exceptions.

[mhils]

I'd personally rather like to see raise NotImplementedError added to .coveragerc's exclude_lines.
Thoughts?

[hynek]

1. If we do this, we should totally add tests to it that verify those errors are raised. That’s also an indication of intent.
2. I would like more input on this (@glyph @alex @reaperhulk ): this is clearly breaking backward compat. But I have a hunch that it's the right thing to do…opinions?

[glyph]

If code is currently calls these APIs and expects to get the security properties they advertise, it is a security flaw to pretend that it worked. Granted, none of these functions seem like an API which adds a restriction, but on general principle, I think they should be made to raise errors and do a release ASAP. It's compatibility-breaking, but in that sense it is a compatibility-breaking security update.

[alex]

I agree, alernatively we could fix them ASAP.

On Mon, Jan 18, 2016 at 1:57 AM, Glyph [email protected] wrote:

If code is currently calls these APIs and expects to get the security
properties they advertise, it is a security flaw to pretend that it worked.
Granted, none of these functions seem like an API which adds a restriction,
but on general principle, I think they should be made to raise errors and
do a release ASAP. It's compatibility-breaking, but in that sense it is a
compatibility-breaking security update.

—
Reply to this email directly or view it on GitHub
#410 (comment).

"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

[reaperhulk]

I'd suggest fixing everything but renegotiate, but raising notimplemented on all of them is definitely an improvement over silently doing nothing.

[mhils]

I updated the PR with tests for all four functions.

[hynek]

So um it seems to break a lot of tests in Twisted: https://travis-ci.org/pyca/pyopenssl/jobs/103128063 you really cool with that @glyph ? :)

[glyph]

So um it seems to break a lot of tests in Twisted: https://travis-ci.org/pyca/pyopenssl/jobs/103128063 you really cool with that @glyph ? :)

First of all, thanks to whoever set up CI to track that sort of thing. It's very useful to have this information.

Second, it appears that all of the errors are going through the same call to set_session_id.

I'm not crazy about breaking all these tests, but I'm not entirely sure that the call to set_session_id even makes sense. Good use of md5, of course. But also, does it even make sense to call set_session_id and never call SSL_CTX_set_sess_get_cb? The client can't resume a session, so it's a bit nonsense to enable sessions in the first place, as I understand it.

So, I don't know. It appears harmless, so maybe it's not worth breaking, but this code in Twisted probably does need to be rewritten. The question is, if set_session_id starts working, is this code a problem? Somebody who knows TLS a little better than me might have to answer.

[hynek]

*lightspot goes to @reaperhulk & @tiran *

[hynek]

(and as for who set it up: remember pyOpenSSL 0.15.0? :|)

I’d really like at least a smoke test for urllib3.

[tiran]

It's unclear to me what SSL.Context.set_session_id() is suppose to do. I guess it's meant to be used for https://www.openssl.org/docs/manmaster/ssl/SSL_CTX_set_session_id_context.html . However then the name isn't right. The context sid is not related to or to be confused with the session id in during the TLS/SSL handshake. It rather sets an identifier for the session context to ensure that sessions aren't shared between unrelated contexts. It's mostly useful for import/export of serialized sessions on the server side. It doesn't hurt to set it on the client side, though.

The session id context doesn't need to be secure or unpredictable. Apache uses the vhost name. CPython simply sets it to "Python".

[tiran]

By the way the settings are only relevant for server-side sessions. For client side an application has to set the session an application has to set an explicit session, e.g. with SSL_set_session(dst_ssl, SSL_get_session(src_ssl)). Both SSL* should refer to the same SSL_CTX*.

https://www.openssl.org/docs/manmaster/ssl/SSL_set_session.html

SSL_SESSION objects keep internal link information about the session cache list, when being inserted into one SSL_CTX object's session cache. One SSL_SESSION object, regardless of its reference count, must therefore only be used with one SSL_CTX object (and the SSL objects created from this SSL_CTX object).

[hynek]

FYI, this is what “old” pyOpenSSL did:

static char ssl_Context_set_session_id_doc[] = "\n\
Set the session identifier.  This is needed if you want to do session\n\
resumption.\n\
\n\
:param buf: A Python object that can be safely converted to a string\n\
:returns: None\n\
";
static PyObject *
ssl_Context_set_session_id(ssl_ContextObj *self, PyObject *args)
{
    unsigned char *buf;
    unsigned int len;

    if (!PyArg_ParseTuple(args, "s#:set_session_id", &buf, &len))
        return NULL;

    if (!SSL_CTX_set_session_id_context(self->ctx, buf, len))
    {
        exception_from_error_queue(ssl_Error);
        return NULL;
    }
    else
    {
        Py_INCREF(Py_None);
        return Py_None;
    }
}

If the name is wrong, we should implement and deprecate it at once I guess?

[hynek]

After some research it appears to me that the reason why they’re not implemented is that cryptography is missing bindings.

Unless we get a back port to older cryptography versions, we can’t implement them for now.

Would raising a warning be an adequate compromise?

[hynek]

@reaperhulk any particular reason why you’d implement everything but renegotiate? Turns out those two methods are the only that have the necessary bindings. :)

[glyph]

@hynek Yes, I think a warning would be the best option available immediately.

[reaperhulk]

Mostly because renegotiation is not a good feature in TLS and it's not common to actually need it. :)

[glyph]

Mostly because renegotiation is not a good feature in TLS and it's not common to actually need it. :)

The main people asking for it that I've seen are security researchers looking for new exploits in it :)

[hynek]

@mhils so…would you mind transforming those Exceptions into RuntimeWarnings?

I think Base class for warnings about dubious runtime behavior. hits the nail on the head…

[hynek]

Or please don't do anything until #419 is decided on. 😞

[hynek]

I’ve more or less decided to not care about Travis/PyPy anymore and redirect all the hate to them if necessary.

Unfortunately there’s no release with the bindings and 1.3 is weeks away.

@reaperhulk offered to release a cryptography 1.2 point release with the necessary bindings. I guess that would be a viable solution.

We should also figure out how to test on PyPy ourselves since Travis isn’t likely to show any progress anytime soon I’ve heard.

[glyph]

I’ve more or less decided to not care about Travis/PyPy anymore and redirect all the hate to them if necessary.

💯

[glyph]

We should also figure out how to test on PyPy ourselves since Travis isn’t likely to show any progress anytime soon I’ve heard.

Mimic's build configuration sets a reasonable example here.

[hynek]

There’s multiple examples but someone has to actually do it. :) And integrate with our already nontrivial setup. :(

[hynek]

Waiting for pyca/cryptography#2715 to decide what route to go.

[hynek]

We’re implementing them! #422 Sorry @mhils !