Apologies for the delayed response. I'm back to studying so I may take
longer to respond.
On Sun, 26 Jul 2026 at 18:16:07 +0200, Oswald Buddenhagen wrote:
> On Sat, Jul 25, 2026 at 05:44:14PM +1000, Seth McDonald wrote:
> > GNU SASL does not choose mechanisms the same way Cyrus SASL does. [...]
> >
> ok, fair enough. apparently the api is less helpful than it seems at first
> sight.
>
> but i wonder whether it actually makes sense to filter mechanisms
> client-side beyond what the config says. is there a plausible situation
> where the server would offer a mechanism that the client cannot actually use
> because of local constraints?
We can actually test this by spinning up netcat to act as an IMAP server
for isync (as of master-next) to connect to. Consider the following
exchange (C for client/isync; S for server/netcat).
S: * OK IMAP4 hello
C: 1 CAPABILITY
S: * CAPABILITY IMAP4rev1 AUTH=GSSAPI
S: 1 OK completed
This causes isync to give an error message listing all the SASL
mechanisms it can use, accounting for the information available (since
such information was provided to Cyrus SASL to generate this list). And
when linked with Cryus SASL 2.1.28 packaged for Debian 13, I get the
following list (see [0] for the config file).
SCRAM-SHA-512
SCRAM-SHA-384
SCRAM-SHA-256
SCRAM-SHA-224
SCRAM-SHA-1
DIGEST-MD5
EXTERNAL
CRAM-MD5
NTLM
LOGIN
PLAIN
XOAUTH2
ANONYMOUS
Any mechanisms not in this list we cannot use even if we wanted to,
since we don't provide the necessary information. This includes GSSAPI,
SCRAM-SHA-256-PLUS, and SCRAM-SHA-1-PLUS, among others.
> > 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.
> >
> huh? are you suggesting that isync should ignore what the user explicitly
> configured (noting that LOGIN is in the defaults if the connection is
> encrypted)?
> if not, what _exactly_ is the situation you want changed?
Suppose the intersection of the user configured mechs and the server
mechs is { LOGIN, PLAIN, SCRAM-SHA-256 }. In this case, isync will
attempt to use SCRAM-SHA-256. But suppose it fails to initiate the
authentication session due to a library error.
Speaking as a user, if I saw that the program failed to authenticate
with SCRAM-SHA-256, I would expect it to retry with one of the other
availble mechs (LOGIN and PLAIN). The ideal behaviour would be to retry
with each mech in order of descending security.
Now speaking as a developer, since PLAIN also relies on the same SASL
library, it would likely also fail due to the library error. So it's
fair to not waste effort attempting PLAIN. But LOGIN is implemented
separate to the library, meaning it can still potentially succeed. So
we can match the expected behaviour by attempting to fall back to LOGIN.
But as of current, if the library error is anything other than
SASL_NOMECH, then isync will immediately bail on authentication rather
than attempting the fallback. This is the behaviour I take issue with.
> > that will only ever trigger for a non-compliant IMAP4 server (since
> > PLAIN is required),
> >
> well, no. the example in
> https://datatracker.ietf.org/doc/html/rfc3501#section-6.1.1 shows this to be
> false. however, it also shows that absence of AUTH=PLAIN goes with presence
> of LOGINDISABLED.
Thanks for the correction. I did some experimenting with IMAP servers
and found that LOGINDISABLED is only used when the connection is not
over TLS. So my point holds for encrypted connections (which I believe
are the majority).
> in principle one could just do away with LOGIN, but PLAIN is a poor
> substitute in a non-sasl build due to the complexity.
Actually, after thinking about this, I think I've found a serious bug in
the authentication logic.
isync should only ever send plaintext credentials over an insecure
channel when the user explicitly allows it. It does this for LOGIN by
only commencing authentication over an insecure connection if the config
explicitly includes LOGIN in AuthMechs.
PLAIN also sends plaintext credentials for authentication. But isync
does not perform the same secure channel check for PLAIN that it does
for LOGIN. This means it will use PLAIN over an insecure connection
without any pushback, verification, or warning to the user.
In fact, would this be considered a security vulnerability? Since it
may be possible the user thinks the connection should be secure when it
is not. (I'm not a security expert, so forgive me if this is wrong.)
> > > 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?
> >
> yes.
> it's not useful to handle errors that cannot happen if the library is
> reasonably implemented.
The behaviour of potentially returning NULL is explicitly documented, so
it's well within reason for that to occur.
> we don't need to treat it as a black box that could
> do anything.
This would be the case if we controlled the library implementation and
linked-to version. But we don't.
The library will continue to be maintained and new versions will be
released. Versions of isync released in the past or today could be
later built with these future libraries. Since we cannot see the
future, these future libraries are literally black boxes. We cannot
know their exact implementation until they release.
We should want the isync at present to play well with the libraries
released in future. If the library later changes in a way that still
adheres to its documentation and isync is not prepared to handle that,
then that's fully on isync, not the library. And we shouldn't allow UX
to worsen if this happens (crashing is the most user unfriendly way of
informing the user of an error).
> > I was referring to my use of temporary variables across the patchset and
> > in general.
> >
> i wasn't. i don't like _these_ temporaries.
So are you okay with my other uses of temporaries then?
> > > > (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.
> > [...]
> >
> so stick to the wrapping style that is actually used all over the place?
Are you saying I can or cannot wrap here? I don't understand.
> > > you don't seem to have switched your git diff algorithm
> > > to histogram yet?
> >
> > No, I use minimal by default.
> >
> any particular reason? saving bytes/lines isn't a very useful objective when
> reviewing diffs.
I don't have any strong opinions on the matter. I just use minimal
because I haven't seen many examples of the other algorithms being
meaningfully better. Though if you'd like to provide some, I'd be happy
to take a look.
> > > > > > +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
> >
> no, it wouldn't, just like imap_auth_client_t doesn't.
Then how would it call ensure_user() or ensure_password()? Both
functions require the imap_server_conf_t instance.
The reason imap_auth_client_t doesn't is precisely because of the use of
imap_auth_cred_t to remove the direct hardcoded dependency on
imap_server_conf_t, and instead allow the calling code (the IMAP driver)
to clearly configure that dependency.
> > and ensure_(user|password)().
> >
> yes, which should be in imap_auth.c.
Do you not think that would be too tangled? Unless you want to move the
cached username and password from imap_server_conf_t to
imap_auth_client_t, which I could certainly try.
> > > > > > +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 )
> > > > > don't try to make analogous interfaces when there is a cost, but
> > > no obvious
> > > advantage for existing use cases.
> >
> > What's the cost?
> >
> the ugly return casts, if nothing else.
>
> > On the other hand, an advantage is a familiar interface.
> >
> finding an entire string in a string list isn't logically the same as
> finding a substring in one string. it's entirely counter-productive to
> pretend that the apis are the same. it's more like QList::contains().
Using a familar interface is not at all saying it is logically the same
as another function. memcpy() and memove() have identical interfaces,
but they serve different purposes.
I like the comparison to QList::contains() though. I don't think it'll
be comparatively as useful a function, but I can certainly use that kind
of interface in the next revision.
Take care,
Seth McDonald.
[0]
IMAPAccount account
Host localhost
Port 12345
Timeout 0
User [email protected]
Pass LEAKED
TLSType None
IMAPStore remote
Account account
MaildirStore local
SubFolders Verbatim
Path ~/Code/isync/maildir/
Inbox ~/Code/isync/maildir/local-inbox
Channel chan
Far :remote:
Near :local:
CopyArrivalDate yes
SyncState *
--
E9D1 26A5 F0D4 9DF7 792B C2E2 B4BF 4530 D39B 2D51
_______________________________________________
isync-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/isync-devel
