Skip to content

Conversation

@sarroutbi
Copy link
Contributor

@sarroutbi sarroutbi commented Jul 1, 2025

This change introduces a new configuration option, disabled_signing_algorithms, to prevent the agent from using certain signing schemes, even if they are reported as supported by the TPM.

The primary motivation is to avoid issues with the ecschnorr algorithm, which, while technically supported by some TPMs, has been observed to cause rejection from Verifier.

@sarroutbi sarroutbi force-pushed the 202507011144-verifier-does-not-support-ecschnorr branch 2 times, most recently from 0473f96 to 167a222 Compare July 1, 2025 10:01
@sarroutbi sarroutbi marked this pull request as ready for review July 1, 2025 10:12
@sarroutbi sarroutbi requested a review from ansasaki July 1, 2025 10:12
@sarroutbi
Copy link
Contributor Author

/packit retest-failed

1 similar comment
@sarroutbi
Copy link
Contributor Author

/packit retest-failed

@codecov
Copy link

codecov bot commented Jul 1, 2025

Codecov Report

Attention: Patch coverage is 55.00000% with 9 lines in your changes missing coverage. Please review.

Project coverage is 59.51%. Comparing base (3d72de8) to head (2c49f95).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
keylime/src/context_info.rs 53.84% 6 Missing ⚠️
keylime-push-model-agent/src/main.rs 0.00% 3 Missing ⚠️
Additional details and impacted files
Flag Coverage Δ
e2e-testsuite 59.51% <55.00%> (-0.01%) ⬇️
upstream-unit-tests 59.51% <55.00%> (-0.01%) ⬇️

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

Files with missing lines Coverage Δ
keylime-push-model-agent/src/registration.rs 46.55% <ø> (ø)
keylime-push-model-agent/src/struct_filler.rs 27.09% <ø> (ø)
keylime/src/config/base.rs 86.72% <100.00%> (+0.81%) ⬆️
keylime/src/config/push_model.rs 60.00% <ø> (ø)
keylime-push-model-agent/src/main.rs 47.45% <0.00%> (-1.40%) ⬇️
keylime/src/context_info.rs 53.55% <53.84%> (+0.05%) ⬆️

... and 4 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@sarroutbi
Copy link
Contributor Author

sarroutbi commented Jul 1, 2025

@ansasaki, @sergio-correia : Please, forget about coverity data. If I run it locally with tarpaulin, I am getting 100% coverage in config/push_model, for example:

image

@sarroutbi
Copy link
Contributor Author

/packit retest-failed

@sarroutbi sarroutbi force-pushed the 202507011144-verifier-does-not-support-ecschnorr branch from 167a222 to 0bbb317 Compare July 2, 2025 14:18
@sarroutbi sarroutbi requested a review from sergio-correia July 2, 2025 15:25
@sarroutbi sarroutbi force-pushed the 202507011144-verifier-does-not-support-ecschnorr branch 5 times, most recently from 3050ce2 to ec559f7 Compare July 4, 2025 11:40
@sarroutbi sarroutbi requested review from ansasaki and removed request for ansasaki July 4, 2025 15:01
@ansasaki
Copy link
Contributor

ansasaki commented Jul 7, 2025

In my opinion, the wording should be changed from prohibited to disallowed or disabled. Prohibited sounds like there is something inherently wrong with the algorithm, like if it was broken, when actually we just want to not allow its usage.

Another option is to introduce something like enabled_*_algorithms to list all the enabled/allowed algorithms instead of having a list of rejected algorithms. In general having an explicit list of enabled algorithms is better than having to construct a list by taking the supported algorithms and removing the rejected algorithms. It makes it clear what is enabled in a single place, helping also auditing the code in future.

@sarroutbi
Copy link
Contributor Author

sarroutbi commented Jul 7, 2025

In my opinion, the wording should be changed from prohibited to disallowed or disabled. Prohibited sounds like there is something inherently wrong with the algorithm, like if it was broken, when actually we just want to not allow its usage.

Sure. I will change it.

Another option is to introduce something like enabled_*_algorithms to list all the enabled/allowed algorithms instead of having a list of rejected algorithms. In general having an explicit list of enabled algorithms is better than having to construct a list by taking the supported algorithms and removing the rejected algorithms. It makes it clear what is enabled in a single place, helping also auditing the code in future.

At this particular moment, I prefer to keep the "disabled" option, but I opened an issue to track this change

@sarroutbi sarroutbi force-pushed the 202507011144-verifier-does-not-support-ecschnorr branch from ec559f7 to 6e5b25f Compare July 7, 2025 09:11
@sarroutbi sarroutbi changed the title Add prohibited_signing_algorithms, avoid ecschnorr Add disallowed_signing_algorithms, avoid ecschnorr Jul 7, 2025
@sarroutbi sarroutbi force-pushed the 202507011144-verifier-does-not-support-ecschnorr branch 2 times, most recently from 3d8a8c9 to 3283a71 Compare July 7, 2025 14:40
Signed-off-by: Sergio Arroutbi <sarroutb@redhat.com>
@sarroutbi sarroutbi force-pushed the 202507011144-verifier-does-not-support-ecschnorr branch from 3283a71 to 2c49f95 Compare July 7, 2025 14:53
@ansasaki ansasaki merged commit 9654555 into keylime:master Jul 8, 2025
11 of 12 checks passed
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.

3 participants