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

CMake: add support for building with the NSS vtls backend #4663

Closed
wants to merge 1 commit into from

Conversation

Lekensteyn
Copy link
Contributor

Options are cross-checked with configure.ac and acinclude.m4.
Tested on Arch Linux, not tested with other platforms.

Copy link
Contributor

@kdudka kdudka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. It works fine on Fedora 29. On (unreleased) Fedora 32, I got linking failures even without this change, so I could not test it.

As a minor remark, it does not allow to set default CApath with NSS despite it is actually supported when nss-pem is avaialble (and I verified that manually set CApath worked with this build). But nss-pem is now in maintenance mode, so it makes perfect sense to disable setting default CApath with NSS.

@Lekensteyn
Copy link
Contributor Author

I did see potential support for a hard-coded CA path for NSS, but as autotools did not enable it either, I did not bother. I also did not have the required libnsspem.so library on Arch Linux for testing.

If everything is OK, I plan to have this merged after a Travis CI patch.

@kdudka
Copy link
Contributor

kdudka commented Dec 3, 2019

I did see potential support for a hard-coded CA path for NSS, but as autotools did not enable it either, I did not bother.

The autoconf-produced configure script supports --with-nss --with-ca-bundle=... but the current recommendation is not to use it.

I also did not have the required libnsspem.so library on Arch Linux for testing.

Yes, that is not going to improve I am afraid.

If everything is OK, I plan to have this merged after a Travis CI patch.

Sounds like a good idea to me. Thanks!

@Lekensteyn
Copy link
Contributor Author

The autoconf-produced configure script supports --with-nss --with-ca-bundle=... but the current recommendation is not to use it.

The default autotools config appears to set the CA bundle which will fail by default on Arch Linux when built with NSS:

$ src/curl https://example.com -vso /dev/null
*   Trying 93.184.216.34:443...
* TCP_NODELAY set
* Connected to example.com (93.184.216.34) port 443 (#0)
* Initializing NSS with certpath: none
* WARNING: failed to load NSS PEM library libnsspem.so. Using OpenSSL PEM certificates will not work.
*   CAfile: /etc/ssl/certs/ca-certificates.crt
  CApath: none
* Closing connection 0

With the following change relative to my previous patch, the CMake build will be "bug-compatible" with autotools:

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 8d696dfa5..e9727d77a 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -678,5 +678,5 @@ set(CURL_CA_PATH "auto" CACHE STRING
 if("${CURL_CA_BUNDLE}" STREQUAL "")
   message(FATAL_ERROR "Invalid value of CURL_CA_BUNDLE. Use 'none', 'auto' or file path.")
-elseif("${CURL_CA_BUNDLE}" STREQUAL "none" OR USE_NSS)
+elseif("${CURL_CA_BUNDLE}" STREQUAL "none")
   unset(CURL_CA_BUNDLE CACHE)
 elseif("${CURL_CA_BUNDLE}" STREQUAL "auto")
@@ -689,9 +689,11 @@ endif()
 if("${CURL_CA_PATH}" STREQUAL "")
   message(FATAL_ERROR "Invalid value of CURL_CA_PATH. Use 'none', 'auto' or directory path.")
-elseif("${CURL_CA_PATH}" STREQUAL "none" OR USE_NSS)
+elseif("${CURL_CA_PATH}" STREQUAL "none")
   unset(CURL_CA_PATH CACHE)
 elseif("${CURL_CA_PATH}" STREQUAL "auto")
   unset(CURL_CA_PATH CACHE)
-  set(CURL_CA_PATH_AUTODETECT TRUE)
+  if(NOT USE_NSS)
+    set(CURL_CA_PATH_AUTODETECT TRUE)
+  endif()
 else()
   set(CURL_CA_PATH_SET TRUE)

Changing the default CA bundle is potentially for a future patch, as workaround one can use curl --cacert "" or set cmake -DCURL_CA_BUNDLE=none.

I'll push the revised patch, feel free to merge it if you are happy.

Options are cross-checked with configure.ac and acinclude.m4.
Tested on Arch Linux, untested on other platforms like Windows or macOS.

Closes curl#4663
@kdudka
Copy link
Contributor

kdudka commented Dec 4, 2019

No worries, I was already happy with the previous patch. We do not need to be bug-compatible with autotools. Just wanted to record my experiments with this pull request. Having a working default on distros without nss-pem makes perfect sense.

@bagder
Copy link
Member

bagder commented Dec 4, 2019

The default autotools config appears to set the CA bundle which will fail by default on Arch Linux

The configure script doesn't figure out if libnsspem.so is present and working, it just picks a default. Without that PEM-library supported by NSS, I suppose you want to run configure with --without-ca-path.

@Lekensteyn
Copy link
Contributor Author

I think consistency between autotools and CMake is good, hence my previous change. NSS is not a common option, I think that the --without-capath is a good workaround for now. Maybe I'll revisit it in the future when I resume experimentation :-)

I'll push this, last day of the freeze!

@Lekensteyn Lekensteyn closed this in 87b9337 Dec 4, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Mar 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

Successfully merging this pull request may close these issues.

None yet

3 participants