cmake: rework NSS and SSL #9831
Conversation
mattbenjamin
changed the title
| set(USE_NSS 0) | ||
| set(USE_CRYPTOPP 1) | ||
| else() | ||
| MESSAGE(STATUS "Cryptopp not found using NSS instead") |
There was a problem hiding this comment.
this message doesn't appear to be true any more. should the find_package(cryptopp) be REQUIRED so we fail in this case?
There was a problem hiding this comment.
Yes. This needs to fail if cryptopp is not found, now. (Will push an update.)
| option(WITH_NSS "Use NSS crypto and SSL implementations" OFF) | ||
| if (${WITH_NSS}) | ||
| find_package(NSS REQUIRED) | ||
| if(NSS_FOUND) |
There was a problem hiding this comment.
if nss is not found, cmake will bail out with the default error message, so no need to check NSS_FOUND here?
There was a problem hiding this comment.
now that it is required, that's true
|
needs rebase. |
|
I'll clean up and rebase tomorrow |
|
Note wip-cmake-debian has
27c861a
Can we cherry-pick this into master, or do you want a fancier version
that maintains cryptopp? Don't care either way, as long as it doesn't
block merging deb. (The current "check for cryptopp, then check for nss"
fails on many gitbuilders.)
|
|
I'd really rather not dispose of SSL and Crypto choice--I guess if I want that, I need to fix gitbuilders? In the interest of that, how does that check cryptopp, check nss fail? |
|
The problem is that the gitbuilder detected cryptopp installed but it
didn't build. Since right now nobody ever uses it, at a minimum we have
to make nss the default choice. The only reason I didn't do that in my
patch is because it was confusing and I needed it fixed.
|
|
ok, makes sense--if you can wait till midday EST tomorrow, I can retest and push it then? (I guess I could sequence after the zap-cryptopp change, for that matter, if you prefer, I don't think it matters to history); I admit I never consciously used cryptopp, either--but I'm more inclined to think folks may wish to use openssl |
|
btw just hit this again on my laptop. libcrypto++ was install and the
build fails (bad #include somewhere). worked around it by uninstalling
libcrypto++
|
|
Ok, still testing this, but, in addition to requested cleanups, NSS is now the default for cmake and automake builds. |
|
Re-pushed--previous set had unintended changes to some generated m4 files. |
|
@mattbenjamin i am very sorry that i merged unrelated changes into master by mistake, so your changeset conflicts with sage's commit i merged. i have a PR reverting them. i just tested your changeset, both autotools and cmake builds work great. lgtm |
|
@tchaikov no problem, would it be easier if I rebased? |
The cmake build should select NSS or CryptoPP consistently with current autoconf logic, as used in our RHEL builds (-DWITH_NSS), and the SSL linkage must match (for NSS, -lssl3). With this change, there is no direct linkage with libssl when NSS is selected (I still see indirect linkage via libcurl, in turn via libssh2, looking into that). With guidance from upstream and downstream, NSS builds are now selected by default. Signed-off-by: Matt Benjamin <mbenjamin@redhat.com>
This change mainly intends to prevent linking with libssl when the crypto provider is NSS, which provides an SSL implementation (the implementation we must use and prefer when NSS is selected). Secondarily, actually detect openssl when it is selected, which happens by default and co-selects with cryptopp. To do this, we import the ax_check_openssl.m4 from the automake archive. Since upstream builds now prefer NSS crypto, make this the default. Signed-off-by: Matt Benjamin <mbenjamin@redhat.com>
The cmake build should select NSS or CryptoPP consistently with
current automake logic, as used in our RHEL builds (-DWITH_NSS),
and the SSL linkage must match (for NSS, -lssl3).
With this change, there is no direct linkage with libssl when NSS
is selected (I still see indirect linkage, looking into that).
Signed-off-by: Matt Benjamin mbenjamin@redhat.com