Add authtoservices module, replacing sasl/nickserv (#1494) #1496

an-empty-string · 2018-02-18T23:30:44Z

The authtoservices module is an amalgamation of the nickserv and sasl modules. In short, it:

Tries SASL authentication, if available on the network (this can be disabled with /msg *authtoservices TrySasl no)
If that fails, it'll respond to NickServ prompts to identify, like the nickserv module would (this can be disabled with /msg *authtoservices RequireSasl yes)

The first time it is loaded, it will automatically migrate configuration from the sasl module or the nickserv module (precedence is given to sasl), if available. This ensures there's an upgrade path, if we decide the best way forward is to remove the sasl/nickserv modules from the tree and have them load authtoservices instead.

Aside from a few name changes and what's described above, it functions identically to the nickserv and sasl modules.

This PR fixes #1494. This module could in theory be an external module, however, as the original issue was that nickserv and sasl have similar functionality and users often choose the wrong one, it makes sense to combine this PR with removal/redirection of those modules (so this would have to be in core).

codecov · 2018-02-19T01:18:05Z

Codecov Report

Merging #1496 into master will decrease coverage by 0.03%.
The diff coverage is 2.93%.

@@            Coverage Diff             @@
##           master    #1496      +/-   ##
==========================================
- Coverage   36.87%   36.84%   -0.04%     
==========================================
  Files         126      125       -1     
  Lines       30823    30875      +52     
  Branches       93       93              
==========================================
+ Hits        11367    11376       +9     
- Misses      19415    19458      +43     
  Partials       41       41

Impacted Files	Coverage Δ
include/znc/IRCNetwork.h	`31.16% <ø> (ø)`	⬆️
modules/authtoservices.cpp	`1.67% <1.67%> (ø)`
src/IRCNetwork.cpp	`65.33% <11.9%> (-2.23%)`	⬇️
include/znc/Csocket.h	`62.09% <0%> (-0.66%)`	⬇️
src/FileUtils.cpp	`48.28% <0%> (-0.5%)`	⬇️
src/Utils.cpp	`66.61% <0%> (+2.03%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 15ccaca...596dd30. Read the comment docs.

DarthGandalf · 2018-02-18T23:38:04Z

modules/data/authtoservices/tmpl/index.tmpl

@@ -0,0 +1,83 @@
+<? I18N znc-sasl ?>


DarthGandalf · 2018-02-20T00:22:30Z

modules/authtoservices.cpp

+ SetArgs("<hidden>");
+ }
+
+ if(!GetNV("NotFirstLoad").ToBool()) {


This module should be automatically loaded if sasl and/or nickserv was loaded before. That can be done in IRCNetwork.cpp (search for "legacy crap"). Also the migrated settings can mimic the previous settings more closely if you take into account which of those modules were loaded.

Also that could help getting rid of "NotFirstLoad" hack.

an-empty-string · 2018-02-20T05:00:00Z

The most recent commits likely still have a few bugs... expect updates, but I'm about to start another busy week so it might be a while before I can get them in.

an-empty-string · 2018-02-23T23:11:11Z

502d4dd fixes an issue where the account name wouldn't properly default to the nickname during SASL authentication. That's the only problem I observed when testing this.

DarthGandalf · 2018-03-27T22:01:55Z

src/IRCNetwork.cpp

@@ -369,6 +369,21 @@ struct TOption {
 void (CIRCNetwork::*pSetter)(T);
 };

+bool CIRCNetwork::ModuleInConfig(CConfig* pConfig, const CString& sModule) const {
+ VCString vsList;
+ bool bFound;


This is undefined behavior without = false
Compiler is free to optimize this whole function to return true;

DarthGandalf · 2018-03-27T22:03:09Z

src/IRCNetwork.cpp

+ VCString vsList;
+ bool bFound;
+
+ pConfig->FindStringVector("loadmodule", vsList);


It removes all "loadmodule"s from config.

DarthGandalf · 2018-03-27T22:03:53Z

src/IRCNetwork.cpp

+ "which will load using your [sasl] configuration. Not "
+ "loading [nickserv]");
+ continue;
+ } else if (ModuleInConfig(pConfig, "authtoservices")) {


How did you test this?

DarthGandalf · 2018-03-27T22:04:39Z

src/IRCNetwork.cpp

+ if (sModName == "nickserv") {
+ if (ModuleInConfig(pConfig, "sasl")) {
+ CUtils::PrintAction(
+ "NOTICE: [nickserv] was merged into [authtoservices], "


What if sasl module was loaded, but not configured - e.g. user never told sasl the password.

DarthGandalf · 2018-03-27T22:06:03Z

modules/authtoservices.cpp

+ PutIRC(CString::NamedFormat(GetNV("IdentifyCmd"), msValues));
+ }
+
+ void HandleMessage(CNick& Nick, const CString& sMessage) {


Need a way to disable this, e.g. via additional "mechanism"

👍, this can also be a security flaw on sasl only networks where there isn't a NickServ nick. It might be possible for a user to get sasl passwords for ZNC users (and other clients that have similar logic) because they can change their nick to NickServ and send one of messages below to get clients to send passwords to them. Some servers can offer blocking nick names out from use like NickServ and common typos, but that is relying on the server being configured correctly.

I think the spirit of what we're trying to do with #1494 / #1230 is to encourage users to use sasl over nickserv (instead of, not with) because it has many benefits, including the decrease in risks where passwords can be erroneously taken. If its not by automated tooling, even there are known impersonation attacks in the wild with bots using nicknames similar to nickserv such as nicksarv and others to trick users into sending them their passwords.

If a network offers SASL I don't think users should consider using nickserv to idenify. Even if the sasl capability is currently missing or unavailable during connection, there can be room for more sophisticated attacks where you can abuse this feature of clients while services may be offline or down for maintenance. I'd avoid the room for downgrade attacks from sasl to nickserv auth. The original sasl module also has requireauth option to disconnect if we can't authenticate before registration (providing upstream server supports capabilities).

The problem right now is that some users do not know what sasl is, and they are already familiar with nickserv and thus would setup the nickserv module instead. I would suggest that we go with an approach that allows the module to be configured to only allow sasl authentication, and if at any time the module determines it can "upgrade" from from nickserv to sasl auth, it should then set this flag so it would never downgrade without user consent. Or even as alternative to merging the modules together, make the current NickServ module fail if it detects the server supports sasl and facilitate the user on switching to the sasl module instead (and some way to continue with nickserv if the user really knows what they are doing and understands the risks). However this approach may be a bit more user hostile than people would like.

Zarthus · 2018-05-06T19:15:57Z

it's bikesheddy, but I don't know if authtoservices is a good module name

(1) sasl doesn't depend on services, it's an ircd feature
(2) ircds are working to get rid of services support and integrate authentication directly

I have no really good name proposals, but i think just authentication or auth (already taken i think?) might suit better.

Zarthus · 2018-05-06T19:16:49Z

modules/authtoservices.cpp

+
+ public:
+ MODCONSTRUCTOR(CAuthToServices) {
+ AddCommand("Help", t_d("search"), t_d("Generate this output"),


Isn't there a AddHelpCommand(); function?

Zarthus · 2018-05-06T19:22:31Z

modules/authtoservices.cpp

+ true}};
+
+ public:
+ MODCONSTRUCTOR(CAuthToServices) {


What about refactoring the command names to directly indicate what type of authentication it is, normally I would suggest a subcommand structure but I don't know if ZNC is capable of it

e.g.

sasl account zarthus sasl require true sasl password hunter2 nickserv account zarthus nickserv password hunter2 nickserv name Themis

instead however maybe something like this makes sense

SASLAccount zarthus SASLRequire true SASLPassword hunter2 NSAccount zarthus NSPassword hunter2 NSName Themis

at the moment it's not clear to me what I need to provide to configure e.g. SASL, Do I need an account? a password? How do I set a certificate path? It's also inconsistent in e.g. SaslMechanism and RequireSasl

What's the point of unifying sasl and nickserv if all settings stay separate?

The settings can use the same variables, but if I want to configure SASL I just want to know what I need to configure SASL, introducing more ("redundant") options for a nicer to use "API" is, to me, more beneficial than keeping it a confusing mess with 50 commands that don't follow any kind of unified pattern.

Zarthus · 2018-05-06T19:24:36Z

modules/authtoservices.cpp

+ }
+
+ const CString GetAccount() {
+ if(GetNV("Account").empty()) {


I'm not a fan, I'd rather it just aborts and tells the user to set an account name.

Zarthus · 2018-05-06T19:25:24Z

I realize the PR feedback here is perhaps better suited for improvement tickets in the future; I just gave thoughts where I thought they fit, in the end it's remants from the old code.

add authtoservices module, replacing sasl and nickserv

5f8fb2c

an-empty-string changed the title ~~Add authtoservices module, replacing sasl/nickserv (#1494)~~ (WIP) Add authtoservices module, replacing sasl/nickserv (#1494) Feb 18, 2018

dgw mentioned this pull request Feb 19, 2018

Why does ZNC ship with *NickServ and *Q? #786

Closed

DarthGandalf reviewed Feb 20, 2018

View reviewed changes

an-empty-string added 2 commits February 19, 2018 20:34

this is authtoservices, not sasl

c6c3922

load authtoservices instead of sasl/nickserv

91ad67d

an-empty-string added 2 commits February 21, 2018 17:29

again, this is authtoservices, not sasl

a975e38

use network's configured nick instead of active nick

502d4dd

an-empty-string added 2 commits February 24, 2018 16:01

remove nickserv and sasl modules

f028893

rm sasl web templates

596dd30

an-empty-string changed the title ~~(WIP) Add authtoservices module, replacing sasl/nickserv (#1494)~~ Add authtoservices module, replacing sasl/nickserv (#1494) Feb 25, 2018

DarthGandalf reviewed Mar 27, 2018

View reviewed changes

Zarthus reviewed May 6, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add authtoservices module, replacing sasl/nickserv (#1494) #1496

Add authtoservices module, replacing sasl/nickserv (#1494) #1496

an-empty-string commented Feb 18, 2018 •

edited

codecov bot commented Feb 19, 2018 •

edited

DarthGandalf Feb 18, 2018

DarthGandalf Feb 20, 2018

an-empty-string commented Feb 20, 2018

an-empty-string commented Feb 23, 2018

DarthGandalf Mar 27, 2018

DarthGandalf Mar 27, 2018

DarthGandalf Mar 27, 2018

DarthGandalf Mar 27, 2018

DarthGandalf Mar 27, 2018

kylef Nov 17, 2020 •

edited

Zarthus commented May 6, 2018

Zarthus May 6, 2018

Zarthus May 6, 2018 •

edited

DarthGandalf May 6, 2018

Zarthus May 6, 2018

Zarthus May 6, 2018

Zarthus commented May 6, 2018

Add authtoservices module, replacing sasl/nickserv (#1494) #1496

Are you sure you want to change the base?

Add authtoservices module, replacing sasl/nickserv (#1494) #1496

Conversation

an-empty-string commented Feb 18, 2018 • edited

codecov bot commented Feb 19, 2018 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

an-empty-string commented Feb 20, 2018

an-empty-string commented Feb 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kylef Nov 17, 2020 • edited

Choose a reason for hiding this comment

Zarthus commented May 6, 2018

Choose a reason for hiding this comment

Zarthus May 6, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zarthus commented May 6, 2018

an-empty-string commented Feb 18, 2018 •

edited

codecov bot commented Feb 19, 2018 •

edited

kylef Nov 17, 2020 •

edited

Zarthus May 6, 2018 •

edited