* [PATCH v2 1/6] imap-send: note "auth_method", not "host" on auth method failure
2023-02-02 9:44 ` [PATCH v2 0/6] " Ævar Arnfjörð Bjarmason
@ 2023-02-02 9:44 ` Ævar Arnfjörð Bjarmason
2023-02-02 19:11 ` Junio C Hamano
2023-02-02 9:44 ` [PATCH v2 2/6] imap-send doc: the imap.sslVerify is used with imap.tunnel Ævar Arnfjörð Bjarmason
` (4 subsequent siblings)
5 siblings, 1 reply; 37+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2023-02-02 9:44 UTC (permalink / raw)
To: git
Cc: Jiang Xin, Junio C Hamano, Bernhard Reiter, Remi Pommarel,
Jeff King, Ævar Arnfjörð Bjarmason
Fix error reporting added in ae9c606ed22 (imap-send: support CRAM-MD5
authentication, 2010-02-15), the use of "srvc->host" here was
seemingly copy/pasted from other uses added in the same commit.
But here we're complaining about the "auth_method" being incorrect, so
let's note it, and not the hostname.
In a subsequent commit we'll alter other uses of "host" here after
getting rid of the non-tunnel OpenSSL codepath. This preparatory fix
makes that subsequent change smaller.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
imap-send.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/imap-send.c b/imap-send.c
index a50af56b827..b7902babd4c 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -1121,7 +1121,7 @@ static struct imap_store *imap_open_store(struct imap_server_conf *srvc, const c
goto bail;
}
} else {
- fprintf(stderr, "Unknown authentication method:%s\n", srvc->host);
+ fprintf(stderr, "Unknown authentication method:%s\n", srvc->auth_method);
goto bail;
}
} else {
--
2.39.1.1392.g63e6d408230
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH v2 2/6] imap-send doc: the imap.sslVerify is used with imap.tunnel
2023-02-02 9:44 ` [PATCH v2 0/6] " Ævar Arnfjörð Bjarmason
2023-02-02 9:44 ` [PATCH v2 1/6] imap-send: note "auth_method", not "host" on auth method failure Ævar Arnfjörð Bjarmason
@ 2023-02-02 9:44 ` Ævar Arnfjörð Bjarmason
2023-02-02 9:44 ` [PATCH v2 3/6] imap-send: replace auto-probe libcurl with hard dependency Ævar Arnfjörð Bjarmason
` (3 subsequent siblings)
5 siblings, 0 replies; 37+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2023-02-02 9:44 UTC (permalink / raw)
To: git
Cc: Jiang Xin, Junio C Hamano, Bernhard Reiter, Remi Pommarel,
Jeff King, Ævar Arnfjörð Bjarmason
This documentation added in [1] claims that imap.{host,port,sslVerify}
is ignored if imap.tunnel is set. That's correct in the first two
cases, but not for imap.sslVerify.
When we're using the tunnel feature we'll call ssl_socket_connect()
with a 3rd "verify" argument set to the value of the "imap.sslVerify"
config if we're on the !preauth path. There is also a call to
ssl_socket_connect() that's specific to the non-tunnel
codepath.
Perhaps the documentation added in [1] was written for an earlier
version of [2] (which was introduced in the same series). There is an
earlier version of the patch on-list[3] where there's still a "FIXME"
comment indicating that we should read the config in the future before
setting "SSL_VERIFY_PEER", which is what we'll do if "imap.sslVerify"
is set.
1. c82b0748e53 (Documentation: Improve documentation for
git-imap-send(1), 2008-07-09)
2. 684ec6c63cd (git-imap-send: Support SSL, 2008-07-09)
3. https://lore.kernel.org/git/1096648c0806010829n71de92dcmc19ddb87da19931d@mail.gmail.com/
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
Documentation/config/imap.txt | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/Documentation/config/imap.txt b/Documentation/config/imap.txt
index 06166fb5c04..96b1c0927d8 100644
--- a/Documentation/config/imap.txt
+++ b/Documentation/config/imap.txt
@@ -26,8 +26,7 @@ imap.port::
imap.sslverify::
A boolean to enable/disable verification of the server certificate
- used by the SSL/TLS connection. Default is `true`. Ignored when
- imap.tunnel is set.
+ used by the SSL/TLS connection.
imap.preformattedHTML::
A boolean to enable/disable the use of html encoding when sending
--
2.39.1.1392.g63e6d408230
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH v2 3/6] imap-send: replace auto-probe libcurl with hard dependency
2023-02-02 9:44 ` [PATCH v2 0/6] " Ævar Arnfjörð Bjarmason
2023-02-02 9:44 ` [PATCH v2 1/6] imap-send: note "auth_method", not "host" on auth method failure Ævar Arnfjörð Bjarmason
2023-02-02 9:44 ` [PATCH v2 2/6] imap-send doc: the imap.sslVerify is used with imap.tunnel Ævar Arnfjörð Bjarmason
@ 2023-02-02 9:44 ` Ævar Arnfjörð Bjarmason
2023-02-02 19:33 ` Junio C Hamano
2023-02-02 9:44 ` [PATCH v2 4/6] imap-send: make --curl no-optional Ævar Arnfjörð Bjarmason
` (2 subsequent siblings)
5 siblings, 1 reply; 37+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2023-02-02 9:44 UTC (permalink / raw)
To: git
Cc: Jiang Xin, Junio C Hamano, Bernhard Reiter, Remi Pommarel,
Jeff King, Ævar Arnfjörð Bjarmason
Change the "imap-send" command to have a hard dependency on libcurl,
before this it had an optional dependency on both libcurl and OpenSSL,
now only the OpenSSL dependency is optional.
This simplifies our dependency matrix by getting rid of yet another
special-case. Given the prevalence of libcurl and portability of
libcurl it seems reasonable to say that "git imap-send" cannot be used
without libcurl, almost everyone building git needs to be able to push
or pull over http(s), so they'll be building with libcurl already.
So let's remove the previous "USE_CURL_FOR_IMAP_SEND" knob. Whether we
build git-imap-send or not is now controlled by the "NO_CURL"
knob.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
Documentation/config/imap.txt | 3 +--
Documentation/git-imap-send.txt | 3 +--
INSTALL | 8 ++++----
Makefile | 18 +++++-------------
imap-send.c | 23 ++---------------------
5 files changed, 13 insertions(+), 42 deletions(-)
diff --git a/Documentation/config/imap.txt b/Documentation/config/imap.txt
index 96b1c0927d8..7f30080c409 100644
--- a/Documentation/config/imap.txt
+++ b/Documentation/config/imap.txt
@@ -37,7 +37,6 @@ imap.preformattedHTML::
imap.authMethod::
Specify authenticate method for authentication with 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`
+ 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.
diff --git a/Documentation/git-imap-send.txt b/Documentation/git-imap-send.txt
index f7b18515141..202e3e59094 100644
--- a/Documentation/git-imap-send.txt
+++ b/Documentation/git-imap-send.txt
@@ -44,8 +44,7 @@ OPTIONS
--no-curl::
Talk to the IMAP server using git's own IMAP routines instead of
- using libcurl. Ignored if Git was built with the NO_OPENSSL option
- set.
+ using libcurl.
CONFIGURATION
diff --git a/INSTALL b/INSTALL
index d5694f8c470..d9538bbcb45 100644
--- a/INSTALL
+++ b/INSTALL
@@ -129,13 +129,13 @@ Issues of note:
itself, e.g. Digest::MD5, File::Spec, File::Temp, Net::Domain,
Net::SMTP, and Time::HiRes.
- - git-imap-send needs the OpenSSL library to talk IMAP over SSL if
- you are using libcurl older than 7.34.0. Otherwise you can use
- NO_OPENSSL without losing git-imap-send.
+ - git-imap-send needs libcurl 7.34.0 or newer, in addition
+ OpenSSL is needed if using the "imap.tunnel" open to tunnel
+ over SSL. Define NO_OPENSSL to omit the OpenSSL prerequisite.
- "libcurl" library is used for fetching and pushing
repositories over http:// or https://, as well as by
- git-imap-send if the curl version is >= 7.34.0. If you do
+ git-imap-send. If you do
not need that functionality, use NO_CURL to build without
it.
diff --git a/Makefile b/Makefile
index 45bd6ac9c3e..b08a855198c 100644
--- a/Makefile
+++ b/Makefile
@@ -773,7 +773,9 @@ PROGRAMS += $(EXTRA_PROGRAMS)
PROGRAM_OBJS += daemon.o
PROGRAM_OBJS += http-backend.o
+ifndef NO_CURL
PROGRAM_OBJS += imap-send.o
+endif
PROGRAM_OBJS += sh-i18n--envsubst.o
PROGRAM_OBJS += shell.o
.PHONY: program-objs
@@ -1583,7 +1585,6 @@ ifdef HAVE_ALLOCA_H
BASIC_CFLAGS += -DHAVE_ALLOCA_H
endif
-IMAP_SEND_BUILDDEPS =
IMAP_SEND_LDFLAGS =
ifdef NO_CURL
@@ -1592,6 +1593,7 @@ ifdef NO_CURL
REMOTE_CURL_ALIASES =
REMOTE_CURL_NAMES =
EXCLUDED_PROGRAMS += git-http-fetch git-http-push
+ EXCLUDED_PROGRAMS += git-imap-send
else
ifdef CURLDIR
# Try "-Wl,-rpath=$(CURLDIR)/$(lib)" in such a case.
@@ -1617,19 +1619,9 @@ else
REMOTE_CURL_NAMES = $(REMOTE_CURL_PRIMARY) $(REMOTE_CURL_ALIASES)
PROGRAM_OBJS += http-fetch.o
PROGRAMS += $(REMOTE_CURL_NAMES)
+ IMAP_SEND_LDFLAGS += $(CURL_LIBCURL)
ifndef NO_EXPAT
PROGRAM_OBJS += http-push.o
- endif
- curl_check := $(shell (echo 072200; $(CURL_CONFIG) --vernum | sed -e '/^70[BC]/s/^/0/') 2>/dev/null | sort -r | sed -ne 2p)
- ifeq "$(curl_check)" "072200"
- USE_CURL_FOR_IMAP_SEND = YesPlease
- endif
- ifdef USE_CURL_FOR_IMAP_SEND
- BASIC_CFLAGS += -DUSE_CURL_FOR_IMAP_SEND
- IMAP_SEND_BUILDDEPS = http.o
- IMAP_SEND_LDFLAGS += $(CURL_LIBCURL)
- endif
- ifndef NO_EXPAT
ifdef EXPATDIR
BASIC_CFLAGS += -I$(EXPATDIR)/include
EXPAT_LIBEXPAT = -L$(EXPATDIR)/$(lib) $(CC_LD_DYNPATH)$(EXPATDIR)/$(lib) -lexpat
@@ -2786,7 +2778,7 @@ endif
git-%$X: %.o GIT-LDFLAGS $(GITLIBS)
$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(LIBS)
-git-imap-send$X: imap-send.o $(IMAP_SEND_BUILDDEPS) GIT-LDFLAGS $(GITLIBS)
+git-imap-send$X: imap-send.o http.o GIT-LDFLAGS $(GITLIBS)
$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
$(IMAP_SEND_LDFLAGS) $(LIBS)
diff --git a/imap-send.c b/imap-send.c
index b7902babd4c..26f8f01e97a 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -30,20 +30,10 @@
#if defined(NO_OPENSSL) && !defined(HAVE_OPENSSL_CSPRNG)
typedef void *SSL;
#endif
-#ifdef USE_CURL_FOR_IMAP_SEND
#include "http.h"
-#endif
-
-#if defined(USE_CURL_FOR_IMAP_SEND)
-/* Always default to curl if it's available. */
-#define USE_CURL_DEFAULT 1
-#else
-/* We don't have curl, so continue to use the historical implementation */
-#define USE_CURL_DEFAULT 0
-#endif
static int verbosity;
-static int use_curl = USE_CURL_DEFAULT;
+static int use_curl = 1;
static const char * const imap_send_usage[] = { "git imap-send [-v] [-q] [--[no-]curl] < <mbox>", NULL };
@@ -1396,7 +1386,6 @@ static int append_msgs_to_imap(struct imap_server_conf *server,
return 0;
}
-#ifdef USE_CURL_FOR_IMAP_SEND
static CURL *setup_curl(struct imap_server_conf *srvc, struct credential *cred)
{
CURL *curl;
@@ -1515,7 +1504,6 @@ static int curl_append_msgs_to_imap(struct imap_server_conf *server,
return res != CURLE_OK;
}
-#endif
int cmd_main(int argc, const char **argv)
{
@@ -1531,12 +1519,7 @@ int cmd_main(int argc, const char **argv)
if (argc)
usage_with_options(imap_send_usage, imap_send_options);
-#ifndef USE_CURL_FOR_IMAP_SEND
- if (use_curl) {
- warning("--curl not supported in this build");
- use_curl = 0;
- }
-#elif defined(NO_OPENSSL)
+#if defined(NO_OPENSSL)
if (!use_curl) {
warning("--no-curl not supported in this build");
use_curl = 1;
@@ -1580,10 +1563,8 @@ int cmd_main(int argc, const char **argv)
if (server.tunnel)
return append_msgs_to_imap(&server, &all_msgs, total);
-#ifdef USE_CURL_FOR_IMAP_SEND
if (use_curl)
return curl_append_msgs_to_imap(&server, &all_msgs, total);
-#endif
return append_msgs_to_imap(&server, &all_msgs, total);
}
--
2.39.1.1392.g63e6d408230
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH v2 3/6] imap-send: replace auto-probe libcurl with hard dependency
2023-02-02 9:44 ` [PATCH v2 3/6] imap-send: replace auto-probe libcurl with hard dependency Ævar Arnfjörð Bjarmason
@ 2023-02-02 19:33 ` Junio C Hamano
0 siblings, 0 replies; 37+ messages in thread
From: Junio C Hamano @ 2023-02-02 19:33 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Jiang Xin, Bernhard Reiter, Remi Pommarel, Jeff King
> imap.authMethod::
> Specify authenticate method for authentication with 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`
> + 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.
OK.
> diff --git a/Documentation/git-imap-send.txt b/Documentation/git-imap-send.txt
> index f7b18515141..202e3e59094 100644
> --- a/Documentation/git-imap-send.txt
> +++ b/Documentation/git-imap-send.txt
> @@ -44,8 +44,7 @@ OPTIONS
>
> --no-curl::
> Talk to the IMAP server using git's own IMAP routines instead of
> - using libcurl. Ignored if Git was built with the NO_OPENSSL option
> - set.
> + using libcurl.
Hmph, let's read on to resolve "when built with NO_OPENSSL, giving
--no-curl now errors out or do something else? do we need to? why?",
which was my knee-jerk reaction.
> diff --git a/INSTALL b/INSTALL
> index d5694f8c470..d9538bbcb45 100644
> --- a/INSTALL
> +++ b/INSTALL
> @@ -129,13 +129,13 @@ Issues of note:
> itself, e.g. Digest::MD5, File::Spec, File::Temp, Net::Domain,
> Net::SMTP, and Time::HiRes.
>
> - - git-imap-send needs the OpenSSL library to talk IMAP over SSL if
> - you are using libcurl older than 7.34.0. Otherwise you can use
> - NO_OPENSSL without losing git-imap-send.
> + - git-imap-send needs libcurl 7.34.0 or newer, in addition
> + OpenSSL is needed if using the "imap.tunnel" open to tunnel
> + over SSL. Define NO_OPENSSL to omit the OpenSSL prerequisite.
"if using the imap.tunnel to open a tunnel over ssl"?
because I think there are some grammo there.
"to omit the OpenSSL prerequisite" -> "if you do not need it"?
because the original sounds like losing prereq without any penalty.
> - "libcurl" library is used for fetching and pushing
> repositories over http:// or https://, as well as by
> - git-imap-send if the curl version is >= 7.34.0. If you do
> + git-imap-send. If you do
> not need that functionality, use NO_CURL to build without
> it.
OK.
> diff --git a/Makefile b/Makefile
> index 45bd6ac9c3e..b08a855198c 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -773,7 +773,9 @@ PROGRAMS += $(EXTRA_PROGRAMS)
>
> PROGRAM_OBJS += daemon.o
> PROGRAM_OBJS += http-backend.o
> +ifndef NO_CURL
> PROGRAM_OBJS += imap-send.o
> +endif
Nice.
> @@ -1592,6 +1593,7 @@ ifdef NO_CURL
> REMOTE_CURL_ALIASES =
> REMOTE_CURL_NAMES =
> EXCLUDED_PROGRAMS += git-http-fetch git-http-push
> + EXCLUDED_PROGRAMS += git-imap-send
OK.
> @@ -1617,19 +1619,9 @@ else
> REMOTE_CURL_NAMES = $(REMOTE_CURL_PRIMARY) $(REMOTE_CURL_ALIASES)
> PROGRAM_OBJS += http-fetch.o
> PROGRAMS += $(REMOTE_CURL_NAMES)
> + IMAP_SEND_LDFLAGS += $(CURL_LIBCURL)
OK. That is a natural consequence of losing USE_CURL_FOR_IMAP_SEND
conditional, which is good.
> @@ -2786,7 +2778,7 @@ endif
> git-%$X: %.o GIT-LDFLAGS $(GITLIBS)
> $(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) $(LIBS)
>
> -git-imap-send$X: imap-send.o $(IMAP_SEND_BUILDDEPS) GIT-LDFLAGS $(GITLIBS)
> +git-imap-send$X: imap-send.o http.o GIT-LDFLAGS $(GITLIBS)
And this too is a natural consequence of http.o that serves as a
linkage between us and libcURL being always used. Good.
> diff --git a/imap-send.c b/imap-send.c
> index b7902babd4c..26f8f01e97a 100644
> --- a/imap-send.c
> +++ b/imap-send.c
> @@ -30,20 +30,10 @@
> ...
> +static int use_curl = 1;
OK.
> int cmd_main(int argc, const char **argv)
> {
> @@ -1531,12 +1519,7 @@ int cmd_main(int argc, const char **argv)
> if (argc)
> usage_with_options(imap_send_usage, imap_send_options);
>
> -#ifndef USE_CURL_FOR_IMAP_SEND
> - if (use_curl) {
> - warning("--curl not supported in this build");
> - use_curl = 0;
> - }
Naturally ;-)
> -#elif defined(NO_OPENSSL)
> +#if defined(NO_OPENSSL)
> if (!use_curl) {
> warning("--no-curl not supported in this build");
> use_curl = 1;
In the original, this part reached iff we had USE_CURL_FOR_IMAP_SEND,
so "if we are using curl and do not have openssl, then --no-curl is
rejected and we forced use of curl" was how the original behaved here.
Here, we always link with curl, so the updated code is doing exactly
the same thing. Good.
But then the documentation change above that puzzled me was there
not because it was needed to match updated behaviour (the behaviour
stayed the same). So was it meant as a documentation improvement?
> @@ -1580,10 +1563,8 @@ int cmd_main(int argc, const char **argv)
> if (server.tunnel)
> return append_msgs_to_imap(&server, &all_msgs, total);
>
> -#ifdef USE_CURL_FOR_IMAP_SEND
> if (use_curl)
> return curl_append_msgs_to_imap(&server, &all_msgs, total);
> -#endif
Naturally.
> return append_msgs_to_imap(&server, &all_msgs, total);
> }
Looking good. Thanks.
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH v2 4/6] imap-send: make --curl no-optional
2023-02-02 9:44 ` [PATCH v2 0/6] " Ævar Arnfjörð Bjarmason
` (2 preceding siblings ...)
2023-02-02 9:44 ` [PATCH v2 3/6] imap-send: replace auto-probe libcurl with hard dependency Ævar Arnfjörð Bjarmason
@ 2023-02-02 9:44 ` Ævar Arnfjörð Bjarmason
2023-02-02 20:42 ` Junio C Hamano
2023-02-02 9:44 ` [PATCH v2 5/6] imap-send: remove old --no-curl codepath Ævar Arnfjörð Bjarmason
2023-02-02 9:44 ` [PATCH v2 6/6] imap-send: correctly report "host" when using "tunnel" Ævar Arnfjörð Bjarmason
5 siblings, 1 reply; 37+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2023-02-02 9:44 UTC (permalink / raw)
To: git
Cc: Jiang Xin, Junio C Hamano, Bernhard Reiter, Remi Pommarel,
Jeff King, Ævar Arnfjörð Bjarmason
In the preceding commit the old "USE_CURL_FOR_IMAP_SEND" define became
always true, as we now require libcurl for git-imap-send.
But as we require OpenSSL for the "tunnel" mode we still need to keep
the OpenSSL codepath around (ee [1] for an attempt to remove it). But
we don't need to keep supporting "--no-curl" to bypass the curl
codepath for the non-tunnel mode.
As almost all users of "git" use a version of it built with libcurl
we're making what's already the preferred & default codepath
mandatory.
The "imap.authMethod" documentation being changed here has always been
incomplete. It only mentioned "--no-curl", but omitted mentioning that
the same applied for "imap.tunnel". Let's fix it as we're amending it
to be correct, now (as before) with "imap.tunnel" only
"imap.authMethod=CRAM-MD5" is supported.
1. https://lore.kernel.org/git/ab866314-608b-eaca-b335-12cffe165526@morey-chaisemartin.com/
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
Documentation/config/imap.txt | 4 ++--
Documentation/git-imap-send.txt | 10 ----------
imap-send.c | 15 ++++-----------
3 files changed, 6 insertions(+), 23 deletions(-)
diff --git a/Documentation/config/imap.txt b/Documentation/config/imap.txt
index 7f30080c409..5cc46d87216 100644
--- a/Documentation/config/imap.txt
+++ b/Documentation/config/imap.txt
@@ -37,6 +37,6 @@ imap.preformattedHTML::
imap.authMethod::
Specify authenticate method for authentication with IMAP server.
- If you're running git-imap-send with the `--no-curl`
- option, the only supported method is 'CRAM-MD5'. If this is not set
+ If you're using imap.tunnel, the only supported method is 'CRAM-MD5'.
+ If this is not set
then 'git imap-send' uses the basic IMAP plaintext LOGIN command.
diff --git a/Documentation/git-imap-send.txt b/Documentation/git-imap-send.txt
index 202e3e59094..ddbbe819315 100644
--- a/Documentation/git-imap-send.txt
+++ b/Documentation/git-imap-send.txt
@@ -37,16 +37,6 @@ OPTIONS
--quiet::
Be quiet.
---curl::
- Use libcurl to communicate with the IMAP server, unless tunneling
- into it. Ignored if Git was built without the USE_CURL_FOR_IMAP_SEND
- option set.
-
---no-curl::
- Talk to the IMAP server using git's own IMAP routines instead of
- using libcurl.
-
-
CONFIGURATION
-------------
diff --git a/imap-send.c b/imap-send.c
index 26f8f01e97a..9d7cb22285d 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -39,7 +39,7 @@ static const char * const imap_send_usage[] = { "git imap-send [-v] [-q] [--[no-
static struct option imap_send_options[] = {
OPT__VERBOSITY(&verbosity),
- OPT_BOOL(0, "curl", &use_curl, "use libcurl to communicate with the IMAP server"),
+ OPT_HIDDEN_BOOL(0, "curl", &use_curl, "use libcurl to communicate with the IMAP server"),
OPT_END()
};
@@ -1519,12 +1519,8 @@ int cmd_main(int argc, const char **argv)
if (argc)
usage_with_options(imap_send_usage, imap_send_options);
-#if defined(NO_OPENSSL)
- if (!use_curl) {
- warning("--no-curl not supported in this build");
- use_curl = 1;
- }
-#endif
+ if (!use_curl)
+ die(_("the --no-curl option to imap-send has been deprecated"));
if (!server.port)
server.port = server.use_ssl ? 993 : 143;
@@ -1560,10 +1556,7 @@ int cmd_main(int argc, const char **argv)
/* write it to the imap server */
- if (server.tunnel)
- return append_msgs_to_imap(&server, &all_msgs, total);
-
- if (use_curl)
+ if (!server.tunnel)
return curl_append_msgs_to_imap(&server, &all_msgs, total);
return append_msgs_to_imap(&server, &all_msgs, total);
--
2.39.1.1392.g63e6d408230
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH v2 4/6] imap-send: make --curl no-optional
2023-02-02 9:44 ` [PATCH v2 4/6] imap-send: make --curl no-optional Ævar Arnfjörð Bjarmason
@ 2023-02-02 20:42 ` Junio C Hamano
2023-02-03 21:46 ` Ævar Arnfjörð Bjarmason
0 siblings, 1 reply; 37+ messages in thread
From: Junio C Hamano @ 2023-02-02 20:42 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Jiang Xin, Bernhard Reiter, Remi Pommarel, Jeff King
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> In the preceding commit the old "USE_CURL_FOR_IMAP_SEND" define became
> always true, as we now require libcurl for git-imap-send.
>
> But as we require OpenSSL for the "tunnel" mode we still need to keep
> the OpenSSL codepath around (ee [1] for an attempt to remove it). But
"(ee" -> ???
> we don't need to keep supporting "--no-curl" to bypass the curl
> codepath for the non-tunnel mode.
We do not need to because...?
> @@ -1519,12 +1519,8 @@ int cmd_main(int argc, const char **argv)
> if (argc)
> usage_with_options(imap_send_usage, imap_send_options);
>
> -#if defined(NO_OPENSSL)
> - if (!use_curl) {
> - warning("--no-curl not supported in this build");
> - use_curl = 1;
> - }
> -#endif
> + if (!use_curl)
> + die(_("the --no-curl option to imap-send has been deprecated"));
We used to force use of cURL when there is no other way to make the
program work (i.e. there is no direct OpenSSL codepath available),
instead of refusing to work (and forcing user to say --curl or to
stop saying --no-curl, which is one unnecessary roadblock for the
user). Why do we want to change the error handling strategy that
has been in place?
I think I made the same comment in some other thread, but the
principle is the same. If there is no other choice the user can
take, do we force users to stop and be explicit to choose that only
available choice, or do we let the program choose the only available
option for the user while clearly telling the user that is what we
did? Here, changing the behaviour sounds like a disservice to the
users.
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH v2 4/6] imap-send: make --curl no-optional
2023-02-02 20:42 ` Junio C Hamano
@ 2023-02-03 21:46 ` Ævar Arnfjörð Bjarmason
2023-02-04 5:22 ` Junio C Hamano
0 siblings, 1 reply; 37+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2023-02-03 21:46 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jiang Xin, Bernhard Reiter, Remi Pommarel, Jeff King
On Thu, Feb 02 2023, Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> In the preceding commit the old "USE_CURL_FOR_IMAP_SEND" define became
>> always true, as we now require libcurl for git-imap-send.
>>
>> But as we require OpenSSL for the "tunnel" mode we still need to keep
>> the OpenSSL codepath around (ee [1] for an attempt to remove it). But
>
> "(ee" -> ???
Should be "e.g.", will fix.
>> we don't need to keep supporting "--no-curl" to bypass the curl
>> codepath for the non-tunnel mode.
>
> We do not need to because...?
We don't have that code anymore, will clarify.
>> @@ -1519,12 +1519,8 @@ int cmd_main(int argc, const char **argv)
>> if (argc)
>> usage_with_options(imap_send_usage, imap_send_options);
>>
>> -#if defined(NO_OPENSSL)
>> - if (!use_curl) {
>> - warning("--no-curl not supported in this build");
>> - use_curl = 1;
>> - }
>> -#endif
>> + if (!use_curl)
>> + die(_("the --no-curl option to imap-send has been deprecated"));
>
> We used to force use of cURL when there is no other way to make the
> program work (i.e. there is no direct OpenSSL codepath available),
> instead of refusing to work (and forcing user to say --curl or to
> stop saying --no-curl, which is one unnecessary roadblock for the
> user). Why do we want to change the error handling strategy that
> has been in place?
I can change this to a soft error, but it seemed more sensible to rip
the band-aid off an option that's never going to do anything now,
whereas before it would do something based on how you compiled git.
> I think I made the same comment in some other thread, but the
> principle is the same. If there is no other choice the user can
> take, do we force users to stop and be explicit to choose that only
> available choice, or do we let the program choose the only available
> option for the user while clearly telling the user that is what we
> did? Here, changing the behaviour sounds like a disservice to the
> users.
At best we can make --no-curl use curl anyway with a warning, would that
be better?
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH v2 4/6] imap-send: make --curl no-optional
2023-02-03 21:46 ` Ævar Arnfjörð Bjarmason
@ 2023-02-04 5:22 ` Junio C Hamano
0 siblings, 0 replies; 37+ messages in thread
From: Junio C Hamano @ 2023-02-04 5:22 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Jiang Xin, Bernhard Reiter, Remi Pommarel, Jeff King
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> On Thu, Feb 02 2023, Junio C Hamano wrote:
>
>> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>>
>>> In the preceding commit the old "USE_CURL_FOR_IMAP_SEND" define became
>>> always true, as we now require libcurl for git-imap-send.
>>>
>>> But as we require OpenSSL for the "tunnel" mode we still need to keep
>>> the OpenSSL codepath around (ee [1] for an attempt to remove it). But
>>
>> "(ee" -> ???
>
> Should be "e.g.", will fix.
I think it's more like a missing 'S' before 'ee'.
>>> -#if defined(NO_OPENSSL)
>>> - if (!use_curl) {
>>> - warning("--no-curl not supported in this build");
>>> - use_curl = 1;
>>> - }
>>> -#endif
>>> + if (!use_curl)
>>> + die(_("the --no-curl option to imap-send has been deprecated"));
>>
>> We used to force use of cURL when there is no other way to make the
>> program work (i.e. there is no direct OpenSSL codepath available),
>> instead of refusing to work (and forcing user to say --curl or to
>> stop saying --no-curl, which is one unnecessary roadblock for the
>> user). Why do we want to change the error handling strategy that
>> has been in place?
>
> I can change this to a soft error, but it seemed more sensible to rip
> the band-aid off an option that's never going to do anything now,
> whereas before it would do something based on how you compiled git.
Stopping and forcing the user to spend an extra step does not sound
sensible to me. Let's never do this kind of behaviour change "while
at it".
Thanks.
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH v2 5/6] imap-send: remove old --no-curl codepath
2023-02-02 9:44 ` [PATCH v2 0/6] " Ævar Arnfjörð Bjarmason
` (3 preceding siblings ...)
2023-02-02 9:44 ` [PATCH v2 4/6] imap-send: make --curl no-optional Ævar Arnfjörð Bjarmason
@ 2023-02-02 9:44 ` Ævar Arnfjörð Bjarmason
2023-02-02 20:43 ` Junio C Hamano
2023-02-02 9:44 ` [PATCH v2 6/6] imap-send: correctly report "host" when using "tunnel" Ævar Arnfjörð Bjarmason
5 siblings, 1 reply; 37+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2023-02-02 9:44 UTC (permalink / raw)
To: git
Cc: Jiang Xin, Junio C Hamano, Bernhard Reiter, Remi Pommarel,
Jeff King, Ævar Arnfjörð Bjarmason
In the preceding the "--curl" codepath was made mandatory, so now we
won't use the OpenSSL implementation codepaths in imap-send.c except
for "imap.tunnel".
So let's follow-up and delete the code on that path which was specific
to the "imap.host" mode.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
imap-send.c | 127 +++++++---------------------------------------------
1 file changed, 16 insertions(+), 111 deletions(-)
diff --git a/imap-send.c b/imap-send.c
index 9d7cb22285d..9712a8d4f93 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -197,14 +197,7 @@ static void socket_perror(const char *func, struct imap_socket *sock, int ret)
}
}
-#ifdef NO_OPENSSL
-static int ssl_socket_connect(struct imap_socket *sock, int use_tls_only, int verify)
-{
- fprintf(stderr, "SSL requested but SSL support not compiled in\n");
- return -1;
-}
-
-#else
+#ifndef NO_OPENSSL
static int host_matches(const char *host, const char *pattern)
{
@@ -253,7 +246,7 @@ static int verify_hostname(X509 *cert, const char *hostname)
cname, hostname);
}
-static int ssl_socket_connect(struct imap_socket *sock, int use_tls_only, int verify)
+static int ssl_socket_connect(struct imap_socket *sock, int verify)
{
#if (OPENSSL_VERSION_NUMBER >= 0x10000000L)
const SSL_METHOD *meth;
@@ -279,8 +272,7 @@ static int ssl_socket_connect(struct imap_socket *sock, int use_tls_only, int ve
return -1;
}
- if (use_tls_only)
- SSL_CTX_set_options(ctx, SSL_OP_NO_SSLv2 | SSL_OP_NO_SSLv3);
+ SSL_CTX_set_options(ctx, SSL_OP_NO_SSLv2 | SSL_OP_NO_SSLv3);
if (verify)
SSL_CTX_set_verify(ctx, SSL_VERIFY_PEER, NULL);
@@ -944,7 +936,8 @@ static struct imap_store *imap_open_store(struct imap_server_conf *srvc, const c
struct imap_store *ctx;
struct imap *imap;
char *arg, *rsp;
- int s = -1, preauth;
+ int preauth;
+ struct child_process tunnel = CHILD_PROCESS_INIT;
CALLOC_ARRAY(ctx, 1);
@@ -953,107 +946,19 @@ static struct imap_store *imap_open_store(struct imap_server_conf *srvc, const c
imap->in_progress_append = &imap->in_progress;
/* open connection to IMAP server */
+ imap_info("Starting tunnel '%s'... ", srvc->tunnel);
- if (srvc->tunnel) {
- struct child_process tunnel = CHILD_PROCESS_INIT;
-
- imap_info("Starting tunnel '%s'... ", srvc->tunnel);
-
- strvec_push(&tunnel.args, srvc->tunnel);
- tunnel.use_shell = 1;
- tunnel.in = -1;
- tunnel.out = -1;
- if (start_command(&tunnel))
- die("cannot start proxy %s", srvc->tunnel);
-
- imap->buf.sock.fd[0] = tunnel.out;
- imap->buf.sock.fd[1] = tunnel.in;
-
- imap_info("ok\n");
- } else {
-#ifndef NO_IPV6
- struct addrinfo hints, *ai0, *ai;
- int gai;
- char portstr[6];
-
- xsnprintf(portstr, sizeof(portstr), "%d", srvc->port);
-
- memset(&hints, 0, sizeof(hints));
- hints.ai_socktype = SOCK_STREAM;
- hints.ai_protocol = IPPROTO_TCP;
-
- imap_info("Resolving %s... ", srvc->host);
- gai = getaddrinfo(srvc->host, portstr, &hints, &ai);
- if (gai) {
- fprintf(stderr, "getaddrinfo: %s\n", gai_strerror(gai));
- goto bail;
- }
- imap_info("ok\n");
-
- for (ai0 = ai; ai; ai = ai->ai_next) {
- char addr[NI_MAXHOST];
-
- s = socket(ai->ai_family, ai->ai_socktype,
- ai->ai_protocol);
- if (s < 0)
- continue;
+ strvec_push(&tunnel.args, srvc->tunnel);
+ tunnel.use_shell = 1;
+ tunnel.in = -1;
+ tunnel.out = -1;
+ if (start_command(&tunnel))
+ die("cannot start proxy %s", srvc->tunnel);
- getnameinfo(ai->ai_addr, ai->ai_addrlen, addr,
- sizeof(addr), NULL, 0, NI_NUMERICHOST);
- imap_info("Connecting to [%s]:%s... ", addr, portstr);
+ imap->buf.sock.fd[0] = tunnel.out;
+ imap->buf.sock.fd[1] = tunnel.in;
- if (connect(s, ai->ai_addr, ai->ai_addrlen) < 0) {
- close(s);
- s = -1;
- perror("connect");
- continue;
- }
-
- break;
- }
- freeaddrinfo(ai0);
-#else /* NO_IPV6 */
- struct hostent *he;
- struct sockaddr_in addr;
-
- memset(&addr, 0, sizeof(addr));
- addr.sin_port = htons(srvc->port);
- addr.sin_family = AF_INET;
-
- imap_info("Resolving %s... ", srvc->host);
- he = gethostbyname(srvc->host);
- if (!he) {
- perror("gethostbyname");
- goto bail;
- }
- imap_info("ok\n");
-
- addr.sin_addr.s_addr = *((int *) he->h_addr_list[0]);
-
- s = socket(PF_INET, SOCK_STREAM, 0);
-
- imap_info("Connecting to %s:%hu... ", inet_ntoa(addr.sin_addr), ntohs(addr.sin_port));
- if (connect(s, (struct sockaddr *)&addr, sizeof(addr))) {
- close(s);
- s = -1;
- perror("connect");
- }
-#endif
- if (s < 0) {
- fputs("Error: unable to connect to server.\n", stderr);
- goto bail;
- }
-
- imap->buf.sock.fd[0] = s;
- imap->buf.sock.fd[1] = dup(s);
-
- if (srvc->use_ssl &&
- ssl_socket_connect(&imap->buf.sock, 0, srvc->ssl_verify)) {
- close(s);
- goto bail;
- }
- imap_info("ok\n");
- }
+ imap_info("ok\n");
/* read the greeting string */
if (buffer_gets(&imap->buf, &rsp)) {
@@ -1081,7 +986,7 @@ static struct imap_store *imap_open_store(struct imap_server_conf *srvc, const c
if (!srvc->use_ssl && CAP(STARTTLS)) {
if (imap_exec(ctx, NULL, "STARTTLS") != RESP_OK)
goto bail;
- if (ssl_socket_connect(&imap->buf.sock, 1,
+ if (ssl_socket_connect(&imap->buf.sock,
srvc->ssl_verify))
goto bail;
/* capabilities may have changed, so get the new capabilities */
--
2.39.1.1392.g63e6d408230
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH v2 5/6] imap-send: remove old --no-curl codepath
2023-02-02 9:44 ` [PATCH v2 5/6] imap-send: remove old --no-curl codepath Ævar Arnfjörð Bjarmason
@ 2023-02-02 20:43 ` Junio C Hamano
0 siblings, 0 replies; 37+ messages in thread
From: Junio C Hamano @ 2023-02-02 20:43 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Jiang Xin, Bernhard Reiter, Remi Pommarel, Jeff King
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> In the preceding the "--curl" codepath was made mandatory, so now we
> won't use the OpenSSL implementation codepaths in imap-send.c except
> for "imap.tunnel".
>
> So let's follow-up and delete the code on that path which was specific
> to the "imap.host" mode.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> imap-send.c | 127 +++++++---------------------------------------------
> 1 file changed, 16 insertions(+), 111 deletions(-)
Nice code reduction.
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH v2 6/6] imap-send: correctly report "host" when using "tunnel"
2023-02-02 9:44 ` [PATCH v2 0/6] " Ævar Arnfjörð Bjarmason
` (4 preceding siblings ...)
2023-02-02 9:44 ` [PATCH v2 5/6] imap-send: remove old --no-curl codepath Ævar Arnfjörð Bjarmason
@ 2023-02-02 9:44 ` Ævar Arnfjörð Bjarmason
2023-02-02 20:56 ` Junio C Hamano
2023-02-03 17:53 ` Jeff King
5 siblings, 2 replies; 37+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2023-02-02 9:44 UTC (permalink / raw)
To: git
Cc: Jiang Xin, Junio C Hamano, Bernhard Reiter, Remi Pommarel,
Jeff King, Ævar Arnfjörð Bjarmason
Before [1] we'd force the "imap.host" to be set, even if the
"imap.tunnel" was set, and then proceed to not use the "host" for
establishing a connection, as we'd use the tunneling command.
However, we'd still use the "imap.host" if it was set as the "host"
field given to the credential helper, and in messages that were shared
with the non-tunnel mode, until a preceding commit made these OpenSSL
codepaths tunnel-only.
Let's always give "host=tunnel" to the credential helper when in the
"imap.tunnel" mode, and rephrase the relevant messages to indicate
that we're tunneling. This changes the existing behavior, but that
behavior was emergent and didn't make much sense. If we were using
"imap.tunnel" the value in "imap.host" might be entirely unrelated to
the host we're tunneling to. Let's not pretend to know more than we do
in that case.
1. 34b5cd1fe9f (Don't force imap.host to be set when imap.tunnel is
set, 2008-04-22)
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
imap-send.c | 17 +++++++----------
1 file changed, 7 insertions(+), 10 deletions(-)
diff --git a/imap-send.c b/imap-send.c
index 9712a8d4f93..24b30c143a7 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -917,7 +917,7 @@ static void server_fill_credential(struct imap_server_conf *srvc, struct credent
return;
cred->protocol = xstrdup(srvc->use_ssl ? "imaps" : "imap");
- cred->host = xstrdup(srvc->host);
+ cred->host = xstrdup(srvc->tunnel ? "tunnel" : srvc->host);
cred->username = xstrdup_or_null(srvc->user);
cred->password = xstrdup_or_null(srvc->pass);
@@ -1004,7 +1004,7 @@ static struct imap_store *imap_open_store(struct imap_server_conf *srvc, const c
if (!CAP(AUTH_CRAM_MD5)) {
fprintf(stderr, "You specified "
"CRAM-MD5 as authentication method, "
- "but %s doesn't support it.\n", srvc->host);
+ "but tunnel doesn't support it.\n");
goto bail;
}
/* CRAM-MD5 */
@@ -1021,8 +1021,8 @@ static struct imap_store *imap_open_store(struct imap_server_conf *srvc, const c
}
} else {
if (CAP(NOLOGIN)) {
- fprintf(stderr, "Skipping account %s@%s, server forbids LOGIN\n",
- srvc->user, srvc->host);
+ fprintf(stderr, "Skipping account %s, server forbids LOGIN\n",
+ srvc->user);
goto bail;
}
if (!imap->buf.sock.ssl)
@@ -1434,12 +1434,9 @@ int cmd_main(int argc, const char **argv)
fprintf(stderr, "no imap store specified\n");
return 1;
}
- if (!server.host) {
- if (!server.tunnel) {
- fprintf(stderr, "no imap host specified\n");
- return 1;
- }
- server.host = "tunnel";
+ if (!server.host && !server.tunnel) {
+ fprintf(stderr, "no imap host specified\n");
+ return 1;
}
/* read the messages */
--
2.39.1.1392.g63e6d408230
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH v2 6/6] imap-send: correctly report "host" when using "tunnel"
2023-02-02 9:44 ` [PATCH v2 6/6] imap-send: correctly report "host" when using "tunnel" Ævar Arnfjörð Bjarmason
@ 2023-02-02 20:56 ` Junio C Hamano
2023-02-03 17:53 ` Jeff King
1 sibling, 0 replies; 37+ messages in thread
From: Junio C Hamano @ 2023-02-02 20:56 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Jiang Xin, Bernhard Reiter, Remi Pommarel, Jeff King
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> Before [1] we'd force the "imap.host" to be set, even if the
> "imap.tunnel" was set, and then proceed to not use the "host" for
> establishing a connection, as we'd use the tunneling command.
>
> However, we'd still use the "imap.host" if it was set as the "host"
> field given to the credential helper, and in messages that were shared
> with the non-tunnel mode, until a preceding commit made these OpenSSL
> codepaths tunnel-only.
>
> Let's always give "host=tunnel" to the credential helper when in the
> "imap.tunnel" mode, and rephrase the relevant messages to indicate
> that we're tunneling. This changes the existing behavior, but that
> behavior was emergent and didn't make much sense. If we were using
> "imap.tunnel" the value in "imap.host" might be entirely unrelated to
> the host we're tunneling to. Let's not pretend to know more than we do
> in that case.
>
> 1. 34b5cd1fe9f (Don't force imap.host to be set when imap.tunnel is
> set, 2008-04-22)
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
I agree with the above flow of thought in principle, but I wonder if
"tunnel" is distinct enough to allow credential helpers to tell that
they are dealing with a "tunnel", and not a host whose name happens
to be "tunnel". Would it help to use a token that can never be a
valid hostname instead, I wonder?
> @@ -1004,7 +1004,7 @@ static struct imap_store *imap_open_store(struct imap_server_conf *srvc, const c
> if (!CAP(AUTH_CRAM_MD5)) {
> fprintf(stderr, "You specified "
> "CRAM-MD5 as authentication method, "
> - "but %s doesn't support it.\n", srvc->host);
> + "but tunnel doesn't support it.\n");
Do we need some article before "tunnel"?
> if (CAP(NOLOGIN)) {
> - fprintf(stderr, "Skipping account %s@%s, server forbids LOGIN\n",
> - srvc->user, srvc->host);
> + fprintf(stderr, "Skipping account %s, server forbids LOGIN\n",
> + srvc->user);
> goto bail;
OK. We are talking to whatever "tunnel" is that was spawned to talk
somewhere we do not have a way to know, so trying to say <this user>
at <that host> is futile. Makes sense.
> - if (!server.host) {
> - if (!server.tunnel) {
> - fprintf(stderr, "no imap host specified\n");
> - return 1;
> - }
> - server.host = "tunnel";
> + if (!server.host && !server.tunnel) {
> + fprintf(stderr, "no imap host specified\n");
> + return 1;
> }
OK, this is a natural consequence that we no longer abuse
server.host in the tunneling case. Makes sense.
Thanks. Will queue.
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH v2 6/6] imap-send: correctly report "host" when using "tunnel"
2023-02-02 9:44 ` [PATCH v2 6/6] imap-send: correctly report "host" when using "tunnel" Ævar Arnfjörð Bjarmason
2023-02-02 20:56 ` Junio C Hamano
@ 2023-02-03 17:53 ` Jeff King
2023-02-03 21:12 ` Ævar Arnfjörð Bjarmason
1 sibling, 1 reply; 37+ messages in thread
From: Jeff King @ 2023-02-03 17:53 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Jiang Xin, Junio C Hamano, Bernhard Reiter, Remi Pommarel
On Thu, Feb 02, 2023 at 10:44:17AM +0100, Ævar Arnfjörð Bjarmason wrote:
> Before [1] we'd force the "imap.host" to be set, even if the
> "imap.tunnel" was set, and then proceed to not use the "host" for
> establishing a connection, as we'd use the tunneling command.
>
> However, we'd still use the "imap.host" if it was set as the "host"
> field given to the credential helper, and in messages that were shared
> with the non-tunnel mode, until a preceding commit made these OpenSSL
> codepaths tunnel-only.
>
> Let's always give "host=tunnel" to the credential helper when in the
> "imap.tunnel" mode, and rephrase the relevant messages to indicate
> that we're tunneling. This changes the existing behavior, but that
> behavior was emergent and didn't make much sense. If we were using
> "imap.tunnel" the value in "imap.host" might be entirely unrelated to
> the host we're tunneling to. Let's not pretend to know more than we do
> in that case.
If you tunnel to two different hosts, how is the credential system
supposed to know which is which?
If you really want to distinguish connecting to $host versus tunneling
to $host, I think you'd have to invent some new URL scheme
(imap-tunnel:// or something).
But IMHO it is not really worth it. Your statement of "the value in
imap.host might be entirely unrelated" does not match my experience. I
don't use imap-send, but I've been doing imap-tunneling with various
programs for two decades, and it's pretty normal to configure both, and
to consider the tunnel command as an implementation detail for getting
to the host. For example, my mutt config is like[1]:
set folder = imap://example.com/
set tunnel = "ssh example.com /etc/rimapd"
and I expect to be able to refer to folders as imap://example.com/foo,
etc (well, in mutt you'd use the shorthand "=foo", but the idea is the
same). So if we see:
[imap]
host = example.com
tunnel = ssh example.com /etc/rimapd
we should likewise think of it as example.com, but with an
implementation detail of how to contact the server.
Of course if you don't set imap.host, then we don't have anything useful
to say. But as you saw, in that case imap-send will default the host
field to the word "tunnel".
-Peff
[1] In my experience the main reason to tunnel is to avoid auth
altogether, so for those cases the credential code wouldn't matter
either way. But I imagine there may be some people who use it to pierce
a firewall or some other network obstacle, and really do want it to
be otherwise just like a connection to $host.
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH v2 6/6] imap-send: correctly report "host" when using "tunnel"
2023-02-03 17:53 ` Jeff King
@ 2023-02-03 21:12 ` Ævar Arnfjörð Bjarmason
2023-02-04 11:09 ` Jeff King
0 siblings, 1 reply; 37+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2023-02-03 21:12 UTC (permalink / raw)
To: Jeff King; +Cc: git, Jiang Xin, Junio C Hamano, Bernhard Reiter, Remi Pommarel
On Fri, Feb 03 2023, Jeff King wrote:
> On Thu, Feb 02, 2023 at 10:44:17AM +0100, Ævar Arnfjörð Bjarmason wrote:
>
>> Before [1] we'd force the "imap.host" to be set, even if the
>> "imap.tunnel" was set, and then proceed to not use the "host" for
>> establishing a connection, as we'd use the tunneling command.
>>
>> However, we'd still use the "imap.host" if it was set as the "host"
>> field given to the credential helper, and in messages that were shared
>> with the non-tunnel mode, until a preceding commit made these OpenSSL
>> codepaths tunnel-only.
>>
>> Let's always give "host=tunnel" to the credential helper when in the
>> "imap.tunnel" mode, and rephrase the relevant messages to indicate
>> that we're tunneling. This changes the existing behavior, but that
>> behavior was emergent and didn't make much sense. If we were using
>> "imap.tunnel" the value in "imap.host" might be entirely unrelated to
>> the host we're tunneling to. Let's not pretend to know more than we do
>> in that case.
>
> If you tunnel to two different hosts, how is the credential system
> supposed to know which is which?
>
> If you really want to distinguish connecting to $host versus tunneling
> to $host, I think you'd have to invent some new URL scheme
> (imap-tunnel:// or something).
>
> But IMHO it is not really worth it. Your statement of "the value in
> imap.host might be entirely unrelated" does not match my experience. I
> don't use imap-send, but I've been doing imap-tunneling with various
> programs for two decades, and it's pretty normal to configure both, and
> to consider the tunnel command as an implementation detail for getting
> to the host. For example, my mutt config is like[1]:
>
> set folder = imap://example.com/
> set tunnel = "ssh example.com /etc/rimapd"
>
> and I expect to be able to refer to folders as imap://example.com/foo,
> etc (well, in mutt you'd use the shorthand "=foo", but the idea is the
> same). So if we see:
>
> [imap]
> host = example.com
> tunnel = ssh example.com /etc/rimapd
>
> we should likewise think of it as example.com, but with an
> implementation detail of how to contact the server.
Except that mutt config is different than the imap-send case in that it
would presumably break if you changed:
set folder = imap://example.com/
set tunnel = "ssh example.com /etc/rimapd"
So that one of the two was example.org instead of example.com (or
whatever).
I agree that "give this to the auth helper" might be useful in general,
but our current documentation says:
To use the tool, `imap.folder` and either `imap.tunnel` or `imap.host` must be set
to appropriate values.
And the docs for "imap.tunnel" say "Required when imap.host is not set",
and "imap.host" says "Ignored when imap.tunnel is set, but required
otherwise".
Perhaps we should bless this as an accidental feature instead of my
proposed patch, but that's why I made this change. It seemed like an
unintentional bug that nobody intended.
Especially as you're focusing on the case where it contrary to the docs
would do what you mean, but consider (same as the doc examples, but the
domains are changed):
[imap]
folder = "INBOX.Drafts"
host = imap://imap.bar.com
user = bob
pass = p4ssw0rd
[imap]
folder = "INBOX.Drafts"
tunnel = "ssh -q -C user@foo.com /usr/bin/imapd ./Maildir 2> /dev/null"
I.e. I have a config for "bar.com" I tried earlier, but now I'm trying
to connect to "foo.com", because I read the docs and notice it prefers
"tunnel" to "host" I think it's going to ignore that "imap.host", but
it's going to provide the password for bar.com to foo.com if challenged.
So I think if we want to keep this it would be better to have a
imap.tunnel.credentialHost or something, to avoid conflating the two.
But I think it's okey to just remove this until someone has this
explicit use-case. I doubt that we have any users relying on this, as
it's not only undocumented, but the documentation explicitly states that
it doesn't work like this.
> Of course if you don't set imap.host, then we don't have anything useful
> to say. But as you saw, in that case imap-send will default the host
> field to the word "tunnel".
Isn't that more of a suggestion that nobody cares about this? Presumably
if we had users trying to get this to work someone would have complained
that they wanted a custom string rather than "tunnel", as the auth
helper isn't very helpful in that case...
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 6/6] imap-send: correctly report "host" when using "tunnel"
2023-02-03 21:12 ` Ævar Arnfjörð Bjarmason
@ 2023-02-04 11:09 ` Jeff King
2023-02-05 21:51 ` Ævar Arnfjörð Bjarmason
2023-02-06 21:41 ` Junio C Hamano
0 siblings, 2 replies; 37+ messages in thread
From: Jeff King @ 2023-02-04 11:09 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Jiang Xin, Junio C Hamano, Bernhard Reiter, Remi Pommarel
On Fri, Feb 03, 2023 at 10:12:27PM +0100, Ævar Arnfjörð Bjarmason wrote:
> Except that mutt config is different than the imap-send case in that it
> would presumably break if you changed:
>
> set folder = imap://example.com/
> set tunnel = "ssh example.com /etc/rimapd"
>
> So that one of the two was example.org instead of example.com (or
> whatever).
No, it would be perfectly happy (and in fact they do not exactly match
in my config). When you have a tunnel, that is the authoritative way to
get to the host, and the hostname mostly becomes a label. But
importantly, it is still used for certain things like local cache keys,
which would be shared with a non-tunnel connection to the same name.
Recent versions of mutt do also use the hostname for TLS verification,
if your tunnel is not itself secure (though this is not the default; you
have to set special options to tell it so).
> I agree that "give this to the auth helper" might be useful in general,
> but our current documentation says:
>
> To use the tool, `imap.folder` and either `imap.tunnel` or `imap.host` must be set
> to appropriate values.
>
> And the docs for "imap.tunnel" say "Required when imap.host is not set",
> and "imap.host" says "Ignored when imap.tunnel is set, but required
> otherwise".
>
> Perhaps we should bless this as an accidental feature instead of my
> proposed patch, but that's why I made this change. It seemed like an
> unintentional bug that nobody intended.
Yes, I agree that the scenario I'm giving is contrary to what the docs
say. But IMHO it is worth preferring what the code does now versus what
the docs say. The current behavior misbehaves if you configure things
badly (accidentally mix and match imap.host and imap.tunnel). Your new
behavior misbehaves if you have two correctly-configure imap stanzas
(both with a host/tunnel combo). Those are both fairly unlikely
scenarios, and the outcomes are similar (we mix up credentials), but:
1. In general, all things being equal, I'd rather trust the code as
the status quo. People will complain if you break their working
setup. They won't if you fix the documentation.
2. In the current behavior, if it's doing the wrong thing, your next
step is to fix your configuration (don't mix and match imap.host
and imap.tunnel). In your proposed behavior, there is no fix. You
are simply not allowed to use two different imap tunnels with
credential helpers, because the helpers don't receive enough
context to distinguish them.
And that is not even "two imap tunnels in the same config". It is
really per user. If I have two repositories, each with
"imap.tunnel" in their local config, they will still invoke the
same credential helpers, and both will just see host=tunnel. The
namespace for "host" really is global and should be unique (ideally
across the Internet, but at the very least among the hosts that the
user contacts).
One fix would be to pass the tunnel command as the hostname. But even
that has potential problems. Certainly you'd have to tweak it (it's a
shell command, and hostnames have syntactic restrictions, including
forbidding newlines). And you'd probably want to swap out protocol=imap
for something else. But I'm not sure if helpers may complain (e.g., I
seem to recall that the osxkeychain helper translates protocol strings
into integer constants that the keychain API understands).
> Especially as you're focusing on the case where it contrary to the docs
> would do what you mean, but consider (same as the doc examples, but the
> domains are changed):
>
> [imap]
> folder = "INBOX.Drafts"
> host = imap://imap.bar.com
> user = bob
> pass = p4ssw0rd
>
> [imap]
> folder = "INBOX.Drafts"
> tunnel = "ssh -q -C user@foo.com /usr/bin/imapd ./Maildir 2> /dev/null"
>
> I.e. I have a config for "bar.com" I tried earlier, but now I'm trying
> to connect to "foo.com", because I read the docs and notice it prefers
> "tunnel" to "host" I think it's going to ignore that "imap.host", but
> it's going to provide the password for bar.com to foo.com if challenged.
Yes, that's a rather unfortunate effect of the way we do config parsing
(it looks to the user like stanzas, but we don't parse it that way; the
second stanza could even be in a different file!).
Though as I said above, I still think this case does not justify making
the code change, I do think it's the most compelling argument, and would
make sense to include in the commit message if we did want to do this.
> So I think if we want to keep this it would be better to have a
> imap.tunnel.credentialHost or something, to avoid conflating the two.
Yes, there are many config schemes that would avoid this problem. If you
are going to tie the two together, I think it would make sense to use
real subsections based on the host-name, like:
# hostname is the subsection key; it also becomes a label when
# necessary
[imap "example.com"]
# does not even need to mention a hostname. We'll assume example.com
# here.
tunnel = "any-command"
# assumes example.com as hostname; not needed if you are using a
# tunnel, of course
protocol = imaps
But I would not bother going to that work myself. IMHO imap-send is
somewhat of an abomination, and I'd actually be just as happy if it went
away. But what you are doing seems to go totally in the wrong direction
to me (keeping it, but breaking a rare but working use case to the
benefit of a rare but broken misconfiguration).
> > Of course if you don't set imap.host, then we don't have anything useful
> > to say. But as you saw, in that case imap-send will default the host
> > field to the word "tunnel".
>
> Isn't that more of a suggestion that nobody cares about this? Presumably
> if we had users trying to get this to work someone would have complained
> that they wanted a custom string rather than "tunnel", as the auth
> helper isn't very helpful in that case...
Not if they did:
[imap]
host = example.com
tunnel = some-command
-Peff
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH v2 6/6] imap-send: correctly report "host" when using "tunnel"
2023-02-04 11:09 ` Jeff King
@ 2023-02-05 21:51 ` Ævar Arnfjörð Bjarmason
2023-02-07 18:30 ` Jeff King
2023-02-06 21:41 ` Junio C Hamano
1 sibling, 1 reply; 37+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2023-02-05 21:51 UTC (permalink / raw)
To: Jeff King; +Cc: git, Jiang Xin, Junio C Hamano, Bernhard Reiter, Remi Pommarel
On Sat, Feb 04 2023, Jeff King wrote:
> On Fri, Feb 03, 2023 at 10:12:27PM +0100, Ævar Arnfjörð Bjarmason wrote:
> [...]
>> I agree that "give this to the auth helper" might be useful in general,
>> but our current documentation says:
>>
>> To use the tool, `imap.folder` and either `imap.tunnel` or `imap.host` must be set
>> to appropriate values.
>>
>> And the docs for "imap.tunnel" say "Required when imap.host is not set",
>> and "imap.host" says "Ignored when imap.tunnel is set, but required
>> otherwise".
>>
>> Perhaps we should bless this as an accidental feature instead of my
>> proposed patch, but that's why I made this change. It seemed like an
>> unintentional bug that nobody intended.
>
> Yes, I agree that the scenario I'm giving is contrary to what the docs
> say. But IMHO it is worth preferring what the code does now versus what
> the docs say. The current behavior misbehaves if you configure things
> badly (accidentally mix and match imap.host and imap.tunnel). Your new
> behavior misbehaves if you have two correctly-configure imap stanzas
> (both with a host/tunnel combo). Those are both fairly unlikely
> scenarios, and the outcomes are similar (we mix up credentials), but:
>
> 1. In general, all things being equal, I'd rather trust the code as
> the status quo. People will complain if you break their working
> setup. They won't if you fix the documentation.
>
> 2. In the current behavior, if it's doing the wrong thing, your next
> step is to fix your configuration (don't mix and match imap.host
> and imap.tunnel). In your proposed behavior, there is no fix. You
> are simply not allowed to use two different imap tunnels with
> credential helpers, because the helpers don't receive enough
> context to distinguish them.
>
> And that is not even "two imap tunnels in the same config". It is
> really per user. If I have two repositories, each with
> "imap.tunnel" in their local config, they will still invoke the
> same credential helpers, and both will just see host=tunnel. The
> namespace for "host" really is global and should be unique (ideally
> across the Internet, but at the very least among the hosts that the
> user contacts).
>
> One fix would be to pass the tunnel command as the hostname. But even
> that has potential problems. Certainly you'd have to tweak it (it's a
> shell command, and hostnames have syntactic restrictions, including
> forbidding newlines). And you'd probably want to swap out protocol=imap
> for something else. But I'm not sure if helpers may complain (e.g., I
> seem to recall that the osxkeychain helper translates protocol strings
> into integer constants that the keychain API understands).
>
>> Especially as you're focusing on the case where it contrary to the docs
>> would do what you mean, but consider (same as the doc examples, but the
>> domains are changed):
>>
>> [imap]
>> folder = "INBOX.Drafts"
>> host = imap://imap.bar.com
>> user = bob
>> pass = p4ssw0rd
>>
>> [imap]
>> folder = "INBOX.Drafts"
>> tunnel = "ssh -q -C user@foo.com /usr/bin/imapd ./Maildir 2> /dev/null"
>>
>> I.e. I have a config for "bar.com" I tried earlier, but now I'm trying
>> to connect to "foo.com", because I read the docs and notice it prefers
>> "tunnel" to "host" I think it's going to ignore that "imap.host", but
>> it's going to provide the password for bar.com to foo.com if challenged.
>
> Yes, that's a rather unfortunate effect of the way we do config parsing
> (it looks to the user like stanzas, but we don't parse it that way; the
> second stanza could even be in a different file!).
>
> Though as I said above, I still think this case does not justify making
> the code change, I do think it's the most compelling argument, and would
> make sense to include in the commit message if we did want to do this.
>
>> So I think if we want to keep this it would be better to have a
>> imap.tunnel.credentialHost or something, to avoid conflating the two.
>
> Yes, there are many config schemes that would avoid this problem. If you
> are going to tie the two together, I think it would make sense to use
> real subsections based on the host-name, like:
>
> # hostname is the subsection key; it also becomes a label when
> # necessary
> [imap "example.com"]
>
> # does not even need to mention a hostname. We'll assume example.com
> # here.
> tunnel = "any-command"
>
> # assumes example.com as hostname; not needed if you are using a
> # tunnel, of course
> protocol = imaps
>
> But I would not bother going to that work myself. IMHO imap-send is
> somewhat of an abomination, and I'd actually be just as happy if it went
> away. But what you are doing seems to go totally in the wrong direction
> to me (keeping it, but breaking a rare but working use case to the
> benefit of a rare but broken misconfiguration).
>
>> > Of course if you don't set imap.host, then we don't have anything useful
>> > to say. But as you saw, in that case imap-send will default the host
>> > field to the word "tunnel".
>>
>> Isn't that more of a suggestion that nobody cares about this? Presumably
>> if we had users trying to get this to work someone would have complained
>> that they wanted a custom string rather than "tunnel", as the auth
>> helper isn't very helpful in that case...
>
> Not if they did:
>
> [imap]
> host = example.com
> tunnel = some-command
Yes, but how would they have ended up doing that? By discarding the
documentation and throwing things at the wall & hoping they'd stick?
I take all your general points above, and generally agree with them. Re
#1 we should generally prefer current behavior over the docs, and re #2:
Yes, I agree this might be useful in princple, and hardcoding
"host=tunnel" doesn't leave a way to pass a custom "host to the auth
helper.
I also don't care enough to argue about it, so I'll leave the first hunk
here out of any re-roll. We'll continue to pass "host" down in that
case, i.e. I'll only adjust the error messages.
I just don't get how anyone could have come to rely on this so that we'd
care about supporting it.
Because mutt has a feature that looks similar, users might have
configured git-imap-send thinking it might do the same thing, and gotten
lucky?
I guess in principle that could be true, but I think it's more likely
that nobody's ever had reason to use it that way. I.e. if you use the
"tunnel" the way the docs suggest you won't hit the credential helper,
as you're authenticating with "ssh", and using "imapd" to directly
operate on a Maildir path.
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 6/6] imap-send: correctly report "host" when using "tunnel"
2023-02-05 21:51 ` Ævar Arnfjörð Bjarmason
@ 2023-02-07 18:30 ` Jeff King
2023-02-07 20:39 ` Ævar Arnfjörð Bjarmason
0 siblings, 1 reply; 37+ messages in thread
From: Jeff King @ 2023-02-07 18:30 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Jiang Xin, Junio C Hamano, Bernhard Reiter, Remi Pommarel
On Sun, Feb 05, 2023 at 10:51:04PM +0100, Ævar Arnfjörð Bjarmason wrote:
> > Not if they did:
> >
> > [imap]
> > host = example.com
> > tunnel = some-command
>
> Yes, but how would they have ended up doing that? By discarding the
> documentation and throwing things at the wall & hoping they'd stick?
That's what I would have tried without reading the documentation at all,
based on using other programs that tunnel imap. I'm just one data point,
of course.
> I just don't get how anyone could have come to rely on this so that we'd
> care about supporting it.
>
> Because mutt has a feature that looks similar, users might have
> configured git-imap-send thinking it might do the same thing, and gotten
> lucky?
It's less "mutt happens to do it this way" and more "associating a host
is strictly more useful, because it lets you interact with all the other
host-like features". It's only imap-send's funky config scheme that
makes it easy to mis-configure.
> I guess in principle that could be true, but I think it's more likely
> that nobody's ever had reason to use it that way. I.e. if you use the
> "tunnel" the way the docs suggest you won't hit the credential helper,
> as you're authenticating with "ssh", and using "imapd" to directly
> operate on a Maildir path.
As I said, my main use of tunneling is to trigger the imap server's
preauth mode. But there are other reasons one might want to do so, like
piercing a firewall. E.g.:
[imap]
host = internal.example.com
tunnel = "ssh bastion-server nc internal.example.com 143"
-Peff
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 6/6] imap-send: correctly report "host" when using "tunnel"
2023-02-07 18:30 ` Jeff King
@ 2023-02-07 20:39 ` Ævar Arnfjörð Bjarmason
2023-02-07 21:26 ` Junio C Hamano
2023-02-07 22:15 ` Jeff King
0 siblings, 2 replies; 37+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2023-02-07 20:39 UTC (permalink / raw)
To: Jeff King; +Cc: git, Jiang Xin, Junio C Hamano, Bernhard Reiter, Remi Pommarel
On Tue, Feb 07 2023, Jeff King wrote:
> On Sun, Feb 05, 2023 at 10:51:04PM +0100, Ævar Arnfjörð Bjarmason wrote:
>
>> > Not if they did:
>> >
>> > [imap]
>> > host = example.com
>> > tunnel = some-command
>>
>> Yes, but how would they have ended up doing that? By discarding the
>> documentation and throwing things at the wall & hoping they'd stick?
>
> That's what I would have tried without reading the documentation at all,
> based on using other programs that tunnel imap. I'm just one data point,
> of course.
>
>> I just don't get how anyone could have come to rely on this so that we'd
>> care about supporting it.
>>
>> Because mutt has a feature that looks similar, users might have
>> configured git-imap-send thinking it might do the same thing, and gotten
>> lucky?
>
> It's less "mutt happens to do it this way" and more "associating a host
> is strictly more useful, because it lets you interact with all the other
> host-like features". It's only imap-send's funky config scheme that
> makes it easy to mis-configure.
>
>> I guess in principle that could be true, but I think it's more likely
>> that nobody's ever had reason to use it that way. I.e. if you use the
>> "tunnel" the way the docs suggest you won't hit the credential helper,
>> as you're authenticating with "ssh", and using "imapd" to directly
>> operate on a Maildir path.
*nod* I'll just note that you elided the part where I noted that I don't
really care, and will submit some re-roll that's compatible with the
current imap.{host,tunnel} interaction.
I think you might be right that people might rely on this after having
discovered this undocumented interaction by accident.
But I also think that the lack of questions about how to get imap-send's
tunnel mode to work with auth helpers (at least I couldn't find any
on-list), which is what you'd run into if you went by the documentation
& were trying to get htat ot work, is a pretty good sign that this may
be either entirely unused by anyone, or at best very obscure.
> As I said, my main use of tunneling is to trigger the imap server's
> preauth mode. But there are other reasons one might want to do so, like
> piercing a firewall. E.g.:
>
> [imap]
> host = internal.example.com
> tunnel = "ssh bastion-server nc internal.example.com 143"
I'll definitely leave this out of a re-roll of this topic, but I did
come up with an opinionated replacement on top.
That commitdwhich rips out non-PREAUTH (i.e. any authentication)
support, as well as SSL support that isn't using curl from
git-imap-send.c.
Here:
https://github.com/avar/git/commit/8498089f8e5a3d050b44008a7947ef3cefe2a2dd
I.e. if we just say that we're not going to support this use-case
anymore we can get rid of all of the OpenSSL reliance in-tree, except
for the optional (and hardly ever used) OPENSSL_SHA1, and
uses-only-one-API-function "HAVE_OPENSSL_CSPRNG" use.
I.e. we'd support tunneling like this still (from the manpage):
[imap]
folder = "INBOX.Drafts"
tunnel = "ssh -q -C user@example.com /usr/bin/imapd ./Maildir 2> /dev/null"
But if your use of imap.tunnel is to essentially use git-imap-send.c for
what you could use another shell (or systemd or whatever) to invoke a
"ssh" or "stunnel" command for you, we'd say too bad, just do that instead.
So your example of:
[imap]
host = internal.example.com
tunnel = "ssh bastion-server nc internal.example.com 143"
Would instead be:
1. Arrange for the equivalent of that to run outside of
git-imap-send, e.g.:
ssh -N -R 1430:internal.example.com:143 bastion-server
2. Use "imap.host" to connect to that "remote" box with libcurl,
but just use "localhost:1430"
Given the obscurity of git-imap-send overall, and how trivial the
workaround is I don't think that's unreasonable, even with an aggressive
transition period.
As that commit shows we have a surprising amount of code required to
support just this one use-case (and I'm not even sure I got all of
it). Or at least:
7 files changed, 89 insertions(+), 509 deletions(-)
With most being OpenSSL library use, so if we can find a way to not
keeping supporting that...
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH v2 6/6] imap-send: correctly report "host" when using "tunnel"
2023-02-07 20:39 ` Ævar Arnfjörð Bjarmason
@ 2023-02-07 21:26 ` Junio C Hamano
2023-02-07 22:04 ` Ævar Arnfjörð Bjarmason
2023-02-07 22:16 ` Jeff King
2023-02-07 22:15 ` Jeff King
1 sibling, 2 replies; 37+ messages in thread
From: Junio C Hamano @ 2023-02-07 21:26 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Jeff King, git, Jiang Xin, Bernhard Reiter, Remi Pommarel
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> I think you might be right that people might rely on this after having
> discovered this undocumented interaction by accident.
>
> But I also think that the lack of questions about how to get imap-send's
> tunnel mode to work with auth helpers (at least I couldn't find any
> on-list), which is what you'd run into if you went by the documentation
> & were trying to get htat ot work, is a pretty good sign that this may
> be either entirely unused by anyone, or at best very obscure.
I actually think the misconfiguration (from documentation's point of
view) Peff is taking advantage of is a behaviour you would naturally
expect, if you do not read the documentation but are merely aware of
the presence of .host and .tunnel and guess what these do. And
those who felt it was a natural design would probably not have asked
any question about it. Documenting the current behaviour better
would not hurt. Updating the behaviour and documenting the new
behaviour would not help anybody.
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 6/6] imap-send: correctly report "host" when using "tunnel"
2023-02-07 21:26 ` Junio C Hamano
@ 2023-02-07 22:04 ` Ævar Arnfjörð Bjarmason
2023-02-07 22:16 ` Jeff King
1 sibling, 0 replies; 37+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2023-02-07 22:04 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git, Jiang Xin, Bernhard Reiter, Remi Pommarel
On Tue, Feb 07 2023, Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> I think you might be right that people might rely on this after having
>> discovered this undocumented interaction by accident.
>>
>> But I also think that the lack of questions about how to get imap-send's
>> tunnel mode to work with auth helpers (at least I couldn't find any
>> on-list), which is what you'd run into if you went by the documentation
>> & were trying to get htat ot work, is a pretty good sign that this may
>> be either entirely unused by anyone, or at best very obscure.
>
> I actually think the misconfiguration (from documentation's point of
> view) Peff is taking advantage of is a behaviour you would naturally
> expect, if you do not read the documentation but are merely aware of
> the presence of .host and .tunnel and guess what these do. And
> those who felt it was a natural design would probably not have asked
> any question about it. Documenting the current behaviour better
> would not hurt. Updating the behaviour and documenting the new
> behaviour would not help anybody.
Sure, we don't have to belabor the point. It's moot for a re-roll of
this topic in any case (I won't be changing this behavior).
But do I take it from the non-reply to what came afterwards that you're
not interested in a (not a part of this topic) proposal for us to say
"if you want that, arrange for ssh to do it for you", which would allow
for finally dropping libssl as a non-trivial direct dependency? Or just
that you didn't get to considering that?
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 6/6] imap-send: correctly report "host" when using "tunnel"
2023-02-07 21:26 ` Junio C Hamano
2023-02-07 22:04 ` Ævar Arnfjörð Bjarmason
@ 2023-02-07 22:16 ` Jeff King
1 sibling, 0 replies; 37+ messages in thread
From: Jeff King @ 2023-02-07 22:16 UTC (permalink / raw)
To: Junio C Hamano
Cc: Ævar Arnfjörð Bjarmason, git, Jiang Xin,
Bernhard Reiter, Remi Pommarel
On Tue, Feb 07, 2023 at 01:26:10PM -0800, Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
> > I think you might be right that people might rely on this after having
> > discovered this undocumented interaction by accident.
> >
> > But I also think that the lack of questions about how to get imap-send's
> > tunnel mode to work with auth helpers (at least I couldn't find any
> > on-list), which is what you'd run into if you went by the documentation
> > & were trying to get htat ot work, is a pretty good sign that this may
> > be either entirely unused by anyone, or at best very obscure.
>
> I actually think the misconfiguration (from documentation's point of
> view) Peff is taking advantage of is a behaviour you would naturally
> expect, if you do not read the documentation but are merely aware of
> the presence of .host and .tunnel and guess what these do.
Just to be clear, I am not taking advantage of anything. I do not use
imap-send myself, because a much better solution is to have a decent
mail client that can access both imap and local maildirs. ;)
I was only trying to offer some perspective as a general imap-tunnel
user.
-Peff
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 6/6] imap-send: correctly report "host" when using "tunnel"
2023-02-07 20:39 ` Ævar Arnfjörð Bjarmason
2023-02-07 21:26 ` Junio C Hamano
@ 2023-02-07 22:15 ` Jeff King
2023-02-07 22:24 ` Junio C Hamano
2023-02-08 1:06 ` Ævar Arnfjörð Bjarmason
1 sibling, 2 replies; 37+ messages in thread
From: Jeff King @ 2023-02-07 22:15 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Jiang Xin, Junio C Hamano, Bernhard Reiter, Remi Pommarel
On Tue, Feb 07, 2023 at 09:39:48PM +0100, Ævar Arnfjörð Bjarmason wrote:
> *nod* I'll just note that you elided the part where I noted that I don't
> really care, and will submit some re-roll that's compatible with the
> current imap.{host,tunnel} interaction.
Yeah, sorry, I should have said "yes, thank you" there. :) I wasn't
meaning to continue arguing, but just trying to answer your "how would
they even find this?" confusion.
> I.e. if we just say that we're not going to support this use-case
> anymore we can get rid of all of the OpenSSL reliance in-tree, except
> for the optional (and hardly ever used) OPENSSL_SHA1, and
> uses-only-one-API-function "HAVE_OPENSSL_CSPRNG" use.
Yeah, getting rid of that openssl code is a reasonable goal. And this
may seem counter-intuitive, but I'm actually _more_ in favor of that
than the change you proposed here, even though it potentially breaks
more users. That's because I feel like we're buying something useful
with it, whereas with the patch we've been discussing, the tradeoff was
less clear to me.
That said, it seems like there should be a path forward for supporting
tunnels via curl, and then we could be getting rid of the openssl
dependency _and_ all of the custom and rarely-run imap code. But that's
an even bigger task, and I not only wouldn't want to work on it, I'm not
even sure I'd want to review it. I'm slightly regretting getting
involved here at all, because it seems like none of us actually care at
all about imap-send, and this has turned into a big discussion. I mostly
chimed in because it seemed like I had a perspective you didn't on how
people might use tunnels, and it felt like I should speak up for folks
whose use cases might be getting broken.
Side note: If somebody were proposing to add imap-send at all today,
I'd probably say "no, that should be a separate project, and you
should probably write it in some language that has a decent imap
library". It really has nothing at all to do with Git in terms of
implementation, and I suspect it's not super well maintained in
general. But perhaps it is too late for that.
> So your example of:
>
> [imap]
> host = internal.example.com
> tunnel = "ssh bastion-server nc internal.example.com 143"
>
> Would instead be:
>
> 1. Arrange for the equivalent of that to run outside of
> git-imap-send, e.g.:
>
> ssh -N -R 1430:internal.example.com:143 bastion-server
>
> 2. Use "imap.host" to connect to that "remote" box with libcurl,
> but just use "localhost:1430"
Having done something like that before, the "arrange" step is more
finicky than you might think (because sometimes it goes away, and you
really want to trigger it on demand).
-Peff
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH v2 6/6] imap-send: correctly report "host" when using "tunnel"
2023-02-07 22:15 ` Jeff King
@ 2023-02-07 22:24 ` Junio C Hamano
2023-02-08 1:06 ` Ævar Arnfjörð Bjarmason
1 sibling, 0 replies; 37+ messages in thread
From: Junio C Hamano @ 2023-02-07 22:24 UTC (permalink / raw)
To: Jeff King
Cc: Ævar Arnfjörð Bjarmason, git, Jiang Xin,
Bernhard Reiter, Remi Pommarel
Jeff King <peff@peff.net> writes:
> Side note: If somebody were proposing to add imap-send at all today,
> I'd probably say "no, that should be a separate project, and you
> should probably write it in some language that has a decent imap
> library". It really has nothing at all to do with Git in terms of
> implementation, and I suspect it's not super well maintained in
> general. But perhaps it is too late for that.
amen..
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 6/6] imap-send: correctly report "host" when using "tunnel"
2023-02-07 22:15 ` Jeff King
2023-02-07 22:24 ` Junio C Hamano
@ 2023-02-08 1:06 ` Ævar Arnfjörð Bjarmason
2023-02-17 20:50 ` Jeff King
1 sibling, 1 reply; 37+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2023-02-08 1:06 UTC (permalink / raw)
To: Jeff King; +Cc: git, Jiang Xin, Junio C Hamano, Bernhard Reiter, Remi Pommarel
On Tue, Feb 07 2023, Jeff King wrote:
> On Tue, Feb 07, 2023 at 09:39:48PM +0100, Ævar Arnfjörð Bjarmason wrote:
>
>> *nod* I'll just note that you elided the part where I noted that I don't
>> really care, and will submit some re-roll that's compatible with the
>> current imap.{host,tunnel} interaction.
>
> Yeah, sorry, I should have said "yes, thank you" there. :) I wasn't
> meaning to continue arguing, but just trying to answer your "how would
> they even find this?" confusion.
*nod*
>> I.e. if we just say that we're not going to support this use-case
>> anymore we can get rid of all of the OpenSSL reliance in-tree, except
>> for the optional (and hardly ever used) OPENSSL_SHA1, and
>> uses-only-one-API-function "HAVE_OPENSSL_CSPRNG" use.
>
> Yeah, getting rid of that openssl code is a reasonable goal. And this
> may seem counter-intuitive, but I'm actually _more_ in favor of that
> than the change you proposed here, even though it potentially breaks
> more users. That's because I feel like we're buying something useful
> with it, whereas with the patch we've been discussing, the tradeoff was
> less clear to me.
Makes sense.
> That said, it seems like there should be a path forward for supporting
> tunnels via curl, and then we could be getting rid of the openssl
> dependency _and_ all of the custom and rarely-run imap code.
I really think it's obscure enough that just offerng users a documented
way out should be neough.
> [...]
> Side note: If somebody were proposing to add imap-send at all today,
> I'd probably say "no, that should be a separate project, and you
> should probably write it in some language that has a decent imap
> library". It really has nothing at all to do with Git in terms of
> implementation, and I suspect it's not super well maintained in
> general. But perhaps it is too late for that.
I think it's a reasonable feature, but in hindsight our mistake was to
think that we should be perma-forking isync, which has since moved
on. I've used isync's "mbsync" extensively for IMAP in other contexts,
and it works well for that.
So if we were going back to the drawing board a "git-imap-sync" really
should just be something in our mail tooling that can produce a Maildir,
and if we wanted an IMAP helper it could invoke mbsync, offlineimap or
various other "maildir to IMAP" bidirectional syncing utilities to
"send" via IMAP.
So, just some hook support for format-patch with some documented
examples should do it, but I won't be working on that task...
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH v2 6/6] imap-send: correctly report "host" when using "tunnel"
2023-02-08 1:06 ` Ævar Arnfjörð Bjarmason
@ 2023-02-17 20:50 ` Jeff King
0 siblings, 0 replies; 37+ messages in thread
From: Jeff King @ 2023-02-17 20:50 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Jiang Xin, Junio C Hamano, Bernhard Reiter, Remi Pommarel
On Wed, Feb 08, 2023 at 02:06:55AM +0100, Ævar Arnfjörð Bjarmason wrote:
> > Side note: If somebody were proposing to add imap-send at all today,
> > I'd probably say "no, that should be a separate project, and you
> > should probably write it in some language that has a decent imap
> > library". It really has nothing at all to do with Git in terms of
> > implementation, and I suspect it's not super well maintained in
> > general. But perhaps it is too late for that.
>
> I think it's a reasonable feature, but in hindsight our mistake was to
> think that we should be perma-forking isync, which has since moved
> on. I've used isync's "mbsync" extensively for IMAP in other contexts,
> and it works well for that.
>
> So if we were going back to the drawing board a "git-imap-sync" really
> should just be something in our mail tooling that can produce a Maildir,
> and if we wanted an IMAP helper it could invoke mbsync, offlineimap or
> various other "maildir to IMAP" bidirectional syncing utilities to
> "send" via IMAP.
>
> So, just some hook support for format-patch with some documented
> examples should do it, but I won't be working on that task...
Yes, I think format-patch plus a sync program would be good. I did
briefly look at the state of imap sync programs and was a bit
disappointed. Many older recommendations are for software that is no
longer packaged, or hard to find. And none of the ones I looked at do
something as simple as "copy these messages to this imap server".
They're all very interested in bidirectional sync, incremental updates,
and so on. But I do think one could make mbsync or offlineimap work, if
you used a dedicated folder on the server as the destination.
But yeah, I don't think you or I needs to come up with a solution there.
I was more proposing along the lines of: let's drop imap-send, and
interested people can then make a solution based on other tools, or even
spin off imap-send into its own repository.
But I get that even that is some work, and it may mean complaining
users.
-Peff
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 6/6] imap-send: correctly report "host" when using "tunnel"
2023-02-04 11:09 ` Jeff King
2023-02-05 21:51 ` Ævar Arnfjörð Bjarmason
@ 2023-02-06 21:41 ` Junio C Hamano
1 sibling, 0 replies; 37+ messages in thread
From: Junio C Hamano @ 2023-02-06 21:41 UTC (permalink / raw)
To: Jeff King
Cc: Ævar Arnfjörð Bjarmason, git, Jiang Xin,
Bernhard Reiter, Remi Pommarel
Jeff King <peff@peff.net> writes:
> Yes, I agree that the scenario I'm giving is contrary to what the docs
> say. But IMHO it is worth preferring what the code does now versus what
> the docs say. The current behavior misbehaves if you configure things
> badly (accidentally mix and match imap.host and imap.tunnel). Your new
> behavior misbehaves if you have two correctly-configure imap stanzas
> (both with a host/tunnel combo). Those are both fairly unlikely
> scenarios, and the outcomes are similar (we mix up credentials), but:
>
> 1. In general, all things being equal, I'd rather trust the code as
> the status quo. People will complain if you break their working
> setup. They won't if you fix the documentation.
>
> 2. In the current behavior, if it's doing the wrong thing, your next
> step is to fix your configuration (don't mix and match imap.host
> and imap.tunnel). In your proposed behavior, there is no fix. You
> are simply not allowed to use two different imap tunnels with
> credential helpers, because the helpers don't receive enough
> context to distinguish them.
>
> And that is not even "two imap tunnels in the same config". It is
> really per user. If I have two repositories, each with
> "imap.tunnel" in their local config, they will still invoke the
> same credential helpers, and both will just see host=tunnel. The
> namespace for "host" really is global and should be unique (ideally
> across the Internet, but at the very least among the hosts that the
> user contacts).
All good points.
> Yes, there are many config schemes that would avoid this problem. If you
> are going to tie the two together, I think it would make sense to use
> real subsections based on the host-name, like:
>
> # hostname is the subsection key; it also becomes a label when
> # necessary
> [imap "example.com"]
>
> # does not even need to mention a hostname. We'll assume example.com
> # here.
> tunnel = "any-command"
>
> # assumes example.com as hostname; not needed if you are using a
> # tunnel, of course
> protocol = imaps
>
> But I would not bother going to that work myself. IMHO imap-send is
> somewhat of an abomination, and I'd actually be just as happy if it went
> away. But what you are doing seems to go totally in the wrong direction
> to me (keeping it, but breaking a rare but working use case to the
> benefit of a rare but broken misconfiguration).
Yup.
^ permalink raw reply [flat|nested] 37+ messages in thread