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

Send NickServ messages when SASL is disabled #999

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

Conversation

palmer-dabbelt
Copy link
Contributor

I'm not sure this is the correct user interface, so I'm writing this patch as
more of a way to describe my issue.

I'm running my own instance of this appservice, and I'm using it to connect to
OFTC. OFTC supports SASL, but requires a certificate-based authentication
mechanism as opposed to the password-based mechanism that's supported by this
appservice. As a result, even after sending a "!storepass irc.oftc.net
$PASSWORD" I don't get logged in and still must identify with NickServ manually
whenever the appservice reconnects to OFTC. In practice I just keep getting
kicked from the one OFTC channel I care about, which requires registered nicks.

I can think of two possible ways to fix this: either add support
certificate-based SASL authentication (which seems complicated) or add support
for NickServ. I chose to add support for NickServ, which simply involves
sending a private message to NickServ containing the user's password.

I think that's a sound feature, but I don't like how it's exposed to the user.
With this patch the appservice will send a NickServ message containing the
user's password whenever SASL is disabled in the server's configuration file.
While this works for me, I'm not sure it's a good idea to just start sending
out passwords with such an implicit configuration option.

If I was to spend some more time on this, I'd probably do something like add an
"authentication_mechanism" configuration option that can be set to either
"none" (which would do nothing, aka sasl=false), "sasl_password" (which would
do sasl=true) or "nickserv" (which would do this). Since there's a bit of
plumbing involved and I've never written any typescript before, I wanted to
send this patch out before going any farther -- if you're not interested in
NickServ auth then I can just carry the patch myself.

Signed-off-by: Palmer Dabbelt [email protected]

I'm not sure this is the correct user interface, so I'm writing this patch as
more of a way to describe my issue.

I'm running my own instance of this appservice, and I'm using it to connect to
OFTC.  OFTC supports SASL, but requires a certificate-based authentication
mechanism as opposed to the password-based mechanism that's supported by this
appservice.  As a result, even after sending a "!storepass irc.oftc.net
$PASSWORD" I don't get logged in and still must identify with NickServ manually
whenever the appservice reconnects to OFTC.  In practice I just keep getting
kicked from the one OFTC channel I care about, which requires registered nicks.

I can think of two possible ways to fix this: either add support
certificate-based SASL authentication (which seems complicated) or add support
for NickServ.  I chose to add support for NickServ, which simply involves
sending a private message to NickServ containing the user's password.

I think that's a sound feature, but I don't like how it's exposed to the user.
With this patch the appservice will send a NickServ message containing the
user's password whenever SASL is disabled in the server's configuration file.
While this works for me, I'm not sure it's a good idea to just start sending
out passwords with such an implicit configuration option.

If I was to spend some more time on this, I'd probably do something like add an
"authentication_mechanism" configuration option that can be set to either
"none" (which would do nothing, aka sasl=false), "sasl_password" (which would
do sasl=true) or "nickserv" (which would do this).  Since there's a bit of
plumbing involved and I've never written any typescript before, I wanted to
send this patch out before going any farther -- if you're not interested in
NickServ auth then I can just carry the patch myself.

Signed-off-by: Palmer Dabbelt <[email protected]>
@Half-Shot
Copy link
Contributor

Hey! Sorry this took so long to get to.

The bridge currently handles authentication by sending a PASS command to the ircd on connection, which freenode and other ircds handle but it sounds like OFTC does not.

I'd be in favor of a more configurable approach to solve this issue. Perhaps we could have the concept of authentication providers in the code that can be toggled on for certain irc networks. For instance, there could be a NickservAuthProvider class for OFTC which sends the correct commands during connection without having to toggle that behaviour on for other networks.

However, I realize that's some quite difficult typescript to get right but I think it's probably the correct solution to the problem.

@thomwiggers
Copy link
Contributor

thomwiggers commented Jan 26, 2021

How about, instead of trying to solve this problem for particular servers like OFTC, there's a nickserv_identify_command template in the server yaml, perhaps with a nickserv_bot variable?

@thomwiggers
Copy link
Contributor

How about, instead of trying to solve this problem for particular servers like OFTC, there's a nickserv_identify_command template in the server yaml, perhaps with a nickserv_bot variable?

I looked into this a little bit. Solving this properly should probably do the following:

@Half-Shot Half-Shot requested a review from a team July 28, 2022 10:56
Copy link
Contributor

@Half-Shot Half-Shot left a comment

Choose a reason for hiding this comment

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

The described approach of having a config for nickserv bot and nickserve command template works for me, which at least explicitly makes it opt-in for networks that need it.

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.

3 participants