A more extensible ActionModule: Split _configure_module

[Raph]

https://github.com/ansible/ansible/pull/77503 suggested that an ActionModule could legitimately want to access both become_* and modules_* variables right before encoding, and doing so via moving a small code block about become_kwargs into a distinct method, thus allowing an ActionModule to (easily) override it.

Context:

_configure_module is a critical but very monolithic method.

An ActionModule could find a use-case for altering this method what implies a bunch of boilerplate code copy/pasting. This method is also responsible of calling modify_module after which module's arguments are definitely encoded.

The newly introduced method takes as its arguments multiple variables not available to a child-class otherwise. The default behavior is totally preserved and such a new method gives a chance to an ActionModule to modify them.

This trivial change greatly opens up extensibility of ActionModule in the most needed code-path.
Bonus point: It may make that part of the code easier to unit-test.

Replying to s-hertel concerns:

- Security: An ActionModule can already alter argument processing in pretty much any way... and it's fine. (User must install modules deemed secure).

Having a code difficult to override keeps good developers from improving and extending it rather than keeping evil developers from writing evil modules.

- Examples: The use-case of a module which needs two things: 1) Passing become_* arguments down to the remote execution context. 2) Alter the modules' arguments before AnsiballZ encoding (which may be cleaner and more generic than overriding multiple individual modules) : It's useful for sandboxed runs, for logging purpose, granular command-line processing, playbook simulations and more generally to any mod that need altering processing/arguments for several modules at once.

I hope that, contrary to the GitHub PR, this group is a room for discussion.

Regards

[Raph]

Since this didn't received replies yet and the above-mentioned PR was closed with the following:

> However, we're absolutely always up for discussion.

> starting a discussion on the development list prior to implementing a feature can make getting things included a little easier

... I'd like to politely request reconsideration of the above message, the corresponding PR and my response to s-hertel comments.

Thank you

[Brian Coca]

Actually I was already thinking of spliting _modify_module, for one, it should not need/handle/know about become_kwargs and we should eliminate the passing of these, which is kind of the opposite direction of your proposal. 

As for action plugins, they already do too much and have too much information for their needs, we are looking at moving these things into a more generic/changeable 'executor' that should handle those parts and leave the 'action plugin' to just coordinate local and remote actions w/o having to deal with the details as they do now.

[Raphaël]

On Tue, Jul 12, 2022 at 10:30:58AM -0700, Brian Coca wrote:
> Actually I was already thinking of spliting _modify_module, for one, it
> should not need/handle/know about become_kwargs and we should eliminate the
> passing of these, which is kind of the opposite direction of your
> proposal.

I requested a more extensible ActionModule for one actual use-case where code needs to
intervene over both `become_*` and `modules_*` variables (before these get encoded).

There are actual benefits in doing so and I suggested a non-intrusive patch for this.
(I'd be a bit... abrupt if a stance like "xxx should not need/handle/know about yyy" would
lead to disregard an actual and reasonable user request)


I made a case for a dynamically altering some of a module's arguments according to `become`
and that the existing code path for this must not only be absolutely preserved but also made
more easily overridable/changeable/hookable.


To be more precise about my particular case (even though one can easily think about many
more): This one (Ansibhook น) is an attempt to harden Ansible privileged command execution
to make it safer for use as a project auto-deployment tool.

That means tweaking some modules' arguments **according** to the become_method in order to
make the `sudo` requirement of some module more granular than it is currently
(whole `python -m` module execution vs only the `subprocess.Popen()` part)


And a way to tweak `run_command()` in an elegant and generic way depends on the
above-mentioned ability to access both `become_*` and `modules_*` variables before
the AnsiballZ creation (and add knowledge of the `become_*` context to the module)


I hope this will help to better understand the initial request.

> As for action plugins, they already do too much and have too much
> information for their needs, we are looking at moving these things into a
> more generic/changeable 'executor' that should handle those parts and leave
> the 'action plugin' to just coordinate local and remote actions w/o having
> to deal with the details as they do now.

If "generic" really means "more open to extensions, overrides, user freedom and
imagination", then I'm all for it.


Please let me know if a PR appears improving the situation.


Regards


น https://gitlab.com/drzraf/ansibhook

[Brian Coca]

I understand the request, but the push is to separate those concerns
as both from a security and simplicity perspective, action plugins
should not be able or care about 'become settings', much less take
part in manipulating module input.

Also note that while run_command is used by some modules, the vast
majority use APIs instead of running commands, so become is more
geared to running the modules themselves vs what the modules might
shell out to do. So while what you ask for is not w/o reason, it is a
niche use for a minority of modules and pushes against the desired
design for most other use cases.


----------
Brian Coca

[Raphaël]

> > Also note that while run_command is used by some modules, the vast
> majority use APIs instead of running commands, so become is more
> geared to running the modules themselves

[...]

> what you ask for is not w/o reason, it is a niche use for a minority of modules and pushes
> against the desired design for most other use cases.

I agree "it is a niche use for a minority of modules" [currently] but I disagree in that it
would "push against the desired design".

Although this discussion may slide a bit away from the `become` vs `action plugins`, please let me
question the "security" part of such a redesign based on "security".


1. *Granular permissions* are a fundamental aspect of security.

2. Currently, an Ansible task can *not* be run with elevated privileges without running the
whole python interpreter with elevated privileges even if the actual task was `git pull`
or `curl -o`

> I understand the request, but the push is to separate those concerns
> as both from a security and simplicity perspective, action plugins
> should not be able or care about 'become settings',

3. Given I come up with a prototype of an hardened Ansible task execution mechanism (for
some modules) the "concerns" are not so independent as one could believe from a naive
"code purity" standpoint.

3. And from a security perspective, more *granular* and *limited* privileges is clearly more
desirable and more straightforward than removing some (useful) variables from a python
function scope.
(The former is a tangible security gain while the later an hypothetical mean for such a goal)


In consequence, I don't believe the abstract security of an internal code-path refactor is
significant in comparison of the actual gain of running `composer`, `git` (and many more
actually) on a host in a *safe* way (instead of adding `/usr/bin/python3` to /etc/sudoers)


I believe this example is enough to limit the apparent "separation of concerns":
`action plugins` have a (proven) legitimacy to care about `become` settings

[Brian Coca]

A few things:

- It is not just the 'python interpreter', modules can be written in
any language or even be binaries, modules that 'ship with Ansible' are
Powershell and Python, but that is not a limitation of the engine,
which makes it even harder to deal with.

- As much as 'per command granular permissions' would be nice to have,
it is almost impossible given the nature of the dynamic payload and
the reliance on systemcalls other than shell

- I had a working branch with something close to what you desire, but
using 'sudo cached credential initiator' + sudo wrapper in
'run_command' , this did not work well and basically only covered
shell/command modules when the playbook author did exactly what was
expected from them by the fine grained permissions ... which they
could do just by sudo themselves (- shell: sudo ...) in the play
instead of using become, so it never went anywhere.

This is something we have been considering for years, and after many
attempts we have not found something worthwhile merging into core. I
would argue the closest we can get to this is doing something like the
mitogen project, running the become plugins themselves at the remote
via a temporary agent, but then this also limits us to python in many
respects and removes several abilities (changing
become/connection/interpreter settings per loop item, for example).
----------
Brian Coca