Skip to content

Move reusable code from keylime-agent to the keylime library #1018

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

Merged
merged 10 commits into from
Jun 10, 2025

Conversation

ansasaki
Copy link
Contributor

@ansasaki ansasaki commented Jun 4, 2025

This moves the code that can be used by other applications, such as the push-attestation prototype, to the keylime library.

Copy link

codecov bot commented Jun 4, 2025

Codecov Report

Attention: Patch coverage is 63.28671% with 105 lines in your changes missing coverage. Please review.

Project coverage is 62.10%. Comparing base (07167a0) to head (c0ed8f1).
Report is 10 commits behind head on master.

Files with missing lines Patch % Lines
keylime/src/permissions.rs 68.85% 38 Missing ⚠️
keylime/src/context_info.rs 42.30% 30 Missing ⚠️
keylime/src/error.rs 10.00% 9 Missing ⚠️
keylime/src/config/base.rs 70.37% 8 Missing ⚠️
keylime-agent/src/main.rs 0.00% 6 Missing ⚠️
keylime-agent/src/payloads.rs 58.33% 5 Missing ⚠️
keylime/src/config/env.rs 87.50% 3 Missing ⚠️
keylime/src/config/push_model.rs 57.14% 3 Missing ⚠️
keylime/src/version.rs 33.33% 2 Missing ⚠️
keylime/src/json_wrapper.rs 94.44% 1 Missing ⚠️
Additional details and impacted files
Flag Coverage Δ
e2e-testsuite 62.10% <63.28%> (-7.83%) ⬇️
upstream-unit-tests 62.10% <63.28%> (-7.83%) ⬇️

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

Files with missing lines Coverage Δ
keylime-agent/src/agent_handler.rs 58.69% <ø> (-2.18%) ⬇️
keylime-agent/src/api.rs 77.38% <ø> (ø)
keylime-agent/src/errors_handler.rs 49.48% <100.00%> (-1.04%) ⬇️
keylime-agent/src/keys_handler.rs 65.45% <ø> (ø)
keylime-agent/src/notifications_handler.rs 66.66% <ø> (ø)
keylime-agent/src/quotes_handler.rs 51.73% <ø> (ø)
keylime-agent/src/revocation.rs 70.10% <ø> (ø)
keylime-push-model-agent/src/main.rs 80.43% <ø> (+1.08%) ⬆️
keylime-push-model-agent/src/registration.rs 87.50% <ø> (ø)
keylime-push-model-agent/src/struct_filler.rs 89.02% <ø> (ø)
... and 15 more
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ansasaki
Copy link
Contributor Author

ansasaki commented Jun 4, 2025

The tests need to be updated to allow debug messages coming from the library. I proposed the PR RedHat-SP-Security/keylime-tests#779 making that change.

I temporarily changed the tests to use that PR code to confirm it is the only required change

@ansasaki ansasaki marked this pull request as draft June 4, 2025 15:00
@ansasaki
Copy link
Contributor Author

ansasaki commented Jun 4, 2025

/packit retest-failed

@ansasaki
Copy link
Contributor Author

ansasaki commented Jun 5, 2025

The updated tests were merged and I dropped the temporary commit changing the location of the tests.
This is ready for review.

@ansasaki ansasaki marked this pull request as ready for review June 5, 2025 07:58
@ansasaki
Copy link
Contributor Author

ansasaki commented Jun 6, 2025

@sergio-correia @sarroutbi I think the coverage for the error.rs is not correct. I would say you can ignore it for now. We should aim for removing the errors.rs file completely as part of the refactoring

@@ -0,0 +1,210 @@
// SPDX-License-Identifier: Apache-2.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Is file header necessary? I have not included it in the latest changes but I doubt if it should be added or not. If so, I will create an additional PR for the ones that don't contain it

@@ -0,0 +1,87 @@
use serde::{Deserialize, Serialize};
Copy link
Contributor

Choose a reason for hiding this comment

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

I would try to homogeneize files and and/remove license header

Copy link
Contributor

@sarroutbi sarroutbi left a comment

Choose a reason for hiding this comment

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

Minor comments regarding file headers (we should homogeneize them, but it can be done in a different PR). Rest of changes LGTM

ansasaki added 2 commits June 10, 2025 11:00
Move the permissions code to the library as the permissions module.

Signed-off-by: Anderson Toshiyuki Sasaki <[email protected]>
Signed-off-by: Anderson Toshiyuki Sasaki <[email protected]>
ansasaki added 8 commits June 10, 2025 11:18
The following changes were made:
 - Moved keylime/src/global_config.rs to keylime/src/config/errors.rs
 - Moved keylime-agent/src/config.rs to keylime/src/config/base.rs
 - Moved the EnvConfig structure to the dedicated file
   keylime/src/config/env.rs
 - Moved the SUPPORTED_API_VERSIONS value from keylime-agent/src/api.rs
   to keylime/src/config/base.rs
 - Added temporary values for DEFAULT_PUSH_API_VERSIONS and
   DEFAULT_PUSH_EK_HANDLE in keylme/src/config/push_model.rs
 - Modified other files as necessary

Signed-off-by: Anderson Toshiyuki Sasaki <[email protected]>
Add specific error type ContextInfoError for the context_info
module and remove the usage of expect() to avoid panic.

Signed-off-by: Anderson Toshiyuki Sasaki <[email protected]>
The structures APIVersion defined in common.rs was equivalent to the
Version structure defined in the library.

Remove APIVersion in favor of the Version structure from the library.

Signed-off-by: Anderson Toshiyuki Sasaki <[email protected]>
Move the tests related to the AgentData structure from the
keylime-agent/src/common.rs to keylime/src/agent_data.rs

Signed-off-by: Anderson Toshiyuki Sasaki <[email protected]>
Move the JsonWrapper structure from common.rs to the library,
effectively making the common.rs empty.

Remove the common.rs file.

Signed-off-by: Anderson Toshiyuki Sasaki <[email protected]>
Signed-off-by: Anderson Toshiyuki Sasaki <[email protected]>
Signed-off-by: Anderson Toshiyuki Sasaki <[email protected]>
@ansasaki
Copy link
Contributor Author

@sarroutbi Hi, could you please take a look again? I added a specific error type ContextInfoError to remove the expect() usage in the code. I overlooked it during my review of your code, but please avoid using unwrap() or expect() in the code as it will generate panic which kills the agent. It is acceptable to use them as part of the tests, but not as part of the actual code. I know we still have some of them spread around, but ideally we should remove all of them from the code.

Copy link
Contributor

@sarroutbi sarroutbi left a comment

Choose a reason for hiding this comment

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

LGTM

@ansasaki ansasaki merged commit 4f0360d into keylime:master Jun 10, 2025
11 of 12 checks passed
@ansasaki ansasaki deleted the lib_config branch June 10, 2025 14:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants