Bug 212254 - security/openconnect: fix build with LibreSSL, add LZ4 option
Summary: security/openconnect: fix build with LibreSSL, add LZ4 option
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Only Me
Assignee: Ryan Steinmetz
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-08-30 07:41 UTC by Piotr Kubaj
Modified: 2016-08-31 22:34 UTC (History)
3 users (show)

See Also:
pkubaj: maintainer-feedback?


Attachments
patch (3.36 KB, patch)
2016-08-30 07:41 UTC, Piotr Kubaj
pkubaj: maintainer-approval? (zi)
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Piotr Kubaj freebsd_committer freebsd_triage 2016-08-30 07:41:38 UTC
Created attachment 174209 [details]
patch

Openconnect 7.07 fails to build with LibreSSL. The reason is missing checks for LIBRESSL_VERSION_NUMBER.

Openconnect also builds blindly against liblz4.so if it's present, so I've added the option to use it or not.
Comment 1 Piotr Kubaj freebsd_committer freebsd_triage 2016-08-30 07:43:56 UTC
Builds fine in Poudriere on 10.3.
Comment 2 Ryan Steinmetz freebsd_committer freebsd_triage 2016-08-30 16:54:26 UTC
Has this been given to the openconnect developer for review?

I don't have a huge interest in maintaining local patches if he hasn't accepted it and committed it upstream.
Comment 3 Piotr Kubaj freebsd_committer freebsd_triage 2016-08-30 18:42:40 UTC
(In reply to Ryan Steinmetz from comment #2)
I've sent it to OC's developers but there's been no response yet.

The patch is simple and quite straightforward. LibreSSL defines OPENSSL_VERSION_NUMBER as 0x20000000L and there were changes in OpenSSL API in versions 1.0.2 and 1.1.0, which is why there is:
#if OPENSSL_VERSION_NUMBER < 0x10100000L
#if OPENSSL_VERSION_NUMBER < 0x10002000L

The patch doesn't change anything for OpenSSL and only fixes the build for LibreSSL.
Comment 4 Piotr Kubaj freebsd_committer freebsd_triage 2016-08-30 18:48:21 UTC
Adding brnrd@, the LibreSSL master so that he can review my patch :)
Comment 5 Ryan Steinmetz freebsd_committer freebsd_triage 2016-08-30 23:57:58 UTC
Please contact David Woodhouse--he's very active and committed changes to the openconnect tree today.

The triviality of the patch isn't of huge consequence to me.  I want the developer(s) of the tool to accept the patches into their tree before I adopt them.
Comment 6 dwmw2 2016-08-31 13:02:02 UTC
I'm not keen on this patch. It makes things *build* but it doesn't work and it fails even 'make check'.

(Btw, please add 'make check' to your build process by default).

It looks like LibreSSL has removed the support for Cisco DTLS that OpenConnect needs. It's only a trivial variation in the way the Finished MAC is calculated, and the Change Cipher Spec message. And we have unit tests for it now. But it's gone.

So on the whole I'd *very* much prefer that people aren't building against LibreSSL. If OpenSSL isn't available, perhaps build against GnuTLS instead?
Comment 7 Ryan Steinmetz freebsd_committer freebsd_triage 2016-08-31 13:10:56 UTC
Upstream/lead developer has indicated that this patch breaks things.  Patch/PR rejected.
Comment 8 Piotr Kubaj freebsd_committer freebsd_triage 2016-08-31 13:12:09 UTC
(In reply to Ryan Steinmetz from comment #7)
You could at least accept the LZ4 option.
Comment 9 commit-hook freebsd_committer freebsd_triage 2016-08-31 13:23:06 UTC
A commit references this bug:

Author: zi
Date: Wed Aug 31 13:22:30 UTC 2016
New revision: 421160
URL: https://svnweb.freebsd.org/changeset/ports/421160

Log:
  - Add LZ4 OPTION

  PR:		212254
  Submitted by:	Piotr Kubaj

Changes:
  head/security/openconnect/Makefile
Comment 10 Bernard Spil freebsd_committer freebsd_triage 2016-08-31 16:58:31 UTC
Hi David,

Thanks for the response.

`make check` doesn't do anything here, I'm not seeing any tests in the tests dir.

Ryan: Running tests by default isn't proper for ports. I added

> TEST_TARGET= check 

to the ports' Makefile but that doesn't run any tests either

Cheers,

Bernard.
Comment 11 dwmw2 2016-08-31 17:08:19 UTC
(In reply to Bernard Spil from comment #10)

Ah, right. The really interesting test was added in git after the 7.07 release. But when you're submitting patches upstream you should be generating and testing them against git master... :)
Comment 12 Piotr Kubaj freebsd_committer freebsd_triage 2016-08-31 17:11:19 UTC
(In reply to dwmw2 from comment #11)
I have OpenConnect 7.06 working with LibreSSL 2.4.2 right now. It works just fine, even though there are DTLS-related errors when connecting (but it connects anyway).

Is it possible to disable DTLS tests or even disable DTLS support compile-time?
Comment 13 dwmw2 2016-08-31 18:18:56 UTC
(In reply to Piotr Kubaj from comment #12)

It doesn't work fine. http://sites.inka.de/bigred/devel/tcp-tcp.html

I'm happy for you that you got it to build, but please don't enable the port to build against LibreSSL and please don't encourage anyone to use it this way without functional DTLS support.

Fix LibreSSL, or build the port against OpenSSL or GnuTLS.
Comment 15 Ryan Steinmetz freebsd_committer freebsd_triage 2016-08-31 22:34:06 UTC
David will release a new version in the near future and the patches will be included.

Gonna close this.