[Security-sig] Unified TLS API for Python: Round 2

[Nick Coghlan]

On 20 January 2017 at 04:29, Cory Benfield <cory at lukasa.co.uk> wrote:
> Please let me know what you think.
>
> The version of the draft PEP, from commit ce74bc60, is reproduced below.

Thanks Cory, this is looking really good. I don't have anything to add
on the security design front, but do have a few comments/questions on
the API design and explanation front.

> Configuration
> ~~~~~~~~~~~~~
>
> The ``TLSConfiguration`` concrete class defines an object that can hold and
> manage TLS configuration. The goals of this class are as follows:
>
> 1. To provide a method of specifying TLS configuration that avoids the risk of
>    errors in typing (this excludes the use of a simple dictionary).
> 2. To provide an object that can be safely compared to other configuration
>    objects to detect changes in TLS configuration, for use with the SNI
>    callback.
>
> This class is not an ABC, primarily because it is not expected to have
> implementation-specific behaviour. The responsibility for transforming a
> ``TLSConfiguration`` object into a useful set of configuration for a given TLS
> implementation belongs to the Context objects discussed below.
>
> This class has one other notable property: it is immutable. This is a desirable
> trait for a few reasons. The most important one is that it allows these objects
> to be used as dictionary keys, which is potentially extremely valuable for
> certain TLS backends and their SNI configuration. On top of this, it frees
> implementations from needing to worry about their configuration objects being
> changed under their feet, which allows them to avoid needing to carefully
> synchronize changes between their concrete data structures and the
> configuration object.
>
> The ``TLSConfiguration`` object would be defined by the following code:
>
>     ServerNameCallback = Callable[[TLSBufferObject, Optional[str], TLSConfiguration], Any]
>
>
>     _configuration_fields = [
>         'validate_certificates',
>         'certificate_chain',
>         'ciphers',
>         'inner_protocols',
>         'lowest_supported_version',
>         'highest_supported_version',
>         'trust_store',
>         'sni_callback',
>     ]
>
>
>     _DEFAULT_VALUE = object()
>
>
>     class TLSConfiguration(namedtuple('TLSConfiguration', _configuration_fields)):

I agree with Wes that the backwards compatibility guarantees around
adding new configuration fields should be clarified.

I think it will suffice to say that "new fields are only appended,
existing fields are never removed, renamed, or reordered". That means
that:

- tuple unpacking will be forward compatible as long as you use *args at the end
- numeric lookup will be forward compatible

That doesn't make either of them a good idea (vs just using attribute
lookups), but it does provide an indication to future maintainers that
such code shouldn't be gratuitously broken either.

> Context
> ~~~~~~~
>
> The ``Context`` abstract base class defines an object that allows configuration
> of TLS. It can be thought of as a factory for ``TLSWrappedSocket`` and
> ``TLSWrappedBuffer`` objects.
>
> As much as possible implementers should aim to make these classes immutable:
> that is, they should prefer not to allow users to mutate their internal state
> directly, instead preferring to create new contexts from new TLSConfiguration
> objects. Obviously, the ABCs cannot enforce this constraint, and so they do not
> attempt to.
>
> The ``Context`` abstract base class has the following class definition::

This intro section talks about a combined "Context" objection, but the
implementation has been split into ServerContext and ClientContext.

That split could also use some explanation in the background section of the PEP.

> Proposed Interface
> ^^^^^^^^^^^^^^^^^^
>
> The proposed interface for the new module is influenced by the combined set of
> limitations of the above implementations. Specifically, as every implementation
> *except* OpenSSL requires that each individual cipher be provided, there is no
> option but to provide that lowest-common denominator approach.

The second sentence here doesn't match the description of SChannel
cipher configuration, so I'm not clear on how the proposed interface
would map to an SChannel backend.

> Errors
> ~~~~~~
>
> This module would define three base classes for use with error handling. Unlike
> the other classes defined here, these classes are not *abstract*, as they have
> no behaviour. They exist simply to signal certain common behaviours. Backends
> should subclass these exceptions in their own packages, but needn't define any
> behaviour for them.
>
> In general, concrete implementations should subclass these exceptions rather
> than throw them directly. This makes it moderately easier to determine which
> concrete TLS implementation is in use during debugging of unexpected errors.
> However, this is not mandatory.

This is the one part of the PEP that I think may need to discuss
transition strategies for libraries and frameworks that currently let
ssl module exceptions escape to their users: how do they do that in a
way that's transparent to API consumers that currently capture the ssl
module exceptions?

Cheers,
Nick.



-- 
Nick Coghlan   |   ncoghlan at gmail.com   |   Brisbane, Australia