[Twisted-Python] The role of twisted.internet._sslverify.IOpenSSLTrustRoot
![](https://secure.gravatar.com/avatar/c194a4d2f2f8269aa052942e87985198.jpg?s=120&d=mm&r=g)
Hi, What is the purpose of IOpenSSLTrustRoot ? It it documented as a private interface, it has only private methods, but then it is exposed in twisted.internet.ssl.optionsForClientTLS Why? I am confused while trying to review https://twistedmatrix.com/trac/ticket/7671 Please take a look at my review and add your wisdom :) Thanks! -- Adi Roiban
![](https://secure.gravatar.com/avatar/e1554622707bedd9202884900430b838.jpg?s=120&d=mm&r=g)
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.
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 am confused while trying to review https://twistedmatrix.com/trac/ticket/7671 <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, -glyph
![](https://secure.gravatar.com/avatar/c194a4d2f2f8269aa052942e87985198.jpg?s=120&d=mm&r=g)
Hi, On 25 December 2015 at 11:20, Glyph Lefkowitz <glyph@twistedmatrix.com> wrote:
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
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
Many thanks for your comment. -- Adi Roiban
![](https://secure.gravatar.com/avatar/e1554622707bedd9202884900430b838.jpg?s=120&d=mm&r=g)
This is incorrect, though. The term "marker interface" means "interface with no attributes or methods, used to denote a complex property that Interface is insufficiently expressive to describe". Oh and hey we didn't even invent the term: https://en.wikipedia.org/wiki/Marker_interface_pattern :)
I think the documentation as it is does maintainers a slight disservice, as the assumption is that you've come to IOpenSSLTrustRoot after having read the docstring for optionsForClientTLS, or platformTrust, or OpenSSLCertificateOptions. So it could definitely be re-written to take into account the fact that a maintainer might discover this interface first. Honestly it seems to me that a "@see:" with links to the thing that take it as a parameter would get us 70% of the way there, just so that maintainers could use it as a jumping-off point. Some of the explanation from these emails would also be useful, of course, to give some context. It says "things outside of twisted shouldn't implement it" but it doesn't really explain why.
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.
We have definitely made some missteps with this API design (I should probably write a giant "mea culpa" blog post at some point pointing out what many of those errors were). However, the point of things like IOpenSSLTrustRoot is not to prematurely introduce extra interfaces and functionality where they are not needed - we need such an interface even for a pure OpenSSL-only backend. The point is to keep those things out of the public interface so that callers are not overly coupled to them. One thing that's important to remember is that "private" is a social convention in Python, so if needs must, a Twisted application can always just go ahead and implement this interface. -glyph
![](https://secure.gravatar.com/avatar/a93db92c60e9d5435d204407264457f2.jpg?s=120&d=mm&r=g)
Glyph Lefkowitz <glyph@twistedmatrix.com> writes: (Thanks for the review, Adi!)
On Dec 20, 2015, at 9:05 AM, Adi Roiban <adi@roiban.ro> wrote:
So in the context of #7671 and testing, can the tests for multiTrust() just check that they get an IOpenSSLTrustRoot implementation back or do ... something else? That is, currently I'm just confirming this via asserting IOpenSSLTrustRoot.providedBy(theReturnValue) or so in the tests. Of course, this means I'm importing the private interface into the tests... Thanks! and merry christmas, meejah
![](https://secure.gravatar.com/avatar/e1554622707bedd9202884900430b838.jpg?s=120&d=mm&r=g)
Twisted's own tests may always feel free to import private implementation details if they're useful for testing. However, ideally, the tests for multiTrust ought to pass along a fake OpenSSL context object and ensure the correct methods are called on it; you can do this without importing any private API, just public API from pyOpenSSL and public API from Twisted, but it does involve a bit more set-up. There are, however, lots of examples of doing that setup, particularly in test_sslverify (which already has a FakeContext). So if you can, please do it that way. Thanks! -glyph
![](https://secure.gravatar.com/avatar/e1554622707bedd9202884900430b838.jpg?s=120&d=mm&r=g)
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.
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 am confused while trying to review https://twistedmatrix.com/trac/ticket/7671 <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, -glyph
![](https://secure.gravatar.com/avatar/c194a4d2f2f8269aa052942e87985198.jpg?s=120&d=mm&r=g)
Hi, On 25 December 2015 at 11:20, Glyph Lefkowitz <glyph@twistedmatrix.com> wrote:
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
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
Many thanks for your comment. -- Adi Roiban
![](https://secure.gravatar.com/avatar/e1554622707bedd9202884900430b838.jpg?s=120&d=mm&r=g)
This is incorrect, though. The term "marker interface" means "interface with no attributes or methods, used to denote a complex property that Interface is insufficiently expressive to describe". Oh and hey we didn't even invent the term: https://en.wikipedia.org/wiki/Marker_interface_pattern :)
I think the documentation as it is does maintainers a slight disservice, as the assumption is that you've come to IOpenSSLTrustRoot after having read the docstring for optionsForClientTLS, or platformTrust, or OpenSSLCertificateOptions. So it could definitely be re-written to take into account the fact that a maintainer might discover this interface first. Honestly it seems to me that a "@see:" with links to the thing that take it as a parameter would get us 70% of the way there, just so that maintainers could use it as a jumping-off point. Some of the explanation from these emails would also be useful, of course, to give some context. It says "things outside of twisted shouldn't implement it" but it doesn't really explain why.
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.
We have definitely made some missteps with this API design (I should probably write a giant "mea culpa" blog post at some point pointing out what many of those errors were). However, the point of things like IOpenSSLTrustRoot is not to prematurely introduce extra interfaces and functionality where they are not needed - we need such an interface even for a pure OpenSSL-only backend. The point is to keep those things out of the public interface so that callers are not overly coupled to them. One thing that's important to remember is that "private" is a social convention in Python, so if needs must, a Twisted application can always just go ahead and implement this interface. -glyph
![](https://secure.gravatar.com/avatar/a93db92c60e9d5435d204407264457f2.jpg?s=120&d=mm&r=g)
Glyph Lefkowitz <glyph@twistedmatrix.com> writes: (Thanks for the review, Adi!)
On Dec 20, 2015, at 9:05 AM, Adi Roiban <adi@roiban.ro> wrote:
So in the context of #7671 and testing, can the tests for multiTrust() just check that they get an IOpenSSLTrustRoot implementation back or do ... something else? That is, currently I'm just confirming this via asserting IOpenSSLTrustRoot.providedBy(theReturnValue) or so in the tests. Of course, this means I'm importing the private interface into the tests... Thanks! and merry christmas, meejah
![](https://secure.gravatar.com/avatar/e1554622707bedd9202884900430b838.jpg?s=120&d=mm&r=g)
Twisted's own tests may always feel free to import private implementation details if they're useful for testing. However, ideally, the tests for multiTrust ought to pass along a fake OpenSSL context object and ensure the correct methods are called on it; you can do this without importing any private API, just public API from pyOpenSSL and public API from Twisted, but it does involve a bit more set-up. There are, however, lots of examples of doing that setup, particularly in test_sslverify (which already has a FakeContext). So if you can, please do it that way. Thanks! -glyph
participants (3)
-
Adi Roiban
-
Glyph Lefkowitz
-
meejah