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

X509_check_host detection always fails #304

Closed
tadeboro opened this Issue May 11, 2018 · 33 comments

Comments

Projects
None yet
3 participants
@tadeboro
Copy link
Contributor

tadeboro commented May 11, 2018

Configuration process fails to detect X509_check_host function even on systems that have it defined.

Steps to reproduce

  1. Ensure we have proper verison of OpenSSL using pkg-config --modversion libssl (should return 1.1.0 or 1.0.2).
  2. Clone the repository.
  3. Run ./autogen.sh.
  4. Run ./configure.sh.
  5. One of the last lines in the output is checking for X509_check_host... no.

This was tested on Fedora 27 (OpenSSL 1.1.0h) and unstable Gentoo (OpenSSL 1.0.2o).

What is causing the issue

Main reason for this issue is the fact that PKG_CHECK_MODULES does not add proper linker flags to the set of flags that configure is using to check for function presence. This can be seen in the config.log file:

configure:5102: checking for X509_check_host
configure:5102: gcc -o conftest -g -O2   conftest.c -lutil -lpthread  >&5
/tmp/cckiBJJz.o: In function `main':
/home/tadej/devel/openfortivpn/conftest.c:184: undefined reference to `X509_check_host'
collect2: error: ld returned 1 exit status

In the second line, -lssl and -lcrypto linker flags are missing.

How to fix this

Initial stab at fixing this issue in in pull request #303

@DimitriPapadopoulos

This comment has been minimized.

Copy link
Collaborator

DimitriPapadopoulos commented May 11, 2018

I've merged your pull request as it doesn't seem to be breaking anything.

But why "initial stab"? Is there anything else we might need to change?

@tadeboro

This comment has been minimized.

Copy link
Contributor

tadeboro commented May 11, 2018

@DimitriPapadopoulos That "initial stab" was there because it has been almost 5 years since I last dabbled with autotools machinery seriously. I did my best to verify the changes before sending a PR, but added that just to be safe;)

@DimitriPapadopoulos

This comment has been minimized.

Copy link
Collaborator

DimitriPapadopoulos commented May 11, 2018

Excellent then.

It just occurred to me that we may need to check a couple things:

  1. How does this behave on macOS? @mnencia Does it still build on macOS or do you need to change the homebrew recipe?
  2. Is pkg-config still useful? @mrbaseman Should we modify configure.ac by removing the relevant lines since this should be entirely handled by autoconf internally? m4_ifndef([PKG_PROG_PKG_CONFIG], [m4_fatal([Please install pkg-config.])]) PKG_PROG_PKG_CONFIG
@DimitriPapadopoulos

This comment has been minimized.

Copy link
Collaborator

DimitriPapadopoulos commented May 12, 2018

I had to revert 79f52ef as building openfortivpn based on pkg-config output is expected to work on macOS and perhaps other platforms. Indeed on macOS the system OpenSSL library is an obsolete compatibility library which seems to be lacking headers. On macOS openfortivpn requires an alternative OpenSSL library at build time and setting PKG_CONFIG_PATH looks like a good way to point to the alternative library.

In short we do need PKG_CHECK_MODULES() to check PKG_CONFIG_PATH.

We need an alternative solution to fix the AC_CHECK_FUNCS() failure - perhaps a call to AC_CHECK_LIB() in addition to PKG_CHECK_MODULES().

@DimitriPapadopoulos

This comment has been minimized.

Copy link
Collaborator

DimitriPapadopoulos commented May 12, 2018

It looks like autotools are a little bit broken/inconsistent here:

  • PKG_CHECK_MODULES() does set/report a few variables in config.log:
pkg_cv_OPENSSL_CFLAGS=
pkg_cv_OPENSSL_LIBS='-lcrypto -lssl'
OPENSSL_CFLAGS=''
OPENSSL_LIBS='-lcrypto -lssl'
  • AC_CHECK_FUNCS() does not use them:
configure:5114: gcc -o conftest -g -O2   conftest.c -lutil -lpthread  >&5
/tmp/cc4iZwHK.o: In function `main':
/volatile/src/openfortivpn/conftest.c:184: undefined reference to `X509_check_host'
collect2: error: ld returned 1 exit status
@DimitriPapadopoulos

This comment has been minimized.

Copy link
Collaborator

DimitriPapadopoulos commented May 12, 2018

I see two solutions:

  1. use AC_CHECK_LIB() in addition to PKG_CHECK_MODULES(),
  2. get AC_CHECK_FUNCS() to honour variables from PKG_CHECK_MODULES(), probably by passing explicitly a few arguments to AC_CHECK_FUNCS().
@DimitriPapadopoulos

This comment has been minimized.

Copy link
Collaborator

DimitriPapadopoulos commented May 12, 2018

I cannot find documentation on solution 2, AC_CHECK_FUNCS() doesn't take additional arguments.

Let's go for solution 1.

@DimitriPapadopoulos

This comment has been minimized.

Copy link
Collaborator

DimitriPapadopoulos commented May 12, 2018

The problem with solution 1 is that:

  • AC_CHECK_FUNCS() does not use the header and library search paths provided by pkg-config,
  • the openfortivpn build instructions do take in to account the header and library search paths provided by pkg-config.

So these two different build steps may test / build against different headers/libraries!

Either we take the solution of @tadeboro and abandon PKG_CHECK_MODULES(), or we find a way to reconcile PKG_CHECK_MODULES() and AC_CHECK_FUNCS().

@DimitriPapadopoulos

This comment has been minimized.

@DimitriPapadopoulos

This comment has been minimized.

Copy link
Collaborator

DimitriPapadopoulos commented May 12, 2018

Let's try solution 2.

It looks like adding:

CPPFLAGS="${CPPFLAGS} ${OPENSSL_CFLAGS}"
LIBS="${LIBS} ${OPENSSL_LIBS}"

right after

PKG_CHECK_MODULES(OPENSSL, [libcrypto libssl])

enforces the relevant header and library search paths are taken into account both when running AC_CHECK_FUNCS() and during actual compilation.

@DimitriPapadopoulos

This comment has been minimized.

Copy link
Collaborator

DimitriPapadopoulos commented May 12, 2018

By the way, I'm putting OPENSSL_CLFAGS into CPPFLAGS because of this remark on PKG_CHECK_MODULES():

By default, the macro will set up two variables, joining the given prefix with the suffixes _CFLAGS and _LIBS. The names of these variables can be somewhat misleading, since the former will generally provide the flags to pass to the preprocessor, rather than the compiler, such as include paths and macro definitions, and the latter will provide the library paths as well as the libraries themselves.

@tadeboro

This comment has been minimized.

Copy link
Contributor

tadeboro commented May 13, 2018

What about if openfortivpn would simply require OpenSSL >= 1.0.2? This version of OpenSSL was released in January 2015 and is present in most currently supported distributions and OSX. I checked:

  1. Ubuntu has this from Trusty (released in April 2014).
  2. Debian has this from Jessie (released in April 2015).
  3. Fedora has this form 23 (released in November 2015).
  4. Gentoo has this in stable tree.
  5. Homebrew for OSX has latest stable (1.0.2o) already packaged.

This way, we can simply set the minimum version directly in the PKG_CHECK_MODULES, get rid of the x509_check_host function check and remove the string comparison check in tunnel.c file.

Any thoughts about this?

@DimitriPapadopoulos

This comment has been minimized.

Copy link
Collaborator

DimitriPapadopoulos commented May 13, 2018

@mrbaseman has recently changed the whole autotools machinery to check for each required function and fail if any required function is missing - or define HAVE_FUNCTION if an optional function is available.
We cannot avoid checking X509_check_host() in AC_CHECK_FUNCS(), either as an optional or as required function.

@tadeboro

This comment has been minimized.

Copy link
Contributor

tadeboro commented May 13, 2018

Current autotools machinery in openfortivpn only performs checks for functions that reside in either standard library or in one of the libraries that we find using AC_CHECK_LIB macro. None of the OpenSSL functions are being checked for, with the exception of X509_check_host which this is meant to be an optional dependency. If OpenSSL functions were checked for using AC_CHECK_FUNCS, the configure would fail because some linker flags would be missing (just like described in initial bug report).

If we require OpenSSL to be of version 1.0.2 or higher, we can rely on X509_check_host being available just like we already do with other OpenSSL functions. This would make it possible to remove the preprocessor if in tunnel.c and always use x509_check_host. And since HAVE_X509_CHECK_HOST define is only used in this particular place, we can also remove the offending AC_CHECK_FUNCS call, resolving our troubles.

But as said before, this can only be done if maintainers are happy to leave some really old systems not being able to use latest openfortivpn.

@DimitriPapadopoulos

This comment has been minimized.

Copy link
Collaborator

DimitriPapadopoulos commented May 13, 2018

True. Then perhaps your solution is the proper one - check for a recent version of OpenSSL and fail if it's not found. Yes, I agree with you: let's just require a minimal version of OpenSSL and get rid of AC_CHECK_FUNCS([X509_check_host]).

@DimitriPapadopoulos

This comment has been minimized.

Copy link
Collaborator

DimitriPapadopoulos commented May 13, 2018

I vote for the pull request #306 suggested by @tadeboro.

The alternative would be to fix the current solution as in pull request #307, where we take into account compiler/linker flags provided by PKG_CHECK_MODULES not only in Makefile.am but also when calling AC_CHECK_FUNCS in configure.ac.

@DimitriPapadopoulos

This comment has been minimized.

Copy link
Collaborator

DimitriPapadopoulos commented May 13, 2018

Ouch! The Travis CI platform obviously uses an older version of OpenSSL, so we cannot pass the test!

How do we get Travis CI to use a more recent distribution?

@tadeboro

This comment has been minimized.

Copy link
Contributor

tadeboro commented May 13, 2018

We can instruct Travis to run test on Ubuntu Trusty (14.04) by adding dist: trusty line to the .travis.yml file. This should resolve the issue.

@DimitriPapadopoulos

This comment has been minimized.

Copy link
Collaborator

DimitriPapadopoulos commented May 13, 2018

@DimitriPapadopoulos

This comment has been minimized.

Copy link
Collaborator

DimitriPapadopoulos commented May 13, 2018

Besides this doesn't work either:
dist: xenial

Strange. This needs more debugging but I don't have much experience with Travis CI and don't know how to debug.

@tadeboro tadeboro referenced this issue May 13, 2018

Merged

Refactor CI #308

@tadeboro

This comment has been minimized.

Copy link
Contributor

tadeboro commented May 13, 2018

I reworked Travis integration a bit and made sure that we test the compilation against specific version of the OpenSSL package. Those changes should make it possible to resolve issues in pull request #306

@DimitriPapadopoulos

This comment has been minimized.

Copy link
Collaborator

DimitriPapadopoulos commented May 13, 2018

Ah xenial and bionic are indeed not available on Travis CI and in that case Travis CI fails silently. Let's have a look at #308.

@tadeboro

This comment has been minimized.

Copy link
Contributor

tadeboro commented May 14, 2018

Is there anything else needed in order to resolve this issue one way or another?

@DimitriPapadopoulos

This comment has been minimized.

Copy link
Collaborator

DimitriPapadopoulos commented May 14, 2018

Not at all. I'd just like some more feedback. As far as I'm concerned, I vote for #306, like you.

@mrbaseman

This comment has been minimized.

Copy link
Collaborator

mrbaseman commented May 15, 2018

  • I would vote for #307 because older, but still supported enterprise distros (RHEL/CentOS 6, SLES 11sp4) ship with (heavily patched) older versions of openssl

  • There was also the question if pkg-config is still useful. I think it is a good way to specify the location of some dependencies, e.g. within homebrew.
    First we had a configure option in #144 and switched to pkg-config in #151. Obviously, there were some bits missing for the transition to pkg-config, but all in all I believe it is better than maintaining our own configure switches for openssl.

  • X509_check_host currently is the only openssl function that is explicitly checked, because we have to determine if it exists. All other functions should always be there (assuming that nobody uses an ssl library below 0.9.8 nowadays...). The assumption is that autoconf and pkg-config would find the library and do some basic tests like if compiling and linking works.

@DimitriPapadopoulos

This comment has been minimized.

Copy link
Collaborator

DimitriPapadopoulos commented May 15, 2018

@mrbaseman Let's go for #307 then. Indeed it can be useful to be able to ship a recent version of openfortivpn on older distributions that have not yet reached EOL (CentOS 6 in 2020 and SLESS 11 SP4 in 2022).

Perhaps I'll change:

PKG_CHECK_MODULES(OPENSSL, [libcrypto libssl])

into:

PKG_CHECK_MODULES(OPENSSL, [libcrypto >= 0.9.8 libssl >= 0.9.8])

to explicitly state that we expect OpenSSL >= 0.9.8.

@mrbaseman

This comment has been minimized.

Copy link
Collaborator

mrbaseman commented May 15, 2018

That sounds good. I didn't check the exact version requirement, but >=0.9.8 should be fine.

I'm not sure if I understood your question about pkg-config correctly. If we remove these lines
from configure.ac, does autoconf internally use pkg-config automatically if it finds it and silently skip this internal mechanism if pkg-config is not available?

@DimitriPapadopoulos

This comment has been minimized.

Copy link
Collaborator

DimitriPapadopoulos commented May 15, 2018

Forget my comment about pkg-config - I don't understand it either! Perhaps I had misread some documentation. I really don't know.

@DimitriPapadopoulos

This comment has been minimized.

Copy link
Collaborator

DimitriPapadopoulos commented May 15, 2018

Found the documentation that states that PKG_CHECK_MODULES implicitly calls PKG_PROG_PKG_CONFIG first:

Adding these lines:

m4_ifndef([PKG_PROG_PKG_CONFIG], [m4_fatal([Please install pkg-config.])])
PKG_PROG_PKG_CONFIG

may result in a cleaner error message (Please install pkg-config. instead of PKG_PROG_PKG_CONFIG: command not found) but is it worth the pain?

@DimitriPapadopoulos

This comment has been minimized.

Copy link
Collaborator

DimitriPapadopoulos commented May 15, 2018

See pkg.m4.

@mrbaseman

This comment has been minimized.

Copy link
Collaborator

mrbaseman commented May 16, 2018

I think we need PKG_PROG_PKG_CONFIG in order to be able to use PKG_CHECK_MODULES, otherwise pkg-config might not be found even if it is installed, and therefore information from the .pc files would not be considered. So, if we support pkg-config we should keep PKG_PROG_PKG_CONFIG and the m4_ifndef-line makes the error message more understandable when pkg-config is not installed.

So, your question "is it worth the pain" targets the whole pkg-config interface, including PKG_CHECK_MODULES, right?

The alternative would be going back to something along the lines of #144 but compared to that #307 looks much more elegant.

@DimitriPapadopoulos

This comment has been minimized.

Copy link
Collaborator

DimitriPapadopoulos commented May 16, 2018

As far as I can see PKG_CHECK_MODULES starts by calling call PKG_PROG_PKG_CONFIG. See this line of pkg.m4.

I meant to say that you don't need to call PKG_PROG_PKG_CONFIG explicitly, since you call PKG_CHECK_MODULES which in turn calls PKG_PROG_PKG_CONFIG.

I agree though that the m4_ifndef-line makes the error message more understandable when pkg-config is not installed. My question "is it worth the pain" targets this specific line only, not PKG_CHECK_MODULES.

@mrbaseman

This comment has been minimized.

Copy link
Collaborator

mrbaseman commented May 16, 2018

Ah, now I got your point. hmm...

...but then the "pain" is not very big :-)

it's one line which is redundant (at least in current versions of pkg-config (not sure if this line of pkg.m4 has always been there), and one line which helps finding the problem when pkg-config is not installed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment