Re: [PATCH v13 02/10] imap-send: add support for OAuth2.0 authentication

> On 6 Jun 2025, at 8:58 AM, Aditya Garg <gargaditya08@live.com> wrote:
> 
> 
> 
>> On 6 June 2025 12:18:25 am IST, Junio C Hamano <gitster@pobox.com> wrote:
>> Aditya Garg <gargaditya08@live.com> 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