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

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

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