Implement SASL for IRCv3 using GSASL

Review Request #2185 — Created Jan. 13, 2023 and submitted

Information

pidgin/pidgin
default

Reviewers

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.

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

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
Description From Last Updated

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

QuLogicQuLogic

As all these handlers are registered regardless of whether SASL is requested, they all need to check whether fetching PURPLE_IRCV3_SASL_DATA_KEY …

QuLogicQuLogic

This should probably default to GSASL_NO_CALLBACK as it would be unimplemented.

QuLogicQuLogic

an admin

QuLogicQuLogic

s/have/with/

QuLogicQuLogic

s/mechanism> '/mechanism> `/

QuLogicQuLogic

s/means would/means we would/

QuLogicQuLogic

s/which would leave/leaving/

QuLogicQuLogic

s/`E/` E/

QuLogicQuLogic

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

QuLogicQuLogic

In libpurple/protocols/ircv3/purpleircv3connection.c:280, getting this defaults to TRUE.

QuLogicQuLogic

Needs to be " " here as well.

QuLogicQuLogic

This duplicates what's done in purple_ircv3_sasl_attempt_mechanism.

QuLogicQuLogic

Server

QuLogicQuLogic

Not unused.

QuLogicQuLogic

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

QuLogicQuLogic

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

QuLogicQuLogic
QuLogic
  1. 
      
  2. 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.

    1. 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.

    2. 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.

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

    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.

  4. This should probably default to GSASL_NO_CALLBACK as it would be unimplemented.

  5. s/have/with/

  6. s/mechanism> '/mechanism> `/

  7. s/means would/means we would/

  8. s/which would leave/leaving/

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

  10. In libpurple/protocols/ircv3/purpleircv3connection.c:280, getting this defaults to TRUE.

  11. Needs to be " " here as well.

    1. 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.

    2. 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.

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

    This duplicates what's done in purple_ircv3_sasl_attempt_mechanism.

  13. Not unused.

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

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

    1. 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.

    2. 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.

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

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

    1. 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?

  16. 
      
grim
QuLogic
  1. Ship It!
  2. 
      
grim
Review request changed

Status: Closed (submitted)

Loading...