Fix proxy connection reuse with basic-auth #1350
Conversation
|
@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. |
|
I don't suppose this clears up 'NTLM auth to server when Negotiate is used with a proxy is broken in libcurl 7.51.0' |
|
@jay, that mail thread got me looking at this code indeed ;-) |
|
Ok. Even if this is a regression I wonder if it should be fixed. Like I saw that example you gave And I thought well it's going to connect a second time and use |
|
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. |
|
It will of course use bob:b for the second request, so that's ok. |
|
Also note that we re-use the connection in the equivalent server authentication with basic-auth, like: |
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 |
My second change is only for the case where no user is given in the second request.
Sure. |
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>
|
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. |
|
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... |
|
Yea, but as of now this bug is hidden by the other bug so it makes it easier to address. |
|
Thanks, just landed. I modified the commit slightly by enabling connection-monitor in the test to tell if the connection is actually reused.
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? |
|
Thanks for reviewing and landing this!
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. |
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