ejabberdctl: support OpenBSD su

[catap]

OpenBSD has a different than Linux su:

1. -c before username is treated as login class;
2. it doesn't require -- as arguments separator.

Without (1) it complains as:

su: no such login class: exec "$0" "$@"

and without (2):

-: --: not found

Here, I've added detection of OS via uname -s which routes to the right su. I really think that other BSD may need it as well.

[weiss]

Thanks. Having no way to use su(1) in a portable manner keeps being a PITA.

-c before username is treated as login class;

I think this could be swapped on Linux as well (in which case -c $args is just handed over to the shell directly, rather than being parsed by su first, which seems just fine).

it doesn't require -- as arguments separator.

At first glance I'm unsure why we need the -- "$@" part at all, but I'm probably just overlooking something (I'll test things later).

I really think that other BSD may need it as well.

On other BSDs, there's the unrelated issue that they don't support -s "$SHELL" (unlike OpenBSD and Linux). For eturnal, I just omit that option on other systems and hope for the best (i.e., I hope the user running the service has a usable shell):

https://github.com/processone/eturnal/blob/1.12.1/overlay/eturnalctl#L157-L166

[catap]

I really think that other BSD may need it as well.

On other BSDs, there's the unrelated issue that they don't support -s "$SHELL" (unlike OpenBSD and Linux). For eturnal, I just omit that option on other systems and hope for the best (i.e., I hope the user running the service has a usable shell):

Probably something like this?

exec_cmd()
{
    case $EXEC_CMD,$(uname -s) in
        as_install_user,OpenBSD)
            su -s /bin/sh "$INSTALLUSER" -c 'exec "$0" "$@"' "$@"
            ;;
        as_install_user,Linux)
            su -s /bin/sh -c 'exec "$0" "$@"' "$INSTALLUSER" -- "$@"
            ;;
        as_install_user,*)
            su "$INSTALLUSER" "$@"
            ;;
        as_current_user,*)
            "$@"
            ;;
    esac
}

[coveralls]

coverage: 32.9%. remained the same
when pulling 1b77abc on catap:openbsd
into 7d0c20e on processone:master.

[catap]

@weiss friendly ping!

[gdt]

In pkgsrc we are carrying a patch for su as well. I am not at all sure it's right.

-        as_install_user) su -s /bin/sh -c 'exec "$0" "$@"' "$INSTALLUSER" -- "$@" ;;
+       as_install_user) su - "$INSTALLUSER" -c 'exec "$0" "$@"' -- "$@" ;;

On NetBSD, su(1) doesn't document -s. -c is a login class before the username, but after it is part of the shell arguments. So what we have now is probably wrong too. NetBSD su doesn't document --. Reading the Linux man page, I don't see it either.

The intent is not clear; if it's supposed to run something with /bin/sh, that's ok, but it means that if it's a script, it needs to comply with POSIX sh (and thus be free of bashisms).

Overall, I would lean to writing the simplest command line that follows historical norms (4.2BSD) and also is known to work on Linux and Mac (likely it's ok), rather than trying to incrementally fix what's already complicated.

[weiss]

Overall, I would lean to writing the simplest command line that follows historical norms (4.2BSD) and also is known to work on Linux and Mac (likely it's ok), rather than trying to incrementally fix what's already complicated.

That would be awesome, I'm just not aware of a 4.2BSD-compliant way to setuid to a user who possibly doesn't have a usable login shell (but e.g. /bin/false instead).

[gdt]

4.2BSD was probably excessively aspirational.

On NetBSD, with a non-root use with /sbin/nologin as the shell, from root:
su foo: fails
su -l foo; fails
su -m foo: works, with root's shell
so I'm afraid this is hard.

I would suggest first adding a comment to the file where the invocation is, specifying what has to happen, and trying to be minimal.

[catap]

I think that this PR should contains also NetBSD patch. So, I just included it and rebased the branch to latest master.

I haven't tested it, but I hope that @gdt can help with that.

[gdt]

I'm feeling pretty confused. I rebuilt with this PR's diff, and not my previous patch. I find that if I am ejabberd user already, ejabberdctl works, and if I am root it does not, and I think that was the case before.

I don't understand why $@ is twice and what -- means.

I printed out $@ for a simple command and got:

/usr/pkg/bin/erl -sname undefined +K true +P 250000 -hidden -noinput -eval "net_kernel:connect_node('ejabberd@localhost')" -s ejabberd_ctl -extra 'ejabberd@localhost' connected_users_info

which leads to

Error! Failed to load module 'ejabberd_ctl' because it cannot be found.

and ktrace says that it looks for ejabberd_ctl.beam only in $cwd, and not where it ought to be, even though other beam files are found.

So there's something wrong, but I don't really understand how this is supposed to work, what's tricky about module loading, and why $@ appears twice.

[catap]

I had duble checked and the line for NetBSD should be the same with this patch: https://github.com/NetBSD/pkgsrc/blob/trunk/chat/ejabberd/patches/patch-ae

am I wrong?

[gdt]

I think you did, but I now believe the patch was to accommodate Solaris and never worked on NetBSD. (pkgsrc supports a huge number of operating systems.) I have not been running ejabberdctl as root, so I didn't notice. Sorry, should have checked.

I still think the first step is documenting what the function is supposed to do, and then for the Linux case, explaining why $@ appears twice and what -- does. I do not find -- documented in any su man page I've read.

[gdt]

After a lot of reading and reading the commit history, I have this change (to 25.04) that works on NetBSD:

-        as_install_user) su -s /bin/sh -c 'exec "$0" "$@"' "$INSTALLUSER" -- "$@" ;;
+        as_install_user) su "$INSTALLUSER" -c 'exec "$0" "$@"' -- "$@" ;;

This is almost the same as your OpenBSD line (no real surprise it matches), except that I didn't need to delete the --. If I remove the --, it still works. But I lean to minimal changes, especially when I don't understand.

Reading man pages, -s /bin/sh is a Linux extension to su and instructs that the given shell be used instead of the user's shell. FreeBSD su has -s, but it's about mandatory access control labels. macOS does not have -s.

Thus, I think what you have for OpenBSD, with the -- added back, should be the default case, and the -s /bin/sh restricted to Linux.

I don't understand how $INSTALLUSER being so late works on linux, from the man page. su has historically been "su user command".

On solaris, apparently 'su -' is needed; that's a synonym for 'su -l' which clears the environment, simulating a login.

[catap]

Yeah, I had made a lot of reading to figure out how su is works on Linux to rewrite it for OpenBSD...

So, here an updated version of the commit where I had used your line.

Interesting, which syntax is FreeBSD using? I haven't found any patch on their ports... can we assume that Linux-like is ok for them?

[gdt]

I read FreeBSD's man page and would say make it like NetBSD.

I think Linux's use of -s is unusual, and that should be reserved for an "is this Linux" case. I don't think it makes sense to treat the linux way as default. My impression is that freebsd/netbsd/openbsd and macos are all the same, and if POSIX were to standardize su (which they don't seem to have), that's what they'd pick.

[gdt]

Reading the diff:

• why did you drop the -- in the openbsd case? (Why do you think it is there in the first place?)
• I am surprised that OpenBSD has -s /bin/sh, but to my knowledge that makes 2 systems out of N
• do you think linux su will fail if INSTALLUSER is earlier? As I read the man page it should be fine, and the invocation as written is wrong - but I've read too many to be 100% sure at this point.

[catap]

@gdt I haven't drop anything as far as I see. I just added the second commit which brings a line for NetBSD.

Regarding FreeBSD: yes, man page reads like this but it hasn't got any patches for this and FreeBSD has quite a few compatibility with Linux... I not sure that we need touch it without access to FreeBSD with ejabberd to test it.