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

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

[Aditya Garg]

> On 6 Jun 2025, at 12:18 AM, 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?

Ok so I was wrong here. The warning actually comes when we compile
WITHOUT NO_OPENSSL, since auth_oauthbearer is a function there, and
will attract waddress warnings,

And NOT_CONSTANT doesn't help here.
> 
>    if (NOT_CONSTANT(!auth_oauthbearer)) {
>        ... skip the thing ...
>    }
> 
> 
>>>>    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?
> 
>> 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");
> 
> 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.

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

[Aditya Garg]

> On 6 Jun 2025, at 12:29 PM, Aditya Garg <gargaditya08@live.com> wrote:
> 
> 
> 
> 
>> On 6 Jun 2025, at 12:18 AM, 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?
> 
> Ok so I was wrong here. The warning actually comes when we compile
> WITHOUT NO_OPENSSL, since auth_oauthbearer is a function there, and
> will attract waddress warnings,
> 
> And NOT_CONSTANT doesn't help here.

So this brings us back to the same problem. Compiled warnings (Waddress).
Shown without NO_OPENSSL and not with OPENSSL, with no other way in my
mind than using macros for the compiler.

Or maybe we can just use the original way, in which it died using a function?
>> 
>>   if (NOT_CONSTANT(!auth_oauthbearer)) {
>>       ... skip the thing ...
>>   }
>> 
>> 
>>>>>   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?
>> 
>>> 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");
>> 
>> 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.

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

[Junio C Hamano]

Aditya Garg <gargaditya08@live.com> writes:

> Ok so I was wrong here. The warning actually comes when we compile
> WITHOUT NO_OPENSSL, since auth_oauthbearer is a function there, and
> will attract waddress warnings,

But the patch in this thread, when compiled with NO_OPENSSL, gives
"-Wunreachable".

--- * ------ * ------ * ------ * ------ * ---

$ make V=1 CC=clang DEVELOPER=YesPlease CFLAGS=-Waddress NO_OPENSSL=NoThanks imap-send.o
    * new build flags
clang -o imap-send.o -c -MF ./.depend/imap-send.o.d -MQ imap-send.o -MMD -MP  -Werror -Wall -pedantic -Wpedantic -std=gnu99 -Wdeclaration-after-statement -Wformat-security -Wold-style-definition -Woverflow -Wpointer-arith -Wstrict-prototypes -Wunused -Wvla -Wwrite-strings -fno-common -Wunreachable-code -Wcomma -Wtautological-constant-out-of-range-compare -Wextra -Wmissing-prototypes -Wno-empty-body -Wno-missing-field-initializers  -Waddress  -I. -DGIT_HOST_CPU="\"x86_64\"" -DHAVE_ALLOCA_H  -DUSE_CURL_FOR_IMAP_SEND -DNO_OPENSSL -DSUPPORTS_SIMPLE_IPC -DSHA1_DC -DSHA1DC_NO_STANDARD_INCLUDES -DSHA1DC_INIT_SAFE_HASH_DEFAULT=0 -DSHA1DC_CUSTOM_INCLUDE_SHA1_C="\"git-compat-util.h\"" -DSHA1DC_CUSTOM_INCLUDE_UBC_CHECK_C="\"git-compat-util.h\"" -DSHA256_BLK  -DHAVE_PATHS_H -DHAVE_DEV_TTY -DHAVE_CLOCK_GETTIME -DHAVE_CLOCK_MONOTONIC -DHAVE_SYNC_FILE_RANGE -DHAVE_SYSINFO -DHAVE_GETDELIM -DHAVE_GETRANDOM -DFREAD_READS_DIRECTORIES -DNO_STRLCPY -DSHELL_PATH='"/bin/sh"'  imap-send.c
imap-send.c:1344:5: error: code will never be executed [-Werror,-Wunreachable-code]
 1344 |                                 memset(&cb, 0, sizeof(cb));
      |                                 ^~~~~~
imap-send.c:1322:5: error: code will never be executed [-Werror,-Wunreachable-code]
 1322 |                                 memset(&cb, 0, sizeof(cb));
      |                                 ^~~~~~
imap-send.c:1300:5: error: code will never be executed [-Werror,-Wunreachable-code]
 1300 |                                 memset(&cb, 0, sizeof(cb));
      |                                 ^~~~~~
imap-send.c:1278:5: error: code will never be executed [-Werror,-Wunreachable-code]
 1278 |                                 memset(&cb, 0, sizeof(cb));
      |                                 ^~~~~~
4 errors generated.
make: *** [Makefile:2820: imap-send.o] Error 1

--- * ------ * ------ * ------ * ------ * ---

See the patch attached at the end [*] that you can apply to 'seen'
to try out; the result compiles with all combinations of gcc/clang
and with and without NO_OPENSSL.

Having said that, I think you should first clean up the repetitions
before piling a lot of different auth_method on top.

A part of the resulting code after applying [*] to 'seen' looks like
this:

	} else if (!strcmp(srvc->auth_method, "CRAM-MD5")) {
		if (!CAP(AUTH_CRAM_MD5)) {
			fprintf(stderr, "You specified "
				"CRAM-MD5 as authentication method, "
				"but %s doesn't support it.\n", srvc->host);
			goto bail;
		}

		/* CRAM-MD5 */
		memset(&cb, 0, sizeof(cb));
		cb.cont = auth_cram_md5;

		if (NOT_CONSTANT(!cb.cont)) {
		fprintf(stderr, "If you want to use CRAM-MD5 authentication mechanism, "
			"you have to build git-imap-send with OpenSSL library.");
		goto bail;
		}
		if (imap_exec(ctx, &cb, "AUTHENTICATE CRAM-MD5") != RESP_OK) {
			fprintf(stderr, "IMAP error: AUTHENTICATE CRAM-MD5 failed\n");
			goto bail;
		}

This pattern repeats for different auth methods as you add
copy-and-paste, which made the code way too long and too deeply
indented to be readable.  Why not make the above into a single
helper function that you can reuse, so that the call site can become
something like

	} else if (!strcmp(srvc->auth_method, "PLAIN")) {
	    if (try_auth_method(srvc, ctx, "PLAIN", AUTH_PLAIN, auth_plain))
		goto bail;
	} else if (!strcmp(srvc->auth_method, "CRAM-MD5")) {
	    if (try_auth_method(srvc, ctx, "CRAM-MD5", AUTH_CRAM_MD5, auth_cram_md5))
		goto bail;
	} ...

If you did it at the very beginning of the series (i.e. when you
only have CRAM_MD5 you inherited from the original before adding all
the other new ones), each addition would become a lot simpler and
main flow in the the resulting code would become quite simple like
the above.

static int try_auth_method(struct imap_server_conf *srvc,
			   struct imap_store *ctx,
			   const char *auth_method,
			   enum CAPABILITY cap,
			   int (*fn)(struct imap_store *, const char *))
{
        struct imap_cmd_cb cb = {0};

	if (!CAP(cap)) {
		fprintf(stderr, "You specified "
			"%s as authentication method, "
			"but %s doesn't support it.\n",
			auth_method, srvc->host);
		return -1;
	}
	cb.cont = fn;

	if (NOT_CONSTANT(!cb.cont)) {
		fprintf(stderr, "If you want to use %s authentication mechanism, "
			"you have to build git-imap-send with OpenSSL library.",
			auth_method);
		return -1;
	}
	if (imap_exec(ctx, &cb, "AUTHENTICATE %s", auth_method) != RESP_OK) {
		fprintf(stderr, "IMAP error: AUTHENTICATE %s failed\n",
                	auth_method);
		return -1;
	}
        return 0;
}

So here is that [*] patch that is incorrectly indented to avoid even
deeper nesting.

---- >8 ----

 imap-send.c | 49 ++++++++++++++++++++++++-------------------------
 1 file changed, 24 insertions(+), 25 deletions(-)

diff --git i/imap-send.c w/imap-send.c
index 6dbbf37125..bba2ef0cd0 100644
--- i/imap-send.c
+++ w/imap-send.c
@@ -1267,16 +1267,16 @@ static struct imap_store *imap_open_store(struct imap_server_conf *srvc, const c
 					goto bail;
 				}
 
-				#ifdef NO_OPENSSL
+				/* PLAIN */
+				memset(&cb, 0, sizeof(cb));
+				cb.cont = auth_plain;
+
+				if (NOT_CONSTANT(!cb.cont)) {
 				fprintf(stderr, "You are trying to use PLAIN authentication mechanism "
 					"with OpenSSL library, but its support has not been compiled in.");
 				goto bail;
-				#endif
-
-				/* PLAIN */
+				}
 
-				memset(&cb, 0, sizeof(cb));
-				cb.cont = auth_plain;
 				if (imap_exec(ctx, &cb, "AUTHENTICATE PLAIN") != RESP_OK) {
 					fprintf(stderr, "IMAP error: AUTHENTICATE PLAIN failed\n");
 					goto bail;
@@ -1289,16 +1289,16 @@ static struct imap_store *imap_open_store(struct imap_server_conf *srvc, const c
 					goto bail;
 				}
 
-				#ifdef NO_OPENSSL
-				fprintf(stderr, "If you want to use CRAM-MD5 authentication mechanism, "
-					"you have to build git-imap-send with OpenSSL library.");
-				goto bail;
-				#endif
 
 				/* CRAM-MD5 */
-
 				memset(&cb, 0, sizeof(cb));
 				cb.cont = auth_cram_md5;
+
+				if (NOT_CONSTANT(!cb.cont)) {
+				fprintf(stderr, "If you want to use CRAM-MD5 authentication mechanism, "
+					"you have to build git-imap-send with OpenSSL library.");
+				goto bail;
+				}
 				if (imap_exec(ctx, &cb, "AUTHENTICATE CRAM-MD5") != RESP_OK) {
 					fprintf(stderr, "IMAP error: AUTHENTICATE CRAM-MD5 failed\n");
 					goto bail;
@@ -1311,16 +1311,15 @@ static struct imap_store *imap_open_store(struct imap_server_conf *srvc, const c
 					goto bail;
 				}
 
-				#ifdef NO_OPENSSL
-				fprintf(stderr, "You are trying to use OAUTHBEARER authentication mechanism "
-					"with OpenSSL library, but its support has not been compiled in.");
-				goto bail;
-				#endif
-
 				/* OAUTHBEARER */
-
 				memset(&cb, 0, sizeof(cb));
 				cb.cont = auth_oauthbearer;
+
+				if (NOT_CONSTANT(!cb.cont)) {
+				fprintf(stderr, "You are trying to use OAUTHBEARER authentication mechanism "
+					"with OpenSSL library, but its support has not been compiled in.");
+				goto bail;
+				}
 				if (imap_exec(ctx, &cb, "AUTHENTICATE OAUTHBEARER") != RESP_OK) {
 					fprintf(stderr, "IMAP error: AUTHENTICATE OAUTHBEARER failed\n");
 					goto bail;
@@ -1333,16 +1332,16 @@ static struct imap_store *imap_open_store(struct imap_server_conf *srvc, const c
 					goto bail;
 				}
 
-				#ifdef NO_OPENSSL
+				/* XOAUTH2 */
+				memset(&cb, 0, sizeof(cb));
+				cb.cont = auth_xoauth2;
+
+				if (NOT_CONSTANT(!cb.cont)) {
 				fprintf(stderr, "You are trying to use XOAUTH2 authentication mechanism "
 					"with OpenSSL library, but its support has not been compiled in.");
 				goto bail;
-				#endif
-
-				/* XOAUTH2 */
+				}
 
-				memset(&cb, 0, sizeof(cb));
-				cb.cont = auth_xoauth2;
 				if (imap_exec(ctx, &cb, "AUTHENTICATE XOAUTH2") != RESP_OK) {
 					fprintf(stderr, "IMAP error: AUTHENTICATE XOAUTH2 failed\n");
 					goto bail;

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

[Aditya Garg]

> On 6 Jun 2025, at 10:08 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 
> Aditya Garg <gargaditya08@live.com> writes:
> 
>> Ok so I was wrong here. The warning actually comes when we compile
>> WITHOUT NO_OPENSSL, since auth_oauthbearer is a function there, and
>> will attract waddress warnings,
> 
> But the patch in this thread, when compiled with NO_OPENSSL, gives
> "-Wunreachable".

Uggh
> 
> --- * ------ * ------ * ------ * ------ * ---
> 
> $ make V=1 CC=clang DEVELOPER=YesPlease CFLAGS=-Waddress NO_OPENSSL=NoThanks imap-send.o
>    * new build flags
> clang -o imap-send.o -c -MF ./.depend/imap-send.o.d -MQ imap-send.o -MMD -MP  -Werror -Wall -pedantic -Wpedantic -std=gnu99 -Wdeclaration-after-statement -Wformat-security -Wold-style-definition -Woverflow -Wpointer-arith -Wstrict-prototypes -Wunused -Wvla -Wwrite-strings -fno-common -Wunreachable-code -Wcomma -Wtautological-constant-out-of-range-compare -Wextra -Wmissing-prototypes -Wno-empty-body -Wno-missing-field-initializers  -Waddress  -I. -DGIT_HOST_CPU="\"x86_64\"" -DHAVE_ALLOCA_H  -DUSE_CURL_FOR_IMAP_SEND -DNO_OPENSSL -DSUPPORTS_SIMPLE_IPC -DSHA1_DC -DSHA1DC_NO_STANDARD_INCLUDES -DSHA1DC_INIT_SAFE_HASH_DEFAULT=0 -DSHA1DC_CUSTOM_INCLUDE_SHA1_C="\"git-compat-util.h\"" -DSHA1DC_CUSTOM_INCLUDE_UBC_CHECK_C="\"git-compat-util.h\"" -DSHA256_BLK  -DHAVE_PATHS_H -DHAVE_DEV_TTY -DHAVE_CLOCK_GETTIME -DHAVE_CLOCK_MONOTONIC -DHAVE_SYNC_FILE_RANGE -DHAVE_SYSINFO -DHAVE_GETDELIM -DHAVE_GETRANDOM -DFREAD_READS_DIRECTORIES -DNO_STRLCPY -DSHELL_PATH='"/bin/sh"'  imap-send.c
> imap-send.c:1344:5: error: code will never be executed [-Werror,-Wunreachable-code]
> 1344 |                                 memset(&cb, 0, sizeof(cb));
>      |                                 ^~~~~~
> imap-send.c:1322:5: error: code will never be executed [-Werror,-Wunreachable-code]
> 1322 |                                 memset(&cb, 0, sizeof(cb));
>      |                                 ^~~~~~
> imap-send.c:1300:5: error: code will never be executed [-Werror,-Wunreachable-code]
> 1300 |                                 memset(&cb, 0, sizeof(cb));
>      |                                 ^~~~~~
> imap-send.c:1278:5: error: code will never be executed [-Werror,-Wunreachable-code]
> 1278 |                                 memset(&cb, 0, sizeof(cb));
>      |                                 ^~~~~~
> 4 errors generated.
> make: *** [Makefile:2820: imap-send.o] Error 1
> 
> --- * ------ * ------ * ------ * ------ * ---
> 
> See the patch attached at the end [*] that you can apply to 'seen'
> to try out; the result compiles with all combinations of gcc/clang
> and with and without NO_OPENSSL.

Great, thanks for the patch. Lemme see if I can make something nice.
> 
> Having said that, I think you should first clean up the repetitions
> before piling a lot of different auth_method on top.
> 
> A part of the resulting code after applying [*] to 'seen' looks like
> this:
> 
>    } else if (!strcmp(srvc->auth_method, "CRAM-MD5")) {
>        if (!CAP(AUTH_CRAM_MD5)) {
>            fprintf(stderr, "You specified "
>                "CRAM-MD5 as authentication method, "
>                "but %s doesn't support it.\n", srvc->host);
>            goto bail;
>        }
> 
>        /* CRAM-MD5 */
>        memset(&cb, 0, sizeof(cb));
>        cb.cont = auth_cram_md5;
> 
>        if (NOT_CONSTANT(!cb.cont)) {
>        fprintf(stderr, "If you want to use CRAM-MD5 authentication mechanism, "
>            "you have to build git-imap-send with OpenSSL library.");
>        goto bail;
>        }
>        if (imap_exec(ctx, &cb, "AUTHENTICATE CRAM-MD5") != RESP_OK) {
>            fprintf(stderr, "IMAP error: AUTHENTICATE CRAM-MD5 failed\n");
>            goto bail;
>        }
> 
> This pattern repeats for different auth methods as you add
> copy-and-paste, which made the code way too long and too deeply
> indented to be readable.  Why not make the above into a single
> helper function that you can reuse, so that the call site can become
> something like

I was thinking the same but though the code would be more readable
in case if repeats. With GCC, even the binaries were of the same size.
> 
>    } else if (!strcmp(srvc->auth_method, "PLAIN")) {
>        if (try_auth_method(srvc, ctx, "PLAIN", AUTH_PLAIN, auth_plain))
>        goto bail;
>    } else if (!strcmp(srvc->auth_method, "CRAM-MD5")) {
>        if (try_auth_method(srvc, ctx, "CRAM-MD5", AUTH_CRAM_MD5, auth_cram_md5))
>        goto bail;
>    } ...
> 
> If you did it at the very beginning of the series (i.e. when you
> only have CRAM_MD5 you inherited from the original before adding all
> the other new ones), each addition would become a lot simpler and
> main flow in the the resulting code would become quite simple like
> the above.
> 
> static int try_auth_method(struct imap_server_conf *srvc,
>               struct imap_store *ctx,
>               const char *auth_method,
>               enum CAPABILITY cap,
>               int (*fn)(struct imap_store *, const char *))
> {
>        struct imap_cmd_cb cb = {0};
> 
>    if (!CAP(cap)) {
>        fprintf(stderr, "You specified "
>            "%s as authentication method, "
>            "but %s doesn't support it.\n",
>            auth_method, srvc->host);
>        return -1;
>    }
>    cb.cont = fn;
> 
>    if (NOT_CONSTANT(!cb.cont)) {
>        fprintf(stderr, "If you want to use %s authentication mechanism, "
>            "you have to build git-imap-send with OpenSSL library.",
>            auth_method);
>        return -1;
>    }
>    if (imap_exec(ctx, &cb, "AUTHENTICATE %s", auth_method) != RESP_OK) {
>        fprintf(stderr, "IMAP error: AUTHENTICATE %s failed\n",
>                    auth_method);
>        return -1;
>    }
>        return 0;
> }
> 
> So here is that [*] patch that is incorrectly indented to avoid even
> deeper nesting.
> 
> ---- >8 ----
> 
> imap-send.c | 49 ++++++++++++++++++++++++-------------------------
> 1 file changed, 24 insertions(+), 25 deletions(-)
> 
> diff --git i/imap-send.c w/imap-send.c
> index 6dbbf37125..bba2ef0cd0 100644
> --- i/imap-send.c
> +++ w/imap-send.c
> @@ -1267,16 +1267,16 @@ static struct imap_store *imap_open_store(struct imap_server_conf *srvc, const c
>                    goto bail;
>                }
> 
> -                #ifdef NO_OPENSSL
> +                /* PLAIN */
> +                memset(&cb, 0, sizeof(cb));
> +                cb.cont = auth_plain;
> +
> +                if (NOT_CONSTANT(!cb.cont)) {
>                fprintf(stderr, "You are trying to use PLAIN authentication mechanism "
>                    "with OpenSSL library, but its support has not been compiled in.");
>                goto bail;
> -                #endif
> -
> -                /* PLAIN */
> +                }
> 
> -                memset(&cb, 0, sizeof(cb));
> -                cb.cont = auth_plain;
>                if (imap_exec(ctx, &cb, "AUTHENTICATE PLAIN") != RESP_OK) {
>                    fprintf(stderr, "IMAP error: AUTHENTICATE PLAIN failed\n");
>                    goto bail;
> @@ -1289,16 +1289,16 @@ static struct imap_store *imap_open_store(struct imap_server_conf *srvc, const c
>                    goto bail;
>                }
> 
> -                #ifdef NO_OPENSSL
> -                fprintf(stderr, "If you want to use CRAM-MD5 authentication mechanism, "
> -                    "you have to build git-imap-send with OpenSSL library.");
> -                goto bail;
> -                #endif
> 
>                /* CRAM-MD5 */
> -
>                memset(&cb, 0, sizeof(cb));
>                cb.cont = auth_cram_md5;
> +
> +                if (NOT_CONSTANT(!cb.cont)) {
> +                fprintf(stderr, "If you want to use CRAM-MD5 authentication mechanism, "
> +                    "you have to build git-imap-send with OpenSSL library.");
> +                goto bail;
> +                }
>                if (imap_exec(ctx, &cb, "AUTHENTICATE CRAM-MD5") != RESP_OK) {
>                    fprintf(stderr, "IMAP error: AUTHENTICATE CRAM-MD5 failed\n");
>                    goto bail;
> @@ -1311,16 +1311,15 @@ static struct imap_store *imap_open_store(struct imap_server_conf *srvc, const c
>                    goto bail;
>                }
> 
> -                #ifdef NO_OPENSSL
> -                fprintf(stderr, "You are trying to use OAUTHBEARER authentication mechanism "
> -                    "with OpenSSL library, but its support has not been compiled in.");
> -                goto bail;
> -                #endif
> -
>                /* OAUTHBEARER */
> -
>                memset(&cb, 0, sizeof(cb));
>                cb.cont = auth_oauthbearer;
> +
> +                if (NOT_CONSTANT(!cb.cont)) {
> +                fprintf(stderr, "You are trying to use OAUTHBEARER authentication mechanism "
> +                    "with OpenSSL library, but its support has not been compiled in.");
> +                goto bail;
> +                }
>                if (imap_exec(ctx, &cb, "AUTHENTICATE OAUTHBEARER") != RESP_OK) {
>                    fprintf(stderr, "IMAP error: AUTHENTICATE OAUTHBEARER failed\n");
>                    goto bail;
> @@ -1333,16 +1332,16 @@ static struct imap_store *imap_open_store(struct imap_server_conf *srvc, const c
>                    goto bail;
>                }
> 
> -                #ifdef NO_OPENSSL
> +                /* XOAUTH2 */
> +                memset(&cb, 0, sizeof(cb));
> +                cb.cont = auth_xoauth2;
> +
> +                if (NOT_CONSTANT(!cb.cont)) {
>                fprintf(stderr, "You are trying to use XOAUTH2 authentication mechanism "
>                    "with OpenSSL library, but its support has not been compiled in.");
>                goto bail;
> -                #endif
> -
> -                /* XOAUTH2 */
> +                }
> 
> -                memset(&cb, 0, sizeof(cb));
> -                cb.cont = auth_xoauth2;
>                if (imap_exec(ctx, &cb, "AUTHENTICATE XOAUTH2") != RESP_OK) {
>                    fprintf(stderr, "IMAP error: AUTHENTICATE XOAUTH2 failed\n");
>                    goto bail;

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

[Junio C Hamano]

Aditya Garg <gargaditya08@live.com> writes:

> I was thinking the same but though the code would be more readable
> in case if repeats.

Sorry but I do not understand this comment at all.  Not repeating so
that you do not have to fix or update all the repetitions later when
the code needs to do something different is one of the basics in the
software engineering, and less repetitious code is also easier to
understnad in the art of software field.

If you start from a 20-line block and repeat it 5 times to grow it
100 lines of code, the eyes of readers will start coasting over
after a few repetitions, and you may be able to smuggle unwanted
lines unnoticed.  Unless you are aiming for such technique to do
something malicious, don't go there.

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

[Aditya Garg]

> On 6 Jun 2025, at 10:34 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 
> Aditya Garg <gargaditya08@live.com> writes:
> 
>> I was thinking the same but though the code would be more readable
>> in case if repeats.
> 
> Sorry but I do not understand this comment at all.  Not repeating so
> that you do not have to fix or update all the repetitions later when
> the code needs to do something different is one of the basics in the
> software engineering, and less repetitious code is also easier to
> understnad in the art of software field.
> 
> If you start from a 20-line block and repeat it 5 times to grow it
> 100 lines of code, the eyes of readers will start coasting over
> after a few repetitions, and you may be able to smuggle unwanted
> lines unnoticed.  Unless you are aiming for such technique to do
> something malicious, don't go there.

OMG why would I be malicious (˃̣̣̥ᯅ˂̣̣̥) It's just a matter of my personal taste.
Obviously your points are valid. And sorry for my ignorance, I am still
a rookie dev.

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

[Aditya Garg]

> On 6 Jun 2025, at 10:08 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 
> Aditya Garg <gargaditya08@live.com> writes:
> 
>> Ok so I was wrong here. The warning actually comes when we compile
>> WITHOUT NO_OPENSSL, since auth_oauthbearer is a function there, and
>> will attract waddress warnings,
> 
> But the patch in this thread, when compiled with NO_OPENSSL, gives
> "-Wunreachable".
> 
> --- * ------ * ------ * ------ * ------ * ---
> 
> $ make V=1 CC=clang DEVELOPER=YesPlease CFLAGS=-Waddress NO_OPENSSL=NoThanks imap-send.o
>    * new build flags
> clang -o imap-send.o -c -MF ./.depend/imap-send.o.d -MQ imap-send.o -MMD -MP  -Werror -Wall -pedantic -Wpedantic -std=gnu99 -Wdeclaration-after-statement -Wformat-security -Wold-style-definition -Woverflow -Wpointer-arith -Wstrict-prototypes -Wunused -Wvla -Wwrite-strings -fno-common -Wunreachable-code -Wcomma -Wtautological-constant-out-of-range-compare -Wextra -Wmissing-prototypes -Wno-empty-body -Wno-missing-field-initializers  -Waddress  -I. -DGIT_HOST_CPU="\"x86_64\"" -DHAVE_ALLOCA_H  -DUSE_CURL_FOR_IMAP_SEND -DNO_OPENSSL -DSUPPORTS_SIMPLE_IPC -DSHA1_DC -DSHA1DC_NO_STANDARD_INCLUDES -DSHA1DC_INIT_SAFE_HASH_DEFAULT=0 -DSHA1DC_CUSTOM_INCLUDE_SHA1_C="\"git-compat-util.h\"" -DSHA1DC_CUSTOM_INCLUDE_UBC_CHECK_C="\"git-compat-util.h\"" -DSHA256_BLK  -DHAVE_PATHS_H -DHAVE_DEV_TTY -DHAVE_CLOCK_GETTIME -DHAVE_CLOCK_MONOTONIC -DHAVE_SYNC_FILE_RANGE -DHAVE_SYSINFO -DHAVE_GETDELIM -DHAVE_GETRANDOM -DFREAD_READS_DIRECTORIES -DNO_STRLCPY -DSHELL_PATH='"/bin/sh"'  imap-send.c
> imap-send.c:1344:5: error: code will never be executed [-Werror,-Wunreachable-code]
> 1344 |                                 memset(&cb, 0, sizeof(cb));
>      |                                 ^~~~~~
> imap-send.c:1322:5: error: code will never be executed [-Werror,-Wunreachable-code]
> 1322 |                                 memset(&cb, 0, sizeof(cb));
>      |                                 ^~~~~~
> imap-send.c:1300:5: error: code will never be executed [-Werror,-Wunreachable-code]
> 1300 |                                 memset(&cb, 0, sizeof(cb));
>      |                                 ^~~~~~
> imap-send.c:1278:5: error: code will never be executed [-Werror,-Wunreachable-code]
> 1278 |                                 memset(&cb, 0, sizeof(cb));
>      |                                 ^~~~~~
> 4 errors generated.
> make: *** [Makefile:2820: imap-send.o] Error 1
> 
> --- * ------ * ------ * ------ * ------ * ---
> 
> See the patch attached at the end [*] that you can apply to 'seen'
> to try out; the result compiles with all combinations of gcc/clang
> and with and without NO_OPENSSL.
> 
> Having said that, I think you should first clean up the repetitions
> before piling a lot of different auth_method on top.
> 
> A part of the resulting code after applying [*] to 'seen' looks like
> this:
> 
>    } else if (!strcmp(srvc->auth_method, "CRAM-MD5")) {
>        if (!CAP(AUTH_CRAM_MD5)) {
>            fprintf(stderr, "You specified "
>                "CRAM-MD5 as authentication method, "
>                "but %s doesn't support it.\n", srvc->host);
>            goto bail;
>        }
> 
>        /* CRAM-MD5 */
>        memset(&cb, 0, sizeof(cb));
>        cb.cont = auth_cram_md5;
> 
>        if (NOT_CONSTANT(!cb.cont)) {
>        fprintf(stderr, "If you want to use CRAM-MD5 authentication mechanism, "
>            "you have to build git-imap-send with OpenSSL library.");
>        goto bail;
>        }
>        if (imap_exec(ctx, &cb, "AUTHENTICATE CRAM-MD5") != RESP_OK) {
>            fprintf(stderr, "IMAP error: AUTHENTICATE CRAM-MD5 failed\n");
>            goto bail;
>        }
> 
> This pattern repeats for different auth methods as you add
> copy-and-paste, which made the code way too long and too deeply
> indented to be readable.  Why not make the above into a single
> helper function that you can reuse, so that the call site can become
> something like
> 
>    } else if (!strcmp(srvc->auth_method, "PLAIN")) {
>        if (try_auth_method(srvc, ctx, "PLAIN", AUTH_PLAIN, auth_plain))
>        goto bail;
>    } else if (!strcmp(srvc->auth_method, "CRAM-MD5")) {
>        if (try_auth_method(srvc, ctx, "CRAM-MD5", AUTH_CRAM_MD5, auth_cram_md5))
>        goto bail;
>    } ...
> 
> If you did it at the very beginning of the series (i.e. when you
> only have CRAM_MD5 you inherited from the original before adding all
> the other new ones), each addition would become a lot simpler and
> main flow in the the resulting code would become quite simple like
> the above.
> 
> static int try_auth_method(struct imap_server_conf *srvc,
>               struct imap_store *ctx,
Had to add struct imap *imap, as well here. With that added, a v14 has been sent.
>               const char *auth_method,
>               enum CAPABILITY cap,
>               int (*fn)(struct imap_store *, const char *))
> {
>        struct imap_cmd_cb cb = {0};
> 
>    if (!CAP(cap)) {
>        fprintf(stderr, "You specified "
>            "%s as authentication method, "
>            "but %s doesn't support it.\n",
>            auth_method, srvc->host);
>        return -1;
>    }
>    cb.cont = fn;
> 
>    if (NOT_CONSTANT(!cb.cont)) {
>        fprintf(stderr, "If you want to use %s authentication mechanism, "
>            "you have to build git-imap-send with OpenSSL library.",
>            auth_method);
>        return -1;
>    }
>    if (imap_exec(ctx, &cb, "AUTHENTICATE %s", auth_method) != RESP_OK) {
>        fprintf(stderr, "IMAP error: AUTHENTICATE %s failed\n",
>                    auth_method);
>        return -1;
>    }
>        return 0;
> }
> 
> So here is that [*] patch that is incorrectly indented to avoid even
> deeper nesting.
> 
> ---- >8 ----
> 
> imap-send.c | 49 ++++++++++++++++++++++++-------------------------
> 1 file changed, 24 insertions(+), 25 deletions(-)
> 
> diff --git i/imap-send.c w/imap-send.c
> index 6dbbf37125..bba2ef0cd0 100644
> --- i/imap-send.c
> +++ w/imap-send.c
> @@ -1267,16 +1267,16 @@ static struct imap_store *imap_open_store(struct imap_server_conf *srvc, const c
>                    goto bail;
>                }
> 
> -                #ifdef NO_OPENSSL
> +                /* PLAIN */
> +                memset(&cb, 0, sizeof(cb));
> +                cb.cont = auth_plain;
> +
> +                if (NOT_CONSTANT(!cb.cont)) {
>                fprintf(stderr, "You are trying to use PLAIN authentication mechanism "
>                    "with OpenSSL library, but its support has not been compiled in.");
>                goto bail;
> -                #endif
> -
> -                /* PLAIN */
> +                }
> 
> -                memset(&cb, 0, sizeof(cb));
> -                cb.cont = auth_plain;
>                if (imap_exec(ctx, &cb, "AUTHENTICATE PLAIN") != RESP_OK) {
>                    fprintf(stderr, "IMAP error: AUTHENTICATE PLAIN failed\n");
>                    goto bail;
> @@ -1289,16 +1289,16 @@ static struct imap_store *imap_open_store(struct imap_server_conf *srvc, const c
>                    goto bail;
>                }
> 
> -                #ifdef NO_OPENSSL
> -                fprintf(stderr, "If you want to use CRAM-MD5 authentication mechanism, "
> -                    "you have to build git-imap-send with OpenSSL library.");
> -                goto bail;
> -                #endif
> 
>                /* CRAM-MD5 */
> -
>                memset(&cb, 0, sizeof(cb));
>                cb.cont = auth_cram_md5;
> +
> +                if (NOT_CONSTANT(!cb.cont)) {
> +                fprintf(stderr, "If you want to use CRAM-MD5 authentication mechanism, "
> +                    "you have to build git-imap-send with OpenSSL library.");
> +                goto bail;
> +                }
>                if (imap_exec(ctx, &cb, "AUTHENTICATE CRAM-MD5") != RESP_OK) {
>                    fprintf(stderr, "IMAP error: AUTHENTICATE CRAM-MD5 failed\n");
>                    goto bail;
> @@ -1311,16 +1311,15 @@ static struct imap_store *imap_open_store(struct imap_server_conf *srvc, const c
>                    goto bail;
>                }
> 
> -                #ifdef NO_OPENSSL
> -                fprintf(stderr, "You are trying to use OAUTHBEARER authentication mechanism "
> -                    "with OpenSSL library, but its support has not been compiled in.");
> -                goto bail;
> -                #endif
> -
>                /* OAUTHBEARER */
> -
>                memset(&cb, 0, sizeof(cb));
>                cb.cont = auth_oauthbearer;
> +
> +                if (NOT_CONSTANT(!cb.cont)) {
> +                fprintf(stderr, "You are trying to use OAUTHBEARER authentication mechanism "
> +                    "with OpenSSL library, but its support has not been compiled in.");
> +                goto bail;
> +                }
>                if (imap_exec(ctx, &cb, "AUTHENTICATE OAUTHBEARER") != RESP_OK) {
>                    fprintf(stderr, "IMAP error: AUTHENTICATE OAUTHBEARER failed\n");
>                    goto bail;
> @@ -1333,16 +1332,16 @@ static struct imap_store *imap_open_store(struct imap_server_conf *srvc, const c
>                    goto bail;
>                }
> 
> -                #ifdef NO_OPENSSL
> +                /* XOAUTH2 */
> +                memset(&cb, 0, sizeof(cb));
> +                cb.cont = auth_xoauth2;
> +
> +                if (NOT_CONSTANT(!cb.cont)) {
>                fprintf(stderr, "You are trying to use XOAUTH2 authentication mechanism "
>                    "with OpenSSL library, but its support has not been compiled in.");
>                goto bail;
> -                #endif
> -
> -                /* XOAUTH2 */
> +                }
> 
> -                memset(&cb, 0, sizeof(cb));
> -                cb.cont = auth_xoauth2;
>                if (imap_exec(ctx, &cb, "AUTHENTICATE XOAUTH2") != RESP_OK) {
>                    fprintf(stderr, "IMAP error: AUTHENTICATE XOAUTH2 failed\n");
>                    goto bail;

[PATCH v13 00/10] imap-send: make it usable again and add OAuth2.0 support

[Aditya Garg]

This patch series does the following things:
Firstly it basically makes the imap-send command usable again since it
was broken because of not being able to correctly parse the config file.

Further it adds support for OAuth2.0 and PLAIN authentication to git
imap-send.

Last, it does some minor improvements including adding the ability to
specify the folder using the command line and ability to list the
available folders by adding a `--list` option.

P.S.: I am surprised this thing even exists xD.

v2:  - Added support for OAuth2.0 with curl.
     - Fixed the memory leak in case auth_cram_md5 fails.
v3:  - Improve wording in first patch
     - Change misleading message if OAuth2.0 is used without OpenSSL
v4:  - Add PLAIN authentication mechanism for OpenSSL
     - Improved wording in the first patch a bit more
v5:  - Add ability to specify destination folder using the command line
     - Add ability to set a default between curl and openssl using the config
v6:  - Fix minor mistakes in --folder documentation
v7:  - Fix spelling and grammar mistakes in logs shown to the user when running imap-send
     - Display port alongwith host when git credential is invoked and asks for a password
     - Display the destination mailbox when sending a message
v8:  - Drop the patch that enabled user to choose between libcurl and openssl using the config
     - Add ability to list the available folders by adding a `--list` option
v9:  - Encourage users to use OAuth2.0 for Gmail (similar change done for send-email docs).
v10: - Fix comment styles
     - Fix failing tests
v11: - Use lower case letters for the first word of a sendtence in an error message
       and avoid using full stops at the end of a sentence.
v12: - Gracefully exit PLAIN, CRAM-MD5, OAUTHBEARER and XOAUTH2 authentication methods
       if OpenSSL support is not compiled in, but is requested by the user.
     - Use backticks for string literals.
     - Wrap documentation text to 75 columns.
     - End the last member of enum CAPABILITY with a trailing comma.
v13: - Fix logic error which was using || instead of && when checking if
       the authentication method is neither XOAUTH2 nor OAUTHBEARER.

Aditya Garg (10):
  imap-send: fix bug causing cfg->folder being set to NULL
  imap-send: add support for OAuth2.0 authentication
  imap-send: add PLAIN authentication method to OpenSSL
  imap-send: fix memory leak in case auth_cram_md5 fails
  imap-send: gracefully fail if CRAM-MD5 authentication is requested
    without OpenSSL
  imap-send: enable specifying the folder using the command line
  imap-send: fix minor mistakes in the logs
  imap-send: display port alongwith host when git credential is invoked
  imap-send: display the destination mailbox when sending a message
  imap-send: add ability to list the available folders

 Documentation/config/imap.adoc   |  11 +-
 Documentation/git-imap-send.adoc |  68 ++++-
 imap-send.c                      | 425 +++++++++++++++++++++++++++----
 3 files changed, 441 insertions(+), 63 deletions(-)

Range-diff against v12:
 -:  ---------- >  1:  3e3ddf7077 imap-send: fix bug causing cfg->folder being set to NULL
 1:  ab12f713d2 !  2:  0d28e337cf imap-send: add support for OAuth2.0 authentication
    @@ imap-send.c: static CURL *setup_curl(struct imap_server_conf *srvc, struct crede
     -	curl_easy_setopt(curl, CURLOPT_PASSWORD, srvc->pass);
     +
     +	if (!srvc->auth_method ||
    -+	    strcmp(srvc->auth_method, "XOAUTH2") ||
    -+	    strcmp(srvc->auth_method, "OAUTHBEARER"))
    ++	    (strcmp(srvc->auth_method, "XOAUTH2") &&
    ++	    strcmp(srvc->auth_method, "OAUTHBEARER")))
     +		curl_easy_setopt(curl, CURLOPT_PASSWORD, srvc->pass);
      
      	strbuf_addstr(&path, srvc->use_ssl ? "imaps://" : "imap://");
 2:  ba9c3fb756 =  3:  d934bdcb82 imap-send: add PLAIN authentication method to OpenSSL
 3:  3d1a66da57 =  4:  f2773c646f imap-send: fix memory leak in case auth_cram_md5 fails
 4:  70bb9388b8 =  5:  c111ee6bc1 imap-send: gracefully fail if CRAM-MD5 authentication is requested without OpenSSL
 5:  0d00a5e135 =  6:  f12713f24b imap-send: enable specifying the folder using the command line
 6:  999c65438f =  7:  d38caeae5e imap-send: fix minor mistakes in the logs
 7:  d0315aebd4 =  8:  3ba02f2b0c imap-send: display port alongwith host when git credential is invoked
 8:  73352a18cf =  9:  6dbd0bf0bc imap-send: display the destination mailbox when sending a message
 9:  36d50d01f0 = 10:  f77f2423e1 imap-send: add ability to list the available folders
-- 
2.49.0.639.gf77f2423e1

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

[Aditya Garg]

OAuth2.0 is a new way of authentication supported by various email providers
these days. OAUTHBEARER and XOAUTH2 are the two most common mechanisms used
for OAuth2.0. OAUTHBEARER is described in RFC5801[1] and RFC7628[2], whereas
XOAUTH2 is Google's proprietary mechanism (See [3]).

[1]: https://datatracker.ietf.org/doc/html/rfc5801
[2]: https://datatracker.ietf.org/doc/html/rfc7628
[3]: https://developers.google.com/workspace/gmail/imap/xoauth2-protocol#initial_client_response

Signed-off-by: Aditya Garg <gargaditya08@live.com>
---
 Documentation/config/imap.adoc   |   5 +-
 Documentation/git-imap-send.adoc |  47 +++++++-
 imap-send.c                      | 182 +++++++++++++++++++++++++++++--
 3 files changed, 221 insertions(+), 13 deletions(-)

diff --git a/Documentation/config/imap.adoc b/Documentation/config/imap.adoc
index 3d28f72643..29b998d5ff 100644
--- a/Documentation/config/imap.adoc
+++ b/Documentation/config/imap.adoc
@@ -40,5 +40,6 @@ imap.authMethod::
 	Specify the authentication method for authenticating with the IMAP server.
 	If Git was built with the NO_CURL option, or if your curl version is older
 	than 7.34.0, or if you're running git-imap-send with the `--no-curl`
-	option, the only supported method is 'CRAM-MD5'. If this is not set
-	then 'git imap-send' uses the basic IMAP plaintext LOGIN command.
+	option, the only supported methods are `CRAM-MD5`, `OAUTHBEARER` and
+	`XOAUTH2`. If this is not set then `git imap-send` uses the basic IMAP
+	plaintext `LOGIN` command.
diff --git a/Documentation/git-imap-send.adoc b/Documentation/git-imap-send.adoc
index 26ccf4e433..8adf0e5aac 100644
--- a/Documentation/git-imap-send.adoc
+++ b/Documentation/git-imap-send.adoc
@@ -102,12 +102,18 @@ Using Gmail's IMAP interface:
 
 ---------
 [imap]
-	folder = "[Gmail]/Drafts"
-	host = imaps://imap.gmail.com
-	user = user@gmail.com
-	port = 993
+    folder = "[Gmail]/Drafts"
+    host = imaps://imap.gmail.com
+    user = user@gmail.com
+    port = 993
 ---------
 
+Gmail does not allow using your regular password for `git imap-send`.
+If you have multi-factor authentication set up on your Gmail account, you
+can generate an app-specific password for use with `git imap-send`.
+Visit https://security.google.com/settings/security/apppasswords to create
+it. Alternatively, use OAuth2.0 authentication as described below.
+
 [NOTE]
 You might need to instead use: `folder = "[Google Mail]/Drafts"` if you get an error
 that the "Folder doesn't exist".
@@ -116,6 +122,35 @@ that the "Folder doesn't exist".
 If your Gmail account is set to another language than English, the name of the "Drafts"
 folder will be localized.
 
+If you want to use OAuth2.0 based authentication, you can specify
+`OAUTHBEARER` or `XOAUTH2` mechanism in your config. It is more secure
+than using app-specific passwords, and also does not enforce the need of
+having multi-factor authentication. You will have to use an OAuth2.0
+access token in place of your password when using this authentication.
+
+---------
+[imap]
+    folder = "[Gmail]/Drafts"
+    host = imaps://imap.gmail.com
+    user = user@gmail.com
+    port = 993
+    authmethod = OAUTHBEARER
+---------
+
+Using Outlook's IMAP interface:
+
+Unlike Gmail, Outlook only supports OAuth2.0 based authentication. Also, it
+supports only `XOAUTH2` as the mechanism.
+
+---------
+[imap]
+    folder = "Drafts"
+    host = imaps://outlook.office365.com
+    user = user@outlook.com
+    port = 993
+    authmethod = XOAUTH2
+---------
+
 Once the commits are ready to be sent, run the following command:
 
   $ git format-patch --cover-letter -M --stdout origin/master | git imap-send
@@ -124,6 +159,10 @@ Just make sure to disable line wrapping in the email client (Gmail's web
 interface will wrap lines no matter what, so you need to use a real
 IMAP client).
 
+In case you are using OAuth2.0 authentication, it is easier to use credential
+helpers to generate tokens. Credential helpers suggested in
+linkgit:git-send-email[1] can be used for `git imap-send` as well.
+
 CAUTION
 -------
 It is still your responsibility to make sure that the email message
diff --git a/imap-send.c b/imap-send.c
index 37f94a37e8..829e957abd 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -139,7 +139,9 @@ enum CAPABILITY {
 	LITERALPLUS,
 	NAMESPACE,
 	STARTTLS,
-	AUTH_CRAM_MD5
+	AUTH_CRAM_MD5,
+	AUTH_OAUTHBEARER,
+	AUTH_XOAUTH2,
 };
 
 static const char *cap_list[] = {
@@ -149,6 +151,8 @@ static const char *cap_list[] = {
 	"NAMESPACE",
 	"STARTTLS",
 	"AUTH=CRAM-MD5",
+	"AUTH=OAUTHBEARER",
+	"AUTH=XOAUTH2",
 };
 
 #define RESP_OK    0
@@ -885,6 +889,108 @@ static char *cram(const char *challenge_64, const char *user, const char *pass)
 	return (char *)response_64;
 }
 
+static char *oauthbearer_base64(const char *user, const char *access_token)
+{
+	int raw_len, b64_len;
+	char *raw, *b64;
+
+	/*
+	 * Compose the OAUTHBEARER string
+	 *
+	 * "n,a=" {User} ",^Ahost=" {Host} "^Aport=" {Port} "^Aauth=Bearer " {Access Token} "^A^A
+	 *
+	 * The first part `n,a=" {User} ",` is the gs2 header described in RFC5801.
+	 * * gs2-cb-flag `n` -> client does not support CB
+	 * * gs2-authzid `a=" {User} "`
+	 *
+	 * The second part are key value pairs containing host, port and auth as
+	 * described in RFC7628.
+	 *
+	 * https://datatracker.ietf.org/doc/html/rfc5801
+	 * https://datatracker.ietf.org/doc/html/rfc7628
+	 */
+	raw_len = strlen(user) + strlen(access_token) + 20;
+	raw = xmallocz(raw_len + 1);
+	snprintf(raw, raw_len + 1, "n,a=%s,\001auth=Bearer %s\001\001", user, access_token);
+
+	/* Base64 encode */
+	b64 = xmallocz(ENCODED_SIZE(strlen(raw)));
+	b64_len = EVP_EncodeBlock((unsigned char *)b64, (unsigned char *)raw, strlen(raw));
+	free(raw);
+
+	if (b64_len < 0) {
+		free(b64);
+		return NULL;
+	}
+	return b64;
+}
+
+static char *xoauth2_base64(const char *user, const char *access_token)
+{
+	int raw_len, b64_len;
+	char *raw, *b64;
+
+	/*
+	 * Compose the XOAUTH2 string
+	 * "user=" {User} "^Aauth=Bearer " {Access Token} "^A^A"
+	 * https://developers.google.com/workspace/gmail/imap/xoauth2-protocol#initial_client_response
+	 */
+	raw_len = strlen(user) + strlen(access_token) + 20;
+	raw = xmallocz(raw_len + 1);
+	snprintf(raw, raw_len + 1, "user=%s\001auth=Bearer %s\001\001", user, access_token);
+
+	/* Base64 encode */
+	b64 = xmallocz(ENCODED_SIZE(strlen(raw)));
+	b64_len = EVP_EncodeBlock((unsigned char *)b64, (unsigned char *)raw, strlen(raw));
+	free(raw);
+
+	if (b64_len < 0) {
+		free(b64);
+		return NULL;
+	}
+	return b64;
+}
+
+static int auth_oauthbearer(struct imap_store *ctx, const char *prompt UNUSED)
+{
+	int ret;
+	char *b64;
+
+	b64 = oauthbearer_base64(ctx->cfg->user, ctx->cfg->pass);
+	if (!b64)
+		return error("OAUTHBEARER: base64 encoding failed");
+
+	/* Send the base64-encoded response */
+	ret = socket_write(&ctx->imap->buf.sock, b64, strlen(b64));
+	if (ret != (int)strlen(b64)) {
+		free(b64);
+		return error("IMAP error: sending OAUTHBEARER response failed");
+	}
+
+	free(b64);
+	return 0;
+}
+
+static int auth_xoauth2(struct imap_store *ctx, const char *prompt UNUSED)
+{
+	int ret;
+	char *b64;
+
+	b64 = xoauth2_base64(ctx->cfg->user, ctx->cfg->pass);
+	if (!b64)
+		return error("XOAUTH2: base64 encoding failed");
+
+	/* Send the base64-encoded response */
+	ret = socket_write(&ctx->imap->buf.sock, b64, strlen(b64));
+	if (ret != (int)strlen(b64)) {
+		free(b64);
+		return error("IMAP error: sending XOAUTH2 response failed");
+	}
+
+	free(b64);
+	return 0;
+}
+
 #else
 
 static char *cram(const char *challenge_64 UNUSED,
@@ -895,6 +1001,9 @@ static char *cram(const char *challenge_64 UNUSED,
 	    "you have to build git-imap-send with OpenSSL library.");
 }
 
+#define auth_oauthbearer NULL
+#define auth_xoauth2 NULL
+
 #endif
 
 static int auth_cram_md5(struct imap_store *ctx, const char *prompt)
@@ -1104,6 +1213,50 @@ static struct imap_store *imap_open_store(struct imap_server_conf *srvc, const c
 					fprintf(stderr, "IMAP error: AUTHENTICATE CRAM-MD5 failed\n");
 					goto bail;
 				}
+			} else if (!strcmp(srvc->auth_method, "OAUTHBEARER")) {
+				if (!CAP(AUTH_OAUTHBEARER)) {
+					fprintf(stderr, "You specified "
+						"OAUTHBEARER as authentication method, "
+						"but %s doesn't support it.\n", srvc->host);
+					goto bail;
+				}
+
+				#ifdef NO_OPENSSL
+				fprintf(stderr, "You are trying to use OAUTHBEARER authentication mechanism "
+					"with OpenSSL library, but its support has not been compiled in.");
+				goto bail;
+				#endif
+
+				/* OAUTHBEARER */
+
+				memset(&cb, 0, sizeof(cb));
+				cb.cont = auth_oauthbearer;
+				if (imap_exec(ctx, &cb, "AUTHENTICATE OAUTHBEARER") != RESP_OK) {
+					fprintf(stderr, "IMAP error: AUTHENTICATE OAUTHBEARER failed\n");
+					goto bail;
+				}
+			} else if (!strcmp(srvc->auth_method, "XOAUTH2")) {
+				if (!CAP(AUTH_XOAUTH2)) {
+					fprintf(stderr, "You specified "
+						"XOAUTH2 as authentication method, "
+						"but %s doesn't support it.\n", srvc->host);
+					goto bail;
+				}
+
+				#ifdef NO_OPENSSL
+				fprintf(stderr, "You are trying to use XOAUTH2 authentication mechanism "
+					"with OpenSSL library, but its support has not been compiled in.");
+				goto bail;
+				#endif
+
+				/* XOAUTH2 */
+
+				memset(&cb, 0, sizeof(cb));
+				cb.cont = auth_xoauth2;
+				if (imap_exec(ctx, &cb, "AUTHENTICATE XOAUTH2") != RESP_OK) {
+					fprintf(stderr, "IMAP error: AUTHENTICATE XOAUTH2 failed\n");
+					goto bail;
+				}
 			} else {
 				fprintf(stderr, "Unknown authentication method:%s\n", srvc->host);
 				goto bail;
@@ -1405,7 +1558,11 @@ static CURL *setup_curl(struct imap_server_conf *srvc, struct credential *cred)
 
 	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);
 
 	strbuf_addstr(&path, srvc->use_ssl ? "imaps://" : "imap://");
 	strbuf_addstr(&path, srvc->host);
@@ -1423,11 +1580,22 @@ static CURL *setup_curl(struct imap_server_conf *srvc, struct credential *cred)
 	curl_easy_setopt(curl, CURLOPT_PORT, srvc->port);
 
 	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);
+		if (!strcmp(srvc->auth_method, "XOAUTH2") ||
+		    !strcmp(srvc->auth_method, "OAUTHBEARER")) {
+
+			/*
+			 * While CURLOPT_XOAUTH2_BEARER looks as if it only supports XOAUTH2,
+			 * upon debugging, it has been found that it is capable of detecting
+			 * the best option out of OAUTHBEARER and XOAUTH2.
+			 */
+			curl_easy_setopt(curl, CURLOPT_XOAUTH2_BEARER, srvc->pass);
+		} else {
+			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);
+		}
 	}
 
 	if (!srvc->use_ssl)
-- 
2.49.0.639.gf77f2423e1

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

[Junio C Hamano]

Aditya Garg <gargaditya08@live.com> writes:

> +			} else if (!strcmp(srvc->auth_method, "OAUTHBEARER")) {
> +				if (!CAP(AUTH_OAUTHBEARER)) {
> +					fprintf(stderr, "You specified "
> +						"OAUTHBEARER as authentication method, "
> +						"but %s doesn't support it.\n", srvc->host);
> +					goto bail;
> +				}
> +
> +				#ifdef NO_OPENSSL
> +				fprintf(stderr, "You are trying to use OAUTHBEARER authentication mechanism "
> +					"with OpenSSL library, but its support has not been compiled in.");
> +				goto bail;
> +				#endif

Ugly.  Can we avoid #ifdef/#endif in the middle of such a main flow
of the logic?  Hiding such ugliness by indenting the #ifdef/#endif
directives as if they are just one of the code lines is doubly ugly.

>  	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?

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

[Aditya Garg]

On 5 June 2025 10:03:51 pm IST, Junio C Hamano <gitster@pobox.com> wrote:
>Aditya Garg <gargaditya08@live.com> writes:
>
>> +			} else if (!strcmp(srvc->auth_method, "OAUTHBEARER")) {
>> +				if (!CAP(AUTH_OAUTHBEARER)) {
>> +					fprintf(stderr, "You specified "
>> +						"OAUTHBEARER as authentication method, "
>> +						"but %s doesn't support it.\n", srvc->host);
>> +					goto bail;
>> +				}
>> +
>> +				#ifdef NO_OPENSSL
>> +				fprintf(stderr, "You are trying to use OAUTHBEARER authentication mechanism "
>> +					"with OpenSSL library, but its support has not been compiled in.");
>> +				goto bail;
>> +				#endif
>
>Ugly.  Can we avoid #ifdef/#endif in the middle of such a main flow
>of the logic?  Hiding such ugliness by indenting the #ifdef/#endif
>directives as if they are just one of the code lines is doubly ugly.
>

RESENDING AS PLAIN TEXT


Your suggestion in a previous review said:

           if (!auth_oauthbearer) {
               ... we do not support ...
               goto bail;
           }

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 :(.

>>  	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. OAUTHBEARER and XOAUTH2 use CURLOPT_XOAUTH2_BEARER
in curl, which can use either of them based on what server says. Other auth methods
are not supported yet in this code, and this is the reason CRAM_MD5 is supported
by only OpenSSL.

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

[Junio C Hamano]

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 ...
	}


>>>  	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?

> 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");

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.

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

[Aditya Garg]

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
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.

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

[Aditya Garg]

> 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

SMTP was a typo. It's IMAP.

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

[Junio C Hamano]

Aditya Garg <gargaditya08@live.com> writes:

>>Which makes it sound like if (!srvc->auth_method) is enough?
>>
>
> No. If the user specifies PLAIN or LOGIN then it's not enough.

That invites another question.  Why aren't we checking for PLAIN or
LOGIN and when one of them is given use the password?  What is
written in the patch looks backwards, in that we seem to assume that
a method that is not XOAUTH2 and OAUTHBEARER must be PLAIN or LOGIN
(or anything that wants us to pass CURLOPT_PASSWORD).  IOW, why
isn't the code more like

	if (/* not using the more advanced method interface? */
	    !srvc->auth_method ||
	    /* method interface that takes password? */
	    !strcmp(srvc->auth_method, "PLAIN")	||
	    !strcmp(srvc->auth_method, "LOGIN"))
		curl_easy_setopt(curl, CURLOPT_PASSWORD, srvc->pass);

if, after all, PLAIN/LOGIN are what triggers password
authentication?

> 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.

Don't be short ;-) Be long in your log message to help future
developers.  In short, you want to make your proposed log message so
clear that future developers who found this commit in "git log -p"
to come asking you these questions---that is why reviewers are
supposed to ask questions and ask for clarifications.

> "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"

That is a very good thing to write down either in-code comment
and/or the log message to avoid future developers come bugging you
with the same questions as I did.

Thanks.

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

[Aditya Garg]

> On 6 Jun 2025, at 10:05 AM, Junio C Hamano <gitster@pobox.com> wrote:
> 
> Aditya Garg <gargaditya08@live.com> writes:
> 
>>> Which makes it sound like if (!srvc->auth_method) is enough?
>>> 
>> 
>> No. If the user specifies PLAIN or LOGIN then it's not enough.
> 
> That invites another question.  Why aren't we checking for PLAIN or
> LOGIN and when one of them is given use the password?  What is
> written in the patch looks backwards, in that we seem to assume that
> a method that is not XOAUTH2 and OAUTHBEARER must be PLAIN or LOGIN
> (or anything that wants us to pass CURLOPT_PASSWORD).  IOW, why
> isn't the code more like
> 
>    if (/* not using the more advanced method interface? */
>        !srvc->auth_method ||
>        /* method interface that takes password? */
>        !strcmp(srvc->auth_method, "PLAIN")    ||
>        !strcmp(srvc->auth_method, "LOGIN"))
>        curl_easy_setopt(curl, CURLOPT_PASSWORD, srvc->pass);
> 
> if, after all, PLAIN/LOGIN are what triggers password
> authentication?
> 
>> 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.
> 
> Don't be short ;-) Be long in your log message to help future
> developers.  In short, you want to make your proposed log message so
> clear that future developers who found this commit in "git log -p"
> to come asking you these questions---that is why reviewers are
> supposed to ask questions and ask for clarifications.

Ok :). So, you want me to add checks for PLAIN and LOGIN, or the current
logic is fine. I'd prefer using the current logic to avoid potential regressions,
but its your call.
> 
>> "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"
> 
> That is a very good thing to write down either in-code comment
> and/or the log message to avoid future developers come bugging you
> with the same questions as I did.

Alright, I'll add it as a comment in the code.