On 20 January 2017 at 04:29, Cory Benfield <cory@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@gmail.com | Brisbane, Australia