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

KNOWN_BUGS cleanup #9871

Closed
wants to merge 8 commits into from
Closed

KNOWN_BUGS cleanup #9871

wants to merge 8 commits into from

Conversation

bagder
Copy link
Member

@bagder bagder commented Nov 8, 2022

A range of items mentioned in the document that I think are not actually bugs and thus do not belong therein.

bagder added 8 commits Nov 8, 2022
1.2 Multiple methods in a single WWW-Authenticate: header

This is not considered a bug anymore but a restriction and one that we
keep because we have NEVER gotten this reported by users in the wild and
because of this I consider this a fringe edge case we don't need to
support.
1.6 Unnecessary close when 401 received waiting for 100

This is not a bug, but possibly an optimization that *can* be done.
1.7 Deflate error after all content was received

This is not a curl bug. This happens due to broken servers.
2.1 CURLINFO_SSL_VERIFYRESULT has limited support

This is not a bug. This is just the nature of the implementation.
2.2 DER in keychain

This is not a bug.
@bagder bagder closed this in e46d388 Nov 9, 2022
@bagder bagder deleted the bagder/not-bugs branch Nov 9, 2022
@micheljung
Copy link

micheljung commented Nov 22, 2022

@bagder
Copy link
Member Author

bagder commented Nov 22, 2022

Ah wow I clearly forgot about that: so wrong of me. It has been reported, but it is still such a very rare thing that I do not consider it a bug.

@micheljung
Copy link

micheljung commented Nov 22, 2022

okay, so whether something is a bug or not is not decided by if it works according to the RFC that even has an explicit example for this but by how many people report it? 😀

But sure, curl is free to sell it as "just not supported" :)

It cost us many man hours and forced us to come up with an ugly workaround which is still in production today just to deal with this non-bug. I'd rather have spent a fraction of that sponsoring you or someone else to fix it. Unfortunately, that was not my decision to make :(

@bagder
Copy link
Member Author

bagder commented Nov 22, 2022

Sort of, yes. It is now re-labeled as "a known implementation restriction", not a bug. It does not serve the project well to keep items in the known-bugs list that none of us intends to fix and that none of the project developers actually considers a bug.

We need to stay pragmatic, we can never read or act on RFC specifications by their exact words only. curl does not live in a vacuum.

This does not rule out that a future version of curl could have this auth header handling improved.

@jay
Copy link
Member

jay commented Nov 23, 2022

It's tricky because the comma in WWW-Authenticate is used in the scheme as well as to separate schemes, which means you have to parse each scheme in order to know when the next scheme starts. You can see the code in Curl_http_input_auth does loop for multiple schemes but I guess it would need to be improved.

curl/lib/http.c

Lines 1001 to 1139 in cd95ee9

/*
* Here we check if we want the specific single authentication (using ==) and
* if we do, we initiate usage of it.
*
* If the provided authentication is wanted as one out of several accepted
* types (using &), we OR this authentication type to the authavail
* variable.
*
* Note:
*
* ->picked is first set to the 'want' value (one or more bits) before the
* request is sent, and then it is again set _after_ all response 401/407
* headers have been received but then only to a single preferred method
* (bit).
*/
while(*auth) {
#ifdef USE_SPNEGO
if(checkprefix("Negotiate", auth) && is_valid_auth_separator(auth[9])) {
if((authp->avail & CURLAUTH_NEGOTIATE) ||
Curl_auth_is_spnego_supported()) {
*availp |= CURLAUTH_NEGOTIATE;
authp->avail |= CURLAUTH_NEGOTIATE;
if(authp->picked == CURLAUTH_NEGOTIATE) {
CURLcode result = Curl_input_negotiate(data, conn, proxy, auth);
if(!result) {
DEBUGASSERT(!data->req.newurl);
data->req.newurl = strdup(data->state.url);
if(!data->req.newurl)
return CURLE_OUT_OF_MEMORY;
data->state.authproblem = FALSE;
/* we received a GSS auth token and we dealt with it fine */
*negstate = GSS_AUTHRECV;
}
else
data->state.authproblem = TRUE;
}
}
}
else
#endif
#ifdef USE_NTLM
/* NTLM support requires the SSL crypto libs */
if(checkprefix("NTLM", auth) && is_valid_auth_separator(auth[4])) {
if((authp->avail & CURLAUTH_NTLM) ||
(authp->avail & CURLAUTH_NTLM_WB) ||
Curl_auth_is_ntlm_supported()) {
*availp |= CURLAUTH_NTLM;
authp->avail |= CURLAUTH_NTLM;
if(authp->picked == CURLAUTH_NTLM ||
authp->picked == CURLAUTH_NTLM_WB) {
/* NTLM authentication is picked and activated */
CURLcode result = Curl_input_ntlm(data, proxy, auth);
if(!result) {
data->state.authproblem = FALSE;
#ifdef NTLM_WB_ENABLED
if(authp->picked == CURLAUTH_NTLM_WB) {
*availp &= ~CURLAUTH_NTLM;
authp->avail &= ~CURLAUTH_NTLM;
*availp |= CURLAUTH_NTLM_WB;
authp->avail |= CURLAUTH_NTLM_WB;
result = Curl_input_ntlm_wb(data, conn, proxy, auth);
if(result) {
infof(data, "Authentication problem. Ignoring this.");
data->state.authproblem = TRUE;
}
}
#endif
}
else {
infof(data, "Authentication problem. Ignoring this.");
data->state.authproblem = TRUE;
}
}
}
}
else
#endif
#ifndef CURL_DISABLE_CRYPTO_AUTH
if(checkprefix("Digest", auth) && is_valid_auth_separator(auth[6])) {
if((authp->avail & CURLAUTH_DIGEST) != 0)
infof(data, "Ignoring duplicate digest auth header.");
else if(Curl_auth_is_digest_supported()) {
CURLcode result;
*availp |= CURLAUTH_DIGEST;
authp->avail |= CURLAUTH_DIGEST;
/* We call this function on input Digest headers even if Digest
* authentication isn't activated yet, as we need to store the
* incoming data from this header in case we are going to use
* Digest */
result = Curl_input_digest(data, proxy, auth);
if(result) {
infof(data, "Authentication problem. Ignoring this.");
data->state.authproblem = TRUE;
}
}
}
else
#endif
if(checkprefix("Basic", auth) &&
is_valid_auth_separator(auth[5])) {
*availp |= CURLAUTH_BASIC;
authp->avail |= CURLAUTH_BASIC;
if(authp->picked == CURLAUTH_BASIC) {
/* We asked for Basic authentication but got a 40X back
anyway, which basically means our name+password isn't
valid. */
authp->avail = CURLAUTH_NONE;
infof(data, "Authentication problem. Ignoring this.");
data->state.authproblem = TRUE;
}
}
else
if(checkprefix("Bearer", auth) &&
is_valid_auth_separator(auth[6])) {
*availp |= CURLAUTH_BEARER;
authp->avail |= CURLAUTH_BEARER;
if(authp->picked == CURLAUTH_BEARER) {
/* We asked for Bearer authentication but got a 40X back
anyway, which basically means our token isn't valid. */
authp->avail = CURLAUTH_NONE;
infof(data, "Authentication problem. Ignoring this.");
data->state.authproblem = TRUE;
}
}
/* there may be multiple methods on one line, so keep reading */
while(*auth && *auth != ',') /* read up to the next comma */
auth++;
if(*auth == ',') /* if we're on a comma, skip it */
auth++;
while(*auth && ISSPACE(*auth))
auth++;
}

Take your example

curl -kv -L --negotiate -u : -b ~/cookie.txt -c ~/cookie.txt http://localhost:8081/negotiate

I would think in this case curl sends an Authorization for the first request, then if it gets this in reply:

WWW-Authenticate: Negotiate, Basic realm="TM1"

It assumes failure (data->state.authproblem = TRUE). Interesting it passes on ", Basic realm="TM1" as the negotiate data, which can't possibly be right according to rfc 4559. That causes Curl_input_negotiate to return a bad encoding error instead of a login denied error.

I think he has a point that this should be in known bugs even if we aren't working on it. This is a known bug that libcurl does not handle multiple auth on the same line well, even though it may try (it does loop around, I don't know what good that does in cases with multiple options per scheme)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants