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

Configure openssl #144

Merged
merged 6 commits into from Jun 22, 2017
Merged

Configure openssl #144

merged 6 commits into from Jun 22, 2017

Conversation

mnencia
Copy link
Contributor

@mnencia mnencia commented Jun 12, 2017

Closes #143

Script autogen.sh will run required GNU Autotools commands.
Copy link
Owner

@adrienverge adrienverge left a comment

Choose a reason for hiding this comment

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

Looks good!

Does the --with-openssl option work on Linux also?

autogen.sh Outdated
@@ -0,0 +1,6 @@
#!/bin/sh
set -ex
Copy link
Owner

Choose a reason for hiding this comment

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

Can you add the u option here? (set -eux)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added it together with the travis CI update

@DimitriPapadopoulos
Copy link
Collaborator

Travis CI configuration file .travis.yml should be updated as well:

script:
  - ./tests/lint/run.sh
  - >
      aclocal && autoconf && automake --add-missing
      && ./configure --prefix=/usr --sysconfdir=/etc
      && make

@mnencia
Copy link
Contributor Author

mnencia commented Jun 12, 2017

The --with-openssl option should work on also on Linux. Just point it to the openssl installation prefix.

@mnencia
Copy link
Contributor Author

mnencia commented Jun 14, 2017

Unfortunately this doesn't work out of the box. I'll debug it asap.

@mnencia
Copy link
Contributor Author

mnencia commented Jun 14, 2017

It fails to detect the location of openssl libraries

root@precise:/tmp/openfortivpn# dpkg -S ssl.so
libssl1.0.0: /lib/x86_64-linux-gnu/libssl.so.1.0.0

@mrbaseman
Copy link
Collaborator

I have noticed that openfortivpn indeed does not link against openssl 1.1.0. I'm still looking for a way to check the openssl version in the configure script.
I have found another source which might be helpful : the autoconf-archive

@mnencia
Copy link
Contributor Author

mnencia commented Jun 14, 2017

I've added the OpenSSL version check to the configure

@DimitriPapadopoulos
Copy link
Collaborator

@mrbaseman Just wondering, what does this mean exactly?

I have noticed that openfortivpn indeed does not link against openssl 1.1.0.

Does this mean that openfortivpn should be compatible with openssl 1.1.0 but currently isn't? If so should openfortivpn be fixed? And if so in the source code or in the build infrastructure only?

@mrbaseman
Copy link
Collaborator

@DimitriPapadopoulos on the long term we should support openssl 1.1.x, but for the current issue I just would check if the user who supplies a path to a non-standard openssl installation tries to use a supported one. Most distributions ship patched versions of 1.0.0, 1.0.1 or 1.0.2, but this will also change when upstream support for these ends. A few enterprise versions probably will still backport patches like they currently do for 0.9.8 but that's probably not the standard case.
In order to support openssl 1.1.0 the code has to be changed. I think the problem is that most structures have been made opaque, but I need a closer look at the compiler errors and compare them with the FAQ or the openssl mailing list archives to tell for sure.

configure.ac Outdated
AC_MSG_CHECKING([that we can build OpenSSL programs])
AC_COMPILE_IFELSE(
[AC_LANG_PROGRAM([#include <openssl/ssl.h>],
[init_openssl_library()])],
Copy link
Collaborator

Choose a reason for hiding this comment

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

I have noticed that the init function should read SSL_library_init() - and also this check seems not to work. It silently goes passes and just writes out a warning into config.log:

warning: implicit declaration of function 'init_openssl_library' [-Wimplicit-function-declaration]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I may be wrong, but I think that we could completely remove this check, because there already is the library check some lines below:

AC_CHECK_LIB([ssl], [SSL_connect], [], [AC_MSG_ERROR([Cannot find libssl.])])

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I decided to keep this test because it is useful to detect and eventual compiler misconfiguration.

@mrbaseman
Copy link
Collaborator

@mnencia thanks for adding the version check. I hope that I find more time for a closer look next week

@mnencia
Copy link
Contributor Author

mnencia commented Jun 16, 2017

I've fixed the openssl library function name that we use in the configure check.

@mrbaseman
Copy link
Collaborator

@mnencia Great! Thanks a lot! I have just tested building against a few openssl installations and it works nicely. I think we can merge this into master. The homebrew formula however refers to version 1.3.1 which doesn't contain this pull request. I think it makes sense to tag a new release 1.3.2 then. @adrienverge and @DimitriPapadopoulos what do you think?

@DimitriPapadopoulos
Copy link
Collaborator

There have been quite a few changes for Mac OS X. Perhaps it's time for a new release indeed.

@adrienverge
Copy link
Owner

Sure, we can release a new version once it's merged.

@mnencia Can you squash your last commit into f81c28a?

@mnencia
Copy link
Contributor Author

mnencia commented Jun 20, 2017

Last commit squashed into 3408e41

@mrbaseman mrbaseman merged commit fffb197 into adrienverge:master Jun 22, 2017
@adrienverge
Copy link
Owner

Thanks!

v1.4.0 released.

@mnencia mnencia deleted the configure-openssl branch June 26, 2017 07:29
@mrbaseman mrbaseman mentioned this pull request Jul 26, 2017
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

Successfully merging this pull request may close these issues.

None yet

4 participants