[PATCH] build: add option to choose to build with or without JSON audit support

[Björn Baumbach via samba-technical]

Hi!

I've prepared an option to disable or enforce the build with support for
JSON auth audit, which needs the jansson library.

By default the jansson library is auto-detected and is then used for the
JSON auth audit method.

Review is appreciated!

Best regards,
Björn

[Douglas Bagnall via samba-technical]

hi Björn,

Post by Björn Baumbach via samba-technical
I've prepared an option to disable or enforce the build with support for
JSON auth audit, which needs the jansson library.
By default the jansson library is auto-detected and is then used for the
JSON auth audit method.

This is largely reverting 8e54bc463adcfbc322249ba626e2c503ca13a864,
the justification for which was (quoting Andrew Bartlett, 2018-02-11 in

Post by Björn Baumbach via samba-technical

I worry that changes to our build system could mean we run less tests
than we expect, either if the host changes or if the glue between the
two parts breaks, and we might not notice.
So instead I propose to make Samba fail to build if jansson is missing
on the AD DC, and lock in the requirement for TLS support in the test
scripts.

We have ended up before with tests that never get run because they
depend on config options that nobody uses.

Is there some way of meeting those concerns in this patch? Would
squashing this in be acceptable?:

diff --git a/script/autobuild.py b/script/autobuild.py
index e00eebda1c1..0dd52ebbc67 100755
--- a/script/autobuild.py
+++ b/script/autobuild.py
@@ -75,7 +75,8 @@ if os.environ.get("AUTOBUILD_SKIP_SAMBA_O3", "0") == "1":
defaulttasks.remove("samba-o3")

ctdb_configure_params = " --enable-developer --picky-developer ${PREFIX}"
-samba_configure_params = " --picky-developer ${PREFIX} ${EXTRA_PYTHON} --with-profiling-data"
+samba_configure_params = (" --picky-developer ${PREFIX} ${EXTRA_PYTHON}"
+ " --with-profiling-data --with-json-audit")


That lets you run make test without jansson but means autobuild
requires it.

cheers,
Douglas

[Stefan Metzmacher via samba-technical]

Hi Douglas,

Post by Douglas Bagnall via samba-technical

Post by Björn Baumbach via samba-technical
I've prepared an option to disable or enforce the build with support for
JSON auth audit, which needs the jansson library.
By default the jansson library is auto-detected and is then used for the
JSON auth audit method.

This is largely reverting 8e54bc463adcfbc322249ba626e2c503ca13a864,
the justification for which was (quoting Andrew Bartlett, 2018-02-11 in

Post by Björn Baumbach via samba-technical

I worry that changes to our build system could mean we run less tests
than we expect, either if the host changes or if the glue between the
two parts breaks, and we might not notice.
So instead I propose to make Samba fail to build if jansson is missing
on the AD DC, and lock in the requirement for TLS support in the test
scripts.

We have ended up before with tests that never get run because they
depend on config options that nobody uses.
Is there some way of meeting those concerns in this patch? Would

Isn't this part of Björn's patch enough?

+ if conf.CONFIG_GET('ENABLE_SELFTEST') and \
+ (not Options.options.without_ad_dc):
+ raise Utils.WafError('jansson JSON library required for '
+ '--enable-selftest when building the
AD DC')

Post by Douglas Bagnall via samba-technical
diff --git a/script/autobuild.py b/script/autobuild.py
index e00eebda1c1..0dd52ebbc67 100755
--- a/script/autobuild.py
+++ b/script/autobuild.py
defaulttasks.remove("samba-o3")
ctdb_configure_params = " --enable-developer --picky-developer ${PREFIX}"
-samba_configure_params = " --picky-developer ${PREFIX} ${EXTRA_PYTHON} --with-profiling-data"
+samba_configure_params = (" --picky-developer ${PREFIX} ${EXTRA_PYTHON}"
+ " --with-profiling-data --with-json-audit")
That lets you run make test without jansson but means autobuild
requires it.

We don't want make test without jansson, we want to build some packages
without json-audit support, even if jansson is installed an the system.

metze

[Douglas Bagnall via samba-technical]

Post by Stefan Metzmacher via samba-technical

Post by Douglas Bagnall via samba-technical
Is there some way of meeting those concerns in this patch? Would

Isn't this part of Björn's patch enough?
+ if conf.CONFIG_GET('ENABLE_SELFTEST') and \
+ raise Utils.WafError('jansson JSON library required for '
+ '--enable-selftest when building the
AD DC')

Ah, OK, sorry. Thanks for pointing this out Metze.

Post by Stefan Metzmacher via samba-technical

Post by Douglas Bagnall via samba-technical
That lets you run make test without jansson but means autobuild
requires it.

We don't want make test without jansson, we want to build some packages
without json-audit support, even if jansson is installed an the system.

Right. I am curious -- does the dependency cause problems on some platforms?

Douglas

[Stefan Metzmacher via samba-technical]

Post by Douglas Bagnall via samba-technical

Post by Stefan Metzmacher via samba-technical

Post by Douglas Bagnall via samba-technical
Is there some way of meeting those concerns in this patch? Would

Isn't this part of Björn's patch enough?
+ if conf.CONFIG_GET('ENABLE_SELFTEST') and \
+ raise Utils.WafError('jansson JSON library required for '
+ '--enable-selftest when building the
AD DC')

Ah, OK, sorry. Thanks for pointing this out Metze.

Post by Stefan Metzmacher via samba-technical

Post by Douglas Bagnall via samba-technical
That lets you run make test without jansson but means autobuild
requires it.

We don't want make test without jansson, we want to build some packages
without json-audit support, even if jansson is installed an the system.

Right. I am curious -- does the dependency cause problems on some platforms?

No. We just want didn't have jansson-dev in the build environments
before, but want to install it now, but don't build all packages with
the dependency.

metze

[Björn Baumbach via samba-technical]

Post by Björn Baumbach via samba-technical
I've prepared an option to disable or enforce the build with support for
JSON auth audit, which needs the jansson library.

Hi!

My last patch breaks the build, if building without JSON audit support.
The build fails with "Unknown dependency 'jansson' in
'common_auth.objlist'".

Metze has already reviewed the attached patch. I'll push it soon.

Best regards,
Björn

[Andrew Bartlett via samba-technical]

On Tue, 2018-04-17 at 15:59 +0200, Björn Baumbach via samba-technical

Post by Björn Baumbach via samba-technical
Hi!
I've prepared an option to disable or enforce the build with support for
JSON auth audit, which needs the jansson library.
By default the jansson library is auto-detected and is then used for the
JSON auth audit method.
Review is appreciated!

I realise this is already in, but could we please have the default
being True? It is correct that this is the no-change behaviour, but on
reflection we have made many other things bug the user to install
packages or explicitly disable the feature, and doing so would ensure
that more packages and more installs get to use this functionality.

(Because folks doing the build are prompted).

Either way, the smb.conf manpage section also needs to be updated.

Andrew Bartlett

[Björn Baumbach via samba-technical]

Post by Andrew Bartlett via samba-technical
On Tue, 2018-04-17 at 15:59 +0200, Björn Baumbach via samba-technical

Post by Björn Baumbach via samba-technical
Hi!
I've prepared an option to disable or enforce the build with support for
JSON auth audit, which needs the jansson library.
By default the jansson library is auto-detected and is then used for the
JSON auth audit method.
Review is appreciated!

I realise this is already in, but could we please have the default
being True? It is correct that this is the no-change behaviour, but on
reflection we have made many other things bug the user to install
packages or explicitly disable the feature, and doing so would ensure
that more packages and more installs get to use this functionality.
(Because folks doing the build are prompted).
Either way, the smb.conf manpage section also needs to be updated.

Hi Andrew,

I didn't change the behaviour. The default is still the auto-detection.
In my opinion this feature is an optional feature and I am fine with the
current default. I would not like to set the default to True.

In which way would you like to update the man page? The condition "if
Samba is compiled against the jansson JSON library" is still correct here:

Authentication and authorization audit information is logged under
the auth_audit, and if Samba is compiled against the jansson JSON
library, a JSON representation is logged under auth_json_audit.

Best regards,
Björn

[Andrew Bartlett via samba-technical]

Post by Björn Baumbach via samba-technical
Hi Andrew,
I didn't change the behaviour. The default is still the auto-detection.
In my opinion this feature is an optional feature and I am fine with the
current default.

Sure, but in retrospect I'm not. This was missed when we first added
the feature.

Post by Björn Baumbach via samba-technical
I would not like to set the default to True.

Why is that?  

A while back we realised that folks were getting builds missing
features, so we stopped making krb5 and ldap automatic in this way,
requiring an explicit --without-krb5, --without-ldap et al.  

Now that an explicit option is being added, I'm asking that is match
the behaviour of our other 'optional' libraries.

Post by Björn Baumbach via samba-technical
In which way would you like to update the man page? The condition "if
Authentication and authorization audit information is logged under
the auth_audit, and if Samba is compiled against the jansson JSON
library, a JSON representation is logged under auth_json_audit.

How about:

and unless Samba was compiled --without-json-audit, a JSON
representation is logged under auth_json_audit.

Thanks,

Andrew Bartlett

[Björn Baumbach via samba-technical]

Post by Andrew Bartlett via samba-technical

Post by Björn Baumbach via samba-technical
I didn't change the behaviour. The default is still the auto-detection.
In my opinion this feature is an optional feature and I am fine with the
current default.

Sure, but in retrospect I'm not. This was missed when we first added
the feature.

Post by Björn Baumbach via samba-technical
I would not like to set the default to True.

Why is that?  
A while back we realised that folks were getting builds missing
features, so we stopped making krb5 and ldap automatic in this way,
requiring an explicit --without-krb5, --without-ldap et al.  
Now that an explicit option is being added, I'm asking that is match
the behaviour of our other 'optional' libraries.

In my opinion it's not needed to enable all available optional features
by default. There are some additional switches like the
--with-json-audit switch:

--with-gpgme
--with-dmapi
--with-fam
--with-libarchive
--with-regedit

But actually, it's not that important to me. I just like the 'auto'
default more than 'true', because I like to keep it small.

Post by Andrew Bartlett via samba-technical

Post by Björn Baumbach via samba-technical
In which way would you like to update the man page? The condition "if
Authentication and authorization audit information is logged under
the auth_audit, and if Samba is compiled against the jansson JSON
library, a JSON representation is logged under auth_json_audit.

and unless Samba was compiled --without-json-audit, a JSON
representation is logged under auth_json_audit.

My thoughts were "If I compile with --without-json-audit, Samba will not
compiled against the jansson libary as well". But yes, you're right -
mention the --without-json-audit switch is more clear.
Please find a patch attached.

Best regards,
Björn

[Andrew Bartlett via samba-technical]

Post by Björn Baumbach via samba-technical

Post by Andrew Bartlett via samba-technical

Post by Björn Baumbach via samba-technical
I didn't change the behaviour. The default is still the auto-detection.
In my opinion this feature is an optional feature and I am fine with the
current default.

Sure, but in retrospect I'm not. This was missed when we first added
the feature.

Post by Björn Baumbach via samba-technical
I would not like to set the default to True.

Why is that?
A while back we realised that folks were getting builds missing
features, so we stopped making krb5 and ldap automatic in this way,
requiring an explicit --without-krb5, --without-ldap et al.
Now that an explicit option is being added, I'm asking that is match
the behaviour of our other 'optional' libraries.

In my opinion it's not needed to enable all available optional features
by default. There are some additional switches like the
--with-gpgme
--with-dmapi
--with-fam
--with-libarchive
--with-regedit

We should fix those, at least gpgme and libarchive.

Post by Björn Baumbach via samba-technical
But actually, it's not that important to me. I just like the 'auto'
default more than 'true', because I like to keep it small.

OK. I'll prepare a patch to handle the others.

Post by Björn Baumbach via samba-technical
My thoughts were "If I compile with --without-json-audit, Samba will not
compiled against the jansson libary as well". But yes, you're right -
mention the --without-json-audit switch is more clear.
Please find a patch attached.

I'll wrangle the language again once I sort out the configure options,
but we are close.

Thanks for keeping an eye on this and being willing to discuss how we
can improve it.

Andrew Bartlett

[Rowland Penny via samba-technical]

On Thu, 3 May 2018 09:49:49 +0200

Post by Björn Baumbach via samba-technical

Post by Andrew Bartlett via samba-technical

Post by Björn Baumbach via samba-technical
I didn't change the behaviour. The default is still the
auto-detection. In my opinion this feature is an optional feature
and I am fine with the current default.

Sure, but in retrospect I'm not. This was missed when we first
added the feature.

Post by Björn Baumbach via samba-technical
I would not like to set the default to True.

Why is that?  
A while back we realised that folks were getting builds missing
features, so we stopped making krb5 and ldap automatic in this way,
requiring an explicit --without-krb5, --without-ldap et al.  
Now that an explicit option is being added, I'm asking that is match
the behaviour of our other 'optional' libraries.

In my opinion it's not needed to enable all available optional
features by default. There are some additional switches like the
--with-gpgme
--with-dmapi
--with-fam
--with-libarchive
--with-regedit
But actually, it's not that important to me. I just like the 'auto'
default more than 'true', because I like to keep it small.

Post by Andrew Bartlett via samba-technical

Post by Björn Baumbach via samba-technical
In which way would you like to update the man page? The condition
"if Samba is compiled against the jansson JSON library" is still
Authentication and authorization audit information is logged
under the auth_audit, and if Samba is compiled against the jansson
JSON library, a JSON representation is logged under
auth_json_audit.

and unless Samba was compiled --without-json-audit, a JSON
representation is logged under auth_json_audit.

My thoughts were "If I compile with --without-json-audit, Samba will
not compiled against the jansson libary as well". But yes, you're
right - mention the --without-json-audit switch is more clear.
Please find a patch attached.
Best regards,
Björn

I feel, 'Samba was not compiled with --without-json-audit' is clearer

Rowland

[Andrew Bartlett via samba-technical]

On Thu, 2018-05-03 at 09:22 +0100, Rowland Penny via samba-technical

Post by Rowland Penny via samba-technical
I feel, 'Samba was not compiled with --without-json-audit' is clearer

Thanks Rowland. I agree, but I have to make that true first. A task
for Monday (I hope to slay LMDB tomorrow).

Thanks,

Andrew

[Volker Lendecke via samba-technical]

Post by Andrew Bartlett via samba-technical
On Thu, 2018-05-03 at 09:22 +0100, Rowland Penny via samba-technical

Post by Rowland Penny via samba-technical
I feel, 'Samba was not compiled with --without-json-audit' is clearer

Thanks Rowland. I agree, but I have to make that true first. A task
for Monday (I hope to slay LMDB tomorrow).

I might be late in the game. I just tried to compile --without-ad-dc,
but still get a link dependency against libjansson. What is that used
for without having an Active Directory Domain Controller even
compiled? Why is libjansson a strict dependency in that case? Are we
using it for other things now also?

Volker

[Andrew Bartlett via samba-technical]

Post by Volker Lendecke via samba-technical

Post by Andrew Bartlett via samba-technical
On Thu, 2018-05-03 at 09:22 +0100, Rowland Penny via samba-technical

Post by Rowland Penny via samba-technical
I feel, 'Samba was not compiled with --without-json-audit' is clearer

Thanks Rowland. I agree, but I have to make that true first. A task
for Monday (I hope to slay LMDB tomorrow).

I might be late in the game. I just tried to compile --without-ad-dc,
but still get a link dependency against libjansson. What is that used
for without having an Active Directory Domain Controller even
compiled? Why is libjansson a strict dependency in that case? Are we
using it for other things now also?

The audit logging was added for the file server and NT4 DC as well.

The testing and coverage is comprehensive for the AD DC, but where the
same paths are used (and where it wasn't hard to add) the file server
and NT4 DC wasn't left out.

My hope is that this can be quite helpful in monitoring large
installations.

Andrew Bartlett

[Volker Lendecke via samba-technical]

Post by Andrew Bartlett via samba-technical

Post by Volker Lendecke via samba-technical

Post by Andrew Bartlett via samba-technical
On Thu, 2018-05-03 at 09:22 +0100, Rowland Penny via samba-technical

Post by Rowland Penny via samba-technical
I feel, 'Samba was not compiled with --without-json-audit' is clearer

Thanks Rowland. I agree, but I have to make that true first. A task
for Monday (I hope to slay LMDB tomorrow).

I might be late in the game. I just tried to compile --without-ad-dc,
but still get a link dependency against libjansson. What is that used
for without having an Active Directory Domain Controller even
compiled? Why is libjansson a strict dependency in that case? Are we
using it for other things now also?

The audit logging was added for the file server and NT4 DC as well.
The testing and coverage is comprehensive for the AD DC, but where the
same paths are used (and where it wasn't hard to add) the file server
and NT4 DC wasn't left out.
My hope is that this can be quite helpful in monitoring large
installations.

Ok, but the only way to not link against this is to remove
libjansson-dev from the build machine, right? This means a build
machine per target environment. Easy with containers. Thanks for the
info.

I thought we had removed Javascript in favor of python years ago. I
don't understand why are forced to pull it in again now. Do you plan
to replace our python scripting by Javascript also in the future?

Volker

[Stefan Metzmacher via samba-technical]

Hi Volker,

Post by Volker Lendecke via samba-technical

Post by Andrew Bartlett via samba-technical
The audit logging was added for the file server and NT4 DC as well.
The testing and coverage is comprehensive for the AD DC, but where the
same paths are used (and where it wasn't hard to add) the file server
and NT4 DC wasn't left out.
My hope is that this can be quite helpful in monitoring large
installations.

Ok, but the only way to not link against this is to remove
libjansson-dev from the build machine, right? This means a build
machine per target environment. Easy with containers. Thanks for the
info.

Björn Baumbach recently added the option '--without-json-audit'
in order to avoid the autodetection.

metze

[Volker Lendecke via samba-technical]

Post by Stefan Metzmacher via samba-technical
Hi Volker,

Post by Volker Lendecke via samba-technical

Post by Andrew Bartlett via samba-technical
The audit logging was added for the file server and NT4 DC as well.
The testing and coverage is comprehensive for the AD DC, but where the
same paths are used (and where it wasn't hard to add) the file server
and NT4 DC wasn't left out.
My hope is that this can be quite helpful in monitoring large
installations.

Ok, but the only way to not link against this is to remove
libjansson-dev from the build machine, right? This means a build
machine per target environment. Easy with containers. Thanks for the
info.

Björn Baumbach recently added the option '--without-json-audit'
in order to avoid the autodetection.

Will this go into 4.8 too? My customer is running that. It's not
really important, I removed libjansson-dev for my build machine, but
it's a nuisance.

Volker

[Alexander Bokovoy via samba-technical]

Post by Volker Lendecke via samba-technical

Post by Andrew Bartlett via samba-technical

Post by Volker Lendecke via samba-technical

Post by Andrew Bartlett via samba-technical
On Thu, 2018-05-03 at 09:22 +0100, Rowland Penny via samba-technical

Post by Rowland Penny via samba-technical
I feel, 'Samba was not compiled with --without-json-audit' is clearer

Thanks Rowland. I agree, but I have to make that true first. A task
for Monday (I hope to slay LMDB tomorrow).

I might be late in the game. I just tried to compile --without-ad-dc,
but still get a link dependency against libjansson. What is that used
for without having an Active Directory Domain Controller even
compiled? Why is libjansson a strict dependency in that case? Are we
using it for other things now also?

The audit logging was added for the file server and NT4 DC as well.
The testing and coverage is comprehensive for the AD DC, but where the
same paths are used (and where it wasn't hard to add) the file server
and NT4 DC wasn't left out.
My hope is that this can be quite helpful in monitoring large
installations.

Ok, but the only way to not link against this is to remove
libjansson-dev from the build machine, right? This means a build
machine per target environment. Easy with containers. Thanks for the
info.
I thought we had removed Javascript in favor of python years ago. I
don't understand why are forced to pull it in again now. Do you plan
to replace our python scripting by Javascript also in the future?

libjansson has nothing to do with javascript. It is a C library for JSON
format processing (RFC 4627), not a javascript compiler/interpreter. It
has no ability to evaluate javascript code at all.

JSON format is actively used by many systems that consume logs from
other applications. If data (log event) is encoded in JSON, it is easier
to import that into a centralized logging infrastructure. Without JSON
formatting you'd need to parse existing logs and transform them into
something similarly unified before the import. In Samba case it also
means you'd need to run with a higer debug level too and parsing
multiple lines to obtain the same information that is now available
through the audit messages formatted with the help of jansson library.

I think --with-json-audit patch would need to make to 4.8 too, this is a
matter of opening a bugzilla and combining commits
57d9969f5cd681cd02017c0b41fcbb12fe10fb08 and
cc1eed99bda3773dc13e927f1df1c4fd72e4b848 together.

[Jeremy Allison via samba-technical]

Post by Volker Lendecke via samba-technical
I thought we had removed Javascript in favor of python years ago. I
don't understand why are forced to pull it in again now. Do you plan
to replace our python scripting by Javascript also in the future?

Volker, this is passive-aggressive behavior and isn't helpful
on the lists or to another Team member. Please don't do that.

Jeremy.

[Richard Sharpe via samba-technical]

On Fri, May 4, 2018 at 9:12 AM, Jeremy Allison via samba-technical

Post by Jeremy Allison via samba-technical

Post by Volker Lendecke via samba-technical
I thought we had removed Javascript in favor of python years ago. I
don't understand why are forced to pull it in again now. Do you plan
to replace our python scripting by Javascript also in the future?

Volker, this is passive-aggressive behavior and isn't helpful
on the lists or to another Team member. Please don't do that.

I don't agree. Volker asked a fair question. It just happens that he
misunderstood the purpose of the library that was being pulled in and
the question was wrong.

I don't think he was being passive aggressive (whatever that means)
because he was seeking info.

[Jeremy Allison via samba-technical]

Post by Richard Sharpe via samba-technical
On Fri, May 4, 2018 at 9:12 AM, Jeremy Allison via samba-technical

Post by Jeremy Allison via samba-technical

Post by Volker Lendecke via samba-technical
I thought we had removed Javascript in favor of python years ago. I
don't understand why are forced to pull it in again now. Do you plan
to replace our python scripting by Javascript also in the future?

Volker, this is passive-aggressive behavior and isn't helpful
on the lists or to another Team member. Please don't do that.

I don't agree. Volker asked a fair question.

Asking if we're planning to replace all the python scripting
with Javascript in the future isn't a fair question. It's
not credible and an obvious jab.

All IMHO of course. If I am wrong and it was meant to be
a reasonable question I will of course apologise.

Jeremy.