tool_operate: CURLOPT_PROXY_CAINFO defaults to cacert #1257

Open
wants to merge 2 commits into
from

Projects

None yet

4 participants

@jay
Member
jay commented Feb 11, 2017

If config->proxy_cacert is unset then default CURLOPT_PROXY_CAINFO to
config->cacert.

Closes #xxxx


Currently CURLOPT_PROXY_CAPATH falls back on regular capath if there's no proxy specific capath, but CURLOPT_PROXY_CAINFO does not fall back on regular cacert if there's no proxy specific cacert. I wonder if that was an oversight? This change fixes it. On Windows it allows me to specify an HTTPS proxy without having to use --proxy-cacert, since the cacert bundle is detected automatically (FindWin32CACert), but there is no automatic detection for proxy cacert, so now it will default to cacert.

An alternative would be automatic detection for proxy cacert as well, that way if a user did something like --cacert specific-host.crt curl would still call FindWin32CACert to find the default bundle for the proxy cacert, which would be more robust. Thoughts?

/cc @rousskov

@mention-bot

@jay, thanks for your PR! By analyzing the history of the files in this pull request, we identified @yangtse, @bagder and @captain-caveman2k to be potential reviewers.

@rousskov
Contributor

@jay, I suspect (but cannot verify) that our thinking was that a single CA is unlikely to validate both the proxy and the origin server because most proxy certificates do not come from well-known CAs (but I am sure there are many situations where a single CA would work). My speculation is based primarily on the following sentence in our HTTPS commit documentation:

All --proxy-foo options are independent from their --foo counterparts,
except --proxy-crlfile which defaults to --crlfile and --proxy-capath
which defaults to --capath.

Both CRL file and CA path point to a "collection" of "often sharable" things while CA certificate is a single entity that is often server-specific.

Please do not misinterpret my comment as a negative vote. I have no standing and am just trying to guess what our original (and possibly wrong) thinking was.

@bagder
Member
bagder commented Feb 11, 2017 edited

I'm casually for this change.

I don't think there's a lot to lose by defaulting to using the same CA bundle. If users happen to share them for servers and proxies, it will make slightly more connections work at first try. If they have separate bundles, they need to set both options anyway and this change has no negative effect.

@jay
Member
jay commented Feb 11, 2017

Both CRL file and CA path point to a "collection" of "often sharable" things while CA certificate is a single entity that is often server-specific.

@rousskov Ok. CA bundles are a pretty common use of cacert, so I would argue we have the same collection-style considerations.

I'm casually for this change.

@bagder I agree with the comments you made however since it seems like you're hedging a bit I can leave this open to gather more feedback.

jay added some commits Feb 16, 2017
@jay jay tool_operate: default --proxy-cacert to --cacert
- Change --proxy-cacert behavior so if it's not set and --cacert is set
  use that instead, which is the same as what we already do for
  --proxy-{capath,crlfile}.

- Change when --proxy-{cacert,capath,crlfile} defaults to its regular
  non-proxy counterpart not to be a fatal error if the option was not
  found or built in.

- Change docs for --proxy-{cacert,capath,crlfile} to explain that those
  options if not set will use their regular non-proxy counterparts if
  set.

- Change CURLOPT_PROXY_CAPATH to return CURLE_NOT_BUILT_IN if the option
  is not supported, which is the same as what we already do for
  CURLOPT_CAPATH.

- Change docs for CURLOPT_{CAPATH,PROXY_CAPATH} to explain that
  CURLE_NOT_BUILT_IN is returned when the option is not supported.

- Check the return code after setting STRING_SSL_CAPATH_PROXY.

- Set preferred CA bundle from compile-time CURL_CA_BUNDLE for
  STRING_SSL_CAFILE_PROXY, which is the same as what we already do for
  compile-time CURL_CA_PATH with STRING_SSL_CAPATH_PROXY.

Closes #xxxx (s/xxxx/1257/)
7ea192a
@jay jay tool_operate: on second thought...
SQUASHME

- Change --proxy-{cacert,capath,crlfile} not to default to their regular
  non-proxy counterparts, since it's counterintuitive.

The CA path and certificate bundle may be set at compile-time, and if so
will be the default values in libcurl for those options. However by
defaulting for example --proxy-cacert if unset to --cacert if set that
would override the compile-time certificate file, which may not be what
was intended.

What is more intuitive is curl --cacert foo.pem and expecting that
since --proxy-cacert is unused then libcurl will use the default
certificate bundle set at compile time.
793b6a3
@jay
Member
jay commented Feb 16, 2017

I'm having second thoughts about this after a review and fixing some problems in the code. Though on Windows the curl tool will auto-detect the certificate bundle it is more common on Linux for example for libcurl to use a compile-time string. So consider this for a moment:

curl --cacert foo.pem --proxy https://bar https://qux

On Linux --proxy-cacert since it's unset I think should use the compile-time value in libcurl, if any.

I've updated this PR basically doing a 180. The first commit in the branch shows the problems found and fixed, and does what was originally proposed by defaulting --proxy-cacert to --cacert. However the second commit changes it so that --proxy-{cacert,capath,crlfile} do not use their regular non-proxy counterpart. Since this was not documented and very early on, I think we could do this. Recall as I mentioned above if there's a compile-time value that is what would be used. Windows is a little more complicated since its detection is in the curl tool, and I have not addressed that yet.

Please take a look at both commits and tell me what you think.

@jay
Member
jay commented Feb 21, 2017

@bagder Even if we don't reach a consensus by 7.53 on the whole thing I think perhaps we could all agree on the compile time CURL_CA_BUNDLE string set as the default for STRING_SSL_CAFILE_PROXY, see this line.

@bagder
Member
bagder commented Feb 21, 2017

Yes, that seems like a totally sensible thing to do by default. And with a sensible compile-time default, I agree that there's less of a need for fancy tool-side things.

@jay jay added a commit that referenced this pull request Feb 22, 2017
@jay jay url: Improve CURLOPT_PROXY_CAPATH error handling
- Change CURLOPT_PROXY_CAPATH to return CURLE_NOT_BUILT_IN if the option
  is not supported, which is the same as what we already do for
  CURLOPT_CAPATH.

- Change the curl tool to handle CURLOPT_PROXY_CAPATH error
  CURLE_NOT_BUILT_IN as a warning instead of as an error, which is the
  same as what we already do for CURLOPT_CAPATH.

- Fix CAPATH docs to show that CURLE_NOT_BUILT_IN is returned when the
  respective CAPATH option is not supported by the SSL library.

Ref: #1257
b259646
@jay jay added a commit that referenced this pull request Feb 23, 2017
@jay jay url: Default the CA proxy bundle location to CURL_CA_BUNDLE
If the compile-time CURL_CA_BUNDLE location is defined use it as the
default value for the proxy CA bundle location, which is the same as
what we already do for the regular CA bundle location.

Ref: #1257
93cce24
@jay
Member
jay commented Feb 23, 2017

I have landed the fix to default to CURL_CA_BUNDLE. Remaining issues are in the tool for the next feature window. I see one of two routes for consistency:

  1. Stop --proxy-capath and crl from using their non-proxy counterpart options as defaults. Also (Windows): Same auto-detection in tool for Windows ca cert bundle in PATH for --proxy-cacert as we do now for --cacert.
  2. or make --proxy-cacert default to --cacert to be consistent with the other two.

Just to recap we have room to move here because this isn't documented, and I had originally proposed 2 but now think 1 is better.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment