[go: up one dir, main page]
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.

Sign up for GitHub

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

Jump to bottom

common: Default to SSL/TLS for new IRC networks #537

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

common: Default to SSL/TLS for new IRC networks #537

wants to merge 1 commit into from

Conversation

digitalcircuit
Copy link
Contributor
@digitalcircuit digitalcircuit commented Jun 19, 2020 •
edited
Loading

In short

  • Enable SSL/TLS by default for new IRC networks
    • Existing networks will stay as they are
    • SSL/TLS can be disabled by editing the network
    • Default port will revert to 6667 when unchecking Use encrypted connection
    • Client-side change; core has no impact
  • Fix up Doxygen comments for Server::useSSL and PortDefaults
Criteria Rank Reason
Impact ★★☆ 2/3 User-facing better security by default
Risk ★★★ 3/3 May frustrate, confuse those who don't know to try disabling SSL/TLS
Intrusiveness ★☆☆ 1/3 UI/defaults changes, shouldn't interfere with other pull requests

Rationale

It's the year 2020, and several networks offer SSL/TLS with valid certificates. Quassel could default to enabling SSL/TLS for newly added networks.

However, a few large networks still do not support SSL/TLS on port 6697, alongside locally-hosted bridges, such as Bitlbee. This will require unchecking Use encrypted connection to connect those networks without SSL/TLS.

All preset networks will work as before, according to the preset (most have SSL/TLS ports added already).

Subjective choice. This may result in increased support requests in #quassel.

When SSL/TLS is not available on the configured port, Quassel will fail to connect to the IRC network with an error message (i.e. Connection refused). This can be remedied by editing the network and unchecking Use encrypted connection, but that's an extra step that might put off new users.

There are other, more complex options, such as auto-detecting if an SSL/TLS port is available, or supporting the IRCv3 sts capability.

Opinions in freenode/#ircv3 are mixed - some clients are doing this already without reported issues, others are in favor of the more complex methods.

Examples

Before

Add Network dialog
On the Settings - IRC - Networks page under the Networks heading, Add.. button clicked to add a new network to an existing network.

Add Network dialog, showing a "Port" of "6667" and "Use encrypted connection" unchecked by default

Server Edit dialog
On the Settings - IRC - Networks page under the Servers heading, Add.. button clicked to add a new server to an existing network.

Edit Server dialog, showing a "Port" of "6667" and "Use encrypted connection" unchecked by default

After

Add Network dialog
On the Settings - IRC - Networks page under the Networks heading, Add.. button clicked to add a new network to an existing network.

Add Network dialog, showing a "Port" of "6697" and "Use encrypted connection" checked by default

Server Edit dialog
On the Settings - IRC - Networks page under the Servers heading, Add.. button clicked to add a new server to an existing network.

Edit Server dialog, showing a "Port" of "6697" and "Use encrypted connection" checked by default

@digitalcircuit digitalcircuit changed the title common: Default to SSL/TLS for new IRC networks [discussion needed] common: Default to SSL/TLS for new IRC networks Jun 20, 2020
@digitalcircuit digitalcircuit changed the title [discussion needed] common: Default to SSL/TLS for new IRC networks [discussion suggested] common: Default to SSL/TLS for new IRC networks Jul 18, 2020
@digitalcircuit
common: Default to SSL/TLS for new IRC networks
312d00e 
Change default for newly created IRC networks to use port 6697 with
SSL/TLS.  It's the year 2020, this should hopefully be a sane default.
SSL/TLS can always be disabled as before.

Fix up the Doxygen comments for Server::useSSL and PortDefaults.
@digitalcircuit
Copy link
Contributor Author
digitalcircuit commented Nov 29, 2020 •
edited
Loading

One course of action:

  • Add boolean column to ircserver table called sslprobed
    • Default to false
  • Make use of the existing code to update/set network info to store this
    • Inefficient, but should only happen once, and the server list doesn't have unique IDs at the moment
    • IRC server details may need updated anyways, too (port and connection settings)

Then…

  1. On CoreNetwork connect after selecting an IRC server, check sslProbed for the selected IRC server
    • If sslProbed == true, connect with current settings as before
    • If sslProbed == false, then…
  2. Check IRC server useSsl / "use encrypted connection" option
    • If already enabled, set sslProbed = true, connect with current settings as before
  3. Check IRC port number
    • If not PortDefaults::PORT_PLAINTEXT, connect with current settings as before
    • Intentionally not setting the sslProbed flag so if someone reverts to the default plaintext port in the future, SSL/TLS will get auto-attempted once
  4. Safe to probe. Set sslProbed = true
    • If there's somehow a crash or hang from connecting to SSL/TLS port, Quassel won't get stuck
  5. Set temporary flag probingSsl = true (or such) in CoreNetwork
  6. Connect to the chosen IRC server over PortDefaults::PORT_SSL with useSsl enabled and sslVerify active
    • Making use of Quassel's default IRC network ping timeout detection for timing out
    • Put a message in the status buffer after existing connection message (* Connecting to chat.freenode.net:6667...)
    • Example message: * Checking if encrypted connection available at chat.freenode.net:6697…
  7. On successful connect (reached point of sending CAP LS 302), modify the IRC server to PortDefaults::PORT_SSL with useSsl enabled and sslVerify active, set probingSsl = false
    • Example message: * Encrypted connection to chat.freenode.net:6697 successful, saved in settings
  8. On ping timeout, SSL/TLS error, or any disconnect when probingSsl is set to true and current server has PortDefaults::PORT_SSL with useSsl enabled and sslVerify active, emit message and auto-reconnect once
    • Example message: * Encrypted connection to chat.freenode.net:6697 not successful, reverting to unencrypted connection...
    • Trigger auto-reconnect even if normally disabled (this is a once-off per IRC server)
    • As sslProbed was previously set to true, a standard reconnect will skip this

Daddy on freenode/#ircv3 suggested having this retry more than once. Perhaps an advanced setting of Upgrade to encrypted connection when possible, so Quassel will retry every X interval or every connect instead, with this being disabled by unchecking the option. At some point this just becomes starttls/sts - perhaps this probing logic is a stepping stone to that?

@digitalcircuit
Copy link
Contributor Author
digitalcircuit commented Jun 19, 2021 •
edited
Loading

Okay, so… HexChat is doing this by default. Time to consider Quassel switching as well..?

hexchat/hexchat@747a52a (thanks to Sadie in #ircv3 for pointing this out, pull request here).

sts would still be nice to support, of course.

Edit: Of note, Quassel's default network wizard is not affected by this change. Folks that just follow the wizard should still wind up in #quassel or elsewhere (according to distro/packager customizations).

@digitalcircuit digitalcircuit changed the title [discussion suggested] common: Default to SSL/TLS for new IRC networks [discussion] common: Default to SSL/TLS for new IRC networks Jun 19, 2021
@digitalcircuit digitalcircuit changed the title [discussion] common: Default to SSL/TLS for new IRC networks common: Default to SSL/TLS for new IRC networks Jun 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@digitalcircuit