[Buildroot] BR2_DL_DIR permissions

[Buildroot] BR2_DL_DIR permissions

[Pesce Luca via buildroot]

Hi all,
    it seems that it is not possible to setup a rw BR2_DL_DIR location shared
among users of the same group on a host machine (e.g. a build server used by many
users, all belonging to a certain "developers" group).

Buildroot's root Makefile imposes 'umask 0022' (REQ_UMASK = 0022), which means
that every file/folder created by BR will have no write perm for the group, including
the per-package download directories under BR2_DL_DIR (which will have drwxr-xr-x perms).
These are writeable just for the owner - the user that issued the first build
that populated the per-package dl dir for the first time (say user A).
Thus, if a BR package changes its version (e.g. for buildroot update), and
another user (say user B, in the same group of A) starts a build, BR fails the
creation of package-xxx.tar.gz inside the dl dir, because user B has no write
permissions on that path.

A possible solution would be to chmod 775 the dl dir inside dl-wrapper script,
after the actual download, like it already does for the tmpf file (output file
of the download backend).
A more radical solution would be to change that global REQ_UMASK value to 0002,
or to change the root Makefile logic in order to set umask only if the system
one is more restrictive than 0022 (it seems to me that the default umask on
moderm distros is 0002, so keeping it intact, without resetting it, would suffice).

I am not really aware of BR's internals on file permissions management, so any
thought or comment on this would be highly appreciated.

Thanks and regards,
Luca
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

[Buildroot] [PATCH] support/download/dl-wrapper: make the whole dl_dir writeable for the group

[Luca Pesce via buildroot]

Root makefile imposes 'umask 0022', which means that every file/folder in
the per-package download directories has no write permission for the group.
These are writeable just for the owner - the user that issued the first build
that populated the per-package dl dir for the first time (say user A).
Thus, if a BR package changes its version (e.g. for buildroot update), and
another user (say user B, in the same group of A) starts a build, BR fails the
creation of package-xxx.tar.gz inside the dl dir, because user B has no write
permissions on that path. Furthermore, in the case of the git backend, this
makes the git cache not updatable by a different user.

So, to allow sharing of a rw BR2_DL_DIR location among users of the same group
on a host machine (e.g. a build server used by many users, all belonging to a
certain "developers" group), set group write permission to the whole package
dl dir.

Signed-off-by: Luca Pesce <luca.pesce@vimar.com>
---
 support/download/dl-wrapper | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/support/download/dl-wrapper b/support/download/dl-wrapper
index 09a6ac1..b7a4319 100755
--- a/support/download/dl-wrapper
+++ b/support/download/dl-wrapper
@@ -190,6 +190,10 @@ main() {
     new_mode=$(printf "%04o" $((0${new_mode} & ~0$(umask))))
     chmod ${new_mode} "${tmp_output}"
 
+    # Make the whole dl_dir writeable for the group, so other users within
+    # the group can download new versions and update any vcs cache in it.
+    chmod -f -R g+w "${dl_dir}"
+
     # We must *not* unlink tmp_output, otherwise there is a small window
     # during which another download process may create the same tmp_output
     # name (very, very unlikely; but not impossible.)
-- 
2.7.4

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

[Buildroot] R: [PATCH] support/download/dl-wrapper: make the whole dl_dir writeable for the group

[Pesce Luca via buildroot]

Sorry, forgot to put Yann in cc, as utils/get-developers suggests.

Luca

________________________________________
Da: Pesce Luca <Luca.Pesce@vimar.com>
Inviato: mercoledì 30 novembre 2022 14:43
A: buildroot@buildroot.org
Cc: Pesce Luca
Oggetto: [PATCH] support/download/dl-wrapper: make the whole dl_dir writeable for the group

Root makefile imposes 'umask 0022', which means that every file/folder in
the per-package download directories has no write permission for the group.
These are writeable just for the owner - the user that issued the first build
that populated the per-package dl dir for the first time (say user A).
Thus, if a BR package changes its version (e.g. for buildroot update), and
another user (say user B, in the same group of A) starts a build, BR fails the
creation of package-xxx.tar.gz inside the dl dir, because user B has no write
permissions on that path. Furthermore, in the case of the git backend, this
makes the git cache not updatable by a different user.

So, to allow sharing of a rw BR2_DL_DIR location among users of the same group
on a host machine (e.g. a build server used by many users, all belonging to a
certain "developers" group), set group write permission to the whole package
dl dir.

Signed-off-by: Luca Pesce <luca.pesce@vimar.com>
---
 support/download/dl-wrapper | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/support/download/dl-wrapper b/support/download/dl-wrapper
index 09a6ac1..b7a4319 100755
--- a/support/download/dl-wrapper
+++ b/support/download/dl-wrapper
@@ -190,6 +190,10 @@ main() {
     new_mode=$(printf "%04o" $((0${new_mode} & ~0$(umask))))
     chmod ${new_mode} "${tmp_output}"

+    # Make the whole dl_dir writeable for the group, so other users within
+    # the group can download new versions and update any vcs cache in it.
+    chmod -f -R g+w "${dl_dir}"
+
     # We must *not* unlink tmp_output, otherwise there is a small window
     # during which another download process may create the same tmp_output
     # name (very, very unlikely; but not impossible.)
--
2.7.4

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

Re: [Buildroot] [PATCH] support/download/dl-wrapper: make the whole dl_dir writeable for the group

[Yann E. MORIN]

Luca, All,

On 2022-11-30 14:43 +0100, Luca Pesce via buildroot spake thusly:
> Root makefile imposes 'umask 0022', which means that every file/folder in
> the per-package download directories has no write permission for the group.
> These are writeable just for the owner - the user that issued the first build
> that populated the per-package dl dir for the first time (say user A).
> Thus, if a BR package changes its version (e.g. for buildroot update), and
> another user (say user B, in the same group of A) starts a build, BR fails the
> creation of package-xxx.tar.gz inside the dl dir, because user B has no write
> permissions on that path. Furthermore, in the case of the git backend, this
> makes the git cache not updatable by a different user.
> 
> So, to allow sharing of a rw BR2_DL_DIR location among users of the same group
> on a host machine (e.g. a build server used by many users, all belonging to a
> certain "developers" group), set group write permission to the whole package
> dl dir.
> 
> Signed-off-by: Luca Pesce <luca.pesce@vimar.com>
> ---
>  support/download/dl-wrapper | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/support/download/dl-wrapper b/support/download/dl-wrapper
> index 09a6ac1..b7a4319 100755
> --- a/support/download/dl-wrapper
> +++ b/support/download/dl-wrapper
> @@ -190,6 +190,10 @@ main() {
>      new_mode=$(printf "%04o" $((0${new_mode} & ~0$(umask))))

This code was written before we enforced our umask, so it is now indeed
incorrect (at least, the comment above is misleading).

>      chmod ${new_mode} "${tmp_output}"
>  
> +    # Make the whole dl_dir writeable for the group, so other users within
> +    # the group can download new versions and update any vcs cache in it.
> +    chmod -f -R g+w "${dl_dir}"

But what if the user initially had umask 0022 to begin with? By forcing
the group authorization with the chmod, you are overriding the user's
umask settings, which is not good... I for one would not want to have
group-writable directories (or files) created when I would have not
expected it.

Instead, what about something like:

    diff --git a/Makefile b/Makefile
    index 827ab230ef..949f27f1eb 100644
    --- a/Makefile
    +++ b/Makefile
    @@ -83,10 +83,15 @@ $(MAKECMDGOALS): _all
     _all:
     	@umask $(REQ_UMASK) && \
     		$(MAKE) -C $(CANONICAL_CURDIR) --no-print-directory \
    +			BR_ORIG_UMASK=$(shell umask) \
     			$(MAKECMDGOALS) $(EXTRAMAKEARGS)
     
     else # umask / $(CURDIR) / $(O)
     
    +# Save the user's original umask, as we may need it later on, like
    +# during the download dl-wrapper
    +export BR_ORIG_UMASK := $(or $(BR_ORIG_UMASK),$(shell umask))
    +
     # This is our default rule, so must come first
     all:
     .PHONY: all
    diff --git a/package/pkg-download.mk b/package/pkg-download.mk
    index 0718f21aad..e9dfec635d 100644
    --- a/package/pkg-download.mk
    +++ b/package/pkg-download.mk
    @@ -107,9 +107,11 @@ endif
     #
     ################################################################################
     
    +# Use user's original umask, in case they have provisions set to share
    +# the download directory with their group (or the whole world).
     define DOWNLOAD
    -$(Q)mkdir -p $($(2)_DL_DIR)
    -		$(Q)$(EXTRA_ENV) $($(2)_DL_ENV) \
    +	$(Q)umask $(BR_ORIG_UMASK); mkdir -p $($(2)_DL_DIR)
    +	$(Q)umask $(BR_ORIG_UMASK); $(EXTRA_ENV) $($(2)_DL_ENV) \
     		flock $($(2)_DL_DIR)/.lock $(DL_WRAPPER) \
     		-c '$($(2)_DL_VERSION)' \
     		-d '$($(2)_DL_DIR)' \

The comments are only mostly stubs and need being expanded a bit...

Regards,
Yann E. MORIN.

>      # We must *not* unlink tmp_output, otherwise there is a small window
>      # during which another download process may create the same tmp_output
>      # name (very, very unlikely; but not impossible.)
> -- 
> 2.7.4
> 
> _______________________________________________
> 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

[Buildroot] R: [PATCH] support/download/dl-wrapper: make the whole dl_dir writeable for the group

[Pesce Luca via buildroot]

Hi Yann,

> Da: Yann E. MORIN <yann.morin.1998@free.fr>
> Inviato: giovedì 1 dicembre 2022 21:25
> A: Pesce Luca
> Cc: buildroot@buildroot.org
> Oggetto: Re: [Buildroot] [PATCH] support/download/dl-wrapper: make the whole dl_dir writeable for the group
> 
> Luca, All,
> 
> On 2022-11-30 14:43 +0100, Luca Pesce via buildroot spake thusly:
> > Root makefile imposes 'umask 0022', which means that every file/folder in
> > the per-package download directories has no write permission for the group.
> > These are writeable just for the owner - the user that issued the first build
> > that populated the per-package dl dir for the first time (say user A).
> > Thus, if a BR package changes its version (e.g. for buildroot update), and
> > another user (say user B, in the same group of A) starts a build, BR fails the
> > creation of package-xxx.tar.gz inside the dl dir, because user B has no write
> > permissions on that path. Furthermore, in the case of the git backend, this
> > makes the git cache not updatable by a different user.
> >
> > So, to allow sharing of a rw BR2_DL_DIR location among users of the same group
> > on a host machine (e.g. a build server used by many users, all belonging to a
> > certain "developers" group), set group write permission to the whole package
> > dl dir.
> >
> > Signed-off-by: Luca Pesce <luca.pesce@vimar.com>
> > ---
> >  support/download/dl-wrapper | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/support/download/dl-wrapper b/support/download/dl-wrapper
> > index 09a6ac1..b7a4319 100755
> > --- a/support/download/dl-wrapper
> > +++ b/support/download/dl-wrapper
> > @@ -190,6 +190,10 @@ main() {
> >      new_mode=$(printf "%04o" $((0${new_mode} & ~0$(umask))))
> 
> This code was written before we enforced our umask, so it is now indeed
> incorrect (at least, the comment above is misleading).
> 
> >      chmod ${new_mode} "${tmp_output}"
> >
> > +    # Make the whole dl_dir writeable for the group, so other users within
> > +    # the group can download new versions and update any vcs cache in it.
> > +    chmod -f -R g+w "${dl_dir}"
> 
> But what if the user initially had umask 0022 to begin with? By forcing
> the group authorization with the chmod, you are overriding the user's
> umask settings, which is not good... I for one would not want to have
> group-writable directories (or files) created when I would have not
> expected it.

Yes, I can agree with this. However, BR already overrides the user's umask,
and, at least in my case (umask=0002), this changes the user's default,
denying g+w by design.

> 
> Instead, what about something like:
> 
>     diff --git a/Makefile b/Makefile
>     index 827ab230ef..949f27f1eb 100644
>     --- a/Makefile
>     +++ b/Makefile
>     @@ -83,10 +83,15 @@ $(MAKECMDGOALS): _all
>      _all:
>         @umask $(REQ_UMASK) && \
>                 $(MAKE) -C $(CANONICAL_CURDIR) --no-print-directory \
>     +                   BR_ORIG_UMASK=$(shell umask) \
>                         $(MAKECMDGOALS) $(EXTRAMAKEARGS)
> 
>      else # umask / $(CURDIR) / $(O)
> 
>     +# Save the user's original umask, as we may need it later on, like
>     +# during the download dl-wrapper
>     +export BR_ORIG_UMASK := $(or $(BR_ORIG_UMASK),$(shell umask))
>     +
>      # This is our default rule, so must come first
>      all:
>      .PHONY: all
>     diff --git a/package/pkg-download.mk b/package/pkg-download.mk
>     index 0718f21aad..e9dfec635d 100644
>     --- a/package/pkg-download.mk
>     +++ b/package/pkg-download.mk
>     @@ -107,9 +107,11 @@ endif
>      #
>      ################################################################################
> 
>     +# Use user's original umask, in case they have provisions set to share
>     +# the download directory with their group (or the whole world).
>      define DOWNLOAD
>     -$(Q)mkdir -p $($(2)_DL_DIR)
>     -           $(Q)$(EXTRA_ENV) $($(2)_DL_ENV) \
>     +   $(Q)umask $(BR_ORIG_UMASK); mkdir -p $($(2)_DL_DIR)
>     +   $(Q)umask $(BR_ORIG_UMASK); $(EXTRA_ENV) $($(2)_DL_ENV) \
>                 flock $($(2)_DL_DIR)/.lock $(DL_WRAPPER) \
>                 -c '$($(2)_DL_VERSION)' \
>                 -d '$($(2)_DL_DIR)' \
> 
> The comments are only mostly stubs and need being expanded a bit...

Yes, this looks good to me: you honor user's umask in the whole download
phase, and keep the 0022 for other files created by BR. So a better approach,
for sure!

> 
> Regards,
> Yann E. MORIN.
> 

Thanks and regards,
Luca
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] R: [PATCH] support/download/dl-wrapper: make the whole dl_dir writeable for the group

[Yann E. MORIN]

Luca, All,

On 2022-12-02 13:47 +0000, Pesce Luca spake thusly:
> > Da: Yann E. MORIN <yann.morin.1998@free.fr>
> > Inviato: giovedì 1 dicembre 2022 21:25
> > A: Pesce Luca
> > Cc: buildroot@buildroot.org
> > Oggetto: Re: [Buildroot] [PATCH] support/download/dl-wrapper: make the whole dl_dir writeable for the group
[--SNIP--]
> > > +    # Make the whole dl_dir writeable for the group, so other users within
> > > +    # the group can download new versions and update any vcs cache in it.
> > > +    chmod -f -R g+w "${dl_dir}"
> > But what if the user initially had umask 0022 to begin with? By forcing
> > the group authorization with the chmod, you are overriding the user's
> > umask settings, which is not good... I for one would not want to have
> > group-writable directories (or files) created when I would have not
> > expected it.
> Yes, I can agree with this. However, BR already overrides the user's umask,
> and, at least in my case (umask=0002), this changes the user's default,
> denying g+w by design.

Yes, but the reason we do enforce a known umask, is to guarantee some
kind of reproducibility in the generated target image. See commit
bee5745ccc20 (Makefile: don't depend on the umask) which explains,
albeit only briefly, why we do it:

    Some packages and BR itself create files and directories on the target
    with cp/mkdir/etc which depend on the umask at the time of building.

Doing the fixups in target with an explicit chmod or whatever is not
possible. Indeed, some packages also do their own chmod, and we have no
way to know.

So, we can only resort to using a known umask. We chose 0022 because it
is sane by default, and usually, packages that install files like
executalbes, will want those to be world-executable (a system would not
be very useful is only root could run programs).

> > Instead, what about something like:
[--SNIP--]
> Yes, this looks good to me: you honor user's umask in the whole download
> phase, and keep the 0022 for other files created by BR. So a better approach,
> for sure!

Could you handle refining this change, test it, and when/if that works,
send it as a proper patch, please?

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

[Buildroot] R: R: [PATCH] support/download/dl-wrapper: make the whole dl_dir writeable for the group

[Pesce Luca via buildroot]

Hello Yann,

> ________________________________________
> Da: Yann E. MORIN <yann.morin.1998@free.fr>
> Inviato: venerdì 2 dicembre 2022 20:29
> A: Pesce Luca
> Cc: buildroot@buildroot.org
> Oggetto: Re: R: [Buildroot] [PATCH] support/download/dl-wrapper: make the whole dl_dir writeable for the group
> 
> Luca, All,
> 
> On 2022-12-02 13:47 +0000, Pesce Luca spake thusly:
> > > Da: Yann E. MORIN <yann.morin.1998@free.fr>
> > > Inviato: giovedì 1 dicembre 2022 21:25
> > > A: Pesce Luca
> > > Cc: buildroot@buildroot.org
> > > Oggetto: Re: [Buildroot] [PATCH] support/download/dl-wrapper: make the whole dl_dir writeable for the group
> [--SNIP--]
> > > > +    # Make the whole dl_dir writeable for the group, so other users within
> > > > +    # the group can download new versions and update any vcs cache in it.
> > > > +    chmod -f -R g+w "${dl_dir}"
> > > But what if the user initially had umask 0022 to begin with? By forcing
> > > the group authorization with the chmod, you are overriding the user's
> > > umask settings, which is not good... I for one would not want to have
> > > group-writable directories (or files) created when I would have not
> > > expected it.
> > Yes, I can agree with this. However, BR already overrides the user's umask,
> > and, at least in my case (umask=0002), this changes the user's default,
> > denying g+w by design.
> 
> Yes, but the reason we do enforce a known umask, is to guarantee some
> kind of reproducibility in the generated target image. See commit
> bee5745ccc20 (Makefile: don't depend on the umask) which explains,
> albeit only briefly, why we do it:
> 
>     Some packages and BR itself create files and directories on the target
>     with cp/mkdir/etc which depend on the umask at the time of building.
> 
> Doing the fixups in target with an explicit chmod or whatever is not
> possible. Indeed, some packages also do their own chmod, and we have no
> way to know.

Ok, now I see the reason for the imposed umask, thanks for the explanation.

> 
> So, we can only resort to using a known umask. We chose 0022 because it
> is sane by default, and usually, packages that install files like
> executalbes, will want those to be world-executable (a system would not
> be very useful is only root could run programs).
> 
> > > Instead, what about something like:
> [--SNIP--]
> > Yes, this looks good to me: you honor user's umask in the whole download
> > phase, and keep the 0022 for other files created by BR. So a better approach,
> > for sure!
> 
> Could you handle refining this change, test it, and when/if that works,
> send it as a proper patch, please?
> 

Yes, will send something soon. Do I have to ack you in some way in the upcoming
patch?


> Regards,
> Yann E. MORIN.
> 

Thanks and regards,
Luca
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] R: R: [PATCH] support/download/dl-wrapper: make the whole dl_dir writeable for the group

[Yann E. MORIN]

Luca, All,

On 2022-12-05 12:36 +0000, Pesce Luca via buildroot spake thusly:
[--SNIP--]
> > Could you handle refining this change, test it, and when/if that works,
> > send it as a proper patch, please?
> Yes, will send something soon. Do I have to ack you in some way in the upcoming
> patch?

You can add me in Cc when sending the patch; usually, 'git send-email'
will do it for you automatically if you add just (without quotes) at the
end of your commit message: 
    Cc: someone <some-email>

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