On 6 June 2025 12:18:25 am IST, Junio C Hamano <gitster@xxxxxxxxx> wrote:
>Aditya Garg <gargaditya08@xxxxxxxx> writes:
>
>> Might look less ugly, but will result in a compiler warning that this will always
>> be true if compiled with NO_OPENSSL. If you are fine with that, good. Else tbh
>> I am out of ideas :(.
>
>Sounds like a good place to use NOT_CONSTANT(), it seems?
>
> if (NOT_CONSTANT(!auth_oauthbearer)) {
> ... skip the thing ...
> }
>
>
Ok
>>>> server_fill_credential(srvc, cred);
>>>> curl_easy_setopt(curl, CURLOPT_USERNAME, srvc->user);
>>>> - curl_easy_setopt(curl, CURLOPT_PASSWORD, srvc->pass);
>>>> +
>>>> + if (!srvc->auth_method ||
>>>> + (strcmp(srvc->auth_method, "XOAUTH2") &&
>>>> + strcmp(srvc->auth_method, "OAUTHBEARER")))
>>>> + curl_easy_setopt(curl, CURLOPT_PASSWORD, srvc->pass);
>>>
>>>Can we clarify this part, possibly with an in-code comment?
>>>
>>>"Unless XOAUTH2 or OAUTHBEARER, use the password" sounds a bit
>>>strange. What about methods other than these two that are not a
>>>plain simple password authentication? Will we remember extending
>>>this code when we add yet another one to exclude it like XOAUTH2 and
>>>OAUTHBEARER are excluded with this patch?
>
>> Let me answer this first. CURLOPT_PASSWORD is for plain or login type
>> authentication, and if srvc->auth_method is not defined, curl's behaviour
>> defaults to them.
>
>Which makes it sound like if (!srvc->auth_method) is enough?
>
No. If the user specifies PLAIN or LOGIN then it's not enough.
>> OAUTHBEARER and XOAUTH2 use CURLOPT_XOAUTH2_BEARER
>> in curl, which can use either of them based on what server says.
>
>That is what we can read from the updated code.
>
>The question is what happens when the user sets srvc->auth_method to
>something other than NULL (unused---use plain password), "XOAUTH2"
>or "OAUTHBEARER".
>
>If the answer to that question is ...
>
>> Other auth methods
>> are not supported yet in this code, and this is the reason CRAM_MD5 is supported
>> by only OpenSSL.
>
>... "with srvc->auth_method set to other methods like CRAM_MD5, the
>control would never enter this codepath, as they are implemented
>elsewhere", then I think it would make more sense to write the above
>like this:
>
> if (!srvc->auth_method)
> curl_easy_setopt(curl, CURLOPT_PASSWORD, srvc->pass);
> else if (strcmp(srvc->auth_method, "XOAUTH2") &&
> strcmp(srvc->auth_method, "OAUTHBEARER"))
> BUG("we only support XOAUTH2 and OAUTHBEARER in this codepath");
>
We can implement this, but:
1. It will fail if user specifies PLAIN or LOGIN as auth method.
2. We have this in the code as well:
if (srvc->auth_method) {
struct strbuf auth = STRBUF_INIT;
strbuf_addstr(&auth, "AUTH=");
strbuf_addstr(&auth, srvc->auth_method);
curl_easy_setopt(curl, CURLOPT_LOGIN_OPTIONS, auth.buf);
strbuf_release(&auth);
}
Which basically means that if a user specifies an auth method,
curl will try to use SMTP AUTH command with that method.
So ideally, this should have worked for OAUTHBEAER and XOAUTH2
But the problem with that would be a) we would need to format
the access token as per the specifications of these mechanisms.
and b) curl simply says these methods are not supported when
we try with that.
I filed a bug report regarding this and they were not really clear
on whether CURLOPT_LOGIN_OPTIONS is meant for PLAIN only or should work like this with other methods too.
But, the docs indicate it's for PLAIN auth only.
So, considering the fact that the original code for imap-send
was setting CURLOPT_LOGIN_OPTIONS
unconditionally and
was running the AUTH command even if auth was set to CRAM-MD5
or whatever, I just preferred to not change that behaviour since I
may cause some regression. There is a tiny possibility that CRAM-MD5
may work, but I don't really have any free SMTP server which uses
that method itself.
In short, just to be very safe here, I decided to not mingle with the
logic much and simple decided to use a seperate tested logic
for OAuth2.0 and let the same logic be used for rest cases.
Therefore, the previous logic said:
"Set CURLOPT_LOGIN_OPTIONS irrespective of whether there is
an auth method specified or not."
Now it says
"Set CURLOPT_LOGIN_OPTIONS irrespective of whether there is
an auth method specified or not, unless it's OAuth2.0, where we
use a different curl API"
The bug report I filed with curl for reference:
https://github.com/curl/curl/issues/17420
>Or the code is not protecting this code path so control can reach
>with auth_method set to CRAM_MD5 here (e.g. when built without
>OpenSSL)? If so, replace BUG("message") with die(_("message"))
>above.
>
>On the other hand, if you are trying to fall back to plain password
>when other unhandled methods are specified, I would expect that the
>code to read more like:
>
> if (srvc->auth_method &&
> (!strcmp(srvc->auth_method, "XOAUTH2") ||
> !strcmp(srvc->auth_method, "OAUTHBEARER")))
> ;
> else {
> if (srvc->auth_method)
> warning("auth method %s not supported,
> falling back to plain password",
> srvc->auth_method);
> curl_easy_setopt(curl, CURLOPT_PASSWORD, srvc->pass);
> }
>
>I cannot quite tell which one you meant, but I am guessing that the
>former is the case from your explanation.
>
>Thanks.
