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

Adding "mutable log paths" feature #665

Merged
merged 2 commits into from
Jan 30, 2024

Conversation

galmasi
Copy link

@galmasi galmasi commented Oct 2, 2023

As discussed in Issue #657 -- initial commit for implementing configurable logging paths

@galmasi
Copy link
Author

galmasi commented Oct 2, 2023

As mentioned above, please do not merge yet -- this is a trial balloon to see how many testing features I run afoul in one try. Also, the implementation is incomplete: the feature discussed in Issue #657 , info logging nonstandard log paths, is not yet implemented.

@codecov
Copy link

codecov bot commented Oct 2, 2023

Codecov Report

Attention: 13 lines in your changes are missing coverage. Please review.

Comparison is base (63bb12d) 65.21% compared to head (f56bd27) 65.28%.

Additional details and impacted files
Flag Coverage Δ
e2e-testsuite 59.15% <77.55%> (+0.13%) ⬆️
upstream-unit-tests 52.30% <83.33%> (+0.22%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
keylime-agent/src/common.rs 67.70% <ø> (+1.03%) ⬆️
keylime-agent/src/crypto.rs 77.93% <ø> (ø)
keylime-agent/src/main.rs 62.99% <42.85%> (-0.36%) ⬇️
keylime-agent/src/config.rs 78.40% <79.54%> (-0.34%) ⬇️

@galmasi galmasi changed the title [DO NOT MERGE] Adding "mutable log paths" feature Adding "mutable log paths" feature Oct 4, 2023
@galmasi
Copy link
Author

galmasi commented Oct 9, 2023

@ansasaki @kkaarreell I do not believe I can improve the rust agent code in this PR anymore. I have been laboring under a misunderstanding of how rust agent testing is performed. I have created this one-line [PR] (RedHat-SP-Security/keylime-tests#498) in keylime-tests to help pass this PR.

Note that the testing feature in the keylime agent is not actually used for measured boot in the test framework; instead, the test framework directly seds the source code. This raises the issue that either

(a) the testing harness should be updated to compile the rust agent correctly by using cargo with the --feature testing parameters; -- or --
(b) let the SED command stay in the test code and remove the unused testing feature of the keylime agent.

@ansasaki
Copy link
Contributor

ansasaki commented Oct 10, 2023

@ansasaki @kkaarreell I do not believe I can improve the rust agent code in this PR anymore. I have been laboring under a misunderstanding of how rust agent testing is performed. I have created this one-line [PR] (RedHat-SP-Security/keylime-tests#498) in keylime-tests to help pass this PR.

Note that the testing feature in the keylime agent is not actually used for measured boot in the test framework; instead, the test framework directly seds the source code. This raises the issue that either

(a) the testing harness should be updated to compile the rust agent correctly by using cargo with the --feature testing parameters; -- or -- (b) let the SED command stay in the test code and remove the unused testing feature of the keylime agent.

This looks like a leftover. It should use the approach a and avoid changing the code. I'll try to get rid of that sed.

EDIT: BTW, the testing feature is used when the rust agent is installed from the RPM in Copr (which is always build with the testing feature enabled). Only in the cases it is build and installed during the tests (for code coverage measurement) it was not being used and the sed approach was applied.

@galmasi
Copy link
Author

galmasi commented Oct 10, 2023

I closed RedHat-SP-Security/keylime-tests#498 in the expectation that RedHat-SP-Security/keylime-tests#499 will solve this problem. Will force a re-test as soon as 499 is merged.

GNUmakefile Show resolved Hide resolved
keylime-agent/src/config.rs Outdated Show resolved Hide resolved
keylime-agent/src/main.rs Show resolved Hide resolved
@ansasaki
Copy link
Contributor

@galmasi Hi, is there anything blocking you here that I can help you with?

@galmasi
Copy link
Author

galmasi commented Jan 17, 2024

I'll take another look at all the changes I have made and try to reconcile. I have rebased to the current master, but there are a number of thinking errors that I need to address: chief among them, config_file_get_path(). I need to build a test case that shows it used wrong, then fix it.

@@ -515,6 +532,14 @@ impl Source for KeylimeConfig {
"agent_data_path".to_string(),
self.agent.agent_data_path.to_string().into(),
);
_ = m.insert(
Copy link
Contributor

Choose a reason for hiding this comment

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

So many _ seems a bit ugly. I wonder if this can help: rust-lang/rfcs#3092

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the pointer, but unfortunately we cannot use this (yet). It is still available only on nightly:
https://doc.rust-lang.org/std/collections/struct.HashMap.html#method.try_insert

@ansasaki ansasaki added the configuration Involves changes to configuration file format label Jan 29, 2024
@ansasaki
Copy link
Contributor

ansasaki commented Jan 29, 2024

Hi @galmasi, I took the liberty to make the formatting changes and rebase. I'd like to ask if I can also add the configuration change required (in the case, include the new configuration options and bump the version)

EDIT: we will also need to update the templates in the python side and bump the version there. If possible, we should do both before @maugustosilva creates the new tag for the monthly release.

George Almasi and others added 2 commits January 30, 2024 14:31
…ylime configuration.

Signed-off-by: George Almasi <gheorghe@us.ibm.com>
Signed-off-by: Anderson Toshiyuki Sasaki <ansasaki@redhat.com>
This introduces the 'ima_ml_path' and 'measuredboot_ml_path' options to
set the IMA measurement log and Measured Boot event log, respectively.

This also bumps the configuration version to 2.2 as new options were
added.

Signed-off-by: Anderson Toshiyuki Sasaki <ansasaki@redhat.com>
@ansasaki
Copy link
Contributor

I'll squash some commits and rebase. Note that I added a commit to add the new configuration options.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
configuration Involves changes to configuration file format
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants