All of lore.kernel.org
 help / color / mirror / Atom feed
* [Buildroot] [PATCH v2 1/1] package/shadow: new package
@ 2022-09-04 12:43 Raphael Pavlidis
  2022-09-05 10:06 ` Arnout Vandecappelle
                   ` (2 more replies)
  0 siblings, 3 replies; 18+ messages in thread
From: Raphael Pavlidis @ 2022-09-04 12:43 UTC (permalink / raw)
  To: buildroot; +Cc: Raphael Pavlidis, Thomas Petazzoni

shadow provides utilities to deal with user accounts.

Signed-off-by: Raphael Pavlidis <raphael.pavlidis@gmail.com>
---
Changes v1 -> v2:
- DEVELOPERS: add Raphael Pavlids for shadow

 DEVELOPERS                 |   3 +
 package/Config.in          |   1 +
 package/shadow/Config.in   |  81 ++++++++++++++++++
 package/shadow/shadow.hash |   3 +
 package/shadow/shadow.mk   | 171 +++++++++++++++++++++++++++++++++++++
 5 files changed, 259 insertions(+)
 create mode 100644 package/shadow/Config.in
 create mode 100644 package/shadow/shadow.hash
 create mode 100644 package/shadow/shadow.mk

diff --git a/DEVELOPERS b/DEVELOPERS
index d2bd0d809a..38c25a0ae2 100644
--- a/DEVELOPERS
+++ b/DEVELOPERS
@@ -2506,6 +2506,9 @@ F:	support/testing/tests/package/test_python_jmespath.py
 F:	support/testing/tests/package/test_python_rsa.py
 F:	support/testing/tests/package/test_python_s3transfer.py
 
+N:	Raphael Pavlidis <raphael.pavlidis@gmail.com>
+F:	package/shadow/
+
 N:	Refik Tuzakli <tuzakli.refik@gmail.com>
 F:	package/freescale-imx/
 F:	package/paho-mqtt-cpp/
diff --git a/package/Config.in b/package/Config.in
index d1c098c48f..c13ba09056 100644
--- a/package/Config.in
+++ b/package/Config.in
@@ -2635,6 +2635,7 @@ menu "System tools"
 	source "package/sdbus-cpp/Config.in"
 	source "package/sdbusplus/Config.in"
 	source "package/seatd/Config.in"
+	source "package/shadow/Config.in"
 	source "package/smack/Config.in"
 	source "package/start-stop-daemon/Config.in"
 	source "package/supervisor/Config.in"
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
+	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
+	help
+	  Enable shadow group support.
+
+if BR2_PACKAGE_LINUX_PAM
+
+config BR2_PACKAGE_SHADOW_ACCOUNT_TOOLS_SETUID
+	bool "account-tools-setuid"
+	help
+	  Install the user and group management tools setuid and authenticate the
+	  callers.
+
+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.
+
+config BR2_PACKAGE_SHADOW_SHA_CRYPT
+	bool "sha-crypt"
+	default y
+	help
+	  Allow the SHA256 and SHA512 password encryption algorithms.
+
+config BR2_PACKAGE_SHADOW_BCRYPT
+	bool "bcrypt"
+	help
+	  Allow the bcrypt password encryption algorithm.
+
+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
+	help
+	  Set max group name length. (0 equals infinity)
+
+config BR2_PACKAGE_SHADOW_SU
+	bool "su"
+	default y
+	help
+	  Build and install su program.
+
+endif # BR2_PACKAGE_SHADOW
+
+comment "shadow needs a toolchain w/ headers >= 4.14"
+	depends on !BR2_TOOLCHAIN_HEADERS_AT_LEAST_4_14
diff --git a/package/shadow/shadow.hash b/package/shadow/shadow.hash
new file mode 100644
index 0000000000..6b9faac10f
--- /dev/null
+++ b/package/shadow/shadow.hash
@@ -0,0 +1,3 @@
+# Locally computed
+sha256  41f093ce58b2ae5f389a1c5553e0c18bc73e6fe27f66273891991198a7707c95  shadow-4.11.1.tar.xz
+sha256  3d25ab8f43fdc14624296a56ff8dc3e72e499ad35f32ae0c803f4959cfe17c0a  COPYING
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 += \
+	--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
+
+ifeq ($(BR2_PACKAGE_SHADOW_SHADOWGRP),y)
+SHADOW_CONF_OPTS += --enable-shadowgrp
+else
+SHADOW_CONF_OPTS += --disable-shadowgrp
+endif
+
+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 - - - - -
+else
+SHADOW_CONF_OPTS += --disable-account-tools-setuid
+endif
+
+ifeq ($(BR2_PACKAGE_SHADOW_UTMPX),y)
+SHADOW_CONF_OPTS += --enable-utmpx
+else
+SHADOW_CONF_OPTS += --disable-utmpx
+endif
+
+ifeq ($(BR2_PACKAGE_SHADOW_SUBORDINATE_IDS),y)
+SHADOW_CONF_OPTS += --enable-subordinate-ids
+SHADOW_SUBORDINATE_IDS_PERMISSIONS =  \
+	/usr/bin/newuidmap f 4755 0 0 - - - - - \
+	/usr/bin/newgidmap f 4755 0 0 - - - - -
+else
+SHADOW_CONF_OPTS += --disable-subordinate-ids
+endif
+
+ifeq ($(BR2_PACKAGE_ACL),y)
+SHADOW_CONF_OPTS += --with-acl
+SHADOW_DEPENDENCIES += acl
+else
+SHADOW_CONF_OPTS += --without-acl
+endif
+
+ifeq ($(BR2_PACKAGE_ATTR),y)
+SHADOW_CONF_OPTS += --with-attr
+SHADOW_DEPENDENCIES += attr
+else
+SHADOW_CONF_OPTS += --without-attr
+endif
+
+ifeq ($(BR2_PACKAGE_AUDIT),y)
+SHADOW_CONF_OPTS += --with-audit
+SHADOW_DEPENDENCIES += audit
+else
+SHADOW_CONF_OPTS += --without-audit
+endif
+
+ifeq ($(BR2_PACKAGE_CRACKLIB),y)
+SHADOW_CONF_OPTS += --with-libcrack
+SHADOW_DEPENDENCIES += cracklib
+else
+SHADOW_CONF_OPTS += --without-libcrack
+endif
+
+ifeq ($(BR2_PACKAGE_LIBSELINUX),y)
+SHADOW_CONF_OPTS += --with-selinux
+SHADOW_DEPENDENCIES += libselinux libsemanage
+else
+SHADOW_CONF_OPTS += --without-selinux
+endif
+
+ifeq ($(BR2_PACKAGE_LINUX_PAM),y)
+SHADOW_CONF_OPTS += --with-libpam
+SHADOW_DEPENDENCIES += linux-pam
+else
+SHADOW_CONF_OPTS += --without-libpam
+endif
+
+ifeq ($(BR2_ENABLE_LOCALE),y)
+SHADOW_CONF_OPTS += --enable-nls
+else
+SHADOW_CONF_OPTS += --disable-nls
+endif
+
+ifeq ($(BR2_PACKAGE_SHADOW_SHA_CRYPT),y)
+SHADOW_CONF_OPTS += --with-sha-crypt
+else
+SHADOW_CONF_OPTS += --without-sha-crypt
+endif
+
+ifeq ($(BR2_PACKAGE_SHADOW_BCRYPT),y)
+SHADOW_CONF_OPTS += --with-bcrypt
+else
+SHADOW_CONF_OPTS += --without-bcrypt
+endif
+
+ifeq ($(BR2_PACKAGE_SHADOW_YESCRYPT),y)
+SHADOW_CONF_OPTS += --with-yescrypt
+else
+SHADOW_CONF_OPTS += --without-yescrypt
+endif
+
+ifeq ($(BR2_PACKAGE_SHADOW_NSCD),y)
+SHADOW_CONF_OPTS += --with-nscd
+else
+SHADOW_CONF_OPTS += --without-nscd
+endif
+
+ifeq ($(BR2_PACKAGE_SHADOW_SSSD),y)
+SHADOW_CONF_OPTS += --with-sssd
+else
+SHADOW_CONF_OPTS += --without-sssd
+endif
+
+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

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [Buildroot] [PATCH v2 1/1] package/shadow: new package
  2022-09-04 12:43 [Buildroot] [PATCH v2 1/1] package/shadow: new package Raphael Pavlidis
@ 2022-09-05 10:06 ` Arnout Vandecappelle
  2022-09-05 11:51 ` Yann E. MORIN
  2022-10-13 16:34 ` [Buildroot] [PATCH v3 " Raphael Pavlidis
  2 siblings, 0 replies; 18+ messages in thread
From: Arnout Vandecappelle @ 2022-09-05 10:06 UTC (permalink / raw)
  To: Raphael Pavlidis, buildroot; +Cc: Thomas Petazzoni

  Hi Raphael,

On 04/09/2022 14:43, Raphael Pavlidis wrote:
> shadow provides utilities to deal with user accounts.
> 
> Signed-off-by: Raphael Pavlidis <raphael.pavlidis@gmail.com>

  Not a full review, but just a small comment: I believe the shadow package 
installs (or may install) some files that also have a busybox equivalent. If 
this is the case, a dependency has to be added to busybox, to make sure that the 
busybox version doesn't overwrite the one from shadow when per-package 
directories are enabled. See the large list of other dependencies already 
present in busybox.mk.

  Regards,
  Arnout

> ---
> Changes v1 -> v2:
> - DEVELOPERS: add Raphael Pavlids for shadow
> 
>   DEVELOPERS                 |   3 +
>   package/Config.in          |   1 +
>   package/shadow/Config.in   |  81 ++++++++++++++++++
>   package/shadow/shadow.hash |   3 +
>   package/shadow/shadow.mk   | 171 +++++++++++++++++++++++++++++++++++++
>   5 files changed, 259 insertions(+)
>   create mode 100644 package/shadow/Config.in
>   create mode 100644 package/shadow/shadow.hash
>   create mode 100644 package/shadow/shadow.mk
> 
> diff --git a/DEVELOPERS b/DEVELOPERS
> index d2bd0d809a..38c25a0ae2 100644
> --- a/DEVELOPERS
> +++ b/DEVELOPERS
> @@ -2506,6 +2506,9 @@ F:	support/testing/tests/package/test_python_jmespath.py
>   F:	support/testing/tests/package/test_python_rsa.py
>   F:	support/testing/tests/package/test_python_s3transfer.py
>   
> +N:	Raphael Pavlidis <raphael.pavlidis@gmail.com>
> +F:	package/shadow/
> +
>   N:	Refik Tuzakli <tuzakli.refik@gmail.com>
>   F:	package/freescale-imx/
>   F:	package/paho-mqtt-cpp/
> diff --git a/package/Config.in b/package/Config.in
> index d1c098c48f..c13ba09056 100644
> --- a/package/Config.in
> +++ b/package/Config.in
> @@ -2635,6 +2635,7 @@ menu "System tools"
>   	source "package/sdbus-cpp/Config.in"
>   	source "package/sdbusplus/Config.in"
>   	source "package/seatd/Config.in"
> +	source "package/shadow/Config.in"
>   	source "package/smack/Config.in"
>   	source "package/start-stop-daemon/Config.in"
>   	source "package/supervisor/Config.in"
> 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
> +	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
> +	help
> +	  Enable shadow group support.
> +
> +if BR2_PACKAGE_LINUX_PAM
> +
> +config BR2_PACKAGE_SHADOW_ACCOUNT_TOOLS_SETUID
> +	bool "account-tools-setuid"
> +	help
> +	  Install the user and group management tools setuid and authenticate the
> +	  callers.
> +
> +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.
> +
> +config BR2_PACKAGE_SHADOW_SHA_CRYPT
> +	bool "sha-crypt"
> +	default y
> +	help
> +	  Allow the SHA256 and SHA512 password encryption algorithms.
> +
> +config BR2_PACKAGE_SHADOW_BCRYPT
> +	bool "bcrypt"
> +	help
> +	  Allow the bcrypt password encryption algorithm.
> +
> +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
> +	help
> +	  Set max group name length. (0 equals infinity)
> +
> +config BR2_PACKAGE_SHADOW_SU
> +	bool "su"
> +	default y
> +	help
> +	  Build and install su program.
> +
> +endif # BR2_PACKAGE_SHADOW
> +
> +comment "shadow needs a toolchain w/ headers >= 4.14"
> +	depends on !BR2_TOOLCHAIN_HEADERS_AT_LEAST_4_14
> diff --git a/package/shadow/shadow.hash b/package/shadow/shadow.hash
> new file mode 100644
> index 0000000000..6b9faac10f
> --- /dev/null
> +++ b/package/shadow/shadow.hash
> @@ -0,0 +1,3 @@
> +# Locally computed
> +sha256  41f093ce58b2ae5f389a1c5553e0c18bc73e6fe27f66273891991198a7707c95  shadow-4.11.1.tar.xz
> +sha256  3d25ab8f43fdc14624296a56ff8dc3e72e499ad35f32ae0c803f4959cfe17c0a  COPYING
> 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 += \
> +	--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
> +
> +ifeq ($(BR2_PACKAGE_SHADOW_SHADOWGRP),y)
> +SHADOW_CONF_OPTS += --enable-shadowgrp
> +else
> +SHADOW_CONF_OPTS += --disable-shadowgrp
> +endif
> +
> +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 - - - - -
> +else
> +SHADOW_CONF_OPTS += --disable-account-tools-setuid
> +endif
> +
> +ifeq ($(BR2_PACKAGE_SHADOW_UTMPX),y)
> +SHADOW_CONF_OPTS += --enable-utmpx
> +else
> +SHADOW_CONF_OPTS += --disable-utmpx
> +endif
> +
> +ifeq ($(BR2_PACKAGE_SHADOW_SUBORDINATE_IDS),y)
> +SHADOW_CONF_OPTS += --enable-subordinate-ids
> +SHADOW_SUBORDINATE_IDS_PERMISSIONS =  \
> +	/usr/bin/newuidmap f 4755 0 0 - - - - - \
> +	/usr/bin/newgidmap f 4755 0 0 - - - - -
> +else
> +SHADOW_CONF_OPTS += --disable-subordinate-ids
> +endif
> +
> +ifeq ($(BR2_PACKAGE_ACL),y)
> +SHADOW_CONF_OPTS += --with-acl
> +SHADOW_DEPENDENCIES += acl
> +else
> +SHADOW_CONF_OPTS += --without-acl
> +endif
> +
> +ifeq ($(BR2_PACKAGE_ATTR),y)
> +SHADOW_CONF_OPTS += --with-attr
> +SHADOW_DEPENDENCIES += attr
> +else
> +SHADOW_CONF_OPTS += --without-attr
> +endif
> +
> +ifeq ($(BR2_PACKAGE_AUDIT),y)
> +SHADOW_CONF_OPTS += --with-audit
> +SHADOW_DEPENDENCIES += audit
> +else
> +SHADOW_CONF_OPTS += --without-audit
> +endif
> +
> +ifeq ($(BR2_PACKAGE_CRACKLIB),y)
> +SHADOW_CONF_OPTS += --with-libcrack
> +SHADOW_DEPENDENCIES += cracklib
> +else
> +SHADOW_CONF_OPTS += --without-libcrack
> +endif
> +
> +ifeq ($(BR2_PACKAGE_LIBSELINUX),y)
> +SHADOW_CONF_OPTS += --with-selinux
> +SHADOW_DEPENDENCIES += libselinux libsemanage
> +else
> +SHADOW_CONF_OPTS += --without-selinux
> +endif
> +
> +ifeq ($(BR2_PACKAGE_LINUX_PAM),y)
> +SHADOW_CONF_OPTS += --with-libpam
> +SHADOW_DEPENDENCIES += linux-pam
> +else
> +SHADOW_CONF_OPTS += --without-libpam
> +endif
> +
> +ifeq ($(BR2_ENABLE_LOCALE),y)
> +SHADOW_CONF_OPTS += --enable-nls
> +else
> +SHADOW_CONF_OPTS += --disable-nls
> +endif
> +
> +ifeq ($(BR2_PACKAGE_SHADOW_SHA_CRYPT),y)
> +SHADOW_CONF_OPTS += --with-sha-crypt
> +else
> +SHADOW_CONF_OPTS += --without-sha-crypt
> +endif
> +
> +ifeq ($(BR2_PACKAGE_SHADOW_BCRYPT),y)
> +SHADOW_CONF_OPTS += --with-bcrypt
> +else
> +SHADOW_CONF_OPTS += --without-bcrypt
> +endif
> +
> +ifeq ($(BR2_PACKAGE_SHADOW_YESCRYPT),y)
> +SHADOW_CONF_OPTS += --with-yescrypt
> +else
> +SHADOW_CONF_OPTS += --without-yescrypt
> +endif
> +
> +ifeq ($(BR2_PACKAGE_SHADOW_NSCD),y)
> +SHADOW_CONF_OPTS += --with-nscd
> +else
> +SHADOW_CONF_OPTS += --without-nscd
> +endif
> +
> +ifeq ($(BR2_PACKAGE_SHADOW_SSSD),y)
> +SHADOW_CONF_OPTS += --with-sssd
> +else
> +SHADOW_CONF_OPTS += --without-sssd
> +endif
> +
> +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))
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [Buildroot] [PATCH v2 1/1] package/shadow: new package
  2022-09-04 12:43 [Buildroot] [PATCH v2 1/1] package/shadow: new package Raphael Pavlidis
  2022-09-05 10:06 ` Arnout Vandecappelle
@ 2022-09-05 11:51 ` Yann E. MORIN
  2022-09-05 12:01   ` Yann E. MORIN
  2022-09-11 11:22   ` Raphael Pavlidis
  2022-10-13 16:34 ` [Buildroot] [PATCH v3 " Raphael Pavlidis
  2 siblings, 2 replies; 18+ messages in thread
From: Yann E. MORIN @ 2022-09-05 11:51 UTC (permalink / raw)
  To: Raphael Pavlidis; +Cc: Thomas Petazzoni, buildroot

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [Buildroot] [PATCH v2 1/1] package/shadow: new package
  2022-09-05 11:51 ` Yann E. MORIN
@ 2022-09-05 12:01   ` Yann E. MORIN
  2022-09-11 11:22   ` Raphael Pavlidis
  1 sibling, 0 replies; 18+ messages in thread
From: Yann E. MORIN @ 2022-09-05 12:01 UTC (permalink / raw)
  To: Raphael Pavlidis; +Cc: Thomas Petazzoni, buildroot

Raphael, All,

On 2022-09-05 13:51 +0200, Yann E. MORIN spake thusly:
> On 2022-09-04 14:43 +0200, Raphael Pavlidis spake thusly:
> > shadow provides utilities to deal with user accounts.
[--SNIP--]
> > +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.

Oh, and of course, Buildroot can be configured with neither
BR2_STATIC_LIBS nor BR2_SHARED_LIBS, but with BR2_SHARED_STATIC_LIBS,
which means to generate both static and shared, but the code above would
actually disable both.

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [Buildroot] [PATCH v2 1/1] package/shadow: new package
  2022-09-05 11:51 ` Yann E. MORIN
  2022-09-05 12:01   ` Yann E. MORIN
@ 2022-09-11 11:22   ` Raphael Pavlidis
  2022-09-11 12:14     ` Yann E. MORIN
  1 sibling, 1 reply; 18+ messages in thread
From: Raphael Pavlidis @ 2022-09-11 11:22 UTC (permalink / raw)
  To: Yann E. MORIN; +Cc: Thomas Petazzoni, buildroot

Yann, All,

On 05.09.22 13:51, Yann E. MORIN wrote:
> 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...

How about using the description of the GitHub repository? Or is this too 
long? Also, using it as a description in the Config.in?

"The shadow package includes the necessary programs for converting UNIX 
password files to the shadow password format, plus programs for managing 
user and group accounts. The [snip]"

[--SNIP--]>
> 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.

Can you explain, what exactly this option 
BR2_PACKAGE_BUSYBOX_SHOW_OTHERS does? I did not understand it, I 
apologize for the inconvenience.

[--SNIP--]
> 
> 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.

All default values and description of the option were taken from the 
configure.ac file of the repository. The intention behind was, that the 
developers know best which option should be activated on default.

[--SNIP--]>
> 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.

As far I understood it, it uses PAM to authenticate the callers, so the 
user and group management operation should be executed. So, I will 
change it to a select. Thanks for the suggestion.

[--SNIP--]
>> +	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.

The help entry was taken also from the configure.ac file. I will remove 
it. ;)

> 
>> +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.

I will change it. Thanks

[--SNIP--]
>> +
>> +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?
> 

Oh, my mistake. The default value in the configure.ac is 32. Is it okay 
to change it to 32 then?

I also think it should be configurable. The developers provide this 
option, so we should also provide this option to the users of buildroot.

> 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"
> 

I will change the name.

>> +	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.

Okay. :)

> 
> [--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.

Oh, I did not know that. I am relative new here, sorry. I will drop it.

[--SNIP--]
> 
> 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...)
> 

I will change it. :)

[--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.

I like the other way, but if this is required then I will change it. :)

> 
>> +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?

As far as I understood it, shadow also use linux-pam generally, but is 
required if account-tools-setuid is set.


[--SNIP--]
> 
> 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?

Oh, I did not know that. I will drop it.

[--SNIP--]

Thanks,
Raphael Pavlidis
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [Buildroot] [PATCH v2 1/1] package/shadow: new package
  2022-09-11 11:22   ` Raphael Pavlidis
@ 2022-09-11 12:14     ` Yann E. MORIN
  2022-09-11 12:55       ` Raphael Pavlidis
  0 siblings, 1 reply; 18+ messages in thread
From: Yann E. MORIN @ 2022-09-11 12:14 UTC (permalink / raw)
  To: Raphael Pavlidis; +Cc: Thomas Petazzoni, buildroot

Raphael, All,

On 2022-09-11 13:22 +0200, Raphael Pavlidis spake thusly:
> On 05.09.22 13:51, Yann E. MORIN wrote:
> >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...
> How about using the description of the GitHub repository? Or is this too
> long? Also, using it as a description in the Config.in?
> 
> "The shadow package includes the necessary programs for converting UNIX
> password files to the shadow password format, plus programs for managing
> user and group accounts. The [snip]"

Starting the commit log with a terse explanations of the package purpose
is interesting, but what really matters are the details of the
integration in Buildroot.

For example:

    package/shawdow: new package

    shadow provides utilities to deal with user accounts.

    We decided to expose all the options present in configure, as
    options in Config.in, because those are sensitive, security-related
    options, and we want the user to take responsibility on the
    settings. The defaults are as they are exposed by the configure
    script; we especially default the max group name mength to 32,
    because accepting too long group names is a path to DoS attacks.

    Signed-off-by: Your REALNAM <your-email>

Of course, the above is just for demonstration and mostly made up, the
actual commit content should be adapted. But you get the idea.

> [--SNIP--]>
> >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.
> 
> Can you explain, what exactly this option BR2_PACKAGE_BUSYBOX_SHOW_OTHERS
> does? I did not understand it, I apologize for the inconvenience.

Right, this is not trivial. Busybox installs a set of programs, for
example /bin/ash or /bin/wc, which are also provided by the bigger ones,
resp. dash and coreutils.

So, we by default do not want to show dash and coreutils in the
menuconfig, as the programs they install are already installed by
busybox, and even though the busybox variants may be slightly less
capable that the bigger ones, they are far smaller and, more often than
not, are sufficient.

Howerver, in some cases, most busybox applets are enough for the system,
except a very sall subset, for which we want to be able to use the
bigger ones.

That's when BR2_PACKAGE_BUSYBOX_SHOW_OTHERS comes into play: the user
can enable that option in the menuconfig, and so packages that install
the same set of programs as busybox applets, are now visible in the
menuconfig too.

See for example package/dash/Config.in:

    1 config BR2_PACKAGE_DASH
    2     bool "dash"
    3     depends on BR2_USE_MMU # fork()
    4     depends on BR2_PACKAGE_BUSYBOX_SHOW_OTHERS

So, for shadow, I think at least the 'su' option should also depend on
BR2_PACKAGE_BUSYBOX_SHOW_OTHERS, if not the whole package (yet, I'd
vote for the whole package for simplicity sake).

> [--SNIP--]
> >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.
> All default values and description of the option were taken from the
> configure.ac file of the repository. The intention behind was, that the
> developers know best which option should be activated on default.

I see the reasoning.

I'm still not entirely sure, and stating "shadow developpers know best"
is still not very correct, because the one who knows best _in their
specific case_ is the user.

Also, if you did not have an actual use-case for an option, then do not
expose it at all. When/if someone actually has a need for that option,
then they can send a patch to add it.

[--SNIP--]
> >>+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.
> The help entry was taken also from the configure.ac file. I will remove it.
> ;)

Yeah, I get it:
    --enable-foo    Enable foo.

That still does not help at all. Help text should be able to actually
help, and so must provide more info than the prompt does.

[--SNIP--]
> >>+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?
> >
> 
> Oh, my mistake. The default value in the configure.ac is 32. Is it okay to
> change it to 32 then?

You still have to explain why providing a non-zero (thus unlimited)
default is better. Relying on "shadow developpers default that to 32,
so let's do the same" does not help much (but is still better than
not explaining it).

> I also think it should be configurable. The developers provide this option,
> so we should also provide this option to the users of buildroot.

The packaging in Buildroot mostly only (globally) expose just a very
small subset of all options exposed by the configure (or similar)
scripts.

We expose options in the menuconfig only when it actually makes sense.
What is the purpose of limiting the group name length? Why do we want to
allow the user to be able to set that value, rather than let the package
decide?

> >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"
> I will change the name.

Note: it is not the _name_, it is the _prompt_ (i.e. what is shown to
the user).

[--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?
> As far as I understood it, shadow also use linux-pam generally, but is
> required if account-tools-setuid is set.

OK, so this indeed gets a little bit more tricky.

The Config.in entry for account-tools-setuid should indeed use select
(as seen above; plus help text as an example):

    config BR2_PACAKGE_SHADOW_ACCOUNT_TOOLS_SETUID
        bool "account-tool setuid"
        select BR2_PACKAGE_LINUX_PAM
        help
          chmod the account-tool utility setuid, so that non-root
          users can use it, subjet to their PAM profile.

and then the .mk should propbably look like:

    ifeq ($(BR2_PACKAGE_LINUX_PAM),y)
    SHADOW_DEPENDENCIES += linux-pam
    SHADOW_CONF_OPTS += --enable-pam
    else
    SHADOW_CONF_OPTS += --disable-pam
    endif

    ifeq ($(BR2_PACAKGE_SHADOW_ACCOUNT_TOOLS_SETUID),y)
    # PAM dependency handled above
    SHADOW_CONF_OPTS += --enable-account-tools-setuid-I-can-t-remember-the-option-name
    else
    SHADOW_CONF_OPTS += --disable-account-tools-setuid-I-can-t-remember-the-option-name
    endif

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [Buildroot] [PATCH v2 1/1] package/shadow: new package
  2022-09-11 12:14     ` Yann E. MORIN
@ 2022-09-11 12:55       ` Raphael Pavlidis
  2022-09-11 17:57         ` Yann E. MORIN
  0 siblings, 1 reply; 18+ messages in thread
From: Raphael Pavlidis @ 2022-09-11 12:55 UTC (permalink / raw)
  To: Yann E. MORIN; +Cc: Thomas Petazzoni, buildroot

Yann, All,

(Thank you Yann for your review comments)

On 11.09.22 14:14, Yann E. MORIN wrote:
> Raphael, All,
> [--SNIP--]
> 
> Starting the commit log with a terse explanations of the package purpose
> is interesting, but what really matters are the details of the
> integration in Buildroot.
> 
> For example:
> 
>      package/shawdow: new package
> 
>      shadow provides utilities to deal with user accounts.
> 
>      We decided to expose all the options present in configure, as
>      options in Config.in, because those are sensitive, security-related
>      options, and we want the user to take responsibility on the
>      settings. The defaults are as they are exposed by the configure
>      script; we especially default the max group name mength to 32,
>      because accepting too long group names is a path to DoS attacks.
> 
>      Signed-off-by: Your REALNAM <your-email>
> 
> Of course, the above is just for demonstration and mostly made up, the
> actual commit content should be adapted. But you get the idea.

I will try it, thanks. Technically, I need this package to use podman 
for non-root user (newuidmap and newgidmap).

[--SNIP--]
> 
> Right, this is not trivial. Busybox installs a set of programs, for
> example /bin/ash or /bin/wc, which are also provided by the bigger ones,
> resp. dash and coreutils.
> 
> So, we by default do not want to show dash and coreutils in the
> menuconfig, as the programs they install are already installed by
> busybox, and even though the busybox variants may be slightly less
> capable that the bigger ones, they are far smaller and, more often than
> not, are sufficient.
> 
> Howerver, in some cases, most busybox applets are enough for the system,
> except a very sall subset, for which we want to be able to use the
> bigger ones.
> 
> That's when BR2_PACKAGE_BUSYBOX_SHOW_OTHERS comes into play: the user
> can enable that option in the menuconfig, and so packages that install
> the same set of programs as busybox applets, are now visible in the
> menuconfig too.
> 
> See for example package/dash/Config.in:
> 
>      1 config BR2_PACKAGE_DASH
>      2     bool "dash"
>      3     depends on BR2_USE_MMU # fork()
>      4     depends on BR2_PACKAGE_BUSYBOX_SHOW_OTHERS
> 
> So, for shadow, I think at least the 'su' option should also depend on
> BR2_PACKAGE_BUSYBOX_SHOW_OTHERS, if not the whole package (yet, I'd
> vote for the whole package for simplicity sake).

I think I understand it now. It is an option to show option or package, 
which install a non-busybox version of a binary, correct? I will add it 
to the whole package then.

[--SNIP--]
> 
> I see the reasoning.
> 
> I'm still not entirely sure, and stating "shadow developpers know best"
> is still not very correct, because the one who knows best _in their
> specific case_ is the user.
> 
> Also, if you did not have an actual use-case for an option, then do not
> expose it at all. When/if someone actually has a need for that option,
> then they can send a patch to add it.

But this approach, I think have the disadvantage, that if it happens 
that somebody needs an option then he/she have to wait until is there, 
which it can take sometime. (Happen at least to me)

I understand it for such options, which are useless for buildroot like 
if it is something Windows specific. IMHO, I do not see any harm to 
expose those options.

[--SNIP--]
> 
> Yeah, I get it:
>      --enable-foo    Enable foo.
> 
> That still does not help at all. Help text should be able to actually
> help, and so must provide more info than the prompt does.

Okay, I will remove such help text then.

[--SNIP--]> The packaging in Buildroot mostly only (globally) expose 
just a very
> small subset of all options exposed by the configure (or similar)
> scripts.
> 
> We expose options in the menuconfig only when it actually makes sense.
> What is the purpose of limiting the group name length? Why do we want to
> allow the user to be able to set that value, rather than let the package
> decide?
> 

At least in my case, I need only BR2_PACKAGE_SHADOW_SUBORDINATE_IDS, so 
it would be nice that everything else could be deactivated to keep it small.

I tried to figure out, why this option was set to 32, and it seems that 
Linux only support username up to 32 characters. So, I will remove this 
option and set the value to 32 in the package because buildroot is only 
supporting Linux, as far as I know, correct?

https://github.com/shadow-maint/shadow/commit/1882c66bda31e50367d41b36fea41cd04fa19c73

[--SNIP--]
> 
> Note: it is not the _name_, it is the _prompt_ (i.e. what is shown to
> the user).

Sorry, I meant the prompt ;)

[--SNIP--]
> 
> OK, so this indeed gets a little bit more tricky.
> 
> The Config.in entry for account-tools-setuid should indeed use select
> (as seen above; plus help text as an example):
> 
>      config BR2_PACAKGE_SHADOW_ACCOUNT_TOOLS_SETUID
>          bool "account-tool setuid"
>          select BR2_PACKAGE_LINUX_PAM
>          help
>            chmod the account-tool utility setuid, so that non-root
>            users can use it, subjet to their PAM profile.
> 
> and then the .mk should propbably look like:
> 
>      ifeq ($(BR2_PACKAGE_LINUX_PAM),y)
>      SHADOW_DEPENDENCIES += linux-pam
>      SHADOW_CONF_OPTS += --enable-pam
>      else
>      SHADOW_CONF_OPTS += --disable-pam
>      endif
> 
>      ifeq ($(BR2_PACAKGE_SHADOW_ACCOUNT_TOOLS_SETUID),y)
>      # PAM dependency handled above
>      SHADOW_CONF_OPTS += --enable-account-tools-setuid-I-can-t-remember-the-option-name
>      else
>      SHADOW_CONF_OPTS += --disable-account-tools-setuid-I-can-t-remember-the-option-name
>      endif

Okay, I will change it then.

> 
> Regards,
> Yann E. MORIN.
>
Thanks,
Raphael Pavlidis
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [Buildroot] [PATCH v2 1/1] package/shadow: new package
  2022-09-11 12:55       ` Raphael Pavlidis
@ 2022-09-11 17:57         ` Yann E. MORIN
  0 siblings, 0 replies; 18+ messages in thread
From: Yann E. MORIN @ 2022-09-11 17:57 UTC (permalink / raw)
  To: Raphael Pavlidis; +Cc: Thomas Petazzoni, buildroot

Raphael, All,

On 2022-09-11 14:55 +0200, Raphael Pavlidis spake thusly:
> On 11.09.22 14:14, Yann E. MORIN wrote:
[--SNIP--]
> >Of course, the above is just for demonstration and mostly made up, the
> >actual commit content should be adapted. But you get the idea.
> I will try it, thanks. Technically, I need this package to use podman for
> non-root user (newuidmap and newgidmap).

You can also indeed add a (terse) explanation why that package is
useful, indeed, as that explains the purpose and can then lead to a
better understanding of the integration. The sentence above is good, but
should be rephrased to a more neutral form:

    shadow is used by podman to enable support for non-root users (with
    newuidmap and newgidmap).

(again, adapt as appropriate.)

[--SNIP--]
> >So, for shadow, I think at least the 'su' option should also depend on
> >BR2_PACKAGE_BUSYBOX_SHOW_OTHERS, if not the whole package (yet, I'd
> >vote for the whole package for simplicity sake).
> I think I understand it now. It is an option to show option or package,
> which install a non-busybox version of a binary, correct? I will add it to
> the whole package then.

Yes, you got it. :-)

[--SNIP--]
> >Also, if you did not have an actual use-case for an option, then do not
> >expose it at all. When/if someone actually has a need for that option,
> >then they can send a patch to add it.
> But this approach, I think have the disadvantage, that if it happens that
> somebody needs an option then he/she have to wait until is there, which it
> can take sometime. (Happen at least to me)
> I understand it for such options, which are useless for buildroot like if it
> is something Windows specific. IMHO, I do not see any harm to expose those
> options.

But exposing options you did not have a need for, and thus did not
exercise, means they can easily be mis-handled. Case in point: the max
length for group names.

[--SNIP--]
> >We expose options in the menuconfig only when it actually makes sense.
> >What is the purpose of limiting the group name length? Why do we want to
> >allow the user to be able to set that value, rather than let the package
> >decide?
> At least in my case, I need only BR2_PACKAGE_SHADOW_SUBORDINATE_IDS, so it
> would be nice that everything else could be deactivated to keep it small.

If you have a need for the option and you did exercise it, then that's
fine exposing it in the menuconfig.

If you want to "keep it small", then just make it so by disabling
everything, and leave it to people that actually need an option and can
test it, to add support for it.

Yes, it can take some time before a new feature lands in Buildroot.

> I tried to figure out, why this option was set to 32, and it seems that
> Linux only support username up to 32 characters. So, I will remove this
> option and set the value to 32 in the package because buildroot is only
> supporting Linux, as far as I know, correct?
> 
> https://github.com/shadow-maint/shadow/commit/1882c66bda31e50367d41b36fea41cd04fa19c73

Aha! ;-)

I won't say I knew it, but I really suspected something along those
lines. Indeed, we do not need to expose that option. It is a very good
example that exposing an untested option is not correct.

Just do not set that at all; just let the configure script use its
default. It is better to leave that untouched.

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [Buildroot] [PATCH v3 1/1] package/shadow: new package
  2022-09-04 12:43 [Buildroot] [PATCH v2 1/1] package/shadow: new package Raphael Pavlidis
  2022-09-05 10:06 ` Arnout Vandecappelle
  2022-09-05 11:51 ` Yann E. MORIN
@ 2022-10-13 16:34 ` Raphael Pavlidis
  2022-12-05 15:48   ` Nicolas Carrier
  2022-12-05 21:55   ` Yann E. MORIN
  2 siblings, 2 replies; 18+ messages in thread
From: Raphael Pavlidis @ 2022-10-13 16:34 UTC (permalink / raw)
  To: buildroot; +Cc: Raphael Pavlidis, Yann E . MORIN, Thomas Petazzoni

shadow provides utilities to deal with user accounts.

The shadow package includes the necessary programs for converting UNIX
password files to the shadow password format, plus programs for managing
user and group accounts. Especially it is useful if rootless podman
container should be used, which requires newuidmap and newgidmap.

Signed-off-by: Raphael Pavlidis <raphael.pavlidis@gmail.com>
---
Changes v2 -> v3:
- remove nscd support
- remove sssd support
- remove group name max length parameter
- remove su build
- improve help text of subordinate-ids
- use a define instead of variable for SHADOW_ACCOUNT_TOOLS_SETUID
  SHADOW_SUBORDINATE_IDS_PERMISSIONS and 

Changes v1 -> v2:
- DEVELOPERS: add Raphael Pavlids for shadow

 DEVELOPERS                 |   3 +-
 package/Config.in          |   1 +
 package/shadow/Config.in   |  61 +++++++++++++++++
 package/shadow/shadow.hash |   3 +
 package/shadow/shadow.mk   | 133 +++++++++++++++++++++++++++++++++++++
 5 files changed, 200 insertions(+), 1 deletion(-)
 create mode 100644 package/shadow/Config.in
 create mode 100644 package/shadow/shadow.hash
 create mode 100644 package/shadow/shadow.mk

diff --git a/DEVELOPERS b/DEVELOPERS
index 59121c6a54..0dad0ba0ba 100644
--- a/DEVELOPERS
+++ b/DEVELOPERS
@@ -2474,7 +2474,8 @@ F:	support/testing/tests/package/test_python_jmespath.py
 F:	support/testing/tests/package/test_python_rsa.py
 F:	support/testing/tests/package/test_python_s3transfer.py
 
-N:	Raphael Pavlidis <raphael.pavlidis@googlemail.com>
+N:	Raphael Pavlidis <raphael.pavlidis@gmail.com>
+F:	package/shadow/
 F:	package/slirp4netns/
 
 N:	Refik Tuzakli <tuzakli.refik@gmail.com>
diff --git a/package/Config.in b/package/Config.in
index e3a34d6e97..d9ead48647 100644
--- a/package/Config.in
+++ b/package/Config.in
@@ -2642,6 +2642,7 @@ menu "System tools"
 	source "package/sdbus-cpp/Config.in"
 	source "package/sdbusplus/Config.in"
 	source "package/seatd/Config.in"
+	source "package/shadow/Config.in"
 	source "package/smack/Config.in"
 	source "package/start-stop-daemon/Config.in"
 	source "package/supervisor/Config.in"
diff --git a/package/shadow/Config.in b/package/shadow/Config.in
new file mode 100644
index 0000000000..6b1fe0a61f
--- /dev/null
+++ b/package/shadow/Config.in
@@ -0,0 +1,61 @@
+menuconfig BR2_PACKAGE_SHADOW
+	bool "shadow"
+	depends on BR2_TOOLCHAIN_HEADERS_AT_LEAST_4_14
+	help
+	  Utilities to deal with user accounts.
+
+	  https://github.com/shadow-maint/shadow
+
+if BR2_PACKAGE_SHADOW
+
+config BR2_PACKAGE_SHADOW_SHADOWGRP
+	bool "shadowgrp"
+	help
+	  Enable shadow group support.
+
+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
+	help
+	  Install the user and group management tools (e.g. groupadd) with setuid and
+	  authenticate the callers via 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
+
+config BR2_PACKAGE_SHADOW_UTMPX
+	bool "utmpx"
+	help
+	  Enable loggin in utmpx / wtmpx.
+
+config BR2_PACKAGE_SHADOW_SUBORDINATE_IDS
+	bool "subordinate-ids"
+	help
+	  Support subordinate ids. Helpful to use container solution like podman
+	  without root.
+
+config BR2_PACKAGE_SHADOW_SHA_CRYPT
+	bool "sha-crypt"
+	default y
+	help
+	  Allow the SHA256 and SHA512 password encryption algorithms.
+
+config BR2_PACKAGE_SHADOW_BCRYPT
+	bool "bcrypt"
+	help
+	  Allow the bcrypt password encryption algorithm.
+
+config BR2_PACKAGE_SHADOW_YESCRYPT
+	bool "yescrypt"
+	help
+	  Allow the yescrypt password encryption algorithm.
+
+endif # BR2_PACKAGE_SHADOW
+
+comment "shadow needs a toolchain w/ headers >= 4.14"
+	depends on !BR2_TOOLCHAIN_HEADERS_AT_LEAST_4_14
diff --git a/package/shadow/shadow.hash b/package/shadow/shadow.hash
new file mode 100644
index 0000000000..6b9faac10f
--- /dev/null
+++ b/package/shadow/shadow.hash
@@ -0,0 +1,3 @@
+# Locally computed
+sha256  41f093ce58b2ae5f389a1c5553e0c18bc73e6fe27f66273891991198a7707c95  shadow-4.11.1.tar.xz
+sha256  3d25ab8f43fdc14624296a56ff8dc3e72e499ad35f32ae0c803f4959cfe17c0a  COPYING
diff --git a/package/shadow/shadow.mk b/package/shadow/shadow.mk
new file mode 100644
index 0000000000..261f28dd28
--- /dev/null
+++ b/package/shadow/shadow.mk
@@ -0,0 +1,133 @@
+################################################################################
+#
+# 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 = \
+	--disable-man \
+    --without-btrfs \
+    --without-nscd \
+    --without-skey \
+    --without-sssd \
+    --without-su \
+    --without-tcb
+
+ifeq ($(BR2_PACKAGE_SHADOW_SHADOWGRP),y)
+SHADOW_CONF_OPTS += --enable-shadowgrp
+else
+SHADOW_CONF_OPTS += --disable-shadowgrp
+endif
+
+ifeq ($(BR2_PACKAGE_SHADOW_ACCOUNT_TOOLS_SETUID),y)
+SHADOW_CONF_OPTS += --enable-account-tools-setuid
+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
+else
+SHADOW_CONF_OPTS += --disable-account-tools-setuid
+endif
+
+ifeq ($(BR2_PACKAGE_SHADOW_UTMPX),y)
+SHADOW_CONF_OPTS += --enable-utmpx
+else
+SHADOW_CONF_OPTS += --disable-utmpx
+endif
+
+ifeq ($(BR2_PACKAGE_SHADOW_SUBORDINATE_IDS),y)
+SHADOW_CONF_OPTS += --enable-subordinate-ids
+define SHADOW_SUBORDINATE_IDS_PERMISSIONS
+	/usr/bin/newuidmap f 4755 0 0 - - - - -
+	/usr/bin/newgidmap f 4755 0 0 - - - - -
+endef
+else
+SHADOW_CONF_OPTS += --disable-subordinate-ids
+endif
+
+ifeq ($(BR2_PACKAGE_ACL),y)
+SHADOW_CONF_OPTS += --with-acl
+SHADOW_DEPENDENCIES += acl
+else
+SHADOW_CONF_OPTS += --without-acl
+endif
+
+ifeq ($(BR2_PACKAGE_ATTR),y)
+SHADOW_CONF_OPTS += --with-attr
+SHADOW_DEPENDENCIES += attr
+else
+SHADOW_CONF_OPTS += --without-attr
+endif
+
+ifeq ($(BR2_PACKAGE_AUDIT),y)
+SHADOW_CONF_OPTS += --with-audit
+SHADOW_DEPENDENCIES += audit
+else
+SHADOW_CONF_OPTS += --without-audit
+endif
+
+ifeq ($(BR2_PACKAGE_CRACKLIB),y)
+SHADOW_CONF_OPTS += --with-libcrack
+SHADOW_DEPENDENCIES += cracklib
+else
+SHADOW_CONF_OPTS += --without-libcrack
+endif
+
+ifeq ($(BR2_PACKAGE_LIBSELINUX),y)
+SHADOW_CONF_OPTS += --with-selinux
+SHADOW_DEPENDENCIES += libselinux libsemanage
+else
+SHADOW_CONF_OPTS += --without-selinux
+endif
+
+# linux-pam is also used without account-tools-setuid enabled
+ifeq ($(BR2_PACKAGE_LINUX_PAM),y)
+SHADOW_CONF_OPTS += --with-libpam
+SHADOW_DEPENDENCIES += linux-pam
+else
+SHADOW_CONF_OPTS += --without-libpam
+endif
+
+ifeq ($(BR2_PACKAGE_SHADOW_SHA_CRYPT),y)
+SHADOW_CONF_OPTS += --with-sha-crypt
+else
+SHADOW_CONF_OPTS += --without-sha-crypt
+endif
+
+ifeq ($(BR2_PACKAGE_SHADOW_BCRYPT),y)
+SHADOW_CONF_OPTS += --with-bcrypt
+else
+SHADOW_CONF_OPTS += --without-bcrypt
+endif
+
+ifeq ($(BR2_PACKAGE_SHADOW_YESCRYPT),y)
+SHADOW_CONF_OPTS += --with-yescrypt
+else
+SHADOW_CONF_OPTS += --without-yescrypt
+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)
+endef
+
+$(eval $(autotools-package))
-- 
2.35.1

_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [Buildroot] [PATCH v3 1/1] package/shadow: new package
  2022-10-13 16:34 ` [Buildroot] [PATCH v3 " Raphael Pavlidis
@ 2022-12-05 15:48   ` Nicolas Carrier
  2022-12-05 21:55   ` Yann E. MORIN
  1 sibling, 0 replies; 18+ messages in thread
From: Nicolas Carrier @ 2022-12-05 15:48 UTC (permalink / raw)
  To: Raphael Pavlidis, buildroot; +Cc: Yann E . MORIN, Thomas Petazzoni

I tested it with a basic config (in fact, a draft unit test) and it compiles fine and the commands I tested worked fine.

Tested-by: Nicolas Carrier <nicolas.carrier@orolia.com>
Approved-by: Nicolas Carrier <nicolas.carrier@orolia.com>


Nicolas Carrier | Software Developer | nicolas.carrier@orolia.com


De : buildroot <buildroot-bounces@buildroot.org> de la part de Raphael Pavlidis <raphael.pavlidis@gmail.com>
Envoyé : jeudi 13 octobre 2022 18:34
À : buildroot@buildroot.org <buildroot@buildroot.org>
Cc : Raphael Pavlidis <raphael.pavlidis@gmail.com>; Yann E . MORIN <yann.morin.1998@free.fr>; Thomas Petazzoni <thomas.petazzoni@bootlin.com>
Objet : [Buildroot] [PATCH v3 1/1] package/shadow: new package 
 
CAUTION: This email originated from outside of the organization.
Do not click links or open attachments unless you recognize the sender and know the content is safe.

shadow provides utilities to deal with user accounts.

The shadow package includes the necessary programs for converting UNIX
password files to the shadow password format, plus programs for managing
user and group accounts. Especially it is useful if rootless podman
container should be used, which requires newuidmap and newgidmap.

Signed-off-by: Raphael Pavlidis <raphael.pavlidis@gmail.com>
---
Changes v2 -> v3:
- remove nscd support
- remove sssd support
- remove group name max length parameter
- remove su build
- improve help text of subordinate-ids
- use a define instead of variable for SHADOW_ACCOUNT_TOOLS_SETUID
  SHADOW_SUBORDINATE_IDS_PERMISSIONS and

Changes v1 -> v2:
- DEVELOPERS: add Raphael Pavlids for shadow

 DEVELOPERS                 |   3 +-
 package/Config.in          |   1 +
 package/shadow/Config.in   |  61 +++++++++++++++++
 package/shadow/shadow.hash |   3 +
 package/shadow/shadow.mk   | 133 +++++++++++++++++++++++++++++++++++++
 5 files changed, 200 insertions(+), 1 deletion(-)
 create mode 100644 package/shadow/Config.in
 create mode 100644 package/shadow/shadow.hash
 create mode 100644 package/shadow/shadow.mk

diff --git a/DEVELOPERS b/DEVELOPERS
index 59121c6a54..0dad0ba0ba 100644
--- a/DEVELOPERS
+++ b/DEVELOPERS
@@ -2474,7 +2474,8 @@ F:        support/testing/tests/package/test_python_jmespath.py
 F:     support/testing/tests/package/test_python_rsa.py
 F:     support/testing/tests/package/test_python_s3transfer.py

-N:     Raphael Pavlidis <raphael.pavlidis@googlemail.com>
+N:     Raphael Pavlidis <raphael.pavlidis@gmail.com>
+F:     package/shadow/
 F:     package/slirp4netns/

 N:     Refik Tuzakli <tuzakli.refik@gmail.com>
diff --git a/package/Config.in b/package/Config.in
index e3a34d6e97..d9ead48647 100644
--- a/package/Config.in
+++ b/package/Config.in
@@ -2642,6 +2642,7 @@ menu "System tools"
        source "package/sdbus-cpp/Config.in"
        source "package/sdbusplus/Config.in"
        source "package/seatd/Config.in"
+       source "package/shadow/Config.in"
        source "package/smack/Config.in"
        source "package/start-stop-daemon/Config.in"
        source "package/supervisor/Config.in"
diff --git a/package/shadow/Config.in b/package/shadow/Config.in
new file mode 100644
index 0000000000..6b1fe0a61f
--- /dev/null
+++ b/package/shadow/Config.in
@@ -0,0 +1,61 @@
+menuconfig BR2_PACKAGE_SHADOW
+       bool "shadow"
+       depends on BR2_TOOLCHAIN_HEADERS_AT_LEAST_4_14
+       help
+         Utilities to deal with user accounts.
+
+         https://github.com/shadow-maint/shadow
+
+if BR2_PACKAGE_SHADOW
+
+config BR2_PACKAGE_SHADOW_SHADOWGRP
+       bool "shadowgrp"
+       help
+         Enable shadow group support.
+
+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
+       help
+         Install the user and group management tools (e.g. groupadd) with setuid and
+         authenticate the callers via 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
+
+config BR2_PACKAGE_SHADOW_UTMPX
+       bool "utmpx"
+       help
+         Enable loggin in utmpx / wtmpx.
+
+config BR2_PACKAGE_SHADOW_SUBORDINATE_IDS
+       bool "subordinate-ids"
+       help
+         Support subordinate ids. Helpful to use container solution like podman
+         without root.
+
+config BR2_PACKAGE_SHADOW_SHA_CRYPT
+       bool "sha-crypt"
+       default y
+       help
+         Allow the SHA256 and SHA512 password encryption algorithms.
+
+config BR2_PACKAGE_SHADOW_BCRYPT
+       bool "bcrypt"
+       help
+         Allow the bcrypt password encryption algorithm.
+
+config BR2_PACKAGE_SHADOW_YESCRYPT
+       bool "yescrypt"
+       help
+         Allow the yescrypt password encryption algorithm.
+
+endif # BR2_PACKAGE_SHADOW
+
+comment "shadow needs a toolchain w/ headers >= 4.14"
+       depends on !BR2_TOOLCHAIN_HEADERS_AT_LEAST_4_14
diff --git a/package/shadow/shadow.hash b/package/shadow/shadow.hash
new file mode 100644
index 0000000000..6b9faac10f
--- /dev/null
+++ b/package/shadow/shadow.hash
@@ -0,0 +1,3 @@
+# Locally computed
+sha256  41f093ce58b2ae5f389a1c5553e0c18bc73e6fe27f66273891991198a7707c95  shadow-4.11.1.tar.xz
+sha256  3d25ab8f43fdc14624296a56ff8dc3e72e499ad35f32ae0c803f4959cfe17c0a  COPYING
diff --git a/package/shadow/shadow.mk b/package/shadow/shadow.mk
new file mode 100644
index 0000000000..261f28dd28
--- /dev/null
+++ b/package/shadow/shadow.mk
@@ -0,0 +1,133 @@
+################################################################################
+#
+# 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 = \
+       --disable-man \
+    --without-btrfs \
+    --without-nscd \
+    --without-skey \
+    --without-sssd \
+    --without-su \
+    --without-tcb
+
+ifeq ($(BR2_PACKAGE_SHADOW_SHADOWGRP),y)
+SHADOW_CONF_OPTS += --enable-shadowgrp
+else
+SHADOW_CONF_OPTS += --disable-shadowgrp
+endif
+
+ifeq ($(BR2_PACKAGE_SHADOW_ACCOUNT_TOOLS_SETUID),y)
+SHADOW_CONF_OPTS += --enable-account-tools-setuid
+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
+else
+SHADOW_CONF_OPTS += --disable-account-tools-setuid
+endif
+
+ifeq ($(BR2_PACKAGE_SHADOW_UTMPX),y)
+SHADOW_CONF_OPTS += --enable-utmpx
+else
+SHADOW_CONF_OPTS += --disable-utmpx
+endif
+
+ifeq ($(BR2_PACKAGE_SHADOW_SUBORDINATE_IDS),y)
+SHADOW_CONF_OPTS += --enable-subordinate-ids
+define SHADOW_SUBORDINATE_IDS_PERMISSIONS
+       /usr/bin/newuidmap f 4755 0 0 - - - - -
+       /usr/bin/newgidmap f 4755 0 0 - - - - -
+endef
+else
+SHADOW_CONF_OPTS += --disable-subordinate-ids
+endif
+
+ifeq ($(BR2_PACKAGE_ACL),y)
+SHADOW_CONF_OPTS += --with-acl
+SHADOW_DEPENDENCIES += acl
+else
+SHADOW_CONF_OPTS += --without-acl
+endif
+
+ifeq ($(BR2_PACKAGE_ATTR),y)
+SHADOW_CONF_OPTS += --with-attr
+SHADOW_DEPENDENCIES += attr
+else
+SHADOW_CONF_OPTS += --without-attr
+endif
+
+ifeq ($(BR2_PACKAGE_AUDIT),y)
+SHADOW_CONF_OPTS += --with-audit
+SHADOW_DEPENDENCIES += audit
+else
+SHADOW_CONF_OPTS += --without-audit
+endif
+
+ifeq ($(BR2_PACKAGE_CRACKLIB),y)
+SHADOW_CONF_OPTS += --with-libcrack
+SHADOW_DEPENDENCIES += cracklib
+else
+SHADOW_CONF_OPTS += --without-libcrack
+endif
+
+ifeq ($(BR2_PACKAGE_LIBSELINUX),y)
+SHADOW_CONF_OPTS += --with-selinux
+SHADOW_DEPENDENCIES += libselinux libsemanage
+else
+SHADOW_CONF_OPTS += --without-selinux
+endif
+
+# linux-pam is also used without account-tools-setuid enabled
+ifeq ($(BR2_PACKAGE_LINUX_PAM),y)
+SHADOW_CONF_OPTS += --with-libpam
+SHADOW_DEPENDENCIES += linux-pam
+else
+SHADOW_CONF_OPTS += --without-libpam
+endif
+
+ifeq ($(BR2_PACKAGE_SHADOW_SHA_CRYPT),y)
+SHADOW_CONF_OPTS += --with-sha-crypt
+else
+SHADOW_CONF_OPTS += --without-sha-crypt
+endif
+
+ifeq ($(BR2_PACKAGE_SHADOW_BCRYPT),y)
+SHADOW_CONF_OPTS += --with-bcrypt
+else
+SHADOW_CONF_OPTS += --without-bcrypt
+endif
+
+ifeq ($(BR2_PACKAGE_SHADOW_YESCRYPT),y)
+SHADOW_CONF_OPTS += --with-yescrypt
+else
+SHADOW_CONF_OPTS += --without-yescrypt
+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)
+endef
+
+$(eval $(autotools-package))
--
2.35.1

_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [Buildroot] [PATCH v3 1/1] package/shadow: new package
  2022-10-13 16:34 ` [Buildroot] [PATCH v3 " Raphael Pavlidis
  2022-12-05 15:48   ` Nicolas Carrier
@ 2022-12-05 21:55   ` Yann E. MORIN
  2022-12-06 18:20     ` Raphael Pavlidis
  1 sibling, 1 reply; 18+ messages in thread
From: Yann E. MORIN @ 2022-12-05 21:55 UTC (permalink / raw)
  To: Raphael Pavlidis; +Cc: Thomas Petazzoni, buildroot

Raphael, All,

On 2022-10-13 18:34 +0200, Raphael Pavlidis spake thusly:
> shadow provides utilities to deal with user accounts.
> 
> The shadow package includes the necessary programs for converting UNIX
> password files to the shadow password format, plus programs for managing
> user and group accounts. Especially it is useful if rootless podman
> container should be used, which requires newuidmap and newgidmap.
> 
> Signed-off-by: Raphael Pavlidis <raphael.pavlidis@gmail.com>

I was about to apply this, after fixing the minor issues (see below),
but there is a rather major blocker, see below too...

> ---
[--SNIP--]
> diff --git a/package/shadow/Config.in b/package/shadow/Config.in
> new file mode 100644
> index 0000000000..6b1fe0a61f
> --- /dev/null
> +++ b/package/shadow/Config.in
> @@ -0,0 +1,61 @@
[--SNIP--]
> +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
> +	help
> +	  Install the user and group management tools (e.g. groupadd) with setuid and

    $ make check-package
    package/shadow/Config.in:24: help text: <tab><2 spaces><62 chars> (http://nightly.buildroot.org/#writing-rules-config-in)

[--SNIP--]
> +config BR2_PACKAGE_SHADOW_SUBORDINATE_IDS
> +	bool "subordinate-ids"
> +	help
> +	  Support subordinate ids. Helpful to use container solution like podman

    $ make check-package
    package/shadow/Config.in:39: help text: <tab><2 spaces><62 chars> (http://nightly.buildroot.org/#writing-rules-config-in)

[--SNIP--]
> diff --git a/package/shadow/shadow.mk b/package/shadow/shadow.mk
> new file mode 100644
> index 0000000000..261f28dd28
> --- /dev/null
> +++ b/package/shadow/shadow.mk
> @@ -0,0 +1,133 @@
> +################################################################################
> +#
> +# shadow
> +#
> +################################################################################
> +
> +SHADOW_VERSION = 4.11.1

Why 4.11.1? It was released in 2022-01-03, and is affected by
CVE-2013-4235, with version 4.12.2 being the first to include the fix
for it, and there is now 4.13:

    https://www.cve.org/CVERecord?id=CVE-2013-4235
    https://github.com/shadow-maint/shadow/releases/tag/4.12.2
    https://github.com/shadow-maint/shadow/pull/545

> +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

And:

    SHADOW_CPE_ID_VENDOR = debian

=> https://nvd.nist.gov/products/cpe/detail/11DE0412-97D8-4ABC-9807-101628A40DBE?namingFormat=2.3&orderBy=CPEURI&keyword=shadow&status=FINAL

> +SHADOW_CONF_OPTS = \
> +	--disable-man \
> +    --without-btrfs \
> +    --without-nscd \
> +    --without-skey \
> +    --without-sssd \
> +    --without-su \
> +    --without-tcb

    $ make check-package
    package/shadow/shadow.mk:15: expected indent with tabs
    package/shadow/shadow.mk:16: expected indent with tabs
    package/shadow/shadow.mk:17: expected indent with tabs
    package/shadow/shadow.mk:18: expected indent with tabs
    package/shadow/shadow.mk:19: expected indent with tabs
    package/shadow/shadow.mk:20: expected indent with tabs

> +ifeq ($(BR2_PACKAGE_SHADOW_SHADOWGRP),y)
> +SHADOW_CONF_OPTS += --enable-shadowgrp
> +else
> +SHADOW_CONF_OPTS += --disable-shadowgrp
> +endif
> +
> +ifeq ($(BR2_PACKAGE_SHADOW_ACCOUNT_TOOLS_SETUID),y)
> +SHADOW_CONF_OPTS += --enable-account-tools-setuid
> +define SHADOW_ACCOUNT_TOOLS_SETUID_PERMISSIONS

This is named SHADOW_ACCOUNT_TOOLS_SETUID_PERMISSIONS, but [0]...

> +	/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 - - - - -

What about userdel?

[--SNIP--]
> +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)

... [0] here the expansion uses the wrong name...

So, I had fixed all the minor issues, but the version bump will require
a bit more testing that I can do locally. Nicolas (in Cc) who reviewed
this patch, said he had a runtime test; maybe you can both sync to get
that test part of the series when you respin?

Regards,
Yann E. MORIN.

> +	$(SHADOW_SUBORDINATE_IDS_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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [Buildroot] [PATCH v3 1/1] package/shadow: new package
  2022-12-05 21:55   ` Yann E. MORIN
@ 2022-12-06 18:20     ` Raphael Pavlidis
  2022-12-08 15:15       ` Nicolas Carrier
  0 siblings, 1 reply; 18+ messages in thread
From: Raphael Pavlidis @ 2022-12-06 18:20 UTC (permalink / raw)
  To: Yann E. MORIN; +Cc: Thomas Petazzoni, buildroot

Yann, All,

Thanks for your review, again. I will create a new patch and add Nicolas 
to the CC.

Regards,
Raphael Pavlidis

On 05.12.22 22:55, Yann E. MORIN wrote:
> Raphael, All,
> 
> On 2022-10-13 18:34 +0200, Raphael Pavlidis spake thusly:
>> shadow provides utilities to deal with user accounts.
>>
>> The shadow package includes the necessary programs for converting UNIX
>> password files to the shadow password format, plus programs for managing
>> user and group accounts. Especially it is useful if rootless podman
>> container should be used, which requires newuidmap and newgidmap.
>>
>> Signed-off-by: Raphael Pavlidis <raphael.pavlidis@gmail.com>
> 
> I was about to apply this, after fixing the minor issues (see below),
> but there is a rather major blocker, see below too...
> 
>> ---
> [--SNIP--]
>> diff --git a/package/shadow/Config.in b/package/shadow/Config.in
>> new file mode 100644
>> index 0000000000..6b1fe0a61f
>> --- /dev/null
>> +++ b/package/shadow/Config.in
>> @@ -0,0 +1,61 @@
> [--SNIP--]
>> +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
>> +	help
>> +	  Install the user and group management tools (e.g. groupadd) with setuid and
> 
>      $ make check-package
>      package/shadow/Config.in:24: help text: <tab><2 spaces><62 chars> (http://nightly.buildroot.org/#writing-rules-config-in)
> 
> [--SNIP--]
>> +config BR2_PACKAGE_SHADOW_SUBORDINATE_IDS
>> +	bool "subordinate-ids"
>> +	help
>> +	  Support subordinate ids. Helpful to use container solution like podman
> 
>      $ make check-package
>      package/shadow/Config.in:39: help text: <tab><2 spaces><62 chars> (http://nightly.buildroot.org/#writing-rules-config-in)
> 
> [--SNIP--]
>> diff --git a/package/shadow/shadow.mk b/package/shadow/shadow.mk
>> new file mode 100644
>> index 0000000000..261f28dd28
>> --- /dev/null
>> +++ b/package/shadow/shadow.mk
>> @@ -0,0 +1,133 @@
>> +################################################################################
>> +#
>> +# shadow
>> +#
>> +################################################################################
>> +
>> +SHADOW_VERSION = 4.11.1
> 
> Why 4.11.1? It was released in 2022-01-03, and is affected by
> CVE-2013-4235, with version 4.12.2 being the first to include the fix
> for it, and there is now 4.13:
> 
>      https://www.cve.org/CVERecord?id=CVE-2013-4235
>      https://github.com/shadow-maint/shadow/releases/tag/4.12.2
>      https://github.com/shadow-maint/shadow/pull/545
> 
>> +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
> 
> And:
> 
>      SHADOW_CPE_ID_VENDOR = debian
> 
> => https://nvd.nist.gov/products/cpe/detail/11DE0412-97D8-4ABC-9807-101628A40DBE?namingFormat=2.3&orderBy=CPEURI&keyword=shadow&status=FINAL
> 
>> +SHADOW_CONF_OPTS = \
>> +	--disable-man \
>> +    --without-btrfs \
>> +    --without-nscd \
>> +    --without-skey \
>> +    --without-sssd \
>> +    --without-su \
>> +    --without-tcb
> 
>      $ make check-package
>      package/shadow/shadow.mk:15: expected indent with tabs
>      package/shadow/shadow.mk:16: expected indent with tabs
>      package/shadow/shadow.mk:17: expected indent with tabs
>      package/shadow/shadow.mk:18: expected indent with tabs
>      package/shadow/shadow.mk:19: expected indent with tabs
>      package/shadow/shadow.mk:20: expected indent with tabs
> 
>> +ifeq ($(BR2_PACKAGE_SHADOW_SHADOWGRP),y)
>> +SHADOW_CONF_OPTS += --enable-shadowgrp
>> +else
>> +SHADOW_CONF_OPTS += --disable-shadowgrp
>> +endif
>> +
>> +ifeq ($(BR2_PACKAGE_SHADOW_ACCOUNT_TOOLS_SETUID),y)
>> +SHADOW_CONF_OPTS += --enable-account-tools-setuid
>> +define SHADOW_ACCOUNT_TOOLS_SETUID_PERMISSIONS
> 
> This is named SHADOW_ACCOUNT_TOOLS_SETUID_PERMISSIONS, but [0]...
> 
>> +	/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 - - - - -
> 
> What about userdel?
> 
> [--SNIP--]
>> +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)
> 
> ... [0] here the expansion uses the wrong name...
> 
> So, I had fixed all the minor issues, but the version bump will require
> a bit more testing that I can do locally. Nicolas (in Cc) who reviewed
> this patch, said he had a runtime test; maybe you can both sync to get
> that test part of the series when you respin?
> 
> Regards,
> Yann E. MORIN.
> 
>> +	$(SHADOW_SUBORDINATE_IDS_PERMISSIONS)
>> +endef
>> +
>> +$(eval $(autotools-package))
>> -- 
>> 2.35.1
>>
>> _______________________________________________
>> buildroot mailing list
>> buildroot@buildroot.org
>> https://lists.buildroot.org/mailman/listinfo/buildroot
> 
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [Buildroot] [PATCH v3 1/1] package/shadow: new package
  2022-12-06 18:20     ` Raphael Pavlidis
@ 2022-12-08 15:15       ` Nicolas Carrier
  2022-12-09 10:24         ` Raphael Pavlidis
  0 siblings, 1 reply; 18+ messages in thread
From: Nicolas Carrier @ 2022-12-08 15:15 UTC (permalink / raw)
  To: Raphael Pavlidis, Yann E. MORIN; +Cc: Thomas Petazzoni, buildroot

Ok, so here is the (non exhaustive!) test I used to test the shadow package (in the support/testing/tests/package/test_shadow.py file, using this command line ./support/testing/run-tests -d dl -o output_folder -k tests.pack
age.test_shadow.TestShadow):

import os

from infra.basetest import BRTest, BASIC_TOOLCHAIN_CONFIG


class TestShadow(BRTest):
    username = 'user_test'
    config = BASIC_TOOLCHAIN_CONFIG + \
        """
        BR2_arm=y
        BR2_PACKAGE_SHADOW=y
        BR2_TARGET_ROOTFS_EXT2=y
        BR2_TARGET_ROOTFS_EXT2_4=y
        BR2_TARGET_ROOTFS_EXT2_SIZE="65536"
        """
    timeout = 60

    def login(self):
        img = os.path.join(self.builddir, "images", "rootfs.ext4")
        self.emulator.boot(arch="armv7",
                           kernel="builtin",
                           kernel_cmdline=["root=/dev/mmcblk0",
                                           "rootfstype=ext4"],
                           options=["-drive", f"file={img},if=sd,format=raw"])
        self.emulator.login()

    def test_nologin(self):
        self.login()

        self.assertRunOk("! nologin")
        cmd = 'test "$(nologin)" = "This account is currently not available."'
        self.assertRunOk(cmd)

    def test_useradd_del(self):
        username = self.username
        self.login()

        self.assertRunOk(f'userdel {username} || true')
        self.assertRunOk(f'groupdel {username} || true')
        self.assertRunOk(f'useradd -s /bin/sh {username}')
        self.assertRunOk(f'test $(su {username} -c "whoami") = {username}')
        self.assertRunOk(f'userdel {username}')

    def test_usermod(self):
        username = self.username
        new_home = '/tmp/'
        self.login()

        self.assertRunOk(f'userdel {username} || true')
        self.assertRunOk(f'groupdel {username} || true')
        self.assertRunOk(f'useradd -s /bin/sh {username}')
        self.assertRunOk(f'usermod {username} --home {new_home}')
        self.assertRunOk(f'test $(su {username} -c "echo $HOME") = {new_home}')
        self.assertRunOk(f'userdel {username}')


Nicolas Carrier | Software Developer | nicolas.carrier@orolia.com



De : Raphael Pavlidis <raphael.pavlidis@gmail.com>
Envoyé : mardi 6 décembre 2022 19:20
À : Yann E. MORIN <yann.morin.1998@free.fr>
Cc : buildroot@buildroot.org <buildroot@buildroot.org>; Nicolas Carrier <Nicolas.Carrier@orolia.com>; Thomas Petazzoni <thomas.petazzoni@bootlin.com>
Objet : Re: [Buildroot] [PATCH v3 1/1] package/shadow: new package 
 
CAUTION: This email originated from outside of the organization.
Do not click links or open attachments unless you recognize the sender and know the content is safe.

Yann, All,

Thanks for your review, again. I will create a new patch and add Nicolas
to the CC.

Regards,
Raphael Pavlidis

On 05.12.22 22:55, Yann E. MORIN wrote:
> Raphael, All,
>
> On 2022-10-13 18:34 +0200, Raphael Pavlidis spake thusly:
>> shadow provides utilities to deal with user accounts.
>>
>> The shadow package includes the necessary programs for converting UNIX
>> password files to the shadow password format, plus programs for managing
>> user and group accounts. Especially it is useful if rootless podman
>> container should be used, which requires newuidmap and newgidmap.
>>
>> Signed-off-by: Raphael Pavlidis <raphael.pavlidis@gmail.com>
>
> I was about to apply this, after fixing the minor issues (see below),
> but there is a rather major blocker, see below too...
>
>> ---
> [--SNIP--]
>> diff --git a/package/shadow/Config.in b/package/shadow/Config.in
>> new file mode 100644
>> index 0000000000..6b1fe0a61f
>> --- /dev/null
>> +++ b/package/shadow/Config.in
>> @@ -0,0 +1,61 @@
> [--SNIP--]
>> +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
>> +    help
>> +      Install the user and group management tools (e.g. groupadd) with setuid and
>
>      $ make check-package
>      package/shadow/Config.in:24: help text: <tab><2 spaces><62 chars> (http://nightly.buildroot.org/#writing-rules-config-in)
>
> [--SNIP--]
>> +config BR2_PACKAGE_SHADOW_SUBORDINATE_IDS
>> +    bool "subordinate-ids"
>> +    help
>> +      Support subordinate ids. Helpful to use container solution like podman
>
>      $ make check-package
>      package/shadow/Config.in:39: help text: <tab><2 spaces><62 chars> (http://nightly.buildroot.org/#writing-rules-config-in)
>
> [--SNIP--]
>> diff --git a/package/shadow/shadow.mk b/package/shadow/shadow.mk
>> new file mode 100644
>> index 0000000000..261f28dd28
>> --- /dev/null
>> +++ b/package/shadow/shadow.mk
>> @@ -0,0 +1,133 @@
>> +################################################################################
>> +#
>> +# shadow
>> +#
>> +################################################################################
>> +
>> +SHADOW_VERSION = 4.11.1
>
> Why 4.11.1? It was released in 2022-01-03, and is affected by
> CVE-2013-4235, with version 4.12.2 being the first to include the fix
> for it, and there is now 4.13:
>
>      https://www.cve.org/CVERecord?id=CVE-2013-4235
>      https://github.com/shadow-maint/shadow/releases/tag/4.12.2
>      https://github.com/shadow-maint/shadow/pull/545
>
>> +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
>
> And:
>
>      SHADOW_CPE_ID_VENDOR = debian
>
> => https://nvd.nist.gov/products/cpe/detail/11DE0412-97D8-4ABC-9807-101628A40DBE?namingFormat=2.3&orderBy=CPEURI&keyword=shadow&status=FINAL
>
>> +SHADOW_CONF_OPTS = \
>> +    --disable-man \
>> +    --without-btrfs \
>> +    --without-nscd \
>> +    --without-skey \
>> +    --without-sssd \
>> +    --without-su \
>> +    --without-tcb
>
>      $ make check-package
>      package/shadow/shadow.mk:15: expected indent with tabs
>      package/shadow/shadow.mk:16: expected indent with tabs
>      package/shadow/shadow.mk:17: expected indent with tabs
>      package/shadow/shadow.mk:18: expected indent with tabs
>      package/shadow/shadow.mk:19: expected indent with tabs
>      package/shadow/shadow.mk:20: expected indent with tabs
>
>> +ifeq ($(BR2_PACKAGE_SHADOW_SHADOWGRP),y)
>> +SHADOW_CONF_OPTS += --enable-shadowgrp
>> +else
>> +SHADOW_CONF_OPTS += --disable-shadowgrp
>> +endif
>> +
>> +ifeq ($(BR2_PACKAGE_SHADOW_ACCOUNT_TOOLS_SETUID),y)
>> +SHADOW_CONF_OPTS += --enable-account-tools-setuid
>> +define SHADOW_ACCOUNT_TOOLS_SETUID_PERMISSIONS
>
> This is named SHADOW_ACCOUNT_TOOLS_SETUID_PERMISSIONS, but [0]...
>
>> +    /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 - - - - -
>
> What about userdel?
>
> [--SNIP--]
>> +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)
>
> ... [0] here the expansion uses the wrong name...
>
> So, I had fixed all the minor issues, but the version bump will require
> a bit more testing that I can do locally. Nicolas (in Cc) who reviewed
> this patch, said he had a runtime test; maybe you can both sync to get
> that test part of the series when you respin?
>
> Regards,
> Yann E. MORIN.
>
>> +    $(SHADOW_SUBORDINATE_IDS_PERMISSIONS)
>> +endef
>> +
>> +$(eval $(autotools-package))
>> --
>> 2.35.1
>>
>> _______________________________________________
>> buildroot mailing list
>> buildroot@buildroot.org
>> https://lists.buildroot.org/mailman/listinfo/buildroot
>
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [Buildroot] [PATCH v3 1/1] package/shadow: new package
  2022-12-08 15:15       ` Nicolas Carrier
@ 2022-12-09 10:24         ` Raphael Pavlidis
  2022-12-09 11:07           ` Nicolas Carrier
  0 siblings, 1 reply; 18+ messages in thread
From: Raphael Pavlidis @ 2022-12-09 10:24 UTC (permalink / raw)
  To: Nicolas Carrier, Yann E. MORIN; +Cc: Thomas Petazzoni, buildroot

Hi Nicolas, All,
thanks for the Test. Should I add it to the new patch then? If yes, what 
about the authors of the patch, should I add you to the authors? (Is it 
possible to have multiple authors in one commit?)

I already apply the comments from Yann. Therefore, I would like to 
create a new patch. :)

Regards,
Raphael Pavlidis
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [Buildroot] [PATCH v3 1/1] package/shadow: new package
  2022-12-09 10:24         ` Raphael Pavlidis
@ 2022-12-09 11:07           ` Nicolas Carrier
  2022-12-10  8:28             ` Yann E. MORIN
  0 siblings, 1 reply; 18+ messages in thread
From: Nicolas Carrier @ 2022-12-09 11:07 UTC (permalink / raw)
  To: Raphael Pavlidis, Yann E. MORIN; +Cc: Thomas Petazzoni, buildroot

Hello Raphael, All,

About the author name, don't bother, I don't mind if you take the ownership (read: the blame :D) of the script.
That said, maybe buildroot maintainers would like it to be expanded? I don't know the policy for this kind of tests, is it sufficient that it demonstrates that the package is able to build something that works or is it expected to have something more exhaustive?

Also, there are those:
self.assertRunOk(f'userdel {username} || true')
self.assertRunOk(f'groupdel {username} || true')

Which I had to had, but I never fully understood why they were needed.


Nicolas Carrier | Software Developer | nicolas.carrier@orolia.com


De : Raphael Pavlidis <raphael.pavlidis@gmail.com>
Envoyé : vendredi 9 décembre 2022 11:24
À : Nicolas Carrier <Nicolas.Carrier@orolia.com>; Yann E. MORIN <yann.morin.1998@free.fr>
Cc : buildroot@buildroot.org <buildroot@buildroot.org>; Thomas Petazzoni <thomas.petazzoni@bootlin.com>
Objet : Re: [Buildroot] [PATCH v3 1/1] package/shadow: new package 
 
CAUTION: This email originated from outside of the organization.
Do not click links or open attachments unless you recognize the sender and know the content is safe.

Hi Nicolas, All,
thanks for the Test. Should I add it to the new patch then? If yes, what
about the authors of the patch, should I add you to the authors? (Is it
possible to have multiple authors in one commit?)

I already apply the comments from Yann. Therefore, I would like to
create a new patch. :)

Regards,
Raphael Pavlidis
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [Buildroot] [PATCH v3 1/1] package/shadow: new package
  2022-12-09 11:07           ` Nicolas Carrier
@ 2022-12-10  8:28             ` Yann E. MORIN
  2022-12-16  9:42               ` Raphael Pavlidis
  0 siblings, 1 reply; 18+ messages in thread
From: Yann E. MORIN @ 2022-12-10  8:28 UTC (permalink / raw)
  To: Nicolas Carrier; +Cc: Thomas Petazzoni, Raphael Pavlidis, buildroot

Nicolas, All,

On 2022-12-09 11:07 +0000, Nicolas Carrier spake thusly:
> About the author name, don't bother, I don't mind if you take the
> ownership (read: the blame :D) of the script.

What one can do in this case, is add a co-author tag, like;

    Co-authored-by: Nicolas Carrier <Nicolas.Carrier@orolia.com>
    [Nicolas.Carrier@orolia.com provided the test case]
    Signed-off-by: Raphael Pavlidis <raphael.pavlidis@gmail.com>

> That said, maybe buildroot maintainers would like it to be expanded?
> I don't know the policy for this kind of tests, is it sufficient
> that it demonstrates that the package is able to build something that
> works or is it expected to have something more exhaustive?

It is perfectly fine to have a test case that demonstrates the basics of
the package. This helps prove that there are no missing runtime
dependencies (e.g. for pythn packages), that the basic features do work
as expected, at least in the use-case the submitter is interested in.

If there are more to test, it can be added as time passes (e.g. when we
get a report that something does not work).

We definitely do not want to be able to run the full test-suite of the
package itself; that's not our role.

> Also, there are those:
> self.assertRunOk(f'userdel {username} || true')
> self.assertRunOk(f'groupdel {username} || true')
> Which I had to had, but I never fully understood why they were needed.

assertRunOk() checks that the return code is zero, and drops the output.
So, if self.assertRunOk(f'userdel {username}') fails, then it means that
userdel in the target did fail, so it would be interesting to check why:

    output, ret = self.emulator.run(f'userdel {username}')
    self.assertEqual(ret, 0, f'Failed with {output!r}')

(I think in fact ythat this is what we should do in assertRunOk())

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [Buildroot] [PATCH v3 1/1] package/shadow: new package
  2022-12-10  8:28             ` Yann E. MORIN
@ 2022-12-16  9:42               ` Raphael Pavlidis
  2022-12-16 14:34                 ` Nicolas Carrier
  0 siblings, 1 reply; 18+ messages in thread
From: Raphael Pavlidis @ 2022-12-16  9:42 UTC (permalink / raw)
  To: Yann E. MORIN, Nicolas Carrier; +Cc: Thomas Petazzoni, buildroot

Nicolas, All,
I added your test and executed it, but I got an error:

======================================================================
FAIL: test_usermod (tests.package.test_shadow.TestShadow)
----------------------------------------------------------------------
Traceback (most recent call last):
   File "/tmp/buildroot/support/testing/tests/package/test_shadow.py", 
line 53, in test_usermod
     self.assertRunOk(f'test $(su {username} -c "echo $HOME") = {new_home}')
   File "/tmp/buildroot/support/testing/infra/basetest.py", line 94, in 
assertRunOk
     self.assertEqual(exit_code, 0)
AssertionError: 1 != 0


I used the following command:
./support/testing/run-tests -d dl -o output_folder -k 
tests.package.test_shadow.TestShadow

Did I something wrong?


Regards,
Raphael Pavlidis
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [Buildroot] [PATCH v3 1/1] package/shadow: new package
  2022-12-16  9:42               ` Raphael Pavlidis
@ 2022-12-16 14:34                 ` Nicolas Carrier
  0 siblings, 0 replies; 18+ messages in thread
From: Nicolas Carrier @ 2022-12-16 14:34 UTC (permalink / raw)
  To: Raphael Pavlidis, Yann E. MORIN; +Cc: Thomas Petazzoni, buildroot

No you didn't do anything wrong, I did :/

FTR if you want to troubleshoot a failing test, you can inspect the output_folder/TestShadow-run.log file, which will give, at its beginning, the qemu command to run, in order to replicate the issue.

Here there were 2, the /tmp/ string should have been /tmp and the quotes for the su command were incorrect, single quotes were needed to avoid the interpreting of the $HOME in the shell.

Here is the fixed version:

import os

from infra.basetest import BRTest, BASIC_TOOLCHAIN_CONFIG


class TestShadow(BRTest):
    username = 'user_test'
    config = BASIC_TOOLCHAIN_CONFIG + \
        """
        BR2_arm=y
        BR2_PACKAGE_SHADOW=y
        BR2_TARGET_ROOTFS_EXT2=y
        BR2_TARGET_ROOTFS_EXT2_4=y
        BR2_TARGET_ROOTFS_EXT2_SIZE="65536"
        """
    timeout = 60

    def login(self):
        img = os.path.join(self.builddir, "images", "rootfs.ext4")
        self.emulator.boot(arch="armv7",
                           kernel="builtin",
                           kernel_cmdline=["root=/dev/mmcblk0",
                                           "rootfstype=ext4"],
                           options=["-drive", f"file={img},if=sd,format=raw"])
        self.emulator.login()

    def test_nologin(self):
        self.login()

        self.assertRunOk("! nologin")
        cmd = 'test "$(nologin)" = "This account is currently not available."'
        self.assertRunOk(cmd)

    def test_useradd_del(self):
        username = self.username
        self.login()

        self.assertRunOk(f'userdel {username} || true')
        self.assertRunOk(f'groupdel {username} || true')
        self.assertRunOk(f'useradd -s /bin/sh {username}')
        self.assertRunOk(f'test $(su {username} -c "whoami") = {username}')
        self.assertRunOk(f'userdel {username}')

    def test_usermod(self):
        username = self.username
        new_home = '/tmp'
        self.login()

        self.assertRunOk(f'userdel {username} || true')
        self.assertRunOk(f'groupdel {username} || true')
        self.assertRunOk(f'useradd -s /bin/sh {username}')
        self.assertRunOk(f'usermod {username} --home {new_home}')
        self.assertRunOk(f'test $(su {username} -c \'echo $HOME\') = {new_home}')
        self.assertRunOk(f'userdel {username}')


Nicolas Carrier | Software Developer | nicolas.carrier@orolia.com


De : Raphael Pavlidis <raphael.pavlidis@gmail.com>
Envoyé : vendredi 16 décembre 2022 10:42
À : Yann E. MORIN <yann.morin.1998@free.fr>; Nicolas Carrier <Nicolas.Carrier@orolia.com>
Cc : Thomas Petazzoni <thomas.petazzoni@bootlin.com>; buildroot@buildroot.org <buildroot@buildroot.org>
Objet : Re: [Buildroot] [PATCH v3 1/1] package/shadow: new package 
 
CAUTION: This email originated from outside of the organization.
Do not click links or open attachments unless you recognize the sender and know the content is safe.

Nicolas, All,
I added your test and executed it, but I got an error:

======================================================================
FAIL: test_usermod (tests.package.test_shadow.TestShadow)
----------------------------------------------------------------------
Traceback (most recent call last):
   File "/tmp/buildroot/support/testing/tests/package/test_shadow.py",
line 53, in test_usermod
     self.assertRunOk(f'test $(su {username} -c "echo $HOME") = {new_home}')
   File "/tmp/buildroot/support/testing/infra/basetest.py", line 94, in
assertRunOk
     self.assertEqual(exit_code, 0)
AssertionError: 1 != 0


I used the following command:
./support/testing/run-tests -d dl -o output_folder -k
tests.package.test_shadow.TestShadow

Did I something wrong?


Regards,
Raphael Pavlidis
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2022-12-16 14:34 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-04 12:43 [Buildroot] [PATCH v2 1/1] package/shadow: new package Raphael Pavlidis
2022-09-05 10:06 ` Arnout Vandecappelle
2022-09-05 11:51 ` Yann E. MORIN
2022-09-05 12:01   ` Yann E. MORIN
2022-09-11 11:22   ` Raphael Pavlidis
2022-09-11 12:14     ` Yann E. MORIN
2022-09-11 12:55       ` Raphael Pavlidis
2022-09-11 17:57         ` Yann E. MORIN
2022-10-13 16:34 ` [Buildroot] [PATCH v3 " Raphael Pavlidis
2022-12-05 15:48   ` Nicolas Carrier
2022-12-05 21:55   ` Yann E. MORIN
2022-12-06 18:20     ` Raphael Pavlidis
2022-12-08 15:15       ` Nicolas Carrier
2022-12-09 10:24         ` Raphael Pavlidis
2022-12-09 11:07           ` Nicolas Carrier
2022-12-10  8:28             ` Yann E. MORIN
2022-12-16  9:42               ` Raphael Pavlidis
2022-12-16 14:34                 ` Nicolas Carrier

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.