* [PATCH 0/5] Improvements for useradd-staticids.bbclass
@ 2015-11-04 0:06 Peter Kjellerstedt
2015-11-04 0:06 ` [PATCH 1/5] useradd-staticids.bbclass: Treat mutually exclusive options as such Peter Kjellerstedt
` (5 more replies)
0 siblings, 6 replies; 14+ messages in thread
From: Peter Kjellerstedt @ 2015-11-04 0:06 UTC (permalink / raw)
To: openembedded-core
This series of patches aims to improve useradd-staticids.bbclass.
We are currently using useradd-staticids.bbclass to make sure all
users and groups have well defined IDs. So far we have had the
definitions of the users both in the recipes and in the passwd file
used by useradd-staticids.bbclass. Since we have a huge number of
recipes that create users, having to duplicate the definitions all
over the place has turned out to be a burden we should be able to
avoid.
So the current plan for us is to have one passwd file per layer with
the definitions of all users that layer needs. These definitions do
not include the static IDs for the users. Instead the static IDs for
the users are specified in a distro specific passwd-static file. There
is also a distro specific group-static file for the group IDs. With
that in place it should be enough to define a user as:
USERADD_PARAM_${PN} = "--system foobar"
in a recipe and let useradd-staticids.bbclass handle the specifics for
how that user should be defined.
The above worked fine for all users that had a primary group with the
same name as the user. However, it turned out that for users that
wanted some other primary group, specifying it in the passwd file was
not enough. We still had to add --gid <some group> in the recipe where
<some group> had to match what was specified in the passwd file. This
was less than optimal, and somewhat defeated the setup.
It also turned out that for users with a primary group that does not
match the user name, useradd-staticids.bbclass would still add the
creation of a group with the same name as the user (when it parsed the
passwd-static file) and the add another creation of the correct group
(when it parsed the passwd file).
So after spending quite a lot of time on trying to decode how
rewrite_useradd() calculated the --gid option, I came up with this
series of changes that should correct the problems described above and
make the code easier to understand while (hopefully) maintaining
compatibility with the old code.
I fully understand that this is very late for Jethro and do not really
expect it to make it into 2.0.0, but if at all possible I hope it can
make it since that would make my life much easier. :)
//Peter
The following changes since commit fc45deac89ef63ca1c44e763c38ced7dfd72cbe1:
build-appliance-image: Update to jethro head revision (2015-11-03 14:03:03 +0000)
are available in the git repository at:
git://git.yoctoproject.org/poky-contrib pkj/useradd_improvements
http://git.yoctoproject.org/cgit.cgi/poky-contrib/log/?h=pkj/useradd_improvements
Peter Kjellerstedt (5):
useradd-staticids.bbclass: Treat mutually exclusive options as such
useradd-staticids.bbclass: Make --no-user-group have effect
useradd-staticids.bbclass: Simplify some logic
useradd-staticids.bbclass: Simplify the logic for when to add groups
useradd-staticids.bbclass: Read passwd/group files before parsing
meta/classes/useradd-staticids.bbclass | 192 ++++++++++++++++++---------------
1 file changed, 103 insertions(+), 89 deletions(-)
--
2.1.0
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/5] useradd-staticids.bbclass: Treat mutually exclusive options as such
2015-11-04 0:06 [PATCH 0/5] Improvements for useradd-staticids.bbclass Peter Kjellerstedt
@ 2015-11-04 0:06 ` Peter Kjellerstedt
2015-11-04 0:06 ` [PATCH 2/5] useradd-staticids.bbclass: Make --no-user-group have effect Peter Kjellerstedt
` (4 subsequent siblings)
5 siblings, 0 replies; 14+ messages in thread
From: Peter Kjellerstedt @ 2015-11-04 0:06 UTC (permalink / raw)
To: openembedded-core
The useradd options --create-home/--no-create-home and
--user-group/--no-user-group are mutually exclusive and should be
treated as such.
Signed-off-by: Peter Kjellerstedt <peter.kjellerstedt@axis.com>
---
meta/classes/useradd-staticids.bbclass | 27 ++++++++++++++-------------
1 file changed, 14 insertions(+), 13 deletions(-)
diff --git a/meta/classes/useradd-staticids.bbclass b/meta/classes/useradd-staticids.bbclass
index 924d6ea..9d59aca 100644
--- a/meta/classes/useradd-staticids.bbclass
+++ b/meta/classes/useradd-staticids.bbclass
@@ -37,21 +37,21 @@ def update_useradd_static_config(d):
parser.add_argument("-k", "--skel", metavar="SKEL_DIR", help="use this alternative skeleton directory")
parser.add_argument("-K", "--key", metavar="KEY=VALUE", help="override /etc/login.defs defaults")
parser.add_argument("-l", "--no-log-init", help="do not add the user to the lastlog and faillog databases", action="store_true")
- parser.add_argument("-m", "--create-home", help="create the user's home directory", action="store_true")
- parser.add_argument("-M", "--no-create-home", help="do not create the user's home directory", action="store_true")
- parser.add_argument("-N", "--no-user-group", help="do not create a group with the same name as the user", action="store_true")
+ parser.add_argument("-m", "--create-home", help="create the user's home directory", action="store_const", const=True)
+ parser.add_argument("-M", "--no-create-home", dest="create_home", help="do not create the user's home directory", action="store_const", const=False)
+ parser.add_argument("-N", "--no-user-group", dest="user_group", help="do not create a group with the same name as the user", action="store_const", const=False)
parser.add_argument("-o", "--non-unique", help="allow to create users with duplicate (non-unique UID)", action="store_true")
parser.add_argument("-p", "--password", metavar="PASSWORD", help="encrypted password of the new account")
parser.add_argument("-R", "--root", metavar="CHROOT_DIR", help="directory to chroot into")
parser.add_argument("-r", "--system", help="create a system account", action="store_true")
parser.add_argument("-s", "--shell", metavar="SHELL", help="login shell of the new account")
parser.add_argument("-u", "--uid", metavar="UID", help="user ID of the new account")
- parser.add_argument("-U", "--user-group", help="create a group with the same name as the user", action="store_true")
+ parser.add_argument("-U", "--user-group", help="create a group with the same name as the user", action="store_const", const=True)
parser.add_argument("LOGIN", help="Login name of the new user")
# Return a list of configuration files based on either the default
# files/passwd or the contents of USERADD_UID_TABLES
- # paths are resulved via BBPATH
+ # paths are resolved via BBPATH
def get_passwd_list(d):
str = ""
bbpath = d.getVar('BBPATH', True)
@@ -106,7 +106,8 @@ def update_useradd_static_config(d):
# So if the implicit username-group creation is on, then the implicit groupname (LOGIN)
# is used, and we disable the user_group option.
#
- uaargs.groupname = [uaargs.gid, uaargs.LOGIN][not uaargs.gid or uaargs.user_group]
+ user_group = uaargs.user_group is None or uaargs.user_group is True
+ uaargs.groupname = [uaargs.gid, uaargs.LOGIN][not uaargs.gid or user_group]
uaargs.groupid = [uaargs.gid, uaargs.groupname][not uaargs.gid]
uaargs.groupid = [field[3], uaargs.groupid][not field[3]]
@@ -120,7 +121,7 @@ def update_useradd_static_config(d):
# We don't have a number, so we have to add a name
bb.debug(1, "Adding group %s!" % (uaargs.groupname))
uaargs.gid = uaargs.groupid
- uaargs.user_group = False
+ uaargs.user_group = None
groupadd = d.getVar("GROUPADD_PARAM_%s" % pkg, True)
newgroup = "%s %s" % (['', ' --system'][uaargs.system], uaargs.groupname)
if groupadd:
@@ -131,7 +132,7 @@ def update_useradd_static_config(d):
# We have a group name and a group number to assign it to
bb.debug(1, "Adding group %s gid (%s)!" % (uaargs.groupname, uaargs.groupid))
uaargs.gid = uaargs.groupid
- uaargs.user_group = False
+ uaargs.user_group = None
groupadd = d.getVar("GROUPADD_PARAM_%s" % pkg, True)
newgroup = "-g %s %s" % (uaargs.gid, uaargs.groupname)
if groupadd:
@@ -161,16 +162,16 @@ def update_useradd_static_config(d):
newparam += ['', ' --skel %s' % uaargs.skel][uaargs.skel != None]
newparam += ['', ' --key %s' % uaargs.key][uaargs.key != None]
newparam += ['', ' --no-log-init'][uaargs.no_log_init]
- newparam += ['', ' --create-home'][uaargs.create_home]
- newparam += ['', ' --no-create-home'][uaargs.no_create_home]
- newparam += ['', ' --no-user-group'][uaargs.no_user_group]
+ newparam += ['', ' --create-home'][uaargs.create_home is True]
+ newparam += ['', ' --no-create-home'][uaargs.create_home is False]
+ newparam += ['', ' --no-user-group'][uaargs.user_group is False]
newparam += ['', ' --non-unique'][uaargs.non_unique]
newparam += ['', ' --password %s' % uaargs.password][uaargs.password != None]
newparam += ['', ' --root %s' % uaargs.root][uaargs.root != None]
newparam += ['', ' --system'][uaargs.system]
newparam += ['', ' --shell %s' % uaargs.shell][uaargs.shell != None]
newparam += ['', ' --uid %s' % uaargs.uid][uaargs.uid != None]
- newparam += ['', ' --user-group'][uaargs.user_group]
+ newparam += ['', ' --user-group'][uaargs.user_group is True]
newparam += ' %s' % uaargs.LOGIN
newparams.append(newparam)
@@ -192,7 +193,7 @@ def update_useradd_static_config(d):
# Return a list of configuration files based on either the default
# files/group or the contents of USERADD_GID_TABLES
- # paths are resulved via BBPATH
+ # paths are resolved via BBPATH
def get_group_list(d):
str = ""
bbpath = d.getVar('BBPATH', True)
--
2.1.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 2/5] useradd-staticids.bbclass: Make --no-user-group have effect
2015-11-04 0:06 [PATCH 0/5] Improvements for useradd-staticids.bbclass Peter Kjellerstedt
2015-11-04 0:06 ` [PATCH 1/5] useradd-staticids.bbclass: Treat mutually exclusive options as such Peter Kjellerstedt
@ 2015-11-04 0:06 ` Peter Kjellerstedt
2015-11-04 0:06 ` [PATCH 3/5] useradd-staticids.bbclass: Simplify some logic Peter Kjellerstedt
` (3 subsequent siblings)
5 siblings, 0 replies; 14+ messages in thread
From: Peter Kjellerstedt @ 2015-11-04 0:06 UTC (permalink / raw)
To: openembedded-core
If --no-user-group is specified in USERADD_PARAM_${PN} for a user and
no --gid is specified, then we should not assume that the group name
for the user is the user name.
Signed-off-by: Peter Kjellerstedt <peter.kjellerstedt@axis.com>
---
meta/classes/useradd-staticids.bbclass | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/meta/classes/useradd-staticids.bbclass b/meta/classes/useradd-staticids.bbclass
index 9d59aca..c2e6579 100644
--- a/meta/classes/useradd-staticids.bbclass
+++ b/meta/classes/useradd-staticids.bbclass
@@ -107,7 +107,7 @@ def update_useradd_static_config(d):
# is used, and we disable the user_group option.
#
user_group = uaargs.user_group is None or uaargs.user_group is True
- uaargs.groupname = [uaargs.gid, uaargs.LOGIN][not uaargs.gid or user_group]
+ uaargs.groupname = [uaargs.LOGIN, uaargs.gid][not user_group]
uaargs.groupid = [uaargs.gid, uaargs.groupname][not uaargs.gid]
uaargs.groupid = [field[3], uaargs.groupid][not field[3]]
--
2.1.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 3/5] useradd-staticids.bbclass: Simplify some logic
2015-11-04 0:06 [PATCH 0/5] Improvements for useradd-staticids.bbclass Peter Kjellerstedt
2015-11-04 0:06 ` [PATCH 1/5] useradd-staticids.bbclass: Treat mutually exclusive options as such Peter Kjellerstedt
2015-11-04 0:06 ` [PATCH 2/5] useradd-staticids.bbclass: Make --no-user-group have effect Peter Kjellerstedt
@ 2015-11-04 0:06 ` Peter Kjellerstedt
2015-11-04 0:06 ` [PATCH 4/5] useradd-staticids.bbclass: Simplify the logic for when to add groups Peter Kjellerstedt
` (2 subsequent siblings)
5 siblings, 0 replies; 14+ messages in thread
From: Peter Kjellerstedt @ 2015-11-04 0:06 UTC (permalink / raw)
To: openembedded-core
The [<on_true>, <on_false>][not <condition>] construct may solve the
problem of implementing a conditional operator, but it is not very
readable. At least I find this:
uaargs.groupid = field[3] or uaargs.gid or uaargs.groupname
a lot more readable than this:
uaargs.groupid = [uaargs.gid, uaargs.groupname][not uaargs.gid]
uaargs.groupid = [field[3], uaargs.groupid][not field[3]]
Also, the official conditional operator since Python 2.5 (<on_true> if
<condition> else <on_false>) does not evaluate both <on_false> and
<on_true> as [<on_true>, <on_false>][not <condition>] does.
Signed-off-by: Peter Kjellerstedt <peter.kjellerstedt@axis.com>
---
meta/classes/useradd-staticids.bbclass | 15 +++++++--------
1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/meta/classes/useradd-staticids.bbclass b/meta/classes/useradd-staticids.bbclass
index c2e6579..0e855e9 100644
--- a/meta/classes/useradd-staticids.bbclass
+++ b/meta/classes/useradd-staticids.bbclass
@@ -97,7 +97,7 @@ def update_useradd_static_config(d):
if field[0] == uaargs.LOGIN:
if uaargs.uid and field[2] and (uaargs.uid != field[2]):
bb.warn("%s: Changing username %s's uid from (%s) to (%s), verify configuration files!" % (d.getVar('PN', True), uaargs.LOGIN, uaargs.uid, field[2]))
- uaargs.uid = [field[2], uaargs.uid][not field[2]]
+ uaargs.uid = field[2] or uaargs.uid
# Determine the possible groupname
# Unless the group name (or gid) is specified, we assume that the LOGIN is the groupname
@@ -107,9 +107,8 @@ def update_useradd_static_config(d):
# is used, and we disable the user_group option.
#
user_group = uaargs.user_group is None or uaargs.user_group is True
- uaargs.groupname = [uaargs.LOGIN, uaargs.gid][not user_group]
- uaargs.groupid = [uaargs.gid, uaargs.groupname][not uaargs.gid]
- uaargs.groupid = [field[3], uaargs.groupid][not field[3]]
+ uaargs.groupname = uaargs.LOGIN if user_group else uaargs.gid
+ uaargs.groupid = field[3] or uaargs.gid or uaargs.groupname
if not uaargs.gid or uaargs.gid != uaargs.groupid:
if (uaargs.groupid and uaargs.groupid.isdigit()) and (uaargs.groupname and uaargs.groupname.isdigit()) and (uaargs.groupid != uaargs.groupname):
@@ -123,7 +122,7 @@ def update_useradd_static_config(d):
uaargs.gid = uaargs.groupid
uaargs.user_group = None
groupadd = d.getVar("GROUPADD_PARAM_%s" % pkg, True)
- newgroup = "%s %s" % (['', ' --system'][uaargs.system], uaargs.groupname)
+ newgroup = "%s %s" % (' --system' if uaargs.system else '', uaargs.groupname)
if groupadd:
d.setVar("GROUPADD_PARAM_%s" % pkg, "%s ; %s" % (groupadd, newgroup))
else:
@@ -140,9 +139,9 @@ def update_useradd_static_config(d):
else:
d.setVar("GROUPADD_PARAM_%s" % pkg, newgroup)
- uaargs.comment = ["'%s'" % field[4], uaargs.comment][not field[4]]
- uaargs.home_dir = [field[5], uaargs.home_dir][not field[5]]
- uaargs.shell = [field[6], uaargs.shell][not field[6]]
+ uaargs.comment = "'%s'" % field[4] if field[4] else uaargs.comment
+ uaargs.home_dir = field[5] or uaargs.home_dir
+ uaargs.shell = field[6] or uaargs.shell
break
# Should be an error if a specific option is set...
--
2.1.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 4/5] useradd-staticids.bbclass: Simplify the logic for when to add groups
2015-11-04 0:06 [PATCH 0/5] Improvements for useradd-staticids.bbclass Peter Kjellerstedt
` (2 preceding siblings ...)
2015-11-04 0:06 ` [PATCH 3/5] useradd-staticids.bbclass: Simplify some logic Peter Kjellerstedt
@ 2015-11-04 0:06 ` Peter Kjellerstedt
2015-11-04 0:06 ` [PATCH 5/5] useradd-staticids.bbclass: Read passwd/group files before parsing Peter Kjellerstedt
2015-11-04 0:33 ` [PATCH 0/5] Improvements for useradd-staticids.bbclass Mark Hatle
5 siblings, 0 replies; 14+ messages in thread
From: Peter Kjellerstedt @ 2015-11-04 0:06 UTC (permalink / raw)
To: openembedded-core
The original code was near impossible to follow, and missed a couple
of cases. For example, if one added the following line to the passwd
file specified in USERADD_UID_TABLES:
foobar:x:12345:nogroup::/:/bin/sh
and then specified the user as:
USERADD_PARAM_${PN} = "--system foobar"
one would then assume that the foobar user would be created with the
primary group set to nogroup. However, it was not (the primary group
would be foobar), and the only way to get it correct was to explicitly
add --gid nogroup to the USERADD_PARAM_${PN}.
Signed-off-by: Peter Kjellerstedt <peter.kjellerstedt@axis.com>
---
meta/classes/useradd-staticids.bbclass | 36 +++++++++++++++-------------------
1 file changed, 16 insertions(+), 20 deletions(-)
diff --git a/meta/classes/useradd-staticids.bbclass b/meta/classes/useradd-staticids.bbclass
index 0e855e9..df4902e 100644
--- a/meta/classes/useradd-staticids.bbclass
+++ b/meta/classes/useradd-staticids.bbclass
@@ -110,30 +110,26 @@ def update_useradd_static_config(d):
uaargs.groupname = uaargs.LOGIN if user_group else uaargs.gid
uaargs.groupid = field[3] or uaargs.gid or uaargs.groupname
- if not uaargs.gid or uaargs.gid != uaargs.groupid:
- if (uaargs.groupid and uaargs.groupid.isdigit()) and (uaargs.groupname and uaargs.groupname.isdigit()) and (uaargs.groupid != uaargs.groupname):
+ if uaargs.groupid and uaargs.gid != uaargs.groupid:
+ newgroup = None
+ if not uaargs.groupid.isdigit():
+ # We don't have a group number, so we have to add a name
+ bb.debug(1, "Adding group %s!" % uaargs.groupid)
+ newgroup = "%s %s" % (' --system' if uaargs.system else '', uaargs.groupid)
+ elif uaargs.groupname and not uaargs.groupname.isdigit():
+ # We have a group name and a group number to assign it to
+ bb.debug(1, "Adding group %s (gid %s)!" % (uaargs.groupname, uaargs.groupid))
+ newgroup = "-g %s %s" % (uaargs.groupid, uaargs.groupname)
+ else:
# We want to add a group, but we don't know it's name... so we can't add the group...
# We have to assume the group has previously been added or we'll fail on the adduser...
# Note: specifying the actual gid is very rare in OE, usually the group name is specified.
- bb.warn("%s: Changing gid for login %s from (%s) to (%s), verify configuration files!" % (d.getVar('PN', True), uaargs.LOGIN, uaargs.groupname, uaargs.gid))
- elif (uaargs.groupid and not uaargs.groupid.isdigit()) and uaargs.groupid == uaargs.groupname:
- # We don't have a number, so we have to add a name
- bb.debug(1, "Adding group %s!" % (uaargs.groupname))
- uaargs.gid = uaargs.groupid
- uaargs.user_group = None
- groupadd = d.getVar("GROUPADD_PARAM_%s" % pkg, True)
- newgroup = "%s %s" % (' --system' if uaargs.system else '', uaargs.groupname)
- if groupadd:
- d.setVar("GROUPADD_PARAM_%s" % pkg, "%s ; %s" % (groupadd, newgroup))
- else:
- d.setVar("GROUPADD_PARAM_%s" % pkg, newgroup)
- elif uaargs.groupname and (uaargs.groupid and uaargs.groupid.isdigit()):
- # We have a group name and a group number to assign it to
- bb.debug(1, "Adding group %s gid (%s)!" % (uaargs.groupname, uaargs.groupid))
- uaargs.gid = uaargs.groupid
- uaargs.user_group = None
+ bb.warn("%s: Changing gid for login %s to %s, verify configuration files!" % (d.getVar('PN', True), uaargs.LOGIN, uaargs.groupid))
+
+ uaargs.gid = uaargs.groupid
+ uaargs.user_group = None
+ if newgroup:
groupadd = d.getVar("GROUPADD_PARAM_%s" % pkg, True)
- newgroup = "-g %s %s" % (uaargs.gid, uaargs.groupname)
if groupadd:
d.setVar("GROUPADD_PARAM_%s" % pkg, "%s ; %s" % (groupadd, newgroup))
else:
--
2.1.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 5/5] useradd-staticids.bbclass: Read passwd/group files before parsing
2015-11-04 0:06 [PATCH 0/5] Improvements for useradd-staticids.bbclass Peter Kjellerstedt
` (3 preceding siblings ...)
2015-11-04 0:06 ` [PATCH 4/5] useradd-staticids.bbclass: Simplify the logic for when to add groups Peter Kjellerstedt
@ 2015-11-04 0:06 ` Peter Kjellerstedt
2015-11-04 0:32 ` Mark Hatle
2015-11-04 0:33 ` [PATCH 0/5] Improvements for useradd-staticids.bbclass Mark Hatle
5 siblings, 1 reply; 14+ messages in thread
From: Peter Kjellerstedt @ 2015-11-04 0:06 UTC (permalink / raw)
To: openembedded-core
Read and merge the passwd/group files before parsing the user and
group definitions. This means they will only be read once per
recipe. This solves a problem where if a user was definied in multiple
files, it could generate group definitions for groups that should not
be created. E.g., if the first passwd file read defines a user as:
foobar::1234::::
and the second passwd file defines it as:
foobar:::nogroup:The foobar user:/:/bin/sh
then a foobar group would be created even if the user will use the
nogroup as its primary group.
Signed-off-by: Peter Kjellerstedt <peter.kjellerstedt@axis.com>
---
meta/classes/useradd-staticids.bbclass | 168 ++++++++++++++++++---------------
1 file changed, 93 insertions(+), 75 deletions(-)
diff --git a/meta/classes/useradd-staticids.bbclass b/meta/classes/useradd-staticids.bbclass
index df4902e..a9b506d 100644
--- a/meta/classes/useradd-staticids.bbclass
+++ b/meta/classes/useradd-staticids.bbclass
@@ -22,6 +22,30 @@ def update_useradd_static_config(d):
and return it as a list"""
return list(itertools.islice(itertools.chain(iterable, itertools.repeat(obj)), length))
+ def merge_files(file_list, exp_fields):
+ """Read each passwd/group file in file_list, split each line and create
+ a dictionary with the user/group names as keys and the split lines as
+ values. If the user/group name already exists in the dictionary, then
+ update any fields in the list with the values from the new list (if they
+ are set)."""
+ id_table = dict()
+ for conf in file_list.split():
+ if os.path.exists(conf):
+ f = open(conf, "r")
+ for line in f:
+ if line.startswith('#'):
+ continue
+ # Make sure there always are at least exp_fields elements in
+ # the field list. This allows for leaving out trailing
+ # colons in the files.
+ fields = list_extend(line.rstrip().split(":"), exp_fields)
+ if fields[0] not in id_table:
+ id_table[fields[0]] = fields
+ else:
+ id_table[fields[0]] = list(itertools.imap(lambda x, y: x or y, fields, id_table[fields[0]]))
+
+ return id_table
+
# We parse and rewrite the useradd components
def rewrite_useradd(params):
# The following comes from --help on useradd from shadow
@@ -63,6 +87,7 @@ def update_useradd_static_config(d):
return str
newparams = []
+ users = None
for param in re.split('''[ \t]*;[ \t]*(?=(?:[^'"]|'[^']*'|"[^"]*")*$)''', params):
param = param.strip()
if not param:
@@ -72,10 +97,9 @@ def update_useradd_static_config(d):
except:
raise bb.build.FuncFailed("%s: Unable to parse arguments for USERADD_PARAM_%s: '%s'" % (d.getVar('PN', True), pkg, param))
- # files/passwd or the contents of USERADD_UID_TABLES
+ # Read all passwd files specified in USERADD_UID_TABLES or files/passwd
# Use the standard passwd layout:
# username:password:user_id:group_id:comment:home_directory:login_shell
- # (we want to process in reverse order, as 'last found' in the list wins)
#
# If a field is left blank, the original value will be used. The 'username'
# field is required.
@@ -84,61 +108,57 @@ def update_useradd_static_config(d):
# in the useradd command may introduce a security hole. It's assumed that
# all new users get the default ('*' which prevents login) until the user is
# specifically configured by the system admin.
- for conf in get_passwd_list(d).split()[::-1]:
- if os.path.exists(conf):
- f = open(conf, "r")
- for line in f:
- if line.startswith('#'):
- continue
- # Make sure there always are at least seven elements in
- # the field list. This allows for leaving out trailing
- # colons in the passwd file.
- field = list_extend(line.rstrip().split(":"), 7)
- if field[0] == uaargs.LOGIN:
- if uaargs.uid and field[2] and (uaargs.uid != field[2]):
- bb.warn("%s: Changing username %s's uid from (%s) to (%s), verify configuration files!" % (d.getVar('PN', True), uaargs.LOGIN, uaargs.uid, field[2]))
- uaargs.uid = field[2] or uaargs.uid
-
- # Determine the possible groupname
- # Unless the group name (or gid) is specified, we assume that the LOGIN is the groupname
- #
- # By default the system has creation of the matching groups enabled
- # So if the implicit username-group creation is on, then the implicit groupname (LOGIN)
- # is used, and we disable the user_group option.
- #
- user_group = uaargs.user_group is None or uaargs.user_group is True
- uaargs.groupname = uaargs.LOGIN if user_group else uaargs.gid
- uaargs.groupid = field[3] or uaargs.gid or uaargs.groupname
-
- if uaargs.groupid and uaargs.gid != uaargs.groupid:
- newgroup = None
- if not uaargs.groupid.isdigit():
- # We don't have a group number, so we have to add a name
- bb.debug(1, "Adding group %s!" % uaargs.groupid)
- newgroup = "%s %s" % (' --system' if uaargs.system else '', uaargs.groupid)
- elif uaargs.groupname and not uaargs.groupname.isdigit():
- # We have a group name and a group number to assign it to
- bb.debug(1, "Adding group %s (gid %s)!" % (uaargs.groupname, uaargs.groupid))
- newgroup = "-g %s %s" % (uaargs.groupid, uaargs.groupname)
- else:
- # We want to add a group, but we don't know it's name... so we can't add the group...
- # We have to assume the group has previously been added or we'll fail on the adduser...
- # Note: specifying the actual gid is very rare in OE, usually the group name is specified.
- bb.warn("%s: Changing gid for login %s to %s, verify configuration files!" % (d.getVar('PN', True), uaargs.LOGIN, uaargs.groupid))
-
- uaargs.gid = uaargs.groupid
- uaargs.user_group = None
- if newgroup:
- groupadd = d.getVar("GROUPADD_PARAM_%s" % pkg, True)
- if groupadd:
- d.setVar("GROUPADD_PARAM_%s" % pkg, "%s ; %s" % (groupadd, newgroup))
- else:
- d.setVar("GROUPADD_PARAM_%s" % pkg, newgroup)
-
- uaargs.comment = "'%s'" % field[4] if field[4] else uaargs.comment
- uaargs.home_dir = field[5] or uaargs.home_dir
- uaargs.shell = field[6] or uaargs.shell
- break
+ if not users:
+ users = merge_files(get_passwd_list(d), 7)
+
+ if uaargs.LOGIN not in users:
+ continue
+
+ field = users[uaargs.LOGIN]
+
+ if uaargs.uid and field[2] and (uaargs.uid != field[2]):
+ bb.warn("%s: Changing username %s's uid from (%s) to (%s), verify configuration files!" % (d.getVar('PN', True), uaargs.LOGIN, uaargs.uid, field[2]))
+ uaargs.uid = field[2] or uaargs.uid
+
+ # Determine the possible groupname
+ # Unless the group name (or gid) is specified, we assume that the LOGIN is the groupname
+ #
+ # By default the system has creation of the matching groups enabled
+ # So if the implicit username-group creation is on, then the implicit groupname (LOGIN)
+ # is used, and we disable the user_group option.
+ #
+ user_group = uaargs.user_group is None or uaargs.user_group is True
+ uaargs.groupname = uaargs.LOGIN if user_group else uaargs.gid
+ uaargs.groupid = field[3] or uaargs.gid or uaargs.groupname
+
+ if uaargs.groupid and uaargs.gid != uaargs.groupid:
+ newgroup = None
+ if not uaargs.groupid.isdigit():
+ # We don't have a group number, so we have to add a name
+ bb.debug(1, "Adding group %s!" % uaargs.groupid)
+ newgroup = "%s %s" % (' --system' if uaargs.system else '', uaargs.groupid)
+ elif uaargs.groupname and not uaargs.groupname.isdigit():
+ # We have a group name and a group number to assign it to
+ bb.debug(1, "Adding group %s (gid %s)!" % (uaargs.groupname, uaargs.groupid))
+ newgroup = "-g %s %s" % (uaargs.groupid, uaargs.groupname)
+ else:
+ # We want to add a group, but we don't know it's name... so we can't add the group...
+ # We have to assume the group has previously been added or we'll fail on the adduser...
+ # Note: specifying the actual gid is very rare in OE, usually the group name is specified.
+ bb.warn("%s: Changing gid for login %s to %s, verify configuration files!" % (d.getVar('PN', True), uaargs.LOGIN, uaargs.groupid))
+
+ uaargs.gid = uaargs.groupid
+ uaargs.user_group = None
+ if newgroup:
+ groupadd = d.getVar("GROUPADD_PARAM_%s" % pkg, True)
+ if groupadd:
+ d.setVar("GROUPADD_PARAM_%s" % pkg, "%s; %s" % (groupadd, newgroup))
+ else:
+ d.setVar("GROUPADD_PARAM_%s" % pkg, newgroup)
+
+ uaargs.comment = "'%s'" % field[4] if field[4] else uaargs.comment
+ uaargs.home_dir = field[5] or uaargs.home_dir
+ uaargs.shell = field[6] or uaargs.shell
# Should be an error if a specific option is set...
if d.getVar('USERADD_ERROR_DYNAMIC', True) == '1' and not ((uaargs.uid and uaargs.uid.isdigit()) and uaargs.gid):
@@ -171,7 +191,7 @@ def update_useradd_static_config(d):
newparams.append(newparam)
- return " ;".join(newparams).strip()
+ return ";".join(newparams).strip()
# We parse and rewrite the groupadd components
def rewrite_groupadd(params):
@@ -200,6 +220,7 @@ def update_useradd_static_config(d):
return str
newparams = []
+ groups = None
for param in re.split('''[ \t]*;[ \t]*(?=(?:[^'"]|'[^']*'|"[^"]*")*$)''', params):
param = param.strip()
if not param:
@@ -210,7 +231,7 @@ def update_useradd_static_config(d):
except:
raise bb.build.FuncFailed("%s: Unable to parse arguments for GROUPADD_PARAM_%s: '%s'" % (d.getVar('PN', True), pkg, param))
- # Need to iterate over layers and open the right file(s)
+ # Read all group files specified in USERADD_GID_TABLES or files/group
# Use the standard group layout:
# groupname:password:group_id:group_members
#
@@ -219,21 +240,18 @@ def update_useradd_static_config(d):
#
# Note: similar to the passwd file, the 'password' filed is ignored
# Note: group_members is ignored, group members must be configured with the GROUPMEMS_PARAM
- for conf in get_group_list(d).split()[::-1]:
- if os.path.exists(conf):
- f = open(conf, "r")
- for line in f:
- if line.startswith('#'):
- continue
- # Make sure there always are at least four elements in
- # the field list. This allows for leaving out trailing
- # colons in the group file.
- field = list_extend(line.rstrip().split(":"), 4)
- if field[0] == gaargs.GROUP and field[2]:
- if gaargs.gid and (gaargs.gid != field[2]):
- bb.warn("%s: Changing groupname %s's gid from (%s) to (%s), verify configuration files!" % (d.getVar('PN', True), gaargs.GROUP, gaargs.gid, field[2]))
- gaargs.gid = field[2]
- break
+ if not groups:
+ groups = merge_files(get_group_list(d), 4)
+
+ if gaargs.GROUP not in groups:
+ continue
+
+ field = groups[gaargs.GROUP]
+
+ if field[2]:
+ if gaargs.gid and (gaargs.gid != field[2]):
+ bb.warn("%s: Changing groupname %s's gid from (%s) to (%s), verify configuration files!" % (d.getVar('PN', True), gaargs.GROUP, gaargs.gid, field[2]))
+ gaargs.gid = field[2]
if d.getVar('USERADD_ERROR_DYNAMIC', True) == '1' and not (gaargs.gid and gaargs.gid.isdigit()):
#bb.error("Skipping recipe %s, package %s which adds groupname %s does not have a static gid defined." % (d.getVar('PN', True), pkg, gaargs.GROUP))
@@ -251,7 +269,7 @@ def update_useradd_static_config(d):
newparams.append(newparam)
- return " ;".join(newparams).strip()
+ return ";".join(newparams).strip()
# Load and process the users and groups, rewriting the adduser/addgroup params
useradd_packages = d.getVar('USERADD_PACKAGES', True)
--
2.1.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 5/5] useradd-staticids.bbclass: Read passwd/group files before parsing
2015-11-04 0:06 ` [PATCH 5/5] useradd-staticids.bbclass: Read passwd/group files before parsing Peter Kjellerstedt
@ 2015-11-04 0:32 ` Mark Hatle
2015-11-06 20:09 ` Peter Kjellerstedt
0 siblings, 1 reply; 14+ messages in thread
From: Mark Hatle @ 2015-11-04 0:32 UTC (permalink / raw)
To: Peter Kjellerstedt, openembedded-core
On 11/3/15 6:06 PM, Peter Kjellerstedt wrote:
> Read and merge the passwd/group files before parsing the user and
> group definitions. This means they will only be read once per
> recipe. This solves a problem where if a user was definied in multiple
> files, it could generate group definitions for groups that should not
> be created. E.g., if the first passwd file read defines a user as:
>
> foobar::1234::::
>
> and the second passwd file defines it as:
>
> foobar:::nogroup:The foobar user:/:/bin/sh
>
> then a foobar group would be created even if the user will use the
> nogroup as its primary group.
One minor thing
> @@ -251,7 +269,7 @@ def update_useradd_static_config(d):
>
> newparams.append(newparam)
>
> - return " ;".join(newparams).strip()
> + return ";".join(newparams).strip()
>
> # Load and process the users and groups, rewriting the adduser/addgroup params
> useradd_packages = d.getVar('USERADD_PACKAGES', True)
>
The space was required because you could generate a user/group add line that
ended with a string. Without the space, you could end up merging two sets of
arguments causing a failure condition.
So I think that it should be retained unless there is a specific reason you
believe it should be removed.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/5] Improvements for useradd-staticids.bbclass
2015-11-04 0:06 [PATCH 0/5] Improvements for useradd-staticids.bbclass Peter Kjellerstedt
` (4 preceding siblings ...)
2015-11-04 0:06 ` [PATCH 5/5] useradd-staticids.bbclass: Read passwd/group files before parsing Peter Kjellerstedt
@ 2015-11-04 0:33 ` Mark Hatle
5 siblings, 0 replies; 14+ messages in thread
From: Mark Hatle @ 2015-11-04 0:33 UTC (permalink / raw)
To: Peter Kjellerstedt, openembedded-core
I looked over this series (I have not tried it, but) everything looks ok to me.
I did make one comment on a minor hunk -- but otherwise I don't see any issues.
The python style police might have some further comments.
Acked-by: Mark Hatle <mark.hatle@windriver.com>
On 11/3/15 6:06 PM, Peter Kjellerstedt wrote:
> This series of patches aims to improve useradd-staticids.bbclass.
>
> We are currently using useradd-staticids.bbclass to make sure all
> users and groups have well defined IDs. So far we have had the
> definitions of the users both in the recipes and in the passwd file
> used by useradd-staticids.bbclass. Since we have a huge number of
> recipes that create users, having to duplicate the definitions all
> over the place has turned out to be a burden we should be able to
> avoid.
>
> So the current plan for us is to have one passwd file per layer with
> the definitions of all users that layer needs. These definitions do
> not include the static IDs for the users. Instead the static IDs for
> the users are specified in a distro specific passwd-static file. There
> is also a distro specific group-static file for the group IDs. With
> that in place it should be enough to define a user as:
>
> USERADD_PARAM_${PN} = "--system foobar"
>
> in a recipe and let useradd-staticids.bbclass handle the specifics for
> how that user should be defined.
>
> The above worked fine for all users that had a primary group with the
> same name as the user. However, it turned out that for users that
> wanted some other primary group, specifying it in the passwd file was
> not enough. We still had to add --gid <some group> in the recipe where
> <some group> had to match what was specified in the passwd file. This
> was less than optimal, and somewhat defeated the setup.
>
> It also turned out that for users with a primary group that does not
> match the user name, useradd-staticids.bbclass would still add the
> creation of a group with the same name as the user (when it parsed the
> passwd-static file) and the add another creation of the correct group
> (when it parsed the passwd file).
>
> So after spending quite a lot of time on trying to decode how
> rewrite_useradd() calculated the --gid option, I came up with this
> series of changes that should correct the problems described above and
> make the code easier to understand while (hopefully) maintaining
> compatibility with the old code.
>
> I fully understand that this is very late for Jethro and do not really
> expect it to make it into 2.0.0, but if at all possible I hope it can
> make it since that would make my life much easier. :)
>
> //Peter
>
> The following changes since commit fc45deac89ef63ca1c44e763c38ced7dfd72cbe1:
>
> build-appliance-image: Update to jethro head revision (2015-11-03 14:03:03 +0000)
>
> are available in the git repository at:
>
> git://git.yoctoproject.org/poky-contrib pkj/useradd_improvements
> http://git.yoctoproject.org/cgit.cgi/poky-contrib/log/?h=pkj/useradd_improvements
>
> Peter Kjellerstedt (5):
> useradd-staticids.bbclass: Treat mutually exclusive options as such
> useradd-staticids.bbclass: Make --no-user-group have effect
> useradd-staticids.bbclass: Simplify some logic
> useradd-staticids.bbclass: Simplify the logic for when to add groups
> useradd-staticids.bbclass: Read passwd/group files before parsing
>
> meta/classes/useradd-staticids.bbclass | 192 ++++++++++++++++++---------------
> 1 file changed, 103 insertions(+), 89 deletions(-)
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 5/5] useradd-staticids.bbclass: Read passwd/group files before parsing
2015-11-04 0:32 ` Mark Hatle
@ 2015-11-06 20:09 ` Peter Kjellerstedt
2015-11-06 20:14 ` Mark Hatle
0 siblings, 1 reply; 14+ messages in thread
From: Peter Kjellerstedt @ 2015-11-06 20:09 UTC (permalink / raw)
To: Mark Hatle; +Cc: openembedded-core@lists.openembedded.org
> -----Original Message-----
> From: Mark Hatle [mailto:mark.hatle@windriver.com]
> Sent: den 4 november 2015 01:33
> To: Peter Kjellerstedt; openembedded-core@lists.openembedded.org
> Subject: Re: [PATCH 5/5] useradd-staticids.bbclass: Read passwd/group
> files before parsing
>
> On 11/3/15 6:06 PM, Peter Kjellerstedt wrote:
> > Read and merge the passwd/group files before parsing the user and
> > group definitions. This means they will only be read once per
> > recipe. This solves a problem where if a user was definied in multiple
> > files, it could generate group definitions for groups that should not
> > be created. E.g., if the first passwd file read defines a user as:
> >
> > foobar::1234::::
> >
> > and the second passwd file defines it as:
> >
> > foobar:::nogroup:The foobar user:/:/bin/sh
> >
> > then a foobar group would be created even if the user will use the
> > nogroup as its primary group.
>
> One minor thing
>
> > @@ -251,7 +269,7 @@ def update_useradd_static_config(d):
> >
> > newparams.append(newparam)
> >
> > - return " ;".join(newparams).strip()
> > + return ";".join(newparams).strip()
> >
> > # Load and process the users and groups, rewriting the adduser/addgroup params
> > useradd_packages = d.getVar('USERADD_PACKAGES', True)
> >
>
> The space was required because you could generate a user/group add
> line that ended with a string. Without the space, you could end up
> merging two sets of arguments causing a failure condition.
>
> So I think that it should be retained unless there is a specific
> reason you believe it should be removed.
I cannot see how that space can make any difference. Each set of
useradd/grouppadd options added to newparams has the user/group
name at the end of the string. And if that somehow interferes with
the semicolon, then the code in useradd.bbclass which simply does
"cut -d ';'" to split the useradd/groupadd line would break already.
Actually, now that I think about it, I do wonder why
useradd-staticids.bbclass use this advanced variant to split the
useradd/groupadd lines:
for param in re.split('''[ \t]*;[ \t]*(?=(?:[^'"]|'[^']*'|"[^"]*")*$)''', params):
when this would do the job just as well:
for param in params.split(';'):
given that that is what useradd.bbclass does. It looks as if tries
to support something like --comment "something with a ; in it", but
using that would break in useradd.bbclass anyway...
//Peter
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 5/5] useradd-staticids.bbclass: Read passwd/group files before parsing
2015-11-06 20:09 ` Peter Kjellerstedt
@ 2015-11-06 20:14 ` Mark Hatle
2015-11-10 15:54 ` Peter Kjellerstedt
0 siblings, 1 reply; 14+ messages in thread
From: Mark Hatle @ 2015-11-06 20:14 UTC (permalink / raw)
To: Peter Kjellerstedt; +Cc: openembedded-core@lists.openembedded.org
On 11/6/15 2:09 PM, Peter Kjellerstedt wrote:
>> -----Original Message-----
>> From: Mark Hatle [mailto:mark.hatle@windriver.com]
>> Sent: den 4 november 2015 01:33
>> To: Peter Kjellerstedt; openembedded-core@lists.openembedded.org
>> Subject: Re: [PATCH 5/5] useradd-staticids.bbclass: Read passwd/group
>> files before parsing
>>
>> On 11/3/15 6:06 PM, Peter Kjellerstedt wrote:
>>> Read and merge the passwd/group files before parsing the user and
>>> group definitions. This means they will only be read once per
>>> recipe. This solves a problem where if a user was definied in multiple
>>> files, it could generate group definitions for groups that should not
>>> be created. E.g., if the first passwd file read defines a user as:
>>>
>>> foobar::1234::::
>>>
>>> and the second passwd file defines it as:
>>>
>>> foobar:::nogroup:The foobar user:/:/bin/sh
>>>
>>> then a foobar group would be created even if the user will use the
>>> nogroup as its primary group.
>>
>> One minor thing
>>
>>> @@ -251,7 +269,7 @@ def update_useradd_static_config(d):
>>>
>>> newparams.append(newparam)
>>>
>>> - return " ;".join(newparams).strip()
>>> + return ";".join(newparams).strip()
>>>
>>> # Load and process the users and groups, rewriting the adduser/addgroup params
>>> useradd_packages = d.getVar('USERADD_PACKAGES', True)
>>>
>>
>> The space was required because you could generate a user/group add
>> line that ended with a string. Without the space, you could end up
>> merging two sets of arguments causing a failure condition.
>>
>> So I think that it should be retained unless there is a specific
>> reason you believe it should be removed.
>
> I cannot see how that space can make any difference. Each set of
> useradd/grouppadd options added to newparams has the user/group
> name at the end of the string. And if that somehow interferes with
> the semicolon, then the code in useradd.bbclass which simply does
> "cut -d ';'" to split the useradd/groupadd line would break already.
The contents when originally parsed my be run as arguments to a shell script or
as parameters to these functions.
In the shell script world not have a space can confuse the argument parsing into
thinking the ; is part of the argument.
You don't have that in the python world with the split behavior.
> Actually, now that I think about it, I do wonder why
> useradd-staticids.bbclass use this advanced variant to split the
> useradd/groupadd lines:
>
> for param in re.split('''[ \t]*;[ \t]*(?=(?:[^'"]|'[^']*'|"[^"]*")*$)''', params):
It is perfectly legal to allow a ';' in the middle of a parameter (that allows
it), a parameter that is quoted.
Something like:
adduser -c "This user;that user;all users" -d /home/allusers alluser
it's odd, but I've certainly seen people put ';' in the comment before.. and it
is legal in other palces, like the home dir and such -- just not advised.
> when this would do the job just as well:
>
> for param in params.split(';'):
>
> given that that is what useradd.bbclass does. It looks as if tries
> to support something like --comment "something with a ; in it", but
> using that would break in useradd.bbclass anyway...
Then the useradd class is broken in this case. The --comment processing needs
to work, it's just rarely used in the normal case, but very much used in the
"lets take a previously generated passwd file and reuse it" case of the
adduser-static.
> //Peter
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 5/5] useradd-staticids.bbclass: Read passwd/group files before parsing
2015-11-06 20:14 ` Mark Hatle
@ 2015-11-10 15:54 ` Peter Kjellerstedt
2015-11-10 16:07 ` Mark Hatle
0 siblings, 1 reply; 14+ messages in thread
From: Peter Kjellerstedt @ 2015-11-10 15:54 UTC (permalink / raw)
To: Mark Hatle; +Cc: openembedded-core@lists.openembedded.org
> -----Original Message-----
> From: Mark Hatle [mailto:mark.hatle@windriver.com]
> Sent: den 6 november 2015 21:14
> To: Peter Kjellerstedt
> Cc: openembedded-core@lists.openembedded.org
> Subject: Re: [PATCH 5/5] useradd-staticids.bbclass: Read passwd/group
> files before parsing
>
> On 11/6/15 2:09 PM, Peter Kjellerstedt wrote:
> >> -----Original Message-----
> >> From: Mark Hatle [mailto:mark.hatle@windriver.com]
> >> Sent: den 4 november 2015 01:33
> >> To: Peter Kjellerstedt; openembedded-core@lists.openembedded.org
> >> Subject: Re: [PATCH 5/5] useradd-staticids.bbclass: Read
> >> passwd/group files before parsing
> >>
> >> On 11/3/15 6:06 PM, Peter Kjellerstedt wrote:
> >>> Read and merge the passwd/group files before parsing the user and
> >>> group definitions. This means they will only be read once per
> >>> recipe. This solves a problem where if a user was definied in
> >>> multiple files, it could generate group definitions for groups
> >>> that should not be created. E.g., if the first passwd file read
> >>> defines a user as:
> >>>
> >>> foobar::1234::::
> >>>
> >>> and the second passwd file defines it as:
> >>>
> >>> foobar:::nogroup:The foobar user:/:/bin/sh
> >>>
> >>> then a foobar group would be created even if the user will use the
> >>> nogroup as its primary group.
> >>
> >> One minor thing
> >>
> >>> @@ -251,7 +269,7 @@ def update_useradd_static_config(d):
> >>>
> >>> newparams.append(newparam)
> >>>
> >>> - return " ;".join(newparams).strip()
> >>> + return ";".join(newparams).strip()
> >>>
> >>> # Load and process the users and groups, rewriting the adduser/addgroup params
> >>> useradd_packages = d.getVar('USERADD_PACKAGES', True)
> >>>
> >>
> >> The space was required because you could generate a user/group add
> >> line that ended with a string. Without the space, you could end up
> >> merging two sets of arguments causing a failure condition.
> >>
> >> So I think that it should be retained unless there is a specific
> >> reason you believe it should be removed.
> >
> > I cannot see how that space can make any difference. Each set of
> > useradd/grouppadd options added to newparams has the user/group
> > name at the end of the string. And if that somehow interferes with
> > the semicolon, then the code in useradd.bbclass which simply does
> > "cut -d ';'" to split the useradd/groupadd line would break already.
>
> The contents when originally parsed my be run as arguments to a shell
> script or as parameters to these functions.
>
> In the shell script world not have a space can confuse the argument
> parsing into thinking the ; is part of the argument.
No shell I have heard of (bash, zsh and dash comes to mind) would be
affected by the lack of a space before the semicolon. Moreover, this
is never actually parsed by a shell (except as part of a variable value).
The semicolon is used by useradd.bbclass to split the variables, after
which it lets the shell evaluate the part before the semicolon (which
will ignore any trailing whitespace).
> You don't have that in the python world with the split behavior.
>
> > Actually, now that I think about it, I do wonder why
> > useradd-staticids.bbclass use this advanced variant to split the
> > useradd/groupadd lines:
> >
> > for param in re.split('''[ \t]*;[ \t]*(?=(?:[^'"]|'[^']*'|"[^"]*")*$)''', params):
>
> It is perfectly legal to allow a ';' in the middle of a parameter (that
> allows it), a parameter that is quoted.
Sure, and the code above handles some cases, but definitely not all.
E.g., this would not be parsed as intended by useradd-staticids.bbclass:
USERADD_PARAMS_${PN} += "-c Comment\ with\ an\ \'\ in\ it oddcomment; \
-c Other\ odd \'\ comment otheruser"
but it would be handled correctly by useradd.bbclass...
> Something like:
>
> adduser -c "This user;that user;all users" -d /home/allusers alluser
>
> it's odd, but I've certainly seen people put ';' in the comment
> before.. and it is legal in other palces, like the home dir and
> such -- just not advised.
It may be legal, but it has never been supported by the adduser.bbclass.
And thus it has never worked in practice to take an existing passwd
file that contains a semicolon in the comment field and expect it to
work as input to adduser-staticids.bbclass.
Moreover, neither adduser.bbclass nor adduser-staticids.bbclass will
handle a semicolon correctly in any other field. And let's not get
started on other special characters like ", \, $ and > which could
wreak havoc on both classes in a correctly crafted passwd file.
> > when this would do the job just as well:
> >
> > for param in params.split(';'):
> >
> > given that that is what useradd.bbclass does. It looks as if tries
> > to support something like --comment "something with a ; in it", but
> > using that would break in useradd.bbclass anyway...
>
> Then the useradd class is broken in this case. The --comment
> processing needs to work, it's just rarely used in the normal
> case, but very much used in the "lets take a previously generated
> passwd file and reuse it" case of the adduser-static.
>
> > //Peter
So, from reading the code in both adduser.bbclass and
adduser-staticids.bbclass, having spaces or no spaces before the
semicolon in the USERADD_PARAM_${PN} variable cannot make any
difference to how the users are created in the end. Thus it should
be safe to remove them.
//Peter
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 5/5] useradd-staticids.bbclass: Read passwd/group files before parsing
2015-11-10 15:54 ` Peter Kjellerstedt
@ 2015-11-10 16:07 ` Mark Hatle
2015-11-13 12:51 ` Peter Kjellerstedt
0 siblings, 1 reply; 14+ messages in thread
From: Mark Hatle @ 2015-11-10 16:07 UTC (permalink / raw)
To: Peter Kjellerstedt; +Cc: openembedded-core@lists.openembedded.org
On 11/10/15 9:54 AM, Peter Kjellerstedt wrote:
>> -----Original Message-----
>> From: Mark Hatle [mailto:mark.hatle@windriver.com]
>> Sent: den 6 november 2015 21:14
>> To: Peter Kjellerstedt
>> Cc: openembedded-core@lists.openembedded.org
>> Subject: Re: [PATCH 5/5] useradd-staticids.bbclass: Read passwd/group
>> files before parsing
>>
>> On 11/6/15 2:09 PM, Peter Kjellerstedt wrote:
>>>> -----Original Message-----
>>>> From: Mark Hatle [mailto:mark.hatle@windriver.com]
>>>> Sent: den 4 november 2015 01:33
>>>> To: Peter Kjellerstedt; openembedded-core@lists.openembedded.org
>>>> Subject: Re: [PATCH 5/5] useradd-staticids.bbclass: Read
>>>> passwd/group files before parsing
>>>>
>>>> On 11/3/15 6:06 PM, Peter Kjellerstedt wrote:
>>>>> Read and merge the passwd/group files before parsing the user and
>>>>> group definitions. This means they will only be read once per
>>>>> recipe. This solves a problem where if a user was definied in
>>>>> multiple files, it could generate group definitions for groups
>>>>> that should not be created. E.g., if the first passwd file read
>>>>> defines a user as:
>>>>>
>>>>> foobar::1234::::
>>>>>
>>>>> and the second passwd file defines it as:
>>>>>
>>>>> foobar:::nogroup:The foobar user:/:/bin/sh
>>>>>
>>>>> then a foobar group would be created even if the user will use the
>>>>> nogroup as its primary group.
>>>>
>>>> One minor thing
>>>>
>>>>> @@ -251,7 +269,7 @@ def update_useradd_static_config(d):
>>>>>
>>>>> newparams.append(newparam)
>>>>>
>>>>> - return " ;".join(newparams).strip()
>>>>> + return ";".join(newparams).strip()
>>>>>
>>>>> # Load and process the users and groups, rewriting the adduser/addgroup params
>>>>> useradd_packages = d.getVar('USERADD_PACKAGES', True)
>>>>>
>>>>
>>>> The space was required because you could generate a user/group add
>>>> line that ended with a string. Without the space, you could end up
>>>> merging two sets of arguments causing a failure condition.
>>>>
>>>> So I think that it should be retained unless there is a specific
>>>> reason you believe it should be removed.
>>>
>>> I cannot see how that space can make any difference. Each set of
>>> useradd/grouppadd options added to newparams has the user/group
>>> name at the end of the string. And if that somehow interferes with
>>> the semicolon, then the code in useradd.bbclass which simply does
>>> "cut -d ';'" to split the useradd/groupadd line would break already.
>>
>> The contents when originally parsed my be run as arguments to a shell
>> script or as parameters to these functions.
>>
>> In the shell script world not have a space can confuse the argument
>> parsing into thinking the ; is part of the argument.
>
> No shell I have heard of (bash, zsh and dash comes to mind) would be
> affected by the lack of a space before the semicolon. Moreover, this
> is never actually parsed by a shell (except as part of a variable value).
> The semicolon is used by useradd.bbclass to split the variables, after
> which it lets the shell evaluate the part before the semicolon (which
> will ignore any trailing whitespace).
I've seen broken shells in the past where you would do something like:
/bin/echo foo;/bin/echo bar
and get: "/bin echo foo;/bin/echo bar" since it treated the middle item as a
single command. I'm not saying it wasn't a bug in the shell or system -- just
that I've been burned by it in the past and because of this, I try not to rely
on it.. (when adding a space solves the issue.)
>> You don't have that in the python world with the split behavior.
>>
>>> Actually, now that I think about it, I do wonder why
>>> useradd-staticids.bbclass use this advanced variant to split the
>>> useradd/groupadd lines:
>>>
>>> for param in re.split('''[ \t]*;[ \t]*(?=(?:[^'"]|'[^']*'|"[^"]*")*$)''', params):
>>
>> It is perfectly legal to allow a ';' in the middle of a parameter (that
>> allows it), a parameter that is quoted.
>
> Sure, and the code above handles some cases, but definitely not all.
> E.g., this would not be parsed as intended by useradd-staticids.bbclass:
>
> USERADD_PARAMS_${PN} += "-c Comment\ with\ an\ \'\ in\ it oddcomment; \
> -c Other\ odd \'\ comment otheruser"
>
> but it would be handled correctly by useradd.bbclass...
In this case, we need to emulate a reasonable set of argument processing. If
there is something built into bitbake/oe then we can use it. The re.split
though was a good approximation of the common configurations I was seeing at the
time. (Specifically quotes parameters for spaces and ;)
>> Something like:
>>
>> adduser -c "This user;that user;all users" -d /home/allusers alluser
>>
>> it's odd, but I've certainly seen people put ';' in the comment
>> before.. and it is legal in other palces, like the home dir and
>> such -- just not advised.
>
> It may be legal, but it has never been supported by the adduser.bbclass.
> And thus it has never worked in practice to take an existing passwd
> file that contains a semicolon in the comment field and expect it to
> work as input to adduser-staticids.bbclass.
Then that is a bug we should fix. At one time this was working (perhaps not in
OE, but locally?) Since I did have a customer who had both spaces and ';' in
their comment field. (This is how I originally found the problem and needed to
figure out a regex that worked in more cases.)
> Moreover, neither adduser.bbclass nor adduser-staticids.bbclass will
> handle a semicolon correctly in any other field. And let's not get
> started on other special characters like ", \, $ and > which could
> wreak havoc on both classes in a correctly crafted passwd file.
I realize there are may corner cases here. We need to try to support the
reasonable ones and prevent the others. If that list should be avoided in
certain cases -- then we should enhance the system catch things "not to do".
But using ';' in the comment is fairly common in some environments.
>>> when this would do the job just as well:
>>>
>>> for param in params.split(';'):
>>>
>>> given that that is what useradd.bbclass does. It looks as if tries
>>> to support something like --comment "something with a ; in it", but
>>> using that would break in useradd.bbclass anyway...
>>
>> Then the useradd class is broken in this case. The --comment
>> processing needs to work, it's just rarely used in the normal
>> case, but very much used in the "lets take a previously generated
>> passwd file and reuse it" case of the adduser-static.
>>
>>> //Peter
>
> So, from reading the code in both adduser.bbclass and
> adduser-staticids.bbclass, having spaces or no spaces before the
> semicolon in the USERADD_PARAM_${PN} variable cannot make any
> difference to how the users are created in the end. Thus it should
> be safe to remove them.
The (initial) generated adduser/addgroup/etc command is dumped into the pre/post
install script section of the packages. So when the package manager runs it
needs to execute the shell script. This is where I've seen the problem of the
';' in the past...
Adduser and adduser-static should be synced to use the same delimitation
mechanism. And for things like a ; in the comment, both should equally support
it. We've got a mismatch and that is definitely a bug.
--Mark
> //Peter
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 5/5] useradd-staticids.bbclass: Read passwd/group files before parsing
2015-11-10 16:07 ` Mark Hatle
@ 2015-11-13 12:51 ` Peter Kjellerstedt
2015-11-24 9:36 ` Peter Kjellerstedt
0 siblings, 1 reply; 14+ messages in thread
From: Peter Kjellerstedt @ 2015-11-13 12:51 UTC (permalink / raw)
To: Mark Hatle; +Cc: openembedded-core@lists.openembedded.org
> -----Original Message-----
> From: openembedded-core-bounces@lists.openembedded.org
> [mailto:openembedded-core-bounces@lists.openembedded.org] On Behalf Of
> Mark Hatle
> Sent: den 10 november 2015 17:07
> To: Peter Kjellerstedt
> Cc: openembedded-core@lists.openembedded.org
> Subject: Re: [OE-core] [PATCH 5/5] useradd-staticids.bbclass: Read
> passwd/group files before parsing
>
> On 11/10/15 9:54 AM, Peter Kjellerstedt wrote:
> >> -----Original Message-----
> >> From: Mark Hatle [mailto:mark.hatle@windriver.com]
> >> Sent: den 6 november 2015 21:14
> >> To: Peter Kjellerstedt
> >> Cc: openembedded-core@lists.openembedded.org
> >> Subject: Re: [PATCH 5/5] useradd-staticids.bbclass: Read
> >> passwd/group files before parsing
> >>
> >> On 11/6/15 2:09 PM, Peter Kjellerstedt wrote:
> >>>> -----Original Message-----
> >>>> From: Mark Hatle [mailto:mark.hatle@windriver.com]
> >>>> Sent: den 4 november 2015 01:33
> >>>> To: Peter Kjellerstedt; openembedded-core@lists.openembedded.org
> >>>> Subject: Re: [PATCH 5/5] useradd-staticids.bbclass: Read
> >>>> passwd/group files before parsing
> >>>>
> >>>> On 11/3/15 6:06 PM, Peter Kjellerstedt wrote:
> >>>>> Read and merge the passwd/group files before parsing the user and
> >>>>> group definitions. This means they will only be read once per
> >>>>> recipe. This solves a problem where if a user was definied in
> >>>>> multiple files, it could generate group definitions for groups
> >>>>> that should not be created. E.g., if the first passwd file read
> >>>>> defines a user as:
> >>>>>
> >>>>> foobar::1234::::
> >>>>>
> >>>>> and the second passwd file defines it as:
> >>>>>
> >>>>> foobar:::nogroup:The foobar user:/:/bin/sh
> >>>>>
> >>>>> then a foobar group would be created even if the user will use
> >>>>> the nogroup as its primary group.
> >>>>
> >>>> One minor thing
> >>>>
> >>>>> @@ -251,7 +269,7 @@ def update_useradd_static_config(d):
> >>>>>
> >>>>> newparams.append(newparam)
> >>>>>
> >>>>> - return " ;".join(newparams).strip()
> >>>>> + return ";".join(newparams).strip()
> >>>>>
> >>>>> # Load and process the users and groups, rewriting the adduser/addgroup params
> >>>>> useradd_packages = d.getVar('USERADD_PACKAGES', True)
> >>>>>
> >>>>
> >>>> The space was required because you could generate a user/group
> >>>> add line that ended with a string. Without the space, you could
> >>>> end up merging two sets of arguments causing a failure condition.
> >>>>
> >>>> So I think that it should be retained unless there is a specific
> >>>> reason you believe it should be removed.
> >>>
> >>> I cannot see how that space can make any difference. Each set of
> >>> useradd/grouppadd options added to newparams has the user/group
> >>> name at the end of the string. And if that somehow interferes with
> >>> the semicolon, then the code in useradd.bbclass which simply does
> >>> "cut -d ';'" to split the useradd/groupadd line would break
> >>> already.
> >>
> >> The contents when originally parsed my be run as arguments to a
> >> shell script or as parameters to these functions.
> >>
> >> In the shell script world not have a space can confuse the argument
> >> parsing into thinking the ; is part of the argument.
> >
> > No shell I have heard of (bash, zsh and dash comes to mind) would be
> > affected by the lack of a space before the semicolon. Moreover, this
> > is never actually parsed by a shell (except as part of a variable
> > value). The semicolon is used by useradd.bbclass to split the
> > variables, after which it lets the shell evaluate the part before
> > the semicolon (which will ignore any trailing whitespace).
>
> I've seen broken shells in the past where you would do something like:
>
> /bin/echo foo;/bin/echo bar
>
> and get: "/bin echo foo;/bin/echo bar" since it treated the middle
> item as a single command. I'm not saying it wasn't a bug in the shell
> or system -- just that I've been burned by it in the past and because
> of this, I try not to rely on it.. (when adding a space solves the issue.)
Ok, sounds weird, but I will take your word for it.
> >> You don't have that in the python world with the split behavior.
> >>
> >>> Actually, now that I think about it, I do wonder why
> >>> useradd-staticids.bbclass use this advanced variant to split the
> >>> useradd/groupadd lines:
> >>>
> >>> for param in re.split('''[ \t]*;[ \t]*(?=(?:[^'"]|'[^']*'|"[^"]*")*$)''', params):
> >>
> >> It is perfectly legal to allow a ';' in the middle of a parameter
> >> (that allows it), a parameter that is quoted.
> >
> > Sure, and the code above handles some cases, but definitely not all.
> > E.g., this would not be parsed as intended by useradd-
> > staticids.bbclass:
> >
> > USERADD_PARAMS_${PN} += "-c Comment\ with\ an\ \'\ in\ it oddcomment; \
> > -c Other\ odd \'\ comment otheruser"
> >
> > but it would be handled correctly by useradd.bbclass...
>
> In this case, we need to emulate a reasonable set of argument
> processing. If there is something built into bitbake/oe then we can
> use it. The re.split though was a good approximation of the common
> configurations I was seeing at the time. (Specifically quotes
> parameters for spaces and ;)
Normally I would suggest shlex.split(). It is already used by both
BitBake and OE-Core. Unfortunately, it cannot be used in this case
as it cannot distinguish between an escaped/quoted semicolon and
one that is not escaped/quoted. This is because it is designed to
split one command line, not multiple lines.
So the split of multiple commands would probably still have to be
done using a regular expression, extended to support escaping.
shlex.split() can be used for the second split though (where the
command is fed to the parser).
We would also need the opposite to construct a command line with
all options properly quoted. Unfortunately, shlex.quote() was not
added until Python 3.3. However, there is an unofficial pipes.quote()
that is already used by OE-Core, so I assume it is ok to use.
> >> Something like:
> >>
> >> adduser -c "This user;that user;all users" -d /home/allusers alluser
> >>
> >> it's odd, but I've certainly seen people put ';' in the comment
> >> before.. and it is legal in other palces, like the home dir and
> >> such -- just not advised.
> >
> > It may be legal, but it has never been supported by the
> > adduser.bbclass. And thus it has never worked in practice to take an
> > existing passwd file that contains a semicolon in the comment field
> > and expect it to work as input to adduser-staticids.bbclass.
>
> Then that is a bug we should fix. At one time this was working
> (perhaps not in OE, but locally?) Since I did have a customer who
> had both spaces and ';' in their comment field. (This is how I
> originally found the problem and needed to figure out a regex that
> worked in more cases.)
>
> > Moreover, neither adduser.bbclass nor adduser-staticids.bbclass will
> > handle a semicolon correctly in any other field. And let's not get
> > started on other special characters like ", \, $ and > which could
> > wreak havoc on both classes in a correctly crafted passwd file.
>
> I realize there are may corner cases here. We need to try to support
> the reasonable ones and prevent the others. If that list should be
> avoided in certain cases -- then we should enhance the system catch
> things "not to do". But using ';' in the comment is fairly common in
> some environments.
>
> >>> when this would do the job just as well:
> >>>
> >>> for param in params.split(';'):
> >>>
> >>> given that that is what useradd.bbclass does. It looks as if tries
> >>> to support something like --comment "something with a ; in it", but
> >>> using that would break in useradd.bbclass anyway...
> >>
> >> Then the useradd class is broken in this case. The --comment
> >> processing needs to work, it's just rarely used in the normal
> >> case, but very much used in the "lets take a previously generated
> >> passwd file and reuse it" case of the adduser-static.
> >>
> >>> //Peter
> >
> > So, from reading the code in both adduser.bbclass and
> > adduser-staticids.bbclass, having spaces or no spaces before the
> > semicolon in the USERADD_PARAM_${PN} variable cannot make any
> > difference to how the users are created in the end. Thus it should
> > be safe to remove them.
>
> The (initial) generated adduser/addgroup/etc command is dumped into the
> pre/post install script section of the packages. So when the package
> manager runs it needs to execute the shell script. This is where I've
> seen the problem of the ';' in the past...
>
> Adduser and adduser-static should be synced to use the same delimitation
> mechanism. And for things like a ; in the comment, both should equally
> support it. We've got a mismatch and that is definitely a bug.
I agree that the correct thing to do is to make them support the same
mechanism. However, this would require quite some more work, especially
with the useradd.bbclass, and as it does not affect the changes in my
patches, do you think the patches can be accepted as they are, and we
can work on improving the parsing afterwards?
> --Mark
>
> > //Peter
//Peter
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 5/5] useradd-staticids.bbclass: Read passwd/group files before parsing
2015-11-13 12:51 ` Peter Kjellerstedt
@ 2015-11-24 9:36 ` Peter Kjellerstedt
0 siblings, 0 replies; 14+ messages in thread
From: Peter Kjellerstedt @ 2015-11-24 9:36 UTC (permalink / raw)
To: Mark Hatle; +Cc: openembedded-core@lists.openembedded.org
*ping*
//Peter
> -----Original Message-----
> From: openembedded-core-bounces@lists.openembedded.org
> [mailto:openembedded-core-bounces@lists.openembedded.org] On Behalf Of
> Peter Kjellerstedt
> Sent: den 13 november 2015 13:52
> To: Mark Hatle
> Cc: openembedded-core@lists.openembedded.org
> Subject: Re: [OE-core] [PATCH 5/5] useradd-staticids.bbclass: Read
> passwd/group files before parsing
>
> > -----Original Message-----
> > From: openembedded-core-bounces@lists.openembedded.org
> > [mailto:openembedded-core-bounces@lists.openembedded.org] On Behalf
> Of
> > Mark Hatle
> > Sent: den 10 november 2015 17:07
> > To: Peter Kjellerstedt
> > Cc: openembedded-core@lists.openembedded.org
> > Subject: Re: [OE-core] [PATCH 5/5] useradd-staticids.bbclass: Read
> > passwd/group files before parsing
> >
> > On 11/10/15 9:54 AM, Peter Kjellerstedt wrote:
> > >> -----Original Message-----
> > >> From: Mark Hatle [mailto:mark.hatle@windriver.com]
> > >> Sent: den 6 november 2015 21:14
> > >> To: Peter Kjellerstedt
> > >> Cc: openembedded-core@lists.openembedded.org
> > >> Subject: Re: [PATCH 5/5] useradd-staticids.bbclass: Read
> > >> passwd/group files before parsing
> > >>
> > >> On 11/6/15 2:09 PM, Peter Kjellerstedt wrote:
> > >>>> -----Original Message-----
> > >>>> From: Mark Hatle [mailto:mark.hatle@windriver.com]
> > >>>> Sent: den 4 november 2015 01:33
> > >>>> To: Peter Kjellerstedt; openembedded-core@lists.openembedded.org
> > >>>> Subject: Re: [PATCH 5/5] useradd-staticids.bbclass: Read
> > >>>> passwd/group files before parsing
> > >>>>
> > >>>> On 11/3/15 6:06 PM, Peter Kjellerstedt wrote:
> > >>>>> Read and merge the passwd/group files before parsing the user
> and
> > >>>>> group definitions. This means they will only be read once per
> > >>>>> recipe. This solves a problem where if a user was definied in
> > >>>>> multiple files, it could generate group definitions for groups
> > >>>>> that should not be created. E.g., if the first passwd file read
> > >>>>> defines a user as:
> > >>>>>
> > >>>>> foobar::1234::::
> > >>>>>
> > >>>>> and the second passwd file defines it as:
> > >>>>>
> > >>>>> foobar:::nogroup:The foobar user:/:/bin/sh
> > >>>>>
> > >>>>> then a foobar group would be created even if the user will use
> > >>>>> the nogroup as its primary group.
> > >>>>
> > >>>> One minor thing
> > >>>>
> > >>>>> @@ -251,7 +269,7 @@ def update_useradd_static_config(d):
> > >>>>>
> > >>>>> newparams.append(newparam)
> > >>>>>
> > >>>>> - return " ;".join(newparams).strip()
> > >>>>> + return ";".join(newparams).strip()
> > >>>>>
> > >>>>> # Load and process the users and groups, rewriting the
> adduser/addgroup params
> > >>>>> useradd_packages = d.getVar('USERADD_PACKAGES', True)
> > >>>>>
> > >>>>
> > >>>> The space was required because you could generate a user/group
> > >>>> add line that ended with a string. Without the space, you could
> > >>>> end up merging two sets of arguments causing a failure
> condition.
> > >>>>
> > >>>> So I think that it should be retained unless there is a specific
> > >>>> reason you believe it should be removed.
> > >>>
> > >>> I cannot see how that space can make any difference. Each set of
> > >>> useradd/grouppadd options added to newparams has the user/group
> > >>> name at the end of the string. And if that somehow interferes
> with
> > >>> the semicolon, then the code in useradd.bbclass which simply does
> > >>> "cut -d ';'" to split the useradd/groupadd line would break
> > >>> already.
> > >>
> > >> The contents when originally parsed my be run as arguments to a
> > >> shell script or as parameters to these functions.
> > >>
> > >> In the shell script world not have a space can confuse the
> argument
> > >> parsing into thinking the ; is part of the argument.
> > >
> > > No shell I have heard of (bash, zsh and dash comes to mind) would
> be
> > > affected by the lack of a space before the semicolon. Moreover,
> this
> > > is never actually parsed by a shell (except as part of a variable
> > > value). The semicolon is used by useradd.bbclass to split the
> > > variables, after which it lets the shell evaluate the part before
> > > the semicolon (which will ignore any trailing whitespace).
> >
> > I've seen broken shells in the past where you would do something
> like:
> >
> > /bin/echo foo;/bin/echo bar
> >
> > and get: "/bin echo foo;/bin/echo bar" since it treated the middle
> > item as a single command. I'm not saying it wasn't a bug in the
> shell
> > or system -- just that I've been burned by it in the past and because
> > of this, I try not to rely on it.. (when adding a space solves the
> issue.)
>
> Ok, sounds weird, but I will take your word for it.
>
> > >> You don't have that in the python world with the split behavior.
> > >>
> > >>> Actually, now that I think about it, I do wonder why
> > >>> useradd-staticids.bbclass use this advanced variant to split the
> > >>> useradd/groupadd lines:
> > >>>
> > >>> for param in re.split('''[ \t]*;[
> \t]*(?=(?:[^'"]|'[^']*'|"[^"]*")*$)''', params):
> > >>
> > >> It is perfectly legal to allow a ';' in the middle of a parameter
> > >> (that allows it), a parameter that is quoted.
> > >
> > > Sure, and the code above handles some cases, but definitely not
> all.
> > > E.g., this would not be parsed as intended by useradd-
> > > staticids.bbclass:
> > >
> > > USERADD_PARAMS_${PN} += "-c Comment\ with\ an\ \'\ in\ it
> oddcomment; \
> > > -c Other\ odd \'\ comment otheruser"
> > >
> > > but it would be handled correctly by useradd.bbclass...
> >
> > In this case, we need to emulate a reasonable set of argument
> > processing. If there is something built into bitbake/oe then we can
> > use it. The re.split though was a good approximation of the common
> > configurations I was seeing at the time. (Specifically quotes
> > parameters for spaces and ;)
>
> Normally I would suggest shlex.split(). It is already used by both
> BitBake and OE-Core. Unfortunately, it cannot be used in this case
> as it cannot distinguish between an escaped/quoted semicolon and
> one that is not escaped/quoted. This is because it is designed to
> split one command line, not multiple lines.
>
> So the split of multiple commands would probably still have to be
> done using a regular expression, extended to support escaping.
>
> shlex.split() can be used for the second split though (where the
> command is fed to the parser).
>
> We would also need the opposite to construct a command line with
> all options properly quoted. Unfortunately, shlex.quote() was not
> added until Python 3.3. However, there is an unofficial pipes.quote()
> that is already used by OE-Core, so I assume it is ok to use.
>
> > >> Something like:
> > >>
> > >> adduser -c "This user;that user;all users" -d /home/allusers
> alluser
> > >>
> > >> it's odd, but I've certainly seen people put ';' in the comment
> > >> before.. and it is legal in other palces, like the home dir and
> > >> such -- just not advised.
> > >
> > > It may be legal, but it has never been supported by the
> > > adduser.bbclass. And thus it has never worked in practice to take
> an
> > > existing passwd file that contains a semicolon in the comment field
> > > and expect it to work as input to adduser-staticids.bbclass.
> >
> > Then that is a bug we should fix. At one time this was working
> > (perhaps not in OE, but locally?) Since I did have a customer who
> > had both spaces and ';' in their comment field. (This is how I
> > originally found the problem and needed to figure out a regex that
> > worked in more cases.)
> >
> > > Moreover, neither adduser.bbclass nor adduser-staticids.bbclass
> will
> > > handle a semicolon correctly in any other field. And let's not get
> > > started on other special characters like ", \, $ and > which could
> > > wreak havoc on both classes in a correctly crafted passwd file.
> >
> > I realize there are may corner cases here. We need to try to support
> > the reasonable ones and prevent the others. If that list should be
> > avoided in certain cases -- then we should enhance the system catch
> > things "not to do". But using ';' in the comment is fairly common in
> > some environments.
> >
> > >>> when this would do the job just as well:
> > >>>
> > >>> for param in params.split(';'):
> > >>>
> > >>> given that that is what useradd.bbclass does. It looks as if
> tries
> > >>> to support something like --comment "something with a ; in it",
> but
> > >>> using that would break in useradd.bbclass anyway...
> > >>
> > >> Then the useradd class is broken in this case. The --comment
> > >> processing needs to work, it's just rarely used in the normal
> > >> case, but very much used in the "lets take a previously generated
> > >> passwd file and reuse it" case of the adduser-static.
> > >>
> > >>> //Peter
> > >
> > > So, from reading the code in both adduser.bbclass and
> > > adduser-staticids.bbclass, having spaces or no spaces before the
> > > semicolon in the USERADD_PARAM_${PN} variable cannot make any
> > > difference to how the users are created in the end. Thus it should
> > > be safe to remove them.
> >
> > The (initial) generated adduser/addgroup/etc command is dumped into
> the
> > pre/post install script section of the packages. So when the package
> > manager runs it needs to execute the shell script. This is where
> I've
> > seen the problem of the ';' in the past...
> >
> > Adduser and adduser-static should be synced to use the same
> delimitation
> > mechanism. And for things like a ; in the comment, both should
> equally
> > support it. We've got a mismatch and that is definitely a bug.
>
> I agree that the correct thing to do is to make them support the same
> mechanism. However, this would require quite some more work, especially
> with the useradd.bbclass, and as it does not affect the changes in my
> patches, do you think the patches can be accepted as they are, and we
> can work on improving the parsing afterwards?
>
> > --Mark
> >
> > > //Peter
>
> //Peter
>
> --
> _______________________________________________
> Openembedded-core mailing list
> Openembedded-core@lists.openembedded.org
> http://lists.openembedded.org/mailman/listinfo/openembedded-core
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2015-11-24 9:36 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-11-04 0:06 [PATCH 0/5] Improvements for useradd-staticids.bbclass Peter Kjellerstedt
2015-11-04 0:06 ` [PATCH 1/5] useradd-staticids.bbclass: Treat mutually exclusive options as such Peter Kjellerstedt
2015-11-04 0:06 ` [PATCH 2/5] useradd-staticids.bbclass: Make --no-user-group have effect Peter Kjellerstedt
2015-11-04 0:06 ` [PATCH 3/5] useradd-staticids.bbclass: Simplify some logic Peter Kjellerstedt
2015-11-04 0:06 ` [PATCH 4/5] useradd-staticids.bbclass: Simplify the logic for when to add groups Peter Kjellerstedt
2015-11-04 0:06 ` [PATCH 5/5] useradd-staticids.bbclass: Read passwd/group files before parsing Peter Kjellerstedt
2015-11-04 0:32 ` Mark Hatle
2015-11-06 20:09 ` Peter Kjellerstedt
2015-11-06 20:14 ` Mark Hatle
2015-11-10 15:54 ` Peter Kjellerstedt
2015-11-10 16:07 ` Mark Hatle
2015-11-13 12:51 ` Peter Kjellerstedt
2015-11-24 9:36 ` Peter Kjellerstedt
2015-11-04 0:33 ` [PATCH 0/5] Improvements for useradd-staticids.bbclass Mark Hatle
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox