[tegrarcm PATCH] Don't assume cryptopp is system-wide installed

[tegrarcm PATCH] Don't assume cryptopp is system-wide installed

[Thomas Petazzoni]

The current build system adds "-isystem /usr/include/$(CRYPTOLIB)" to
AM_CPPFLAGS, but this is wrong because cryptopp might not be installed
in this location. Instead, the build system should simply include
<cryptopp/...> or <crypto++/...> and rely on the compiler include
path.

The tricky part is that it can be <cryptopp/...> or <crypto++/...>. To
solve this, we use a solution similar to the one used in
https://github.com/bingmann/crypto-speedtest/blob/master/m4/cryptopp.m4
and
https://github.com/bingmann/crypto-speedtest/blob/master/src/speedtest_cryptopp.cpp:
the configure script fills in a variable called
CRYPTOLIB_HEADER_PREFIX, and we use that in the C++ code to include
the right header file.

It is worth mentioning that doing #include
<CRYPTOLIB_HEADER_PREFIX/foobar.h> doesn't work, and we have to use an
intermediate #define'd constant to overcome this C preprocessor
limitation.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 configure.ac     |  1 +
 src/Makefile.am  |  2 +-
 src/aes-cmac.cpp | 28 +++++++++++++++++-----------
 3 files changed, 19 insertions(+), 12 deletions(-)

diff --git a/configure.ac b/configure.ac
index 943654f..620e158 100644
--- a/configure.ac
+++ b/configure.ac
@@ -33,6 +33,7 @@ AC_LINK_IFELSE(
 		[AC_MSG_ERROR([libcrypto++/libcryptopp is not installed.])])]
 )
 AC_SUBST(CRYPTOLIB)
+AC_DEFINE_UNQUOTED([CRYPTOLIB_HEADER_PREFIX], [$CRYPTOLIB], [Location of cryptolib header])
 LDFLAGS=$SAVED_LDFLAGS
 AC_LANG(C)
 
diff --git a/src/Makefile.am b/src/Makefile.am
index 3dad0e6..35a606f 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -1,5 +1,5 @@
 AM_CFLAGS = -Wall -std=c99
-AM_CPPFLAGS = -isystem /usr/include/$(CRYPTOLIB) $(LIBUSB_CFLAGS)
+AM_CPPFLAGS = $(LIBUSB_CFLAGS)
 
 bin_PROGRAMS = tegrarcm
 tegrarcm_SOURCES = \
diff --git a/src/aes-cmac.cpp b/src/aes-cmac.cpp
index 24c89f8..da8be5a 100644
--- a/src/aes-cmac.cpp
+++ b/src/aes-cmac.cpp
@@ -26,6 +26,9 @@
  * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
  * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
+
+#include "config.h"
+
 #include <iostream>
 using std::cout;
 using std::cerr;
@@ -40,26 +43,29 @@ using std::string;
 #include <cstdlib>
 using std::exit;
 
-#include "cryptlib.h"
-using CryptoPP::Exception;
+#define CRYPTOPP_INCLUDE_CRYPTLIB	<CRYPTOLIB_HEADER_PREFIX/cryptlib.h>
+#define CRYPTOPP_INCLUDE_CMAC		<CRYPTOLIB_HEADER_PREFIX/cmac.h>
+#define CRYPTOPP_INCLUDE_AES		<CRYPTOLIB_HEADER_PREFIX/aes.h>
+#define CRYPTOPP_INCLUDE_HEX		<CRYPTOLIB_HEADER_PREFIX/hex.h>
+#define CRYPTOPP_INCLUDE_FILTERS	<CRYPTOLIB_HEADER_PREFIX/filters.h>
+#define CRYPTOPP_INCLUDE_SECBLOCK	<CRYPTOLIB_HEADER_PREFIX/secblock.h>
 
-#include "cmac.h"
-using CryptoPP::CMAC;
+#include CRYPTOPP_INCLUDE_CRYPTLIB
+#include CRYPTOPP_INCLUDE_CMAC
+#include CRYPTOPP_INCLUDE_AES
+#include CRYPTOPP_INCLUDE_HEX
+#include CRYPTOPP_INCLUDE_FILTERS
+#include CRYPTOPP_INCLUDE_SECBLOCK
 
-#include "aes.h"
+using CryptoPP::Exception;
+using CryptoPP::CMAC;
 using CryptoPP::AES;
-
-#include "hex.h"
 using CryptoPP::HexEncoder;
 using CryptoPP::HexDecoder;
-
-#include "filters.h"
 using CryptoPP::StringSink;
 using CryptoPP::StringSource;
 using CryptoPP::HashFilter;
 using CryptoPP::HashVerificationFilter;
-
-#include "secblock.h"
 using CryptoPP::SecByteBlock;
 
 extern "C" int cmac_hash(const unsigned char *msg, int len, unsigned char *cmac_buf)
-- 
2.6.4

Re: [tegrarcm PATCH] Don't assume cryptopp is system-wide installed

[Stephen Warren]

On 04/19/2016 02:25 PM, Thomas Petazzoni wrote:
> The current build system adds "-isystem /usr/include/$(CRYPTOLIB)" to
> AM_CPPFLAGS, but this is wrong because cryptopp might not be installed
> in this location. Instead, the build system should simply include
> <cryptopp/...> or <crypto++/...> and rely on the compiler include
> path.
>
> The tricky part is that it can be <cryptopp/...> or <crypto++/...>. To
> solve this, we use a solution similar to the one used in
> https://github.com/bingmann/crypto-speedtest/blob/master/m4/cryptopp.m4
> and
> https://github.com/bingmann/crypto-speedtest/blob/master/src/speedtest_cryptopp.cpp:
> the configure script fills in a variable called
> CRYPTOLIB_HEADER_PREFIX, and we use that in the C++ code to include
> the right header file.
>
> It is worth mentioning that doing #include
> <CRYPTOLIB_HEADER_PREFIX/foobar.h> doesn't work, and we have to use an
> intermediate #define'd constant to overcome this C preprocessor
> limitation.

I think this looks conceptually OK. CC += Allen to double-check since he 
wrote the original cryptopp autoconf support.

Could you please re-spin this on top of the latest git commits? There's 
a new file rsa-pss.cpp that needs to be updated too. Without that, this 
patch causes compile failures.

> diff --git a/src/aes-cmac.cpp b/src/aes-cmac.cpp

> -#include "cryptlib.h"
> -using CryptoPP::Exception;
> +#define CRYPTOPP_INCLUDE_CRYPTLIB	<CRYPTOLIB_HEADER_PREFIX/cryptlib.h>
> +#define CRYPTOPP_INCLUDE_CMAC		<CRYPTOLIB_HEADER_PREFIX/cmac.h>
> +#define CRYPTOPP_INCLUDE_AES		<CRYPTOLIB_HEADER_PREFIX/aes.h>
> +#define CRYPTOPP_INCLUDE_HEX		<CRYPTOLIB_HEADER_PREFIX/hex.h>
> +#define CRYPTOPP_INCLUDE_FILTERS	<CRYPTOLIB_HEADER_PREFIX/filters.h>
> +#define CRYPTOPP_INCLUDE_SECBLOCK	<CRYPTOLIB_HEADER_PREFIX/secblock.h>
>
> -#include "cmac.h"
> -using CryptoPP::CMAC;
> +#include CRYPTOPP_INCLUDE_CRYPTLIB
> +#include CRYPTOPP_INCLUDE_CMAC
> +#include CRYPTOPP_INCLUDE_AES
> +#include CRYPTOPP_INCLUDE_HEX
> +#include CRYPTOPP_INCLUDE_FILTERS
> +#include CRYPTOPP_INCLUDE_SECBLOCK

It'd be nice to avoid re-ordering everything; just edit the existing 
lines in place. That'd make the diff smaller, and make it more obvious 
that the only thing changing is the include filenames and not the using 
statements.

Re: [tegrarcm PATCH] Don't assume cryptopp is system-wide installed

[Allen Martin]

On Tue, Apr 19, 2016 at 04:32:46PM -0600, Stephen Warren wrote:
> On 04/19/2016 02:25 PM, Thomas Petazzoni wrote:
> >The current build system adds "-isystem /usr/include/$(CRYPTOLIB)" to
> >AM_CPPFLAGS, but this is wrong because cryptopp might not be installed
> >in this location. Instead, the build system should simply include
> ><cryptopp/...> or <crypto++/...> and rely on the compiler include
> >path.
> >
> >The tricky part is that it can be <cryptopp/...> or <crypto++/...>. To
> >solve this, we use a solution similar to the one used in
> >https://github.com/bingmann/crypto-speedtest/blob/master/m4/cryptopp.m4
> >and
> >https://github.com/bingmann/crypto-speedtest/blob/master/src/speedtest_cryptopp.cpp:
> >the configure script fills in a variable called
> >CRYPTOLIB_HEADER_PREFIX, and we use that in the C++ code to include
> >the right header file.
> >
> >It is worth mentioning that doing #include
> ><CRYPTOLIB_HEADER_PREFIX/foobar.h> doesn't work, and we have to use an
> >intermediate #define'd constant to overcome this C preprocessor
> >limitation.
> 
> I think this looks conceptually OK. CC += Allen to double-check
> since he wrote the original cryptopp autoconf support.

Won't the same problem with the naming happen when we link as well?
(The "-lcryptopp") ?  Can pkg-config help here?

-Allen

Re: [tegrarcm PATCH] Don't assume cryptopp is system-wide installed

[Allen Martin]

On Tue, Apr 19, 2016 at 04:32:46PM -0600, Stephen Warren wrote:
> On 04/19/2016 02:25 PM, Thomas Petazzoni wrote:
> >The current build system adds "-isystem /usr/include/$(CRYPTOLIB)" to
> >AM_CPPFLAGS, but this is wrong because cryptopp might not be installed
> >in this location. Instead, the build system should simply include
> ><cryptopp/...> or <crypto++/...> and rely on the compiler include
> >path.
> >
> >The tricky part is that it can be <cryptopp/...> or <crypto++/...>. To
> >solve this, we use a solution similar to the one used in
> >https://github.com/bingmann/crypto-speedtest/blob/master/m4/cryptopp.m4
> >and
> >https://github.com/bingmann/crypto-speedtest/blob/master/src/speedtest_cryptopp.cpp:
> >the configure script fills in a variable called
> >CRYPTOLIB_HEADER_PREFIX, and we use that in the C++ code to include
> >the right header file.
> >
> >It is worth mentioning that doing #include
> ><CRYPTOLIB_HEADER_PREFIX/foobar.h> doesn't work, and we have to use an
> >intermediate #define'd constant to overcome this C preprocessor
> >limitation.
> 
> I think this looks conceptually OK. CC += Allen to double-check
> since he wrote the original cryptopp autoconf support.

What about the following as an alternative?

--
diff --git a/src/Makefile.am b/src/Makefile.am
index 3dad0e6d5e72..22410b3f81bf 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -1,5 +1,6 @@
 AM_CFLAGS = -Wall -std=c99
-AM_CPPFLAGS = -isystem /usr/include/$(CRYPTOLIB) $(LIBUSB_CFLAGS)
+CRYPTO_PREFIX = $(shell pkg-config --variable=includedir libcrypto++)
+AM_CPPFLAGS = -isystem $(CRYPTO_PREFIX)/$(CRYPTOLIB) $(LIBUSB_CFLAGS)
 
 bin_PROGRAMS = tegrarcm
 tegrarcm_SOURCES = \

Re: [tegrarcm PATCH] Don't assume cryptopp is system-wide installed

[Thomas Petazzoni]

Hello,

On Tue, 19 Apr 2016 16:57:45 -0700, Allen Martin wrote:

> What about the following as an alternative?
> 
> --
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 3dad0e6d5e72..22410b3f81bf 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -1,5 +1,6 @@
>  AM_CFLAGS = -Wall -std=c99
> -AM_CPPFLAGS = -isystem /usr/include/$(CRYPTOLIB) $(LIBUSB_CFLAGS)
> +CRYPTO_PREFIX = $(shell pkg-config --variable=includedir libcrypto++)

Using pkg-config is indeed a much better option. However, there are
several gotchas:

 - First, calling pkg-config like this is not the correct way of using
   pkg-config in autoconf/automake based packages. Instead, you should
   use the PKG_CHECK_MODULES() autoconf macro, which will fill in for
   you the <foo>_CFLAGS and <foo>_LIBS variables. Look at your
   configure.ac, it is already used for the detection of libusb!

 - Second, your logic assumes that libcrypto is called libcrypto++
   while the crux of the matter is that on some platforms it is named
   libcryptopp, on others it's called libcrypto++. Of course, this can
   be solved in the configure.ac by calling PKG_CHECK_MODULES() for
   both.

 - Third, the upstream version of libcrypto++ does *not* install a
   pkg-config file. The Debian packagers have added one, so you
   probably have one on your Debian system, but if you build
   libcrypto++ from source and simply "make install" it, you won't have
   a .pc file that describes it for pkg-config.

The last point is the very reason why my patch didn't switch to simply
use pkg-config.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [tegrarcm PATCH] Don't assume cryptopp is system-wide installed

[Allen Martin]

On Wed, Apr 20, 2016 at 11:27:35AM +0200, Thomas Petazzoni wrote:
> On Tue, 19 Apr 2016 16:57:45 -0700, Allen Martin wrote:
> 
> > What about the following as an alternative?
> > 
> > --
> > diff --git a/src/Makefile.am b/src/Makefile.am
> > index 3dad0e6d5e72..22410b3f81bf 100644
> > --- a/src/Makefile.am
> > +++ b/src/Makefile.am
> > @@ -1,5 +1,6 @@
> >  AM_CFLAGS = -Wall -std=c99
> > -AM_CPPFLAGS = -isystem /usr/include/$(CRYPTOLIB) $(LIBUSB_CFLAGS)
> > +CRYPTO_PREFIX = $(shell pkg-config --variable=includedir libcrypto++)
> 
> Using pkg-config is indeed a much better option. However, there are
> several gotchas:
> 
>  - First, calling pkg-config like this is not the correct way of using
>    pkg-config in autoconf/automake based packages. Instead, you should
>    use the PKG_CHECK_MODULES() autoconf macro, which will fill in for
>    you the <foo>_CFLAGS and <foo>_LIBS variables. Look at your
>    configure.ac, it is already used for the detection of libusb!

Unfortunately, libcrypto++ doesn't export any CFLAGS, so
PKG_CHECK_MODULES() doesn't work:

  arm@chrome~$ pkg-config --cflags libcrypto++
 
  arm@chrome~$ pkg-config --libs libcrypto++
  -lcrypto++  
  arm@chrome~$ pkg-config --print-variables libcrypto++
  exec_prefix
  prefix
  libdir
  includedir
  arm@chrome~$ pkg-config --variable=includedir libcrypto++
  /usr/include


> 
>  - Second, your logic assumes that libcrypto is called libcrypto++
>    while the crux of the matter is that on some platforms it is named
>    libcryptopp, on others it's called libcrypto++. Of course, this can
>    be solved in the configure.ac by calling PKG_CHECK_MODULES() for
>    both.

Looking at configure.ac I think there's another assumption baked in
too.  The compile test uses:

  LDFLAGS="$LDFLAGS -lcryptopp -lpthread"

> 
>  - Third, the upstream version of libcrypto++ does *not* install a
>    pkg-config file. The Debian packagers have added one, so you
>    probably have one on your Debian system, but if you build
>    libcrypto++ from source and simply "make install" it, you won't have
>    a .pc file that describes it for pkg-config.
> 
> The last point is the very reason why my patch didn't switch to simply
> use pkg-config.
> 

Well that sucks.  What about a command line option to configure to
specify the libcrypto++ include path?  My issue here is that if you
have the headers in a non standard location (like $HOME/include), the
compiler probably won't know how to find them either, and the compile
check will fail at configure time.

I'd like to change the -isystem to a plain -I too.  I think I added
that because the source code used #include "foo.h" instead of 
#include <foo.h> and I didn't want to make any source code changes
with the automake changes. 

-Allen

Re: [tegrarcm PATCH] Don't assume cryptopp is system-wide installed

[Thomas Petazzoni]

Hello,

On Thu, 21 Apr 2016 11:13:59 -0700, Allen Martin wrote:

> Unfortunately, libcrypto++ doesn't export any CFLAGS, so
> PKG_CHECK_MODULES() doesn't work:
> 
>   arm@chrome~$ pkg-config --cflags libcrypto++
>  
>   arm@chrome~$ pkg-config --libs libcrypto++
>   -lcrypto++  
>   arm@chrome~$ pkg-config --print-variables libcrypto++
>   exec_prefix
>   prefix
>   libdir
>   includedir
>   arm@chrome~$ pkg-config --variable=includedir libcrypto++
>   /usr/include

Ah, crap. So obviously relying on pkg-config is not an option.

> >  - Second, your logic assumes that libcrypto is called libcrypto++
> >    while the crux of the matter is that on some platforms it is named
> >    libcryptopp, on others it's called libcrypto++. Of course, this can
> >    be solved in the configure.ac by calling PKG_CHECK_MODULES() for
> >    both.
> 
> Looking at configure.ac I think there's another assumption baked in
> too.  The compile test uses:
> 
>   LDFLAGS="$LDFLAGS -lcryptopp -lpthread"

Yes, but no, because:

configure.ac:   [CRYPTOLIB="cryptopp"],
configure.ac:           [CRYPTOLIB="crypto++"],
[...]
src/Makefile.am:tegrarcm_LDADD = $(LIBUSB_LIBS) -l$(CRYPTOLIB) -lpthread

So the proper -lcryptopp or -lcrypto++ is added.

> Well that sucks.  What about a command line option to configure to
> specify the libcrypto++ include path?  My issue here is that if you
> have the headers in a non standard location (like $HOME/include), the
> compiler probably won't know how to find them either, and the compile
> check will fail at configure time.

Yes, of course, and it's my use case. I'm building tegracrm in the
context of a build system, as a native tool (i.e built for the build
machine, not cross-compiled for the target). And for such tools, we
install all host libraries in some non-standard location (since we
don't run as root, obviously).

The solution I proposed keeps the "autodetection" mechanism that is
currently in place.

> I'd like to change the -isystem to a plain -I too.  I think I added
> that because the source code used #include "foo.h" instead of 
> #include <foo.h> and I didn't want to make any source code changes
> with the automake changes. 

Sounds like a good idea.

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com