[Pkg-privacy-maintainers] Bug#911907: [monkeysphere] Bug#911907: Bug#911907: monkeysphere: Install fails on systems with PAM login restrictions

[Sunil Mohan Adapa]

Thank you for the feedback.

On Friday 26 October 2018 05:41 AM, Daniel Kahn Gillmor wrote:
[...]
>>              # requote arguments using bash builtin feature (see "help printf"):
>> -	    su "$MONKEYSPHERE_USER" -s "$(which bash)" -c "$(printf "%q " "$@")"
>> +	    runuser -u "$MONKEYSPHERE_USER" -- "$@"
>>             ;;
> 
> This makes several different changes besides just moving to runuser.  in
> particular, it drops the -s "$(which bash)" (and therefore i think
> relies on the monkeysphere user's chosen shell in /etc/passwd --
> hopefully that's correct, but it might have changed somehow).  Would you
> be ok with reinstating the -s "$(which bash)" arguments?

-s "$(which bash)" was necessary to fix #827660 because su, which uses
user's shell, was used and monkeysphere user had a shell other than bash.

runuser does not seem to depend on the user's shell. It simply runs a
given process under that user's id (with a different PAM configuration).
 This is better at least for the purposes we are using it. Whenever
evaluating bash expressions as a user, we are already doing
'su_monkeysphere_user bash -c "<EXPR>"'.

# usermod -s /usr/sbin/nologin monkeysphere
# su monkeysphere ls
This account is currently not available.
# runuser --user monkeysphere ls
<output>

I am additionally proposing another patch which will remove the shell
for the monkeysphere user in Debian packaging. This closes #901489. We
don't need the monkeysphere user's shell.

> 
> 
> And, it changes how the invocation is passed through -- rather than
> using -c (which can pass shell-specific data) it passes the arguments
> directly to runuser.  This is fine for most cases, but it might fail if
> invoked as something like:
> 
>    su_monkeysphere_user echo test '>' /tmp/foo
> 
> this could do two different things:
> 
>  a) echo "test", the literal ">" symbol,  and the string "/tmp/foo"
>  b) echo "test" into the file "/tmp/foo" as the monkeysphere user.
> 
> The current code seems to assume that the you might want to pass shell
> metacharacters through to be executed that way, and that (b) should be
> the correct result.  I think it's a bad idea to do that, though, and i
> prefer your simpler formulation.
> 
> I'm concerned about the complex constructions (sigh, shell is such a
> mess) passed to su_monkeysphere_user in update_users and add_certifier
> (for monkeysphere-authentication), and add_revoker (for
> monkeysphere-host), though.  Have you tested those subcommands on a live
> system as the superuser?
> 
> If we can go with your proposed simpler argument passing, we definitely
> should!
> 

I didn't test all the situations before, but in this round I did. I ran
all commands with bash -x and ensured that every invocation of the
method in all branches was indeed triggered.

I explicitly changed the name of the method to
'run_as_monkeysphere_user' and changed all invocations to make them
reflect the changed semantics and also to see the full impact. All the
invocations fall into the these categories:

1) Simple invocation of gpg/openpgp2ssh command without passing
environment variables and shell expressions. There is no issues here.

2) Execution of bash snippets. All of these executions where already
wrapped with bash -c "<EXPR>". So they all work as is with runuser.

3) Execution of commands with environment variable passing. Since
runuser preserves environment from the parent process, we can now say
`KEY=VALUE runuser -u monkeysphere gpg` instead of the shell expression
`su monkeysphere KEY=VALUE gpg`. I have updated the patch to handle
these situations properly.

# TESTVAR1='Hello!' runuser -u monkeysphere -- bash -c 'set' |grep TESTVAR1
TESTVAR1='Hello!'

4) Redirection expressions like what you have described above currently
do not exist. I added documentation that bash expressions should not be
run using the method.

It looks like we can keep the simplification.

Thanks,

-- 
Sunil

PS: I ran into issues with TMPDIR as described in #656750 and I am
currently working around them.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Use-runuser-instead-of-su.patch
Type: text/x-patch
Size: 8314 bytes
Desc: not available
URL: <http://alioth-lists.debian.net/pipermail/pkg-privacy-maintainers/attachments/20181029/9b7df83c/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-debian-Remove-shell-for-monkeysphere-user.patch
Type: text/x-patch
Size: 1313 bytes
Desc: not available
URL: <http://alioth-lists.debian.net/pipermail/pkg-privacy-maintainers/attachments/20181029/9b7df83c/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <http://alioth-lists.debian.net/pipermail/pkg-privacy-maintainers/attachments/20181029/9b7df83c/attachment-0001.sig>