Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Curl error with Secure Transport and SSL_CERT_DIR variable #492

Closed
bfabiszewski opened this issue Oct 15, 2015 · 5 comments
Closed

Curl error with Secure Transport and SSL_CERT_DIR variable #492

bfabiszewski opened this issue Oct 15, 2015 · 5 comments
Assignees
@bagder
Labels
cmdline tool TLS

Comments

@bfabiszewski
Copy link

Setting CURLOPT_CAPATH option when libcurl is built against darwinssl returns CURLE_NOT_BUILT_IN (8250f93).
In command line curl, CURLOPT_CAPATH option is invoked when user sets --capath argument and also when SSL_CERT_DIR environment variable is set.
In both cases curl quits with CURLE_NOT_BUILT_IN error.

Users may want to use curl with darwinssl support and still have SSL_CERT_DIR variable set. Curl should ignore this environment variable when built against darwinssl instead of quitting with error. Especially that the error message is not very informative in this case: "A requested feature, protocol or option was not found built-in in this libcurl due to a build-time decision." Users may even don’t realise they have environment variable set or that it is the cause of the error.

Also I wonder whether --capath command line option shouldn't be disabled in this case.
@bagder bagder self-assigned this Oct 16, 2015
@bagder
Copy link
Member

bagder commented Oct 16, 2015

I wonder whether --capath command line option shouldn't be disabled in this case.

The tool code doesn't know the capabilities of the underlying TLS backend so it is hard to make it act different depending on that.

I think we should just make sure setting CURLOPT_CAPATH doesn't cause the tool to exit with an error if it fails. It should probably instead output some information about it not working.

@bagder
Copy link
Member

bagder commented Oct 17, 2015

@bfabiszewski, what do you think about a patch like this:

diff --git a/src/tool_operate.c b/src/tool_operate.c
index 41a71dd..bf5649a 100644
--- a/src/tool_operate.c
+++ b/src/tool_operate.c
@@ -1014,12 +1014,17 @@ static CURLcode operate_do(struct GlobalConfig *global,
                         config->hostpubmd5);
         }

         if(config->cacert)
           my_setopt_str(curl, CURLOPT_CAINFO, config->cacert);
-        if(config->capath)
-          my_setopt_str(curl, CURLOPT_CAPATH, config->capath);
+        if(config->capath) {
+          result = res_setopt_str(curl, CURLOPT_CAPATH, config->capath);
+          if(result == CURLE_NOT_BUILT_IN)
+            warnf(config->global, "--capath is not supported by libcurl!\n");
+          else if(result)
+            goto show_error;
+        }
         if(config->crlfile)
           my_setopt_str(curl, CURLOPT_CRLFILE, config->crlfile);

         if(config->pinnedpubkey)
           my_setopt_str(curl, CURLOPT_PINNEDPUBLICKEY, config->pinnedpubkey);

@bfabiszewski
Copy link
Author

Nice solution!

My only concern is that the warning message is misleading. In case of SSL_CERT_DIR variable set, users may be warned about the option (--capath) they didn't use.

A workaround could be to set a flag to differentiate between CA path being set by command line option and environment variable. Then we could just rise a warning about ignoring SSL_CERT_DIR variable.
In case of --capath standard error message would be presented about feature "not found built-in", which would be consistent with other backend dependent options. Man page for --capath option just needs to be adjusted accordingly with "This is currently only implemented in…" line, as in case of other backend dependent features.

diff --git a/src/tool_operate.c b/src/tool_operate.c
index 41a71dd..2e5061b 100644
--- a/src/tool_operate.c
+++ b/src/tool_operate.c
@@ -239,6 +239,7 @@ static CURLcode operate_do(struct GlobalConfig *global,
    * We support the environment variable thing for non-Windows platforms
    * too. Just for the sake of it.
    */
+  bool capath_from_env = false;
   if(!config->cacert &&
      !config->capath &&
      !config->insecure_ok) {
@@ -263,6 +264,7 @@ static CURLcode operate_do(struct GlobalConfig *global,
           result = CURLE_OUT_OF_MEMORY;
           goto quit_curl;
         }
+        capath_from_env = true;
       }
       else {
         env = curlx_getenv("SSL_CERT_FILE");
@@ -1016,8 +1018,13 @@ static CURLcode operate_do(struct GlobalConfig *global,

         if(config->cacert)
           my_setopt_str(curl, CURLOPT_CAINFO, config->cacert);
-        if(config->capath)
-          my_setopt_str(curl, CURLOPT_CAPATH, config->capath);
+        if(config->capath) {
+          result = res_setopt_str(curl, CURLOPT_CAPATH, config->capath);
+          if(result == CURLE_NOT_BUILT_IN && capath_from_env)
+            warnf(config->global, "ignoring SSL_CERT_DIR environment variable, not supported by libcurl!\n");
+          else if(result)
+            goto show_error;
+        }
         if(config->crlfile)
           my_setopt_str(curl, CURLOPT_CRLFILE, config->crlfile);

@bagder bagder closed this as completed in ab86007 Mar 28, 2016
@bagder
Copy link
Member

bagder commented Mar 28, 2016

Thanks, it took forever but now a fix similar to this was merged!

@bfabiszewski
Copy link
Author

Thanks!

@layus layus mentioned this issue Aug 23, 2016
Closed
@lock lock bot locked as resolved and limited conversation to collaborators May 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@bagder bagder

Labels
cmdline tool TLS
Milestone
No milestone
Development

No branches or pull requests
2 participants
@bagder @bfabiszewski