[WIP] Introduce limeCtl wrapper over keylime_tenant and keylimectl - #1129
Open
kkaarreell wants to merge 13 commits into
Open
kkaarreell wants to merge 13 commits into
kkaarreell wants to merge 13 commits into
Conversation
Reviewer's GuideIntroduces File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="Library/test-helpers/lib.sh" line_range="1008-1010" />
<code_context>
+ esac
+
+ case "$_subcmd" in
+ agent) __limeCtlAgent "$@" ;;
+ policy) __limeCtlPolicy "$@" ;;
+ measured-boot) __limeCtlMeasuredBoot "$@" ;;
+ *)
+ echo "limeCtl: unknown subcommand '$_subcmd'" >&2
+ __limeCtlKtGlobal=()
+ return 1
+ ;;
+ esac
+ __limeCtlKtGlobal=()
+}
+
+__limeCtlAgent() {
</code_context>
<issue_to_address>
**issue (bug_risk):** The default `keylime_tenant` backend's exit status is discarded because `__limeCtlKtGlobal=()` runs after the handler and becomes the function's final command. `limeCtl` therefore returns success even when agent, policy, or measured-boot operations fail, causing `rlRun ... 1` checks and callers that depend on failure propagation to behave incorrectly.
**Triggers:** When `limeKeylimeTenant` returns nonzero while `limeCtlCommand` is unset or set to `keylime_tenant`.
**Suggested fix:** Save the handler status, clear `__limeCtlKtGlobal`, and return the saved status.
```suggestion
esac
local _status=$?
__limeCtlKtGlobal=()
return "$_status"
}
```
</issue_to_address>
### Comment 2
<location path="Library/test-helpers/lib.sh" line_range="956-959" />
<code_context>
+
+ while [ $# -gt 0 ]; do
+ case "$1" in
+ --verifier-ip) _vip="$2"; shift 2 ;;
+ --registrar-ip) _rip="$2"; shift 2 ;;
+ --verifier-port) _vport="$2"; shift 2 ;;
+ --registrar-port) _rport="$2"; shift 2 ;;
+ -*)
+ echo "limeCtl: unknown option '$1'" >&2
</code_context>
<issue_to_address>
**issue (bug_risk):** `--verifier-port` and `--registrar-port` are parsed and retained in `_vport` and `_rport`, but the default `keylime_tenant` translation only appends `-v` and `-r` for the IP values. The port options are silently ignored whenever the wrapper uses the default backend.
**Triggers:** When a caller uses `limeCtl --verifier-port` or `limeCtl --registrar-port` with `limeCtlCommand=keylime_tenant`, especially against non-default service ports.
**Suggested fix:** Translate the port values into the keylime_tenant arguments/configuration format, or reject them explicitly for the backend that cannot honor them.
</issue_to_address>
### Comment 3
<location path="Library/test-helpers/lib.sh" line_range="976-984" />
<code_context>
+ [ -n "$_rip" ] && __limeCtlKtGlobal+=(-r "$_rip")
+
+ case "$cmd" in
+ keylimectl)
+ case "$_subcmd" in
+ *)
+ local _g=()
+ [ -n "$_vip" ] && _g+=(--verifier-ip "$_vip")
+ [ -n "$_rip" ] && _g+=(--registrar-ip "$_rip")
+ [ -n "$_vport" ] && _g+=(--verifier-port "$_vport")
+ [ -n "$_rport" ] && _g+=(--registrar-port "$_rport")
+ keylimectl "${_g[@]}" "$_subcmd" "$@"
+ __limeCtlKtGlobal=()
+ return
</code_context>
<issue_to_address>
**issue (bug_risk):** With `limeCtlCommand=keylimectl`, every subcommand, including `registrar`, is passed directly to `keylimectl` because the inner case has only a wildcard branch. `keylimectl` has no registrar management commands, so a `limeCtl registrar ...` call fails instead of falling through to `limeKeylimeTenant` as required by the wrapper's migration behavior.
**Triggers:** When a caller uses the documented backend switch and invokes a registrar operation through `limeCtl`.
**Suggested fix:** Handle `registrar` before the keylimectl pass-through and dispatch it to the keylime_tenant registrar implementation.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 3 findings to address first, and a bad option translation could enroll or update an agent with the wrong attestation, measured-boot, or runtime policy, or remove it from the verifier; those verifier-side changes persist after the wrapper is reverted and require cleanup or re-enrollment. The broad migration also makes failures possible across many tests, although the impact appears bounded to the environments running these test helpers rather than production.
Blocking findings: Library/test-helpers/lib.sh:1010, Library/test-helpers/lib.sh:959, Library/test-helpers/lib.sh:984
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+976
to
+984
| keylimectl) | ||
| case "$_subcmd" in | ||
| *) | ||
| local _g=() | ||
| [ -n "$_vip" ] && _g+=(--verifier-ip "$_vip") | ||
| [ -n "$_rip" ] && _g+=(--registrar-ip "$_rip") | ||
| [ -n "$_vport" ] && _g+=(--verifier-port "$_vport") | ||
| [ -n "$_rport" ] && _g+=(--registrar-port "$_rport") | ||
| keylimectl "${_g[@]}" "$_subcmd" "$@" |
There was a problem hiding this comment.
issue (bug_risk): With limeCtlCommand=keylimectl, every subcommand, including registrar, is passed directly to keylimectl because the inner case has only a wildcard branch. keylimectl has no registrar management commands, so a limeCtl registrar ... call fails instead of falling through to limeKeylimeTenant as required by the wrapper's migration behavior.
Triggers: When a caller uses the documented backend switch and invokes a registrar operation through limeCtl.
Suggested fix: Handle registrar before the keylimectl pass-through and dispatch it to the keylime_tenant registrar implementation.
Collaborator
Author
|
FTR, tests with |
| rlRun -s "keylime_tenant -c cvlist" | ||
| rlAssertGrep "{'code': 200, 'status': 'Success', 'results': {'uuids':.*'$AGENT_ID'" $rlRun_LOG -E | ||
| rlRun -s "limeCtl agent list" | ||
| rlRun "limeAssertJsonField $rlRun_LOG code=200 status=Success uuids=$AGENT_ID" |
| rlRun -s "keylime_tenant -c cvlist" | ||
| rlAssertGrep "{'code': 200, 'status': 'Success', 'results': {'uuids':.*'$AGENT_ID'" $rlRun_LOG -E | ||
| rlRun -s "limeCtl agent list" | ||
| rlRun "limeAssertJsonField $rlRun_LOG code=200 status=Success uuids=$AGENT_ID" |
| rlRun -s "keylime_tenant -c cvlist" | ||
| rlAssertGrep "{'code': 200, 'status': 'Success', 'results': {'uuids':.*'$AGENT_ID'" $rlRun_LOG -E | ||
| rlRun -s "limeCtl agent list" | ||
| rlRun "limeAssertJsonField $rlRun_LOG code=200 status=Success uuids=$AGENT_ID" |
| rlRun -s "keylime_tenant -c cvlist" | ||
| rlAssertGrep "{'code': 200, 'status': 'Success', 'results': {'uuids':.*'$AGENT_ID'" $rlRun_LOG -E | ||
| rlRun -s "limeCtl agent list" | ||
| rlRun "limeAssertJsonField $rlRun_LOG code=200 status=Success uuids=$AGENT_ID" |
| rlRun -s "keylime_tenant -c cvlist" | ||
| rlAssertGrep "{'code': 200, 'status': 'Success', 'results': {'uuids':.*'$AGENT_ID'" $rlRun_LOG -E | ||
| rlRun -s "limeCtl agent list" | ||
| rlRun "limeAssertJsonField $rlRun_LOG code=200 status=Success uuids=$AGENT_ID" |
| rlRun -s "keylime_tenant -c cvlist" | ||
| rlAssertGrep "{'code': 200, 'status': 'Success', 'results': {'uuids':.*'${AGENT_ID}'" $rlRun_LOG -E | ||
| rlRun -s "limeCtl agent list" | ||
| rlRun "limeAssertJsonField $rlRun_LOG code=200 status=Success uuids=${AGENT_ID}" |
| rlRun -s "keylime_tenant -c cvlist" | ||
| rlAssertGrep "{'code': 200, 'status': 'Success', 'results': {'uuids':.*'$AGENT_ID'" "$rlRun_LOG" -E | ||
| rlRun -s "limeCtl agent list" | ||
| rlRun "limeAssertJsonField $rlRun_LOG code=200 status=Success uuids=$AGENT_ID" |
| rlRun -s "keylime_tenant -c cvlist" | ||
| rlAssertGrep "{'code': 200, 'status': 'Success', 'results': {'uuids':.*'$AGENT_ID'" $rlRun_LOG -E | ||
| rlRun -s "limeCtl agent list" | ||
| rlRun "limeAssertJsonField $rlRun_LOG code=200 status=Success uuids=$AGENT_ID" |
| rlRun "keylime_tenant -v 127.0.0.1 -t 127.0.0.1 -u $AGENT_ID -c delete" | ||
| rlRun "keylime_tenant -v 127.0.0.1 -t 127.0.0.1 -u $AGENT_ID --verify --runtime-policy policy.json --file /etc/hostname -c add" | ||
| rlRun -s "limeCtl agent list" | ||
| rlRun "limeAssertJsonField $rlRun_LOG code=200 status=Success uuids=$AGENT_ID" |
| rlRun -s "keylime_tenant -c cvlist" | ||
| rlAssertGrep "{'code': 200, 'status': 'Success', 'results': {'uuids':.*'$AGENT_ID'" $rlRun_LOG -E | ||
| rlRun -s "limeCtl agent list" | ||
| rlRun "limeAssertJsonField $rlRun_LOG code=200 status=Success uuids=$AGENT_ID" |
| for I in `seq $TIMEOUT`; do | ||
| limeTimeoutCommand $TIMEOUT "limeKeylimeTenant -c status -u $UUID" &> $OUTPUT | ||
| AGTSTATE=$(cat "$OUTPUT" | grep "^{" | tail -1 | jq -r ".[].${FIELD}") | ||
| limeTimeoutCommand $TIMEOUT "limeCtl agent status $UUID --verifier" &> $OUTPUT |
Introduce limeCtl() as a unified tenant management function whose argument style mirrors keylimectl CLI. When limeCtlCommand=keylimectl it passes calls straight through; the default keylime_tenant backend translates each subcommand to equivalent keylime_tenant flags. This also: - Migrates all test scripts from keylime_tenant to limeCtl calls - Adds limeAssertJsonField for JSON output validation - Replaces rlAssertGrep JSON checks with limeAssertJsonField - Switches status checks from operational_state to attestation_status - Extends limeUpdateConf to support keylimectl TOML config - Adjusts agent setup to build keylimectl - Adds keylimectl migration documentation and help dump
kkaarreell
force-pushed
the
ks_limeCtl
branch
from
September 1, 2026 10:59
8c38287 to
818a80c
Compare
Contributor
|
/packit test |
Introduce limePolicy wrapper that uses keylimectl policy syntax as the canonical interface and translates to keylime-policy when $limeCtlCommand is set to keylime_tenant (default). Migrate all non-dedicated test usages of keylime-policy to use the wrapper with long options to avoid short-flag collisions between the two tools. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
kkaarreell
force-pushed
the
ks_limeCtl
branch
from
September 4, 2026 07:16
ef0c89b to
4f43ff0
Compare
Explicitly back up /etc/keylime/keylimectl.conf so it is properly restored during test cleanup. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The generic limeUpdateConf code path writes unquoted values to all *.conf files in /etc/keylime. Since keylimectl.conf uses TOML format, unquoted string values (e.g. mode = push) cause keylimectl to fail with "invalid TOML value" parse errors. Exclude keylimectl.conf and keylimectl.conf.d/ from the generic find — these files are already handled by the dedicated TOML-aware "keylimectl" prefix path. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| FILES="$( find ${CONF_DIR} -name '*.conf' )" | ||
| # Exclude keylimectl config files — they use TOML format and are handled | ||
| # by the "keylimectl" prefix path above. | ||
| FILES="$( find ${CONF_DIR} -name '*.conf' ! -name 'keylimectl.conf' ! -path '*/keylimectl.conf.d/*' )" |
The sed range '/^{/,/^}/p' never terminates for single-line JSON
because the closing } is on the same line that starts with {. This
caused sed to capture everything to EOF, feeding log messages to jq
which failed with "Invalid numeric literal".
Branch on limeCtlCommand: keylime_tenant uses grep "^{" | tail -1
(the proven single-line approach), keylimectl keeps the sed range
for pretty-printed multi-line JSON.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
keylime_tenant agent list output only contains the inner results JSON
(e.g. {"uuids": [...]}) without the code/status wrapper that keylimectl
includes. Assert only on uuids which is the meaningful field present in
both backends.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
keylime_tenant requires a payload (-k, -f, or --cert) when --verify is used. These limeCtl calls had no payload, causing failures. The --verify flag remains covered by direct keylime_tenant tests that do provide payloads. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
kkaarreell
force-pushed
the
ks_limeCtl
branch
from
September 10, 2026 08:52
9978a9b to
033f2b4
Compare
Contributor
|
/packit test |
1 similar comment
Contributor
|
/packit test |
| rlRun "limeWaitForAgentRegistration ${AGENT_ID}" | ||
| # create allowlist and excludelist | ||
| limeCreateTestPolicy | ||
| rlRun "limePolicy generate runtime --base-policy policy.json --add-ima-signature-verification-key ${limeIMAPublicKey} --output policy-with-keys.json" |
kkaarreell
force-pushed
the
ks_limeCtl
branch
from
September 16, 2026 13:08
bde6cba to
75486ac
Compare
The --ima-key argument is not available in keylimectl. Instead, embed IMA signature verification keys directly into the runtime policy using limePolicy generate runtime --base-policy --add-ima-signature-verification-key. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The --mb-refstate argument is not supported by keylimectl agent add. Use --mb-policy instead to pass measured boot policy files. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
kkaarreell
force-pushed
the
ks_limeCtl
branch
from
September 17, 2026 13:06
75486ac to
e24e4e2
Compare
Contributor
|
/packit test |
2 similar comments
Contributor
|
/packit test |
Contributor
|
/packit test |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Introduce limeCtl as a unified, keylimectl-shaped interface for Keylime management and migrate the test suite to use it.
New Features:
Enhancements:
Build:
Documentation:
Tests:
Chores: