support CAfile in memory #6109

gvollant · 2020-10-21T08:29:21Z

Adds CURLOPT_CAINFO_PEM and CURLOPT_PROXY_CAINFO_PEM that let applications provide the CA bundle to libcurl as a PEM formatted in-memory buffer.

bagder · 2020-10-21T08:36:44Z

It'd be helpful if you provided a description in this PR so that we can tell what this PR is!

gvollant · 2020-10-21T09:24:06Z

I added on my fork https://github.com/gvollant/curl a rebased version of #4679
I invite @moparisthebest on my curl fork. He made the real work

I just wanted a rebased version of this PR to test it against current curl master

Validation : 397e11b [397e11b]
Parents : 4506607
Auteur : moparisthebest admin@moparisthebest.com
Date : vendredi 3 avril 2020 00:32:42
Auteur : moparisthebest
Date de validation : samedi 4 avril 2020 00:31:23
Add CURLOPT_CAINFO_PEM

original description:
This is a first go at bundling cacert.pem into the curl binary, which I plan to use for the static-curl binaries.

It works for openssl right now, but I don't actually expect you want a ~19k line cacert.h file in source control, so I'm looking for guidance on how you might accept a feature like this.

Options I've thought of:

Leave it as-is except CACERT_PEM_LEN set to 0 and the array empty, then if someone wants to hard-code a few, they run the curl+xxd command before compilation. This does make the CACERT_PEM_LEN check happen at runtime.
Option 1, but where the include/used code is wrapped in a #ifdef BUNDLE_CACERT or similar, not sure if this would allow the configure script to generate the file automatically with a --bundle-cacert=/path/to/cacert.pem option or so
Directly embedding the binary cacert.pem into the executable like this which was based on this, presumably gated by a configure flag+ifdef again, but I have no idea how sane and/or portable this is.

@JCMais want work on this PR

gvollant · 2020-10-21T09:27:39Z

@moparisthebest I also invited you on my curl fork git repo

bagder · 2020-10-21T09:36:21Z

I understand that and I think it's all good. I just think a PR submitted to the porject should have a description if title isn't detailed enough, so that we all can read and understand what it is about without having to click around and read up on older closed previous pull-requests with their own individual histories.

JCMais · 2020-10-22T17:24:07Z

What I'm most interested in about this PR is the CURLOPT_CAINFO_PEM option, which is already implemented, not so much about the other features.

If we can get this merged and then work later on the remaining parts to get the ca file bundled in the binary directly, it would be better.

macOS build is failing on the brew install step, which seems unrelated to this PR.

bagder · 2020-10-22T17:32:27Z

The github actions CI jobs seem to have gone nuts and "brew install" fails almost all the time the last few days... :-/

nadrolinux · 2020-11-14T00:53:06Z

Do you know when this feature may be finished?

gvollant · 2020-11-15T14:35:17Z

It seem unsucessful check are same than other recent PR

gvollant · 2020-11-17T09:06:44Z

@bagder I believe the test error are same than other current PR ?

bagder · 2020-11-17T11:25:45Z

I'm sorry, I'm still waiting for a description of this PR. Please update.

JCMais · 2020-11-17T14:46:40Z

@gvollant I think that what @bagder means is that you should put these in the PR description, not as a comment.

gvollant · 2020-11-18T09:27:55Z

@gvollant I think that what @bagder means is that you should put these in the PR description, not as a comment.

Ok, I updated the first comment

gvollant · 2020-11-21T09:12:56Z

@moparisthebest @JCMais @nadrolinux I suggest you test this rebased version

bagder · 2020-12-06T22:19:51Z

There are still questions in the description. Is this then still work in progress or even actually a draft?

gvollant · 2020-12-06T22:23:08Z

for me it is a draft

gvollant · 2020-12-06T22:23:34Z

your review is welcome

ghost · 2021-02-17T19:01:00Z

Congratulations 🎉. DeepCode analyzed your code in 10.511 seconds and we found no issues. Enjoy a moment of no bugs ☀️.

👉 View analysis in DeepCode’s Dashboard | Configure the bot

gvollant · 2021-02-17T19:01:55Z

Retry DeepCode

gvollant · 2021-02-18T07:36:40Z

@bagder hello, I have no idea about the "DeepCode failed to analyze this pull request" message...

bagder · 2021-02-18T09:00:13Z

That's just deepcode being silly. Ignore it.

bagder

The commit message must explain the change it brings.

bagder · 2021-02-18T09:02:01Z

docs/libcurl/opts/CURLOPT_CAINFO_PEM.3

+or more certificates to verify the HTTPS server with.
+
+If \fICURLOPT_SSL_VERIFYPEER(3)\fP is zero and you avoid verifying the
+server's certificate, \fICURLOPT_CAINFO(3)\fP need not even indicate an


You meant it to say CURLOPT_CAINFO_PEM here? The text talks about "accessible file" and I don't understand why this is present for this option that doesn't even work with files?

I just say now CURLOPT_CAINFO_PEM is not needed

lib/urldata.h

lib/vtls/openssl.c

bagder · 2021-02-18T09:17:38Z

lib/urldata.h

@@ -254,6 +254,7 @@ struct ssl_primary_config {
  char *pinned_key;
  struct curl_blob *cert_blob;
  char *curves;          /* list of curves to use */
+  char *ca_file_pem;


Why call this 'file' when it isn't a file at all? It points to a buffer holding PEM formatting, right?

replaced all ca_file_pem by ca_info_pem

bagder · 2021-02-18T09:18:56Z

lib/vtls/openssl.c

@@ -3025,6 +3081,15 @@ static CURLcode ossl_connect_step1(struct Curl_easy *data,
      infof(data, "error importing windows ca store, continuing anyway\n");
  }
 #endif
+  if(ca_file_pem && load_cacert_from_memory(backend->ctx, ca_file_pem)) {


load_cacert_from_memory() returns a CURLcode but here you ignore that and just assumes the error is always due to badfile. Isn't it better to use the passed back return code?

I rewrote this line and return the result of load_cacert_from_memory

docs/libcurl/opts/CURLOPT_CAINFO_PEM.3

bagder · 2021-02-18T09:21:06Z

docs/libcurl/opts/CURLOPT_CAINFO_PEM.3

+
+
+The application does not have to keep the string around after setting this
+option.


This description doesn't say (yet), but I got curious: what happens if a user sets both *CAINFO and *CAINFO_PEM ? Does one of them override the other or how does it work? I think we need to explain.

load_cacert_from_memory just add certificate. I moved it after loading *CAINFO, so both are loaded.

Then please mention in the man page that this provided PEM data is used in addition to any other CA certs loaded with other option(s).

gvollant · 2021-02-23T08:20:08Z

@jay @bagder After thinking, there is pro and cons using a string or a curl_blob

For string:

current PR use string, so we don't need rewrite it. More chance include in next curl revision
string are more easy to use from no-C curl binding

for curl_blob

there is the no_copy option, so if user create 10 curl connexion object at same time, the string is not duplicated
if user call a decompress/decrypt code which give him the PEM string without '\0' terminator, he can call curl_blob without need to copy on another buffer

jay · 2021-02-24T23:08:50Z

lib/vtls/openssl.c

+
+    /* if we didn't end up importing anything, treat that as an error */
+    if(count > 0)
+      result = CURLE_OK;


I find this comment slightly misleading since the return code of the import functions is not checked. I don't think it should fail if there was nothing to import in the certificate, and I wonder if it should fail at all since other certificates may be read in. Maybe make it always a warning instead of just in the insecure case. Interested in what others think about this.

Here's another thing, every time an X509_store_add_ is called it increases the reference count, so what if it's called twice don't you have to free two objects x509 and crl?

"X509_STORE_add_cert() and X509_STORE_add_crl() add the respective object to the X509_STORE's local storage. Untrusted objects should not be added in this way. The added object's reference count is incremented by one, hence the caller retains ownership of the object and needs to free it when it is no longer needed."

Edit: Nevermind I think I misinterpreted it. We actually do want the objects to have an increased reference count since they are now part of the x509 store, the reference count having later been decremented by X509_INFO_free

I find this comment slightly misleading since the return code of the import functions is not checked.
This was true on first version, but now load_cacert_from_memory value is checked (as requester by @bagder ).

But I think we must remove this test and don't fail if there is nothing to import.

ok, I removed the test

jay · 2021-02-24T23:23:30Z

for curl_blob

CURLOPT_CAINFO_BLOB would be consistent with some of the other ssl cert blob options. Also those blob options allow ASN1 binary format as well, I think, even if they are not documented that way.

bagder · 2021-02-25T15:27:28Z

docs/libcurl/opts/CURLOPT_CAINFO_PEM.3

+
+
+The application does not have to keep the string around after setting this
+option.


Then please mention in the man page that this provided PEM data is used in addition to any other CA certs loaded with other option(s).

bagder · 2021-02-25T16:34:51Z

lib/vtls/openssl.c

+        return result;
+      }
+      /* Only warning if no certificate verification is required. */
+      infof(data, "error setting certificate file, continuing anyway\n");


It didn't "set" a file though, did it? It failed to import a CA cert from the given PEM buffer, right?

Also, is that really a reason for failing the connection? What if a CA bundle on file is also used?

If the function failed due to out of memory I think it should fail no matter what

bagder · 2021-02-25T16:36:30Z

lib/vtls/openssl.c

+        ++count;
+      }
+      if(itmp->crl) {
+        X509_STORE_add_crl(cts, itmp->crl);


So the PEM buffer can also provide a CRL! Should be mentioned in the docs, right?

bagder · 2021-02-25T16:43:00Z

for curl_blob
* there is the no_copy option, so if user create 10 curl connexion object at same time, the string is not duplicated

I think this is an excellent point that I hadn't though of before! What a difficult decision... But I'll confess I now maybe lean towards a blob! 😨

gvollant · 2021-02-25T22:42:57Z

I opened another PR #6662 with a blob version.

When decision will be made between CURLOPT_CAINFO_PEM and CURLOPT_CAINFO_BLOB , one of these PR will need to be closed

gvollant · 2021-02-26T07:48:41Z

My option is that #6662 is better.

…Y_CAINFO_PEM Let applications provide the CA bundle to libcurl as a PEM formatted in-memory buffer. Rebased branch bundled_cacert from moparisthebest Validation : 397e11b [397e11b] Parents : 4506607 Auteur : moparisthebest <admin@moparisthebest.com> Date : vendredi 3 avril 2020 00:32:42 Auteur : moparisthebest Date de validation : samedi 4 avril 2020 00:31:23 Add CURLOPT_CAINFO_PEM

gvollant · 2021-03-06T06:54:18Z

Jay worked on new #6662, so I close this PR

- New options CURLOPT_CAINFO_BLOB and CURLOPT_PROXY_CAINFO_BLOB to specify in-memory PEM certificates for OpenSSL, Schannel (Windows) and Secure Transport (Apple) SSL backends. Prior to this change PEM certificates could only be imported from a file and not from memory. Co-authored-by: moparisthebest@users.noreply.github.com Ref: #4679 Ref: #5677 Ref: #6109 Closes #6662

gvollant · 2021-05-05T18:12:53Z

#6662 is now merged with CA as blob

gvollant mentioned this pull request Oct 21, 2020

Rebased version of [WIP] Bundled cacert #4679 #5677

Closed

gvollant force-pushed the gv_bundled_cacert branch from 2d3d2c9 to a36417c Compare October 21, 2020 08:32

bagder changed the title ~~rebased branch bundled_cacert from moparisthebest like #4679 #5677~~ support CAfile in memory Oct 21, 2020

bagder added needs-info TLS labels Oct 24, 2020

gvollant force-pushed the gv_bundled_cacert branch from a36417c to 7b9b21a Compare October 27, 2020 20:38

gvollant force-pushed the gv_bundled_cacert branch from 7b9b21a to 285ae72 Compare November 7, 2020 08:01

gvollant force-pushed the gv_bundled_cacert branch 3 times, most recently from ff88b0d to fadb293 Compare November 14, 2020 20:24

gvollant force-pushed the gv_bundled_cacert branch from fadb293 to dbc3e1d Compare November 20, 2020 14:22

gvollant force-pushed the gv_bundled_cacert branch from dbc3e1d to cb1ea34 Compare December 2, 2020 21:21

bagder marked this pull request as draft December 6, 2020 22:23

gvollant force-pushed the gv_bundled_cacert branch from cb1ea34 to aa82cb5 Compare December 9, 2020 12:22

gvollant force-pushed the gv_bundled_cacert branch 2 times, most recently from 8e99b65 to 0bab71e Compare February 11, 2021 07:02

gvollant force-pushed the gv_bundled_cacert branch from 0bab71e to 8121560 Compare February 17, 2021 19:00

bagder reviewed Feb 18, 2021

View reviewed changes

gvollant force-pushed the gv_bundled_cacert branch 5 times, most recently from 8b5d11f to 4cd02dc Compare February 23, 2021 07:41

jay reviewed Feb 24, 2021

View reviewed changes

bagder reviewed Feb 25, 2021

View reviewed changes

gvollant force-pushed the gv_bundled_cacert branch from 4cd02dc to 087d295 Compare February 25, 2021 21:34

gvollant mentioned this pull request Feb 25, 2021

Add support CA bundle in memory with CURLOPT_CAINFO_BLOB and CURLOPT_PROXY_CAINFO_BLOB #6662

Closed

gvollant force-pushed the gv_bundled_cacert branch from 087d295 to 275e573 Compare February 25, 2021 23:04

gvollant force-pushed the gv_bundled_cacert branch from b96b8d3 to 9ff2f20 Compare February 26, 2021 13:39

gvollant closed this Mar 6, 2021

gvollant mentioned this pull request Apr 23, 2021

Add support CA bundle in memory with CURLOPT_CAINFO_BLOB and CURLOPT_PROXY_CAINFO_BLOB #6940

Closed



		The application does not have to keep the string around after setting this
		option.

support CAfile in memory #6109

support CAfile in memory #6109

Conversation

gvollant commented Oct 21, 2020 • edited by bagder

bagder commented Oct 21, 2020

gvollant commented Oct 21, 2020

gvollant commented Oct 21, 2020

bagder commented Oct 21, 2020

JCMais commented Oct 22, 2020

bagder commented Oct 22, 2020

nadrolinux commented Nov 14, 2020

gvollant commented Nov 15, 2020

gvollant commented Nov 17, 2020

bagder commented Nov 17, 2020

JCMais commented Nov 17, 2020

gvollant commented Nov 18, 2020

gvollant commented Nov 21, 2020

bagder commented Dec 6, 2020

gvollant commented Dec 6, 2020

gvollant commented Dec 6, 2020

ghost commented Feb 17, 2021 • edited by ghost

👉 View analysis in DeepCode’s Dashboard | Configure the bot

gvollant commented Feb 17, 2021

gvollant commented Feb 18, 2021

bagder commented Feb 18, 2021

bagder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gvollant commented Feb 23, 2021 • edited

Choose a reason for hiding this comment

jay Feb 24, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jay commented Feb 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bagder commented Feb 25, 2021

gvollant commented Feb 25, 2021

gvollant commented Feb 26, 2021

gvollant commented Mar 6, 2021

gvollant commented May 5, 2021

gvollant commented Oct 21, 2020 •

edited by bagder

ghost commented Feb 17, 2021 •

edited by ghost

gvollant commented Feb 23, 2021 •

edited

jay Feb 24, 2021 •

edited