Skip to content

Add option for secure IRC connections#5473

Merged
vadi2 merged 10 commits intoMudlet:developmentfrom
atari2600tim:atari-secureIRC
Oct 8, 2021
Merged

Add option for secure IRC connections#5473
vadi2 merged 10 commits intoMudlet:developmentfrom
atari2600tim:atari-secureIRC

Conversation

@atari2600tim
Copy link
Copy Markdown
Contributor

Brief overview of PR changes/additions

Now it can connect to IRC servers using Transport Layer Security. setIrcServer includes optional boolean argument. getIrcServer returns an extra boolean. New checkbox on preferences.

Motivation for adding to Mudlet

Allows secure connection to IRC servers.

Other info (issues closed, discussion etc)

Closes #5345

Release post highlight

IRC client can connect to secure servers

@add-deployment-links
Copy link
Copy Markdown

add-deployment-links bot commented Oct 2, 2021

Hey there! Thanks for helping Mudlet improve. 🌟

Test versions

You can directly test the changes here:

No need to install anything - just unzip and run.
Let us know if it works well, and if it doesn't, please give details.

Copy link
Copy Markdown
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

Copy link
Copy Markdown
Member

@SlySven SlySven left a comment

Choose a reason for hiding this comment

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

Some changes to consider!

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Oct 3, 2021

clang-tidy review says "All clean, LGTM! 👍"

this part made it where if you changed settings while connected then it would not properly connect again

QPair<bool, QString> dlgIRC::writeIrcHostSecure(Host* pH, bool secure)
{
return pH->writeProfileData(dlgIRC::HostSecureCfgItem, (secure ? QLatin1String("true") : QLatin1String("false")));
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This works great as of now. But when it saves false to file, it apparently throws a newline in front. Still functional but confusing to me. Maybe writeProfileData does something I'm not expecting.

$ cat irc_secure
true
Alpha@Alpha-F5BZ322 MSYS /c/Users/Alpha/.config/mudlet/profiles/zzz-three
$ cat irc_secure

false

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@atari2600tim
Copy link
Copy Markdown
Contributor Author

atari2600tim commented Oct 4, 2021

Should I switch the default connection from irc.libera.chat 6667 to irc.libera.chat +6697 also? I don't want players to go in there and set their own server and have it automatically be set to secure and maybe confuse them. Although it is right there and obvious enough I guess.

Otherwise SlySven caught everything that I was planning to come back to and more, very much appreciated. I'll switch it out of draft mode.

@atari2600tim atari2600tim marked this pull request as ready for review October 4, 2021 16:13
@atari2600tim atari2600tim requested a review from a team as a code owner October 4, 2021 16:13
@atari2600tim atari2600tim requested review from a team October 4, 2021 16:13
Copy link
Copy Markdown
Member

@vadi2 vadi2 left a comment

Choose a reason for hiding this comment

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

Works! Though I was confused at first because I just ticked the option, didn't realise I have to change the port as well, and the connection wasn't working. Similar problem that the connection dialog would be having...

Agree on changing the default to be more secure for new profiles only. We don't want to break existing profiles by changing them to the secure option without having updated the port...

<item row="2" column="0" colspan="2">
<widget class="QCheckBox" name="ircHostSecure">
<property name="text">
<string>TLS/SSL secure connection</string>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
<string>TLS/SSL secure connection</string>
<string>Use a secure connection</string>
  • tooltip saying it'll use TLS/SSL for the technically-minded folk who'd like more details

Communi desktop client automatically checks the box if you type in 6697, 7000, or 7070.
https://github.com/communi/communi-desktop/blob/master/src/app/connectpage.cpp#L45

RFC 7194 just mentions 6697
@vadi2 vadi2 changed the title Secure IRC over TLS Add option for secure IRC connections Oct 8, 2021
@vadi2 vadi2 merged commit 8936e32 into Mudlet:development Oct 8, 2021
@atari2600tim atari2600tim deleted the atari-secureIRC branch October 8, 2021 04:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

support IRC over TLS

3 participants