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.

Sign up for GitHub

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

Jump to bottom

crypto: remove cryptopp library #20015

Merged
merged 5 commits into from Jan 22, 2018
Merged

crypto: remove cryptopp library #20015

merged 5 commits into from Jan 22, 2018

Conversation

cbodley
Copy link
Contributor

@cbodley cbodley commented Jan 18, 2018

our cryptopp implementation was broken with the change to c++17, and the cost of fixing it likely outweighs the utility of keeping it around

@cbodley cbodley added the common label Jan 18, 2018
@cbodley cbodley mentioned this pull request Jan 18, 2018
Closed
@gregsfortytwo
Copy link
Member

Grepping, I see

  • the file cmake/modules/Findcryptopp.cmake
  • src/msg/async/dpdk/TCP.h:#include <cryptopp/md5.h>

At least the first one of those looks to be ours. Not certain about the second.

There's also

  • CMakeLists.txt: message(WARNING "ssl not found: rgw civetweb may fail to dlopen libssl libcrypto")

...and I confess I'm a bit confused; I see it removes "security/libcryptopp" from the FreeBSD portion of install-deps.sh, but shouldn't we be removing it from the .deb and rpm builders? Or are those all configured to use nss and already don't have cryptopp at all?

gregsfortytwo
gregsfortytwo approved these changes Jan 19, 2018
View reviewed changes
Copy link
Member

@gregsfortytwo gregsfortytwo left a comment

Choose a reason for hiding this comment

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

Anyway I don't know much about the details but those are the remaining instances I saw, so you appear to have pretty well killed it.
And I guess I can sign off on removing cryptopp from a project level.

So have a
Reviewed-by: Greg Farnum gfarnum@redhat.com

tchaikov
tchaikov reviewed Jan 19, 2018
View reviewed changes
CMakeLists.txt
find_package(NSPR REQUIRED)
set(CRYPTO_LIBS ${NSS_LIBRARIES} ${NSPR_LIBRARIES})
else ()
find_package(cryptopp REQUIRED)
Copy link
Contributor

Choose a reason for hiding this comment

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

we might want to remove cmake/modules/Findcryptopp.cmake along with this line.

@cbodley
cmake: remove cryptopp option
ba85740 
Signed-off-by: Casey Bodley <cbodley@redhat.com>
@cbodley
auth: remove cryptopp CryptoKeyHandler impl
fcf9ccc 
Signed-off-by: Casey Bodley <cbodley@redhat.com>
@cbodley
rgw: remove cryptopp BlockCrypt impls
097b9d4 
Signed-off-by: Casey Bodley <cbodley@redhat.com>
@cbodley
common: remove cryptopp digest impls
a18f058 
Signed-off-by: Casey Bodley <cbodley@redhat.com>
@cbodley
common: remove typedef for byte
52e3cb7 
Signed-off-by: Casey Bodley <cbodley@redhat.com>
@cbodley
Copy link
Contributor Author

cbodley commented Jan 19, 2018 •
edited

src/msg/async/dpdk/TCP.h:#include <cryptopp/md5.h>

thanks, I didn't catch that one! TCP.h is including other ceph headers, but all of those files look they were copied from an older version of seastar (Copyright (C) 2014 Cloudius Systems, Ltd.) - and i see that seastar does have a transitive dependency on cryptopp. that might be a reason to reconsider removing it?

anyhow, trying to build WITH_DPDK=ON is giving me some unrelated errors in msg/async/dpdk/shared_ptr.h, so we don't even get to the point where this use of cryptopp would show up as a link error

I see it removes "security/libcryptopp" from the FreeBSD portion of install-deps.sh, but shouldn't we be removing it from the .deb and rpm builders? Or are those all configured to use nss and already don't have cryptopp at all?

I didn't see any references to cryptopp/crypto++ in ceph.spec.in or debian/*, they're both using nss unconditionally

@cbodley
Copy link
Contributor Author

cbodley commented Jan 22, 2018

updated to remove the Findcryptopp.cmake module

jenkins test this please

@liewegas
Copy link
Member

http://pulpito.ceph.com/sage-2018-01-22_16:28:42-rados-wip-sage3-testing-2018-01-22-0845-distro-basic-smithi/

@liewegas liewegas merged commit 6eb85b3 into ceph:master Jan 22, 2018
@cbodley cbodley deleted the wip-crypto-- branch January 23, 2018 14:49
tchaikov added a commit to tchaikov/ceph that referenced this pull request Aug 20, 2018
@tchaikov
cmake: fix the build WITH_DPDK=ON
195d3a3 
* add Findcryptopp.cmake back
  cryptopp support was dropped in ceph#20015, but it's required by
  src/msg/async/dpdk/TCP.h, which `#include <cryptopp/md5.h>`
  so, to fix the FTBFS of WITH_DPDK=ON, we need to bring
  Findcryptopp.cmake back. it was also removed in ceph#20015.
* pass "-march=core2" when building sources which include dpdk headers.
  i was wrong that the headers shipped by distro are generic.
  the headers use the sse instructions for speedup memcpy, see
  /usr/include/x86_64-linux-gnu/dpdk/rte_memcpy.h .
* also, we need to include the arch specific include directory
  for building with dpdk.

Signed-off-by: Kefu Chai <kchai@redhat.com>
@tchaikov tchaikov mentioned this pull request Aug 20, 2018
Merged
3 tasks
@cbodley cbodley mentioned this pull request Aug 20, 2018
Closed
tchaikov added a commit to tchaikov/ceph that referenced this pull request Aug 21, 2018
@tchaikov
cmake: fix the build WITH_DPDK=ON
11b1f62 
* add Findcryptopp.cmake back
  cryptopp support was dropped in ceph#20015, but it's required by
  src/msg/async/dpdk/TCP.h, which `#include <cryptopp/md5.h>`
  so, to fix the FTBFS of WITH_DPDK=ON, we need to bring
  Findcryptopp.cmake back. it was also removed in ceph#20015.
* pass "-march=core2" when building sources which include dpdk headers.
  i was wrong that the headers shipped by distro are generic.
  the headers use the sse instructions for speedup memcpy, see
  /usr/include/x86_64-linux-gnu/dpdk/rte_memcpy.h .
* also, we need to include the arch specific include directory
  for building with dpdk.

Signed-off-by: Kefu Chai <kchai@redhat.com>
tchaikov added a commit to tchaikov/ceph that referenced this pull request Aug 21, 2018
@tchaikov
cmake: fix the build WITH_DPDK=ON
2a6357d 
* add Findcryptopp.cmake back
  cryptopp support was dropped in ceph#20015, but it's required by
  src/msg/async/dpdk/TCP.h, which `#include <cryptopp/md5.h>`
  so, to fix the FTBFS of WITH_DPDK=ON, we need to bring
  Findcryptopp.cmake back. it was also removed in ceph#20015.
* pass "-march=core2" when building sources which include dpdk headers.
  i was wrong that the headers shipped by distro are generic.
  the headers use the sse instructions for speedup memcpy, see
  /usr/include/x86_64-linux-gnu/dpdk/rte_memcpy.h .
* also, we need to include the arch specific include directory
  for building with dpdk.

Signed-off-by: Kefu Chai <kchai@redhat.com>
tchaikov added a commit to tchaikov/ceph that referenced this pull request Aug 29, 2018
@tchaikov
cmake: fix the build WITH_DPDK=ON
1b6d91b 
* add Findcryptopp.cmake back
  cryptopp support was dropped in ceph#20015, but it's required by
  src/msg/async/dpdk/TCP.h, which `#include <cryptopp/md5.h>`
  so, to fix the FTBFS of WITH_DPDK=ON, we need to bring
  Findcryptopp.cmake back. it was also removed in ceph#20015.
* pass "-march=core2" when building sources which include dpdk headers.
  i was wrong that the headers shipped by distro are generic.
  the headers use the sse instructions for speedup memcpy, see
  /usr/include/x86_64-linux-gnu/dpdk/rte_memcpy.h .
* also, we need to include the arch specific include directory
  for building with dpdk.

Signed-off-by: Kefu Chai <kchai@redhat.com>
tchaikov added a commit to tchaikov/ceph that referenced this pull request Sep 5, 2018
@tchaikov
cmake: fix the build WITH_DPDK=ON
c976f58 
* add Findcryptopp.cmake back
  cryptopp support was dropped in ceph#20015, but it's required by
  src/msg/async/dpdk/TCP.h, which `#include <cryptopp/md5.h>`
  so, to fix the FTBFS of WITH_DPDK=ON, we need to bring
  Findcryptopp.cmake back. it was also removed in ceph#20015.
* pass "-march=core2" when building sources which include dpdk headers.
  i was wrong that the headers shipped by distro are generic.
  the headers use the sse instructions for speedup memcpy, see
  /usr/include/x86_64-linux-gnu/dpdk/rte_memcpy.h .
* also, we need to include the arch specific include directory
  for building with dpdk.

Signed-off-by: Kefu Chai <kchai@redhat.com>
@tchaikov tchaikov mentioned this pull request Oct 2, 2019
Closed
3 tasks
gentoo-bot pushed a commit to gentoo/gentoo that referenced this pull request Jun 26, 2023
@thesamesam
sys-cluster/ceph: drop stale crypto++ dep
9e6a27a 
See ceph/ceph#20015. Removed upstream in "Mimic"
(ceph 13.x).

Signed-off-by: Sam James <sam@gentoo.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tchaikov tchaikov tchaikov approved these changes

@gregsfortytwo gregsfortytwo gregsfortytwo approved these changes

Assignees
No one assigned
Labels
common wip-sage3-testing
Projects
None yet
Milestone
No milestone
4 participants
@cbodley @gregsfortytwo @liewegas @tchaikov