Allow building with multiple SSL backends, selecting which one to use at runtime

[dscho]

Git for Windows uses cURL and offers users the choice whether to use OpenSSL or Secure Channel for its HTTPS transport. Unfortunately, this is an install-time option (swapping out binaries because it is a build time option in cURL), when it would be so much better a user experience if this could be a runtime option.

The latest cURL user survey also lists the desire for runtime-configurable SSL backends (and I was not the only one asking for it).

So here is a start.

What it does is this: it moves OpenSSL's and Secure Channel's private data into a union, and then introduces a new SSL backend dubbed "multi" that enables both members of that union, and sets a set of function pointers upon Curl_ssl_init() to the function implementations of the backend configured via the new environment variable CURL_SSL_BACKEND. Supported values are "openssl" and "schannel" for the moment.

In the interest of efficiency, I would like to solicit reviews and advice at this stage, which is already usable as far as Git for Windows' needs are concerned. In particular, I would love to get feedback on these questions:

• Is this the way to go, to move backend-specific stuff into the newly-introduced backend union in struct connectdata? Or would another method of adjusting the backends' code be preferable?
• Would it be okay to start just with OpenSSL and Secure Channel for now (because that is what Git for Windows needs)? As in: could this Pull Request go forward without converting the remaining SSL backends just yet (I am prepared to work on this in the future, of course).
• For the moment, I #undef HAS_ALPN before including the Secure Channel code in the multi backend. That's because my setup (a slightly modified MSYS2 environment) seems to offer ALPN support with OpenSSL, but not with Secure Channel. This would need to be a runtime option, right? Would it be okay to fold that into the newly-introduced curlssl_features enum? What other flags need to go there (I guess HTTPS_PROXY_SUPPORT, at least)?
• Each SSL backend seems to have a ton of #define curlssl_* at the end of its header file, offering backend-specific functions via a unified interface. Should I introduce a new C++-like "virtual table" struct for these functions instead of setting function pointers as I do right now in multi_init()?
• How should I deal with the backend-specific dynamic discovery code in configure? I do not want to duplicate it (--with-openssl would need to perform the same auto-detection of files and features as --with-multissl=openssl[...])...
• What API would you suggest to configure this from C code (I do not want to set the environment variable CURL_SSL_BACKEND in the caller, that looks a little too fragile to me)?

I would love to get lots of good feedback how to develop this topic branch so that it can eventually be integrated into cURL.

[dscho]

Oh, and any idea why Travis fails with:

make[2]: *** No rule to make target `../../src/libcurltool.la', needed by `unit1300'.  Stop.
make[2]: Leaving directory `/home/travis/build/curl/curl/tests/unit'

[MarcelRaad]

Oh, and any idea why Travis fails with:

This is because checksrc.pl fails:

1804./getinfo.c:394:87: warning: Longer than 79 columns (LONGLINE)

... and some more. On Windows, you can run projects\checksrc.bat.

[bagder]

Is this the way to go, to move backend-specific stuff into the newly-introduced backend union in struct connectdata? Or would another method of adjusting the backends' code be preferable?

Sorry, I haven't yet looked at the code so I'll just answer in general terms first: connectdata is the struct for each independent connection so I think it makes sense to make sure that knows which SSL backend is being used for that. I don't think we'll be able to mix/change SSL-backend on an existing connection so if you want to change, you need a new connection. (As a side-note to remember: this needs to be taken into account when re-using connections in multi-ssl mode.)

Would it be okay to start just with OpenSSL and Secure Channel

Sure. But as you say we should probably consider how to make it work with other combos later on. And while the pandora is already out of the box: having more than two...

For the moment, I #undef HAS_ALPN

Yeah, some of the backend build-time stuff clearly needs to be made into run-time conditionals since I suppose there will be a few features that not all backends in the multi-ssl bundle support uniformly!

Each SSL backend seems to have a ton of #define curlssl_* at the end of its header file, offering backend-specific functions via a unified interface. Should I introduce a new C++-like "virtual table" struct for these functions

Yes, sounds like the way forward. Each backend could setup their own structs, similar to protocols set up structs for each protocol. Then we'd end up with a struct pointer to use for doing backend-ssl operations and for multi-ssl we would simply need to be able to use more than one/change the pointer run-time.

What API would you suggest to configure this from C code

Tricky stuff. Can we start with you saying something how you envision this support to be used by application code? With a better understanding for the use case, maybe figuring out the API for it will be easier.

How should I deal with the backend-specific dynamic discovery code in configure

From a user's perspective, I would imagine that just adding SSL-options to configure would be the logical way. Ie both --with-ssl and --with-winssl implies multi-ssl...

[coveralls]

Coverage decreased (-73.8%) to 0.0% when pulling 499e1ff on dscho:ssl-multi-backend into c1dfc8a on curl:master.

[dscho]

Oh, and any idea why Travis fails with:

This is because checksrc.pl fails:

1804./getinfo.c:394:87: warning: Longer than 79 columns (LONGLINE)

Ah, thanks! That is a problem, of course, as the <fieldname> -> backend.openssl.<fieldname> type of changes require a lot of rewrapping.

Should I use a shorter name? Or introduce a macro? If so, what name would you prefer?

[dscho]

I don't think we'll be able to mix/change SSL-backend on an existing connection

Oh... I did not mean to mix SSL backends, sorry, that use case never crossed my mind...

as you say we should probably consider how to make it work with other combos later on. And while the pandora is already out of the box: having more than two...

That is easy: just add the straight-forward support for another SSL backend to multissl.c by #include'ing the source, #undefing the curlssl_* constants, and extending multi_init() to handle the new set of functions.

It will be even easier with the vtable-like approach.

For the moment, I #undef HAS_ALPN

Yeah, some of the backend build-time stuff clearly needs to be made into run-time conditionals since I suppose there will be a few features that not all backends in the multi-ssl bundle support uniformly!

Right. The first commit already converts quite a few, such as have_curlssl_certinfo. So HAS_ALPN would need to be converted, too. Any other flags you can think off?

Related: I saw that some of the SSL backends' headers define quite a few constants that seem to be used only by the SSL backends themselves, e.g. UNISP_NAME in schannel.h, which is only used in schannel.c. Is that intentional? Isn't this bleeding implementation details, i.e. shouldn't those definitions be moved into the respective .c files?

Each SSL backend seems to have a ton of #define curlssl_* at the end of its header file, offering backend-specific functions via a unified interface. Should I introduce a new C++-like "virtual table" struct for these functions

Yes, sounds like the way forward. Each backend could setup their own structs, similar to protocols set up structs for each protocol. Then we'd end up with a struct pointer to use for doing backend-ssl operations and for multi-ssl we would simply need to be able to use more than one/change the pointer run-time.

You mean struct Curl_handler, where e.g. FTP is supported using struct Curl_handler Curl_handler_ftp?

That sounds really clean to me, too.

What API would you suggest to configure this from C code

Tricky stuff. Can we start with you saying something how you envision this support to be used by application code? With a better understanding for the use case, maybe figuring out the API for it will be easier.

Oh, sorry, should have said that earlier.

The use case is git clone/git fetch/git push via HTTPS. It is quite common in enterprise to use custom certificates that are distributed via enterprise management tools, and every developer's machine's Windows Certificate Store will have them. That requires Secure Channel, as OpenSSL (which Git for Windows used for ages) does not know how to access the Windows Certificate Store.

So what I imagine is that either the administrator or the user set a config variable (say, http.sslbackend) that Git will then use to configure cURL prior to every clone/fetch/push operation.

In other words, it needs to be set once per process. Git for Windows would not need the ability to re-configure cURL to use another one after the initial configuration.

How should I deal with the backend-specific dynamic discovery code in configure

From a user's perspective, I would imagine that just adding SSL-options to configure would be the logical way. Ie both --with-ssl and --with-winssl implies multi-ssl...

That's a good idea! I just do not know how to handle the default SSL backend, then (you'd want to use the first configured one, right?).

And that does not even start to handle CMake, with which I ask for a lot more guidance because I know it so little...

[dscho]

Oh, about the HAS_ALPN issue: this is a non-issue, really, and only a sign that I did things a bit too hacky: the HAS_ALPN constant is only ever defined and used in individual .c files. The problem I faced arose only because I #included .c files...

[dscho]

Okay, buckle up...

[dscho]

This is an almost complete rewrite of the topic branch. And it is quite a bit bigger. I tried my best to reorder the patches in a logical manner, and to split the changes into meaningful commits.

This time, I handle all SSL backends (although you have to configure at build time which ones you want to build against explicitly).

This time, ./configure --with-ssl --with-winssl --with-gnutls works as one would expect.

Since I do not have access to all of the SSL backends (only GNU TLS, OpenSSL and Secure Channel), I could not compile-test, unfortunately. The good news is that in practice, it will only matter if anybody tries to build with a different set of multiple backends: in that case, something like 65c442e may be needed (i.e. some #undef tricks in case of clashing definitions in #included header files).

As before, though, the runtime configuration happens via the environment variable CURL_SSL_BACKEND (the easiest test that it worked, I found, is to run CURL_SSL_BACKEND=openssl ./src/curl --version and look at the right part of the first line). @bagder I still would love to hear your ideas how to improve that in a compile-time safe manner.

Also as before, CMake is not handled at all. But then, it looks as if CMake lags behind on SSL backends anyway 😄

Also as before, I have no test coverage (I have not mentioned this before, though). Any guidance?

[coveralls]

Coverage decreased (-73.8%) to 0.0% when pulling bc6e3478123173a12d4c09738ae38be374789786 on dscho:ssl-multi-backend into 922f800 on curl:master.

[dscho]

Oh, i just realized that GitHub juggled the commits around in the PR view, because it orders them by commit date instead of topologically. Please let me know if you prefer strongly for me to rewrite the commits so that their commit dates do not reflect the original commits' dates, but are completely rewritten instead to force GitHub to show them in the correct order.

[coveralls]

Coverage decreased (-0.01%) to 73.796% when pulling 6896623e7f1cda98934a9cd636cb1b96ddaafe23 on dscho:ssl-multi-backend into 922f800 on curl:master.

[coveralls]

Coverage increased (+0.02%) to 73.828% when pulling 6896623e7f1cda98934a9cd636cb1b96ddaafe23 on dscho:ssl-multi-backend into 922f800 on curl:master.

[dscho]

Any way I can help move this forward?

[bagder]

Sorry, it's just me being a bit slow. I haven't forgot about it and I don't mean to intentionally leave you behind. I'll shape up and get some feedback to you on this soonish!

[dscho]

@bagder no worries! You had a tough week, I fully understand. Besides, you are an OSS maintainer, and I understand the pressures of that role, too.

For the record: I would love to get this into Git for Windows soon-ish, even as an experimental, patched version of cURL. To that end, I want to wait a little for the feedback I get, to gage how much of the design needs to change (if only at most cosmetic changes are necessary to get this merged, I would be happy with taking an early version for an experimental/intermediate MinGit for Windows).

So if there is a chance for an early-bird preview of an overall verdict ("design looks good" or "actually, the way this is done needs to be overhauled to use XYZ instead of ABC"), I would be most thankful.

[bagder]

I think the general design looks perfectly fine!

I see you opted to make it possible to change ssl backend by setting an environment variable. Is that how you actually would like it done? For general applications that's a pretty awkward API so there should at least also be an additional option to set somehow, and possibly one of those options could be one that allows the environment variable control.

There are compiler warnings in vtls/vtls.c (I recommend configure's --enable-werror option, and I plan to add it to some CI build soon to make it detect warnings better). Mostly harmless "unused parameter" ones.

Nit: please don't use Curl_multi_ as a prefix, since that's already used a little for functions in lib/multi.c. I suggest maybe Curl_mssl_ or Curl_multissl_ (although the latter one is a bit on the long side).

[dscho]

I think the general design looks perfectly fine!

Thanks! 😄

I see you opted to make it possible to change ssl backend by setting an environment variable. Is that how you actually would like it done? For general applications that's a pretty awkward API so there should at least also be an additional option to set somehow, and possibly one of those options could be one that allows the environment variable control.

I find the environment variable less than ideal myself. However, I have no idea what the proper way is, according to cURL style.

Is there a common way to pre-configure certain defaults before initializing cURL? Or should we have it as an option to be configured via the "curl_easy", safeguarding against setting it multiple times?

There are compiler warnings in vtls/vtls.c (I recommend configure's --enable-werror option, and I plan to add it to some CI build soon to make it detect warnings better). Mostly harmless "unused parameter" ones.

I tried to compile this with --enable-werror --with-ssl --with-gnutls in my Linux VM (GCC 5.4.0) and with --enable-werror --with-ssl --with-gnutls --with-winssl on Windows (GCC 6.3.0), and I did not get any warnings. Would you mind pasting the compile output (or at least the relevant part thereof)?

Nit: please don't use Curl_multi_ as a prefix, since that's already used a little for functions in lib/multi.c. I suggest maybe Curl_mssl_ or Curl_multissl_ (although the latter one is a bit on the long side).

Okay, fair enough. I am not too happy about "multissl" either. I thought of "varssl", but it still does not convey the "configure at run-time" nature of the backend... So far, I am still leaning toward "multissl".

[bagder]

Is there a common way to pre-configure certain defaults before initializing cURL?

I've struggled with figuring out the best way for this init.

There is the curl_global_init() function that is one that triggers the TLS lib init today, but it only has "flags" argument to add stuff to, and that is probably too limited to ask for multi-ssl-init? (or perhaps it'll work out if we reserve one bit for each known TLS lib?)

Otherwise I've been thinking that since multi-ssl is new functionality anyway, we might want to consider adding a totally new way to init this as well.

Or should we have it as an option to be configured via the "curl_easy", safeguarding against setting it multiple times?

That would be cool, but several of the TLS libraries have thread-unsafe inits, so we want them done at the time of the global_init so that applications can have more control of that specific phase.

warnings

Sorry for being brief, I just rebuilt the code with my "usual" configure setup which boils down to:

./configure --disable-shared --enable-debug --enable-maintainer-mode --enable-werror

.. this finds and uses OpenSSL 1.1.0f (only). That then generates this set of warnings:

vtls/vtls.c: In function ‘Curl_none_shutdown’:
vtls/vtls.c:968:44: error: unused parameter ‘conn’ [-Werror=unused-parameter]
 int Curl_none_shutdown(struct connectdata *conn, int sockindex)
                                            ^~~~
vtls/vtls.c:968:54: error: unused parameter ‘sockindex’ [-Werror=unused-parameter]
 int Curl_none_shutdown(struct connectdata *conn, int sockindex)
                                                      ^~~~~~~~~
vtls/vtls.c: In function ‘Curl_none_check_cxn’:
vtls/vtls.c:973:45: error: unused parameter ‘conn’ [-Werror=unused-parameter]
 int Curl_none_check_cxn(struct connectdata *conn)
                                             ^~~~
vtls/vtls.c: In function ‘Curl_none_random’:
vtls/vtls.c:978:45: error: unused parameter ‘data’ [-Werror=unused-parameter]
 CURLcode Curl_none_random(struct Curl_easy *data, unsigned char *entropy,
                                             ^~~~
vtls/vtls.c:978:66: error: unused parameter ‘entropy’ [-Werror=unused-parameter]
 CURLcode Curl_none_random(struct Curl_easy *data, unsigned char *entropy,
                                                                  ^~~~~~~
vtls/vtls.c:979:34: error: unused parameter ‘length’ [-Werror=unused-parameter]
                           size_t length)
                                  ^~~~~~
vtls/vtls.c: In function ‘Curl_none_close_all’:
vtls/vtls.c:984:44: error: unused parameter ‘data’ [-Werror=unused-parameter]
 void Curl_none_close_all(struct Curl_easy *data)
                                            ^~~~
vtls/vtls.c: In function ‘Curl_none_session_free’:
vtls/vtls.c:987:35: error: unused parameter ‘ptr’ [-Werror=unused-parameter]
 void Curl_none_session_free(void *ptr)
                                   ^~~
vtls/vtls.c: In function ‘Curl_none_data_pending’:
vtls/vtls.c:990:55: error: unused parameter ‘conn’ [-Werror=unused-parameter]
 bool Curl_none_data_pending(const struct connectdata *conn, int connindex)
                                                       ^~~~
vtls/vtls.c:990:65: error: unused parameter ‘connindex’ [-Werror=unused-parameter]
 bool Curl_none_data_pending(const struct connectdata *conn, int connindex)
                                                                 ^~~~~~~~~
vtls/vtls.c: In function ‘Curl_none_set_engine’:
vtls/vtls.c:1000:49: error: unused parameter ‘data’ [-Werror=unused-parameter]
 CURLcode Curl_none_set_engine(struct Curl_easy *data, const char *engine)
                                                 ^~~~
vtls/vtls.c:1000:67: error: unused parameter ‘engine’ [-Werror=unused-parameter]
 CURLcode Curl_none_set_engine(struct Curl_easy *data, const char *engine)
                                                                   ^~~~~~
vtls/vtls.c: In function ‘Curl_none_set_engine_default’:
vtls/vtls.c:1005:57: error: unused parameter ‘data’ [-Werror=unused-parameter]
 CURLcode Curl_none_set_engine_default(struct Curl_easy *data)
                                                         ^~~~
vtls/vtls.c: In function ‘Curl_none_engines_list’:
vtls/vtls.c:1010:61: error: unused parameter ‘data’ [-Werror=unused-parameter]
 struct curl_slist *Curl_none_engines_list(struct Curl_easy *data)
                                                             ^~~~
vtls/vtls.c: In function ‘Curl_none_md5sum’:
vtls/vtls.c:1021:57: error: unused parameter ‘md5len’ [-Werror=unused-parameter]
                           unsigned char *md5sum, size_t md5len)
                                                         ^~~~~~
vtls/vtls.c: At top level:
vtls/vtls.c:1071:1: error: ‘static’ is not at beginning of declaration [-Werror=old-style-declaration]
 const static struct Curl_ssl Curl_ssl_multi = {
 ^~~~~
vtls/vtls.c:1105:1: error: ‘static’ is not at beginning of declaration [-Werror=old-style-declaration]
 const static struct Curl_ssl *available_backends[] = {
 ^~~~~
cc1: all warnings being treated as errors

[bagder]

The warnings are actually also in the travis build (I think we should consider doing --enable-werror on those to detect this better):

See: https://travis-ci.org/curl/curl/jobs/247276853#L1360

[bagder]

Oops, yes, we do werror already since fc2e81c =)

[dscho]

Is there a common way to pre-configure certain defaults before initializing cURL?

I've struggled with figuring out the best way for this init.

There is the curl_global_init() function that is one that triggers the TLS lib init today, but it only has "flags" argument to add stuff to, and that is probably too limited to ask for multi-ssl-init? (or perhaps it'll work out if we reserve one bit for each known TLS lib?)

Otherwise I've been thinking that since multi-ssl is new functionality anyway, we might want to consider adding a totally new way to init this as well.

So maybe a new function curl_select_sslbackend(curl_sslbackend backend); that specifically verifies that curl_global_init() has not yet run? And also errors out if support for the specified has not been compiled in?

Sorry for being brief, I just rebuilt the code with my "usual" configure setup which boils down to:

./configure --disable-shared --enable-debug --enable-maintainer-mode --enable-werror

Ah... got it...

The warnings are actually also in the travis build

D'oh, I should have checked, and then simply read .travis.yml. Sorry for the noise. Will update this Pull Request and ping you.

[bagder]

maybe a new function curl_select_sslbackend(curl_sslbackend backend); that specifically verifies that curl_global_init() has not yet run? And also errors out if support for the specified has not been compiled in?

(brainstorming mode, just writing out my ideas and thoughts here)

Maybe something like this (I picked the curl_global_ prefix here since it is a global function and is similar in spirit to curl_global_init):

CURLsslset curl_global_sslset(curl_sslbackend backend, unsigned int *avail);

Returns:

• CURLSET_OK on success
• CURLSET_UNKNOWN_BACKEND for "unsupported backend" to select
• CURLSET_TOO_LATE when getting called "too late, already inited"
• etc

The avail value is a bitmask that is returned with information about which SSL backends that are available. The bit number in the bitmask represents the particular backend: 1<<backend.

That way an application can call curl_global_sslset(CURLSSLBACKEND_NONE, &alternatives); to query the library for what options there are.

[dscho]

Okay, i added two more commits (and rebased the whole shebang, sorry that my previous "fixed in " no longer apply verbatim).

Now you can set the default SSL backend via ./configure --with-default-ssl-backend=<backend>. It s not compile-time safe, as I mentioned earlier, but the value is tested in ./configure against the list of to-be-compiled-in SSL backends.

While working on that, I noticed something I had missed earlier: the symbols are optionally prefixed with the current SSL backend. If more than one SSL backend was compiled in, one of them was picked for said versioning. I changed that to the MULTISSL_ prefix.

While at it, I also moved the determination whether more than one SSL backend was configured into the ./configure script, as it needs that information for the versioning anyway. This may make things potentially a bit harder for folks interested in getting CMake up to par with multiple SSL backends.

@bagder I hope you agree with this course of action?

[coveralls]

Coverage decreased (-0.2%) to 73.019% when pulling ee68d80 on dscho:ssl-multi-backend into dff069f on curl:master.

[dscho]

It needs to be done by September 6 or it'll miss this release, but it feels like plenty enough time.

Is this PR still on track for 7.56.0?

[bagder]

absolutely!

[bagder]

As of commit a330bab, this is now merged into master.

Thanks @dscho for your hard work on this. Now let's see what bugs we have left to fix for this! =)

[dscho]

Thanks @dscho for your hard work on this. Now let's see what bugs we have left to fix for this! =)

Awesome! Please ping me with whatever fallout there is to fix, and I will try my best to address the issues as quickly as time allows.

[dscho]

(And sorry about the incorrect version in the man page, I saw that you fixed this in master, thank you so much!)