Re: [PATCH v3] config: add support for http.<url>.* settings

From: Jeff King <peff@peff.net>
Date: Jul 12, 2013, 09:59 UTC
Message-ID: <20130712095923.GA4695@sigill.intra.peff.net>
In-Reply-To: <9e7edfbc83a7284615af4ca0de39c1b@f74d39fa044aa309eaea14b9f57fe79>

On Thu, Jul 11, 2013 at 02:50:03PM -0700, Kyle J. McKay wrote:

Show 19 quoted lines

> The <url> value is considered a match to a url if the <url> value
> is either an exact match or a prefix of the url which ends on a
> path component boundary ('/').  So "https://example.com/test" will
> match "https://example.com/test" and "https://example.com/test/too"
> but not "https://example.com/testextra".
> 
> Longer matches take precedence over shorter matches with
> environment variable settings taking precedence over all.
> 
> With this configuration:
> 
> [http]
>         useragent = other-agent
>         noEPSV = true
> [http "https://example.com"]
>         useragent = example-agent
>         sslVerify = false
> [http "https://example.com/path"]
>         useragent = path-agent

I like the general direction you are going, and especially the path prefix matching is something I had always meant to implement for the credential code.

A few comments on the implementation:

Show 25 quoted lines

> +enum http_option_type {
> +	opt_post_buffer = 0,
> +	opt_min_sessions,
> +#ifdef USE_CURL_MULTI
> +	opt_max_requests,
> +#endif
> +	opt_ssl_verify,
> +	opt_ssl_try,
> +	opt_ssl_cert,
> +#if LIBCURL_VERSION_NUM >= 0x070903
> +	opt_ssl_key,
> +#endif
> +#if LIBCURL_VERSION_NUM >= 0x070908
> +	opt_ssl_capath,
> +#endif
> +	opt_ssl_cainfo,
> +	opt_low_speed,
> +	opt_low_time,
> +	opt_no_epsv,
> +	opt_http_proxy,
> +	opt_cookie_file,
> +	opt_user_agent,
> +	opt_passwd_req,
> +	opt_max
> +};

We usually put enum fields in ALL_CAPS to mark them as constants (though you can find few exceptions in the code).

> +static size_t http_options_url_match_prefix(const char *url,
> +					    const char *url_prefix,
> +					    size_t url_prefix_len)
> +{

It looks like you're matching the URLs as raw strings, and I don't see any canonicalization going on. What happens if I have "https://example.com/foo+bar" in my config, but then I visit "https://example.comfoo%20bar"?

Or what about optional components? If I have "https://example.com" in my config, but I am visiting "https://peff@example.com/", shouldn't it match? The config spec is more general than my specific URL; you would not want it to match in the opposite direction, though.

I think you would want to break down the URL into fields, canonicalize each field by undoing any encoding, and then compare the broken-down URLs, as credential_match does (adding in your prefix/boundary matching to the path instead of a straight string comparison).

I think you could mostly reuse the code there by doing:

  1. Create a "struct url" that contains the broken-down form; "struct
     credential" would contain a url.

  2. Pull out credential_from_url into "int url_parse(struct url *,
     const char *)".

  3. Pull out credential_match into "int url_match(struct url *, struct
     url *)".

  4. Parse and compare URLs from "http.$URL.*" during the config read
     (similar to credential_config_callback).

That does make your by-length ordering impossible, but I don't think you can do it in the general case. If I have:

  [http "http://peff@example.com"] foo = 1
  [http "http://example.com:8080"] foo = 2

and I visit "http://peff@example.com:8080", which one is the winner? I don't think there is an unambiguous definition. I'd suggest instead just using the usual "last one wins" strategy that our config uses. It has the unfortunate case that:

  [http "http://example.com"]
     foo = 1
  [http]
     foo = 2

will always choose http.foo=2, but I don't think that is a big problem in practice. You often only set one or the other, and in the cases where you want to have a non-standard default and then override it with another host-specific case, the "last one wins" rule makes it simple to explain that you need to specify keys in most-general to most-specific order.

>  static int http_options(const char *var, const char *value, void *cb)
>  {
> -	if (!strcmp("http.sslverify", var)) {
> +	const char *url = (const char *)cb;

No need to cast from void, this isn't C++. :)

The rest of the http_options() changes look like what I'd expect.

Show 6 quoted lines

> @@ -344,7 +479,7 @@ void http_init(struct remote *remote, const char *url, int proactive_auth)
>  
>  	http_is_verbose = 0;
>  
> -	git_config(http_options, NULL);
> +	git_config(http_options, (void *)url);

No need to cast again. :)

So this is the URL that we got on the command line. Which means that if we get a redirect, we will not re-examine the options. I think that's OK; we do not even _see_ the redirect, as it happens internally within curl. The credential code has the same problem, but it is not a security issue because curl clears the credentials on redirect.

However, it may be worth mentioning in the documentation that the config options operate on the URL you give git, _not_ necessarily on the actual URL you are hitting at any given moment (the gitcredentials(7) page should probably make the same distinction).

-Peff