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
Member

Lekensteyn commented Dec 1, 2019

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

@Lekensteyn Lekensteyn added the cmake label Dec 1, 2019
@Lekensteyn Lekensteyn requested a review from kdudka Dec 1, 2019
@kdudka
kdudka approved these changes Dec 2, 2019
Copy link
Collaborator

kdudka left a comment

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

This comment has been minimized.

Copy link
Member Author

Lekensteyn 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. 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

This comment has been minimized.

Copy link
Collaborator

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

This comment has been minimized.

Copy link
Member Author

Lekensteyn commented Dec 4, 2019

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 #4663
@Lekensteyn Lekensteyn force-pushed the Lekensteyn:cmake-nss branch from 85a547c to 103e605 Dec 4, 2019
@kdudka

This comment has been minimized.

Copy link
Collaborator

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member Author

Lekensteyn commented Dec 4, 2019

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.