Opened 4 years ago

Closed 6 months ago

#4888 defect closed fixed (fixed)

Hostname verification for HTTPS URIs in twisted.web.client.Agent.

Reported by: glyph Owned by: glyph
Priority: normal Milestone: Twisted-14.0.0
Component: web Keywords:
Cc: ivank-twisted-bugs@…, hs@…, alex.gaynor@…, Julian Branch: branches/redux-4888
(diff, github, buildbot, log)
Author: hynek, itamarst, dreid, glyph Launchpad Bug:

Description

When we implemented HTTPS for the new client API, we left out any form of hostname verification or certificate checking.

We should support it in some way.

Change History (63)

comment:1 Changed 4 years ago by glyph

  • Component changed from core to web

First we need to support just checking that the hostname matches as expected. Does Agent do that already? Then, we need to actually verify the peer's certificate.

My inclination would be, by default, to support it opportunistically, verifying when possible, emitting a warning when it's not possible.

There should be separate tickets for supporting each of: the MacOS X certificate store, the Linux conventional default (/etc/ssl/certs) , and the Windows thing, which there are all references to on #4023. This ticket could probably be for the Linux default. Or, maybe just for hostname checking against the (unverified) cert, if we don't do that yet.

There should be some kind of option, which says "for real, be secure, don't transmit my request in the clear, don't accept a response in the clear". In my opinion, this flag should be specified independently of the protocol, since maybe there could be other transports besides HTTPS that could be secure. Unfortunately for compatibility, this is probably what users mean when they take the care to type "https://" into a box that takes an URL.

I expect that there will be some further discussion first though. Thoughts?

comment:2 Changed 3 years ago by exarkun

Or, maybe just for hostname checking against the (unverified) cert, if we don't do that yet.

I think this is a good idea. It's not terribly useful to verify hostnames without verifying certificates, but the implementation of those two things are fairly independent.

comment:3 Changed 3 years ago by glyph

There's a relevant RFC, 6125 that an implementor of this feature should be aware of.

comment:4 Changed 3 years ago by glyph

See also #5446 and #5445 for related tasks.

comment:5 Changed 3 years ago by ivank

  • Cc ivank-twisted-bugs@… added

comment:6 Changed 21 months ago by itamarst

  • Author set to itamarst
  • Branch set to branches/hostname-verification-4888

(In [37200]) Branching to 'hostname-verification-4888'

comment:7 follow-up: Changed 20 months ago by radix

This is a good feature, but we shouldn't have our own certificate database.

  • It's a liability to us in that we'd have to maintain it, even if we generate it from mozilla's DB. curl maintainers learned this the hard way. so are python-requests maintainers.

and it's a liability to our users:

  • the certificate store would become out of date (probably without them even knowing it)
  • administrators don't have management tools that work with this database included in Twisted

You should load certificates out of /etc/ssl/certs if it exists. If you want to solve this for users, then maybe include certs in the windows .exe distribution, but this shouldn't go into Twisted upstream.

comment:8 follow-up: Changed 20 months ago by radix

On top of that, why is the CA bundle being added in this branch? it seems like it should be a part of a different ticket and not be bound to HTTPS hostname verification.

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

Replying to radix:

On top of that, why is the CA bundle being added in this branch? it seems like it should be a part of a different ticket and not be bound to HTTPS hostname verification.

+1 to that. This should be split into as many discrete actions as possible, and shipping a cert bundle is obviously the most contentious.

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

Replying to radix:

This is a good feature, but we shouldn't have our own certificate database.

  • It's a liability to us in that we'd have to maintain it, even if we generate it from mozilla's DB. curl maintainers learned this the hard way. so are python-requests maintainers.

and it's a liability to our users:

  • the certificate store would become out of date (probably without them even knowing it)
  • administrators don't have management tools that work with this database included in Twisted

You should load certificates out of /etc/ssl/certs if it exists. If you want to solve this for users, then maybe include certs in the windows .exe distribution, but this shouldn't go into Twisted upstream.

I mostly agree, except for a few minor points:

Let's not the perfect be the enemy of the good here. We should not be in the cert-bundle shipping business if we can avoid it, but an out-of-date CA bundle is still better security in many cases than having to fall back to cleartext. For example, pip's reluctance to ship a CA bundle lead to years of everyone just downloading and executing stuff plaintext because security was too hard. It would have been a lot better for everyone if they - and we - had just shipped a CA bundle that wasn't getting updates and then worked to figure out a way to update it later. Thanks for the prompting though, because I noticed that the ticket for looking at the platform certificate store didn't actually have any API references on it and therefore it might not have been as clear as it could have what I was proposing there.

comment:11 Changed 20 months ago by glyph

As to the implementation in the branch itself; looking at the function implemented for matching certificate names, I'm wondering why we have one function for extracting the names and one function for matching the names, rather than a matcher object that wraps the certificate object.

For example, RFC 6125 seems very cagey on the concept of wildcard certificates, and wish that they weren't a thing, conceding that support is necessary only for "backward compatibility with deployed infrastructure". It seems quite likely to me that some future X509 certificate or certificate authority metadata would add a field saying "no wildcards please" or otherwise modify the matching algorithm, based on some information that can't be represented in a list of certificate names (plus wildcards) represented by the certificate. I would like this API to be as abstract – and therefore as future-proof – as possible, and so have an API which is just like "here's an object, it matches a hostname or it doesn't, don't worry about how".

(Also always include epytext @type fields on all parameters, especially parameters representing strings of characters or bytes.)

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

I should note that I'm doing this for one of my clients; this branch is just my way of dumping my first pass somewhere public. My plan therefore is to first get this to the point where it works, even if it's not mergeable. Later, in my free time or with the help of others, I will split this up into multiple branches as necessary, address review comments, write documentation etc..

comment:13 Changed 20 months ago by itamar

As far as shipping CA bundle: it's not ideal, but it's better than nothing (as glyph noted in the pip example.) My current thoughts are that we should use the platform cert database if possible, falling back to a bundled cert database that we refresh on every release (if not more frequenly). Initially it'd probably be easiest to only support some set of Linux distributions, but we could add better defaults for other platforms over time.

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

Replying to itamar:

I should note that I'm doing this for one of my clients; this branch is just my way of dumping my first pass somewhere public. My plan therefore is to first get this to the point where it works, even if it's not mergeable. Later, in my free time or with the help of others, I will split this up into multiple branches as necessary, address review comments, write documentation etc..

I thought something like this might be the case, since you seem to be loading up the branch with lots of code :). I'm offering comments now so there isn't one gigantic dump of review feedback at the end - and I thank radix for doing the same. Also, this is security code, and benefits from extra eyeballs even more than usual.

On that note, regarding another changeset:

  1. getCACertificates is a crappy name, in no small part because it starts with "get". Call it authorityCertificates or certificateAuthorityCertificates or trustedRoots or something.
  2. This shouldn't return X509 objects directly. They probably segfault less than we're used to, but we already have a perfectly reasonable wrapper, twisted.internet.ssl.Certificate. Return one of those; if you need to turn it back into an X509, you can.
  3. Oh my goodness, do not tell distributors to patch a function! Make a plugin, aggregate the certificates from the plugins.
  4. Loading multiple certificates from a single file is independently useful of loading platform CAs; in particular I believe this could be helpful when loading trust chains. That code should be in a different function. Certificate.manyFromFile perhaps.

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

For example, pip's reluctance to ship a CA bundle lead to years of everyone just downloading and executing stuff plaintext because security was too hard. It would have been a lot better for everyone if they - and we - had just shipped a CA bundle that wasn't getting updates and then worked to figure out a way to update it later.

I don't think we should actually be in the business of managing and distributing such a bundle, but it sounds like enough other people are that it'll probably happen.

So. Since apparently pip, python-requests, and Twisted should all have been bundling their own private CA databases for years now, how about creating a new, shared, independent package that consists only of such a bundle and can be depended on by all of these projects? Hopefully the numerous advantages of such an approach over Twisted (and each of those other projects, perhaps plus others) are obvious, but if not:

  • It can be updated more frequently than Twisted itself is updated. This is fairly important. If you want to stop trusting a compromised authority, you want to stop trusted them today, not three months from now.
  • It provides consistent trust behavior across multiple projects. It is better if all HTTPS software on a host comes to the same conclusion about a particular certificate (ie, the system provides a CA database). Failing that, the more software agrees the better. Having Twisted and pip always agree is certainly preferable to allowing them to use desynchronized CA databases and come to different conclusions about potentially compromised hosts.
  • It is less work. It is one database to maintain and distribute instead of N.
  • There are more people to help with the work. Many more people use and care about pip than use and care about Twisted.
  • Platform vendors could potentially someday be some of the people helping. For example, Ubuntu could start maintaining an Ubuntu-specific version of this package which just points at the exact same certificates as are included in the ca-certificates package.

comment:16 in reply to: ↑ 15 Changed 20 months ago by glyph

Replying to exarkun:

I don't think we should actually be in the business of managing and distributing such a bundle, but it sounds like enough other people are that it'll probably happen.

I would prefer to avoid this as well, and you make some good points. And we haven't even started talking about licensing issues! But, in any case, as we keep saying, this ticket is not about including a CA bundle or in fact any kind of trust configuration, but about hostname matching, so we should stop filling up the comments here. I have opened a new ticket specifically about this problem, described in such a way to make my feelings clear (i.e. we should not do it, I'm not entirely convinced that the ticket for fixing this the right way is actually sufficiently harder than resolving all these issues).

comment:17 Changed 20 months ago by glyph

PLATFORM = object() is pretty crummy. At the very least, there's no excuse for object() any more because we now have twisted.python.constants, and you can have a symbolic placeholder that actually has a nice stringification.

But, it would also be handy to say "the platform, plus some additional trusted roots that I added myself". Given that you're enumerating the full list of platform CA certs anyway, why not provide an object that offers at least an __iter__ if not a plain list of certs?

comment:18 Changed 20 months ago by glyph

"standard" security rules and "well known" certificate authorities are both kind of weasel words, too. Since there's no actual standard governing which CAs to use, maybe "BrowserLikeContextFactory" would be a better name for it? (LockIconContextFactory, ha ha?)

comment:19 Changed 18 months ago by dreid

Ok, this work actually looks pretty great. But I couldn't help but notice that StandardWebContextFactory does not:

  1. Document the level of verification it does (it only implies that it is similar to browsers) this makes making an informed decision about if the level of verification it does perform is suitable for your application just a little bit harder.
  2. Provide any mechanism for specifying the caCerts. Seems like StandardWebContextFactory.__init__ sould take a caCerts keyword argument and pass it to OpenSSLCertificateOptions with it defaulting to PLATFORM.

comment:20 Changed 17 months ago by dreid

(In [38700]) BrowserLikeContextFactory instead. Refs #4888

comment:21 Changed 17 months ago by itamar

Note to anyone doing more work on this: some of the code needs to be moved to other tickets' branches. In particular the CA bundle packaging should be moved to #6334. #5446 covers using openssl's knowledge of CA database on POSIX systems, and there's a couple of tickets for using OS X and Windows native CAs.

comment:22 Changed 12 months ago by hynek

  • Cc hs@… added

comment:23 Changed 9 months ago by hynek

  • Author changed from itamarst to hynek, itamarst
  • Branch changed from branches/hostname-verification-4888 to branches/hostname-verification-4888-2

(In [41550]) Branching to hostname-verification-4888-2.

comment:24 Changed 9 months ago by hynek

(In [41557]) Merge forward hostname verification for CertificateOptions

refs #4888

comment:25 Changed 9 months ago by hynek

FYI, the implementation/merge of the Agent features is completely blocked by #5446.

comment:26 Changed 9 months ago by hynek

Another FYI, we’ll need our buildbots to install with pyasn1 and pyasn1_modules for the test suite…that is a per buildbot setting, right? Ought I send an email to the mailing list?

comment:27 Changed 9 months ago by hynek

So it occurred to me how to make this branch a) better and b) unblock this ticket.

  1. Long story short, I want BrowserLikeContextFactory to depend on certifi. It is called BrowserLike, what would it make more browser-like than using the CAs from an actual browser?
  2. Also slightly unrelatedly, I dislike the current API of passing the hostname into Certificate options, I would change it into adding an instance method called .withHostnameValidationFor('hostname.com') which would copy the current CF and add the validation.

Anyone feeling the urge to stop me from either? While 1. was born out of desperation, I like it a lot. Using system CAs for web validation is asking for pain (just today I was unable to wget something off my page which has a perfectly valid SSL config including chain from an older FreeBSD box).

If there’s consensus that my approach to 1. is wrong, I’m hereby arguing against calling the new Agent-CF “BrowserLike”, because it isn’t. I also don’t consider waiting for #6934 to implement this an option; I’ll personally port this code to the new APIs it might introduce, but don’t want to wait anymore.

Looking forward to your feedback.

comment:28 Changed 9 months ago by hynek

Let me quickly show you the API I have in mind:

class BrowserLikeContextFactory(object):
    def __init__(self):
        from twisted.internet._sslverify import OpenSSLCertificateOptions
        self._cf = OpenSSLCertificateOptions(verify=True,
                                             caCerts=loadCertifiCAs())


    def getContext(self, hostname, port):
        return self._cf.withHostnameVerificationFor(hostname, port).getContext()

I do know that the amount of Factories is identical but I like the API a lot more and it gives some wiggle room for optimizations.

comment:29 Changed 9 months ago by hynek

Sorry i forgot, please compare to https://github.com/twisted/twisted/blob/27b4422c2460937c84457a08ee7d7e3324d3d28e/twisted/web/client.py#L718 – the old branch is a bit laborious to find.

comment:30 Changed 9 months ago by dreid

What about just adding this to WebContextFactory instead of a new object? (We do still want this to be the default right?)

What specifically do you dislike about constructing CertificateOptions in getContext?

If we had an API like you propose would we need the WebContextFactory where getContext takes a hostname at all?

comment:31 Changed 9 months ago by hynek

  • Can that ever happen?
  • I’m totally giving up on this because I don’t want to delay this ticket any further (I find that API gross non-withstanding).
  • I’m sure I understand your last question. I think different behaviors will always be necessary. Once #5446 is close, we could add a SystemTrustWebContextFactory. The current WebContextFactory should be deprecated and renamed to “InsecureWebContextFactoryDoNoUse” instead. Does my response fit your question? :)

Do you have any feels about my certifi idea? If it gets smashed down, I think I’ll publish something like that on PyPI.

comment:32 Changed 9 months ago by radix

I disagree with the idea of making BrowserLikeContextFactory *depend* on certifi. I think it should default to the platform's SSL database.

Certifi is worse than the platform's certificates and worse than a browser's certificates, because it has no sensible way to do certificate management by an operator of the system. It should just be a fallback. OS X, Windows, and Ubuntu all provide tools or standard methods for manipulating the certificates for your platform. Those tools should all affect the behavior of Twisted applications.

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

Well the goal is to do the right thing for people who can’t/won’t do all that by default (cf. discussion about hostname validation in Python 3 and Ruby’s standard SSL settings).

System trust stores are terrible and just lead to disabling verification. I literally had to run wget with “--no-check-certificate” just today because FreeNAS/FreeBSD has an outdated/none? trust store and it wouldn’t connect to a perfectly valid SSL host. As soon as #5446 gives us the tools, we can add a factory for those who care about such things but I’d like to be secure by default.

comment:34 in reply to: ↑ 33 Changed 9 months ago by glyph

Replying to hynek:

Well the goal is to do the right thing for people who can’t/won’t do all that by default (cf. discussion about hostname validation in Python 3 and Ruby’s standard SSL settings).

+1, we all agree this is a good idea.

System trust stores are terrible and just lead to disabling verification.

This is a reason to fix the system trust store, not for every application to become an ad-hoc provisioner of CA trust.

I literally had to run wget with “--no-check-certificate” just today because FreeNAS/FreeBSD has an outdated/none? trust store and it wouldn’t connect to a perfectly valid SSL host.

Maybe your "SSL host" was signed by TURKTRUST and the platform isn't trusting it on purpose! Maybe your host is configured to only trust intranet sites. The point is that we must leave this in a user's hands or we are effectively trying to be a CA ourselves.

As soon as #5446 gives us the tools, we can add a factory for those who care about such things but I’d like to be secure by default.

So, the subject of this ticket is "hostname verification". I understand the need for a complete system of interlocking mechanisms to establish a real trust path, but that's not the best way to deal with implementation. Can this ticket, or a dedicated sub-ticket, please limit itself to implementing just the rfc6125 rules, checking the host name and SANs and whatnot (given the need for ASN.1 parsing, that seems more than hard enough for one ticket), and leave all the unpleasant stickiness of trust rules to #5446 and its potential successors? This ticket really doesn't need any trust stuff in it in order to fulfill its mission.

comment:35 Changed 9 months ago by glyph

As it happens, there's already a ticket, #5190. That could be just a function that verifies an issuer and returns True or False.

comment:36 Changed 7 months ago by glyph

Before landing, this will also require #6024, because that's the hook which will implement the appropriate place to do the verification.

comment:37 Changed 7 months ago by dreid

  • Author changed from hynek, itamarst to hynek, itamarst, dreid
  • Branch changed from branches/hostname-verification-4888-2 to branches/agent-contextFactory-hostname-4888

(In [42121]) Branching to agent-contextFactory-hostname-4888.

comment:38 Changed 7 months ago by glyph

  • Type changed from enhancement to defect

I'm reclassifying this as a defect. In this case, I think the classification is actually pretty important, because if this were an "enhancement" then it should be implemented in a "compatible" way - i.e. just keep not verifying certificates for HTTPS servers unless you specifically ask for it. As it is I think it falls under the "gross violation of specifications" clause of the compatibility policy, although it's in a bit of a grey area because it possibly fails the "published" part of the published specification test (hostname verification is RFC6125, but as far as I know there's no specific RFC specifying the trust cartel), so I will expound. Perhaps there should be a separate explicit exception for security issues.

I think that it is conventionally well-understood that when a user types an https URI, either an application programmer typing it somewhere in their code, or a user entering it on the command line, they have the expectation that the communication will be verified with traditional CA-cartel trust. More specialized applications might have stricter trust, but it's misleading to have https in a URL that is totally unverified.

To be clear, this is an explicit repudiation of my earlier comment:

There should be some kind of option, which says "for real, be secure, don't transmit my request in the clear, don't accept a response in the clear".

There shouldn't be an option. If anything, we should backport this to earlier point releases of Twisted, as it's a significant security fix; by allowing totally untrusted, unverified https connections to masquerade as "secure" from the users's perspective, I think that we were propagating a common security bug.

Thanks to all the security people who have taken the time to educate me on this issue, and who have help me understand the operational value of cartel trust despite the fact that it's not perfect.

comment:39 Changed 7 months ago by Alex

  • Cc alex.gaynor@… added

comment:40 Changed 7 months ago by glyph

  • Keywords review added

Please review #7098 and see the comment there. I can split up the changes if necessary.

comment:41 Changed 6 months ago by glyph

  • Author changed from hynek, itamarst, dreid to hynek, itamarst, dreid, glyph
  • Branch changed from branches/agent-contextFactory-hostname-4888 to branches/redux-4888

(In [42512]) Branching to redux-4888.

comment:42 Changed 6 months ago by glyph

Okay uh I guess this was already in review but now it's actually in review. Buildbot tells me there are some docstring fixes left to do, and a test skip thing, but other than that, would a reviewer like to opine?

comment:43 follow-up: Changed 6 months ago by Alex

The docstring for creatorForNetloc includes a parameters which doesn't exist. Also, the port parameter is unused, that's ok? Since this is a new interface, is there any reason to include it? The interface contextFactory is not documented, is passing something besides WebClientConnectionCreatorCreator supported?

comment:44 follow-up: Changed 6 months ago by rwall

A few observations from playing with the latest branch.

  1. What if you want to ignore the hostname mismatch in the certificate. ie to download buildbot.twistedmatrix.com pages.
  1. And if people currently have scripts which rely on Agent's lack of hostname verification, won't it be a surprise if those suddenly start raising VerificationError.
  1. I also note that the VerificationError reported is in a private service_identity module and wrapped in another private web failure. Which makes it very difficult to handle and very difficult to work out which public exception to trap in an errback.
(Twisted)[richard@zorin redux-4888]$ python docs/web/examples/https.py https://buildbot.twistedmatrix.com/json/builders/?as_text=1
main function encountered error
Traceback (most recent call last):
Failure: twisted.web._newclient.ResponseNeverReceived: [<twisted.python.failure.Failure <class 'service_identity._common.VerificationError'>>]

Is there any way to make that refer to the proper public APIs

comment:45 in reply to: ↑ 44 Changed 6 months ago by glyph

Replying to rwall:

A few observations from playing with the latest branch.

  1. What if you want to ignore the hostname mismatch in the certificate. ie to download buildbot.twistedmatrix.com pages.
  1. Don't do that.
  2. Seriously, don't do that. Tom should fix buildbot. it's broken. I made him a cert, like, a year ago, and maybe the fact that we will care that it's broken might get us to fix it sooner. (Actually he's probably going to fix it as soon as he gets back from vacation.)
  3. Make your own CreatorCreator which explicitly does the hostname translation. Buildbot has a valid cert, it's just for the wrong domain; so pretend you're connecting to twistedmatrix.com at the SSL layer.
  4. Use plaintext HTTP.
  5. Did I mention you shouldn't do that?
  6. Make your own CreatorCreator which just returns a thing which creates a CertificateOptions and doesn't bother to verify anything. And then feel bad because, as I mentioned, you shouldn't have done that.
  1. And if people currently have scripts which rely on Agent's lack of hostname verification, won't it be a surprise if those suddenly start raising VerificationError.

And I'm sure there were lots of great bits of automation for reading the core memory of other servers on the internet that were all broken by the fact that we patched heartbleed. Breaking this kind of stuff is sort of the point of having security.

  1. I also note that the VerificationError reported is in a private service_identity module and wrapped in another private web failure. Which makes it very difficult to handle and very difficult to work out which public exception to trap in an errback.

...

Is there any way to make that refer to the proper public APIs

Yeah, this is a general badness of Agent, not really related to this change. See #5310 for fixing it.

Thanks for the feedback. Looks like I'm nothing going to deal with in this batch, but it's good to make it explicit. Looking forward to a real review though ;).

comment:46 in reply to: ↑ 43 Changed 6 months ago by glyph

Replying to Alex:

The docstring for creatorForNetloc includes a parameters which doesn't exist. Also, the port parameter is unused, that's ok? Since this is a new interface, is there any reason to include it? The interface contextFactory is not documented, is passing something besides WebClientConnectionCreatorCreator supported?

Good points. I'll fix those in the branch.

Anyone want to do a real review though? :)

comment:47 follow-up: Changed 6 months ago by hynek

FWIW, I agree with Rich we should ship a IAcknowledgeThatIWillBeMITMedByUsingThisCreatorCreator for internal systems etc. We don’t want to add barriers for using Twisted in internal/legacy systems because that’s where it often shines.

comment:48 follow-up: Changed 6 months ago by exarkun

Don't do that.

"Sorry you can't interoperate with a random, large subset of the stupidly misconfigured part of the internet (by the way this is so hard to do *we* don't even do it right for *our* part of the internet)" sounds kind of lame to me.

comment:49 in reply to: ↑ 48 Changed 6 months ago by glyph

Replying to exarkun:

Don't do that.

"Sorry you can't interoperate with a random, large subset of the stupidly misconfigured part of the internet

I never said that. What "don't do that" means is that you shouldn't interoperate with that misconfigured part of the internet; that's what "misconfigured" means, after all ;-).

Step 3 is the "right" answer here. Once you know what hostname and certificate you're expecting, you can easily configure Twisted to verify that; it's just slightly harder (but still possible!) to tell it "don't verify this at all, I don't care about security at all".

(by the way this is so hard to do *we* don't even do it right for *our* part of the internet)"

The difficulty of maintaining Twisted's infrastructure is not a great yardstick for the ease of maintainer infrastructure generally. If this were the only example my only answer would have been "fix your stuff, like we should fix ours"; since I know other people do also make this mistake, there are other options.

sounds kind of lame to me.

There are plenty of options, but they should be slightly hard to discover, because we really really do not want anyone doing this by accident. The first recourse here should always be to fix the infrastructure, and only once that's definitively not an option should you insert workarounds and hacks.

comment:50 in reply to: ↑ 47 Changed 6 months ago by glyph

Replying to hynek:

FWIW, I agree with Rich we should ship a IAcknowledgeThatIWillBeMITMedByUsingThisCreatorCreator for internal systems etc. We don’t want to add barriers for using Twisted in internal/legacy systems because that’s where it often shines.

If you want to add this, I strongly think it should be a separate ticket; the infrastructure is all present here to implement such a thing.

If it were possible to imply to someone using documentation or naming that a thing was ugly and shouldn't be used (hi there, InMemoryUsernamePasswordDatabaseDontUse) then what I'd say is that we should make it an example and not expose it as a public name anywhere, and actually require people to copy and paste the no-verification thing into their own code. (But what that'll probably do is just make people copy a buggy example we can't ship security updates for, so...)

comment:51 Changed 6 months ago by glyph

Also for those of you concerned about ...CreatorCreator, I think I'm going to rename that thing to HTTPSPolicy.

comment:52 Changed 6 months ago by rwall

  • Keywords review removed
  • Owner set to glyph

Glyph, here's a quick and dirty review while I wait for a flight. I'll try and make some more thorough notes later (unless someone gets there first)

Notes:

Points:

  1. source:branches/redux-4888/twisted/web/client.py
    1. Missing epydocs: https://buildbot.twistedmatrix.com/builders/twistedchecker/builds/2047/steps/run-twistedchecker/logs/new%20twistedchecker%20errors
    2. The CreatorCreator suffix seems very confusing (as noted earlier) your suggested name seems better.
  1. source:branches/redux-4888/twisted/web/test/test_agent.py
    1. Build Failures due to OpenSSL import errors - please fix
      1. https://buildbot.twistedmatrix.com/builders/debian-easy-no-zope-py2.6-epoll/builds/1862/steps/epoll/logs/problems
      2. https://buildbot.twistedmatrix.com/builders/debian-easy-old-zope-py2.6-epoll/builds/1857/steps/epoll/logs/problems
      3. https://buildbot.twistedmatrix.com/builders/py-without-modules/builds/2456/steps/trial/logs/problems
      4. https://buildbot.twistedmatrix.com/builders/windows7-64-py2.7/builds/3872/steps/iocp/logs/problems
    2. Missing epydocs - please fix the important ones: https://buildbot.twistedmatrix.com/builders/twistedchecker/builds/2047/steps/run-twistedchecker/logs/new%20twistedchecker%20errors
    3. Add a test for the deprecation of t.w.c.WebClientContextFactory
  2. Missing news file

Please answer or address the numbered points above and resubmit for review.

[ERROR]
Traceback (most recent call last):
  File "/home/buildslave/slaves/twisted/bot-idnar-debian64/easy-old-zope-2.6debian/venv/lib/python2.6/site-packages/Twisted-13.2.0-py2.6-linux-x86_64.egg/twisted/trial/runner.py", line 497, in loadPackage
    module = modinfo.load()
  File "/home/buildslave/slaves/twisted/bot-idnar-debian64/easy-old-zope-2.6debian/venv/lib/python2.6/site-packages/Twisted-13.2.0-py2.6-linux-x86_64.egg/twisted/python/modules.py", line 383, in load
    return self.pathEntry.pythonPath.moduleLoader(self.name)
  File "/home/buildslave/slaves/twisted/bot-idnar-debian64/easy-old-zope-2.6debian/venv/lib/python2.6/site-packages/Twisted-13.2.0-py2.6-linux-x86_64.egg/twisted/python/reflect.py", line 303, in namedAny
    topLevelPackage = _importAndCheckStack(trialname)
  File "/home/buildslave/slaves/twisted/bot-idnar-debian64/easy-old-zope-2.6debian/venv/lib/python2.6/site-packages/Twisted-13.2.0-py2.6-linux-x86_64.egg/twisted/python/reflect.py", line 250, in _importAndCheckStack
    reraise(excValue, excTraceback)
  File "/home/buildslave/slaves/twisted/bot-idnar-debian64/easy-old-zope-2.6debian/venv/lib/python2.6/site-packages/Twisted-13.2.0-py2.6-linux-x86_64.egg/twisted/web/test/test_agent.py", line 40, in <module>
    from twisted.internet._sslverify import ClientTLSOptions
  File "/home/buildslave/slaves/twisted/bot-idnar-debian64/easy-old-zope-2.6debian/venv/lib/python2.6/site-packages/Twisted-13.2.0-py2.6-linux-x86_64.egg/twisted/internet/_sslverify.py", line 13, in <module>
    from OpenSSL import SSL, crypto, version
exceptions.ImportError: No module named OpenSSL
}}

comment:53 Changed 6 months ago by glyph

Thanks, rwall!

comment:54 Changed 6 months ago by Julian

  • Cc Julian added

comment:55 Changed 6 months ago by glyph

(In [42535]) Fix deprecation warning strings.

refs #4888
refs #7242

comment:56 Changed 6 months ago by glyph

  • Keywords review added
  • Owner glyph deleted

Replying to rwall:

Glyph, here's a quick and dirty review while I wait for a flight. I'll try and make some more thorough notes later (unless someone gets there first)

Points:

  1. source:branches/redux-4888/twisted/web/client.py
    1. Missing epydocs: https://buildbot.twistedmatrix.com/builders/twistedchecker/builds/2047/steps/run-twistedchecker/logs/new%20twistedchecker%20errors

Aside from these, there are spurious twistedchecker errors yet again.

I filed https://github.com/twisted/twistedchecker/issues/76 for set_connect_state (which is the only unfixable one) and https://github.com/twisted/twistedchecker/issues/75 for the inner-class stuff which I've worked around.

  1. The CreatorCreator suffix seems very confusing (as noted earlier) your suggested name seems better.

Right-o. Name changed.

  1. source:branches/redux-4888/twisted/web/test/test_agent.py
    1. Build Failures due to OpenSSL import errors - please fix
      1. https://buildbot.twistedmatrix.com/builders/debian-easy-no-zope-py2.6-epoll/builds/1862/steps/epoll/logs/problems
      2. https://buildbot.twistedmatrix.com/builders/debian-easy-old-zope-py2.6-epoll/builds/1857/steps/epoll/logs/problems
      3. https://buildbot.twistedmatrix.com/builders/py-without-modules/builds/2456/steps/trial/logs/problems
      4. https://buildbot.twistedmatrix.com/builders/windows7-64-py2.7/builds/3872/steps/iocp/logs/problems

These all seem to be the same failure. Perhaps more of our build farm should have pyOpenSSL installed? There's really just supposed to be the one builder without optional dependencies, I thought.

  1. Missing epydocs - please fix the important ones: https://buildbot.twistedmatrix.com/builders/twistedchecker/builds/2047/steps/run-twistedchecker/logs/new%20twistedchecker%20errors

I think these should all be fixed now, important or not :).

  1. Add a test for the deprecation of t.w.c.WebClientContextFactory

Hmm. While I'm at it, I think this should probably be using deprecatedModuleAttribute, actually; at least until the tickets referenced by ticket:4804#comment:15 are addressed.

And then when I tried to do the right thing here I discovered #7242 as well, and filed it; you can see the comment in the test code.

  1. Missing news file

Added.

I also added a formal interface because all these silly half-finished
looks-kinda-like-this-one-weird-class types are a bad idea and I should stop
putting them into public interfaces.

Last time when I put it up for review I knew it was in kinda bad shape and I
wanted feedback before putting more effort in; this time I think it's probably
ready to go.

I also filed #7243 for other APIs. Hopefully that's a duplicate of something?

comment:57 Changed 6 months ago by exarkun

  • Type changed from defect to regression

It seems this ticket has been de facto promoted to a regression (it is blocking the 14.0 release because ... "reasons" ... I don't know). Adjusting metadata to reflect this unfortunate reality.

comment:58 Changed 6 months ago by glyph

  • Milestone set to Twisted-14.0.0
  • Type changed from regression to defect

While I'd like this to get into the 14.0 release (we're so close, and this is such an important fix!) this does not actually need to block it. If necessary we can have a quick follow-up 14.1 that closes this issue - which was the original plan anyway.

#7098 did, because it involved a pretty significant re-working of an unreleased public interface, rolling back and re-implementing would have been a ton of additional work, and it changed around a bunch of public exposure of guts which had already landed on trunk which I did not want us to have to live with long term. But that's closed now and nothing in the branch for this ticket needs to change any of those interfaces, so we're OK to have a release that does not include this.

Randomly changing around metadata is fun so I've done that as well. I've put it in the milestone in case we do want to block but I am explicitly leaving this at hawkowl's discretion to wait on or not.

comment:59 Changed 6 months ago by dreid

From 4888.bugfix

twisted.web.client.Agent now verifies 'https' URLs against (its best guess of)
the platform trust roots.

According to the previous docstring for WebClientContextFactory it was already doing basic certificate checking by passing platformRoot as trustRoot. The news fragment should probably mention what this branch actually does which is hostname
verification.

comment:60 follow-up: Changed 6 months ago by Alex

  • Keywords review removed
  • Owner set to glyph

Review:

  • _ContextFactoryWithContext this class should not be created as a closure, it should be a normal top level class which takes a parameter to its constructor. This will be considerably faster on both PyPy and CPython, as well as requiring less memory.
  • A L{HTTPConnectionPool} this should be "an"
  • contextFactory = _DeprecatedToCurrentPolicyForHTTPS(contextFactory) a deprecation warning should be emitted when this is hit.

Please fix these items, and then it looks good to me.

comment:61 in reply to: ↑ 60 Changed 6 months ago by glyph

Replying to Alex:

Review:

  • _ContextFactoryWithContext this class should not be created as a closure, it should be a normal top level class which takes a parameter to its constructor. This will be considerably faster on both PyPy and CPython, as well as requiring less memory.

Sigh, where are JavaScript object literals when you need them…

Sure.

  • A L{HTTPConnectionPool} this should be "an"

Good catch. I should probably make it easier for myself to read the generated API docs as I'm writing them.

  • contextFactory = _DeprecatedToCurrentPolicyForHTTPS(contextFactory) a deprecation warning should be emitted when this is hit.

Oh, that's a good point. I deprecated the other thing, but I guess if you provide something that's like a WebClientContextFactory but doesn't actually import a WebClientContextFactory you won't see the deprecation.

Please fix these items, and then it looks good to me.

Thanks for the review.

comment:62 Changed 6 months ago by glyph

OK, feedback dealt with, I think. Double-checking with buildbot right now, then landing.

comment:63 Changed 6 months ago by glyph

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

(In [42546]) Merge redux-4888

Author: glyph, dreid, hynek
Reviewer: alex, rwall
Fixes: #4888

Make https work right.

Note: See TracTickets for help on using tickets.