Opened 3 years ago

Closed 8 months ago

Last modified 8 months ago

#5446 enhancement closed fixed (fixed)

cross-platform API for enumerating X509 certificates trusted by the platform for transport layer security

Reported by: glyph Owned by: glyph
Priority: normal Milestone:
Component: core Keywords:
Cc: ivank-twisted-bugs@…, tom.most@…, hs@… Branch: branches/trusted-ca-openssl-defaults-5446-2
(diff, github, buildbot, log)
Author: itamarst, rwall, glyph Launchpad Bug:

Description (last modified by glyph)

One component of #5445 (as originally discussed on #4023) would be an API for extracting the native trust roots from the platform. This is actually at least 3 tasks: one for Windows, one for Mac OS X, and at least one for Linux and BSD derivatives (although the only mechanism I'm familiar with there is the ca-certificates package in Debian, so perhaps there are other mechanisms we'd need to use as well).

I think that there's a way to discover the '/etc/ssl/certs' path (the one ca-certificates installs) via some API in OpenSSL, and if there is, we should use it, so that it will work with an arbitrary distro rather than being hard-coded to where Debian decided to stick it.

On Windows - and this is purely from a quick glance at the reference documentation, so take it with a grain of salt - I believe the right way to do this is to use CertOpenSystemStore with the string "CA", or possibly "ROOT", or maybe both, and then do CertEnumCertificatesInStore or maybe just PFXExportCertStoreEx to dump the certs into a format we can import into OpenSSL.

On OS X, and again, I haven't done this, I believe you just have to call SSLCopyTrustedRoots to get the default trusted SSL CA certificates and then SecCertificateCopyData on the retrieved roots to turn them into DER (which we can then load into any SSL implementation).

Change History (92)

comment:1 Changed 3 years ago by ivank

  • Cc ivank-twisted-bugs@… added

comment:2 follow-up: Changed 3 years ago by exarkun

What does "inspect" mean in this context?

comment:3 Changed 20 months ago by glyph

  • Description modified (diff)
  • Summary changed from cross-platform API for inspecting configured trust root to cross-platform API for enumerating X509 certificates trusted by the platform for transport layer security

comment:4 in reply to: ↑ 2 Changed 20 months ago by glyph

Replying to exarkun:

What does "inspect" mean in this context?

I changed it to "enumerate" since by "inspect" I just meant "present in a format that could be imported into an arbitrary TLS implementation so that we can do this before we figure out how to abstract away OpenSSL".

comment:5 follow-up: Changed 20 months ago by exarkun

I think that there's a way to discover the '/etc/ssl/certs' path (the one ca-certificates installs) via some API in OpenSSL, and if there is, we should use it, so that it will work with an arbitrary distro rather than being hard-coded to where Debian decided to stick it.

The way to discover this is probably via the OPENSSLDIR macro exposed by <openssl/opensslconf.h>.

comment:6 in reply to: ↑ 5 ; follow-up: Changed 20 months ago by glyph

Replying to exarkun:

The way to discover this is probably via the OPENSSLDIR macro exposed by <openssl/opensslconf.h>.

Thanks for the pointer. Is this macro exposed by PyOpenSSL?

comment:7 in reply to: ↑ 6 ; follow-up: Changed 20 months ago by exarkun

Replying to glyph:

Replying to exarkun:

The way to discover this is probably via the OPENSSLDIR macro exposed by <openssl/opensslconf.h>.

Thanks for the pointer. Is this macro exposed by PyOpenSSL?

Not by any existing version (<=0.13), no.

comment:8 in reply to: ↑ 7 Changed 20 months ago by glyph

Replying to exarkun:

Not by any existing version (<=0.13), no.

Do you believe that it would be worth opening a pyopenssl ticket to expose it?

comment:9 Changed 20 months ago by exarkun

Do you believe that it would be worth opening a pyopenssl ticket to expose it?

It seems like a useful thing to expose, and a ticket would, the very least, provide a place to discuss why it's actually the wrong idea. So, sure.

comment:11 Changed 20 months ago by itamar

Do we actually need to expose OPENSSLDIR if we can just use Context.set_default_verify_paths? I guess it'll let us check if it's actually set to something sane, rather than my current default of "always use it on Linux".

comment:12 follow-up: Changed 20 months ago by itamar

I'm going to move code out of #4888 branch into here, and then limit its scope to platforms that support Context.set_default_verify_paths; I'm going to assume any reasonable Linux distribution will support this. Separate tickets will be opened for OS X and Windows.

comment:13 Changed 20 months ago by itamarst

  • Author set to itamarst
  • Branch set to branches/trusted-ca-linux-5446

(In [37516]) Branching to 'trusted-ca-linux-5446'

comment:14 Changed 20 months ago by itamar

I opened #6371 and #6372 for Windows and OS X native support. #6334 is the ticket for bundling our own file with CAs certificates.

comment:15 Changed 20 months ago by itamar

I'm going to skip FreeBSD, and do it in separate ticket too (same code as Linux, just need FreeBSD-detection).

comment:16 Changed 20 months ago by itamar

  • Keywords review added

http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/trusted-ca-linux-5446 should be running.

I'm not writing docs until the full set of tickets is done, I think.

comment:17 in reply to: ↑ 12 Changed 20 months ago by glyph

Replying to itamar:

I'm going to move code out of #4888 branch into here, and then limit its scope to platforms that support Context.set_default_verify_paths; I'm going to assume any reasonable Linux distribution will support this.

[citation needed].

It is not a good idea to make security-critical assumptions about "common" distribution configurations. There's basically no way to figure out if set_default_verify_paths worked.

Granted, the failure mode is just that everything will fail to verify, right? So I guess it'll just be broken, not insecure.

comment:18 Changed 20 months ago by itamar

The included unit tests verify that certificates signed with a newly generated CA are not validated, so "broken rather than insecure" is what you'll get, yes.

Once OPENSSLDIR is exposed, we can also verify the file is there.

comment:19 Changed 20 months ago by twm

  • Cc tom.most@… added

comment:20 Changed 20 months ago by exarkun

  • Owner set to exarkun
  • Status changed from new to assigned

comment:21 follow-up: Changed 20 months ago by exarkun

  • Keywords review removed
  • Owner changed from exarkun to itamar
  • Status changed from assigned to new
  1. in the tests:
    1. It is probably obvious, but the approach taken by test_caCertsPlatformRejectsRandomCA is not the ideal approach. Since pyOpenSSL doesn't expose a non-Connection oriented way to invoke the verification logic, I can see that it is hard to exercise this code in any other way. However... isn't this just a test for Context.set_default_verify_paths? In other words, isn't this a test for a pyOpenSSL API which already has unit tests in pyOpenSSL?
    2. A corollary (or something) to the above may be that test_caCertsPlatformLinux should be trying a little harder to verify that it is faking Context correctly. If we were confident we were calling set_default_verify_paths correctly, and pyOpenSSL is confident that method works, then we don't need the integration-style test. Right?
    3. Please re-use some existing keys in the unit tests. Key generation isn't fast and doesn't need to happen each time the tests run.
    4. You could use dumpPEM instead of dump(..., FILETYPE_PEM)
    5. You could run all of the platform variation tests all of the time by parameterizing the platform object.
    6. test_caCertsPlatformRejectsRandomCA introduces some use of fail* methods. If somehow this test method doesn't end up going away, these should be assert* instead. fail* methods are going away.
    7. And use open instead of file :/
    8. Making _contextFactory an attribute of CertificateOptions seems nicer than making it a new "private" parameter of getContext (since positional parameters can't actually be private).
    9. Code should be explicit with bytes literals vs unicode literals
  2. in the implementation:
    1. Adding PLATFORM as a possible value of caCerts precludes the possibility of using the platform certificates plus any custom certificates. It also doesn't help the chances of PrivateCertificate.options being usable with this option (I realize it is not now usable with it, but according to some stated goals for the API, this would be the preferred way to get a CertificateOptions - whether that is believable or not perhaps bears discussion, though). Anyhow, it also makes the docs confusing, because this sentence reads poorly: C{list} of L{OpenSSL.crypto.X509}, or L{CASources} constants.. Can it be a list containing CASources - no, it can't, but I expect it will take some people some time to figure this out.
    2. _makeContext is missing a docstring
    3. Do we have precedent for putting ticket numbers in exception messages? In comments in the source I can understand, but showing this to end users who misconfigure their SSL server seems like it could be another point of confusion.
    4. I'll try to bikeshed on the name CASources only this one time: these aren't really sources of CAs ("certificate authorities"). Perhaps they're sources of certificate authority certificates. So "CACertificateSources" would be a bit better. Of course, they're not necessarily actually event certificates from widely recognized certificate authorities. You may choose to configure your system with your own custom set of certificates. What this option really specifies is something to do with trust roots, I think, in the form of certificates. So perhaps a name involving either "trust" or "root" would be nice. Good luck inventing something elegant and intuitive with that.
  3. Some narrative docs need to be updated to reveal this information to people - presumably the (already somewhat low-level) ssl howto would be a good place. Also, pointing out the pitfalls of the current implementation seems like a must. This never works except on Linux (and fails by preventing you from setting up an SSL connection), and on Linux it works only if you're using a build of OpenSSL which was properly configured at build time. And figuring out which of these situations you're in on a particular machine may not be easy for some people. Let's be proactive and try to keep people from digging themselves into too deep a hole.

Thanks!

comment:22 Changed 16 months ago by rwall

comment:23 Changed 16 months ago by exarkun

Fortunately it looks like they're doing this right, so we shouldn't need to account for Fedora in particular. Quoting:

  • These extracted certificates will be placed in a location so that they can be consumed by OpenSSL by default.
  • The aim is that neither OpenSSL nor OpenSSL applications will have to be changed for this to work.

comment:24 Changed 14 months ago by rwall

(In [40003]) An example of certificate verification using platform CAs and some accompanying howto documentation. refs #5446

comment:25 in reply to: ↑ 21 ; follow-up: Changed 14 months ago by rwall

Replying to exarkun:

  1. Adding PLATFORM as a possible value of caCerts precludes the possibility of using the platform certificates plus any custom certificates. It also doesn't help the chances of PrivateCertificate.options being usable with this option (I realize it is not now usable with it, but according to some stated goals for the API, this would be the preferred way to get a CertificateOptions - whether that is believable or not perhaps bears discussion, though). Anyhow, it also makes the docs confusing, because this sentence reads poorly: C{list} of L{OpenSSL.crypto.X509}, or L{CASources} constants.. Can it be a list containing CASources - no, it can't, but I expect it will take some people some time to figure this out.

I encountered this while writing an example script.

I'd like to have made PLATFORM a flag and then allow the user to provide more trusted certificate authorities.

Isn't it possible to call ctx.set_default_verify_paths() then call get_cert_store and append more certificates?

  1. Some narrative docs need to be updated to reveal this information to people - presumably the (already somewhat low-level) ssl howto would be a good place. Also, pointing out the pitfalls of the current implementation seems like a must. This never works except on Linux (and fails by preventing you from setting up an SSL connection), and on Linux it works only if you're using a build of OpenSSL which was properly configured at build time. And figuring out which of these situations you're in on a particular machine may not be easy for some people. Let's be proactive and try to keep people from digging themselves into too deep a hole.

I wrote an example script and some new paragraphs to go with this branch.

I've tried to highlight the caveats above.

Not sure whether to also mention the lack of hostname verification -- since that will presumably land shortly after this branch.

I also thought it was strange that the deferred returned by SSL4ClientEndpoint.connect seems to fire before the SSL handshake has completed.

So I get the protocol but can't read any of the SSL certificate information from the transport.

I've put in a hacky 0.5s delay...is there a better way? Or is this a bug in the SSL wrapping classes?

The branch needs merging forward. I'd do it myself, but it seems like a non-trivial conflict -- which Itamar probably needs to look at.

comment:26 Changed 14 months ago by rwall

  • Author changed from itamarst to itamarst, rwall
  • Branch changed from branches/trusted-ca-linux-5446 to branches/trusted-ca-linux-5446-2

(In [40005]) Branching to 'trusted-ca-linux-5446-2'

comment:27 in reply to: ↑ 25 ; follow-up: Changed 14 months ago by glyph

Replying to rwall:

Replying to exarkun:
I encountered this while writing an example script.

I'd like to have made PLATFORM a flag and then allow the user to provide more trusted certificate authorities.

Let's please not have flags. Platform should be a - inferring from previous comments - CASource, that can be combined with other CASources.

Isn't it possible to call ctx.set_default_verify_paths() then call get_cert_store and append more certificates?

It seems like the PLATFORM CASource could just call this method.

Not sure whether to also mention the lack of hostname verification -- since that will presumably land shortly after this branch.

Just link to the ticket? We should probably add a thing about grepping for ticket links when we are looking to close a ticket so we can update any documentation that mentions a caveat.

I also thought it was strange that the deferred returned by SSL4ClientEndpoint.connect seems to fire before the SSL handshake has completed.

There's no callback for SSL handshake completion. There should be. I believe there's a ticket for that, but in a bit of a hurry at the moment and I can't find it quickly.

I've put in a hacky 0.5s delay...is there a better way? Or is this a bug in the SSL wrapping classes?

It's a bug in the SSL endpoint classes.

The branch needs merging forward. I'd do it myself, but it seems like a non-trivial conflict -- which Itamar probably needs to look at.

Thanks again for doing this.

comment:28 Changed 13 months ago by rwall

Currently blocked on #6570 -- port t.p.constants to Python3

comment:29 Changed 13 months ago by glyph

Hooray, un-blocked! Thanks for doing that port, rwall.

comment:30 Changed 12 months ago by hynek

  • Cc hs@… added

comment:31 in reply to: ↑ 27 Changed 12 months ago by rwall

Replying to glyph:

Replying to rwall:

Not sure whether to also mention the lack of hostname verification -- since that will presumably land shortly after this branch.

Just link to the ticket? We should probably add a thing about grepping for ticket links when we are looking to close a ticket so we can update any documentation that mentions a caveat.

#4888 is about Hostname verification for web.client.Agent, but I guess there should be a more general TLSHostnameCheckingEnpoint or somthing? Can't find a ticket for that.

comment:32 Changed 10 months ago by itamar

What is the status of this branch? What is it blocked on?

comment:33 Changed 9 months ago by hynek

I’d like to sum up my take-aways from a few discussion on IRC:

  • we should pace up, the current state of client-side TLS in Twisted is rather embarrassing,
  • we should definitely fix this, and don’t spend time on #6334,
  • the solution should be pluggable to allow for e.g. using Mozilla’s trust store.

Personally, I would prefer to abstract it away into an ITrustStore that offers an attribute ITrustStore.rootCAs (or whatever) that can be passed into as CertificateOptions(…, caCerts=myTrustStore.rootCAs, …).

Obvious implementations would be:

That approach would be open for expansion (even through endusers) and wouldn’t meddle with CertificateOptions’s API any more.

Let me know what you think, I would put it on my TLS agenda just after DHE string endpoints.

comment:34 Changed 9 months ago by itamar

On mainstream Linux distros OpenSSL is configured to use a CA database maintained by the distribution. I believe one of the branches implements this already. We should use that when available.

comment:35 Changed 9 months ago by hynek

I’ve read the code and indeed, it uses an OpenSSL API to use the system store, therefore obviously my original idea won’t fly.

I still maintain that it should be pluggable and outside of CertificateOptions though.

So how about keeping the idea of ITrustStore objects but with an callback ITrustStore.addCAcertsToCertStore(context) instead and expand caCerts to accept an ITrustStore too?

The current code could be easily transformed into that; the interesting parts are all there already.

comment:36 follow-up: Changed 9 months ago by exarkun

Defining ITrustStore with a method like addCAcertsToCertStore(context) runs into the same problems we discussed regarding IDHParams (context has no well-defined interface, it is specific to pyOpenSSL, therefore ITrustStore becomes specific to pyOpenSSL if defined this way, therefore why bother having an interface at all).

I still maintain that it should be pluggable and outside of CertificateOptions though.

Does this mean you don't like the idea of ITrustStore.caCerts and leaving the "use the platform certificates" part of the functionality separate from ITrustStore? That is, do you dislike this API:

# Use my personal root CAs
CertificateOptions(caCerts=CertDirectoryTrustStore(home.child(b".certs")))
# Use the system's root CAs
CertificateOptions(caCerts=MAGIC_THING_SYSTEM_ROOT_CAS)

where MAGIC_THING_SYSTEM_ROOT_CAS and CertDirectoryTrustStore are not the same kind of thing in any way.

comment:37 Changed 9 months ago by hynek

Yes, I recalled that discussion shortly after posting. :(

I rather dislike it (three possible types for caCerts…) but I would go along to end this here.

In an ideal world, None would have meant system trust db and we wouldn’t have this argument. *sigh*

comment:38 in reply to: ↑ 36 Changed 9 months ago by glyph

Replying to exarkun:

Defining ITrustStore with a method like addCAcertsToCertStore(context) runs into the same problems we discussed regarding IDHParams (context has no well-defined interface, it is specific to pyOpenSSL, therefore ITrustStore becomes specific to pyOpenSSL if defined this way, therefore why bother having an interface at all).

We want to have an interface for the same reason one always wants an interface - because we have multiple implementations :-).

There's a similar issue, hypothetically, with SecureTransport; if one is using SecureTransport to do TLS, then of course, SecureTransport will use its own already-trusted list of system CAs. I still hope one day that we might get there (although it seems increasingly likely to me that we will end up moving these abstractions to cryptography.io eventually).

In the meanwhile, though:

ITrustStore should be a general interface that creates some Python objects that contain PEMs or ASN1 blobs or something, so if we're getting our trust store from a different place than we're getting our TLS library, we can do something sensible with it. No dependency on OpenSSL at all.

However, CertificateOptions should take something that's adaptable to a "backend-specific" (read: OpenSSL right now) interface. So the current OpenSSL-backed CertificateOptions should take something that's adaptable to IOpenSSLCertificateTrustAdderThing. Maybe with a few more underscores than that; the point is, that interface is ugly, and explicitly depends on 'context'.

Now, normally I hate "something adaptable to" as an interface type; however, we can stipulate that ITrustStore always has a global adapter to every back-end interface (i.e. the definition of such a "back end interface" is something that's adaptable from ITrustStore).

Every trust-discovery backend can then return an ITrustStore for enumerating its certificates in a portable way - for export to a foreign TLS implementation, or even just for debugging / diagnostics / reporting.

But, trust-discovery backends may also, optionally, provide a backend-specific interface (IOpenSSLCertificateTrustAdderThing, ISecureTransportTrustThing) which do something on the order of addCAcertsToSpecificCertStoreType(backendSpecificContext, caCerts). In the case of "system default trust root", these methods can just call one method or maybe even be a no-op if the default behavior of that backend is just to use the system default trust root.

Somehow this feels horribly over-engineered to me, especially given that we only have one backend right now. But I don't have a better idea. The interfaces decompose relatively cleanly, I don't think it will be a ton of extra work to implement, and we need to start weaning ourselves off of OpenSSL eventually if we want to have some level of implementation agility.

comment:39 follow-ups: Changed 9 months ago by exarkun

ITrustStore should be a general interface that creates some Python objects that contain PEMs or ASN1 blobs or something, so if we're getting our trust store from a different place than we're getting our TLS library, we can do something sensible with it. No dependency on OpenSSL at all.

Interfaces don't create things. I suppose you mean Twisted should come with an implementation of the interface that does these things. Let's ignore the question of how it does that (perhaps it includes its own x509 parser, who cares).

Great. Now we have a bag of certificates.

However, CertificateOptions should take something that's adaptable to a "backend-specific" (read: OpenSSL right now) interface. So the current OpenSSL-backed CertificateOptions should take something that's adaptable to IOpenSSLCertificateTrustAdderThing. Maybe with a few more underscores than that; the point is, that interface is ugly, and explicitly depends on 'context'.

Why bother? Why can't OpenSSLCertificateOptions just operate on the bag of certificates?

If you already know that OpenSSLCertificateOptions has to adapt a hypothetical ITrustStore provider to IOpenSSLCertificateTrustAdderThing then why not just put the adapter into OpenSSLCertificateOptions? If it's just about code factoring then put the adapter into _openSSLTrustAdderThing and call it with the ITrustStore provider at the right time.

Every trust-discovery backend can then return an ITrustStore for enumerating its certificates in a portable way - for export to a foreign TLS implementation, or even just for debugging / diagnostics / reporting.

Or it can just return a concrete TrustStore instance containing a bag of certificates.

Of course, OpenSSL doesn't expose this functionality, so the OpenSSL backend won't offer a way to get a TrustStore for its built-in certificates (I could be wrong, maybe there's a really, really obscure API hidden somewhere, but this point doesn't really matter to the big picture).

But, trust-discovery backends may also, optionally, provide a backend-specific interface (IOpenSSLCertificateTrustAdderThing, ISecureTransportTrustThing) which do something on the order of addCAcertsToSpecificCertStoreType(backendSpecificContext, caCerts). In the case of "system default trust root", these methods can just call one method or maybe even be a no-op if the default behavior of that backend is just to use the system default trust root.

Hmm. Again, interfaces don't do things, so I'm not entirely sure I'm following you here. You mean that maybe sometimes when you ask the SecureTransport backend for its ITrustStore it gives you an ISecureTransportTrustThing instead? But that's okay because ... no, I lose the plot there. I don't see how that helps you at all. Only the SecureTransport backend knows anything about ISecureTransportTrustThing so only the SecureTransport backend can do anything special with that object. But it gave you that object in the first place so it doesn't need an interface to know it can do special things with it!

Somehow this feels horribly over-engineered to me, especially given that we only have one backend right now. But I don't have a better idea. The interfaces decompose relatively cleanly, I don't think it will be a ton of extra work to implement, and we need to start weaning ourselves off of OpenSSL eventually if we want to have some level of implementation agility.

Maybe if you explicitly describe the problem it solves it will stop sounding so over-engineered (that's how it sounds to me right now too).

So far, my understanding of the problem statement is:

We need to be able to tell CertificateOptions to do one of two things with respect to root CAs.

  1. Use this exact list of certificates as the root CAs
  2. Use the platform-supplied default list of certificates as the root CAs

It seems like a list of certificates (the type of a certificate is left as a problem to solve some other time) satisfies the first requirement. (Above this is referred to as "a bag of certificates", sorry for the mixed datatypes.)

A magic flag satisfies the second.

This definitely isn't the hippest API ever but it will solve the problem described here. If we're really unhappy with it we can always deprecate caCerts and introduce something equivalent but spelled in a less lame way.

I don't see where ITrustStore plus a number of backend-specific interfaces become necessary. If it still seems necessary to you, please elaborate on the problem definition or point out what I've missed.

Thanks!

comment:40 follow-up: Changed 9 months ago by hynek

So I really haven’t even understood what glyph meant because I’m not into zone.interface that hard core . ;)

I like the idea of “bag of certs” + special value for system trust store but having a special constant sucks.

So how about obscuring it a bit:

Let’s define ITrustStore with an ITrustStore.caCerts attribute. OpenSSLSystemTrustStore would simply return a special value here (a list with a well-known subclass of X509 for all I care).
Then let’s make caCerts accept bag of certs or an ITrustStore. Unbeknownst to the user that CO checks the caCerts for its special value and uses the system store if there’s a match?

This gives us more flexibility and IMHO a nice/more consistent API.


As a matter of fact we could even simplify this even further: define our special X509 (OpenSSLSystemCA?) and special-case it when it’s part of caCerts (raise ValueError if more than that is present?). That would leave our API intact and even make some sense.

I think I like this option most.


Maybe we should discuss this on IRC if we can’t agree; this ticket ping pong would take too long. The code is all there, there’s no reason to not finish this before 14.0.

In any case, I’m so far that I’ll implement anything we agree on, as long as we get this moving.

comment:41 in reply to: ↑ 40 Changed 9 months ago by glyph

Replying to hynek:

So I really haven’t even understood what glyph meant because I’m not into zone.interface that hard core . ;)

We'll have to fix that :-).

I am guessing the part you didn't get is about the 'adaptable to' part?

I like the idea of “bag of certs” + special value for system trust store but having a special constant sucks.

So how about obscuring it a bit:

Let’s define ITrustStore with an ITrustStore.caCerts attribute. OpenSSLSystemTrustStore would simply return a special value here (a list with a well-known subclass of X509 for all I care).

My proposal is that, if OpenSSLSystemTrustStore cannot provide a list of certificates actually loaded by OpenSSL, that it does not implement a generic ITrustStore at all; it does not need to return a dummy value from caCerts. If that's true, and OpenSSL's "default cert store" cannot enumerate its certs, it also means that it cannot be used with any backend except OpenSSL - but that is OK, that is reflecting a real fact of life which we cannot reasonably alter.

Then let’s make caCerts accept bag of certs or an ITrustStore. Unbeknownst to the user that CO checks the caCerts for its special value and uses the system store if there’s a match?

This is gross.

caCerts should have a strict contract. If it's supposed to be a list of bytes or something, then it should be a list of bytes. You may want to use an ITrustStore for something other than this.

What OpenSSLSystemTrustStore should actually provide is IOpenSSLSpecificTrustStore, an interface that defines one method, addCertsToOpenSSLContext. The trustStore argument to the OpenSSL backend (the thing that is going to build a context, like OpenSSLCertificateOptions) prefers this interface. It wants to call addCertsToOpenSSLContext. But of course that is not a very general sort of method, and not one that WindowsPlatformTrustStore will want to implement.

However, since PFXExportCertStoreEx can certainly provide you with a list of certificates, WindowsPlatformTrustStore can easily implement ITrustStore's generic "bag of certificate" interface. But OpenSSLCertificateOptions still wants to call addCertsToOpenSSLContext which is of course unknown to WindowsPlatformTrustStore. So we have an adapter, like this:

@implementer(IOpenSSLTrustStore)
class GenericCertStoreToOpenSSLTrustStore(object):
    def __init__(self, trustStore):
        self.trustStore = trustStore

    def addCertsToOpenSSLContext(self, context):
        store = context.get_cert_store()
        for eachCertsBytes in self.trustStore.certsAsPEMBytes():
            cert = crypto.load_certificate(crypto.FILETYPE_PEM,
                                           eachCertsBytes)
            store.add_cert(cert)

I've defined the interface as certsAsPEMBytes here just because I didn't want to start thinking about what, if anything, it would mean to have a backend-independent representation of a certificate. But perhaps we do actually want a certificate object of some kind.

For each backend, we can then do something like this:

registerAdapter(GenericCertStoreToOpenSSLTrustStore,
                ITrustStore,
                IOpenSSLTrustStore)

This means that, internally, OpenSSLCertificateOptions calls IOpenSSLTrustStore(suppliedTrustStore) and gets something providing that interface. If it's a trust store from a different backend, then it gets the GenericCertStoreToOpenSSLTrustStore adapter around that. If it's an "OpenSSL-native" trust store, then the adapter machinery notices it already provides the interface and passes it through.

And of course, we can have a default ITrustStore implementation that is just an in-memory list of bytes, which any backend or directory-scanning thing can implement.

The "magic token" for "use the system default cert store" for OpenSSL just then looks like this:

@implementer(IOpenSSLTrustStore)
class DefaultVerifyPaths(object):
    def addCertsToOpenSSLContext(self, context):
        context.set_default_verify_paths()
DEFAULT_VERIFY_PATHS = DefaultVerifyPaths()

comment:42 in reply to: ↑ 39 Changed 9 months ago by glyph

Replying to exarkun:

We need to be able to tell CertificateOptions to do one of two things with respect to root CAs.

  1. Use this exact list of certificates as the root CAs
  2. Use the platform-supplied default list of certificates as the root CAs

It seems like a list of certificates (the type of a certificate is left as a problem to solve some other time) satisfies the first requirement. (Above this is referred to as "a bag of certificates", sorry for the mixed datatypes.)

A magic flag satisfies the second.

This definitely isn't the hippest API ever but it will solve the problem described here. If we're really unhappy with it we can always deprecate caCerts and introduce something equivalent but spelled in a less lame way.

I don't see where ITrustStore plus a number of backend-specific interfaces become necessary. If it still seems necessary to you, please elaborate on the problem definition or point out what I've missed.

OK, let me try to focus really closely on how I'd like to solve the present problem.

If we go with the list / magic flag option, then we have to have an argument that can be one of two types, and a type-check to see which one it is, and a conditional to do one of two things. Either we're calling store.add_cert a bunch of times, or we're calling set_default_verify_paths once.

Generally speaking, whenever one has an API that looks like this:

class A(object):
    pass

class B(object):
    pass

def choice(parameter):
    if isinstance(parameter, A):
        a(parameter)
    else:
        b(parameter)

One should refactor it to look like this:

class A(object):
    def a_or_b(self):
        a()

class B(object):
    def a_or_b(self):
        b()

def choice(parameter):
    parameter.a_or_b()

So I would, first and foremost, like to see the distinction between set_default_verify_paths and add_cert implemented with the latter idiom because it's more flexible, more testable, and generally is actually less code.

Practically speaking: this means we have an interface, IOpenSSLTrustSettings, with one method, addCertsToContext, and two implementations: OpenSSLSpecificListOfCerts and OpenSSLDefaultVerifyPaths.

The latter may in fact be specified at the API level as a special constant, but the internal factoring will be better and the documentation will be clearer if it's an instance of a particular type.

Since this is a somewhat discrete point and hopefully makes sense on its own, I'll save my comments about why I'd like the store-independence "over-engineered" layer for a separate comment.

comment:43 in reply to: ↑ 39 Changed 9 months ago by glyph

Replying to exarkun:

Maybe if you explicitly describe the problem it solves it will stop sounding so over-engineered (that's how it sounds to me right now too).

So far, my understanding of the problem statement is:

We need to be able to tell CertificateOptions to do one of two things with respect to root CAs.

Now, as to why I think we should have this slightly more complicated structure.

In fact, soonish, we need to tell OpenSSL do three things with respect to root CAs:

  1. Use this exact list of certificates as the root CAs (not very exciting)
  2. Use the platform-supplied default list of certificates as the root CAs where "platform" means "OpenSSL's idea of where the default root CA certificate lives"
  3. Use the platform-specified default list of certificates as the root CAs where "platform" means the actual platform you're using, i.e. some hypothetical WindowsPlatformCertificateStore or OSXPlatformCertificateStore.

In the more distant future, we may want to tell any of these trust-database backends to talk to a different TLS-implementation backend, as well. I don't think we should put much effort into making that easier right now, but I think the implementation of my proposal doesn't add much in the way of implementation work.

Closer to the present, it is just a matter of good code hygiene that the code which loads certificates with a particular x509 API should not have any dependencies on OpenSSL, just because it's going to turn around and use those certificates with OpenSSL in a minute. If we absolutely loved OpenSSL and had no plans to use anything else ever, then this would be a purely academic concern - but future plans to use a different implementation later do add a smidgen of practical consideration to this otherwise theoretical separation of concerns.

So my proposal is that we define a single TLS-implementation-dependent interface declaration for specifying what CA certificates are trusted, as proposed in the previous comment, then - perhaps in a separate ticket - a single TLS-implementation-independent interface declaration for a thing which can give you a list of CA certificates in some format.

Maybe that latter interface is in fact isomorphic to "list of bytes objects which contain some asn.1", but I am thinking of it as an interface with a couple of methods on it because any API that can load some certificates probably has some options about how to load them incrementally, transform them, options to export them, maybe something to associate certificates with private keys and metadata about what protocols they're trusted for and so on. But maybe we can't use any of that information or those features now, or in fact ever; in any case they're not germane to the argument at hand, so perhaps for now should just talk about IJustListOfCACertsAsBytes and not ITrustStore

Assuming you accept my argument in the previous comment about how to implement this just for OpenSSL, despite all this verbiage, the amount of additional complexity in today's implementation that I'm proposing is simply the difference between:

class OpenSSLCertificateOptions:
    ...
    def _setUpCerts(self, context):
        self.trustSettings.addCertsToContext(context)
class OpenSSLCertificateOptions:
    ...
    def _setUpCerts(self, context):
        IOpenSSLTrustSettings(self.trustSettings).addCertsToContext(context)

Then we have IJustListOfCACertsAsBytes as a backend-independent interface that specifies how to load some certificates and return them, and an adapter from IJustListOfCACertsAsBytes to IOpenSSLTrustSettings which loads the certs and adds them to each context's certificate store.

(And, probably do the adaptation in __init__ so maybe it can cache the results or something, you get the idea.)

What this allows us to do is, as we implement each new platform-specific backend (and hey, maybe there's even more of these, like one for GNOME and one for KDE too) we can be sure to simply implement IJustListOfCACertsAsBytes, and not have to pollute each one with OpenSSL-specific APIs that will need to be ripped out later.

Further off into the future, if we ever do have full TLS implementation pluggability, each platform-specific trust database loader could then add a single interface to its @implementer declaration to provide specific, perhaps much less computationally expensive support for its natively-peered TLS implementation; like I already want OpenSSLDefaultVerifyPaths to supply for OpenSSL.

To reiterate, I don't think this extra layer would be worth doing if it were going to really increase the amount of work required. My goal is simply to avoid making more work for ourselves when we do a more flexible implementation in the future, not trying to front-load that work now. As it happens, that coincides with the goal of having a nice, decoupled implementation of loading certificates and simply passing them to OpenSSL.

I've tried hard to make this explanation a lot clearer, and if it really still isn't, perhaps best to barrel on without paying attention to what I'm trying to say here, I'd rather just have the magic flag implementation then spend another week hashing this out.

comment:44 Changed 9 months ago by glyph

Okay, I've just gone and implemented the concept from my simpler comment in the branch.

I think it requires a little more polishing, and I might not get to that until monday. Therefore, I reiterate that if my ideas don't make sense to those doing the implementation, please just reverse-cherry-pick out my change and get the branch into review, I promise I won't be mad ;-). I would like this to be nicely implemented but I would not like to block it from getting done.

comment:45 follow-up: Changed 9 months ago by glyph

A note to either myself or anyone else, here are the bits of "polish" I think are necessary to finish what I started:

  1. Rather than having OpenSSLDefaultPaths refuse to work anywhere but Linux, have an indirection point, trustFromPlatform() which determines if OpenSSLDefaultPaths actually counts as the "system" trust or not and raises the exception. There's no reason that users who have built their own OpenSSL with correct trust configuration on some weird platform shouldn't be able to call set_default_verify_paths on non-Linux as long as they're clear what it means.
  2. OpenSSLDefaultPaths needs to be public, as does the hypothetical trustFromPlatform.
  3. Regardless of my changes, the docs need to be updated to Sphinx.
  4. Documentation should be updated to refer to the new peerTrust argument everywhere and stop talking about the verify flag.
  5. PrivateCertificate.options should use the new peerTrust argument. As a bonus, it no longer needs the "set three arguments" shenanigans because those attributes are all implied by peerTrust because WTF it would be incoherent to set verify without caCerts and vice versa.

comment:46 in reply to: ↑ 45 Changed 9 months ago by glyph

Replying to glyph:

A note to either myself or anyone else, here are the bits of "polish" I think are necessary to finish what I started:

  1. Rather than having OpenSSLDefaultPaths refuse to work anywhere but Linux, have an indirection point, trustFromPlatform() which determines if OpenSSLDefaultPaths actually counts as the "system" trust or not and raises the exception. There's no reason that users who have built their own OpenSSL with correct trust configuration on some weird platform shouldn't be able to call set_default_verify_paths on non-Linux as long as they're clear what it means.
  2. OpenSSLDefaultPaths needs to be public, as does the hypothetical trustFromPlatform.

I did these yesterday, but there's a slight wrinkle: the previous all-in-one "platform" verifier would just defer actually figuring out if there was such a thing as valid platform trust until making a connection, whereas platformTrust (what I ended up calling it in the code) bails out during construction, resulting in a much earlier failure.

I think that's a better way to go, but it also means a different sort of error reporting. If nobody else has any thoughts I'll probably just update the tests to account for this.

comment:47 Changed 9 months ago by glyph

So, here's a problem.

Right now, the heuristic that we're using to determine whether set_default_verify_paths is actually representative of "platform trust" is somewhat flawed. isLinux doesn't deal with the fact that, for example, on the default Docker Ubuntu image, ca-certificates is not installed and the platform trust roots are empty.

Unhappily, OpenSSL has no public API to discover whether this is the case. X509_STORE seems to be a completely opaque drop-box.

Happily again, on OS X, Apple has actually patched OpenSSL to verify certificates against the user's keychain in a certain very special situation: if the "verify" flag is turned on, but no additional certificate authority certificates are explicitly added by the user. This means that the "linux" trust root here actually happens to work just fine for OS X!

But, unhappy again now, there's still no way to discover whether this is the case at runtime: you have to be using the Apple system OpenSSL (that file is licensed under the APSL, not OpenSSL's license, so it's unlikely that OpenSSL would ever include it; plus, even if they did, TrustEvaluationAgent is a private framework, so normal humans can't see the headers that file includes or build an OpenSSL that includes that functionality).

Now, if you're using Apple's system python, this is of course no problem; the system pyOpenSSL is built against the system OpenSSL, but some jerk keeps telling people to "brew install python" instead (Hi Kenneth!), which, depending on how you've got homebrew configured and what other commands you've run first, may or may not install an alternate OpenSSL, with or without trust roots.

I gave this some thought yesterday, and I'm a little concerned that if we change Twisted so that on some platforms so that it starts verifying certs against an empty trust store, this will encourage those with misconfigurations to start falling back to plaintext rather than setting up some trust roots properly.

However, I think I've come to the conclusion that that's out of scope for this ticket. For the time being, I think I'm just going to drop the isLinux check entirely, assume that the platform always has good trust roots from set_default_verify_paths, and then defer any platform heuristics in platformTrust to a future ticket.

comment:48 Changed 9 months ago by glyph

(To be clear, Kenneth is not actually a jerk, that guide is generally pretty helpful; I just think the bit about "significant changes" that cause "hidden bugs" is a little harsh and in need of a big ol' [citation needed].)

comment:49 Changed 9 months ago by glyph

Also, since oblique sniping in a remote tracker is not helpful I went ahead and filed a ticket so maybe that guide will get updated.

comment:50 Changed 9 months ago by glyph

  • Author changed from itamarst, rwall to itamarst, glyph, rwall
  • Branch changed from branches/trusted-ca-linux-5446-2 to branches/trusted-ca-openssl-defaults-5446

(In [41486]) Branching to trusted-ca-openssl-defaults-5446.

comment:51 follow-up: Changed 9 months ago by hynek

I’d like to point out: http://rentzsch.tumblr.com/post/33696323211/wherein-i-write-apples-technote-about-openssl-on-os-x

So I find all the side-tracking about OS X rather uninteresting. That something that will have to be solved using certifi or something similar anyway eventually.

Since you’ve more or less implemented my original design idea, I’m not going to argue it but I would be great if this could go into review soon so it has a chance to get into Twisted before 14.0; I don’t want to be standing on stage at PyCon saying that Tornado does something vital better than we do.

Some observations in front to speed review up:


For the record, after all the bike shedding we had, I personally would’ve preferred to cover the special case with a boolean option (useSystemTrustStore, every SSL implementation will have some concept of that) and build the rest on top of caCerts (calling some functions with some arguments). That would be a much cleaner and more portable approach than this, which is somewhat muddy by trying to be general without actually being it.

But I won’t stop you here since I ran out of care-juice and just want Twisted to become more secure.

comment:52 in reply to: ↑ 51 Changed 9 months ago by glyph

Replying to hynek:

I’d like to point out: http://rentzsch.tumblr.com/post/33696323211/wherein-i-write-apples-technote-about-openssl-on-os-x

Yes, I'm familiar with all of this stuff.

So I find all the side-tracking about OS X rather uninteresting.

People do use OS X, quite a bit. Importantly, they use it for development, so
if this doesn't work properly there then most developers are going to assume
that it's broken everywhere and implement their own nonsense that doesn't
really work.

That something that will have to be solved using certifi or something similar anyway eventually.

I think there's a little confusion here.

I am not trying to implement some mechanism to load the One Correct And Accurate List of Certificate Authority Certificates into our TLS implementation.

If that's what we wanted we might as well just distribute certs along with Twisted and call it a day.

What I'm trying to get at here is to provide the user with control over the authorities that they trust, ideally without creating any tooling on the Twisted side of things.

So, for example, going from the accidental-keychain-trust-root implementation in OS X's built-in OpenSSL to the fixed, potentially read-only list from certifi would be a step backwards; therefore we should be aware of the caveats here.

Since you’ve more or less implemented my original design idea, I’m not going to argue it

Sorry for the run-around on that. Hopefully my efforts on implementation make up for the somewhat circuitous discussion :).

but I would be great if this could go into review soon so it has a chance to get into Twisted before 14.0; I don’t want to be standing on stage at PyCon saying that Tornado does something vital better than we do.

I'd certainly rather you don't have to say that either.

Some observations in front to speed review up:

  • semantic linefeeds please

It made sense to me in HTML but it looks confusing and odd in ReST, since ReST
seems more analagous to epytext, and since line-breaks are significant or not
depending on indentation. Not to mention the fact that the Sphinx conversion
eliminated all of our existing semantic linefeeds. but I guess we're still
doing it? Hmm.

I think I'm going to stop thinking about that here.

  • AFAIU the TDS, no blank line btw docstring and method.

A single blank line may be used at the developers' discretion :-). At least, if twistedchecker doesn't complain I'm not going to worry about this one.

Thanks for the catch, fixed.

  • True and False should be in L{}

Thanks for that too; fixed, I don't know what I was thinking.

It's an expression of intent.

I might delete that before putting this up for review, or I might write a light
frosting of code that depends on it and then it will have a clearer motivation.
Given the gallons of virtual ink spilled earlier in this ticket trying to
explain my desire for this, I think I'll pass on trying to explain it again.

  • the top file is outdated.

Still working on that and the rest of the docs.

For the record, after all the bike shedding we had, I personally would’ve preferred to cover the special case with a boolean option (useSystemTrustStore, every SSL implementation will have some concept of that) and build the rest on top of caCerts (calling some functions with some arguments). That would be a much cleaner and more portable approach than this, which is somewhat muddy by trying to be general without actually being it.

That's the problem: every TLS implementation will have some concept of
that, except that the actual "system" trust store, the one the user has control
over, does not usually live in the one TLS implementation that we actually
support right now. OpenSSL's concept of "system trust store", for example, is
hopelessly broken, as implementing this ticket has really reinforced for me
;-).

But I won’t stop you here since I ran out of care-juice and just want Twisted to become more secure.

Apparently you haven't entirely run out, since you did just write all that ;).

comment:53 Changed 8 months ago by glyph

  • Keywords review added
  • Owner itamar deleted

these twistedchecker errors appear to be bogus], and everything else (modulo the usual Windows nonsense) seems to be passing.

I have some stuff for determining when it's appropriate to do a fallback to something like certifi, but I think maybe that should live in a separate branch.

For the reviewer's convenience, I unpacked the generated documentation so you can skip directly to reading the new section that this branch adds with all the nice colors and everything.

Thanks.

comment:54 Changed 8 months ago by hynek

  • Owner set to hynek

comment:55 Changed 8 months ago by hynek

  • Keywords review removed
  • Owner changed from hynek to glyph

Thanks for persevering!

  1. Overall: I rather dislike peerTrust because it’s not about peers, it’s about trusting CAs and their signatures. Let me stress that I’m not re-opening the design can of worms. I just dislike that name.
  2. Are changes to docs/_extensions/apilinks.py and docs/_themes/twistedtrac/static/twistedtrac.css purposely part of this branch?
  3. ssl.rst
    1. All examples should use peerTrust now if possible. People copy and paste, they ought to copy and paste the right thing.
    2. 13: If you wish to use dashes for interpunction, please use -- which gets correctly translated into endashes by Sphinx.
    3. 14: I’d split “in particular…” into a new sentences. While I admire your ability to write really polished prose, I feel like docs should be as straightforward as possible. :)
    4. 16–19: Please reflow the rest using semantic linefeeds since the churn’s there anyway.
    5. 179: dashes
    6. 179: “verify the server’s certificate”, not the server.
    7. 180: I guess the “to” before the comma doesn’t belong there?
    8. 186: You have to mention that it works only with OpenSSL shipped by Apple/problably Linux environments. I did some research and the function we’re using isn’t even documented or something…
    9. 187: I love the semicolon too; but the audience is better served with two sentences here.
    10. 368: You used (rightfully) TLS in all instances before.
  4. Both examples: unless you want to print tuples, you want a from __future__ import print_function.
  5. check_echo_certificate.py: You’ve passing a certificate as peerTrust, I guess that works because of CertBase.__conform__? I don’t like such magic to be honest but won’t hang up on that.
  6. iosym.py:
    1. 359: Could you document connectedServerAndClient which you used in your tests? It looks very handy and the docstring makes me sad.
    2. 54: Since you touched the haha, please add a second space before # :)
  7. The top file is still from the old approach.
  8. ssl.py
    1. ContextFactory and ClientContextFactory ought to be deprecated; not added to the header and linked to. :) #6923 I guess we should define an Interface IContextFactory?
  9. _sslverify.py:
    1. 169: L not C
    2. 197: Shouldn’t L work for zope.i?
    3. 199: L not C
    4. Certificate.options is terrible api, but are you sure you don’t want it to have peerTrust? That makes it pretty useless TBH.
    5. 675: I don’t think “certificate-authority certificates” is a thing. And “any relevant” is a bit mushy. How about “Add implementation-specific certificates of trustworthy certificate authorities.”?
    6. 678: That description doesn’t make much sense to me. How about something akin to “An SSL context whose connections' peer certificates ought to be verified against the root certificates that get added.” I know this is a terrible sentence and by all means make it nicer, but at least it captures what’s going on. At this occasion let me smartass for a minute and note that an SSL context is not really connection dependent in the wider OpenSSL world. It’s just settings you want to apply to connections. Our ContextFactory APIs are rather odd IMHO but at least we cache them. That’s also why I don’t like adding hostname verification into context factories but that’s beside this ticket.
    7. 680: L not C
    8. 690: I don’t think the dash is necessary.
    9. 690: the “as” doesn’t IMHO add anything
    10. 691: L not C
    11. 696: what certificate authorities?
    12. 697: L not C
    13. 707: Not “any relevant”. Exactly those, that the user specified. In fact, this docstring is redundant since it’s a private method of an interface. If you insist on writing one, make it specific.
    14. 748: Beside a mission “a” before “web browser”, this sentence is an abomination that only college-educated English native speakers will understand. Please chop it up mercilessly. :) If epytext allows for enumerations, this would be the place to use one.
    15. 790: Again, ; = awesome, but please chop up.
    16. 893, 897, 903: since you churn around these lines anyway, may I suggest adding an L{} to True?
    17. 933: there’s no empty line before @type
    18. 926: Can you make this two sentences? Something like “If this argument is specified, the peer is verified. It requires a certificate, and the certificate must be signed by one of the certificate authorities specified by C{peerTrust}.” I think it’s worth noting, that it silently overrides and supersedes.
  10. test_sslverify:
    1. 879: Please extend and use the global FakeContext.
    2. 872: It doesn’t really test whether it loads…it just tests that the trust provider has been called. Should the comment reflect that?
    3. 888: why is _buildCAandServerCertificates underscored while the other helper methods aren’t? I seem to remember that we don’t underscore test helper methods because it doesn’t make any sense but I might be wrong.
    4. 888: That said, could you make it a function? I recall tickets that called for getting rid of shipped test certificates this function (maybe with some extensions later) could be useful for that. You’ll have to add tests of course; although I’d argue some basic tests wouldn’t hurt for the current state too. :) May I suggest a class CertificateAuthority that has methods like createRSACertificate('example.com', keySize=4096) et al? That sounds great in my head at least. Skip this if you feel it would slow you down too much.
    5. 915: loopbackTLSConnection sounds like a useful function for others too (pass in a CF?).
    6. You’ve put all your tests into ProtocolVersionTests which is probably a mistake. You’ve probably meant to add it into OpenSSLOptions (which really should be renamed).
    7. 927: “certficate“
    8. 981: that would be also a part 4. A man can dream.
    9. 981: asDumpedIntoAFile or dumpableAsFile? asDumped doesn’t tell me anything except that there’s some dumping done.
    10. 986: It’s not “an object”, it’s a list.
    11. It should also have a @type?
    12. 990: @rtype?
    13. 987: I have no idea what “taking a pyOpenSSL filetype” is supposed to mean.
    14. 994: I totally am hungry for dumplings now.
    15. 1002: L not C
    16. 1022: no need to capitalize Error
    17. 1025: lol @ “Some combination”
    18. 1037: don’t lazy here sir :) I actually don’t care what way, but keep it consistent with 1014.
    19. 1043: assertIdentical is an undocumented (I had to grep Twisted for it) alias for assertIs. Please use the latter.


I’m sure I oversaw something, but there’s a quite a bit up there to keep you busy. :| Please address as you see it fit and re-submit for review ASAP. Thanks again for working on this with me!

P.S. Sorry if I appear nitpicky about style in docs, but I’ve been told how terrible our docs are for newbies just the other day (after I made the same experience). Thus it’s high priority to me to make this better.

This review has been brought to you by two cups of strong coffee.

comment:56 Changed 8 months ago by glyph

hynek,

Thanks for the review.

I plan to address all of this feedback, but I feel like I should make a meta-review comment here first:

This review doesn't separate out nitpicky issues of personal taste, questions about design, and needs-to-be-fixed-before-landing issues. In the future, please try to do this as much as possible with your reviews. This is important because the author should have as much discretion as is reasonable when fixing matters of opinion, and it should be clear that this is the case.

However, since I dumped a ginormous diff on you with dozens of unrelated changes in it, I want to be clear that I'm at least equally responsible for the sprawling review, and I appreciate your effort in doing it. I just want to be sure that we improve our review culture to separate these things and reduce the amount of bikeshedding and perfectionism when trying to land things from new contributors.

-glyph

comment:57 Changed 8 months ago by glyph

  • Status changed from new to assigned

comment:58 Changed 8 months ago by hynek

FWIW, I summarize this stuff in the epilogue. In your case it was “I’m sure I oversaw something, but there’s a quite a bit up there to keep you busy. :| Please address as you see it fit and re-submit for review ASAP.” because…yeah what you wrote. :)

comment:59 Changed 8 months ago by glyph

(In [41807]) AP stylebook for the win, use some em dashes, address review feedback 3.2 3.3 and some of 3.4. refs #5446

comment:60 Changed 8 months ago by glyph

(In [41810]) 3.8 and 3.9. refs #5446

comment:61 Changed 8 months ago by glyph

(In [41811]) 3.10. refs #5446

comment:62 Changed 8 months ago by glyph

(In [41813]) review point 4 refs #5446

comment:63 Changed 8 months ago by glyph

(In [41814]) review point 6.2 refs #5446

comment:64 Changed 8 months ago by hynek

AP stylebook may be for the win if you’re a newspaper but it still looks gross.

I have looked up where the endash rule I’m familiar with comes from and it is no less than The Elements of Typographic Style which I find a nicer standard to aspire to than a news agency whose choices may be affected by some historical cruft not relevant to us…

I’m sorry, I just care about typography.


Regardless of your ultimate decision, please make sure to add it to the documentation guide ticked so it is documented because having a mixture of dashes, endashes, and emdashes is even worse.

comment:65 Changed 8 months ago by Alex

iosim.py: 61/63

  • This is a change in semantics, the method now always returns 0.

comment:66 Changed 8 months ago by glyph

(In [41823]) Document connectedServerAndClient as per point 6.1. refs #5446

comment:67 Changed 8 months ago by glyph

(In [41824]) review point 7, fix top file's description (and since this is important, make it a little more expansive) re #5446

comment:68 Changed 8 months ago by glyph

(In [41825]) review point 9.19 refs #5446

comment:69 Changed 8 months ago by glyph

(In [41828]) Make that test pass.

Thanks to Alex for pointing this out.

refs #5446

comment:70 Changed 8 months ago by glyph

(In [41830]) I was talking about the class so I guess I'll put some curly braces around it. review point 9.16 refs #5446

comment:71 Changed 8 months ago by glyph

(In [41831]) review point 9.15; L not C and adjust reference. refs #5446

comment:72 Changed 8 months ago by glyph

(In [41832]) review point 9.13; explain what a filetype constant is with some more L{}s that won't resolve refs #5446

comment:73 Changed 8 months ago by glyph

(In [41833]) review point 9.12; add an rtype refs #5446

comment:74 Changed 8 months ago by glyph

(In [41834]) review point 9.10 and 9.11; I hate describing * and parameters. How's this for a convention. refs #5446

comment:75 Changed 8 months ago by hynek

Since it wasn’t explicitly stated on this issue yet:

  • set_default_verify_paths is a NOP on Apple’s OpenSSL.
  • their TEA is used as fallback on verification failures regardless what the user does.
  • homebrew actually does something smart: on installation they extract the trusted certificates from the keyring and use it for set_default_verify_paths. That means that trustdb (/usr/local/etc/openssl/osx_cert.pem) is basically frozen but contains sensible data. So there isn’t really any need to try to be smart on OS X; the two major uses cases are covered.

comment:76 Changed 8 months ago by glyph

(In [41840]) review point 9.9: fine, fine, whatever you say; refs #5446

comment:77 Changed 8 months ago by glyph

(In [41841]) oops, got confused, the dumpables aren't the filetype constants refs #5446

comment:78 Changed 8 months ago by glyph

(In [41842]) review point 9.7 refs #5446

comment:79 Changed 8 months ago by glyph

(In [41843]) mostly address the rest of point 9; refs #5446

comment:80 Changed 8 months ago by glyph

(In [41876]) Address review points 9.16-9.18.

refs #5446

comment:81 Changed 8 months ago by glyph

(In [41877]) Why you gotta hate semicolons, hynek? Why? Address review point 9.15.

refs #5446

comment:82 Changed 8 months ago by glyph

(In [41878]) Address review point 8.14.

Shorter sentences, easier to understand, fewer five dollar words.

Not sure what you meant about enumerations but hopefully this is clear enough now.

refs #5446

comment:83 Changed 8 months ago by glyph

(In [41879]) Address review point 8.13.

Remove redundant docstrings.

refs #5446

comment:84 Changed 8 months ago by glyph

(In [41880]) Address the rest of point 8.

refs #5446

comment:85 Changed 8 months ago by glyph

(In [41882]) Clean up stuff into proper functions refs #5446

comment:86 Changed 8 months ago by glyph

(In [41891]) Move test cases to their own suite rather than abusing ProtocolVersionTests. refs #5446

comment:87 follow-up: Changed 8 months ago by glyph

  • Keywords review added
  • Owner glyph deleted
  • Status changed from assigned to new

Replying to hynek:

Thanks for persevering!

No problem. Sorry again that the review was so huge.

One comment about the review, though: please don't use line numbers in future reviews. The use of line numbers is one of the things I personally dislike about github's review system, since it encourages thinking about nitpicky ephemera rather than structural issues; but, to the extent that it does work on github, it's because they automatically recompute line numbers as you go and hide comments on lines that you've changed. On this review, I had to work on addressing feedback backwards so that I wouldn't screw up the line numbers on previous comments and I still knew what you were talking about.

Instead, please use FQPNs so that I (I mean, uh, "the author") can quickly jump to the identifier that the feedback is about.

  1. Overall: I rather dislike peerTrust because it’s not about peers, it’s about trusting CAs and their signatures. Let me stress that I’m not re-opening the design can of worms. I just dislike that name.

Sounds good. I went with trustRoot instead. This seems to read a bit better to me, so thanks for the suggestion.

  1. Are changes to docs/_extensions/apilinks.py and docs/_themes/twistedtrac/static/twistedtrac.css purposely part of this branch?

So, I could move them into a separate branch if you want, but they were necessary to make the ssl.rst document look right.

  1. ssl.rst
    1. All examples should use peerTrust now if possible. People copy and paste, they ought to copy and paste the right thing.

Which examples, specifically? None of them use caCerts any more; it's not common for TLS servers to require client peer trust, and the one other client example specifically doesn't do any verification.

  1. 13: If you wish to use dashes for interpunction, please use -- which gets correctly translated into endashes by Sphinx.

Actually, as per the AP Stylebook, I am going with em dashes (therefore ---).

  1. 14: I’d split “in particular…” into a new sentences. While I admire your ability to write really polished prose, I feel like docs should be as straightforward as possible. :)

"in particular" is more like a verbal tic than quality prose, so feel free to issue corrections like this :)

  1. 16–19: Please reflow the rest using semantic linefeeds since the churn’s there anyway.

I really wish there were a button in my editor for this.

  1. check_echo_certificate.py: You’ve passing a certificate as peerTrust, I guess that works because of CertBase.__conform__? I don’t like such magic to be honest but won’t hang up on that.

The magic here is totally static. It's not relying upon some special __init__.py being imported or anything.

  1. iosym.py:
    1. 359: Could you document connectedServerAndClient which you used in your tests? It looks very handy and the docstring makes me sad.

Worth also noting: #5167

  1. 54: Since you touched the haha, please add a second space before # :)

Not really helpful, deleted :-).

  1. ssl.py
    1. ContextFactory and ClientContextFactory ought to be deprecated; not added to the header and linked to. :) #6923 I guess we should define an Interface IContextFactory?

Deprecate them in that ticket, then. For now, they're the closest thing we've got to a public interface, they're incredibly old and used everywhere, and we should document what we're talking about when we talk about them.

  1. _sslverify.py:
  1. Certificate.options is terrible api, but are you sure you don’t want it to have peerTrust? That makes it pretty useless TBH.

Not quite sure what you're referring to here. You can totally just do PrivateCertificate(...).options(peerTrust=...). You just can't do that and also pass a list of authorities as positional args for convenience.

  1. 675: I don’t think “certificate-authority certificates” is a thing.

It is. They're the certificates for a certificate authority. Technically
speaking they're not "trust roots" because you can also have intermediate
authorities.

At this occasion let me smartass for a minute (...)

Your status as a smartass is noted.

  1. 933: there’s no empty line before @type

Prevailing style (and also what my automated tool will do) is to add a blank line between @type and @param if the @param field consists of multiple paragraphs. So don't think of it as a blank line after @param, but rather, blank lines surrounding a separate paragraph :-).

I think it’s worth noting, that it silently overrides and supersedes.

So, this comment made me think that silently overriding and superseding is really pretty bad, especially if it doesn't need to.

I added a general utility to twisted.python.deprecate, because implementing this in the specific case has some undesirable side effects; it can start incorrectly documenting the default value of caCerts et. al. by giving them some magic token that __init__ can check for, and a more ad-hoc solution would not catch positional parameters. However, I left the utility private because I'm not sure if we want to expose it yet.

And yes, before you ask, I know about inspect.getcallargs, but (A) it's only in Python 2.7, and (B) it tells you what the actual values will be, and I care about what the passed values will be; i.e. even if you explicitly passed the default, I want to warn you.

  1. test_sslverify:
  1. 981: that would be also a part 4. A man can dream.

Not sure what you mean by this.

  1. 994: I totally am hungry for dumplings now.

Great, now so am I.

  1. 1025: lol @ “Some combination”

I really can't be bothered to figure out where exactly the failure lies.

  1. 1037: don’t lazy here sir :) I actually don’t care what way, but keep it consistent with 1014.

I guess you were referring to the variable names here?

  1. 1043: assertIdentical is an undocumented (I had to grep Twisted for it) alias for assertIs. Please use the latter.

Interesting. I don't think I even realized this; I always use assertIdentical, although it seems like assertIs is slightly more popular even within Twisted itself.

I’m sure I oversaw something, but there’s a quite a bit up there to keep you busy. :| Please address as you see it fit and re-submit for review ASAP. Thanks again for working on this with me!

I think you mean “overlooked”. You definitely oversaw the whole thing.

P.S. Sorry if I appear nitpicky about style in docs, but I’ve been told how terrible our docs are for newbies just the other day (after I made the same experience). Thus it’s high priority to me to make this better.

This review has been brought to you by two cups of strong coffee.

Replying to Alex:

iosim.py: 61/63

  • This is a change in semantics, the method now always returns 0.

Thanks a lot for pointing this out, Alex; fixed this as well.

comment:88 Changed 8 months ago by glyph

Buildbots look OK, and the API doc builder's warnings are all spurious; there are existing issues to fix the reference errors.

comment:89 in reply to: ↑ 87 Changed 8 months ago by hynek

  • Keywords review removed
  • Owner set to glyph

Replying to glyph:

  1. Overall: I rather dislike peerTrust because it’s not about peers, it’s about trusting CAs and their signatures. Let me stress that I’m not re-opening the design can of worms. I just dislike that name.

Sounds good. I went with trustRoot instead. This seems to read a bit better to me, so thanks for the suggestion.

trustRoot is much better indeed. Also liking the singular.

  1. Are changes to docs/_extensions/apilinks.py and docs/_themes/twistedtrac/static/twistedtrac.css purposely part of this branch?

So, I could move them into a separate branch if you want, but they were necessary to make the ssl.rst document look right.

No that’s fine, I just wondered.

  1. ssl.rst
    1. All examples should use peerTrust now if possible. People copy and paste, they ought to copy and paste the right thing.

Which examples, specifically? None of them use caCerts any more; it's not common for TLS servers to require client peer trust, and the one other client example specifically doesn't do any verification.

Hrhm. I’d preferred if they did because people probably stop reading after that section. Maybe some very stern warning that this kind of code should not be used in production? Maybe above reactor.connectSSL('localhost', 8000, factory, ssl.CertificateOptions()) in the example?

→ Please address this with code or words.

  1. 13: If you wish to use dashes for interpunction, please use -- which gets correctly translated into endashes by Sphinx.

Actually, as per the AP Stylebook, I am going with em dashes (therefore ---).

Please make sure this is documented. The only thing that’s more gross than using a newspaper style guide is using none. :-P

→ Please comment on the style guide ticket

  1. 16–19: Please reflow the rest using semantic linefeeds since the churn’s there anyway.

I really wish there were a button in my editor for this.

I heard you know how to write software. :>

  1. iosym.py:
    1. 359: Could you document connectedServerAndClient which you used in your tests? It looks very handy and the docstring makes me sad.

Worth also noting: #5167

I understand you made iosim.py Python 3-capable? It should be added to https://github.com/twisted/twisted/blob/trunk/twisted/python/dist3.py – which won’t work because that landed after this branch has started. Merge forward?

→ Please address this with code or words.

  1. ssl.py
    1. ContextFactory and ClientContextFactory ought to be deprecated; not added to the header and linked to. :) #6923 I guess we should define an Interface IContextFactory?

Deprecate them in that ticket, then. For now, they're the closest thing we've got to a public interface, they're incredibly old and used everywhere, and we should document what we're talking about when we talk about them.

I just find it odd to write docs for classes people shouldn’t be using. CertificateOptions is the public interface everyone should be using, no? They haven’t got any of the TLS love that went into CertificateOptions and as such are insecure: http://twistedmatrix.com/trac/browser/tags/releases/twisted-13.2.0/twisted/internet/ssl.py#L112

→ I find this point very important.

  1. _sslverify.py:
    1. Certificate.options is terrible api, but are you sure you don’t want it to have peerTrust? That makes it pretty useless TBH.

Not quite sure what you're referring to here. You can totally just do PrivateCertificate(...).options(peerTrust=...). You just can't do that and also pass a list of authorities as positional args for convenience.

I’m not sure I understand. ISTM that .options() only takes positional arguments, so a) peerTrust= is impossible and in particular b) it’s impossible to pass something symbolic like platformTrust that people should use.

→ Please address this with code or words.

I think it’s worth noting, that it silently overrides and supersedes.

So, this comment made me think that silently overriding and superseding is really pretty bad, especially if it doesn't need to.

On that we can agree.

I added a general utility to twisted.python.deprecate, because implementing this in the specific case has some undesirable side effects; it can start incorrectly documenting the default value of caCerts et. al. by giving them some magic token that __init__ can check for, and a more ad-hoc solution would not catch positional parameters. However, I left the utility private because I'm not sure if we want to expose it yet.

I like the approach but I don’t like TypeError. Passing mutually exclusive arguments is not a type mismatch. I guess RuntimeError makes more sense?

→ Please address this with code or words.

  1. 1037: don’t lazy here sir :) I actually don’t care what way, but keep it consistent with 1014.

I guess you were referring to the variable names here?

Uh probably. I can see the problem with line numbers now. :|

I think you mean “overlooked”. You definitely oversaw the whole thing.

Damned be those false friends. :)


Way forward:

I’ve marked those parts that need to be addressed. Especially the ContextFactory/ClientContextFactory thing.

It depends on your decisions how to address the points I’ve marked whether this needs another review cycle. Don’t worry about long review times though, I want this to land as much as you do.

comment:90 Changed 8 months ago by glyph

  • Author changed from itamarst, glyph, rwall to itamarst, rwall, glyph
  • Branch changed from branches/trusted-ca-openssl-defaults-5446 to branches/trusted-ca-openssl-defaults-5446-2

(In [41928]) Branching to trusted-ca-openssl-defaults-5446-2.

comment:91 Changed 8 months ago by glyph

  • Resolution set to fixed
  • Status changed from new to closed

(In [41934]) Merge trusted-ca-openssl-defaults-5446-2: add trustRoot argument to CertificateOptions, and platformTrust API which is what you should always pass to it.

Author: glyph
Reviewer: hynek
Fixes: #5446

Twisted's TLS support now provides a way to ask for user-configured trust roots
rather than having to manually configure such certificate authority
certificates yourself. twisted.internet.ssl.CertificateOptions now accepts a
new argument, trustRoot, which combines verification flags and trust sources,
as well as a new function that provides a value for that argument,
twisted.internet.ssl.platformTrust, which allows using the trusted platform
certificate authorities from OpenSSL for certificate verification.

comment:92 Changed 8 months ago by glyph

Replying to hynek:

Replying to glyph:

  1. Are changes to docs/_extensions/apilinks.py and docs/_themes/twistedtrac/static/twistedtrac.css purposely part of this branch?

So, I could move them into a separate branch if you want, but they were necessary to make the ssl.rst document look right.

No that’s fine, I just wondered.

For posterity, the issue was that I wanted to have some ..note and ..warn sections in there show up in yellow and red respectively in our website's theme. For these important security warnings, I felt having it actually show up in red is important, which is how it landed in this branch.

  1. ssl.rst
    1. All examples should use peerTrust now if possible. People copy and paste, they ought to copy and paste the right thing.

Which examples, specifically? None of them use caCerts any more; it's not common for TLS servers to require client peer trust, and the one other client example specifically doesn't do any verification.

Hrhm. I’d preferred if they did because people probably stop reading after that section. Maybe some very stern warning that this kind of code should not be used in production? Maybe above reactor.connectSSL('localhost', 8000, factory, ssl.CertificateOptions()) in the example?

→ Please address this with code or words.

Actually, I'm going to explicitly reject this as feedback for this ticket. This is a legitimate concern about our TLS documentation, but this ticket is about there being a correct way to deal with the problem, not about having absolutely everything use that correct way and all the documentation reflect the best practices in every place. If I'm going to start talking about how this is insecure, I also need to talk about how not verifying the hostname is insecure, and instead we should move along to integrating service_identity so that that prose can usefully point at a solution.

  1. 13: If you wish to use dashes for interpunction, please use -- which gets correctly translated into endashes by Sphinx.

Actually, as per the AP Stylebook, I am going with em dashes (therefore ---).

Please make sure this is documented. The only thing that’s more gross than using a newspaper style guide is using none. :-P

→ Please comment on the style guide ticket

Done.

  1. iosim.py:
    1. 359: Could you document connectedServerAndClient which you used in your tests? It looks very handy and the docstring makes me sad.

Worth also noting: #5167

I understand you made iosim.py Python 3-capable? It should be added to https://github.com/twisted/twisted/blob/trunk/twisted/python/dist3.py – which won’t work because that landed after this branch has started. Merge forward?

→ Please address this with code or words.

I had some adventures with the py3 buildbot because some tickets in the meanwhile broke things.

  1. ssl.py
    1. ContextFactory and ClientContextFactory ought to be deprecated; not added to the header and linked to. :) #6923 I guess we should define an Interface IContextFactory?

Deprecate them in that ticket, then. For now, they're the closest thing we've got to a public interface, they're incredibly old and used everywhere, and we should document what we're talking about when we talk about them.

I just find it odd to write docs for classes people shouldn’t be using. CertificateOptions is the public interface everyone should be using, no? They haven’t got any of the TLS love that went into CertificateOptions and as such are insecure: http://twistedmatrix.com/trac/browser/tags/releases/twisted-13.2.0/twisted/internet/ssl.py#L112

→ I find this point very important.

I've tried to strongly steer users in the right direction while still documenting the closest thing we have to interfaces. (After all, why is this thing even called a contextFactory?)

  1. _sslverify.py:
    1. Certificate.options is terrible api, but are you sure you don’t want it to have peerTrust? That makes it pretty useless TBH.

Not quite sure what you're referring to here. You can totally just do PrivateCertificate(...).options(peerTrust=...). You just can't do that and also pass a list of authorities as positional args for convenience.

I’m not sure I understand. ISTM that .options() only takes positional arguments, so a) peerTrust= is impossible and in particular b) it’s impossible to pass something symbolic like platformTrust that people should use.

→ Please address this with code or words.

Oh wow I totally, repeatedly mis-read the signature of options, and I kept thinking that it could take arbitrary keyword arguments and pass them on to CertificateOptions.

I think we should change it so that it does.

However, I think we can change it in a separate change. The security this branch is concerned with is the default case for client connections. PrivateCertificate.options(authority) was created mostly for the use-case where you're a server verifying client certificates, in which case working from the system trust roots is actually the wrong thing to do.

Of course you can use it for a client using a client certificate as well, but I don't think it's that much of a hardship to ask people in that slightly unusual case to either wait a release or just construct an appropriate CertificateOptions object directly. There are already dozens of other important options they can't access if they're using this API.

So, this comment made me think that silently overriding and superseding is really pretty bad, especially if it doesn't need to.

On that we can agree.

Thanks for bringing this to my attention, it was a really good catch.

I added a general utility to twisted.python.deprecate, because implementing this in the specific case has some undesirable side effects; it can start incorrectly documenting the default value of caCerts et. al. by giving them some magic token that __init__ can check for, and a more ad-hoc solution would not catch positional parameters. However, I left the utility private because I'm not sure if we want to expose it yet.

I like the approach but I don’t like TypeError. Passing mutually exclusive arguments is not a type mismatch. I guess RuntimeError makes more sense?

→ Please address this with code or words.

I'm going to stick with TypeError, actually, because that is the exception that Python raises whenever there is a mismatch between the expected arguments and the passed arguments:

>>> def foo(x, y):
...     pass
... 
>>> foo(a=1, b=2)
Traceback (most recent call last):
  File "<input>", line 1, in <module>
TypeError: foo() got an unexpected keyword argument 'a'
>>> foo(5, 6, 7)
Traceback (most recent call last):
  File "<input>", line 1, in <module>
TypeError: foo() takes exactly 2 arguments (3 given)

RuntimeError seems to be reserved for much more severe failings internally in the Python runtime, not things that you see because you passed the wrong thing.

(IMHO this is kind of a bad exception for Python to have chosen, and it should have been a lot more specific, and it should include a lot more structured information but this is definitely not the place to try to change that distinction.)

Way forward:

I’ve marked those parts that need to be addressed. Especially the ContextFactory/ClientContextFactory thing.

It depends on your decisions how to address the points I’ve marked whether this needs another review cycle. Don’t worry about long review times though, I want this to land as much as you do.

The changes were pretty minor, so I'll just file a couple of new tickets and then land assuming the buildbots are all relatively happy.

GOOOOOOOOOOOOOOAL

Note: See TracTickets for help on using tickets.