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

Release for Crypto++ 8.7.0 #1

Closed
abdes opened this issue Aug 23, 2022 · 30 comments
Closed

Release for Crypto++ 8.7.0 #1

abdes opened this issue Aug 23, 2022 · 30 comments
Assignees

Comments

@abdes
Copy link
Owner

abdes commented Aug 23, 2022

This is a tracker for general discussions regarding release 8.7.0, being prepared in the master branch.

Please provide feedback, and discuss potential changes here,

The issue will be closed when the release tag is published.

@abdes abdes self-assigned this Aug 23, 2022
@abdes
Copy link
Owner Author

abdes commented Aug 24, 2022

I'm quite happy with the current state for Linux/MacOS/WIN platforms. I'm not adding any more enhancements before 8_7_0 release.

Gonna give it a couple of days for people to check if the master branch is good enough on their own platforms/use cases and by end of the week, I will release a 8.7.0 tag.

@chausner
Copy link
Collaborator

It appears noloader/cryptopp-cmake installed header files into a subfolder cryptopp while this version puts them directly into the include installation folder. The latter seems more consistent with how headers are used in weidai11/cryptopp. On the other hand, it would be a breaking change for anyone updating from noloader/cryptopp-cmake 8.6.0 to 8.7.0.

@abdes
Copy link
Owner Author

abdes commented Aug 25, 2022

It appears noloader/cryptopp-cmake installed header files into a subfolder cryptopp while this version puts them directly into the include installation folder. The latter seems more consistent with how headers are used in weidai11/cryptopp. On the other hand, it would be a breaking change for anyone updating from noloader/cryptopp-cmake 8.6.0 to 8.7.0.

I'll fix this. Not intended.

@Vollstrecker
Copy link
Collaborator

Before we do this work twice, I just wanted to make this an option which is presetable to something different. At least when amule started using cryptopp some distros did use crypto++ as dir for includes. I need this var anyways, as (at least in amule) this dir is passed as define to the code and the compiler is told to search in the parent dir.

@abdes
Copy link
Owner Author

abdes commented Aug 25, 2022

Before we do this work twice, I just wanted to make this an option which is presetable to something different. At least when amule started using cryptopp some distros did use crypto++ as dir for includes. I need this var anyways, as (at least in amule) this dir is passed as define to the code and the compiler is told to search in the parent dir.

Would you like to suggest a PR for this @Vollstrecker ?

@Vollstrecker
Copy link
Collaborator

In fact I planned this for later this day. You would like to have this separate, or combined with the overworked config export. I need to check, but I'm pretty sure 3 quarters of our Config.in would be generated by the export anyways, so this can be shrinked to much more readable ~10 lines.

@abdes
Copy link
Owner Author

abdes commented Aug 25, 2022

Could you please do two PRs? That way we can keep the concerns separate. Thank you.

@abdes
Copy link
Owner Author

abdes commented Aug 25, 2022

There is one more thing we need to decide. Currently, the crypto++ lib puts all its files directly at the repo root. The install now puts include files under include/cryptopp. Projects that use the library via cmake source dependency will get include files directly from crypto++ cloned repo, which means by default, #includes will not be using the 'cryptopp/' prefix.

I understood that some users would prefer to do a #include <cryptopp/xxxxxxx.h> style, which I personally prefer as well.

Currently the target include directories for cmake sub-project usage follow the direct include style, not the prefixed one.

Should we adjust the crypto++ cloned repo to have a cryptopp prefix and then publish includes with a prefix?
Or, should we keep it as is?

@Vollstrecker
Copy link
Collaborator

As this is exactly what I need that var for, I also would prefer to have this style on the target. We could do it with setting an BUILD_INTERFACE on the target and an INSTALL_INTERFACE to the installed one. And then add an define to the interface like CRYPTOPP_INCLUDE_PREFIX which holds the right subdir.

I guess if we set both include-paths, everything should work no matter if the users do their #include with the whole path, or with one level up. it should even work when built as subproject with include-path PROJECT_BUILD_DIR/external and PROJECT_BUILD_DIR/external/ext_cryptopp-src and CRYPTOPP_INCLUDE_PREFIX set to ext_cryptopp-src. But for testing this I need at least a day more.

@Vollstrecker
Copy link
Collaborator

So to clarify the target has then
BUILD_INTERFACE include-dirs: PROJECT_BUILD_DIR/external/ext_cryptopp-src and PROJECT_BUILD_DIR/external
BUILD_INTERFACE defines: ext_cryptopp-src
INSTALL_INTERFACE include-dirs CMAKE_INSTALL_INCLUDEDIR/CRYPTOPP_INCLUDE_PREFIX and CMAKE_INSTALL_INCLUDEDIR
INSTALL_INTERFACE defines: CRYPTOPP_INCLUDE_PREFIX

@noloader
Copy link
Collaborator

noloader commented Aug 25, 2022

Should we adjust the crypto++ cloned repo to have a cryptopp prefix and then publish includes with a prefix?
Or, should we keep it as is?

One comment in the context of Unix & Linux and Windows... during installation...

  • the header files are traditionally copied to ${includedir}. ${includedir} is either ${prefix}/include/crypto++ (Debian and friends) or ${prefix}/include/cryptopp (Red Hat and friends).
  • the library files are traditionally copied to ${libdir}. ${libdir} is typically ${prefix}/lib (Debian and friends) or ${prefix}/lib64 (Red Hat and friends).
  • I don't know where the typical install directories are located on Windows.

Debian and friends are Debian, Ubuntu, Mint, etc. Red Hat and friends are Red Hat, CentOS, Fedora, Scientific Linux, etc.

There's some handwaiving going on since the include and lib directories are controlled by config.site on Linux. But it usually holds when Crypto++ works with our maintainers on Linux, MacPorts (for OS X), and OpenCSW (for Solaris).

Ping @alonbl . I believe he helps with Gentoo, and he may use the updated CMake files if they work as expected. He can probably provide valuable feedback.

Ping @geoffbeier . He is another person I rely on a lot for perspective. I think he does a lot of work with Conan.

@alonbl
Copy link

alonbl commented Aug 25, 2022

Hi,

Not sure I can help, this build system is way too complex for me to digest, I am afraid it went too far with customization, but I have no time to review it.

I can see static library is created libcryptopp.a but in the pkgconfig I see Libs: -L${libdir} cryptopp.lib which is incorrect, I can also see:

prefix=/usr/local
libdir=lib
includedir=include
datadir=share/cryptopp

which is incorrect as it missing the $prefix in the vars.

without shared libraries no distribution will apply the new buildsystem whatever it will be.

very important to support both 32bit and 64bit side-by-side properly, so that include files are well behaved, and if generated are handled correctly.

and not break so version backward compatibility.

@Vollstrecker
Copy link
Collaborator

the header files are traditionally copied to ${includedir}. ${includedir} is either ${prefix}/include/cryptopp (Debian and friends) or ${prefix}/include/crypto++ (Red Hat and friends).

Sure that's why I suggested 2 include-dirs. amule for example uses ${prefix}/include and a define that tell the if it should look for cryptopp/hdr.h or crypto++/hdr.h. With your suggestion they would need to read the properties of cryptopp::cryptopp and construct their own target with the right stuff.

clementine uses hardcoded cryptopp/hdr.h so I guess the maintainers will have to patch this, or the'll leave out the spotify-support.

SecureFS seems to ship their own copy of an older version of cryptopp-cmake

codecrypt hardcodes /usr/local/opt/cryptopp/ and tell their users it's better to use homebrew.

I could list like tegrarcm, but all examples rely either on fixes places (which are not given) or on a define to get the right dir. A quick check in debian brought 5 out of 8 using the define, so if it's this OR that, I would clearly say the define variant should be preffered.

the library files are traditionally copied to ${libdir}. ${libdir} is typically ${prefix}/lib (Debian and friends) or ${prefix}/lib64 (Red Hat and friends).

Sure, but this is covered by the config file, in the past and the future.

I don't know where the typical install directories are located on Windows.

Noone really does, but their is a location where cmake searches for the configs by default, and if that one is found, noone needs to know where the libs, headers etc. are located because the config tells the build-tools and everything is fine.

without shared libraries no distribution will apply the new buildsystem whatever it will be.

As this is a decision made in the lib itself, the buildsystem doesn't matter, you won't get a shared version anyways (iiuc)

very important to support both 32bit and 64bit side-by-side properly, so that include files are well behaved, and if generated are handled correctly.

I don't really get this. If you build this for both (or just one) arches, you always have control over the location where they are installed. The headers are the same for both. I didn't read the headers, but I'm pretty sure if there are differences between them, this is handled with defines, which will be noted in the config if needed.

@abdes
Copy link
Owner Author

abdes commented Aug 26, 2022

We will fix the pkgconfig file once we agree on the install strategy.

Also, we can easily support the lib install side by side for 32 and 64 bits. Could you guide us on what are the best practices here @alonbl ?

The shared lib build is a crypto++ issue not a cmake issue. Maybe @noloader can comment on it.

@alonbl
Copy link

alonbl commented Aug 26, 2022

Tha pkg-config file should follow the build system, it is a template... even if you install files in the wrong locations it should match the wrong locations. You should test this to be working.

The problem with 32bit and 64bit is the installation of headers in case there is platform settings within one or more. the libdir will be set by the distribution using standard cmake arguments.

I do not understand how come the shared lib is not an issue of a build system. This is what a build systems do... Distributions always use shared library as otherwise an upgrade of the package will not effect the consumers.

@Vollstrecker
Copy link
Collaborator

I just gave it a quick shot. The .so isn't part of the build when just using the default target (it s listed in all: so I don't know what happens there), but with make dynamic I am able to build it. So we maybe shouldn't forbid the build completely.

Btw. respecting CMAKE_BUILD_SHARED_LIBS is ok, but I'm a big fan of being able to build static and shared in one run, but that's another topic.

@abdes
Copy link
Owner Author

abdes commented Aug 26, 2022

Crypto++ does not export symbol visibility correctly for shared lib/ dll builds and several templates are in .cpp files with no symbol export. The current shared lib build only exports a subset of the symbols for a soon to be end-of-life FIPS library.

That's what I meant by a crypto++ issue.

The build system does have everything it takes to build shared library but it's not what 's really expected.

@Vollstrecker
Copy link
Collaborator

I've seen in the docs only hints that the dll is not recommended and not FIPS validated. Also without the use of libtool it's always let's say problematic to only export needed symbols. But if there is a wrapper already as example, why not just including that one to ease the usage for maintainers and endusers?

@noloader
Copy link
Collaborator

noloader commented Aug 26, 2022

@abdes, @Vollstrecker,

I don't know where to begin with the problems with the FIPS DLL... The FIPS DLL is a subset of the Crypto++ library. The FIPS DLL only exports classes like DH, AES, SHA, RSA and DSA. Those classes are called "approved cryptography" (some handwaiving) by NIST. If you want a HexEncoder or a RIPEMD hash, then they are missing. It causes a lot of problems for users. Then there's the problem of symbol visibility. The only symbols exported are those for approved cryptography.

I've tried several times over the years to create Windows DLL that functions like users expect. That is, the whole library is available as a DLL instead of a static LIB. Here are the attempts:

So far my attempts have failed. (Be sure to read the head notes/source code comments in dump2def.cpp).

dump2def.cpp seems like it should be a path forward. I seem to recall the problem with dump2def.cpp is, it is missing quite a few symbols and I don't know why. I am using dumpbin.exe to generate the symbol list. Then I am parsing the list to generate the DEF file. But a lot of stuff is missing.

I think we need to get rid of the FIPS DLL. Then, we can apply __declspec dllexport to [nearly] all library symbols, and not just the approved cryptography.

Oh, and the shared object on Unix & Linux is fine. We can export all symbols without a lot of trouble (even some that don't need to be exported). That's why we have a shared object for Unix & Linux, but not a DLL for Windows.

@abdes
Copy link
Owner Author

abdes commented Aug 26, 2022

So, basically we can enable a shared lib build on anything except MSVC, although that shared lib breaks the good rules of exported symbols and visibility. For MSVC, unfortunately I tried that already and it still does not work and MSVC is way more picky on dllexport. In the long run though, symbols need to be properly exported...

@Vollstrecker
Copy link
Collaborator

If it's just a problem on windows we can 1. enable it everywhere else and 2. use WINDOWS_EXPORT_ALL_SYMBOLS which just assumes that all symbols have dllexport given. the user still need to add dllimport everywhere they need to use a function that doesn't have it, but this should be the smallest problem then.

@abdes
Copy link
Owner Author

abdes commented Aug 26, 2022

So, basically we can enable a shared lib build on anything except MSVC, although that shared lib breaks the good rules of exported symbols and visibility. For MSVC, unfortunately I tried that already and it still does not work and MSVC is way more picky on dllexport. In the long run though, symbols need to be properly exported...

I did run a build with -fPIC and -fvisibility=default with g++ on Linux, and tried to link cryptest to the shared library produced. Many undefined symbols... pretty much half of the library. it does not seem like it's only a windows problem.

Please note that in the current builds, cryptest is always linked against the static library.

@Vollstrecker please do give it a try in case I was doing something wrong...

@Vollstrecker
Copy link
Collaborator

I will. first I try to split this topic into some more issues that can be handled separate and start to work them one by one out, plus at least one other thing i've seen. I guess the list is getting longer.

@abdes
Copy link
Owner Author

abdes commented Aug 26, 2022

Oh, and the shared object on Unix & Linux is fine. We can export all symbols without a lot of trouble (even some that don't need to be exported). That's why we have a shared object for Unix & Linux, but not a DLL for Windows.

@noloader I see that even in the crypto++ GMakefile, you always link cryptest to the archive library and never to the shared lib... Is there a reason for that? I'm getting so many undefined symbols if I try to link it against the shared lib.

@noloader
Copy link
Collaborator

noloader commented Aug 26, 2022

@abdes,

@noloader I see that even in the crypto++ GMakefile, you always link cryptest to the archive library and never to the shared lib... Is there a reason for that?

Yes. It is easier to link against the static archive. Runtime linking against a shared object during testing can be very messy. If we linked against the shared object, then we have to worry about runtime linker paths, and use LD_LIBRARY_PATH and LD_PRELOAD tricks.

However, we do test runtime linking against the shared object in the cryptest.sh program around line 2305. From TestScripts/cryptest.sh : 2305:

    CXX="${CXX}" CXXFLAGS="$DEBUG_CXXFLAGS" LINK_LIBRARY=libcryptopp.so \
        "$MAKE" "${MAKEARGS[@]}" static dynamic cryptest.exe 2>&1 | tee -a "$TEST_RESULTS"

There is also a test for a release build, and a pair of tests for OS X.

We also perform a shared object symbol test in cryptest-symbols.sh. We want to ensure symbols don't go missing without a major version bump. (Ask me why I added this test...). The idea is we build cryptest.exe for version N-1. Next we build the shared object for version N. Finally, we make sure the old cryptest.exe (v. N-1) can load the new shared object (v. N). See TestScripts/cryptest-symbols.sh.

@noloader
Copy link
Collaborator

noloader commented Aug 26, 2022

@abdes,

I did run a build with -fPIC and -fvisibility=default with g++ on Linux, and tried to link cryptest to the shared library produced. Many undefined symbols... pretty much half of the library. it does not seem like it's only a windows problem.

If you jump over to Linux and use the Makefile, you can see what the library does. The command output will show you the flags we use to build things for Linux. It is not much more than:

make distclean
LINK_LIBRARY=libcryptopp.so make cryptest.exe shared
LD_LIBRARY_PATH="$PWD:$LD_LIBRARY_PATH" ./cryptest.exe v

Here's the result on my Kubuntu machine:

cryptopp-shared-object

We don't mess around with RPATHS or RUNPATHS. The user can add them if he/she wishes. But a sharp edge is Musl. Musl fails to load a shared object in the case a RPATH or RUNPATH does not exist. All it takes is one bad path and Musl throws away the entire path string. If you are using RPATHS or RUNPATHS, then be sure to test on Alpine Linux (which uses Musl by default).

@chausner
Copy link
Collaborator

Another difference to noloader/cryptopp-cmake that I am seeing is that the previous version installed CMake packages under lib/cmake/cryptopp while now they get installed to share/cmake/cryptopp. Is that change intentional?

@Vollstrecker
Copy link
Collaborator

When looking at the last build in noloader, the package is in share also, I didn't check, but if this changed somewhen in the past, it was before the fork.

@abdes
Copy link
Owner Author

abdes commented Sep 10, 2022

It's not a breaking change type of difference. I looked at tte most common practices with cmake packages and both practices are common. CMake is able to find packages in both places.

@abdes
Copy link
Owner Author

abdes commented Sep 19, 2022

CRYPTOPP_8_7_0 released.
This issue will now be closed, and we'll open a new discussion for next release.
Thank to all who contributed to this massive clean-up. We now have a much more stable cmake build for crypto++.

@abdes abdes closed this as completed Sep 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants