Summary

Implement SASL for IRCv3 using GSASL

Review Request #2185 — Created Jan. 13, 2023 and submitted Jan. 17, 2023, 7:24 a.m.

Information

Owner

grim

Repository

pidgin/pidgin

Branch

default

Bugs

Depends On

Reviewers

Groups

pidgin

People

Description

Right now we really only tested PLAIN but SCRAM _might_ work as well as
external. But we'll need to connect to an external server to test that stuff as
it's really a bit of work to get set up locally.

Testing Done

Connected to a local ergo instance and verified everything there using the PLAIN mechanism.

Commits

Summary	ID
Implement SASL for IRCv3 using GSASL Right now we really only tested PLAIN but SCRAM _might_ work as well as external. But we'll need to connect to an external server to test that stuff as it's really a bit of work to get set up locally.	b150f9e6e34891291408c2bfd7716885ab27f52d

Issues

Description	From	Last Updated
There is a leak here due to a cyclic reference. The PurpleConnection has a data reference to PurpleIRCv3SASLData and that …	QuLogic	Jan. 17, 2023, 5:45 a.m.
As all these handlers are registered regardless of whether SASL is requested, they all need to check whether fetching PURPLE_IRCV3_SASL_DATA_KEY …	QuLogic	Jan. 17, 2023, 5:43 a.m.
This should probably default to GSASL_NO_CALLBACK as it would be unimplemented.	QuLogic	Jan. 17, 2023, 5:44 a.m.
an admin	QuLogic	Jan. 17, 2023, 5:30 a.m.
s/have/with/	QuLogic	Jan. 17, 2023, 5:30 a.m.
s/mechanism> '/mechanism> `/	QuLogic	Jan. 17, 2023, 5:30 a.m.
s/means would/means we would/	QuLogic	Jan. 17, 2023, 5:30 a.m.
s/which would leave/leaving/	QuLogic	Jan. 17, 2023, 5:30 a.m.
s/`E/` E/	QuLogic	Jan. 17, 2023, 5:30 a.m.
This needs to replace with a " " or else you will end up with FOO BAR BAZ -> FOOBAZ …	QuLogic	Jan. 17, 2023, 3:29 a.m.
In libpurple/protocols/ircv3/purpleircv3connection.c:280, getting this defaults to TRUE.	QuLogic	Jan. 17, 2023, 5:31 a.m.
Needs to be " " here as well.	QuLogic	Jan. 17, 2023, 4:33 a.m.
This duplicates what's done in purple_ircv3_sasl_attempt_mechanism.	QuLogic	Jan. 17, 2023, 5:31 a.m.
Server	QuLogic	Jan. 17, 2023, 5:31 a.m.
Not unused.	QuLogic	Jan. 17, 2023, 5:32 a.m.
Probably should set this key to NULL so that the gsasl data can be cleaned up, as it's not used …	QuLogic	Jan. 17, 2023, 3:38 a.m.
Possibly delete it here as well if we've tried all mechanisms.	QuLogic	Jan. 17, 2023, 5:44 a.m.

The issue has been resolved. Show all issues

There is a leak here due to a cyclic reference. The PurpleConnection has a data reference to PurpleIRCv3SASLData and that has a reference to the parent connection.

The data reference is never removed, so we have a cycle whenever the connection manager drops the connection. And since the PurpleIRCv3SASLData is not a GObject, it won't be considered for cyclic references AFAIK.

As noted in code comment, the PurpleIRCv3SASLData data reference should at least be cleared on success/failure of the SASL authentication. There's not really anywhere to comment it, but it should also be cleared when the connection is disconnected.

grim Jan. 17, 2023, 2:34 a.m.

I agree with the reference part.

However, I'm hesitent to clear on success in the event that we want to use GSSAPI or something similar at some point. However, I ran through all of the networks in this list and didn't see much for SASL mechanisms outside of PLAIN, EXTERNAL, SCRAM, and a single DH-BLOWFISH, so we should probably clear this on success for now.

As for failure 100% agree.

As for disconnection, since the connection object is only around while the connection is connected, the g_object_set_data_full is already clearing it on disconnect.

QuLogic Jan. 17, 2023, 3:42 a.m.

That recommendation was primarily for if the SASL data held a reference. I think I saw on stream that you'd be dropping the reference from it, so we don't need to delete this everywhere explicitly. Failure-only removal is probably safe with that.

libpurple/protocols/ircv3/purpleircv3parser.c (Diff revision 1)

The issue has been resolved. Show all issues

As all these handlers are registered regardless of whether SASL is requested, they all need to check whether fetching PURPLE_IRCV3_SASL_DATA_KEY returns a valid pointer (if they use it), in case of a buggy/malicious server that sends any of these before a SASL request.

libpurple/protocols/ircv3/purpleircv3sasl.c (Diff revision 1)
The issue has been resolved. Show all issues
```
This should probably default to GSASL_NO_CALLBACK as it would be unimplemented.
```
libpurple/protocols/ircv3/purpleircv3sasl.c (Diff revision 1)
The issue has been resolved. Show all issues
```
an admin
```
libpurple/protocols/ircv3/purpleircv3sasl.c (Diff revision 1)
The issue has been resolved. Show all issues
```
s/have/with/
```
libpurple/protocols/ircv3/purpleircv3sasl.c (Diff revision 1)
The issue has been resolved. Show all issues
```
s/mechanism> '/mechanism> `/
```
libpurple/protocols/ircv3/purpleircv3sasl.c (Diff revision 1)
The issue has been resolved. Show all issues
```
s/means would/means we would/
```
libpurple/protocols/ircv3/purpleircv3sasl.c (Diff revision 1)
The issue has been resolved. Show all issues
```
s/which would leave/leaving/
```
libpurple/protocols/ircv3/purpleircv3sasl.c (Diff revision 1)
The issue has been resolved. Show all issues
```
s/`E/` E/
```

libpurple/protocols/ircv3/purpleircv3sasl.c (Diff revision 1)

The issue has been dropped. Show all issues

This needs to replace with a " " or else you will end up with FOO BAR BAZ -> FOOBAZ when replacing BAR.

libpurple/protocols/ircv3/purpleircv3sasl.c (Diff revision 1)
The issue has been resolved. Show all issues
```
In libpurple/protocols/ircv3/purpleircv3connection.c:280, getting this defaults to TRUE.
```

libpurple/protocols/ircv3/purpleircv3sasl.c (Diff revision 1)

The issue has been dropped. Show all issues

Needs to be " " here as well.

grim Jan. 17, 2023, 2:34 a.m.

that shouldn't be the case as every entry to should have a leading and trailing space. IE PLAIN EXTERNAL so removing PLAIN results in EXTERNAL because there's 2 spaces between plain and external.

QuLogic Jan. 17, 2023, 3:42 a.m.

Ah, you're right; I think I confused this with XMPP's implementation which does replace(",", " ") and I think may be buggy with the fallback part (i.e., mixing up common prefixes/suffixes), but I've not confirmed that.

libpurple/protocols/ircv3/purpleircv3sasl.c (Diff revision 1)

The issue has been resolved. Show all issues

This duplicates what's done in purple_ircv3_sasl_attempt_mechanism.

libpurple/protocols/ircv3/purpleircv3sasl.c (Diff revision 1)
The issue has been resolved. Show all issues
```
Server
```
libpurple/protocols/ircv3/purpleircv3sasl.c (Diff revision 1)
The issue has been resolved. Show all issues
```
Not unused.
```

libpurple/protocols/ircv3/purpleircv3sasl.c (Diff revision 1)

The issue has been dropped. Show all issues

Probably should set this key to NULL so that the gsasl data can be cleaned up, as it's not used after auth.

grim Jan. 17, 2023, 2:34 a.m.

That's not necessarily true. In the event that services go down and back up, we should see a CAP DEL SASL when it goes down and a CAP ADD SASL when it comes back, at which point, it would be amazing if we just reauthenticate at that point. Which getting back to my previous comment about clearing on success, we probably want to avoid that for this reason. Otherwise we'd have to have the protocol plugin read the password from the credential manager, which is doable, but not something we've done so far to avoid code duplication mostly.

QuLogic Jan. 17, 2023, 3:42 a.m.

Yes, re-auth is definitely good. I'm not sure if we want to keep the password loaded for security reasons, but this is also an issue in XMPP's SASL. I think we can decide on that separately later.

libpurple/protocols/ircv3/purpleircv3sasl.c (Diff revision 1)

The issue has been dropped. Show all issues

Possibly delete it here as well if we've tried all mechanisms.

grim Jan. 17, 2023, 4:39 a.m.

The issue with that is if the server restarts sasl to add a mechanism we support then we can't detect it.

I think the bigger idea here, is to have a "use sasl if available" option and use that to decide some of these corner cases?

Change Summary:

Address all the things! (I think...)

Commits:

	Summary	ID
	Implement SASL for IRCv3 using GSASL Right now we really only tested PLAIN but SCRAM _might_ work as well as external. But we'll need to connect to an external server to test that stuff as it's really a bit of work to get set up locally.	5b0ac716a1ab81cc0a3f3d77ce72fd238e4b17da
	Implement SASL for IRCv3 using GSASL Right now we really only tested PLAIN but SCRAM _might_ work as well as external. But we'll need to connect to an external server to test that stuff as it's really a bit of work to get set up locally.	b150f9e6e34891291408c2bfd7716885ab27f52d

Diff:

Revision 2 (+1578 -6)

Show changes

	meson.build
	libpurple/protocols/ircv3/meson.build
	libpurple/protocols/ircv3/purpleircv3capabilities.c
	libpurple/protocols/ircv3/purpleircv3connection.h
	libpurple/protocols/ircv3/purpleircv3connection.c
	libpurple/protocols/ircv3/purpleircv3parser.c
	libpurple/protocols/ircv3/purpleircv3sasl.h
	libpurple/protocols/ircv3/purpleircv3sasl.c
	1 more

Ship it!

```
Ship It!
```

Status:: Completed