Hi, On 25 December 2015 at 11:20, Glyph Lefkowitz <[email protected]> wrote:
> > On Dec 20, 2015, at 9:05 AM, Adi Roiban <[email protected]> wrote: > > Hi, > > What is the purpose of IOpenSSLTrustRoot ? > > > The idea is that we have public functions, mainly `optionsForClientTLS´, > which need to take a thing that represents a "trust root". We want this to > be something that can abstractly be described at a high level, but then in > reality we need to do with gross implementation details of OpenSSL. So > this interface describes what you pass. > Reading the docstring of IOpenSSLTrustRoot it does not give any hints about optionsForClientTLS So instead of Trust settings for an OpenSSL context. Maybe it should be something like: Marker only interface for private implementations of OpenSSL trust root things. It it documented as a private interface, it has only private methods, but > then it is exposed in twisted.internet.ssl.optionsForClientTLS > > Why? > > > Yes, this is intentional. It is a private interface, so you can't check > if something provides it, you aren't allowed to know what attributes it > has, and you can't implement it. However, you *can* call a function that > is documented to return a value that provides it (such as > `twisted.internet.ssl.platformTrust´) and pass that value to a function > documented to accept it (such as > `twisted.internet.ssl.optionsForClientTLS`). > > It's private because we weren't sure if we'd want to change it. At the > time it was implemented, the only two cases were OpenSSLDefaultPaths and > Certificate. In the case of Certificate, you know what certificate you're > adding, but in the case of OpenSSLDefaultPaths, you just call a method on > the context object to mutate it, and you can't extract information about > which certificates are trusted past that. The method we came up > with, _addCACertsToContext, was a gross compromise which allowed for > implementing this but could not be made abstract, because it reflects a > bizarre flaw in the OpenSSL API, and it by necessity exposes pyOpenSSL > objects, which we are trying to do less of. For one thing, we'd eventually > like to support TLS via OpenSSL using an API provided by Cryptography; for > another, we'd like to one day provide TLS from an API that might not be > backed by OpenSSL at all. So reducing the surface area of our public API > that touches pyOpenSSL is important. > > Hopefully this thoroughly explains the decision? > I understand now, but I find it hard to extract this information just from the code... maybe I am a bad code reader, or maybe the IOpenSSLTrustRoot docstring should inform that it is a gross compromise so that other people will understand that this is not really intentional ... and invited to find something better :) I am happy to see more backend neutral API but unless we have at least another use case for non OpenSSL backed TLS I don't know if it worth designing an API for that unknown API. I am confused while trying to review > https://twistedmatrix.com/trac/ticket/7671 > > Please take a look at my review and add your wisdom :) > > > This is quite a detailed review :-). > > Merry christmas, > > Many thanks for your comment. -- Adi Roiban
_______________________________________________ Twisted-Python mailing list [email protected] http://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-python
