On Sat, Aug 01, 2026 at 03:59:21PM +1000, Seth McDonald wrote:
On Sun, 26 Jul 2026 at 18:16:07 +0200, Oswald Buddenhagen wrote:
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. [...]
stop right there. that does *not* qualify as "plausible situation".
i'm interested in real-world scenarios that would be expected to
actually work, rather than bailing out due to a reasonable follow-up
error.
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.
just no.
the most likely failure is a bad password. we wouldn't want to trigger
rate-limiting by hammering the server with repeated attempts.
if it's a different failure, then the user should want to investigate
it, rather than having isync silently paper over it. it's not like this
is going to happen particularly often to one user ...
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.
this is correct.
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.
debatable.
as i understand it, isync's refusal to automatically use LOGIN should be
understood as a legacy mechanism for really old servers that fail to use
LOGINDISABLED.
with SASL, the server is supposed to prevent it.
there is a hypothetical possibility that a mitm attack would force PLAIN
to steal the password, but if you can't trust the connection to the
server, then why did you disable tls?
one could also imagine the server itself being compromised, but in this
case it doesn't matter whether the connection is secure.
another aspect is that isync used to be opportunistic about using ssl by
default, while it's always enforced now. that reduces the value of the
filter.
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.
it will occur when an invalid error code is passed. in this case a crash
is a good indicator of the problem, akin to a failed assertion.
there is no _reasonable_ situation in which that function would fail
otherwise, and there won't be.
even if that turned out to be wrong for some bizarre reason, it wouldn't
have security or significant reliability implications, so what?
> 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?
i didn't notice any others that bothered me, but i will tell you when i
do a more in-depth review of a more final version.
i'm not going to have a fundamental debate about that now.
> > > (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.
dude! there are dozens of wrapped function calls in isync. just look at
how it's done. it's really a rather trivial matter of style ...
> > 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.
i think that just going by "what it says on the label" is quite
reasonable in this case, unless profound evidence to the contrary is
presented.
> > > > > +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.
so make them require an imap_auth_conf_t instance instead?
as you may have noticed, i'm getting annoyed at this point.
if you currently don't have the capacity to properly process what i
write (including the very deliberately chosen several levels of quotes),
then just do it later. i'm really in no hurry.
good luck with your studies!
_______________________________________________
isync-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/isync-devel
