On 20 January 2017 at 04:29, Cory Benfield <c...@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 | ncogh...@gmail.com | Brisbane, Australia
_______________________________________________
Security-SIG mailing list
Security-SIG@python.org
https://mail.python.org/mailman/listinfo/security-sig
