Merge lp://staging/~zyga/checkbox/path-bug into lp://staging/checkbox

Proposed by Zygmunt Krynicki
Status: Merged
Approved by: Sylvain Pineau
Approved revision: 2519
Merged at revision: 2516
Proposed branch: lp://staging/~zyga/checkbox/path-bug
Merge into: lp://staging/checkbox
Diff against target: 641 lines (+224/-66)
2 files modified
plainbox/plainbox/impl/ctrl.py (+120/-40)
plainbox/plainbox/impl/test_ctrl.py (+104/-26)
To merge this branch: bzr merge lp://staging/~zyga/checkbox/path-bug
Reviewer Review Type Date Requested Status
Sylvain Pineau (community) Approve
Review via email: mp+195637@code.staging.launchpad.net

Description of the change

6f44aa4 plainbox:ctrl: add tests for SymLinkNest class
06f1408 plainbox:ctrl: remove reference to extra_PATH
a482e65 plainbox:ctrl: fix get_differential_execution_environment
73b5948 plainbox:ctrl: refactor nest directory handling
f71746b plainbox:ctrl: fix how nest_dir is inserted into PATH
47c4ead plainbox:ctrl: fix incorrect docstrings
88fd09f plainbox:ctrl: use env trick with sudo controller
2a6f675 plainbox:ctrl: fix copy/paste docstring

This fixes three bugs that together contributed to the "path bug" staying unfixed in certain situations. Please have a thorough look at each patch and do real-world testing.

For reference, this is related to bug #1248894 and directly addresses bug #1252396

To post a comment you must log in.
lp://staging/~zyga/checkbox/path-bug updated
2512. By Zygmunt Krynicki

plainbox:ctrl: add tests for SymLinkNest class

Just missing tests for SymLinkNest class that was added last week.

Signed-off-by: Zygmunt Krynicki <email address hidden>

2513. By Zygmunt Krynicki

plainbox:ctrl: remove reference to extra_PATH

The extra_PATH attribute was removed from Provider1 class sometime ago,
there is no need to provide mocked values for it anymore.

Signed-off-by: Zygmunt Krynicki <email address hidden>

2514. By Zygmunt Krynicki

plainbox:ctrl: fix get_differential_execution_environment

This patch corrects one of the many faces of the 'path bug' (executables
not being in PATH in certain situations).

One aspect of that bug was how the "differential" environment, one that
was about to be passed to another process via the env(1) trick, was
being computed. Due to a simple mistake one of the cases where we wanted
to pass a modified environment variable was written incorrectly, thus
plainbox would not pass any changed but already existing variables
across privilege elevation mechanisms.

The fix is trivial, just compare the right variables.

Signed-off-by: Zygmunt Krynicki <email address hidden>

2515. By Zygmunt Krynicki

plainbox:ctrl: refactor nest directory handling

This patch changes how we construct the nest_dir and populate it with
symlinks to various executables. Instead of being a bit of code inside
execute_job() it is now a dedicated context manager,
configured_filesystem() that can be tested in isolation and, more
importantly, mocked away in other places.

Note that execute_job() still does mkdir() CHECKBOX_DATA but it is worth
considering moving all filesystem manipulations to the new context
manager later on.

Signed-off-by: Zygmunt Krynicki <email address hidden>

2516. By Zygmunt Krynicki

plainbox:ctrl: fix how nest_dir is inserted into PATH

This patch corrects one of the many faces of the 'path bug' (executables
not being in PATH in certain situations).

One aspect of that bug is how nest_dir (directory, that has symlinks to
executables we need in PATH) was being injected into PATH. Previously it
was being done in execute_job(). This fails for execution controllers
that pass environment using a trick in get_execution_command(), such as
pkexec and plainbox-trusted-launcher execution controllers. It fails
because those controllers need to pass "differential" environment inside
the command itself and the change occurs outside of
get_execution_environment().

The fix is straightforward, to move all PATH manipulations to happen
inside get_execution_environment() and adjust all tests that needed
adjusting.

Signed-off-by: Zygmunt Krynicki <email address hidden>

2517. By Zygmunt Krynicki

plainbox:ctrl: fix incorrect docstrings

This patch is seemingly useless as the docstring and associated
funcionality is being replaced by subsequent patch but for completeness
I'd like to properly document how the sudo execution controller used to
work.

Signed-off-by: Zygmunt Krynicki <email address hidden>

2518. By Zygmunt Krynicki

plainbox:ctrl: use env trick with sudo controller

This patch corrects one of the many faces of the 'path bug' (executables
not being in PATH in certain situations).

One aspect of that bug is how that 'sudo -E' which was being used in the
sudo execution controller is especially sensitive to ignore PATH even if
-E is used. Instead it loads the "secure path" from system wide
configuration file. This feature would obviously erase our customized
PATH and thus jobs using sudo this way would fail to see nest_dir with
all the executables.

The fix is to bring the sudo controller in alignment with pkexec
controller that already uses env(1) to pass arbitrary environment
variables into a program executed with sudo (without the -E flag this
time).

Signed-off-by: Zygmunt Krynicki <email address hidden>

2519. By Zygmunt Krynicki

plainbox:ctrl: fix copy/paste docstring

Signed-off-by: Zygmunt Krynicki <email address hidden>

Revision history for this message
Sylvain Pineau (sylvain-pineau) wrote :

Code looks good. Tested ok (w and w/o the fix).

Approved!

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches