Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upon HTTP_1_1_REQUIRED, retry the request with HTTP/1.1 #3349

Closed
wants to merge 1 commit into from

Conversation

dscho
Copy link
Contributor

@dscho dscho commented Dec 7, 2018

This is a companion patch to #3345 which switches to HTTP/1.1 preemptively when we are about to try NTLM authentication.

However, with other (Negotiate) authentication it is not clear to this developer whether there is a way to make it work with HTTP/2, so let's try HTTP/2 first and fall back in case we encounter the error HTTP_1_1_REQUIRED.

This patch was developed with a lot of help by Daniel Stenberg.

(And yes, I tested this, by reverting the NTLM patch and authenticating against the same test server which I used to test the NTLM patch.)

@bagder
Copy link
Member

bagder commented Dec 7, 2018

./multi.c:1957:7: warning: else after closing brace on same line (BRACEELSE)
       } else if(CURLE_HTTP2_STREAM == result && data->set.errorbuffer &&
       ^

@bagder
Copy link
Member

bagder commented Dec 7, 2018

What do you think of this little extra dance to avoid the strstr in the error message? 0001-multi-polish-the-HTTP_1_1_REQUIRED-check.patch

(also inlined here for easier browsing)

From 2ed48bdb07fe3c5bc27a343126e68754bfd2267e Mon Sep 17 00:00:00 2001
From: Daniel Stenberg <daniel@haxx.se>
Date: Fri, 7 Dec 2018 17:22:29 +0100
Subject: [PATCH] multi: polish the HTTP_1_1_REQUIRED check

to avoid strstr() and depending of contents in the error message buffer
---
 lib/http2.c | 8 ++++++++
 lib/http2.h | 4 ++++
 lib/multi.c | 5 +++--
 3 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/lib/http2.c b/lib/http2.c
index c33bee3eb..a61d8c240 100644
--- a/lib/http2.c
+++ b/lib/http2.c
@@ -2365,10 +2365,18 @@ void Curl_http2_cleanup_dependencies(struct Curl_easy *data)
 
   if(data->set.stream_depends_on)
     Curl_http2_remove_child(data->set.stream_depends_on, data);
 }
 
+/* Only call this function for a transfer that already got a HTTP/2
+   CURLE_HTTP2_STREAM error! */
+bool Curl_h2_http_1_1_error(struct connectdata *conn)
+{
+  struct http_conn *httpc = &conn->proto.httpc;
+  return (httpc->error_code == NGHTTP2_HTTP_1_1_REQUIRED);
+}
+
 #else /* !USE_NGHTTP2 */
 
 /* Satisfy external references even if http2 is not compiled in. */
 
 #define CURL_DISABLE_TYPECHECK
diff --git a/lib/http2.h b/lib/http2.h
index 4492ec211..e1d2a79c7 100644
--- a/lib/http2.h
+++ b/lib/http2.h
@@ -57,10 +57,13 @@ CURLcode Curl_http2_add_child(struct Curl_easy *parent,
                               struct Curl_easy *child,
                               bool exclusive);
 void Curl_http2_remove_child(struct Curl_easy *parent,
                              struct Curl_easy *child);
 void Curl_http2_cleanup_dependencies(struct Curl_easy *data);
+
+/* returns true of the HTTP/2 stream error was HTTP_1_1_REQUIRED */
+bool Curl_h2_http_1_1_error(struct connectdata *conn);
 #else /* USE_NGHTTP2 */
 #define Curl_http2_init(x) CURLE_UNSUPPORTED_PROTOCOL
 #define Curl_http2_send_request(x) CURLE_UNSUPPORTED_PROTOCOL
 #define Curl_http2_request_upgrade(x,y) CURLE_UNSUPPORTED_PROTOCOL
 #define Curl_http2_setup(x) CURLE_UNSUPPORTED_PROTOCOL
@@ -72,8 +75,9 @@ void Curl_http2_cleanup_dependencies(struct Curl_easy *data);
 #define Curl_http2_done(x,y)
 #define Curl_http2_done_sending(x)
 #define Curl_http2_add_child(x, y, z)
 #define Curl_http2_remove_child(x, y)
 #define Curl_http2_cleanup_dependencies(x)
+#define Curl_h2_http_1_1_error(x) 0
 #endif
 
 #endif /* HEADER_CURL_HTTP2_H */
diff --git a/lib/multi.c b/lib/multi.c
index d36edfbd9..15fdff069 100644
--- a/lib/multi.c
+++ b/lib/multi.c
@@ -44,10 +44,11 @@
 #include "pipeline.h"
 #include "sigpipe.h"
 #include "vtls/vtls.h"
 #include "connect.h"
 #include "http_proxy.h"
+#include "http2.h"
 /* The last 3 #include files should be in this order */
 #include "curl_printf.h"
 #include "curl_memory.h"
 #include "memdebug.h"
 
@@ -1952,12 +1953,12 @@ static CURLMcode multi_runsingle(struct Curl_multi *multi,
           /* if we are to retry, set the result to OK and consider the
              request as done */
           result = CURLE_OK;
           done = TRUE;
         }
-      } else if(CURLE_HTTP2_STREAM == result && data->set.errorbuffer &&
-                strstr(data->set.errorbuffer, "HTTP_1_1_REQUIRED")) {
+      } else if((CURLE_HTTP2_STREAM == result) &&
+                Curl_h2_http_1_1_error(data->easy_conn)) {
         CURLcode ret = Curl_retry_request(data->easy_conn, &newurl);
 
         infof(data, "Forcing HTTP/1.1 for NTLM");
         data->set.httpversion = CURL_HTTP_VERSION_1_1;
 
-- 
2.19.2

@dscho
Copy link
Contributor Author

dscho commented Dec 7, 2018

warning: else after closing brace on same line (BRACEELSE)

Oops... ;-)

What do you think of this little extra dance to avoid the strstr in the error message? 0001-multi-polish-the-HTTP_1_1_REQUIRED-check.patch

That looks very good, thank you!

Will you make the changes and complete this PR?

@dscho
Copy link
Contributor Author

dscho commented Dec 7, 2018

Will you make the changes and complete this PR?

Actually, let me give it a try.

dscho added a commit to dscho/curl that referenced this pull request Dec 7, 2018
This is a companion patch to cbea2fd (NTLM: force the connection to
HTTP/1.1, 2018-12-06): with NTLM, we can switch to HTTP/1.1
preemptively. However, with other (Negotiate) authentication it is not
clear to this developer whether there is a way to make it work with
HTTP/2, so let's try HTTP/2 first and fall back in case we encounter the
error HTTP_1_1_REQUIRED.

Note: we will still keep the NTLM workaround, as it avoids an extra
round trip.

Daniel Stenberg helped a lot with this patch, in particular by
suggesting to introduce the Curl_h2_http_1_1_error() function.

Closes curl#3349

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
This is a companion patch to cbea2fd (NTLM: force the connection to
HTTP/1.1, 2018-12-06): with NTLM, we can switch to HTTP/1.1
preemptively. However, with other (Negotiate) authentication it is not
clear to this developer whether there is a way to make it work with
HTTP/2, so let's try HTTP/2 first and fall back in case we encounter the
error HTTP_1_1_REQUIRED.

Note: we will still keep the NTLM workaround, as it avoids an extra
round trip.

Daniel Stenberg helped a lot with this patch, in particular by
suggesting to introduce the Curl_h2_http_1_1_error() function.

Closes curl#3349

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
@dscho
Copy link
Contributor Author

dscho commented Dec 7, 2018

Actually, let me give it a try.

Done. And tested (again, reverting that NTLM patch and doing the git ls-remote <server> dance as before).

dscho added a commit to git-for-windows/MINGW-packages that referenced this pull request Dec 7, 2018
NTLM is incompatible with HTTP/2, and most (or all) of Kerberos, too. In
such cases, it is a problem that cURL v7.62.0 changed the default to use
HTTP/2 for https:// connections whenever servers indicate support for
it.

This is a backport of curl/curl#3345 and
curl/curl#3349, so that Git for Windows v2.20.0
will have those fixes.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
@bagder
Copy link
Member

bagder commented Dec 8, 2018

Thanks!

@bagder bagder closed this in d997aa0 Dec 8, 2018
@dscho dscho deleted the http_1_1_required-fallback branch December 8, 2018 19:32
@dscho
Copy link
Contributor Author

dscho commented Dec 8, 2018

Thank you for your help, and I am really happy that this bug has been addressed (and that those support requests have been averted...)!

@lock lock bot locked as resolved and limited conversation to collaborators Mar 8, 2019
dscho added a commit to dscho/curl that referenced this pull request Mar 27, 2019
This is a companion patch to cbea2fd (NTLM: force the connection to
HTTP/1.1, 2018-12-06): with NTLM, we can switch to HTTP/1.1
preemptively. However, with other (Negotiate) authentication it is not
clear to this developer whether there is a way to make it work with
HTTP/2, so let's try HTTP/2 first and fall back in case we encounter the
error HTTP_1_1_REQUIRED.

Note: we will still keep the NTLM workaround, as it avoids an extra
round trip.

Daniel Stenberg helped a lot with this patch, in particular by
suggesting to introduce the Curl_h2_http_1_1_error() function.

Closes curl#3349

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants