On May 29, 2020, at 11:16 PM, Wim Lewis <wiml@hhhh.org> wrote:
I'm looking at a fix for bug <https://twistedmatrix.com/trac/ticket/9804> (Cannot load a PEM certificate with Unicode in subject). The underlying problem is that the DistinguishedName class can't handle non-ascii AVAs. The fix I've made simply avoids creating DistinguishedName instances when it isn't necessary, but that leaves the question of what to do with the class. I think that the best thing to do is to deprecate the class entirely and replace it with simpler API.
Thanks for checking in about this!
Reasons I think that the DN class is broken: - The values in a certificate are conceptually text-strings, not byte strings; they may be in ASCII, UTF8, UTF16, or several other encodings. However - DN represents these textual values as `bytes` instead of `str` - DN can't handle non-ASCII-representable values at all, even if the user never tries to access that value - It can only handle a subset of the attribute-assertions found in a PKIX DN; there's no escape hatch for others (e.g. OID keys or whatever) - It can't represent the full structure of a DN (specific ordering, multiple-value RDNs, AVAs whos values aren't textual, etc.) --- these are not common in the PKIX world but they are valid
Ugh. I wrote this class where I knew literally nothing about TLS, and really just wanted it to be the thing it effectively is to the user (i.e. "connect to a hostname") rather than the thing it actually is (a tarpit of unnecessarily complex asn.1 nonsense). Additionally, this was when pyOpenSSL was unmaintained and buggy as hell and X509Name would segfault when you so much as sneezed at it right.
What I propose as an alternative: - Replace APIs that take `DistinguishedName` classes with ones that take `Union[OpenSSL.crypto.X509Name, dict]` where the `dict` format is parsed with the same convenience semantics as DistinguishedName, except that values are `str` - Replace APIs that return `DistinguishedName` with ones that return OpenSSL.crypto.X509Name, which is already fairly convenient to use (e.g. it has attributes for retrieving/setting commonName and so on without dealing with the full complexity of X.500 names) - Deprecate `DistinguishedName` and the APIs that use it for eventual removal - Expose a convenience function for the dict -> X509Name transform
Any objections? Thoughts on how I should go about doing this? Should I do it as part of this Trac ticket or split it out?
The only downside I can think of is that this exposes the OpenSSL.crypto.X509Name type as part of Twisted's API. I don't think this is a huge reduction in flexibility --- Twisted's API already somewhat assumes that TLS is implemented using OpenSSL, and only users whose needs are *already* not well met by DistinguishedName will care if that `Union` type changes in the future.
We've been slowly trying to paper over the aspects of the API that expose OpenSSL details and provide a more complete abstraction for years. We are not there yet, as you observe! We do still somewhat assume TLS uses OpenSSL. But I really want to get away from that; I want to be able to use SChannel on Windows and Network.framework on macOS and provide a nice abstraction that floats above that. This means - particularly in the latter case - not just eliminating the OpenSSL dependency, but actually adding new APIs that allow some pluggability on the reactor itself to allow for combining higher-level protocols together in the reactor itself. (Maybe even HTTP, given the way that NSURLSession seems to want to provide HTTP3 as a platform service directly, skipping sockets and indeed TCP and TLS entirely...) In a nearer-term, more practical way that we might leverage this soon, is a "lite" TLS provider that uses the stdlib's SSLSocket rather than all of pyOpenSSL; this would substantially reduce the dependency weight of Twisted on embedded platforms (for example, you can get the built-in SSL module in a context like https://omz-software.com/pythonista/ <https://omz-software.com/pythonista/> but there's no way to get cryptography & pyOpenSSL at all). One success story on this front is optionsForClientTLS(). Hopefully one day in the not too distant future we can get an optionsForServerTLS and slowly hide CertificateOptions underneath it. So I would see this refactoring as an opportunity to move further away from pyOpenSSL dependency rather than further towards it. I'm all for deprecating "DN", it's a terrible wrapper, but I think we could have a better wrapper. That said; if abstracting this stuff away is really challenging, and you already have a fix close to ready to go, we can always evolve the API again when we do this for real, and have an actual second backend to prove the interface against. -glyph