ctc-oss / fapolicy-analyzer

Tools to assist with the configuration and management of fapolicyd.
https://ctc-oss.github.io/fapolicy-analyzer
GNU General Public License v3.0
12 stars 5 forks source link

Expand env for profiler args #856

Closed tparchambault closed 1 year ago

tparchambault commented 1 year ago

Expand environment vars in the profiler args prior to dispatch.

Closes #850

jw3 commented 1 year ago

What's the status of this one?

tparchambault commented 1 year ago

I’ve got to write some new tests and rewrite some of the existing tests. Not much time is required IMO. Otherwise, functionally fine wrt $PATH expansion.

Decided not to add $USER and $HOME expansion because would not add much to the end-user UX. Typing /home/someuser/bin vs $HOME/bin is not too much of an inconvenience.

jw3 commented 1 year ago

Decided not to add $USER and $HOME expansion because would not add much to the end-user UX. Typing /home/someuser/bin vs $HOME/bin is not too much of an inconvenience.

I could see these being useful. Say if you were profiling successive runs against different users, the HOME var expansion would prevent the need to update the /home/someuser/bin param.

That could be added as a separate feature, no need to put it in this bug fix.

tparchambault commented 1 year ago

I could see these being useful. Say if you were profiling successive runs against different users, the HOME var expansion would prevent the need to update the /home/someuser/bin param.

Fair enough. I can see that use-case improvement.

tparchambault commented 1 year ago

Refactored the UI level profiler code. Removed faprofiler.py and associated testing. Moved its functionality into profiler_page.py. I believe we are functionally good; Consolidated expansions and path searching. Still need to revisit the unit tests, such that they are meaningful, and migrate some that were in test_faprofiler.py.

jw3 commented 1 year ago

Is this gtg now?

A couple comments below after review.

jw3 commented 1 year ago

@tparchambault

  1. Run profiler with ls executable, other fields empty
  2. After profiling completed switch away from profiler and then return
  3. Observe the Working Directory is set to /root
tparchambault commented 1 year ago

@tparchambault

  1. Run profiler with ls executable, other fields empty
  2. After profiling completed switch away from profiler and then return
  3. Observe the Working Directory is set to /root

This is good. Thanks for capturing this use-case. I'll look into today when I address the above.

tparchambault commented 1 year ago

@tparchambault

  1. Run profiler with ls executable, other fields empty
  2. After profiling completed switch away from profiler and then return
  3. Observe the Working Directory is set to /root

This is good. Thanks for capturing this use-case. I'll look into today when I address the above.

Duplicated on FC38 Testbed w/fapolicy-analyzer installed via rpm.

tparchambault commented 1 year ago

@jw3 (V1.0.2-1, observed on FC38, RHEL8.6) With the user field containing a valid user, I am seeing the working directory field populated in the Profiler Page UI form after a Profiler Run followed by user navigation to another tool's view and then a return back to the Profiler Page view. I'm incorrectly performing my Profiler arg variable expansions/modifications (e.g. setting defaults) on the dictionary elements that ultimately are being stored in Redux and used to refresh the Profiler form data upon returning to the Profiler view. I need to revisit the arg handling from just prior to the Profiler Page dispatch to the PyO/Rust interface that invokes the Profiling instance of fapolicyd

Note: The following images were captured on a RHEL8.6 platform. The original Profiler page view, post-Profiling run, pre-user navigation to the Trust page:

profiler_toma_pwd_null

User-navigation back to the Profiler page view, shows that setting the default working directory to that of the specified user results in the population of the Working Directory UI field:

profiler_toma_post_nav_to_trust

jw3 commented 1 year ago

(V1.0.2-1, observed on FC38, RHEL8.6) With the user field containing a valid user, I am seeing the working directory field populated in the Profiler Page UI form after a Profiler Run followed by user navigation to another tool's view and then a return back to the Profiler Page view

Note that This is slightly different than the issue that I reported above.

There appears to be an additional regression in this PR where the working directory becomes populated whether you specify a user or not.

If it were just the existing bug from 1.0.2, I'd merge it and say fix the old one in another PR, but since the PR has an issue too it should be fixed before merge.

tparchambault commented 1 year ago

NP, and understood. The root cause is essentially the same. Modifying the raw form data directly prior to it getting stored in Redux; I just did it more in this PR, e.g. you'll similarly see the expanded variables (if you use $USER or $HOME) in the env var field of the Profiler Page if you navigate away and then return.

tparchambault commented 1 year ago

Tested on FC38. PR addresses the use of $USER and $HOME macros in the env and pwd fields. Works as expected. Navigating away from the Profiler Page and returning (after a profiler run) maintains the users originally entered text.

Observed anomalies/idiosyncrasies :

  1. Environment variables entered on the Profiler Page are appended to the existing pkexec environment set which are root user-centric, the default $USER=root, and $HOME=/root (i.e. the root user's HOME dir.)
  2. $HOME and $USER are not expanded for the arguments field. At this point, that would be a simple modification.