[PATCH 0/3] pseudo+image.bbclass: changes to avoid host contamination

[PATCH 0/3] pseudo+image.bbclass: changes to avoid host contamination

[Peter A. Bigot]

While determining that an anomaly was self-induced, I found some issues
with pseudo that, with low probability, could result in mis-use of the
build host /etc/passwd and /etc/group to resolve target uid/gid/names.

The red herring I was fishing was that pseudo, in its default
configuration, will fall back to the build host passwd/group files if it
can't access ones in the chroot or specified by PSEUDO_PASSWD.  To rule
out this as a cause of my anomaly, I added --without-passwd-fallback to
the pseudo configuration.

This unexpectedly resulted in failed builds that I tracked down to
pseudo adding an unnecessary directory prefix to the .pwd.lck file,
causing failures in the attempt to lock /etc/passwd.  The first patch
fixes pseudo to support --without-passwd-fallback.

The next problem is that pseudo required the fallback path to be
specified when pseudo itself was configured, and only allowed a single
runtime-specified path.  This breaks image formation: the preferred path
should be ${IMAGE_ROOT}, but etc/passwd doesn't exist in that path until
base-passwd runs pkg_postinst.  Until that happens the version in
${STAGING_DIR_TARGET} should be used as fallback.  The second patch
enhances pseudo with the ability to specify multiple search paths, and
the third uses the feature in image.bbclass to search both ${IMAGE_ROOT}
and ${STAGING_DIR_TARGET} for passwd/group files.

I believe OE should add --without-passwd-fallback to the pseudo 1.6.2
configuration flags early in the 1.8 development cycle, to ensure there
are no host contamination issues.  I can think of no reason why the
build host passwd and group files should ever be considered suitable for
use in determining target user/group characteristics.

However, if this is done various recipes that do things like "chown
root:root files" in their install fail because they don't currently
DEPEND on base-passwd.  How to cleanly add that dependency is a topic
for discussion, and I've left that final step out of the series for now.

Peter

Peter A. Bigot (3):
  pseudo: support --without-passwd-fallback configuration option
  pseudo: support multiple search directories in PSEUDO_PASSWD
  image.bbclass: search both rootfs and staging dir for passwd files

 meta/classes/image.bbclass                         |   4 +-
 ...do_client.c-protect-pwd_lck-against-magic.patch |  56 ++++++++++
 ..._util-modify-interface-to-pseudo_etc_file.patch |  70 +++++++++++++
 ...nt.c-support-multiple-directories-in-PSEU.patch | 115 +++++++++++++++++++++
 meta/recipes-devtools/pseudo/pseudo_1.6.2.bb       |   3 +
 5 files changed, 247 insertions(+), 1 deletion(-)
 create mode 100644 meta/recipes-devtools/pseudo/pseudo-1.6.2/0001-pseudo_client.c-protect-pwd_lck-against-magic.patch
 create mode 100644 meta/recipes-devtools/pseudo/pseudo-1.6.2/0002-pseudo_util-modify-interface-to-pseudo_etc_file.patch
 create mode 100644 meta/recipes-devtools/pseudo/pseudo-1.6.2/0003-pseudo_client.c-support-multiple-directories-in-PSEU.patch

-- 
1.8.5.5

[PATCH 1/3] pseudo: support --without-passwd-fallback configuration option

[Peter A. Bigot]

A bug in pseudo 1.6.2 results in lock failures if this option is
present.

Signed-off-by: Peter A. Bigot <pab@pabigot.com>
---
Cc: peter.seebach@windriver.com

 ...do_client.c-protect-pwd_lck-against-magic.patch | 56 ++++++++++++++++++++++
 meta/recipes-devtools/pseudo/pseudo_1.6.2.bb       |  1 +
 2 files changed, 57 insertions(+)
 create mode 100644 meta/recipes-devtools/pseudo/pseudo-1.6.2/0001-pseudo_client.c-protect-pwd_lck-against-magic.patch

diff --git a/meta/recipes-devtools/pseudo/pseudo-1.6.2/0001-pseudo_client.c-protect-pwd_lck-against-magic.patch b/meta/recipes-devtools/pseudo/pseudo-1.6.2/0001-pseudo_client.c-protect-pwd_lck-against-magic.patch
new file mode 100644
index 0000000..d0c0a27
--- /dev/null
+++ b/meta/recipes-devtools/pseudo/pseudo-1.6.2/0001-pseudo_client.c-protect-pwd_lck-against-magic.patch
@@ -0,0 +1,56 @@
+From e11468a47369596f57c5e99bd0a3dd58b2c6d5e0 Mon Sep 17 00:00:00 2001
+From: "Peter A. Bigot" <pab@pabigot.com>
+Date: Sun, 12 Oct 2014 08:27:14 -0500
+Subject: [PATCH 1/3] pseudo_client.c: protect pwd_lck against magic
+
+While attempting to diagnose unexpected uid/gid assignment I added
+--without-passwd-fallback to the pseudo build.  This caused build
+failures due to inability to lock /etc/passwd.
+
+Instrumentation revealed that attempts to create the lock file ended up
+with pseudo_etc_file() creating the correct lock name, but the
+subsequent open had an extra PSEUDO_PASSWD directory prefix causing
+it to fail.
+
+Inspection of pseudo_client shows the only other use of PSEUDO_ETC_FILE
+to be protected against magic.  Applying the same shield to the
+unprotected calls in pseudo_pwd_lck_{open,close} fixes the issue.
+
+Upstream-Status: Pending
+Signed-off-by: Peter A. Bigot <pab@pabigot.com>
+---
+ pseudo_client.c | 4 ++++
+ 1 file changed, 4 insertions(+)
+
+diff --git a/pseudo_client.c b/pseudo_client.c
+index 8deaa1b..442dd19 100644
+--- a/pseudo_client.c
++++ b/pseudo_client.c
+@@ -416,20 +416,24 @@ pseudo_pwd_lck_open(void) {
+ 		}
+ 	}
+ 	pseudo_pwd_lck_close();
++	pseudo_antimagic();
+ 	pseudo_pwd_lck_fd = PSEUDO_ETC_FILE(".pwd.lock",
+ 					pseudo_pwd_lck_name, O_RDWR | O_CREAT);
++	pseudo_magic();
+ 	return pseudo_pwd_lck_fd;
+ }
+ 
+ int
+ pseudo_pwd_lck_close(void) {
+ 	if (pseudo_pwd_lck_fd != -1) {
++		pseudo_antimagic();
+ 		close(pseudo_pwd_lck_fd);
+ 		if (pseudo_pwd_lck_name) {
+ 			unlink(pseudo_pwd_lck_name);
+ 			free(pseudo_pwd_lck_name);
+ 			pseudo_pwd_lck_name = 0;
+ 		}
++		pseudo_magic();
+ 		pseudo_pwd_lck_fd = -1;
+ 		return 0;
+ 	} else {
+-- 
+1.8.5.5
+
diff --git a/meta/recipes-devtools/pseudo/pseudo_1.6.2.bb b/meta/recipes-devtools/pseudo/pseudo_1.6.2.bb
index ece50bf..df8ce83 100644
--- a/meta/recipes-devtools/pseudo/pseudo_1.6.2.bb
+++ b/meta/recipes-devtools/pseudo/pseudo_1.6.2.bb
@@ -2,6 +2,7 @@ require pseudo.inc
 
 SRC_URI = " \
     http://www.yoctoproject.org/downloads/${BPN}/${BPN}-${PV}.tar.bz2 \
+    file://0001-pseudo_client.c-protect-pwd_lck-against-magic.patch \
 "
 
 SRC_URI[md5sum] = "4d7b4f9d1b4aafa680ce94a5a9a52f1f"
-- 
1.8.5.5

Re: [PATCH 1/3] pseudo: support --without-passwd-fallback configuration option

[Peter Seebach]

On Sun, 12 Oct 2014 18:49:53 -0500
"Peter A. Bigot" <pab@pabigot.com> wrote:

> +Inspection of pseudo_client shows the only other use of PSEUDO_ETC_FILE
> +to be protected against magic.  Applying the same shield to the
> +unprotected calls in pseudo_pwd_lck_{open,close} fixes the issue.

D'oh. This is clearly right. Depending, it may be better to merge this into
upstream and just switch to the branch that gets it (presumably 1.6.3), but
it's right either way.

-s
-- 
Listen, get this.  Nobody with a good compiler needs to be justified.

[PATCH 2/3] pseudo: support multiple search directories in PSEUDO_PASSWD

[Peter A. Bigot]

This makes it possible to use --without-passwd-fallback when building
images where the preferred passwd files are not available until after
installation has begun.

Signed-off-by: Peter A. Bigot <pab@pabigot.com>
---
Cc: peter.seebach@windriver.com

 ..._util-modify-interface-to-pseudo_etc_file.patch |  70 +++++++++++++
 ...nt.c-support-multiple-directories-in-PSEU.patch | 115 +++++++++++++++++++++
 meta/recipes-devtools/pseudo/pseudo_1.6.2.bb       |   2 +
 3 files changed, 187 insertions(+)
 create mode 100644 meta/recipes-devtools/pseudo/pseudo-1.6.2/0002-pseudo_util-modify-interface-to-pseudo_etc_file.patch
 create mode 100644 meta/recipes-devtools/pseudo/pseudo-1.6.2/0003-pseudo_client.c-support-multiple-directories-in-PSEU.patch

diff --git a/meta/recipes-devtools/pseudo/pseudo-1.6.2/0002-pseudo_util-modify-interface-to-pseudo_etc_file.patch b/meta/recipes-devtools/pseudo/pseudo-1.6.2/0002-pseudo_util-modify-interface-to-pseudo_etc_file.patch
new file mode 100644
index 0000000..c7006ef
--- /dev/null
+++ b/meta/recipes-devtools/pseudo/pseudo-1.6.2/0002-pseudo_util-modify-interface-to-pseudo_etc_file.patch
@@ -0,0 +1,70 @@
+From f05def2bbd5507084672bc9072ffe0e5101e9b47 Mon Sep 17 00:00:00 2001
+From: "Peter A. Bigot" <pab@pabigot.com>
+Date: Sun, 12 Oct 2014 11:35:57 -0500
+Subject: [PATCH 2/3] pseudo_util: modify interface to pseudo_etc_file
+
+* Make the search directory pointers const: there is no reason why this
+  function should be allowed to mutate the directories.
+
+* Change the search directory argument from an array of pointers to a
+  pointer-to-pointers to prepare for an upcoming enhancement.
+
+Upstream-Status: Pending
+Signed-off-by: Peter A. Bigot <pab@pabigot.com>
+---
+ pseudo.h        | 2 +-
+ pseudo_client.c | 2 +-
+ pseudo_util.c   | 4 ++--
+ 3 files changed, 4 insertions(+), 4 deletions(-)
+
+diff --git a/pseudo.h b/pseudo.h
+index 92020e4..05813c1 100644
+--- a/pseudo.h
++++ b/pseudo.h
+@@ -86,7 +86,7 @@ extern int pseudo_logfile(char *defname);
+ extern ssize_t pseudo_sys_path_max(void);
+ extern ssize_t pseudo_path_max(void);
+ #define PSEUDO_PWD_MAX 4096
+-extern int pseudo_etc_file(const char *filename, char *realname, int flags, char *path[], int dircount);
++extern int pseudo_etc_file(const char *filename, char *realname, int flags, const char **search_dirs, int dircount);
+ extern void pseudo_stat32_from64(struct stat *, const struct stat64 *);
+ extern void pseudo_stat64_from32(struct stat64 *, const struct stat *);
+ 
+diff --git a/pseudo_client.c b/pseudo_client.c
+index 442dd19..7a4d7fa 100644
+--- a/pseudo_client.c
++++ b/pseudo_client.c
+@@ -93,7 +93,7 @@ gid_t pseudo_egid;
+ gid_t pseudo_sgid;
+ gid_t pseudo_fgid;
+ 
+-#define PSEUDO_ETC_FILE(filename, realname, flags) pseudo_etc_file(filename, realname, flags, (char *[]) { pseudo_chroot, pseudo_passwd, PSEUDO_PASSWD_FALLBACK }, PSEUDO_PASSWD_FALLBACK ? 3 : 2)
++#define PSEUDO_ETC_FILE(filename, realname, flags) pseudo_etc_file(filename, realname, flags, (const char *[]) { pseudo_chroot, pseudo_passwd, PSEUDO_PASSWD_FALLBACK }, PSEUDO_PASSWD_FALLBACK ? 3 : 2)
+ 
+ /* helper function to make a directory, just like mkdir -p.
+  * Can't use system() because the child shell would end up trying
+diff --git a/pseudo_util.c b/pseudo_util.c
+index e4e1fc8..647d3ad 100644
+--- a/pseudo_util.c
++++ b/pseudo_util.c
+@@ -1264,7 +1264,7 @@ FILE *pseudo_host_etc_group_file = &pseudo_fake_group_file;
+ #endif
+ 
+ int
+-pseudo_etc_file(const char *file, char *realname, int flags, char *search_dirs[], int dircount) {
++pseudo_etc_file(const char *file, char *realname, int flags, const char **search_dirs, int dircount) {
+ 	char filename[pseudo_path_max()];
+ 	int rc = -1;
+ 
+@@ -1280,7 +1280,7 @@ pseudo_etc_file(const char *file, char *realname, int flags, char *search_dirs[]
+ 		return -1;
+ 	}
+ 	for (i = 0; i < dircount; ++i) {
+-		char *s = search_dirs[i];
++		const char *s = search_dirs[i];
+ 		if (!s)
+ 			continue;
+ #if PSEUDO_PORT_DARWIN
+-- 
+1.8.5.5
+
diff --git a/meta/recipes-devtools/pseudo/pseudo-1.6.2/0003-pseudo_client.c-support-multiple-directories-in-PSEU.patch b/meta/recipes-devtools/pseudo/pseudo-1.6.2/0003-pseudo_client.c-support-multiple-directories-in-PSEU.patch
new file mode 100644
index 0000000..ccef029
--- /dev/null
+++ b/meta/recipes-devtools/pseudo/pseudo-1.6.2/0003-pseudo_client.c-support-multiple-directories-in-PSEU.patch
@@ -0,0 +1,115 @@
+From 0ba6d83d54423a97bb767a8d753e87f52052505f Mon Sep 17 00:00:00 2001
+From: "Peter A. Bigot" <pab@pabigot.com>
+Date: Sun, 12 Oct 2014 12:17:48 -0500
+Subject: [PATCH 3/3] pseudo_client.c: support multiple directories in
+ PSEUDO_PASSWD
+
+For OpenEmbedded it is highly unlikely that using the build host passwd
+file is the right approach.  Most packages can be built with a pseudo
+that was configured --without-passwd-fallback, since
+PSEUDO_PASSWD=${STAGING_DIR_TARGET} suffices.
+
+This fails when building images, because image.bbclass (correctly)
+overrides to PSEUDO_PASSWD=${IMAGE_ROOTFS}.  However, the rootfs
+/etc/passwd is not created until the post-install phase of base-passwd,
+which is long after a passwd file is required.  For example, the smart
+RPM interface wants to look up uid 0 right away.  The right solution
+here is to look first in ${IMAGE_ROOTFS}, then fallback to
+${STAGING_DIR_TARGET}.
+
+Rather than rework pseudo to change PSEUDO_PASSWD_FALLBACK to be a
+run-time rather than compile-time specification, rework the handling of
+PSEUDO_PASSWD so that it is a colon-separated list of directories that
+are processed in order.
+
+Upstream-Status: Pending
+Signed-off-by: Peter A. Bigot <pab@pabigot.com>
+---
+ pseudo_client.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++++-
+ 1 file changed, 49 insertions(+), 1 deletion(-)
+
+diff --git a/pseudo_client.c b/pseudo_client.c
+index 7a4d7fa..b52b86a 100644
+--- a/pseudo_client.c
++++ b/pseudo_client.c
+@@ -75,6 +75,8 @@ int pseudo_umask = 022;
+ 
+ static char **fd_paths = NULL;
+ static int nfds = 0;
++static const char **passwd_paths = NULL;
++static int npasswd_paths = 0;
+ static int messages = 0;
+ static struct timeval message_time = { .tv_sec = 0 };
+ static int pseudo_inited = 0;
+@@ -93,7 +95,7 @@ gid_t pseudo_egid;
+ gid_t pseudo_sgid;
+ gid_t pseudo_fgid;
+ 
+-#define PSEUDO_ETC_FILE(filename, realname, flags) pseudo_etc_file(filename, realname, flags, (const char *[]) { pseudo_chroot, pseudo_passwd, PSEUDO_PASSWD_FALLBACK }, PSEUDO_PASSWD_FALLBACK ? 3 : 2)
++#define PSEUDO_ETC_FILE(filename, realname, flags) pseudo_etc_file(filename, realname, flags, passwd_paths, npasswd_paths)
+ 
+ /* helper function to make a directory, just like mkdir -p.
+  * Can't use system() because the child shell would end up trying
+@@ -117,6 +119,42 @@ mkdir_p(char *path) {
+ 	(void) mkdir(path, 0755);
+ }
+ 
++static int
++build_passwd_paths(const char **paths)
++{
++	int np = 0;
++	
++	if (pseudo_chroot) {
++		if (paths) {
++			paths[np] = pseudo_chroot;
++		}
++		++np;
++	}
++	if (pseudo_passwd) {
++		const char *cp = pseudo_passwd;
++		const char *next = strchr(cp, ':');
++		while (next) {
++			if (paths) {
++				paths[np] = strndup(cp, next-cp);
++			}
++			++np;
++			cp = next+1;
++			next = strchr(cp, ':');
++		}
++		if (paths) {
++			paths[np] = strdup(cp);
++		}
++		++np;
++	}
++	if (PSEUDO_PASSWD_FALLBACK) {
++		if (paths) {
++			paths[np] = PSEUDO_PASSWD_FALLBACK;
++		}
++		++np;
++	}
++	return np;
++}
++
+ void
+ pseudo_init_client(void) {
+ 	char *env;
+@@ -329,6 +367,16 @@ pseudo_init_client(void) {
+ 		}
+ 		free(env);
+ 
++		npasswd_paths = build_passwd_paths(NULL);
++		if (npasswd_paths) {
++			passwd_paths = malloc(npasswd_paths * sizeof(*passwd_paths));
++			if (!passwd_paths) {
++				pseudo_diag("couldn't allocate space for passwd paths.\n");
++				exit(1);
++			}
++			build_passwd_paths(passwd_paths);
++		}
++
+ 		pseudo_inited = 1;
+ 	}
+ 	if (!pseudo_disabled)
+-- 
+1.8.5.5
+
diff --git a/meta/recipes-devtools/pseudo/pseudo_1.6.2.bb b/meta/recipes-devtools/pseudo/pseudo_1.6.2.bb
index df8ce83..78eeedf 100644
--- a/meta/recipes-devtools/pseudo/pseudo_1.6.2.bb
+++ b/meta/recipes-devtools/pseudo/pseudo_1.6.2.bb
@@ -3,6 +3,8 @@ require pseudo.inc
 SRC_URI = " \
     http://www.yoctoproject.org/downloads/${BPN}/${BPN}-${PV}.tar.bz2 \
     file://0001-pseudo_client.c-protect-pwd_lck-against-magic.patch \
+    file://0002-pseudo_util-modify-interface-to-pseudo_etc_file.patch \
+    file://0003-pseudo_client.c-support-multiple-directories-in-PSEU.patch \
 "
 
 SRC_URI[md5sum] = "4d7b4f9d1b4aafa680ce94a5a9a52f1f"
-- 
1.8.5.5

Re: [PATCH 2/3] pseudo: support multiple search directories in PSEUDO_PASSWD

[Peter Seebach]

On Sun, 12 Oct 2014 18:49:54 -0500
"Peter A. Bigot" <pab@pabigot.com> wrote:

> ++		npasswd_paths = build_passwd_paths(NULL);
> ++		if (npasswd_paths) {
> ++			passwd_paths = malloc(npasswd_paths * sizeof(*passwd_paths));
> ++			if (!passwd_paths) {
> ++				pseudo_diag("couldn't allocate space for passwd paths.\n");
> ++				exit(1);
> ++			}
> ++			build_passwd_paths(passwd_paths);

I'm slightly inclined to think that this might be better done with a function
which counts colons and then builds the path list accordingly. But I
basically like the idea of supporting a path list. As I recall, we talked
about this early on, but for some reason I formed the theory that it wasn't
necessary.

I'd point out that strictly speaking, the change from [] to * doesn't matter
in function parameter lists, and I tend to prefer [] when the argument is
logically an array rather than a pointer to a single item, except that this
preference is not something I'm very consistent about.

But this does seem to me to be correct.

I'll have to think about it a bit because, while the "call once to find out
how much storage you need, again to use the storage" idiom is pretty good, I
think its primary benefit over simpler calling patterns is that it can be
used with arbitrary memory allocation patterns or goals. In this case, with
only one caller, we probably don't need it. That said, if I'm still busy this
week (which looks likely) it's not a bad idea to merge this as-is and wait for
1.6.3 for arguable improvements, since this is pretty clearly correct.

-s
-- 
Listen, get this.  Nobody with a good compiler needs to be justified.

[PATCH 3/3] image.bbclass: search both rootfs and staging dir for passwd files

[Peter A. Bigot]

When pseudo is configured to disallow fallback to the build host
/etc/hosts and /etc/group, the selection of ${IMAGE_ROOT} for
PSEUDO_PASSWD is insufficient as the necessary files will not be
available until base-passwd has been installed and its pkg_postinst
script run.  Fall back to the ${STAGING_DIR_TARGET} version of those
files until the rootfs versions are available.

Signed-off-by: Peter A. Bigot <pab@pabigot.com>
---

NOTE: This patch REQUIRES that preceding two patches to pseudo
that provide multiple path support in PSEUDO_PASSWD.

 meta/classes/image.bbclass | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/meta/classes/image.bbclass b/meta/classes/image.bbclass
index 940bdb6..18f0fdd 100644
--- a/meta/classes/image.bbclass
+++ b/meta/classes/image.bbclass
@@ -186,7 +186,9 @@ IMAGE_LINGUAS ?= "de-de fr-fr en-gb"
 
 LINGUAS_INSTALL ?= "${@" ".join(map(lambda s: "locale-base-%s" % s, d.getVar('IMAGE_LINGUAS', True).split()))}"
 
-PSEUDO_PASSWD = "${IMAGE_ROOTFS}"
+# Prefer image, but fall back to staging dir if image files not yet
+# available.
+PSEUDO_PASSWD = "${IMAGE_ROOTFS}:${STAGING_DIR_TARGET}"
 
 do_rootfs[dirs] = "${TOPDIR}"
 do_rootfs[lockfiles] += "${IMAGE_ROOTFS}.lock"
-- 
1.8.5.5

Re: [PATCH 0/3] pseudo+image.bbclass: changes to avoid host contamination

[Peter Seebach]

On Sun, 12 Oct 2014 18:49:52 -0500
"Peter A. Bigot" <pab@pabigot.com> wrote:

> I believe OE should add --without-passwd-fallback to the pseudo 1.6.2
> configuration flags early in the 1.8 development cycle, to ensure there
> are no host contamination issues.  I can think of no reason why the
> build host passwd and group files should ever be considered suitable for
> use in determining target user/group characteristics.

I endorse this evaluation.

I've been thinking about this more, and I'd like to wave a thought at people:

I propose for consideration the idea that pseudo have a compile-time
hard-coded default answer to "what is uid 0" and "what is gid 0", and use
that instead of the "fallback path".

The rationale is basically: Yes, it's a special case. But it's *the* special
case. It's the special case that is the uid pseudo emulates by default, and
it's the only one that most packages ever need. If we do this, then the only
packages which need to depend on base-passwd are those which need to actually
use a *non-root* uid/gid. And every such package had better already have
a dependency either directly on the passwd stuff, or on some other package
which does.

I am pretty sure that this makes more sense than the default fallback to
host passwd/group. I might want to preserve the option of that fallback, but
make it a non-default.

Anyone have strong feelings on this? Thoughts I may not have thought through
yet? I don't know that I actually had a coherent reason in mind for the
original fallback behavior, and this analysis convinces me that falling back
to host uid/gid is probably wrong for many-to-most use cases.

I guess it might make sense for something more like a debian package build
where you really are targeting the host, but even then, I am not sure that the
default host fallback is a good idea.

-s
-- 
Listen, get this.  Nobody with a good compiler needs to be justified.

Re: [PATCH 0/3] pseudo+image.bbclass: changes to avoid host contamination

[Peter A. Bigot]

[-- Attachment #1: Type: text/plain, Size: 3726 bytes --]

On 10/13/2014 04:28 PM, Peter Seebach wrote:
> On Sun, 12 Oct 2014 18:49:52 -0500
> "Peter A. Bigot" <pab@pabigot.com> wrote:
>
>> I believe OE should add --without-passwd-fallback to the pseudo 1.6.2
>> configuration flags early in the 1.8 development cycle, to ensure there
>> are no host contamination issues.  I can think of no reason why the
>> build host passwd and group files should ever be considered suitable for
>> use in determining target user/group characteristics.
> I endorse this evaluation.
>
> I've been thinking about this more, and I'd like to wave a thought at people:
>
> I propose for consideration the idea that pseudo have a compile-time
> hard-coded default answer to "what is uid 0" and "what is gid 0", and use
> that instead of the "fallback path".
>
> The rationale is basically: Yes, it's a special case. But it's *the* special
> case. It's the special case that is the uid pseudo emulates by default, and
> it's the only one that most packages ever need. If we do this, then the only
> packages which need to depend on base-passwd are those which need to actually
> use a *non-root* uid/gid. And every such package had better already have
> a dependency either directly on the passwd stuff, or on some other package
> which does.


As I noted in an off-list message:

    The thing that worries me is the slippery slope of assumptions.
    Sure, assume that pw_name for uid 0 is "root". *Probably* pw_gid is
    0, but is that required by some standard?  pw_dir could be "/" or
    "/home/root"; which one?  "/bin/sh"?  Not on Ubuntu.  pw_gecos?
    Guesses can be made for all those, but for many there might be a
    legitimate reason why the actual final passwd file on the target
    selects different values, and decisions made based on the guesses
    might result in very difficult to diagnose inconsistencies.


Basically, even if "root" is a special case, taking this path means 
making assumptions about what the application is doing based on what 
system/libc functions it invokes.  Too often when somebody assumes a 
general tool will only be used specific ways, somebody else will prove 
them wrong.

I can't say it won't turn out to be the best approach, but I'd like to 
see some thought applied to how the dependency on base-passwd might be 
added without modifying every recipe.  E.g., something like:

DEPENDS_INSTALL ?= "base-passd:do_populate_sysroot"
d.appendVarFlag('do_install', 'depends', d.expand(' ${DEPENDS_INSTALL}'))

down in base.bbclass.  (Or whatever is necessary to get the intent of 
that snippet.)

> I am pretty sure that this makes more sense than the default fallback to
> host passwd/group. I might want to preserve the option of that fallback, but
> make it a non-default.
>
> Anyone have strong feelings on this? Thoughts I may not have thought through
> yet? I don't know that I actually had a coherent reason in mind for the
> original fallback behavior, and this analysis convinces me that falling back
> to host uid/gid is probably wrong for many-to-most use cases.
>
> I guess it might make sense for something more like a debian package build
> where you really are targeting the host, but even then, I am not sure that the
> default host fallback is a good idea.

I can't offhand think of a reason host fallback would be necessary, but 
since it already exists keeping it as a non-default option is maybe 
worthwhile (see "assumptions" above).

Note that with my enhancement you should be able to do:

PSEUDO_PASSWD = "${STAGING_DIR_TARGET}:"

and get the host files, but that's not a fixed fallback that's always 
available regardless of invocation options.

Peter

[-- Attachment #2: Type: text/html, Size: 4720 bytes --]

Re: [PATCH 0/3] pseudo+image.bbclass: changes to avoid host contamination

[Peter Seebach]

On Mon, 13 Oct 2014 17:29:26 -0500
"Peter A. Bigot" <pab@pabigot.com> wrote:

> Basically, even if "root" is a special case, taking this path means 
> making assumptions about what the application is doing based on what 
> system/libc functions it invokes.  Too often when somebody assumes a 
> general tool will only be used specific ways, somebody else will prove 
> them wrong.

True.

For the oe-core case, it seems like it might be reasonable for us to set
pseudo up to, by default, get configured with preloaded things which match
the expected configuration of base-passwd. If people then go overriding
base-passwd in strange ways, they could break that, but we at least know what
the default configuration would be, and could possibly even make base-passwd
check whether pseudo's configuration matches its answers, and blow up if
there's a mismatch there.

-s
-- 
Listen, get this.  Nobody with a good compiler needs to be justified.

Re: [PATCH 0/3] pseudo+image.bbclass: changes to avoid host contamination

[Peter A. Bigot]

On 10/13/2014 05:35 PM, Peter Seebach wrote:
> On Mon, 13 Oct 2014 17:29:26 -0500
> "Peter A. Bigot" <pab@pabigot.com> wrote:
>
>> Basically, even if "root" is a special case, taking this path means
>> making assumptions about what the application is doing based on what
>> system/libc functions it invokes.  Too often when somebody assumes a
>> general tool will only be used specific ways, somebody else will prove
>> them wrong.
> True.
>
> For the oe-core case, it seems like it might be reasonable for us to set
> pseudo up to, by default, get configured with preloaded things which match
> the expected configuration of base-passwd. If people then go overriding
> base-passwd in strange ways, they could break that, but we at least know what
> the default configuration would be, and could possibly even make base-passwd
> check whether pseudo's configuration matches its answers, and blow up if
> there's a mismatch there.

I've discovered a few more cases where OE packages get the wrong target 
username/group for files, at least in RPM packages.

First, revisiting the discussion above I've played with using 
--without-passwd-fallback and adding base-passwd as an explicit 
dependency. This won't work: glibc-initial requires base-passwd for 
group name lookups, and base-passwd includes update-passwd as an 
executable which requires glibc.

The options seem to be to split base-passwd into separate recipes for 
the data files and the utility to break the circular dependency, or 
having pseudo synthesize a fallback passwd/group.  Prior to gaining 
experience I didn't like the second choice, but I like the first even 
less.  As long as pseudo emits a note saying what it's doing so we can 
add the DEPENDS=base-passwd where that's not circular (as with tzdata), 
my vote goes toward pseudo synthesis. I'm hoping somebody disagrees and 
comes up with a better, third alternative.

However, I've also discovered another issue, possibly related to: 
http://lists.openembedded.org/pipermail/openembedded-core/2011-December/053866.html

On my development machine my uid:gid are both 1000 and correspond to 
pab:pab.  Using poky master with MACHINE=beaglebone building 
core-image-sato the corresponding user:group is xuser:xuser.

Running the following script:

find tmp/deploy/ -name '*.rpm' \
   | while read f ; do \
     rpm -qlvp ${f} 2>/dev/null \
       | awk '$3~/pab|xuser/ || $4~/pab|xuser/ {print;}' \
       > /tmp/c$$ ; \
     if [ -s /tmp/c$$ ] ; then \
       echo ; \
       echo $f; \
       cat /tmp/c$$ ; \
     fi ; \
   done

I find the following packages include files that are given group or user 
xuser, indicating that the files were installed with user:group set to 
the host value 1000:1000 instead of being remapped to 0:0 by pseudo, but 
when packaging pseudo does "correctly" interpret the uid:gid using the 
target passwd/group files:

tmp/deploy/rpm/cortexa8hf_vfp_neon/attr-ptest-2.4.47-r0.0.cortexa8hf_vfp_neon.rpm
tmp/deploy/rpm/cortexa8hf_vfp_neon/acl-ptest-2.2.52-r0.0.cortexa8hf_vfp_neon.rpm
tmp/deploy/rpm/cortexa8hf_vfp_neon/systemd-216+git0+5d0ae62c66-r0.0.cortexa8hf_vfp_neon.rpm
tmp/deploy/rpm/cortexa8hf_vfp_neon/perl-ptest-5.20.0-r1.0.cortexa8hf_vfp_neon.rpm

For example:

llc[11]$ rpm -qlvp 
tmp/deploy/rpm/cortexa8hf_vfp_neon/perl-ptest-5.20.0-r1.0.cortexa8hf_vfp_neon.rpm
-r--r--r--    1 xuser   xuser                   45590 May 26 08:34 
/usr/lib/perl/ptest/AUTHORS
-r--r--r--    1 xuser   xuser                    6321 Jan 31  2014 
/usr/lib/perl/ptest/Artistic
-r--r--r--    1 xuser   xuser                    3168 Jan 31  2014 
/usr/lib/perl/ptest/Changes
-r-xr-xr-x    1 xuser   xuser                  552838 Oct 31 15:34 
/usr/lib/perl/ptest/Configure

Further, this one even manages to get the user:group names from the host:

llc[12]$ rpm -qlvp 
tmp/deploy/rpm/cortexa8hf_vfp_neon/libgcc-s-dev-4.9.1-r0.0.cortexa8hf_vfp_neon.rpm 
| grep pab
lrwxrwxrwx    1 pab     pab                        22 Oct 31 15:17 
/usr/lib/arm-poky-linux -> arm-poky-linux-gnueabi

My tentative conclusion is that these new behaviors aren't related to 
the pseudo falling back to host /etc files, but probably instead by 
files getting installed (and in one case somehow packaged) without using 
pseudo.  I have no idea why that's happening, but it does appear 
something's not working right.

Peter

Re: [PATCH 0/3] pseudo+image.bbclass: changes to avoid host contamination

[Peter A. Bigot]

On 10/31/2014 11:15 PM, Peter A. Bigot wrote:
> On 10/13/2014 05:35 PM, Peter Seebach wrote:
>> On Mon, 13 Oct 2014 17:29:26 -0500
>> "Peter A. Bigot" <pab@pabigot.com> wrote:
>>
>>> Basically, even if "root" is a special case, taking this path means
>>> making assumptions about what the application is doing based on what
>>> system/libc functions it invokes.  Too often when somebody assumes a
>>> general tool will only be used specific ways, somebody else will prove
>>> them wrong.
>> True.
>>
>> For the oe-core case, it seems like it might be reasonable for us to set
>> pseudo up to, by default, get configured with preloaded things which 
>> match
>> the expected configuration of base-passwd. If people then go overriding
>> base-passwd in strange ways, they could break that, but we at least 
>> know what
>> the default configuration would be, and could possibly even make 
>> base-passwd
>> check whether pseudo's configuration matches its answers, and blow up if
>> there's a mismatch there.
>
> I've discovered a few more cases where OE packages get the wrong 
> target username/group for files, at least in RPM packages.
>
> First, revisiting the discussion above I've played with using 
> --without-passwd-fallback and adding base-passwd as an explicit 
> dependency. This won't work: glibc-initial requires base-passwd for 
> group name lookups, and base-passwd includes update-passwd as an 
> executable which requires glibc.
>
> The options seem to be to split base-passwd into separate recipes for 
> the data files and the utility to break the circular dependency, or 
> having pseudo synthesize a fallback passwd/group. Prior to gaining 
> experience I didn't like the second choice, but I like the first even 
> less.  As long as pseudo emits a note saying what it's doing so we can 
> add the DEPENDS=base-passwd where that's not circular (as with 
> tzdata), my vote goes toward pseudo synthesis. I'm hoping somebody 
> disagrees and comes up with a better, third alternative.

I did find an alternative, and the patches to enforce 
--without-passwd-fallback have been posted.

>
> However, I've also discovered another issue, possibly related to: 
> http://lists.openembedded.org/pipermail/openembedded-core/2011-December/053866.html 
>
>
> On my development machine my uid:gid are both 1000 and correspond to 
> pab:pab.  Using poky master with MACHINE=beaglebone building 
> core-image-sato the corresponding user:group is xuser:xuser.
>
> Running the following script:
>
> find tmp/deploy/ -name '*.rpm' \
>   | while read f ; do \
>     rpm -qlvp ${f} 2>/dev/null \
>       | awk '$3~/pab|xuser/ || $4~/pab|xuser/ {print;}' \
>       > /tmp/c$$ ; \
>     if [ -s /tmp/c$$ ] ; then \
>       echo ; \
>       echo $f; \
>       cat /tmp/c$$ ; \
>     fi ; \
>   done
>
> I find the following packages include files that are given group or 
> user xuser, indicating that the files were installed with user:group 
> set to the host value 1000:1000 instead of being remapped to 0:0 by 
> pseudo, but when packaging pseudo does "correctly" interpret the 
> uid:gid using the target passwd/group files:
>
> tmp/deploy/rpm/cortexa8hf_vfp_neon/attr-ptest-2.4.47-r0.0.cortexa8hf_vfp_neon.rpm 
>
> tmp/deploy/rpm/cortexa8hf_vfp_neon/acl-ptest-2.2.52-r0.0.cortexa8hf_vfp_neon.rpm 
>
> tmp/deploy/rpm/cortexa8hf_vfp_neon/systemd-216+git0+5d0ae62c66-r0.0.cortexa8hf_vfp_neon.rpm 
>
> tmp/deploy/rpm/cortexa8hf_vfp_neon/perl-ptest-5.20.0-r1.0.cortexa8hf_vfp_neon.rpm 
>
>
> For example:
>
> llc[11]$ rpm -qlvp 
> tmp/deploy/rpm/cortexa8hf_vfp_neon/perl-ptest-5.20.0-r1.0.cortexa8hf_vfp_neon.rpm
> -r--r--r--    1 xuser   xuser                   45590 May 26 08:34 
> /usr/lib/perl/ptest/AUTHORS
> -r--r--r--    1 xuser   xuser                    6321 Jan 31  2014 
> /usr/lib/perl/ptest/Artistic
> -r--r--r--    1 xuser   xuser                    3168 Jan 31  2014 
> /usr/lib/perl/ptest/Changes
> -r-xr-xr-x    1 xuser   xuser                  552838 Oct 31 15:34 
> /usr/lib/perl/ptest/Configure

The issue of the underlying gid not being correct are still present.  It 
is non-deterministic, and may or may not be happening with uid as well.  
It does seem to happen most with *-ptest-* recipes and others that 
install files probably whole-directory-at-a-time.

> Further, this one even manages to get the user:group names from the host:
>
> llc[12]$ rpm -qlvp 
> tmp/deploy/rpm/cortexa8hf_vfp_neon/libgcc-s-dev-4.9.1-r0.0.cortexa8hf_vfp_neon.rpm 
> | grep pab
> lrwxrwxrwx    1 pab     pab                        22 Oct 31 15:17 
> /usr/lib/arm-poky-linux -> arm-poky-linux-gnueabi

This problem was fixed by eliminating host fallback.

Peter

> My tentative conclusion is that these new behaviors aren't related to 
> the pseudo falling back to host /etc files, but probably instead by 
> files getting installed (and in one case somehow packaged) without 
> using pseudo.  I have no idea why that's happening, but it does appear 
> something's not working right.
>
> Peter
>