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

NTLM: force the connection to HTTP/1.1 #3345

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
2 participants
@dscho
Copy link
Contributor

commented Dec 6, 2018

This addresses #3341.

I took a slightly different approach than suggested in that ticket: instead of trying NTLM via HTTP/2 and then imitating a HTTP redirect when we get HTTP_1_1_REQUIRED, this PR introduces the change where we detect that situation preemptively, i.e. before trying NTLM we already ensure that we're on HTTP/1.1.

NTLM: force the connection to HTTP/1.1
Since v7.62.0, cURL tries to use HTTP/2 whenever the server announces
the capability. However, NTLM authentication only works with HTTP/1.1,
and will likely remain in that boat (for details, see
https://docs.microsoft.com/en-us/iis/get-started/whats-new-in-iis-10/http2-on-iis#when-is-http2-not-supported).

When we just found out that we want to use NTLM, and when the current
connection runs in HTTP/2 mode, let's force the connection to be closed
and to be re-opened using HTTP/1.1.

This fixes #3341.

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

This comment has been minimized.

Copy link
Contributor Author

commented Dec 6, 2018

TBH I am not quite sure why test 408 fails in Travis, it looks like it times out? This cannot be caused by the changes in this PR, can it?

@dscho

This comment has been minimized.

Copy link
Contributor Author

commented Dec 6, 2018

@bagder is this approach okay? Can I set the HTTP version this way or do you insist on a flag (and if so, which struct should hold it)? Do we need the same for CURLAUTH_NEGOTIATE (some documentation I found suggests so, but I am far from sure)?

@bagder

This comment has been minimized.

Copy link
Member

commented Dec 7, 2018

I think this looks clean and understandable when reading the code which is all I need =). Have you verified against a real server that this works?

I too suspect we need the same for negotiate as well, since it can basically imply NTLM (but not always) - but I''m not at all an expert on negotiate and I believe we have more of this style of issues with negotiate in the code: that we treat NTLM as the connection-based authentication scheme. I think I'm suggesting that we can leave negotiate out of this fix for now until we understand that side of the planet a little better.

@dscho

This comment has been minimized.

Copy link
Contributor Author

commented Dec 7, 2018

Have you verified against a real server that this works?

Yes, I have. That's why it took so long, and that's why I realized in the end that I need to call connclose(); Otherwise, cURL would not try to reconnect but instead reuse the connection (which keeps being HTTP/2).

I too suspect we need the same for negotiate as well

Hmm. My original plan was to intercept HTTP_1_1_REQUIRED and do the same dance in that case. I guess this would still be necessary, and would help Negotiate, without the need to downgrade to HTTP/1.1 pre-emptively (at the cost of an extra round trip if we do need to downgrade to HTTP/1.1).

How does that sound?

I think I'm suggesting that we can leave negotiate out of this fix for now until we understand that side of the planet a little better.

Sure, we could do that, too. But I already have the setup to test (actually, my excellent colleague @jeschu1 has this setup and lets me use it). So I will probably work on that, but I guess this is worth a separate PR, what do you think?

@bagder

This comment has been minimized.

Copy link
Member

commented Dec 7, 2018

My original plan was to intercept HTTP_1_1_REQUIRED and do the same dance in that case. I guess this would still be necessary, and would help Negotiate, without the need to downgrade to HTTP/1.1 pre-emptively (at the cost of an extra round trip if we do need to downgrade to HTTP/1.1).

I think that's a really good idea, as that would then also cover other potential cases where this is returned that we haven't yet considered or experienced!

I guess this is worth a separate PR, what do you think?

I agree. Would you like to see this PR merged first/independently or would you rather wait and see where this next PR ends up?

@dscho

This comment has been minimized.

Copy link
Contributor Author

commented Dec 7, 2018

Would you like to see this PR merged first

Yes, please ;-)

@bagder

This comment has been minimized.

Copy link
Member

commented Dec 7, 2018

Thanks!

@bagder bagder closed this in cbea2fd Dec 7, 2018

dscho added a commit to git-for-windows/MINGW-packages that referenced this pull request Dec 7, 2018

curl: downgrade from HTTP/2 to HTTP/1.1 when needed (e.g. for NTLM)
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>

@lock lock bot locked as resolved and limited conversation to collaborators Mar 7, 2019

dscho added a commit to dscho/curl that referenced this pull request Mar 27, 2019

NTLM: force the connection to HTTP/1.1
Since v7.62.0, cURL tries to use HTTP/2 whenever the server announces
the capability. However, NTLM authentication only works with HTTP/1.1,
and will likely remain in that boat (for details, see
https://docs.microsoft.com/en-us/iis/get-started/whats-new-in-iis-10/http2-on-iis#when-is-http2-not-supported).

When we just found out that we want to use NTLM, and when the current
connection runs in HTTP/2 mode, let's force the connection to be closed
and to be re-opened using HTTP/1.1.

Fixes curl#3341.
Closes curl#3345

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.
You can’t perform that action at this time.