On Fri, 24 Jul 2026 at 15:36:26 +0200, Oswald Buddenhagen wrote: > On Fri, Jul 24, 2026 at 02:37:41PM +1000, Seth McDonald wrote: > > On Wed, 22 Jul 2026 at 14:57:28 +0200, Oswald Buddenhagen wrote: > > > On Sat, Jul 18, 2026 at 03:29:40PM +1000, Seth McDonald via isync-devel > > > wrote: > > > > The interface's implementation adds minor changes in behaviour to > > > > simplify the authentication logic. This includes always attempting to > > > > fall back to LOGIN when SASL fails and unifying similar error messages. > > > > semantically, that seems just wrong. you need to work a lot harder > > > to > > > convince me that this is a wanted change. > > > > The primary motivation is that the current "flow" of the SASL logic is > > incompatible with the GNU SASL library. > > > upon a cursory look at the gsasl documentation, i don't see how that would > be fundamentally true.
I think I touch on this throughout this message. > in fact, i have the impression that you didn't study it particularly > thoroughly, <https://lists.gnu.org/archive/html/help-gsasl/2026-07/threads.html> As I said previously, I do the work and take the time to understand my tools. > given that you thought it necessary to hard-code the list of > available mechanisms. GNU SASL does not choose mechanisms the same way Cyrus SASL does. Both libraries (Cyrus and GNU) have a large set of SASL mechanisms they support. Of these, there is the common subset of mechanisms that mbsync is currently able to use, which is effectively the same for both. (This subset is different to the set of user configuered mechs.) Cyrus first establishes a client context via sasl_client_new(), which involves providing information about the connection. There's also sasl_setprop() to give additional info. Then the later call to sasl_client_start() chooses a mechanism that requires no more than the provided information to be successful. So in effect, it determines the subset of usable mechanisms, then selects from that subset. GNU assumes you have already selected a mechanism by the time you start the authentication with gsasl_client_start(). That is, it gives the caller the responsibility to choose an appropriate mechanism. Which in our case, includes first determining the subset of usable mechanisms. Luckily, GNU SASL's documentation specifies exactly what is needed for each mechanism, so we can know this subset ahead of time. Hence the hardcoded whitelist of mechanisms. And to preempt some responses: "But gsasl_client_mechlist() gives the subset of usable mechanisms." No it doesn't. It gives the set of mechanisms supported by the client side of the library. It does not consider whether we can actually use the mechanisms when accounting for the information we have available. "But gsasl_client_suggest_mechanism() does this for you." No it doesn't. It selects the most secure mechanism of a list that is supported by the library (assuming the list is ordered by ascending security). It does not consider whether we can actually use the mechanism when accounting for the information we have available. "Why not filter the list given by gsasl_client_mechlist() then?" That would involve identifying usable mechanisms by name and removing the others. Since the same information* is provided for each authentication process, these usable mechanisms to identify will be the same for each such process. So by itself, the effect will just be a whitelist but with more steps. That said, upon reflection I do think this could be done in addition to the hardcoded whitelist. Since the program may use an older library version, some of the more recent mechanisms may not be supported. So we can either raise the required version with gsasl_check_version() or intersect the whitelist with the result of gsasl_client_mechlist(). I lean towards the latter. *The same types of information. For example, each authentication process may use different values for the username and password, but they all may use a username and password if necessary. > a different angle is that it isn't necessary to utilize the library's full > potential. it's just fine to duplicate a bit of the library's functionality > if it makes the implementations more uniform. e.g., i don't see any > practical benefit in actually using gsasl's LOGIN support. Sure, which is why I didn't use GNU SASL's LOGIN support in the patch. > > Regarding the falling back to LOGIN, it simplifies the logic from > > unnecessarily complex "if it fails in this specific way, goto here; but > > if it fails like this, goto there" to "if it fails, try LOGIN instead". > > > that description of the status quo doesn't fill me with confidence that you > actually understand it. It currently works by attempting to fallback to LOGIN if at some point during initialisation of the SASL client, the library indicates that no mechanism is usable via SASL_NOMECH. Otherwise if any other error occurs, the process gives a message or two and bails out of any authentication, not just SASL. This logic is tied to Cyrus SASL specifically. Because GNU SASL doesn't choose the SASL mechanism for you, there is no GSASL_NOMECH error. We could treat an empty intersection between the whitelist and the given mech list as such an error, but that will only ever trigger for a non-compliant IMAP4 server (since PLAIN is required), meaning the fallback will almost never actually occur. And to reiterate, as a user, I don't want a program bothering me about something it can fix itself. If SASL fails, it should attempt to fix itself by falling back to LOGIN before making it the user's problem. > > > > +static const char * > > > > +query_sasl_error( sasl_conn_t *ctx, int rc ) > > > > i don't want that obfuscation layer without good reason. > > > > I added it for two reasons: > > (0) It's simpler to have a single function to call to retrieve the > > appropriate error message, rather than manually switching > > between sasl_errdetail() and sasl_errstring() all the time. > > > it's two functions for a good reason, and i think that blurring the two > contexts is actively counter-productive. I don't entirely agree about it blurring the contexts, but I'll drop this reason to focus on the more important second. > > (1) To encompass the following fallback. > > > > > > + const char *err = ctx > > > > + ? sasl_errdetail( ctx ) > > > > + : sasl_errstring( rc, NULL, NULL ); > > > > > > > + return err ? err : "Unknown error"; > > > > what is that fallback for? > > > > From the documentation for sasl_errdetail() (see [0]): > > > > | Returns the string describing the error that occurred, or NULL if no > > | error has occurred, or there was an error retrieving it. > > > calling the function when no error occurred would be an internal error in > isync. > and error while retrieving an error string would be an internal error in > libsasl or the calling code. The fallback is intended mainly for the latter case. Such an error in the library is (from the perspective of the calling program) always a possibility, so the fallback accounts for this. > in either case i want that to be glaringly obvious, not papered over. Without the fallback, if such an internal error occurs leading to a NULL return value, the program will just segfault when it attempts to read from the NULL pointer. Is that what's preferred? If not, then a NULL return value should be handled, which makes a function encompassing that handling useful. > > > > +static int > > > > +process_sasl_step( > > > > + } else { > > > > + const char *err = query_sasl_error( session, rc ); > > > > i don't like these pointless temporaries. > > > > I tend to use them for a few reasons: > > (0) They make debugging line-by-line with GDB easier by keeping calls on > > their own lines, which allows for easy stepping into/over select > > calls. This is especially true for function calls inside the > > conditions of if-statements. > > > this is true, but completely irrelevant for the "print stringified error > code" pattern. I was referring to my use of temporary variables across the patchset and in general. But also, are you implying the "print stringified error code" pattern cannot contain bugs? Because if not, then how is this irrelevant? > > (1) They make git diff/blame cleaner by separating operations with > > newlines, which allows diff/blame to better identify and show > > the exact operations that were changed. > > > that's equally achievable by wrapping the "resolver" call to the next line. You say later on that such wrapping of arguments deviates from the style of the codebase and shouldn't be used without sound justification. If this is sound justification, then you agree I can wrap arguments like I did in the patch. If this is not sound justification, then temporary variables are still the way to go to achieve better diffs/blames. > > (2) They tell you what exactly you're retrieving from the function, > > which may not always be so obvious. For example, if a call > > returns a status, does zero indicate success or failure? A line > > like `if (!foo())` doesn't tell you much, but a line like > > `int bad = !foo();` tells you immediately it means failure. > > > also completely irrelevant in this context. Again, I was referring to temporary variables in general. And in particular here, my use of them in the patchset for return values that are immediately checked in an if condition. > but i forgot to mention something before: judging by the sub-optimality of > some of the hunks, you don't seem to have switched your git diff algorithm > to histogram yet? No, I use minimal by default. But I'm happy to switch to histogram for this project. > > > > +typedef const char *(*imap_auth_cred_t)( void * ); > > > > + > > > the consequences of having these are obviously ugly. > > > just factor out imap_auth_conf_t as i hinted at previously. > > > > Sure, but just FYI without it the logic will no longer be > > self-contained. It'll depend on the implementation of the IMAP driver, > > > why would it? It'll directly refer to and hence depend on imap_server_conf_t and ensure_(user|password)(). > > > > +/* > > > > + * Formats a list of SASL mechanisms supported by both client and > > > > server side. > > > > + * The list is space-delimited, including starting and ending with > > > > whitespace. > > > > + * Mechanisms are parsed in a case-insensitive manner. > > > > + */ > > > > +void imap_auth_filter_mechs( > > > > this seems like an implementation detail that should not leak > > > through the > > > abstraction. > > > > Not sure I entirely agree. > > The case-insensitive detail [...] > > The space-delimited detail [...] > > > you didn't pay attention to the reply placement ... > i question the function being part of the iface in the first place. Ah, my bad. The function was exposed mainly to separate the mechanism filtering from the rest of the authentication logic. It's not super important and can be folded into the implementation if desired. > > > > +char * > > > > +find_string_list( const string_list_t *list, const char *str ) > > > > + > > > > +char * > > > > +find_string_list_case( const string_list_t *list, const char *str ) > > > > > > the return values make no sense. these should be bools, and the functions > > > called contains_.... > > > > I was aiming for a function analogous to strstr(). It's purpose is to > > find and, if found, return a string within a string list. And like > > strstr(), it can be used secondarily to only check if the string list > > contains the string by checking if the return value is NULL. > > > don't try to make analogous interfaces when there is a cost, but no obvious > advantage for existing use cases. What's the cost? There's no non-negligible runtime penalty to returning a pointer instead of an integer. It may cost a nanosecond or two if the architecture stores pointers as larger than integers, but that's less meaningful than even a typical micro-optimisation. On the other hand, an advantage is a familiar interface. The closer we stick to standard and familiar conventions and interfaces, the easier it is for others to use them. > > > your prior macos keychain patches should be integrated into this > > > series to > > > reduce overall churn. for that purpose, it's ok to either > > > - during refactoring, leave code in its old place when it will be > > > replaced > > > in subsequent patches, or > > > - to add code in a new file that will be logically created only in the > > > subsequent refactoring (i think this is the case applicable here). > > > > I'm afraid I don't see how integrating the keychain patches into this > > patchset will reduce churn. The imap_auth_* interface doesn't touch nor > > take responsibility for either the keychain code or the > > password-querying logic as a whole. It calls (directly or indirectly) > > ensure_password(), but doesn't care how that function works. > > > that argument is just wrong in light of the imap_auth_conf_t discussion > above. That wasn't an argument; I genuinely don't see how it reduces churn. Take care, Seth McDonald. (Not me spending four hours writing an email lol.) -- E9D1 26A5 F0D4 9DF7 792B C2E2 B4BF 4530 D39B 2D51 _______________________________________________ isync-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/isync-devel
