Implement FTP auth through proxy

Implement FTP auth through proxy

BUG=11227
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=192872

[Paweł Hajdan Jr., 2013-03-26 00:07:19]

Early version, please take a look - I don't consider it really finished.

[mmenke, 2013-03-26 00:24:17]

On 2013/03/26 00:07:19, Paweł Hajdan Jr. wrote:
> Early version, please take a look - I don't consider it really finished.

Due to perf, may not get to this for a couple days.

[Paweł Hajdan Jr., 2013-03-26 18:44:48]

+Eric, could you take a look?

[mmenke, 2013-03-28 16:08:34]

I'll try to get back to you today, though I'll need to read over a fair bit of
the auth code to figure out just how this fits into everything else, so may not
be able to comment intelligently until tomorrow.

[mmenke, 2013-03-28 18:34:52]

One high level comment and some test suggestions.  I don't think we should be
using the ftp cache for proxy credentials.  Seems to me like it could either
result in sending proxies FTP credentials or FTP servers proxy credentials, both
of which are bad (Admittedly, we don't support encryption on either level for
FTP requests, but still doesn't seem like a great idea).

I intend to continue looking at the CL without waiting for a response, but
thought I'd get these to you now.

https://codereview.chromium.org/12582012/diff/4001/net/url_request/url_reques...
File net/url_request/url_request_ftp_job.cc (right):

https://codereview.chromium.org/12582012/diff/4001/net/url_request/url_reques...
net/url_request/url_request_ftp_job.cc:409: FtpAuthCache::Entry* cached_auth =
ftp_auth_cache_->Lookup(origin);
This seems wrong for proxy auth.  Looks like if a proxy asks for auth, and we
have cached credentials site, we'd try using the site credentials for
authenticating with the proxy.

https://codereview.chromium.org/12582012/diff/4001/net/url_request/url_reques...
File net/url_request/url_request_ftp_job_unittest.cc (right):

https://codereview.chromium.org/12582012/diff/4001/net/url_request/url_reques...
net/url_request/url_request_ftp_job_unittest.cc:277: 
Other test suggestions:

1)  After authenticating with a proxy through an http request, the cached
credentials are used for an ftp request.
2)  The other way around.
3)  If we have cached credentials for an ftp site, we don't send them in
response to a 407 request from the server.

https://codereview.chromium.org/12582012/diff/4001/net/url_request/url_reques...
net/url_request/url_request_ftp_job_unittest.cc:425: "realm=\"MyRealm1\"\r\n"),
I'd like to see a different password for the second request.

[Paweł Hajdan Jr., 2013-03-29 17:59:28]

Change updated.

Note that mocking a real FTP transaction is pretty heavyweight, so I'd like to
avoid that.

I went for simplicity, and use FTP auth cache only for real (no proxy) FTP
transactions. Note that when browsing FTP through proxy, the credentials are
cached anyway, so you only need to enter the password once.

I think it is fine for the user to re-enter server password when switching
between proxy and no proxy.

[mmenke1, 2013-03-29 18:23:49]

On 2013/03/29 17:59:28, Paweł Hajdan Jr. wrote:
> Change updated.
> 
> Note that mocking a real FTP transaction is pretty heavyweight, so I'd like to
> avoid that.

Sounds reasonable to me.  My main concern is separating out proxy / FTP
passwords, and think we can adequately probe that without a simulating an FPT
transaction.

> I went for simplicity, and use FTP auth cache only for real (no proxy) FTP
> transactions. Note that when browsing FTP through proxy, the credentials are
> cached anyway, so you only need to enter the password once.
> 
> I think it is fine for the user to re-enter server password when switching
> between proxy and no proxy.

I agree that re-entering the server password when the proxy changes isn't a huge
deal, I think re-entering the HTTP proxy password when going between FTP and
HTTP requests is bad, however.

[Paweł Hajdan Jr., 2013-03-29 20:36:33]

On 2013/03/29 18:23:49, mmenke (incorrect) wrote:
> Sounds reasonable to me.  My main concern is separating out proxy / FTP
> passwords, and think we can adequately probe that without a simulating an FPT
> transaction.

The uploaded patch already does that. I'm just injecting credentials directly to
ftp auth cache to simulate an effect of direct ftp transaction.

> I agree that re-entering the server password when the proxy changes isn't a
huge
> deal, I think re-entering the HTTP proxy password when going between FTP and
> HTTP requests is bad, however.

This is not the case, I have tested it. You only need to enter proxy password
once per browser session.

[mmenke, 2013-03-29 20:47:45]

On 2013/03/29 20:36:33, Paweł Hajdan Jr. wrote:
> On 2013/03/29 18:23:49, mmenke (incorrect) wrote:
> > Sounds reasonable to me.  My main concern is separating out proxy / FTP
> > passwords, and think we can adequately probe that without a simulating an
FPT
> > transaction.
> 
> The uploaded patch already does that. I'm just injecting credentials directly
to
> ftp auth cache to simulate an effect of direct ftp transaction.
> 
> > I agree that re-entering the server password when the proxy changes isn't a
> huge
> > deal, I think re-entering the HTTP proxy password when going between FTP and
> > HTTP requests is bad, however.
> 
> This is not the case, I have tested it. You only need to enter proxy password
> once per browser session.

Great.  Since auth data is cached by the FtpTransaction and doesn't seem to be
cleared, I still don't see anything preventing us from providing the HTTP proxy
auth data to the FTP server when both ask for credentials, though I certainly
could be missing something.

[mmenke, 2013-03-29 20:48:52]

On 2013/03/29 20:47:45, mmenke wrote:
> On 2013/03/29 20:36:33, Paweł Hajdan Jr. wrote:
> > On 2013/03/29 18:23:49, mmenke (incorrect) wrote:
> > > Sounds reasonable to me.  My main concern is separating out proxy / FTP
> > > passwords, and think we can adequately probe that without a simulating an
> FPT
> > > transaction.
> > 
> > The uploaded patch already does that. I'm just injecting credentials
directly
> to
> > ftp auth cache to simulate an effect of direct ftp transaction.
> > 
> > > I agree that re-entering the server password when the proxy changes isn't
a
> > huge
> > > deal, I think re-entering the HTTP proxy password when going between FTP
and
> > > HTTP requests is bad, however.
> > 
> > This is not the case, I have tested it. You only need to enter proxy
password
> > once per browser session.
> 
> Great.  Since auth data is cached by the FtpTransaction and doesn't seem to be
> cleared, I still don't see anything preventing us from providing the HTTP
proxy
> auth data to the FTP server when both ask for credentials, though I certainly
> could be missing something.

Nevermind.

[mmenke, 2013-04-01 14:24:58]

[+asanka]:  Mind taking a look at this?  I'm not terribly familiar with auth.

[asanka, 2013-04-02 17:11:48]

LG, but I'm not very familiar with how HTTP server authentication is used with
FTP over HTTP proxy. Is there a spec somewhere?

I verified that auth caches aren't mixed. It looks like things would be simpler
if URLRequestFtpJob/FtpTransaction would have a similar division of
responsibilities as URLRequestHttpJob/HttpTransaction. I.e. have the FTP
credentials cache be managed below of FtpTransaction layer.

https://codereview.chromium.org/12582012/diff/16002/net/url_request/url_reque...
File net/url_request/url_request_ftp_job.cc (right):

https://codereview.chromium.org/12582012/diff/16002/net/url_request/url_reque...
net/url_request/url_request_ftp_job.cc:237: response_info_ =
http_transaction_->GetResponseInfo();
Minor nit: something like http_response_info_ might help disambiguate this
member from ftp_transaction_->GetResponseInfo(). Then again, response_info_
doesn't seem to be used much.

https://codereview.chromium.org/12582012/diff/16002/net/url_request/url_reque...
net/url_request/url_request_ftp_job.cc:239: if
(response_info_->headers->response_code() == 401 ||
Under which circumstances would you need to handle a 401 here?

[Paweł Hajdan Jr., 2013-04-05 00:03:18]

PTAL

https://codereview.chromium.org/12582012/diff/16002/net/url_request/url_reque...
File net/url_request/url_request_ftp_job.cc (right):

https://codereview.chromium.org/12582012/diff/16002/net/url_request/url_reque...
net/url_request/url_request_ftp_job.cc:237: response_info_ =
http_transaction_->GetResponseInfo();
On 2013/04/02 17:11:48, asanka wrote:
> Minor nit: something like http_response_info_ might help disambiguate this
> member from ftp_transaction_->GetResponseInfo(). Then again, response_info_
> doesn't seem to be used much.

Done.

https://codereview.chromium.org/12582012/diff/16002/net/url_request/url_reque...
net/url_request/url_request_ftp_job.cc:239: if
(response_info_->headers->response_code() == 401 ||
On 2013/04/02 17:11:48, asanka wrote:
> Under which circumstances would you need to handle a 401 here?

When FTP server requires auth, the proxy will issue a 401 response.

[asanka, 2013-04-05 17:27:09]

LGTM

[Paweł Hajdan Jr., 2013-04-08 20:12:47]

Committed patchset #5 manually as r192872 (presubmit successful).