net-misc/curl: enable multiple ssl implementations #16592

tgbugs · 2020-07-04T23:44:04Z

Makes it possible to enable multiple CURL_SSL options.

It also adds the CURL_DEFAULT_SSL prefix to set the default ssl
implementation when multiple implementations are enabled.

There is at least one other package affected by this dev-python/pycurl and I suspect that there are others, and that the CI report will show this.

I am marking this as a draft since there is at least one known issue and because I am not sure that my approach with the CURL_DEFAULT_SSL use expand option is the best approach.

Related to: https://bugs.gentoo.org/674706

@thesamesam
@jer-gentoo
@blueness

Signed-off-by: Tom Gillespie <tgbugs@gmail.com>

gentoo-bot · 2020-07-04T23:44:10Z

Pull Request assignment

Submitter: @tgbugs
Areas affected: ebuilds, profiles
Packages affected: net-misc/curl

net-misc/curl: @blueness

Linked bugs

No bugs to link found. If your pull request references any of the Gentoo bug reports, please add appropriate GLEP 66 tags to the commit message and request reassignment.

If you do not receive any reply to this pull request, please open or link a bug to attract the attention of maintainers.

In order to force reassignment and/or bug reference scan, please append [please reassign] to the pull request title.

Docs: Code of Conduct ● Copyright policy (expl.) ● Devmanual ● GitHub PRs ● Proxy-maint guide

tgbugs · 2020-07-05T00:34:38Z

Some thoughts on what to do about other packages that make use of CURL_SSL are in these two commit messages. Reproduced here as well.
tgbugs/tgbugs-overlay@94359f0
tgbugs/tgbugs-overlay@0bd2db3

I think the right solution is to just use
CURL_SSL but specify it per consuming package, because I think curl is
the only one that needs to know the default, the rest just need to know
that their preferred ssl is enabled, so none of the ebuilds need to
change, just the way that CURL_SSL is set in package.use, namely to be
per curl consuming package. Actually that doesn't work because of the way
net-misc/curl[curl_ssl_impl(-)=] works, but maybe just removing the =
and adding REQUIRE_USE on pycurl itself is the right approach?

Added REQUIRE_USE on the curl_ssl_* options and change = to ? so that we
can ensure that net-misc/curl has support for those methods enabled but
not necessarily enabled as the default, this should minimize the
propagation of the issue through the tree, it might also be possible to
check CURL_DEFAULT_SSL to pick that if CURL_SSL is not set, but that is
something for deeper consideration that I don't have an answer for.
For packages that can only enable a single ssl backend it would simplify
the change in semantics to follow CURL_DEFAULT_SSL so that users
are not affected by the change.

blueness · 2020-07-09T16:59:29Z

I looked over this and it looks like it should work. Have you tested thoroughly?

tgbugs · 2020-07-09T17:57:30Z

I'm in the process of running a grid of tests over all the possible default backends except for libressl (I don't have a convenient environment with it installed instead of openssl) with RESTRICT="!test? ( test )" and FEATURES=test. So far openssl default with nss, gnutls, and openssl passes all tests but now testing other combinations I have come across an issue with use flag combinations that needs to be resolved.

Also the change from CURL_SSL allowing a single value to CURL_SSL allowing multiple values will block emerge for any users that set */* CURL_SSL: backend1 backend2 because of restrictions on other ebuilds that make use of CURL_SSL and expect it to contain only a single value (e.g. dev-python/pycurl). I'm not sure the best way to proceed there.

tgbugs · 2020-07-09T18:56:54Z

I was able to sort out the the issue with the use flags, it has more to do with unintuitive behavior of setting +curl_default_ssl_openssl which forces the user to set */* CURL_DEFAULT: -* backend-not-openssl. Looking into a better solution for that than setting + in the ebuild itself.

edit: looks like adding CURL_DEFAULT_SSL="openssl" in profiles/base/make.defaults is the right solution here, and it may make sense to add CURL_SSL="openssl" to make.defaults as well rather than setting + in the ebuild to avoid forcing users to set */* CURL_SSL: -* in order to exclude openssl.

edit edit: unfortunately it looks like using make.defaults still requires the user to set -*, so not going that route since it just complicates other things down the line.

tgbugs · 2020-07-09T19:59:43Z

Testing on amd64 with CURL_DEFAULT_SSL set to openssl, gnutls, nss, and mbedtls I have encountered only a single error which is a mismatch between the features of curl and curl-config when using mbedtls as the default backend.

I think two additional things need to happen at this point. First we have to run the addition of CURL_DEFAULT_SSL USE_EXPAND by the dev mailing list, and two there needs to be a news item written to explain the change in behavior.
Any existing configs should be fine, but until the other ebuilds that make use of CURL_SSL use flags are updated
setting */* CURL_SSL: to use multiple backends won't work, and multiple backends should initially only be set on net-misc/curl.

HOWEVER I think this is not the end of the story because many packages that make use of CURL_SSL have broken dependency rules which try to force openssl to only have a single backend enabled (it seems likely that that should never have been done in the first place, but I don't know the exact history). I do not think that this would block this pr from being merged though, especially given that nearly every library that makes use of the CURL_SSL flags has a note that says "if curl changes you need to rebuild this too".

As a side note all of these tests were also conducted with curl built using distcc in case that is of interest to anyone.

Another side note is that the default backend does not have to be included in CURL_SSL, it will be enabled, but it
won't show up in the CURL_SSL list. I don't think this is an issue since it is visible under CURL_DEFAULT_SSL.

*/* CURL_SSL: -* openssl gnutls nss mbedtls
*/* CURL_DEFAULT_SSL: -* mbedtls

test 1014...Mismatch in features lists:
curl:        ipv6 libz multissl ntlm ssl tls-srp unixsockets
curl-config: https-proxy ipv6 libz multissl ntlm ssl tls-srp unixsockets
 postcheck FAILED

gentoo-repo-qa-bot · 2020-07-09T20:16:50Z

Pull request CI report

Report generated at: 2020-07-09 20:16 UTC
Newest commit scanned: 51eca6f
Status: ✅ good

There are existing issues already. Please look into the report to make sure none of them affect the packages in question:
https://qa-reports.gentoo.org/output/gentoo-ci/19fbb68bd7/output.html

blueness · 2020-07-23T12:59:45Z

What is the status of this work?

tgbugs · 2020-07-23T18:02:11Z

If there are no objections to the approach from anyone here then, this pr by itself is ready I think, but CURL_DEFAULT_SSL needs to be brought up on the dev mailing list. I can send the email if needs be.

I don't think we need a news item immediately, maybe once other ebuilds are updated to allow multiple backends, but for now the new behavior is backwards compatible with any existing configs, so it shouldn't break existing systems. That said, until other ebuilds using CURL_SSL can be updated to account for the change, users making use of multiple ssl backends should use this as follows.

*/* CURL_SSL: -* single-backend
*/* CURL_DEFAULT_SSL: single-backend
net-misc/curl CURL_SSL: -* multiple back ends

In words, users should only set a single backend globally until the other ebuilds using CURL_SSL are updated to reflect the change.

blueness · 2020-07-24T15:50:49Z

Let me get another reviewer to look at this to see if this might cause other problems.

mgorny · 2020-07-28T02:27:32Z

net-misc/curl/curl-7.71.1-r1.ebuild

+	curl_ssl_winssl? ( elibc_Winnt )
+	threads? ( !adns )
+	ssl? (
+		curl_ssl_libressl? ( !curl_ssl_openssl )


Can we use this as an opportunity to eliminate this artifical libressl/openssl switch, and start using USE=libressl like every other package? It might also make sense to eliminate CURL_SSL if they are no longer mutually exclusive. In fact, it might be cleaner to just use regular flags for the newfangled multi-backend thingie and CURL_SSL for the default backend which is roughly the old behavior.

I like this suggestion a lot. Except for the usual libressl/openssl issue none of the backends are mutually exclusive. I think we may not even need CURL_SSL anymore once the rest of the ebuilds using it have been migrated. They would be able to make use of any global use flag that is set (e.g. USE=gnutls). For packages that currently use CURL_SSL and still need the SSL backends to be mutually exclusive I think we can transition them to the standard pattern for that that is used elsewhere. I think it is much more in line with how things work now and more consistent with how use flags are supposed to work generally. I will take a pass at implementing it.

I added CURL_SSL at the suggestion of another dev and have regretted ever since. Maybe we can take care of one issue at a time though to not confuse the matter.

mgorny · 2020-07-28T02:30:08Z

net-misc/curl/curl-7.71.1-r1.ebuild

+			einfo "SSL provided by gnutls"
+			myconf+=( --with-gnutls --with-nettle )
+		fi
+		if use curl_ssl_libressl || use curl_default_ssl_libressl; then


You don't really need two separate LibreSSL/OpenSSL blocks because both do the same.

I left them this way in the event that at some point in the future there might be a need to handle them differently. If you think that won't ever happen then I can remove the duplicate block.

mgorny · 2020-07-28T02:30:24Z

net-misc/curl/curl-7.71.1-r1.ebuild

+		if use curl_default_ssl_gnutls; then
+			einfo "Default SSL provided by gnutls"
+			myconf+=( --with-default-ssl-backend=gnutls )
+		elif use curl_default_ssl_libressl; then


mgorny · 2020-07-28T02:30:51Z

profiles/desc/curl_default_ssl.desc

@@ -0,0 +1,11 @@
+# Copyright 1999-2020 Gentoo Foundation.


We do 'Gentoo Authors' nowadays.

I will update curl_ssl.desc while I'm at it since that is where that line came from.

mgorny · 2020-07-28T02:31:28Z

profiles/desc/curl_default_ssl.desc

+
+# This file contains descriptions of CURL_DEFAULT_SSL USE_EXPAND flags for net-misc/curl
+
+gnutls - Use GnuTLS


These should explain what happens when you set them, i.e. what's the diff between 'default' and 'multi' target thingie.

mgorny · 2020-07-28T02:31:38Z

profiles/desc/curl_default_ssl.desc

+mbedtls - Use mbed TLS
+nss - Use Mozilla's Network Security Services
+openssl - Use OpenSSL
+winssl - Use WinSSL (only with elibc_Winnt)


Please fix your editor to leave trailing newlines.

I think that was actually me doing it manually since I was confused by why nss and winssl had them in the first place. I will avoid doing so in the future. For the record do you know the reason they are present?

Try cat file on a file without trailing newline.

Oof. Yeah. Will fix. I usually see in commits that I add newlines to the end of files that don't have them rather than remove them so this case is egregiously bad, will see what is going on with it.

mgorny · 2020-08-01T13:14:29Z

I don't see any changes pushed since my last review.

This commit makes it possible to enable multiple ssl backends for curl by setting any of the gnutls, libressl/openssl, mbedtls, nss, and winssl use flags. The behavior of CURL_SSL is slighly modified so that it sets the default ssl backend that curl uses rather than the only backend that it uses. This allows it to continue to be used on other ebuilds without users having to make any changes to their current use flag configuration. Signed-off-by: Tom Gillespie <tgbugs@gmail.com>

This allows testing when FEATURES=test is set explicitly. On amd64 all tests are passing for curl-7.71.1-r1.ebuild with multiple use flag combinations, so it seems to make sense to make it possible to run tests without having to modify the ebuild. Signed-off-by: Tom Gillespie <tgbugs@gmail.com>

tgbugs · 2020-08-01T20:22:48Z

I have implemented @mgorny 's suggestion to simplify things by just using the regular use flags. This leaves the behavior of CURL_SSL in place but now it only applies to the default. This should minimize disruption of other ebuilds while we resolve the question of what to do with CURL_SSL. It seems like there needs to be something like CURL_SSL in order to set the default (ala PYTHON_SINGLE_TARGET ??), but not in the very restrictive way that it is used throughout the tree at the moment.

As a result all the changes related to CURL_DEFAULT_SSL have been removed (thankfully).

mgorny

It's a tad late for me but I suppose this makes sense now. I trust you've tested that it really works.

mgorny · 2020-08-01T20:39:32Z

I suppose you need to update/clone some profile-specific masks for the new flags.

tgbugs · 2020-08-01T21:02:37Z

Yes you're right. The winssl needs to be masked. Will do a review of those and add the fixes.

Signed-off-by: Tom Gillespie <tgbugs@gmail.com>

gentoo-repo-qa-bot · 2020-08-02T01:07:27Z

Pull request CI report

Report generated at: 2020-08-02 01:07 UTC
Newest commit scanned: 24e493c
Status: ❌ broken

New issues caused by PR:
https://qa-reports.gentoo.org/output/gentoo-ci/cf10cb1f93/output.html#net-misc/curl

There are existing issues already. Please look into the report to make sure none of them affect the packages in question:
https://qa-reports.gentoo.org/output/gentoo-ci/cf10cb1f93/output.html

blueness · 2020-08-02T21:22:00Z

Okay, looks like this is ready to be committed. I'll do some testing and then commit.

blueness · 2020-08-04T19:24:51Z

Okay, I had to drop ~riscv because of dependency problems. Other than that, this is good and I pushed it.

net-misc/curl: 7.71.1-r1 base from 7.71.1

8a31f85

Signed-off-by: Tom Gillespie <tgbugs@gmail.com>

gentoo-bot added assigned PR successfully assigned to the package maintainer(s). no bug found No Bug/Closes found in the commits. labels Jul 4, 2020

tgbugs marked this pull request as ready for review July 9, 2020 20:20

blueness requested review from Zlogene and mgorny July 24, 2020 15:48

blueness requested a review from SoapGentoo July 24, 2020 16:20

mgorny requested changes Jul 28, 2020

View reviewed changes

blueness requested a review from mgorny August 1, 2020 13:10

tgbugs added 2 commits August 1, 2020 13:17

tgbugs force-pushed the curl-multi-ssl branch from 51eca6f to a392cc1 Compare August 1, 2020 20:18

mgorny approved these changes Aug 1, 2020

View reviewed changes

tgbugs added 2 commits August 1, 2020 14:54

profiles: base use.mask winssl

d37c6fe

Signed-off-by: Tom Gillespie <tgbugs@gmail.com>

net-misc/curl: update metadata.xml with ssl USE flags

24e493c

Signed-off-by: Tom Gillespie <tgbugs@gmail.com>

blueness closed this Aug 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

net-misc/curl: enable multiple ssl implementations #16592

net-misc/curl: enable multiple ssl implementations #16592

tgbugs commented Jul 4, 2020

gentoo-bot commented Jul 4, 2020

tgbugs commented Jul 5, 2020

blueness commented Jul 9, 2020

tgbugs commented Jul 9, 2020

tgbugs commented Jul 9, 2020 •

edited

tgbugs commented Jul 9, 2020

gentoo-repo-qa-bot commented Jul 9, 2020

blueness commented Jul 23, 2020

tgbugs commented Jul 23, 2020

blueness commented Jul 24, 2020

mgorny Jul 28, 2020

tgbugs Jul 28, 2020

blueness Aug 1, 2020

mgorny Jul 28, 2020

tgbugs Aug 1, 2020 •

edited

mgorny Jul 28, 2020

mgorny Jul 28, 2020

tgbugs Jul 28, 2020

mgorny Jul 28, 2020

mgorny Jul 28, 2020

tgbugs Jul 28, 2020

mgorny Jul 28, 2020

tgbugs Jul 28, 2020

mgorny commented Aug 1, 2020

tgbugs commented Aug 1, 2020 •

edited

mgorny left a comment

mgorny commented Aug 1, 2020

tgbugs commented Aug 1, 2020

gentoo-repo-qa-bot commented Aug 2, 2020

blueness commented Aug 2, 2020

blueness commented Aug 4, 2020


		# This file contains descriptions of CURL_DEFAULT_SSL USE_EXPAND flags for net-misc/curl

		gnutls - Use GnuTLS

net-misc/curl: enable multiple ssl implementations #16592

net-misc/curl: enable multiple ssl implementations #16592

Conversation

tgbugs commented Jul 4, 2020

gentoo-bot commented Jul 4, 2020

Pull Request assignment

Linked bugs

tgbugs commented Jul 5, 2020

blueness commented Jul 9, 2020

tgbugs commented Jul 9, 2020

tgbugs commented Jul 9, 2020 • edited

tgbugs commented Jul 9, 2020

gentoo-repo-qa-bot commented Jul 9, 2020

Pull request CI report

blueness commented Jul 23, 2020

tgbugs commented Jul 23, 2020

blueness commented Jul 24, 2020

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

tgbugs Aug 1, 2020 • edited

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

mgorny commented Aug 1, 2020

tgbugs commented Aug 1, 2020 • edited

mgorny left a comment

Choose a reason for hiding this comment

mgorny commented Aug 1, 2020

tgbugs commented Aug 1, 2020

gentoo-repo-qa-bot commented Aug 2, 2020

Pull request CI report

blueness commented Aug 2, 2020

blueness commented Aug 4, 2020

tgbugs commented Jul 9, 2020 •

edited

tgbugs Aug 1, 2020 •

edited

tgbugs commented Aug 1, 2020 •

edited