Re: [PATCH v5 2/5] http: handle proxy proactive authentication

From: Jeff King <peff@peff.net>
Date: Apr 12, 2012, 20:58 UTC
Message-ID: <20120412205836.GB21018@sigill.intra.peff.net>
In-Reply-To: <7vwr5leyj5.fsf@alter.siamese.dyndns.org>

On Thu, Apr 12, 2012 at 08:54:22AM -0700, Junio C Hamano wrote:

Show 16 quoted lines

> >> I haven't formed an opinion on what the proper solution should be, but
> >> either the credential_from_url() function needs to be updated to accept
> >> the scp style [user@]<host>:<port> argument, or this specific caller
> >> should take the responsibility to do special case the syntax.
> >
> > Well, calling the above "scp" style is a mistake (it is not scp style at
> > all), but the patch to teach the credentail_from_url() to handle the proxy
> > specification may look like this:
> 
> Jeff, do you have an opinion on this?  I briefly wondered if we also want
> to teach the traditional [user@]host:/path/to/repo to this function (it is
> not a URL in RFC1738 sense, but it is in the remote.$name.url sense), but
> because SSH does its own thing interacting with agents, perhaps it may not
> help to teach our credential layer to store and supply cached passphrases
> (or passwords, if the authentication is done by merely sending password
> over the encrypted channel).

My first instinct was "that is not a URL, and should be handled outside this function". In particular, it has no protocol field, and that is an important part of the credential-matching process. It would be up to the caller to supply something sane in the protocol portion. In this case, it would probably be "http" (unless we want to distinguish http proxies from http end-points in the credential store, but I doubt that is useful). But for an ssh-style URL, it would be "ssh". So already the abstraction is a little bit leaky.

As far as parsing ssh goes, I'm not sure that is unambiguous with the proxy syntax. If you see "127.0.0.1:1234", is that short for "http://127.0.0.1:1234/" (what the proxy code wants), or for "ssh://127.0.0.1/1234" (what ssh code would want)? The former seems less odd to me, as it really is just a URL missing some components. The latter is a true alternative syntax.

Like you said, I don't think we will ever want to handle ssh credentials, though. There is already a solution for people who don't want to input ssh passwords, and it is much more advanced and well-supported within the community than what we would provide.

> A safer approach might be to keep externally visible API to this function
> as before, but add another function only for the use of http_proxy and
> friends (whose kosher format is "host:address" without the "<scheme>://"
> part), and call it from the codepath broken by the patch.

I think that would be cleaner conceptually, but it also means reimplementing the user/password-parsing logic. And given that I don't think we want to handle ssh, and that the semantics in your patch are the only sane ones to me, it is not so bad. The caller just needs to be aware of filling in the "protocol" field. Perhaps we could have an alternate version that supplies a "default protocol" parameter. The presence of that parameter would activate this code-path and automatically fill in the protocol field.

-Peff