[Buildroot] [PATCH 1/1] package/frr: make vtysh group configurable

[Buildroot] [PATCH 1/1] package/frr: make vtysh group configurable

[Joachim Wiberg]

Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
---
 package/frr/Config.in | 8 ++++++++
 package/frr/frr.mk    | 7 ++++---
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/package/frr/Config.in b/package/frr/Config.in
index c26b160b2a..9b9ad6f331 100644
--- a/package/frr/Config.in
+++ b/package/frr/Config.in
@@ -24,6 +24,14 @@ config BR2_PACKAGE_FRR
 
 if BR2_PACKAGE_FRR
 
+config BR2_PACKAGE_FRR_VTYSH_GROUP
+	string "FRR vtysh group, default: frrvty"
+	default "frrvty"
+	help
+	  This setting controls the group a user must be member of to connect
+	  to vtysh.  E.g., on systems with an admin user, which may be member
+	  of the 'wheel' group, it may make more sense to use 'wheel' here.
+
 config BR2_PACKAGE_FRR_BMP
 	bool "BMP protocol"
 	select BR2_PACKAGE_C_ARES
diff --git a/package/frr/frr.mk b/package/frr/frr.mk
index 921ef99665..9ec4d3af4f 100644
--- a/package/frr/frr.mk
+++ b/package/frr/frr.mk
@@ -37,6 +37,7 @@ FRR_CONF_ENV = \
 
 # Do not enable -fplugin=frr-format for production, see doc/developer/workflow.rst,
 # it is only intended for FRR's developments
+FRR_VTYSH_GROUP=$(call qstrip,$(BR2_PACKAGE_FRR_VTYSH_GROUP))
 FRR_CONF_OPTS = --with-clippy=$(HOST_DIR)/bin/clippy \
 	--sysconfdir=/etc/frr \
 	--localstatedir=/var/run/frr \
@@ -48,7 +49,7 @@ FRR_CONF_OPTS = --with-clippy=$(HOST_DIR)/bin/clippy \
 	--enable-shell-access \
 	--enable-user=frr \
 	--enable-group=frr \
-	--enable-vty-group=frrvty \
+	--enable-vty-group=$(FRR_VTYSH_GROUP) \
 	--enable-fpm
 
 HOST_FRR_CONF_OPTS = --enable-clippy-only
@@ -107,12 +108,12 @@ define FRR_PERMISSIONS
 	/etc/frr/daemons f 640 frr frr - - - - -
 	/etc/frr/daemons.conf f 640 frr frr - - - - -
 	/etc/frr/frr.conf f 640 frr frr - - - - -
-	/etc/frr/vtysh.conf f 640 frr frrvty - - - - -
+	/etc/frr/vtysh.conf f 640 frr $(FRR_VTYSH_GROUP) - - - - -
 	/etc/frr/support_bundle_commands.conf f 640 frr frr
 endef
 
 define FRR_USERS
-	frr -1 frr -1 * /var/run/frr - frrvty FRR user priv
+	frr -1 frr -1 * /var/run/frr - $(FRR_VTYSH_GROUP) FRR user priv
 endef
 
 define FRR_INSTALL_INIT_SYSV
-- 
2.43.0

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

Re: [Buildroot] [PATCH 1/1] package/frr: make vtysh group configurable

[Thomas Petazzoni via buildroot]

Hello Joachim,

On Mon,  6 Jan 2025 21:34:59 +0100
Joachim Wiberg <troglobit@gmail.com> wrote:

> Signed-off-by: Joachim Wiberg <troglobit@gmail.com>

I am not a big fan of this, because we frequently use a "sane" default
group in packages, and clearly don't want to make such a "sane" default
configurable in each package. We would normally expect people who need
to customize things to do it in their custom post-build script, but I
admit in this specific case it seems not great, so I don't really have
a good idea.

But my main concern is really: do we want to allow config options to
set the group like this for each and every package?

>  # Do not enable -fplugin=frr-format for production, see doc/developer/workflow.rst,
>  # it is only intended for FRR's developments
> +FRR_VTYSH_GROUP=$(call qstrip,$(BR2_PACKAGE_FRR_VTYSH_GROUP))

Nitpick: spaces needed around = sign.

Thomas
-- 
Thomas Petazzoni, co-owner and CEO, Bootlin
Embedded Linux and Kernel engineering and training
https://bootlin.com
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH 1/1] package/frr: make vtysh group configurable

[Joachim Wiberg]

Hi Thomas, all

On Mon, 2025-01-06 at 22:11 +0100, Thomas Petazzoni wrote:
> On Mon,  6 Jan 2025 21:34:59 +0100
> Joachim Wiberg <troglobit@gmail.com> wrote:
> I am not a big fan of this, because we frequently use a "sane" default
> group in packages, and clearly don't want to make such a "sane"
> default
> configurable in each package. We would normally expect people who need
> to customize things to do it in their custom post-build script, but I
> admit in this specific case it seems not great, so I don't really have
> a good idea.

I understand the concern, sane defaults is one of many reasons that make
Buildroot so compelling.  However ...

> But my main concern is really: do we want to allow config options to
> set the group like this for each and every package?

... some packages, in this case the complex beast that is Frr, hard code
the group in the resulting application, without being possible to change
or override in post-build scripts or configuration files.  In such cases
I believe we should allow for keeping deviations to the defaults in the
users defconfig rather than forcing them to maintain local patches.

> >  # Do not enable -fplugin=frr-format for production, see
> > doc/developer/workflow.rst,
> >  # it is only intended for FRR's developments
> > +FRR_VTYSH_GROUP=$(call qstrip,$(BR2_PACKAGE_FRR_VTYSH_GROUP))
> 
> Nitpick: spaces needed around = sign.

Ah, good catch.  Will fix that in v2, pending approval of the general
idea, of course.

Best regards
 /Joachim

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

Re: [Buildroot] [PATCH 1/1] package/frr: make vtysh group configurable

[Vincent Jardin]

Hi Joachim,

Why would you enforce a frrvty user to any other value ? Can you explain
the rational of not using the Buildroot's default value ?

Even if FRR has such capability, it does not mean we should expose it.

Your suggestion starts adding some complexities. Based on the same model, if we
follow it, we should do the same for:
 	--enable-user=frr
 	--enable-group=frr
too. That's why I would be strongly interested in reading your rationals
for such capability.

Best regards,
  Vincent
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH 1/1] package/frr: make vtysh group configurable

[Joachim Wiberg]

[-- Attachment #1.1: Type: text/plain, Size: 1901 bytes --]

Hi Vincent!

On Wed, 2025-02-05 at 02:28 +0100, Vincent Jardin wrote:

> Why would you enforce a frrvty user to any other value ? Can you
> explain the rational of not using the Buildroot's default value ?

Currently, non-root users of a Buildroot system, that you want to give
administrator privileges to manage the system, i.e., edit system files
and configure Frr daemons using vtysh, need to be member of two groups:
wheel and frrvty.

I propose opening up that so it is up to the Buildroot user (developer)
how they model their end system.

> Even if FRR has such capability, it does not mean we should expose it.

This Frr build-time setting controls the group which an administrator
user in a system must be member of to be able to access vtysh over its
UNIX socket, it cannot be set set or changed in another way after build.

A variant of the same patch that would work for my own use-case, and in
some way align with Buildroot defaults, would be to change what we hard-
code it to:

   --enable-vty-group=wheel

I did not think such a patch would fly at all, in particular since it is
not backwards compatible.  Which is why I went with this approach where
the developer can at least decide the group of users should be able to
manage Frr now that it has shifted to everything-via-vtysh from per-
daemon .conf files as their default.

> Your suggestion starts adding some complexities. Based on the same
> model, if we
> follow it, we should do the same for:
>  	--enable-user=frr
>  	--enable-group=frr
> too. That's why I would be strongly interested in reading your
> rationals for such capability.

I see, well that was not my intention at all to imply.  The latter is
for privilege separation of the daemons themselves, I don't see any
value to a user of Buildroot to be able to change that for any package.


All the best
 /Joachim
 

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 150 bytes --]

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

Re: [Buildroot] [PATCH 1/1] package/frr: make vtysh group configurable

[Arnout Vandecappelle via buildroot]

On 05/02/2025 05:14, Joachim Wiberg wrote:
> Hi Vincent!
> 
> On Wed, 2025-02-05 at 02:28 +0100, Vincent Jardin wrote:
> 
>> Why would you enforce a frrvty user to any other value ? Can you
>> explain the rational of not using the Buildroot's default value ?
> 
> Currently, non-root users of a Buildroot system, that you want to give
> administrator privileges to manage the system, i.e., edit system files
> and configure Frr daemons using vtysh, need to be member of two groups:
> wheel and frrvty.

  That is correct, but I still don't see what the problem is with that. It's 
pretty standard UNIX sysadmin practice to use a unique group for each service 
and to add the users who need to access that service to the corresponding group.


> I propose opening up that so it is up to the Buildroot user (developer)
> how they model their end system.

  We don't want to give complete freedom though.


>> Even if FRR has such capability, it does not mean we should expose it.
> 
> This Frr build-time setting controls the group which an administrator
> user in a system must be member of to be able to access vtysh over its
> UNIX socket, it cannot be set set or changed in another way after build.
> 
> A variant of the same patch that would work for my own use-case, and in
> some way align with Buildroot defaults, would be to change what we hard-
> code it to:
> 
>     --enable-vty-group=wheel

  But that would give access to frr to every user in the wheel group, with no 
way at all to reduce that access. Conversely, a separate frrvty group allows you 
to add whatever user is needed to that group.

  Regards,
  Arnout

> 
> I did not think such a patch would fly at all, in particular since it is
> not backwards compatible.  Which is why I went with this approach where
> the developer can at least decide the group of users should be able to
> manage Frr now that it has shifted to everything-via-vtysh from per-
> daemon .conf files as their default.
> 
>> Your suggestion starts adding some complexities. Based on the same
>> model, if we
>> follow it, we should do the same for:
>>   	--enable-user=frr
>>   	--enable-group=frr
>> too. That's why I would be strongly interested in reading your
>> rationals for such capability.
> 
> I see, well that was not my intention at all to imply.  The latter is
> for privilege separation of the daemons themselves, I don't see any
> value to a user of Buildroot to be able to change that for any package.
> 
> 
> All the best
>   /Joachim
>   
> 
> 
> _______________________________________________
> 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

Re: [Buildroot] [PATCH 1/1] package/frr: make vtysh group configurable

[Arnout Vandecappelle via buildroot]

On 06/01/2025 21:34, Joachim Wiberg wrote:
> Signed-off-by: Joachim Wiberg <troglobit@gmail.com>

  After some back-and-forth we didn't reach find the use case compelling enough, 
so I've marked this patch as Rejected.

  BTW in the future, please make sure to include in the commit message an 
explanation _why_ this patch is needed (unless it's obvious, like for version 
bumps, new packages, or fixes).

  Regards,
  Arnout

> ---
>   package/frr/Config.in | 8 ++++++++
>   package/frr/frr.mk    | 7 ++++---
>   2 files changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/package/frr/Config.in b/package/frr/Config.in
> index c26b160b2a..9b9ad6f331 100644
> --- a/package/frr/Config.in
> +++ b/package/frr/Config.in
> @@ -24,6 +24,14 @@ config BR2_PACKAGE_FRR
>   
>   if BR2_PACKAGE_FRR
>   
> +config BR2_PACKAGE_FRR_VTYSH_GROUP
> +	string "FRR vtysh group, default: frrvty"
> +	default "frrvty"
> +	help
> +	  This setting controls the group a user must be member of to connect
> +	  to vtysh.  E.g., on systems with an admin user, which may be member
> +	  of the 'wheel' group, it may make more sense to use 'wheel' here.
> +
>   config BR2_PACKAGE_FRR_BMP
>   	bool "BMP protocol"
>   	select BR2_PACKAGE_C_ARES
> diff --git a/package/frr/frr.mk b/package/frr/frr.mk
> index 921ef99665..9ec4d3af4f 100644
> --- a/package/frr/frr.mk
> +++ b/package/frr/frr.mk
> @@ -37,6 +37,7 @@ FRR_CONF_ENV = \
>   
>   # Do not enable -fplugin=frr-format for production, see doc/developer/workflow.rst,
>   # it is only intended for FRR's developments
> +FRR_VTYSH_GROUP=$(call qstrip,$(BR2_PACKAGE_FRR_VTYSH_GROUP))
>   FRR_CONF_OPTS = --with-clippy=$(HOST_DIR)/bin/clippy \
>   	--sysconfdir=/etc/frr \
>   	--localstatedir=/var/run/frr \
> @@ -48,7 +49,7 @@ FRR_CONF_OPTS = --with-clippy=$(HOST_DIR)/bin/clippy \
>   	--enable-shell-access \
>   	--enable-user=frr \
>   	--enable-group=frr \
> -	--enable-vty-group=frrvty \
> +	--enable-vty-group=$(FRR_VTYSH_GROUP) \
>   	--enable-fpm
>   
>   HOST_FRR_CONF_OPTS = --enable-clippy-only
> @@ -107,12 +108,12 @@ define FRR_PERMISSIONS
>   	/etc/frr/daemons f 640 frr frr - - - - -
>   	/etc/frr/daemons.conf f 640 frr frr - - - - -
>   	/etc/frr/frr.conf f 640 frr frr - - - - -
> -	/etc/frr/vtysh.conf f 640 frr frrvty - - - - -
> +	/etc/frr/vtysh.conf f 640 frr $(FRR_VTYSH_GROUP) - - - - -
>   	/etc/frr/support_bundle_commands.conf f 640 frr frr
>   endef
>   
>   define FRR_USERS
> -	frr -1 frr -1 * /var/run/frr - frrvty FRR user priv
> +	frr -1 frr -1 * /var/run/frr - $(FRR_VTYSH_GROUP) FRR user priv
>   endef
>   
>   define FRR_INSTALL_INIT_SYSV

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

Re: [Buildroot] [PATCH 1/1] package/frr: make vtysh group configurable

[Joachim Wiberg]

[-- Attachment #1.1: Type: text/plain, Size: 696 bytes --]

On Wed, 2025-02-05 at 09:51 +0100, Arnout Vandecappelle wrote:
> On 06/01/2025 21:34, Joachim Wiberg wrote:
> > Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
>   After some back-and-forth we didn't reach find the use case
> compelling enough, so I've marked this patch as Rejected.

OK, that's fine.  It was worth a try, and now I've learned a bit more
about policy and how you reason about things like this in Buildroot.  So
thank you! 🙂

> BTW in the future, please make sure to include in the commit message 
> an explanation _why_ this patch is needed (unless it's obvious, like
> for version bumps, new packages, or fixes).

Understood.

Best regards
 /Joachim


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 150 bytes --]

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