Re: [Buildroot] [PATCH v2 1/1] package/shadow: new package

["Yann E. MORIN" <yann.morin.1998@free.fr>]

Raphael, All,

On 2022-09-04 14:43 +0200, Raphael Pavlidis spake thusly:
> shadow provides utilities to deal with user accounts.

You will probably have more explanations to provide in the commit log,
to explain how the pacakge is integrated in Buildroot. See the qustions
below...

> Signed-off-by: Raphael Pavlidis <raphael.pavlidis@gmail.com>

In addition to Arnout's quick review, here's my own quick review...

[--SNIP--]
> diff --git a/package/shadow/Config.in b/package/shadow/Config.in
> new file mode 100644
> index 0000000000..616f002618
> --- /dev/null
> +++ b/package/shadow/Config.in
> @@ -0,0 +1,81 @@
> +menuconfig BR2_PACKAGE_SHADOW
> +	bool "shadow"
> +	depends on BR2_TOOLCHAIN_HEADERS_AT_LEAST_4_14

As Arnout noted, shadow, or ony some of its utilities, may come
conflicting with busybox' provided applets.

So, we also need a dependency in Config.in:

    depends on BR2_PACKAGE_BUSYBOX_SHOW_OTHERS

Note: *if* only sub-options of shadow do conflict, then the dependency
should be moved dow to those sub-options.

> +	help
> +	  Utilities to deal with user accounts.
> +
> +	  https://github.com/shadow-maint/shadow
> +
> +if BR2_PACKAGE_SHADOW
> +
> +config BR2_PACKAGE_SHADOW_SHADOWGRP
> +	bool "shadowgrp"
> +	default y

We usually have no option that defaults to 'y', and when we do, there
is a reason for that, so please explain that in the commit log. This
comment is also valid for all the symbols below that default to y.

> +	help
> +	  Enable shadow group support.
> +
> +if BR2_PACKAGE_LINUX_PAM

When there is a single symbol that is conditional, I think a singluar
depends on is better:

> +config BR2_PACKAGE_SHADOW_ACCOUNT_TOOLS_SETUID
> +	bool "account-tools-setuid"

    depends on BR2_PACKAGE_LINUX_PAM

Also, I was wondering if that should instead be a select rather than a
depends-on. I.e. is account-tools-setuid something that "manages" PAM
settings, or is it something that uses PAM to amanage accounts? If the
former, then a depends-on is more appropriate, but if the latter, then a
select is better.

If that makes more sense to select, then:

    config BR2_PACKAGE_SHADOW_ACCOUNT_TOOLS_SETUID
        bool "account-tools-setuid"
        depends on BR2_USE_MMU  # linux-pam
        depends on BR2_ENABLE_LOCALE  # linux-pam
        depends on BR2_USE_WCHAR  # linux-pam
        depends on !BR2_STATIC_LIBS  # linux-pam
        select BR2_PACKAGE_LINUX_PAM

    comment "account-tools-setuid needs a toolchain w/ shared libs, wchar, locale"
        depends on BR2_USE_MMU
        depends on BR2_STATIC_LIBS || !BR2_USE_WCHAR \
                || !BR2_ENABLE_LOCALE

> +	help
> +	  Install the user and group management tools setuid and authenticate the
> +	  callers.

(hint: here, it seems to suggest we would better use a select)

> +endif # BR2_PACKAGE_LINUX_PAM
> +
> +config BR2_PACKAGE_SHADOW_UTMPX
> +	bool "utmpx"
> +	help
> +	  Enable loggin in utmpx / wtmpx.
> +
> +config BR2_PACKAGE_SHADOW_SUBORDINATE_IDS
> +	bool "subordinate-ids"
> +	default y
> +	help
> +	  Support subordinate ids.

An help entry that just repeats the prompt is totally useless. If there
is nothing better than to repeat the prompt, then don't provide a help
entry. Otherwise, provide actual help.

> +config BR2_PACKAGE_SHADOW_SHA_CRYPT
> +	bool "sha-crypt"
> +	default y
> +	help
> +	  Allow the SHA256 and SHA512 password encryption algorithms.

Note: the is a very good and terse help entry.

> +config BR2_PACKAGE_SHADOW_BCRYPT
> +	bool "bcrypt"
> +	help
> +	  Allow the bcrypt password encryption algorithm.

s/bcrypt/blowfish block cipher/ and you get a better help entry.

> +config BR2_PACKAGE_SHADOW_YESCRYPT
> +	bool "yescrypt"
> +	help
> +	  Allow the yescrypt password encryption algorithm.
> +
> +config BR2_PACKAGE_SHADOW_NSCD
> +	bool "nscd"
> +	default y
> +	help
> +	  Enable support for nscd.
> +
> +config BR2_PACKAGE_SHADOW_SSSD
> +	bool "sssd"
> +	default y
> +	help
> +	  Define to support flushing of sssd caches.
> +
> +config BR2_PACKAGE_SHADOW_GROUP_NAME_MAX_LENGTH
> +	int "group-name-max-length"
> +	default 16

Does it really make sense to have this be configurable?
If so, why is 16 the default, rather than unlimited?

And if we keep it, then the prompt should not have dashes, but be a
sentence (i.e. it is not the name of program installed by shwadow):

    bool "max length of group names"

> +	help
> +	  Set max group name length. (0 equals infinity)
> +
> +config BR2_PACKAGE_SHADOW_SU
> +	bool "su"
> +	default y

This one will definitely conflict with Busybox' own su.

> +	help
> +	  Build and install su program.

This does not provide much help, so I'd just drop the help entry.

[--SNIP--]
> diff --git a/package/shadow/shadow.mk b/package/shadow/shadow.mk
> new file mode 100644
> index 0000000000..140d830cb9
> --- /dev/null
> +++ b/package/shadow/shadow.mk
> @@ -0,0 +1,171 @@
> +################################################################################
> +#
> +# shadow
> +#
> +################################################################################
> +
> +SHADOW_VERSION = 4.11.1
> +SHADOW_SITE = https://github.com/shadow-maint/shadow/releases/download/v$(SHADOW_VERSION)
> +SHADOW_SOURCE = shadow-$(SHADOW_VERSION).tar.xz
> +SHADOW_LICENSE = BSD-3-Clause
> +SHADOW_LICENSE_FILES = COPYING
> +
> +SHADOW_CONF_OPTS += \

This is the first, unconditional assignment; it should be a simple
assignment, not an append-assignment.

> +	--disable-man \
> +	--without-btrfs \
> +	--without-skey \
> +	--without-tcb
> +
> +ifeq ($(BR2_STATIC_LIBS),y)
> +SHADOW_CONF_OPTS += --enable-static
> +else
> +SHADOW_CONF_OPTS += --disable-static
> +endif
> +
> +ifeq ($(BR2_SHARED_LIBS),y)
> +SHADOW_CONF_OPTS += --enable-shared
> +else
> +SHADOW_CONF_OPTS += --disable-shared
> +endif

So, first, both options are already passed appropriately by the
autotools package infrastructure, so why do you need to pass them?

Second, --{en,disable}-{static,shared} is supposed to drive the build
of static or shared libraries, not the fact that anything is shared or
statically linked.

> +ifeq ($(BR2_PACKAGE_SHADOW_ACCOUNT_TOOLS_SETUID),y)
> +SHADOW_CONF_OPTS += --enable-account-tools-setuid
> +SHADOW_ACCOUNT_TOOLS_SETUID = \
> +	/usr/sbin/chgpasswd f 4755 0 0 - - - - - \
> +	/usr/sbin/chpasswd f 4755 0 0 - - - - - \
> +	/usr/sbin/groupadd f 4755 0 0 - - - - - \
> +	/usr/sbin/groupdel f 4755 0 0 - - - - - \
> +	/usr/sbin/groupmod f 4755 0 0 - - - - - \
> +	/usr/sbin/newusers f 4755 0 0 - - - - - \
> +	/usr/sbin/useradd f 4755 0 0 - - - - - \
> +	/usr/sbin/usermod f 4755 0 0 - - - - -

Use a define here (also, the other two conditional permissions end with
_PERMISSIONS, so do it here to):

    define SHADOW_ACCOUNT_TOOLS_SETUID_PERMISSIONS
    	/usr/sbin/chgpasswd f 4755 0 0 - - - - -
    	/usr/sbin/chpasswd f 4755 0 0 - - - - -
    	/usr/sbin/groupadd f 4755 0 0 - - - - -
    	/usr/sbin/groupdel f 4755 0 0 - - - - -
    	/usr/sbin/groupmod f 4755 0 0 - - - - -
    	/usr/sbin/newusers f 4755 0 0 - - - - -
    	/usr/sbin/useradd f 4755 0 0 - - - - -
    	/usr/sbin/usermod f 4755 0 0 - - - - -
    endef

Note: ditto for SHADOW_SUBORDINATE_IDS_PERMISSIONS: use a define rather
than a multi-line (and I suspect a multi-line does not actually work...)

> +else
> +SHADOW_CONF_OPTS += --disable-account-tools-setuid
> +endif

[--SNIP--]
> +ifeq ($(BR2_PACKAGE_ACL),y)
> +SHADOW_CONF_OPTS += --with-acl
> +SHADOW_DEPENDENCIES += acl

Pet peeve of mine: I prefer that dependencies be listed before config
options. Indeed, semantically, we need the dependency to be fulfilled
before we can use it; it also more closely match the unconditional
dependencies and config options.

> +else
> +SHADOW_CONF_OPTS += --without-acl
> +endif
[--SNIP--]
> +ifeq ($(BR2_PACKAGE_LINUX_PAM),y)
> +SHADOW_CONF_OPTS += --with-libpam
> +SHADOW_DEPENDENCIES += linux-pam
> +else
> +SHADOW_CONF_OPTS += --without-libpam
> +endif

Is the dependency on linux-pam only needed for account-tools-setuid, or
can shadow also use linux-pam for something else?

If the former, then the dependency and activating of the PAM opotion
should be moved together in the conditional block that deals with
enabling account-tools-setuid. If the latter, then a small comment could
be added, like:

    # linux-pam is also used without account-tools-setuid enabled

> +ifeq ($(BR2_ENABLE_LOCALE),y)
> +SHADOW_CONF_OPTS += --enable-nls
> +else
> +SHADOW_CONF_OPTS += --disable-nls
> +endif

This is supposed to also be already handled by the autotools-package
infrastructure, see:
    package/pkg-autotools.mk@201
    package/Makefile.in@392

So, why is it needed to explicitly handle them here?

Regards,
Yann E. MORIN.

> +ifeq ($(BR2_PACKAGE_SHADOW_GROUP_NAME_MAX_LENGTH),0)
> +SHADOW_CONF_OPTS += --without-group-name-max-length
> +else
> +SHADOW_CONF_OPTS += --with-group-name-max-length=$(BR2_PACKAGE_SHADOW_GROUP_NAME_MAX_LENGTH)
> +endif
> +
> +ifeq ($(BR2_PACKAGE_SHADOW_SU),y)
> +SHADOW_CONF_OPTS += --with-su
> +SHADOW_SU_PERMISSIONS = /bin/su f 4755 0 0 - - - - -
> +else
> +SHADOW_CONF_OPTS += --without-su
> +endif
> +
> +define SHADOW_PERMISSIONS
> +	/usr/bin/chage f 4755 0 0 - - - - -
> +	/usr/bin/chfn f 4755 0 0 - - - - -
> +	/usr/bin/chsh f 4755 0 0 - - - - -
> +	/usr/bin/expiry f 4755 0 0 - - - - -
> +	/usr/bin/gpasswd f 4755 0 0 - - - - -
> +	/usr/bin/newgrp f 4755 0 0 - - - - -
> +	/usr/bin/passwd f 4755 0 0 - - - - -
> +	$(SHADOW_ACCOUNT_TOOLS_SETUID)
> +	$(SHADOW_SUBORDINATE_IDS_PERMISSIONS)
> +	$(SHADOW_SU_PERMISSIONS)
> +endef
> +
> +$(eval $(autotools-package))
> -- 
> 2.35.1
> 
> _______________________________________________
> buildroot mailing list
> buildroot@buildroot.org
> https://lists.buildroot.org/mailman/listinfo/buildroot

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot