[Buildroot] [PATCH 1/1] sudo: Add ldap support for sudoers rules

[Buildroot] [PATCH 1/1] sudo: Add ldap support for sudoers rules

[Chris Frederick]

Added Config.in options to enable/disable the option, and check options
in sudo.mk to add openldap as a dependancy and compile with --with-ldap.

When sudo is built with ldap, /etc/sudoers is only read in for defaults,
all rules need to be provided via ldap which is configured by the user
in /etc/ldap.conf.

Since the user explicitly has to provide /etc/ldap.conf, we use 'depends
on' so that the user is obliged to explicitly enable openldap before the
option becomes visible.

Signed-off-by: Chris Frederick <cdf123@cdf123.net>
---
 package/sudo/Config.in | 13 +++++++++++++
 package/sudo/sudo.mk   |  7 +++++++
 2 files changed, 20 insertions(+)

diff --git a/package/sudo/Config.in b/package/sudo/Config.in
index cbef15d..72bb5d7 100644
--- a/package/sudo/Config.in
+++ b/package/sudo/Config.in
@@ -9,3 +9,16 @@ config BR2_PACKAGE_SUDO
 	  but still allow people to get their work done.
 
 	  http://www.sudo.ws/sudo/
+
+if BR2_PACKAGE_SUDO
+config BR2_PACKAGE_SUDO_LDAP
+	bool "ldap integration"
+	depends on BR2_PACKAGE_OPENLDAP
+	help
+	  Allows you to manage sudoers rules in a centralized ldap
+	  directory. This restricts the /etc/sudoers file from
+	  defining rules, only defaults will be read. All rules will
+	  need to be provided via ldap configured in /etc/ldap.conf
+
+	  http://www.sudo.ws/man/1.8.15/sudoers.ldap.man.html
+endif
diff --git a/package/sudo/sudo.mk b/package/sudo/sudo.mk
index f28312a..45273bb 100644
--- a/package/sudo/sudo.mk
+++ b/package/sudo/sudo.mk
@@ -30,6 +30,13 @@ else
 SUDO_CONF_OPTS += --without-pam
 endif
 
+ifeq ($(BR2_PACKAGE_SUDO_LDAP),y)
+SUDO_DEPENDENCIES += openldap
+SUDO_CONF_OPTS += --with-ldap
+else
+SUDO_CONF_OPTS += --without-ldap
+endif
+
 # mksigname/mksiglist needs to run on build host to generate source files
 define SUDO_BUILD_MKSIGNAME_MKSIGLIST_HOST
 	$(MAKE) $(HOST_CONFIGURE_OPTS) \
-- 
2.7.3

[Buildroot] [PATCH 1/1] sudo: Add ldap support for sudoers rules

[Romain Naour]

Hi Chris,

Le 24/10/2016 ? 17:34, Chris Frederick a ?crit :
> Added Config.in options to enable/disable the option, and check options
> in sudo.mk to add openldap as a dependancy and compile with --with-ldap.
> 
> When sudo is built with ldap, /etc/sudoers is only read in for defaults,
> all rules need to be provided via ldap which is configured by the user
> in /etc/ldap.conf.
> 
> Since the user explicitly has to provide /etc/ldap.conf, we use 'depends
> on' so that the user is obliged to explicitly enable openldap before the
> option becomes visible.
> 
> Signed-off-by: Chris Frederick <cdf123@cdf123.net>
> ---
>  package/sudo/Config.in | 13 +++++++++++++
>  package/sudo/sudo.mk   |  7 +++++++
>  2 files changed, 20 insertions(+)
> 
> diff --git a/package/sudo/Config.in b/package/sudo/Config.in
> index cbef15d..72bb5d7 100644
> --- a/package/sudo/Config.in
> +++ b/package/sudo/Config.in
> @@ -9,3 +9,16 @@ config BR2_PACKAGE_SUDO
>  	  but still allow people to get their work done.
>  
>  	  http://www.sudo.ws/sudo/
> +
> +if BR2_PACKAGE_SUDO
> +config BR2_PACKAGE_SUDO_LDAP
> +	bool "ldap integration"
> +	depends on BR2_PACKAGE_OPENLDAP
> +	help
> +	  Allows you to manage sudoers rules in a centralized ldap
> +	  directory. This restricts the /etc/sudoers file from
> +	  defining rules, only defaults will be read. All rules will
> +	  need to be provided via ldap configured in /etc/ldap.conf
> +
> +	  http://www.sudo.ws/man/1.8.15/sudoers.ldap.man.html

Maybe add a comment here

comment "ldap integration needs openldap"
	depends on !BR2_PACKAGE_OPENLDAP

Otherwise:

Reviewed-by: Romain Naour <romain.naour@gmail.com>

Best regards,
Romain

> +endif
> diff --git a/package/sudo/sudo.mk b/package/sudo/sudo.mk
> index f28312a..45273bb 100644
> --- a/package/sudo/sudo.mk
> +++ b/package/sudo/sudo.mk
> @@ -30,6 +30,13 @@ else
>  SUDO_CONF_OPTS += --without-pam
>  endif
>  
> +ifeq ($(BR2_PACKAGE_SUDO_LDAP),y)
> +SUDO_DEPENDENCIES += openldap
> +SUDO_CONF_OPTS += --with-ldap
> +else
> +SUDO_CONF_OPTS += --without-ldap
> +endif
> +
>  # mksigname/mksiglist needs to run on build host to generate source files
>  define SUDO_BUILD_MKSIGNAME_MKSIGLIST_HOST
>  	$(MAKE) $(HOST_CONFIGURE_OPTS) \
> 

[Buildroot] [PATCH 1/1] sudo: Add ldap support for sudoers rules

[Thomas Petazzoni]

Hello,

On Sat, 28 Jan 2017 15:55:22 +0100, Romain Naour wrote:

> Maybe add a comment here
> 
> comment "ldap integration needs openldap"
> 	depends on !BR2_PACKAGE_OPENLDAP

Well, I'm not against Config.in comments, but in that case, isn't it
sufficiently obvious to the user? Surely if you think about using LDAP
support in sudo, you know that you need to have some kind of LDAP
implementation.

Which makes me think that perhaps a new sub-option is maybe not needed.
Why not just enable LDAP support in sudo when BR2_PACKAGE_OPENLDAP=y ?

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[Buildroot] [PATCH 1/1] sudo: Add ldap support for sudoers rules

[Romain Naour]

Hi Thomas, Chris, All,

Le 29/01/2017 ? 14:18, Thomas Petazzoni a ?crit :
> Hello,
> 
> On Sat, 28 Jan 2017 15:55:22 +0100, Romain Naour wrote:
> 
>> Maybe add a comment here
>>
>> comment "ldap integration needs openldap"
>> 	depends on !BR2_PACKAGE_OPENLDAP
> 
> Well, I'm not against Config.in comments, but in that case, isn't it
> sufficiently obvious to the user? Surely if you think about using LDAP
> support in sudo, you know that you need to have some kind of LDAP
> implementation.
> 
> Which makes me think that perhaps a new sub-option is maybe not needed.
> Why not just enable LDAP support in sudo when BR2_PACKAGE_OPENLDAP=y ?

Yes, It seems a better idea :)

Best regards,
Romain

> 
> Thomas
> 

[Buildroot] [PATCH 1/1] sudo: Add ldap support for sudoers rules

[Thomas Petazzoni]

Hello,

On Mon, 24 Oct 2016 10:34:23 -0500, Chris Frederick wrote:
> Added Config.in options to enable/disable the option, and check options
> in sudo.mk to add openldap as a dependancy and compile with --with-ldap.
> 
> When sudo is built with ldap, /etc/sudoers is only read in for defaults,
> all rules need to be provided via ldap which is configured by the user
> in /etc/ldap.conf.
> 
> Since the user explicitly has to provide /etc/ldap.conf, we use 'depends
> on' so that the user is obliged to explicitly enable openldap before the
> option becomes visible.
> 
> Signed-off-by: Chris Frederick <cdf123@cdf123.net>
> ---
>  package/sudo/Config.in | 13 +++++++++++++
>  package/sudo/sudo.mk   |  7 +++++++
>  2 files changed, 20 insertions(+)

I've applied, after removing the explicit Config.in option and instead
relying on whether BR2_PACKAGE_OPENLDAP is enabled or not to decide if
ldap support should be enabled or not in sudo.

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[Buildroot] [PATCH 1/1] sudo: Add ldap support for sudoers rules

[Peter Korsgaard]

>>>>> "Thomas" == Thomas Petazzoni <thomas.petazzoni@free-electrons.com> writes:

 > Hello,
 > On Mon, 24 Oct 2016 10:34:23 -0500, Chris Frederick wrote:
 >> Added Config.in options to enable/disable the option, and check options
 >> in sudo.mk to add openldap as a dependancy and compile with --with-ldap.
 >> 
 >> When sudo is built with ldap, /etc/sudoers is only read in for defaults,
 >> all rules need to be provided via ldap which is configured by the user
 >> in /etc/ldap.conf.
 >> 
 >> Since the user explicitly has to provide /etc/ldap.conf, we use 'depends
 >> on' so that the user is obliged to explicitly enable openldap before the
 >> option becomes visible.
 >> 
 >> Signed-off-by: Chris Frederick <cdf123@cdf123.net>
 >> ---
 >> package/sudo/Config.in | 13 +++++++++++++
 >> package/sudo/sudo.mk   |  7 +++++++
 >> 2 files changed, 20 insertions(+)

 > I've applied, after removing the explicit Config.in option and instead
 > relying on whether BR2_PACKAGE_OPENLDAP is enabled or not to decide if
 > ldap support should be enabled or not in sudo.

Notice that this was discussed during the first review:

http://lists.busybox.net/pipermail/buildroot/2016-October/175480.html

Apparently sudo with pam support really needs a configured
/etc/ldap.conf, which might be confusing to people that (perhaps
unrelated to eachother) enabled both sudo and ldap.

-- 
Bye, Peter Korsgaard

[Buildroot] [PATCH 1/1] sudo: Add ldap support for sudoers rules

[Thomas Petazzoni]

Hello,

On Mon, 06 Feb 2017 22:33:39 +0100, Peter Korsgaard wrote:

>  > I've applied, after removing the explicit Config.in option and instead
>  > relying on whether BR2_PACKAGE_OPENLDAP is enabled or not to decide if
>  > ldap support should be enabled or not in sudo.  
> 
> Notice that this was discussed during the first review:
> 
> http://lists.busybox.net/pipermail/buildroot/2016-October/175480.html
> 
> Apparently sudo with pam support really needs a configured
> /etc/ldap.conf, which might be confusing to people that (perhaps
> unrelated to eachother) enabled both sudo and ldap.

So in other words we really want to have a sub-option for this
situation?

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[Buildroot] [PATCH 1/1] sudo: Add ldap support for sudoers rules

[Peter Korsgaard]

>>>>> "Thomas" == Thomas Petazzoni <thomas.petazzoni@free-electrons.com> writes:

 > Hello,
 > On Mon, 06 Feb 2017 22:33:39 +0100, Peter Korsgaard wrote:

 >> > I've applied, after removing the explicit Config.in option and instead
 >> > relying on whether BR2_PACKAGE_OPENLDAP is enabled or not to decide if
 >> > ldap support should be enabled or not in sudo.  
 >> 
 >> Notice that this was discussed during the first review:
 >> 
 >> http://lists.busybox.net/pipermail/buildroot/2016-October/175480.html
 >> 
 >> Apparently sudo with pam support really needs a configured
 >> /etc/ldap.conf, which might be confusing to people that (perhaps
 >> unrelated to eachother) enabled both sudo and ldap.

 > So in other words we really want to have a sub-option for this
 > situation?

I don't personally know anything about it, but that was the conclusion
of Arnout and Chris, yes.

-- 
Bye, Peter Korsgaard

[Buildroot] [PATCH 1/1] sudo: Add ldap support for sudoers rules

[Chris Frederick]

On 02/06/2017 03:52 PM, Peter Korsgaard wrote:
>>>>>> "Thomas" == Thomas Petazzoni <thomas.petazzoni@free-electrons.com> writes:
> 
>  > Hello,
>  > On Mon, 06 Feb 2017 22:33:39 +0100, Peter Korsgaard wrote:
> 
>  >> > I've applied, after removing the explicit Config.in option and instead
>  >> > relying on whether BR2_PACKAGE_OPENLDAP is enabled or not to decide if
>  >> > ldap support should be enabled or not in sudo.  
>  >> 
>  >> Notice that this was discussed during the first review:
>  >> 
>  >> http://lists.busybox.net/pipermail/buildroot/2016-October/175480.html
>  >> 
>  >> Apparently sudo with pam support really needs a configured
>  >> /etc/ldap.conf, which might be confusing to people that (perhaps
>  >> unrelated to eachother) enabled both sudo and ldap.
> 
>  > So in other words we really want to have a sub-option for this
>  > situation?
> 
> I don't personally know anything about it, but that was the conclusion
> of Arnout and Chris, yes.
> 

It looks like you can configure sudoers access via /etc/nsswitch.conf:

See "Configuring nsswitch.conf" in the sudoers ldap manual:
https://www.sudo.ws/man/1.8.17/sudoers.ldap.man.html

So it could be left to the end user building the image to add their own nsswitch.conf entry to control it.  It also mentions that sudo defaults
to using /etc/sudoers exclusively, so it should be compatible with users not using ldap sudoers with no changes required of them.

Chris Frederick

[Buildroot] [PATCH 1/1] sudo: Add ldap support for sudoers rules

[Thomas Petazzoni]

Hello,

On Mon, 24 Oct 2016 10:34:23 -0500, Chris Frederick wrote:
> Added Config.in options to enable/disable the option, and check options
> in sudo.mk to add openldap as a dependancy and compile with --with-ldap.
> 
> When sudo is built with ldap, /etc/sudoers is only read in for defaults,
> all rules need to be provided via ldap which is configured by the user
> in /etc/ldap.conf.
> 
> Since the user explicitly has to provide /etc/ldap.conf, we use 'depends
> on' so that the user is obliged to explicitly enable openldap before the
> option becomes visible.
> 
> Signed-off-by: Chris Frederick <cdf123@cdf123.net>

The sudo/ldap support causes some build failures in static linking
configurations. See:
http://autobuild.buildroot.net/results/483/4830c69cc6a62080e1516f0d9009c2ba619c23c1/build-end.log
for an example.

From a quick look, it seems like sudo doesn't use pkg-config to
discover the ldap library, and therefore doesn't know that ldap needs
to be linked with openssl. But that's really a 5 seconds look at the
build error.

Could you have a closer look?

Thanks,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com