enableSessions parameter to CertificateOptions does something silly, not what its docs say

[twisted-trac]

	@glyph reported
Trac ID	trac#9583
Type	defect
Created	2019-02-14 05:42:05Z
Branch	https://github.com/twisted/twisted/tree/9583-session-silliness

The documentation explains this field thusly:

@param enableSessions: If True, set a session ID on each context.  This
    allows a shortened handshake to be used when a known client
    reconnects.

However, that is not at all what the implementation of this flag does.

This is a propagation of the unfortunately-named and also-incorrectly-documented OpenSSL.SSL.Context.set_session_id, which is in fact a weird cache-control tuning gizmo in OpenSSL. set_session_id is an incorrect transcription of the name of the actual function being wrapped, SSL_CTX_set_session_id_context.

It is true that failing to set this value will effectively disable sessions when client certificates are in use, but that is a bit of an edge case.

The language in the OpenSSL documentation strongly implies that this value should be a stable identifier; specifically "e.g. the name of the application and/or the hostname and/or service name". However, we are instead setting a random identifier.

The attempt to use the full size of SSL_MAX_SSL_SESSION_ID_LENGTH is also a bit silly / pointless. (NB: this data does not in fact go into the session ID, that just happens to be a common limit on both the session ID and this weird value.)

(The docs here also sort of imply that this has something to do with servers, but doesn't clearly stipulate that it is meaningless on client contexts.)

---

To properly enable (or disable) TLS sessions, in a way that would work across more than a single TLS-terminating node in a cluster, one needs to use the various session storage callbacks which are not yet even exposed by pyOpenSSL.

However, pyOpenSSL does offer us Context.set_session_cache_mode, which could at least be used to honor the "disable" part of this flag's documentation.

Searchable metadata

trac-id__9583 9583
type__defect defect
reporter__glyph glyph
priority__normal normal
milestone__None None
branch__9583_session_silliness 9583-session-silliness
branch_author__ 
status__closed closed
resolution__fixed fixed
component__core core
keywords__None None
time__1550122925405989 1550122925405989
changetime__1584264169980791 1584264169980791
version__None None
owner__glyph glyph

[twisted-trac]

	@glyph commented

Filed the problem with set_session_id's name upstream: pyca/pyopenssl#845

[twisted-trac]

	@glyph commented

I've included a workaround (and some hypothetical mitigations) for #9764 as well since we are probably not going to be able to fully diagnose that on our own, unfortunately.

[twisted-trac]

	@glyph commented

Code is at #1224

[twisted-trac]

	@twm set owner to @twm @twm set status to assigned

[twisted-trac]

	@twm set owner to @glyph @twm set status to new

LGTM: #1224 (review)

[twisted-trac]

	@glyph set status to closed

In changeset eb3248b

#!CommitTicketReference repository="" revision="eb3248b197f2bd2d4091a73bbaf93174d8da19b5"
Merge pull request #1224 from twisted/9583-session-silliness

Author: glyph

Reviewer: twm

Fixes: ticket:9583

Make the `enableSessions` argument to CertificateOptions actually enable and disable session support in TLS.