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

Fix proxy connection reuse with basic-auth #1350

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@iboukris
Contributor

iboukris commented Mar 23, 2017

When using basic-auth, connections and proxy connections
can be re-used with different Authorization headers since
it does not authenticate the connection (like NTLM does).

For instance, the below command should re-use the proxy
connection, but it currently doesn't:
curl -v -U alice:a -x http://localhost:8181 http://localhost/
--next -U bob:b -x http://localhost:8181 http://localhost/

This is a regression since refactoring of ConnectionExists()
as part of: cb4e2be

Fix the above by removing the username and password compare
when re-using proxy connection at proxy_info_matches().

However, this fix brings back another bug would make curl
to re-print the old proxy-authorization header of previous
proxy basic-auth connection because it wasn't cleared.

For instance, in the below command the second request should
fail if the proxy requires authentication, but would succeed
after the above fix (and before aforementioned commit):
curl -v -U alice:a -x http://localhost:8181 http://localhost/
--next -x http://localhost:8181 http://localhost/

Fix this by clearing conn->allocptr.proxyuserpwd after use
unconditionally, same as we do for conn->allocptr.userpwd.

Signed-off-by: Isaac Boukris iboukris@gmail.com

@mention-bot

This comment has been minimized.

mention-bot commented Mar 23, 2017

@frenche, thanks for your PR! By analyzing the history of the files in this pull request, we identified @bagder, @dfandrich and @captain-caveman2k to be potential reviewers.

@jay

This comment has been minimized.

Member

jay commented Mar 23, 2017

@iboukris

This comment has been minimized.

Contributor

iboukris commented Mar 23, 2017

@jay, that mail thread got me looking at this code indeed ;-)
But these changes are not related to Negotiate, sadly that's a bit harder to solve currently.

@jay

This comment has been minimized.

Member

jay commented Mar 23, 2017

Ok. Even if this is a regression I wonder if it should be fixed. Like I saw that example you gave

curl -v -U alice:a -x http://localhost:8181 http://localhost/
--next -U bob:b -x http://localhost:8181 http://localhost/

And I thought well it's going to connect a second time and use bob:b. To me personally that seems more correct. I actually didn't know we don't didn't do that.

@iboukris

This comment has been minimized.

Contributor

iboukris commented Mar 23, 2017

Why should it reconnect? there us a connection to re-use to this host.

The --next example is useful for me to demonstrate connection re-use, but I think more about library usage which can be anything, and in general we always want to reuse connections unless some connection-oriented authentication types are used like NTLM, and to some extent Negotiate.

@iboukris

This comment has been minimized.

Contributor

iboukris commented Mar 23, 2017

It will of course use bob:b for the second request, so that's ok.

@iboukris

This comment has been minimized.

Contributor

iboukris commented Mar 23, 2017

Also note that we re-use the connection in the equivalent server authentication with basic-auth, like:
curl -v -ualice:a http://localhost/private/
--next -ubob:b http://localhost/private/

@jay

This comment has been minimized.

Member

jay commented Mar 23, 2017

It will of course use bob:b for the second request, so that's ok.

I see what's happening. You changed two things. I was thinking the old credentials would be reused. CI test 540 (HTTP proxy auth Digest multi API re-using connection) is failing can you take a look

@iboukris

This comment has been minimized.

Contributor

iboukris commented Mar 23, 2017

I see what's happening. You changed two things. I was thinking the old credentials would be reused.

My second change is only for the case where no user is given in the second request.
If a user is given it will override anyway.

CI test 540 (HTTP proxy auth Digest multi API re-using connection) is failing can you take a look

Sure.

Fix proxy connection reuse with basic-auth
When using basic-auth, connections and proxy connections
can be re-used with different Authorization headers since
it does not authenticate the connection (like NTLM does).

For instance, the below command should re-use the proxy
connection, but it currently doesn't:
curl -v -U alice:a -x http://localhost:8181 http://localhost/
  --next -U bob:b -x http://localhost:8181 http://localhost/

This is a regression since refactoring of ConnectionExists()
as part of: cb4e2be

Fix the above by removing the username and password compare
when re-using proxy connection at proxy_info_matches().

However, this fix brings back another bug would make curl
to re-print the old proxy-authorization header of previous
proxy basic-auth connection because it wasn't cleared.

For instance, in the below command the second request should
fail if the proxy requires authentication, but would succeed
after the above fix (and before aforementioned commit):
curl -v -U alice:a -x http://localhost:8181 http://localhost/
  --next -x http://localhost:8181 http://localhost/

Fix this by clearing conn->allocptr.proxyuserpwd after use
unconditionally, same as we do for conn->allocptr.userpwd.

Also fix test 540 to not expect digest auth header to be
resent when connection is reused.

Signed-off-by: Isaac Boukris <iboukris@gmail.com>

@iboukris iboukris force-pushed the iboukris:proxy_reuse branch from 337ad47 to b6055a8 Mar 23, 2017

@iboukris

This comment has been minimized.

Contributor

iboukris commented Mar 23, 2017

The test was expecting the digest authorization header to be resent when the connection is reused, which is basically what the fix changed, so I fixed the test to expect another 407 when reusing and only then 200 ok.

@kdudka

This comment has been minimized.

Collaborator

kdudka commented Mar 24, 2017

Commit 24a8359 does something similar for host authentication and it was handled as a security issue (CVE-2015-3236). While fixing it, we decided not to apply the same fix for proxy authentication because it would break test540. Hopefully it is going to be fixed properly this time...

@iboukris

This comment has been minimized.

Contributor

iboukris commented Mar 24, 2017

Yea, but as of now this bug is hidden by the other bug so it makes it easier to address.

@jay jay self-assigned this Mar 27, 2017

@jay jay closed this in 7975d10 Mar 28, 2017

@jay

This comment has been minimized.

Member

jay commented Mar 28, 2017

Thanks, just landed. I modified the commit slightly by enabling connection-monitor in the test to tell if the connection is actually reused.

While fixing it, we decided not to apply the same fix for proxy authentication because it would break test540. Hopefully it is going to be fixed properly this time...

Well what's your definition of proper. As far as I can see the fix here is correct but not ideal, since ideally we'd not have to reauth on the connection if the credentials are the same, but doesn't that put us back with the same problem?

@iboukris

This comment has been minimized.

Contributor

iboukris commented Mar 28, 2017

Thanks for reviewing and landing this!

As far as I can see the fix here is correct but not ideal, since ideally we'd not have to reauth on the connection if the credentials are the same

Not sure I agree, in HTTP the connection is merely a transport layer, and it does not get any more holy after an authenticated request was made over it, so we have to re-authenticate for every request.

@iboukris iboukris deleted the iboukris:proxy_reuse branch Mar 28, 2017

@lock lock bot locked as resolved and limited conversation to collaborators May 23, 2018

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