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

Curl error with Secure Transport and SSL_CERT_DIR variable #492

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

Comments

Projects
None yet
2 participants
@bfabiszewski

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

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Oct 16, 2015

Member

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.

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

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Oct 17, 2015

Member

@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);
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

This comment has been minimized.

Show comment
Hide comment
@bfabiszewski

bfabiszewski Oct 18, 2015

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);

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 in ab86007 Mar 28, 2016

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Mar 28, 2016

Member

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

Member

bagder commented Mar 28, 2016

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

@bfabiszewski

This comment has been minimized.

Show comment
Hide comment

Thanks!

@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.