Comment 1 Tobias Kortkamp 2019-02-23 04:33:39 UTC

Comment on attachment 202182 [details]
e2guardian 5.3.1 port update

Can you please submit unified diffs for port updates?  They are
easier to review and work with than shell archives for updates since
they show what has actually changed in the port.

+DEFAULT_VERSIONS+=ssl=openssl

This is wrong.  Ports can never set DEFAULT_VERSIONS.  DEFAULT_VERSIONS
is a global setting.  What version of SSL to use is for users to
decide.  It's not up to individual ports.

What problem does this solve?

+GH_ACCOUNT=    e2guardian

Remove this.  It's set to the default value.

+PORTVERSION=   5.3
...
+GH_TAGNAME=    v5.3.1

Why not actually set PORTVERSION=5.3.1 ?

Comment 2 Renato Botelho 2019-02-27 14:49:01 UTC

Waiting for unified diff with fixes

Removed the ssl default option from makefile and GH_ACCOUNT as its the same name as the package name.

The GH_TAGNAME is specified because 5.3.1 is a tag, not a branch on e2guardian repo

Created attachment 202425 [details]
e2guardian 5.3.1 port update after changes pointed

Comment 5 Renato Botelho 2019-02-27 21:24:08 UTC

(In reply to marcellocoutinho from comment #3)
Please submit a unified diff instead of a shar file.

Created attachment 202426 [details]
e2guardian 5.3.1 diff port update

Comment 7 Renato Botelho 2019-02-28 13:16:37 UTC

(In reply to marcellocoutinho from comment #6)
The best way to create a diff to submit is using subversion.

Regarding the patch I have a few comments:

- PORTVERSION is 5.3 but GH_TAGNAME is v5.3.1. You should use PORTVERSION=5.3.1 then you can delete GH_TAGNAME line
- Keep USES list alphabetically sorted

Created attachment 202443 [details]
e2guardian 5.3.1 diff port update

Comment 9 Renato Botelho 2019-02-28 16:06:07 UTC

(In reply to marcellocoutinho from comment #8)

- You added 'iconv' to general USES, please remove it from NTLM_USES because now it's redundant.
- You added 'ssl' to general USES, please remove SSL from options since it's now mandatory

Comment 10 Renato Botelho 2019-02-28 16:07:56 UTC

Port is broken when all OPTIONS are UNSET

Making install in src
 /bin/mkdir -p '/wrkdirs/usr/ports/www/e2guardian/work/stage/usr/local/sbin'
  install  -s -m 555 e2guardian '/wrkdirs/usr/ports/www/e2guardian/work/stage/usr/local/sbin'
mv: rename /wrkdirs/usr/ports/www/e2guardian/work/stage/usr/local/etc/e2guardian/lists/exceptionvirusextensionlist.sample to /wrkdirs/usr/ports/www/e2guardian/work/stage/usr/local/etc/e2guardian/lists/contentscanners/: No such file or directory
*** Error code 1

Stop.
make: stopped in /usr/ports/www/e2guardian
build of www/e2guardian | e2guardian-5.3.1 ended at Thu Feb 28 13:07:10 -03 2019
build time: 00:00:43
!!! build failure encountered !!!
[00:00:58] Error: Build failed in phase: stage
[00:00:58] Cleaning up
[00:00:58] Unmounting file systems

Comment 11 Renato Botelho 2019-02-28 16:13:22 UTC

And it's broken with all OPTIONS SET

===========================================================================
====> Running Q/A tests (stage-qa)
====> Checking for pkg-plist issues (check-plist)
===> Parsing plist
===> Checking for items in STAGEDIR missing from pkg-plist
Error: Orphaned: %%ETCDIR%%/contentscanners/avastdscan.conf.sample
===> Checking for items in pkg-plist which are not in STAGEDIR
Error: Missing: %%ETCDIR%%/contentscanners/avastd.conf.sample
===> Error: Plist issues found.
*** Error code 1

without ssl in options it does not compile

Warning: you need USES=ssl
Error: /usr/local/sbin/e2guardian is linked to /usr/local/lib/libssl.so.9 from security/openssl but it is not declared as a dependency
Warning: you need USES=ssl
Error: /usr/local/sbin/e2guardian is linked to /usr/local/lib/libcrypto.so.9 from security/openssl but it is not declared as a dependency
Warning: you need USES=ssl

Comment 13 Renato Botelho 2019-02-28 16:24:59 UTC

(In reply to marcellocoutinho from comment #12)
Yes, I understand SSL is now mandatory. But in this case the port doesn't need to have an option called SSL in OPTIONS_DEFINE

Created attachment 202445 [details]
e2guardian 5.3.1 diff port update

Comment 15 Renato Botelho 2019-02-28 16:38:17 UTC

(In reply to marcellocoutinho from comment #14)
With all OPTIONS set and using AVASTD on AV RADIO pkg-plist is still broken

===> Checking for items in STAGEDIR missing from pkg-plist
Error: Orphaned: %%ETCDIR%%/contentscanners/avastdscan.conf.sample
===> Checking for items in pkg-plist which are not in STAGEDIR
Error: Missing: %%ETCDIR%%/contentscanners/avastd.conf.sample
===> Error: Plist issues found.

configure also complains about unrecognized options:

configure: WARNING: unrecognized options: --enable-dgdebug, --enable-trickledm

Please test it with all options disabled, with all options enabled, and with 3 possible AV options

Comment 16 Renato Botelho 2019-02-28 16:45:15 UTC

You should note on `./configure --help` output that it doesn't use that notation of --enable-foo versus --disable-foo, instead they have --enable-foo=yes or --enable-foo=no.

and some options are --with-foo=on --with-foo=off

To make ports options to work with that you will need to use

${OPTION_NAME}_CONFIGURE_ON= --enable-foo
${OPTION_NAME}_CONFIGURE_OFF= --enable-foo=no
${OPTION_NAME}_CONFIGURE_ON= --with-foo
${OPTION_NAME}_CONFIGURE_OFF= --with-foo=off

Also note that configure output doesn't mention the trickledm option, it's only mentioned on INSTALL file so you need to remove it because it will have no effect.

---------------------------------------

Optional Features:
  --disable-option-checking  ignore unrecognized --enable/--with options
  --disable-FEATURE       do not include FEATURE (same as --enable-FEATURE=no)
  --enable-FEATURE[=ARG]  include FEATURE [ARG=yes]
  --enable-silent-rules   less verbose build output (undo: "make V=1")
  --disable-silent-rules  verbose build output (undo: "make V=0")
  --enable-dependency-tracking
                          do not reject slow dependency extractors
  --disable-dependency-tracking
                          speeds up one-time build
  --enable-static-zlib[=no]
                          Enable static linking of zlib
  --enable-pcre[=yes]     Enable support for the PCRE library
  --enable-lfs[=yes]      Enable large file support on 32 bit systems
  --enable-orig-ip[=no]   Enable support for checking the client's original
                          destination IP address against HTTP request details
                          when deployed as a transparent proxy (US-CERT
                          VU#435052). Currently only works on Linux.
  --enable-clamd[=no]     Enable support for the ClamD content scanner
  --enable-avastd[=no]    Enable support for the AvastD content scanner
  --enable-icap[=no]      Enable support for ICAP AV server content scanner
  --enable-kavd[=no]      Enable support for the Kaspersky AV daemon content
                          scanner
  --enable-commandline[=no]
                          Enable support for command-line content scanners
  --enable-ntlm[=no]      Enable support for the NTLM auth plugin
  --enable-sslmitm[=no]   Enable support for the SSL MITM plugin
  --enable-dnsauth[=no]   Enable support for the DNS auth plugin
  --enable-email[=no]     Enable support for email reporting functionality

Optional Packages:
  --with-PACKAGE[=ARG]    use PACKAGE [ARG=yes]
  --without-PACKAGE       do not use PACKAGE (same as --with-PACKAGE=no)
  --with-zlib[=NONE]      non-standard search path for zlib library
  --with-dgdebug[=off]    switch on debug build mode
  --with-newdebug[=off]   switch off debuglevel build mode
  --with-netdebug[=off]   switch on netdebug build mode
  --with-redebug[=off]    switch on redebug build mode
  --with-proxyuser[=nobody]
                          name of proxy user
  --with-proxygroup[=nobody]
                          name of proxy group
  --with-piddir[=${localstatedir}/run]
                          path for pid file
  --with-logdir[=${localstatedir}/log/${PACKAGE_NAME}]
                          path for log files
  --with-libiconv[=NONE]  Specify search path on a system which requires an
                          external iconv library (only used in conjunction
                          with NTLM auth plugin).
  --with-sysconfsubdir[=e2guardian]
                          subdirectory under sysconfdir in which to place
                          config files

Comment 17 Renato Botelho 2019-02-28 16:54:45 UTC

(In reply to Renato Botelho from comment #16)
also note what it says about iconv option

  --with-libiconv[=NONE]  Specify search path on a system which requires an
                          external iconv library (only used in conjunction
                          with NTLM auth plugin).

so the way it is today, adding iconv as a dependency only when NTLM option is selected makes sense.

Probably you just need to rework the way options define configure parameters and it will be good.

Comment 18 Renato Botelho 2019-02-28 16:55:32 UTC

(In reply to Renato Botelho from comment #16)
just ignore my --enable/--disable comment. I was wrong, sorry

Comment 19 Renato Botelho 2019-02-28 17:33:33 UTC

Created attachment 202447 [details]
e2guardian 5.3.1 patch

Here is a patch that passes all tests on amd64 for your approval. I'll test other archs now

Comment on attachment 202447 [details]
e2guardian 5.3.1 patch

replacing 

SSL_LDFLAGS=                    -lssl -lcrypto
SSL_CFLAGS=                     -D__SSLMITM -D__SSLCERT

with
SSL_CONFIGURE_ENV=             OPENSSL_LIBS="-lssl -lcrypto"
                               OPENSSL_CFLAGS="-I${OPENSSLINC} -L${OPENSSLLIB}"

prevents ssl interception compiling

I get this message at the end of make messages:
Warning: you may not need USES=ssl

Also, the patch is not including Tricke download manager
OPTIONS_DEFINE= CLISCAN ICAP NTLM DNS EMAIL DEBUG DOCS SSL TRICKLE
OPTIONS_DEFAULT=CLAMD DNS TRICKLE
TRICKLE_DESC=   Enable the trickle download manager
TRICKLE_CONFIGURE_ENABLE=       trickledm

Created attachment 202454 [details]
e2guardian 5.3.1 port update

Take a look on this one. It includes garga's and mine as well.

Comment 22 Renato Botelho 2019-02-28 20:59:35 UTC

(In reply to marcellocoutinho from comment #20)

Regarding SSL MITM what kind of issues did you see? Could you attach a full log? On my local tests it works perfectly

Configure says:
checking for SSL MITM support... yes
checking for OPENSSL... yes

Regarding Tricke download manager, that parameter --enable-trickledm is not valid. There is no mention of the word trickle on configure script:

❯ grep -i trickle configure
❯ ./configure --help |& grep -i trickle
❯

And it makes it an invalid option. I can't get the reason to add it this way.

I've also checked their repo and based on these 2 commits fancy and trickle download managers were disabled:

https://github.com/e2guardian/e2guardian/commit/3f0b8ebfa8184dba21f7e65858762f9b427ce76a#diff-67e997bcfdac55191033d57a16d1408a

https://github.com/e2guardian/e2guardian/commit/ca543af883f253c51e620c6ddaf24f582d10b62d#diff-67e997bcfdac55191033d57a16d1408a

Created attachment 202460 [details]
make output with ssl_config_env options on makefile

(In reply to Renato Botelho from comment #22)

this is the make output with ssl_config_env options you added and the alert at the end about ssl.

I've tested removing the ssl_use = ssl and ssl selected on make config. It compiles without ssl as it does not complain about missing ssl options.

Created attachment 202461 [details]
make output with current compile options I use since e2guardian v3

(In reply to Renato Botelho from comment #22)

make output with current compile options I use since e2guardian v3

Created attachment 202462 [details]
e2guardian 5.3.1 port update another try

(In reply to Renato Botelho from comment #22)

Created attachment 202463 [details]
e2guardian 5.3.1 port update another try

(In reply to Renato Botelho from comment #22)

Comment 27 Renato Botelho 2019-03-01 11:29:37 UTC

(In reply to marcellocoutinho from comment #23)
Both outputs you sent have this on configure:

checking for SSL MITM support... no

Are you sure you have the following line defined?

SSL_CONFIGURE_ENABLE=sslmitm

When it's defined, --enable-sslmitm will be passed to configure script and it will end up adding the same defines current version of the port are doing now. Can you please send the output of following commands on the port with my patch applied, in the environment you are able to reproduce a problem?

# uname -a
# cat Makefile
# make WITH=SSL -V CONFIGURE_ARGS
# make WITH=SSL -V CONFIGURE_ENV
# make WITH=SSL -V configure

Then attach the output of these commands and also ./work/e2guardian-5.3.1/config.log

I'm just trying to figure out if upstream code has a bug in this regard, and if so I would like to submit them a patch.

And just a side note, I tested that patch on FreeBSD 12 (i386 and amd64), 11 (amd64) and all of them worked as expected detecting OpenSSL, linking binary with libssl and libcrypto.

Comment 28 Renato Botelho 2019-03-01 16:00:49 UTC

Created attachment 202481 [details]
5.3.1 final patch

Final patch after a private discussion with Marcello. It also changes SSL option name to SSL_MITM since it makes more sense.

The problem on Marcello's test was a typo on OPTION configure clause that made it to end up missing a configure parameter

Comment 29 commit-hook 2019-03-01 16:14:19 UTC

A commit references this bug:

Author: garga
Date: Fri Mar  1 16:14:14 UTC 2019
New revision: 494302
URL: https://svnweb.freebsd.org/changeset/ports/494302

Log:
  www/e2guardian: Update to 5.3.1

  - Update to 5.3.1
  - Create a RADIO option for AV items
  - Rename SSL option to SSL_MITM to reflect what it really does
  - Fix DEBUG option to pass expected parameter to configure
  - Use more standard way to enable SSL_MITM
  - Fix install when no AV options are selected

  PR:		235879
  Submitted by:	Marcello Coutinho <marcellocoutinho@gmail.com> (based on)
  Approved by:	Marcello Coutinho <marcellocoutinho@gmail.com> (maintainer)

Changes:
  head/www/e2guardian/Makefile
  head/www/e2guardian/distinfo
  head/www/e2guardian/pkg-plist

Hello.

When I try to build e2guardian5.3.1 without debug option, it builds with debug support.
Details available below:
/usr/ports/www/e2guardian % make showconfig
===> The following configuration options are available for e2guardian-5.3.1:
     CLISCAN=off: Enable support for CLI content scanners
     DEBUG=off: Enable debug build mode
     DNS=on: Include DNS authetication plugin
     DOCS=on: Build and/or install documentation
     EMAIL=off: Enable e-mail reporting support
     ICAP=off: Enable ICAP AV content scanner support
     NTLM=off: Include NTLM authentication plugin
     SSL_MITM=off: Enable support for the SSL MITM plugin
====> Options available for the radio AV: you can only select none or one of them
     AVAST=off: Enable AvastD content scanner
     CLAMD=on: Enable ClamD AV content scanner
     KAV=off: Enable Kaspersky AV support
===> Use 'make config' to modify these settings

When I run:
% /usr/local/sbin/e2guardian
Running in debug mode...
...

Comment 31 Renato Botelho 2019-03-07 14:53:13 UTC

Reopening due to regression report

Comment 32 Renato Botelho 2019-03-07 14:55:07 UTC

Created attachment 202686 [details]
e2guardian fix debug

configure script doesn't deal with --without-dgdebug, it needs --with=dgdebug=off to really disable it. While here also disable --with-newdebug when option is not selected.

Comment 33 commit-hook 2019-03-07 15:46:22 UTC

A commit references this bug:

Author: garga
Date: Thu Mar  7 15:45:58 UTC 2019
New revision: 494959
URL: https://svnweb.freebsd.org/changeset/ports/494959

Log:
  www/e2guardian: Fix DEBUG option

  e2guardian configure script doesn't respect --without-dgdebug option and
  end up building binary with debug support since it was upgraded to 5.3.1.
  Pass --with-dgdebug=off explicit when DEBUG option is not set.  While
  here, include --with-newdebug parameter to make sure all debug will be
  disabled.

  PR:		235879
  Approved by:	Marcello Coutinho <marcellocoutinho@gmail.com> (maintainer)
  Sponsored by:	Rubicon Communications, LLC (Netgate)

Changes:
  head/www/e2guardian/Makefile

Comment 34 Renato Botelho 2019-03-07 15:46:46 UTC

Fixed in 5.3.1_1