url: improve private IP protection

[half-duplex]

Description

Atop #2282
Fixes #2284

Draft implementation should resolve #2348.

• Document TOCTOU

Checklist

• I have read CONTRIBUTING.md
• I can and do license this contribution under the EFLv2
• No issues are reported by make qa (runs make quality and make test)
• I have tested the functionality of the things this change touches

[half-duplex]

• Passing bot.memory to find_title feels weird, but it's needed for the safety checks, which need to be done each redirect
• Both methods of getting the IPs for a hostname ignore whether a system has working IPv6. This is a really annoying problem I didn't foresee.
  • Check for a global IPv6 address? (But some working v6 doesn't have that)
  • Check for IPv6 routing? (Feels dirty
  • If TTL is over a second or two, assume it'll be cached and accept the edge case TOCTOUs?
    • Needs fallback. Fall back to only supporting IPv4? Gross, but only needed when TTL<2, so would work 99.999% of the time.

[dgw]

With #2282 merged, presumably this can proceed toward Ready status after a rebase.

Any idea why this affects VCR tests? Somehow it is causing the example request for google.com to appear as a direct IP instead.

[half-duplex]

If the test checks for the address used in the requests.get call, it'll be getting something different from what it used to in some cases. I figured I'd look into that after seeing if I'm even continuing with this redirect handling strategy because of the above questions.

[dgw]

Assuming this PR graduates from draft status, please remove dnspython from requirements. These changes remove its use from url.py (🥳), and nothing else uses it.

[half-duplex]

Rebased, but still looking for feedback on the above questions.

[Exirel]

accept the edge case TOCTOUs

Yes. With your work in particular, Sopel tries a lot of thing to mitigate the attack surface, which is good enough in my opinion. If someone has a server where you can mess it up with a GET request, or that leaks sensible data if you access a URL without authentication, then it's bound to happen, Sopel or not.

And you are doing a pretty good job at trying to prevent that, so kuddo!

[dgw]

@half-duplex Did you still want to get this in? Haven't seen any activity for a while.

[dgw]

Revisited this a bit today on IRC, especially as regards the unresolved TOCTOU problem:

17:56:31 <+mal> the toctou issue cannot be fixed without some invasive stuff, iirc
17:56:50 <+mal> so if anyone cares to actually try to bypass the local IP protection, they will
17:57:20 <+mal> the feature, unless I do the invasive stuff, can only protect against lazy attackers and
                unintended attempts
17:58:26 <+dgw> I think xrl gave a good perspective in
                https://github.com/sopel-irc/sopel/pull/2285#issuecomment-1183750062
17:58:52 <+mal> yeah, fair
17:59:23 <+dgw> This kinda falls under the idea that "Sopel is a toy; don't use it to guard your nuclear
                launch codes"
17:59:27 <+xrl> That's also why no one should ever have a internal system not locked behind some sort
                of authentication.
18:00:02 <+mal> dgw: in my mind, either the feature should Work Properly™ or it should have clear
                documentation about its threat model
18:00:36 <+dgw> clear documentation would be fine; we can totally push some of the netsec stuff
                onto the bot owner, if they're running it someplace it can access LAN resources
18:00:43 <+mal> yeah

[half-duplex]

For when I look at this next: requests/toolbelt#293 was merged

[half-duplex]

Some dns tests:
http://lo.4.sopel.sec.gd/
http://net.4.sopel.sec.gd/
http://net2.4.sopel.sec.gd/
http://lo.6.sopel.sec.gd/
http://lo4.6.sopel.sec.gd/
http://ula.6.sopel.sec.gd/
http://ll.6.sopel.sec.gd/

[dgw]

Since @half-duplex reminded us this exists during another (unrelated) IRC discussion today, I came by to give it a look. Nothing jumps out at me, but I will defer final approval to @Exirel as the original reviewer who requested changes.

I'm excited at getting rid of dnspython as a dependency, though. 😁

[dgw]

I'm excited at getting rid of dnspython as a dependency, though. 😁

Even more so now that dnspython 2.7.0 has broken type-checks on master (see #2594 (comment)). Python 3.8 isn't supported in the new version, so that CI job proceeded past the lint step to test before getting canceled when a sibling failed. I also confirmed myself, make lint succeeds on master with dnspython 2.6 and fails with 2.7.

@Exirel when you've some time to look at this again, it'd be much appreciated 😸

[Exirel]

Good riddance @ dnspython!