Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

backends: allow setting TLS version and ciphers #2246

Merged
merged 1 commit into from
Jun 5, 2022

Conversation

half-duplex
Copy link
Member

@half-duplex half-duplex commented Feb 5, 2022

Description

This resolves a TODO comment and two LGTM ignores regarding failing to specify a minimum SSL/TLS version for connections. It also introduces a new core config option, tls_ciphers, since python 3.10 tightens the defaults.

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 half-duplex added this to the 8.0.0 milestone Feb 5, 2022
@half-duplex
Copy link
Member Author

half-duplex commented Feb 5, 2022

I don't know what to flag this, since it moves from feature towards bugfix as 3.10 becomes more widely available. I put it on 8.0 since that's when we drop old-python, but sopel 7 will have these default changes applied anyway by py3.10, with no knobs to dial it back.
This draft has potentially-breaking changes:

  • Sets minimum TLS version to 1.2, even in python versions that don't already do that
  • Sets default cipher suites to sane forward-secret ones, even in python versions that don't already do that

CNAME handling needs to be re-added, since it seems that does exist in the real world (#570). Done. Note that get_cnames is not recursive, the only time it would return a list of more than 1 element is if the FQDN violates DNS standards by having multiple CNAMEs.

RFC: Should I add a knob to change the minimum SSL/TLS version?

@half-duplex half-duplex force-pushed the ssl-wrap-socket branch 2 times, most recently from 37f00eb to 366489c Compare February 5, 2022 23:11
@half-duplex
Copy link
Member Author

Thinking more about it, are CNAMEs used for anything where the user can't just update their config? irc.twitch.tv -> irc.chat.twitch.tv

Copy link
Member

@dgw dgw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've reached my limit of digging through SSL/TLS docs for today.

Some knob to override the minimum TLS version would probably be useful, given that IRC networks often seem to fall behind on encryption support vs. the much more common HTTP services we all use every day. No, I would not suggest making this knob support anything ridiculous like SSLv3.

I know you referenced the shocking number of servers that still don't have any TLS support at all as of December 2021. Presumably TLSv1.2 is a sane default (it enjoys support by 54% of surveyed servers, vs. 56% for "any SSL protocol"), but we'd probably do well to allow people the option to explicitly enable at least TLSv1.1 if their network of choice doesn't support anything newer. I can take or leave TLSv1.0.

sopel/irc/backends.py Outdated Show resolved Hide resolved
sopel/config/core_section.py Outdated Show resolved Hide resolved
@half-duplex
Copy link
Member Author

I don't like storing the min_ver string, but I'm not sure if the validation belongs there, but "invalid version TLSv5" is much more useful than "invalid version None"... 🤔

sopel/config/core_section.py Outdated Show resolved Hide resolved
sopel/config/core_section.py Outdated Show resolved Hide resolved
@dgw
Copy link
Member

dgw commented Feb 6, 2022

Would you agree that putting the ini example code before the .. note would look better? I went to the deploy preview to verify that the TLSVersion xref works properly and, well… there's always something more. 🙈

I wish English had a reliable linter, too.

Copy link
Contributor

@Exirel Exirel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if you can use SSLContext.load_verify_locations instead of doing any manual verification, because those look like a lot of work and I'm not really comfortable with that.

sopel/config/core_section.py Outdated Show resolved Hide resolved
sopel/config/core_section.py Outdated Show resolved Hide resolved
@Exirel
Copy link
Contributor

Exirel commented Feb 9, 2022

I'm OK with the default minimum TLS version. I think it's fair anyway, at least if we target Sopel 8 for that (as we should, I think).

@half-duplex
Copy link
Member Author

I'm not sure what you mean about SSLContext.load_verify_locations - that's only for certs, and we only manually check hostname (and I'm not sure we should be, vs "if you find a cname, update your config").

@Exirel
Copy link
Contributor

Exirel commented Feb 16, 2022

I'm not sure we should be, vs "if you find a cname, update your config").

The more I think about it, the less I understand why Sopel does that. Certificates are not supposed to care about CNAME or anything, they only care about the domain name. If someone has a certificate, they must ensure it's valid for all the domain names they want to use, and that includes A records and CNAME, and any other DNS records anyway.

And yes, I was thinking of just letting the SSLContext do its work and never bother with manual check of hostname.

Note: #2256 embed parts of that implementation (without the TLS min version) and you should check it, it's pretty cool IMO.

@dgw
Copy link
Member

dgw commented Feb 16, 2022

Are you two going to make me decide whether to merge this and then immediately replace it with the asyncio version, or would you just work together on the TLS implementation in #2256 and avoid the conflict/resolution cycle altogether? 😛

@Exirel
Copy link
Contributor

Exirel commented Feb 16, 2022

I didn't implement TLS specifically to let @half-duplex see if what I did with SSLContext was in line with his work and if he would like to add TLS on top on my PR. But I don't think it's a good idea to merge this PR, given that the asyncio one is really changing a lot of things.

@dgw
Copy link
Member

dgw commented Feb 16, 2022

But I don't think it's a good idea to merge this PR, given that the asyncio one is really changing a lot of things.

That's why I asked if you guys were planning to work together on it (TLS) and avoid the conflicts before they start. 😺

@half-duplex half-duplex force-pushed the ssl-wrap-socket branch 2 times, most recently from 640c2da to 42ee312 Compare February 22, 2022 18:37
@half-duplex half-duplex changed the title backends: use SSLContext, specify TLS version and ciphers backends: allow setting TLS version and ciphers May 13, 2022
@half-duplex
Copy link
Member Author

Rebased on (so depends on) #2256

@half-duplex half-duplex force-pushed the ssl-wrap-socket branch 3 times, most recently from cb94785 to 9574e45 Compare May 14, 2022 06:32
@half-duplex half-duplex marked this pull request as ready for review May 14, 2022 06:34
Copy link
Contributor

@Exirel Exirel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love it! So simple in the end to add these settings.

sopel/irc/backends.py Outdated Show resolved Hide resolved
sopel/irc/backends.py Outdated Show resolved Hide resolved
sopel/irc/backends.py Outdated Show resolved Hide resolved
@half-duplex half-duplex force-pushed the ssl-wrap-socket branch 2 times, most recently from 9a54600 to 59e846e Compare May 30, 2022 01:53
@dgw
Copy link
Member

dgw commented Jun 4, 2022

@Exirel Is this good to go for real? Time to squash?

Copy link
Contributor

@Exirel Exirel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah all good!

@dgw dgw merged commit fd9e038 into sopel-irc:master Jun 5, 2022
@half-duplex half-duplex deleted the ssl-wrap-socket branch June 5, 2022 16:26
@half-duplex half-duplex restored the ssl-wrap-socket branch May 14, 2023 02:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants