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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]


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

> >> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Newbies FAQ]     [Linux Kernel Development]     [Free Online Dating]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Free Online Dating]     [Photo]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]     [Linux Resources]

Add to Google