Re: [PATCH 0/5] allow creation of users/groups before rootfs creation

[Florian Bezdeka <florian.bezdeka@siemens.com>]

On Thu, 2023-01-26 at 08:21 +0000, Schaffner, Tobias wrote:
> On 25.01.23 22:38, Schild, Henning (T CED SES-DE) wrote:
> > Am Wed, 25 Jan 2023 21:55:00 +0100
> > schrieb "Schaffner, Tobias (T CED SES-DE)"
> > <tobias.schaffner@siemens.com>:
> > 
> > > On 25.01.23 17:29, Schild, Henning (T CED SES-DE) wrote:
> > > > Am Wed, 25 Jan 2023 14:44:40 +0100
> > > > schrieb Gylstorff Quirin <quirin.gylstorff@siemens.com>:
> > > >    
> > > > > On 1/25/23 14:29, Henning Schild wrote:
> > > > > > Am Wed, 25 Jan 2023 10:01:51 +0100
> > > > > > schrieb "T. Schaffner" <tobias.schaffner@siemens.com>:
> > > > > >       
> > > > > > > From: Tobias Schaffner <tobias.schaffner@siemens.com>
> > > > > > > 
> > > > > > > This patch series will allow to specify a `pre` flag for the
> > > > > > > USER_ and GROUP_ bitbake variables. If this flag is set to
> > > > > > > `true` the given user or group will be created in the rootfs
> > > > > > > configuration step instead of on rootfs postprocessing. This is
> > > > > > > helpful when a specific id should be used which would otherwise
> > > > > > > be picked by a user or group created by one of the installed
> > > > > > > packages.
> > > > > > 
> > > > > > While i do understand the reason i am not sure how relevant that
> > > > > > is. Why would anything only function with a fixed ID? Whoever
> > > > > > provided that thing should maybe fix it.
> > > > > 
> > > > > Specific IDs are necessary for Updating an A/B rootfs system with a
> > > > > persistent partition. In this case you do not want to change any
> > > > > IDs as this can lead to wrong file permissions.
> > > > 
> > > > I see, that is much more understandable. It also goes into the
> > > > reproducible build direction.
> > > > But if that is the case the solution does not seem to be good enough
> > > > because it only considers users/groups created by isar. Not the ones
> > > > created by installing debian packages. Where the order could be
> > > > potentially random. Say you DEBIAN_DEPENDS or IMAGE_PREINSTALL "ftpd
> > > > wwwd" which will craete users "ftp www" where the two deamons do not
> > > > have any dep on each other and apt-get could install them in any
> > > > order. That order might in reality not change too often but it
> > > > could i.e. when you move from debian11 to debian12 or when you
> > > > bring the third (or 10th) user-adding package into your new
> > > > firmware.
> > > 
> > > Thanks for the review!
> > > 
> > > This series is not about reproducible builds. The IDs of the users and
> > > groups that are created by debian packages are expected to change
> > > between _image_ versions. So we expect these IDs to be different after
> > > a swupdate.
> > 
> > But your initial argument was that you have to create a user before any
> > debian package comes, or did i misunderstand? Maybe we need to
> > differentiate between upstream packages and isar-created ones?
> 
> Lets make this clearer with an example:
> A downstream layer creates one user X that writes to a separate partition.
> This user gets the ID 1000 as there are no packages that create any users.
> 
> Now they want to create a image update that should be deployed.
> The updated image includes a new debian package that creates a user Y.
> The user Y of this package gets the ID 1000 as it is now the first user that
> is created.
> 
> User X will now get 1001 and it is not possible to change this. The data
> created on the separate partition belongs to user Y.
> 
> The only resort at the moment is to revert the patch that moved the user
> creation to the post processing.

Hm... I have the strong feeling that your approach is broken in the
same way...

I was questioning myself how Debian itself solves this problem. The
answer is: Debian has access to the "old" or "current" user database
while doing updates. So it can not happen that a user gets a different
UID. We would have to feed in the "current" user database during such
updates to do it the same way.

Why your approach is broken: Consider three image versions A, B and C.
There is no guarantee that someone will update from A to B and then to
C. A -> C is also possible.

Image A can add a user using your "pre" logic, B can remove that user,
C can introduce a new different user, but with same ID. That brings you
into the same situation.

As Isar can't tell if that is a dangerous scenario for all the possible
use cases it's a NACK security wise.


> 
> > > For reproducible builds it is important that the ordering of installed
> > > packages and their dependencies stay the same but that is a different
> > > story. I expect the ordering algorithm for a specific set of packages
> > > including dependencies to be deterministic but would have to look into
> > > that in detail.
> > 
> > I expect the algorithm to be non-deterministic ... potentially as you
> > progress using debian. I can just claim, you have to prove.
> 
>  From my point of view this problem derives from the fact that the set of
> installed packages will change on image updates. If the user Y is created
> in the first or in the last package does not matter. Its just the fact
> that an additional user gets created.
> 
> > > Still, if somebody had a requirement that a user or group of a package
> > > (e.g. ftp or www) stays the same, one could use this change to create
> > > the user beforehand and pin its id using this change.
> > 
> > True. But we might still want to keep the old database to run
> > assertions so such things do not go unnoticed.
> > 
> > > > So what you maybe really want is giving isar an /etc/passwd
> > > > /etc/group pair. Every new firmware is build with the given layer
> > > > code and that file-pair from the first release. Where you inject
> > > > those files between bootstrap and install ... hoping that bootstrap
> > > > will always be the same. Maybe one can inject before bootstrap ...
> > > > or write a postproc function that will find all different ids and
> > > > all files and fix up. Or at least start with an assertion in
> > > > postproc that looks at the old database.
> > > 
> > > Care that just adding /etc/passwd and /etc/group might not be enough
> > > but you would have to handle the side effects of a useradd/groupadd
> > > call properly (E.g. creating the home if set). And I expect more
> > > things to come up if you have a detailed look.
> > 
> > Yes. Not even want to think about LDAP or yp where the databases live
> > in different files.
> > 
> > > Take into account that the specific patch that introduces the pre flag
> > > is small, easy to maintain and configure.
> > 
> > It should still be motivated so that we later understand why we have
> > it. And i would not call it easy to configure if one does not
> > understand why ... people will "pre" randomly because they do not know
> > what they are doing. We moved that stuff from pre to post once, and i
> > can not help the feeling that people might want to want to mix pre and
> > post one day ... at which point it might become really un-easy.
> > 
> > > > Is the problem of uid/gid depend on install order known in the
> > > > debian community and how do others solve it? Gentoo has moved from
> > > > such dynamic allocation to static some years ago, probably for
> > > > similar reasons.
> > > 
> > > I am open to discuss this with the reproducible build guys but again
> > > IMHO this discussion belongs into another thread.
> > 
> > IMHO not, the repro guys want full repro ... which we can happily leave
> > out when it comes to me. You want partial repro, which we should
> > discuss here.
> 
> I think you may have a valid point that a non deterministic ordering on
> package install may be a problem for reproducable builds. Not only when
> it comes to users but also e.g. every time two packages append to the same
> file.
> 
> > I do not want to hold this back, just understand it and see if there
> > can be a better solution. Feel free to ignore me and merge if nobody
> > else asks questions.
> 
> I am happy to discuss this, I just think we are mixing things up. But
> maybe I am missing something.
> 
> Best!
> 
> > Henning
> > 
> > > > Henning
> > > >    
> > > > > > 
> > > > > > So i am willing to say that this is super-niche! And it deserves a
> > > > > > niche-solution in its layer, not a feature in Isar.
> > > > > > 
> > > > > > You could hook in a task between bootstrap and image_install. Or
> > > > > > you could rebuild a bootstrap package to have reserved ids. You
> > > > > > could run "the thing" in namespaces ...
> > > > > > 
> > > > > > So is that really relevant? Please go into detail.
> > > > > > 
> > > > > > Whatever happens i think the python rewrite is cool. But the code
> > > > > > may have been coming/inspired from OE ... in which case it would
> > > > > > not be cool, because it would fork away further.
> > > > > 
> > > > > 
> > > > > OE uses a complete different implementation than to original:
> > > > > https://git.openembedded.org/openembedded-core/tree/meta/classes/useradd.bbclass
> > > > > 
> > > > > see also:
> > > > > 
> > > > > https://git.openembedded.org/openembedded-core/tree/meta-skeleton/recipes-skeleton/useradd/useradd-example.bb
> > > > > 
> > > > > 
> > > > > 
> > > > > Quirin
> > > > > > 
> > > > > > Henning
> > > > > >       
> > > > > > > A rewrite of the image-account-extension in python was done on
> > > > > > > the way. This allows us to drop a lot of encoding and parsing
> > > > > > > code that was used to transition to shell and therefore made it
> > > > > > > easier to read and maintain.
> > > > > > > 
> > > > > > > Using python functions for more complex tasks allows us the usage
> > > > > > > of unittests. A very basic infrastructure for unittesting using
> > > > > > > the build in python unittest and the bb.parse module was added.
> > > > > > > This was used to test the re-implementation of the
> > > > > > > image-account-extension as a first showcase.
> > > > > > > 
> > > > > > > Tobias Schaffner (5):
> > > > > > >      simplify image-account-extension
> > > > > > >      allow creation of users/groups before rootfs creation
> > > > > > >      create a minimal python unittest infrastructure
> > > > > > >      add unittests for the image-account-extension
> > > > > > >      set minimal python version in user_manual to 3.5
> > > > > > > 
> > > > > > >     doc/user_manual.md                            |   4 +-
> > > > > > >     meta/classes/image-account-extension.bbclass  | 391
> > > > > > > +++++++----------- testsuite/unittests/README.md
> > > > > > > > 28 ++ testsuite/unittests/bitbake.py                |  37 ++
> > > > > > >     testsuite/unittests/rootfs.py                 |  45 ++
> > > > > > >     .../unittests/test_image_account_extension.py | 175 ++++++++
> > > > > > >     6 files changed, 434 insertions(+), 246 deletions(-)
> > > > > > >     create mode 100644 testsuite/unittests/README.md
> > > > > > >     create mode 100644 testsuite/unittests/bitbake.py
> > > > > > >     create mode 100644 testsuite/unittests/rootfs.py
> > > > > > >     create mode 100644
> > > > > > > testsuite/unittests/test_image_account_extension.py
> > > > > > >      
> > > > > >       
> > > > > 
> > > > > 
> > > > > 
> > > > >   
> > > >    
> > 
>