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: 33.997% (+0.004%) from 33.993%
when pulling 5de3bac on catap:openbsd
into 10f6723 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.

[gdt]

What I meant is that the line for OpenBSD lacks --, but it is present in the way the code was before for all systems. So compared to before, -- has been dropped. Why? Why was it present before? Why is it still present as the default case?

I don't follow your logic about FreeBSD. The man page lacks -s, so straightforwardly it should not be used, unless we hear from people that it works and that FreeBSD people think it's ok to use. There is no reason to expect it to work. We just have the null hypothesis that someone on LInux coded to their system and didn't think about portability. That happens!

I also meant that the call is coded in a way which doesn't even follow the linux man page. So I think that:

• invocations should be adjusted to the centroid of man pages
• FreeBSD should be as NetBSD
• someone(tm) should understand and explain why -- was present before and is now not, for OpenBSD
• the use of '-s /bin/sh' should be limited to systems that document it, which is Linux and OpenBSD, so far.

[catap]

What I meant is that the line for OpenBSD lacks --, but it is present in the way the code was before for all systems. So compared to before, -- has been dropped. Why? Why was it present before? Why is it still present as the default case?

OpenBSD su simple doesn't support -- as far as I know.

And I have no idea about other systems.

I don't follow your logic about FreeBSD. The man page lacks -s, so straightforwardly it should not be used, unless we hear from people that it works and that FreeBSD people think it's ok to use. There is no reason to expect it to work. We just have the null hypothesis that someone on LInux coded to their system and didn't think about portability. That happens!

I don't have FreeBSD near me, but I see that FreeBSD has ejabberd in ports and hasn't patch it. Here I assume that it's ok for them the way it is.

I also meant that the call is coded in a way which doesn't even follow the linux man page. So I think that:

• invocations should be adjusted to the centroid of man pages
• FreeBSD should be as NetBSD
• someone(tm) should understand and explain why -- was present before and is now not, for OpenBSD
• the use of '-s /bin/sh' should be limited to systems that document it, which is Linux and OpenBSD, so far.

I have no good answer about OpenBSD. The only one which I have is: I had recovered ejabberd into OpenBSD ports last October or November. It was in the OpenBSD ports but as version 2.1.12 which was removed in 2020. So, probably nobody uses it on OpenBSD.

[gdt]

Any progress? It is clear that the current code is buggy.

[catap]

@gdt I'd love to have it merged

[badlop]

There's a typo in your latest commit. The fix is:

diff --git a/ejabberdctl.template b/ejabberdctl.template
index 936de47a0..aa54b2d2c 100755
--- a/ejabberdctl.template
+++ b/ejabberdctl.template
@@ -134,7 +134,7 @@ exec_cmd()
             su -s /bin/sh "$INSTALLUSER" -c 'exec "$0" "$@"' "$@"
             ;;
         as_install_user,NetBSD)
-            su "$INSTALLUSER" -c 'exec "$0" "$@"' -- "$@" ;;
+            su "$INSTALLUSER" -c 'exec "$0" "$@"' -- "$@"
             ;;
         as_install_user,*)
             su -s /bin/sh -c 'exec "$0" "$@"' "$INSTALLUSER" -- "$@"

[catap]

@badlop fixed and rebased to the last master.

[badlop]

I propose to merge this PR because:

• it solves problems to work correctly in certain scenarios
• it doesn't seem to affect negatively the scenarios where it already works correctly
• in the future any of the case clauses can be improved individually without affecting the other clauses
• and of course, if someday a solution is found that works in all scenarios, it could replace the case

[gdt]

While I don't like the present situation or the post-merge situation (see comments above about --, -s being viewed as normal vs linuxy, FreeBSD), I agree with you that it's an improvement and we're better off landing it

[catap]

@gdt do you have clenaer approach?

[gdt]

I have outlined it, but I do not have code. Basically

• write to POSIX. Try to use only constructs in the POSIX description to start with. This means dropping "--", I think
• only use "-s" if the OS is GNU/Linux or Solaris (I think). Definitely do not use it as the general case with specific exceptions, because -s is a linuxism, adopted by solaris, or the other way around, not a POSIX_first approach. It's perfectly fine to use it where supported, but I so often see the main path being linux and then a long list of if for other systems and basically as each new system is added it is added to the exception list.
• for FreeBSD, it should end up being treated like NetBSD, because that's how the man pages read, rather than somehow assuming it's like linux instead, if I read the discussion above. But that I think is going to be in the general case, with a su that is presumed to comply with POSIX with no reason to believe it has extra features.

[catap]

In any case we haven't got better solution, and as @badlop points it's much better.

So, shall it be finally merged?