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

http_negotiate: do not close connection until negotiation is completed #3275

Closed
wants to merge 1 commit into from

Conversation

elia-tufarolo
Copy link
Contributor

@elia-tufarolo elia-tufarolo commented Nov 15, 2018

Aim of this PR is to fix the HTTP POST using CURLAUTH_NEGOTIATE.

When connecting to an HTTP URL through a proxy that accepts Negotiation based authorization, libcURL fails to login to the proxy. The Negotiate protocol requires two round trips with the proxy. During the first one the client provides the Negotiate flags and the proxy answers with a Negotiate challenge, returning also HTTP 407. In the second round trip, the client provides the answer to the challenge and, if the proxy accepts it, it returns HTTP 200.
By setting authstatus->done always to true after emitting the Negotiate authorization header, so even after just emitting the header for the first request, libcURL closes the connection after the first round trip. Then it reopens it and executes the second roundtrip, thus answering to the challenge: the proxy, receiving a new connection, loses the context and is unable to associate the challenge response to the original challenge. For this reason it returns again 407.

The change is aimed at setting authstatus->done to true only if the client thinks it is at the last stage of its negotiation, to prevent libcURL from closing the socket while the negotiation is still ongoing. The negotiation can be considered as complete when the final status of the SPNEGO decoding is a completion status (GSS_S_COMPLETE for GSSAPI and SEC_E_OK for SSPI).

Please review.

Aim of this PR is to fix the HTTP POST using CURLAUTH_NEGOTIATE.
Copy link
Member

@bagder bagder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me!

@bagder
Copy link
Member

bagder commented Nov 16, 2018

Thanks!

@bagder bagder closed this in 07ebaf8 Nov 16, 2018
@kdudka
Copy link
Contributor

kdudka commented Dec 14, 2018

This change broke fedkg new-sources, which is a utility that uploads files to Fedora look-aside cache used by Fedora packagers:

https://bugzilla.redhat.com/1659329

I have not had enough time to properly analyze what happened. Just sharing the information we have at this point...

@bagder
Copy link
Member

bagder commented Dec 18, 2018

#3384 also identified this PR as the reason. I think we need to revert this and reconsider this approach. Any thoughts @elia-tufarolo?

@bagder bagder reopened this Dec 18, 2018
@bagder
Copy link
Member

bagder commented Dec 19, 2018

An obvious little problem here is the lack of negotiate test cases in our suite. And I can't reproduce.

@kdudka I presume that tool is using negotiate then?

@kdudka
Copy link
Contributor

kdudka commented Dec 19, 2018

Yes, Fedora uses Kerberos to authenticate users at certain services. Some basic info about their use of Kerberos is available here:

https://fedoraproject.org/wiki/Infrastructure/Kerberos

Sorry, I have not been able to look at this closer yet.

bagder added a commit that referenced this pull request Jan 4, 2019
…completed"

This reverts commit 07ebaf8.

This also reopens PR #3275 which brought the change now reverted.

Fixes #3384
bagder added a commit that referenced this pull request Jan 5, 2019
…completed"

This reverts commit 07ebaf8.

This also reopens PR #3275 which brought the change now reverted.

Fixes #3384
bagder added a commit that referenced this pull request Jan 7, 2019
…completed"

This reverts commit 07ebaf8.

This also reopens PR #3275 which brought the change now reverted.

Fixes #3384
Closes #3439
@MarcelRaad
Copy link
Member

This should be fixed again with #1975, I hope?

@kdudka
Copy link
Contributor

kdudka commented Apr 1, 2019

6c60355 is included curl-7.64.1, which is now in Fedora rawhide, and we have no reports about this problem recurring so far.

@jay
Copy link
Member

jay commented Apr 1, 2019

Fixed until proven otherwise

@jay jay closed this Apr 1, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jun 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

Successfully merging this pull request may close these issues.

None yet

5 participants