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

configure: make the TLS library choice(s) explicit #6897

Closed
wants to merge 5 commits into from

Conversation

@bagder
Copy link
Member

@bagder bagder commented Apr 15, 2021

With this, configure no longer tries to find a TLS library by default, but all libraries are now equal: the user needs to explicitly ask what TLS library or libraries to use.

If no TLS library is selected, configure will error out unless --without-ssl is explicitly used to request a build without TLS (as that is rare these days).

Update: this also removes --with-winssl and --with-darwinssl as they've been deprecated since 2019

@jay
Copy link
Member

@jay jay commented Apr 15, 2021

why? this will break some builds and seems unnecessary

Loading

@bagder
Copy link
Member Author

@bagder bagder commented Apr 15, 2021

Any kind of auto-detection have to use an order of detecting the libraries that makes us have to choose that order of preference. This way, we remove ourselves from making that decision. Without a change like this, we implicitly say that OpenSSL is to prefer, while I'm not sure that's a good leg to lean on forever.

(I also wanted to remove the need for --without-ssl or --without-openssl when selecting another TLS backend, but I will agree that it can be done without forcing the user to make this selection.)

Loading

@bagder bagder force-pushed the bagder/configure-explicit-tls branch from 7a80beb to 48d6bdd Apr 15, 2021
@bagder
Copy link
Member Author

@bagder bagder commented Apr 15, 2021

@mback2k it'd be great if you could update your buildbots' configure invokes to use --with-schannel instead of the deprecated --with-winssl - so that we can make sure things build fine there as well with this change.

Loading

@bagder
Copy link
Member Author

@bagder bagder commented Apr 15, 2021

This change highlights an existing problem we didn't notice before: the three azure pipelines builds msys1_*debug_openssl all fail to detect OpenSSL in configure and therefore builds and continues with TLS disabled. Contrary to expectations.

With this change, those configure runs instead return failure. Ie the problem exists since before this change, but now we see it better.

@mback2k do you know what we can do to make openssl detected and used in those builds?

Loading

@bagder
Copy link
Member Author

@bagder bagder commented Apr 15, 2021

The appveyor builds similarly build without TLS enabled (== fail to detect any). I'll try to enable schannel for those.

Loading

@bagder bagder force-pushed the bagder/configure-explicit-tls branch from efb48f5 to 5a2438b Apr 15, 2021
bagder added a commit that referenced this issue Apr 16, 2021
configure no longer tries to find a TLS library by default, but all
libraries are now equal: the user needs to explicitly ask what TLS
library or libraries to use.

If no TLS library is selected, configure will error out unless
--without-ssl is explicitly used to request a built without TLS (as that
is very rare these days).

Removes: --with-winssl, --with-darwinssl and all --without-* options for
TLS libraries.

Closes #6897
@bagder bagder force-pushed the bagder/configure-explicit-tls branch from c8c6552 to 0937c85 Apr 16, 2021
@mback2k
Copy link
Member

@mback2k mback2k commented Apr 16, 2021

@mback2k it'd be great if you could update your buildbots' configure invokes to use --with-schannel instead of the deprecated --with-winssl - so that we can make sure things build fine there as well with this change.

Done since yesterday evening.

@mback2k do you know what we can do to make openssl detected and used in those builds?

No idea yet, but I will look into this over the weekend.

Loading

bagder added a commit that referenced this issue Apr 17, 2021
configure no longer tries to find a TLS library by default, but all
libraries are now equal: the user needs to explicitly ask what TLS
library or libraries to use.

If no TLS library is selected, configure will error out unless
--without-ssl is explicitly used to request a built without TLS (as that
is very rare these days).

Removes: --with-winssl, --with-darwinssl and all --without-* options for
TLS libraries.

Closes #6897
@bagder bagder force-pushed the bagder/configure-explicit-tls branch from 0937c85 to dad89ce Apr 17, 2021
@mback2k
Copy link
Member

@mback2k mback2k commented Apr 20, 2021

After checking the available packages for classic MinGW, I think there is no native OpenSSL package available. Just the msys1-environment specific msys-openssl package that won't work for native Windows builds and is most likely already present in the build environment due to being a dependency of other packages. So I guess we have 2 options now:

  1. remove the 3 superfluous builds as suggested in this PR
  2. turn them into builds without SSL since I guess we don't have any such builds on native Windows yet

Loading

@mback2k
Copy link
Member

@mback2k mback2k commented Apr 20, 2021

While option 2 basically restores the previous CI situation. Another option could be to build OpenSSL for these CI variants, but I don't think it's worth the effort.

Loading

@bagder
Copy link
Member Author

@bagder bagder commented Apr 20, 2021

ok, let's switch off TLS in those builds for now

Loading

bagder added a commit that referenced this issue Apr 20, 2021
configure no longer tries to find a TLS library by default, but all
libraries are now equal: the user needs to explicitly ask what TLS
library or libraries to use.

If no TLS library is selected, configure will error out unless
--without-ssl is explicitly used to request a built without TLS (as that
is very rare these days).

Removes: --with-winssl, --with-darwinssl and all --without-* options for
TLS libraries.

Closes #6897
@bagder bagder force-pushed the bagder/configure-explicit-tls branch from dad89ce to 9ecbd08 Apr 20, 2021
.azure-pipelines.yml Show resolved Hide resolved
Loading
.azure-pipelines.yml Outdated Show resolved Hide resolved
Loading
.github/workflows/macos.yml Outdated Show resolved Hide resolved
Loading
bagder added 4 commits Apr 22, 2021
Fixes test 1165 when functions are moved from configure.ac to files in
m4/
configure no longer tries to find a TLS library by default, but all
libraries are now equal: the user needs to explicitly ask what TLS
library or libraries to use.

If no TLS library is selected, configure will error out unless
--without-ssl is explicitly used to request a built without TLS (as that
is very rare these days).

Removes: --with-winssl, --with-darwinssl and all --without-* options for
TLS libraries.

Closes #6897
... and put those functions in separate m4 files per TLS library.
@bagder bagder force-pushed the bagder/configure-explicit-tls branch from 48490b2 to 997ed8e Apr 22, 2021
@bagder bagder closed this in 68d89f2 Apr 22, 2021
@bagder bagder deleted the bagder/configure-explicit-tls branch Apr 22, 2021
TisVictress added a commit to cloudfoundry/buildpacks-ci that referenced this issue May 26, 2021
- Related PR: curl/curl#6897

Co-authored-by: Brayan Henao <bhenao@vmware.com>
algitbot pushed a commit to alpinelinux/aports that referenced this issue May 27, 2021
Contains fixes for the following vulnerabilities in 7.76.1:

* CVE-2021-22897
* CVE-2021-22898
* CVE-2021-22901

Also explicitly use OpenSSL as the TLS library, which is necessary
following the merge of curl/curl#6897.
algitbot pushed a commit to alpinelinux/aports that referenced this issue May 27, 2021
Contains fixes for the following vulnerabilities in 7.76.1:

* CVE-2021-22897
* CVE-2021-22898
* CVE-2021-22901

Also explicitly use OpenSSL as the TLS library, which is necessary
following the merge of curl/curl#6897.
algitbot pushed a commit to alpinelinux/aports that referenced this issue May 27, 2021
Contains fixes for the following vulnerabilities in 7.76.1:

* CVE-2021-22897
* CVE-2021-22898
* CVE-2021-22901

Also explicitly use OpenSSL as the TLS library, which is necessary
following the merge of curl/curl#6897.
@dscho
Copy link
Contributor

@dscho dscho commented May 27, 2021

why? this will break some builds and seems unnecessary

It's actually a bit worse: it does not break builds. At least here, I was building Git for Windows' cURL package using --with-winssl and it did not fail to build. What it failed was to tell me that I should fix by build script already ;-)

Loading

@bagder
Copy link
Member Author

@bagder bagder commented May 27, 2021

Oh, you mean when you actually wanted schannel and OpenSSL ... I didn't think about case that's true! 😞

Loading

@dscho
Copy link
Contributor

@dscho dscho commented May 27, 2021

Oh, you mean when you actually wanted schannel and OpenSSL ... I didn't think about case that's true! 😞

Not a big deal, but do you think there might be a way to error out upon unhandled --with-<something> options?

Loading

@MarcelRaad
Copy link
Member

@MarcelRaad MarcelRaad commented May 27, 2021

It was similar for me: I had --without-ssl --win-winssl, which previously enabled schannel and now builds without SSL quietly.

Loading

@stanhu
Copy link
Contributor

@stanhu stanhu commented Nov 12, 2021

#7994 should fix the handling of --without-* flags.

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants