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

Only ever pick CURLAUTH_BEARER if we *have* a Bearer token #2754

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@dscho
Contributor

dscho commented Jul 17, 2018

The Bearer authentication was added to cURL 7.61.0, but there is a
problem: if CURLAUTH_ANY is selected, and the server supports multiple
authentication methods including the Bearer method, we strongly prefer
that latter method (only CURLAUTH_NEGOTIATE beats it), and if the Bearer
authentication fails, we will never even try to attempt any other
method.

This is particularly unfortunate when we already know that we do not
have any Bearer token to work with.

Such a scenario happens e.g. when using Git to push to Visual Studio
Team Services (which supports Basic and Bearer authentication among
other methods) and specifying the Personal Access Token directly in the
URL (this aproach is frequently taken by automated builds).

Let's make sure that we have a Bearer token to work with before we add
it to our available authentication methods.

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

@dscho

This comment has been minimized.

Contributor

dscho commented Jul 17, 2018

Note: while this is a bit pressing of a problem for me because it prevents the Git for Windows SDK from being updated (and I only realized this yesterday, as I forgot to configure the notification mails properly, while the automated build to update the SDK has apparently been broken for over a week...), I realize that this bug might be more complex than meets the eye: I assume that the oauth_bearer is set already when we parse the server's authentication methods, but I do not actually know for sure...

@dscho

This comment has been minimized.

Contributor

dscho commented Jul 17, 2018

@bagder is this a valid approach?

@bagder

bagder approved these changes Jul 17, 2018

It makes perfect sense to me!

@dscho

This comment has been minimized.

Contributor

dscho commented Jul 18, 2018

It makes perfect sense to me!

My only concern is: do you see any way that the Bearer authentication should be added to the avail bit field before some caller might add the token? If there is any chance for that right now, my patch would break it.

lib/http.c Outdated
@@ -925,7 +925,7 @@ CURLcode Curl_http_input_auth(struct connectdata *conn, bool proxy,
}
}
else
if(checkprefix("Bearer", auth)) {
if(conn->oauth_bearer && checkprefix("Bearer", auth)) {

This comment has been minimized.

@dscho

dscho Jul 18, 2018

Contributor

This is probably the wrong spot to do this. I think pickoneauth() is the correct place. Let me test this.

This comment has been minimized.

@dscho

dscho Jul 18, 2018

Contributor

I just force-pushed an update.

The reason: I was not comfortable meddling with the avail bit-field, as the server did report it as available authentication method, and users of libcurl will see that information.

Instead, the patch now really only verifies at pickoneauth() time that we pick the Bearer method only when we have a Bearer token.

Note: for the Basic authentication method, IIRC it is totally legitimate to pick it without valid credentials, and then kick that result back to the caller, so that credentials can be provided interactively. But for the Bearer authentication, I think it makes no sense to ask for a token interactively, like, ever: you either have it, or you don't. And if you don't have a token, let's not try to authenticate with it ;-)

This comment has been minimized.

@dscho

dscho Jul 18, 2018

Contributor

Oh yes, and of course I verified that the force-pushed patch also fixes the symptom on my side.

@dscho dscho force-pushed the dscho:bearer-auth-requires-oauth-bearer branch 2 times, most recently from d526cb3 to a1e36d2 Jul 18, 2018

@dscho

This comment has been minimized.

Contributor

dscho commented Jul 18, 2018

@bagder could you re-review, please? 😊

@dscho

This comment has been minimized.

Contributor

dscho commented Jul 18, 2018

could you re-review, please?

(I ask because I would like to back-port this fix, to fix Git for Windows snapshot builds for the use case I outlined in the commit message.)

@dscho dscho force-pushed the dscho:bearer-auth-requires-oauth-bearer branch from a1e36d2 to 46ea3c8 Jul 18, 2018

@dscho

This comment has been minimized.

Contributor

dscho commented Jul 19, 2018

(I ask because I would like to back-port this fix, to fix Git for Windows snapshot builds for the use case I outlined in the commit message.)

And I would hate to back-port a change that needs to be modified before entering curl's master branch...

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

curl: avoid picking Bearer authentication without a Bearer token
This is necessary e.g. to allow fetching from/pushing to VSTS via
https://<user>:<pass>@<host>/<path> (which is frequently used in
automated builds, as VSTS does offer Bearer authentication, and cURL
would fail to connect because it would not fall back to Basic
authentication.

This patch has been contributed to the cURL project as

	curl/curl#2754

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
lib/http.c Outdated
if(!pickhost)
data->state.authproblem = TRUE;
}
if(conn->bits.proxy_user_passwd &&
((data->req.httpcode == 407) ||
(conn->bits.authneg && data->req.httpcode < 300))) {
pickproxy = pickoneauth(&data->state.authproxy);
pickproxy = pickoneauth(&data->state.authproxy, authmask);

This comment has been minimized.

@bagder

bagder Jul 19, 2018

Member

The oauth_bearer is however only documented and implemented to work for host auth... Which really begs the question what libcurl should do if this is asked to be used for proxy auth. Maybe this should just always disable CURLAUTH_BEARER for proxy since there's no option to set the bearer string for it?

This comment has been minimized.

@dscho

dscho Jul 19, 2018

Contributor

Right, and this code is guarded behind a test for proxy_user_password, too...

lib/http.c Outdated
@@ -517,14 +521,14 @@ CURLcode Curl_http_auth_act(struct connectdata *conn)
if(conn->bits.user_passwd &&

This comment has been minimized.

@dscho

dscho Jul 19, 2018

Contributor

@bagder Oh, BTW, shouldn't this be if((conn->bits.user_passwd || conn->oauth_bearer) && now?

This comment has been minimized.

@bagder

bagder Jul 20, 2018

Member

ah, yes you're right, I believe that would probably be what oauth-bearer users would want!

This comment has been minimized.

@dscho

dscho Jul 20, 2018

Contributor

I'll make it so, in a separate commit, but for convenience in the same PR.

This comment has been minimized.

@dscho

dscho Jul 20, 2018

Contributor

Done: 9453aac

@dscho dscho force-pushed the dscho:bearer-auth-requires-oauth-bearer branch from 46ea3c8 to 9d40f70 Jul 19, 2018

Only ever pick CURLAUTH_BEARER if we *have* a Bearer token
The Bearer authentication was added to cURL 7.61.0, but there is a
problem: if CURLAUTH_ANY is selected, and the server supports multiple
authentication methods including the Bearer method, we strongly prefer
that latter method (only CURLAUTH_NEGOTIATE beats it), and if the Bearer
authentication fails, we will never even try to attempt any other
method.

This is particularly unfortunate when we already know that we do not
have any Bearer token to work with.

Such a scenario happens e.g. when using Git to push to Visual Studio
Team Services (which supports Basic and Bearer authentication among
other methods) and specifying the Personal Access Token directly in the
URL (this aproach is frequently taken by automated builds).

Let's make sure that we have a Bearer token to work with before we
select the Bearer authentication among the available authentication
methods.

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

@dscho dscho force-pushed the dscho:bearer-auth-requires-oauth-bearer branch from 9d40f70 to cd4011f Jul 20, 2018

@dscho

This comment has been minimized.

Contributor

dscho commented Jul 20, 2018

What is this Travis error about ?!?!

E: Unable to locate package clang-6.0
Pick Bearer authentication whenever a token is available
So far, the code tries to pick an authentication method only if
user/password credentials are available, which is not the case for
Bearer authentictation...

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

This comment has been minimized.

Contributor

dscho commented Jul 20, 2018

What is this Travis error about ?!?!

E: Unable to locate package clang-6.0

Seems to have been intermittent...

@bagder

This comment has been minimized.

Member

bagder commented Jul 20, 2018

Seems to have been intermittent...

Yes, unfortunately we have a few like that. I try to spot and restart those when they occur...

@dscho

This comment has been minimized.

Contributor

dscho commented Jul 21, 2018

Seems to have been intermittent...

Yes, unfortunately we have a few like that.

Any idea what causes it?

@dscho

This comment has been minimized.

Contributor

dscho commented Jul 23, 2018

@bagder Oh, and: what should I do, still, to get this merged? I got a report on Friday (and root cause confirmed yesterday) that this very same issue happened to a non-Windows based Git user (where I cannot easily work around the bug by releasing a new Git for Windows version with a patched cURL)...

@bagder

This comment has been minimized.

Member

bagder commented Jul 23, 2018

I will merge this asap. I'm slower than usual a few weeks while I'm on vacation...

@bagder

This comment has been minimized.

Member

bagder commented Jul 24, 2018

Thanks!

@bagder bagder closed this in df57b43 Jul 24, 2018

bagder added a commit that referenced this pull request Jul 24, 2018

auth: pick Bearer authentication whenever a token is available
So far, the code tries to pick an authentication method only if
user/password credentials are available, which is not the case for
Bearer authentictation...

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

This comment has been minimized.

Contributor

dscho commented Jul 24, 2018

I'm slower than usual a few weeks while I'm on vacation...

Oy, I am sorry! I could have easily waited until the end of your vacation... mortified

@dscho dscho deleted the dscho:bearer-auth-requires-oauth-bearer branch Jul 24, 2018

xquery added a commit to xquery/curl that referenced this pull request Aug 9, 2018

auth: only ever pick CURLAUTH_BEARER if we *have* a Bearer token
The Bearer authentication was added to cURL 7.61.0, but there is a
problem: if CURLAUTH_ANY is selected, and the server supports multiple
authentication methods including the Bearer method, we strongly prefer
that latter method (only CURLAUTH_NEGOTIATE beats it), and if the Bearer
authentication fails, we will never even try to attempt any other
method.

This is particularly unfortunate when we already know that we do not
have any Bearer token to work with.

Such a scenario happens e.g. when using Git to push to Visual Studio
Team Services (which supports Basic and Bearer authentication among
other methods) and specifying the Personal Access Token directly in the
URL (this aproach is frequently taken by automated builds).

Let's make sure that we have a Bearer token to work with before we
select the Bearer authentication among the available authentication
methods.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Closes curl#2754

xquery added a commit to xquery/curl that referenced this pull request Aug 9, 2018

auth: pick Bearer authentication whenever a token is available
So far, the code tries to pick an authentication method only if
user/password credentials are available, which is not the case for
Bearer authentictation...

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Closes curl#2754

falconindy added a commit to falconindy/curl that referenced this pull request Sep 10, 2018

auth: only ever pick CURLAUTH_BEARER if we *have* a Bearer token
The Bearer authentication was added to cURL 7.61.0, but there is a
problem: if CURLAUTH_ANY is selected, and the server supports multiple
authentication methods including the Bearer method, we strongly prefer
that latter method (only CURLAUTH_NEGOTIATE beats it), and if the Bearer
authentication fails, we will never even try to attempt any other
method.

This is particularly unfortunate when we already know that we do not
have any Bearer token to work with.

Such a scenario happens e.g. when using Git to push to Visual Studio
Team Services (which supports Basic and Bearer authentication among
other methods) and specifying the Personal Access Token directly in the
URL (this aproach is frequently taken by automated builds).

Let's make sure that we have a Bearer token to work with before we
select the Bearer authentication among the available authentication
methods.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Closes curl#2754

falconindy added a commit to falconindy/curl that referenced this pull request Sep 10, 2018

auth: pick Bearer authentication whenever a token is available
So far, the code tries to pick an authentication method only if
user/password credentials are available, which is not the case for
Bearer authentictation...

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Closes curl#2754

@lock lock bot locked as resolved and limited conversation to collaborators Oct 22, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.