Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Active checks with dbus #566

Merged
merged 12 commits into from Jul 28, 2022
Merged

Active checks with dbus #566

merged 12 commits into from Jul 28, 2022

Conversation

jw3
Copy link
Member

@jw3 jw3 commented Jul 28, 2022

Eliminate the calls to systemctl for service status checks, using dbus instead. This removes one potential pinch point where deployed rules can limit the call to systemctl. A side-effect of this is that the monitoring function now works even in the case of fully locked down system (ie. only the deny+any+all+all rule).

This also aligns both stages of deployment, deployment and rollback. Those stages used to be handled differently, where initial deployment only was a pipe write to refresh trust, while rollback deployment was a full daemon reload. Then the pipe write went away to align rule and trust writing, but there were some straggling issues that were left behind. These changes align the backends for both modes and resolve issues where fixes were present in one mode but not the other.

Closes #565

@jw3 jw3 requested a review from tparchambault July 28, 2022 12:56
@jw3
Copy link
Member Author

jw3 commented Jul 28, 2022

@tparchambault see how these changes play with your testing

@jw3 jw3 changed the title Deployment and rollback updates Active checks with dbus Jul 28, 2022
@jw3 jw3 added the deployment label Jul 28, 2022
@jw3 jw3 added this to the 7 - Rule Administration milestone Jul 28, 2022
@jw3
Copy link
Member Author

jw3 commented Jul 28, 2022

@tparchambault eh hold off, there is another item to address

@tparchambault
Copy link
Contributor

tparchambault commented Jul 28, 2022

Since you committed some of your work 12 hrs ago and I put some observations into that other PR around that same time, did you see the following wrt timing, and did you intend for this change to also address it. I believe it will.

#560 (comment)

@jw3
Copy link
Member Author

jw3 commented Jul 28, 2022

Yep, this PR will address the timing issues too.

@jw3
Copy link
Member Author

jw3 commented Jul 28, 2022

@tparchambault OK, I just tested the latest on rhel, fapolicy-analyzer-0.0.0-1.255.ge34e2ed.el8.x86_64.rpm

See what you see with it.

@tparchambault
Copy link
Contributor

Clean FC34 with only emacs, fapolicyd, and the generated fapolicy-analyzer rpm associated with this PR. Things look good. Ordering of fapolicyd shutdown/start reported in the journal is correct, deployed changes to the ancillary dbase were good, deployment timeout rollback also good.

Copy link
Contributor

@tparchambault tparchambault left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm assuming the pyo3 daemon.rs deploy() throws exceptions lower down the stack on errors, i.e. from the calls it invokes and their children? I only saw this PyRuntimeError::new_err("Daemon unresponsive") in fn wait_for_daemon(target_state: State)

I'll dig around but for the sake of merging, if the answer is yes, we are good to go.

@jw3
Copy link
Member Author

jw3 commented Jul 28, 2022

@tparchambault

Daemon unresponsive

This was an indeed an issue. When fapolicyd was in a Failed state (eg timed out on the max secs), wait_for_daemon didnt consider Failed equivalent to Inactive. This is now fixed.

Take another look and see if it all still functions as expected.

@jw3
Copy link
Member Author

jw3 commented Jul 28, 2022

I am going to leave the stderr prints in there for the near term.

@tparchambault
Copy link
Contributor

Functionally, on a clean FC34 rpm install, no issues observed.

@jw3 jw3 merged commit 84cc324 into master Jul 28, 2022
@jw3 jw3 deleted the deployment/fixes branch July 28, 2022 20:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Service status checks through dbus
2 participants