/functional/agent-resilience-and-reattestation #923
Merged
Conversation
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
Reviewer's GuideThis PR implements support for PUSH-model attestation by extending the test-helper library, adapting existing tests to parameterize agent service types, adding systemd overrides for the push-model agent, updating IMA configuration tests, and introducing new functional suites for agent resilience and push-attestation on localhost. Sequence diagram for PUSH-model attestation interactionsequenceDiagram
actor User
participant Agent
participant Verifier
User->>Agent: Start agent with push-model
Agent->>Verifier: Send attestation data (push)
Verifier->>Agent: Acknowledge attestation
Agent-->>Verifier: Reattest on failure/restart
Class diagram for test-helper library and agent service parameterizationclassDiagram
class TestHelperLibrary {
+setupAgent(serviceType)
+runAttestationTest()
}
class AgentService {
+serviceType: string
+start()
+applySystemdOverride()
}
TestHelperLibrary --> AgentService : parameterizes
AgentService <|-- PushModelAgent
class PushModelAgent {
+startPushAttestation()
+reattestOnFailure()
}
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}
| @@ -0,0 +1,133 @@ | |||
| #!/bin/bash | |||
| # vim: dict+=/usr/share/beakerlib/dictionary.vim cpt=.,w,b,u,t,i,k | |||
| . /usr/share/beakerlib/beakerlib.sh || exit 1 | |||
Check warning
Code scanning / shellcheck
SC1091 Warning test
| rlRun "keylime_tenant -v 127.0.0.1 -t 127.0.0.1 -u $AGENT_ID --runtime-policy policy.json -c add" | ||
| rlRun "limeWaitForAgentStatus $AGENT_ID 'Get Quote'" | ||
| rlRun -s "keylime_tenant -c cvlist" | ||
| rlAssertGrep "{'code': 200, 'status': 'Success', 'results': {'uuids':.*'$AGENT_ID'" $rlRun_LOG -E |
Check warning
Code scanning / shellcheck
SC2154 Warning test
| rlRun "keylime_tenant -v 127.0.0.1 -t 127.0.0.1 -u $AGENT_ID --runtime-policy policy.json -c add" | ||
| rlRun "limeWaitForAgentStatus $AGENT_ID 'Get Quote'" | ||
| rlRun -s "keylime_tenant -c cvlist" | ||
| rlAssertGrep "{'code': 200, 'status': 'Success', 'results': {'uuids':.*'$AGENT_ID'" $rlRun_LOG -E |
Check warning
Code scanning / shellcheck
SC2086 Warning test
| rlRun "keylime_tenant -v 127.0.0.1 -t 127.0.0.1 -u $AGENT_ID -c regdelete" | ||
| # verify agent is deleted | ||
| rlRun -s "keylime_tenant -c reglist" | ||
| rlAssertNotGrep "$AGENT_ID" $rlRun_LOG |
Check warning
Code scanning / shellcheck
SC2086 Warning test
|
|
||
| rlPhaseStartTest "Remove agent and re-add with updated policy" | ||
| # create a script that will be allowed | ||
| TESTDIR=`limeCreateTestDir` |
Check warning
Code scanning / shellcheck
SC2006 Warning test
| limeSubmitCommonLogs | ||
| limeClearData | ||
| limeRestoreConfig | ||
| limeExtendNextExcludelist $TESTDIR |
Check warning
Code scanning / shellcheck
SC2086 Warning test
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Consider centralizing the
push_model_agentservice name and TENANT_ARGS logic into a shared helper or variable so you don’t have to sprinkle the same conditional blocks across every test script. - The repeated appending of
${TENANT_ARGS}in tenant CLI invocations is error-prone—wrap your keylime_tenant calls in a helper function that automatically includes the correct flags for push mode. - The new functional tests are quite long and cover multiple scenarios; splitting them into smaller, FMF-parameterized cases would make them more readable and easier to maintain.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider centralizing the `push_model_agent` service name and TENANT_ARGS logic into a shared helper or variable so you don’t have to sprinkle the same conditional blocks across every test script.
- The repeated appending of `${TENANT_ARGS}` in tenant CLI invocations is error-prone—wrap your keylime_tenant calls in a helper function that automatically includes the correct flags for push mode.
- The new functional tests are quite long and cover multiple scenarios; splitting them into smaller, FMF-parameterized cases would make them more readable and easier to maintain.
## Individual Comments
### Comment 1
<location> `setup/bootc_configure_kernel_ima_module/Containerfile:8-10` </location>
<code_context>
COPY 10-ima_kargs.toml /usr/lib/bootc/kargs.d/10-ima_kargs.toml
COPY ima-policy /etc/ima/ima-policy
COPY yum.repos.d/* /etc/yum.repos.d/
+COPY .ssh /var/roothome/.ssh
+COPY resolv.conf /etc/resolv.conf
ARG KEYLIME_BOOTC_INSTALL_PACKAGES="rsync beakerlib selinux-policy-devel swtpm swtpm-tools nmap keylime expect"
</code_context>
<issue_to_address>
**🚨 suggestion (security):** Consider restricting permissions on the copied .ssh directory.
Explicitly set permissions on the .ssh directory and private keys after copying to prevent unauthorized access.
```suggestion
COPY .ssh /var/roothome/.ssh
RUN chmod 700 /var/roothome/.ssh && \
find /var/roothome/.ssh -type f -name "id_*" -exec chmod 600 {} \; && \
chown -R root:root /var/roothome/.ssh
COPY resolv.conf /etc/resolv.conf
ARG KEYLIME_BOOTC_INSTALL_PACKAGES="rsync beakerlib selinux-policy-devel swtpm swtpm-tools nmap keylime expect"
```
</issue_to_address>
### Comment 2
<location> `setup/bootc_configure_kernel_ima_module/Containerfile:9` </location>
<code_context>
COPY ima-policy /etc/ima/ima-policy
COPY yum.repos.d/* /etc/yum.repos.d/
+COPY .ssh /var/roothome/.ssh
+COPY resolv.conf /etc/resolv.conf
ARG KEYLIME_BOOTC_INSTALL_PACKAGES="rsync beakerlib selinux-policy-devel swtpm swtpm-tools nmap keylime expect"
RUN dnf -y install ${KEYLIME_BOOTC_INSTALL_PACKAGES}
</code_context>
<issue_to_address>
**question (bug_risk):** Overwriting /etc/resolv.conf may interfere with container DNS resolution.
If you must overwrite resolv.conf, verify its contents are suitable for all deployment environments and do not conflict with container runtime DNS management.
</issue_to_address>
### Comment 3
<location> `setup/bootc_configure_kernel_ima_module/Containerfile:13` </location>
<code_context>
ARG KEYLIME_BOOTC_INSTALL_PACKAGES="rsync beakerlib selinux-policy-devel swtpm swtpm-tools nmap keylime expect"
RUN dnf -y install ${KEYLIME_BOOTC_INSTALL_PACKAGES}
RUN sed -i '/tss/ d' /usr/lib/group; ls -ld /etc/keylime /var/lib/keylime; ls -l /etc/keylime /var/lib/keylime; /bin/true
+RUN restorecon -Rv /usr /etc; /bin/true
</code_context>
<issue_to_address>
**suggestion:** Running restorecon recursively on /usr and /etc may have unintended side effects.
Limit restorecon to only the files or directories changed during the build to avoid unnecessary processing and unintended changes.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
8
to
10
| COPY .ssh /var/roothome/.ssh | ||
| COPY resolv.conf /etc/resolv.conf | ||
| ARG KEYLIME_BOOTC_INSTALL_PACKAGES="rsync beakerlib selinux-policy-devel swtpm swtpm-tools nmap keylime expect" |
There was a problem hiding this comment.
🚨 suggestion (security): Consider restricting permissions on the copied .ssh directory.
Explicitly set permissions on the .ssh directory and private keys after copying to prevent unauthorized access.
Suggested change
| COPY .ssh /var/roothome/.ssh | |
| COPY resolv.conf /etc/resolv.conf | |
| ARG KEYLIME_BOOTC_INSTALL_PACKAGES="rsync beakerlib selinux-policy-devel swtpm swtpm-tools nmap keylime expect" | |
| COPY .ssh /var/roothome/.ssh | |
| RUN chmod 700 /var/roothome/.ssh && \ | |
| find /var/roothome/.ssh -type f -name "id_*" -exec chmod 600 {} \; && \ | |
| chown -R root:root /var/roothome/.ssh | |
| COPY resolv.conf /etc/resolv.conf | |
| ARG KEYLIME_BOOTC_INSTALL_PACKAGES="rsync beakerlib selinux-policy-devel swtpm swtpm-tools nmap keylime expect" |
| COPY ima-policy /etc/ima/ima-policy | ||
| COPY yum.repos.d/* /etc/yum.repos.d/ | ||
| COPY .ssh /var/roothome/.ssh | ||
| COPY resolv.conf /etc/resolv.conf |
There was a problem hiding this comment.
question (bug_risk): Overwriting /etc/resolv.conf may interfere with container DNS resolution.
If you must overwrite resolv.conf, verify its contents are suitable for all deployment environments and do not conflict with container runtime DNS management.
| ARG KEYLIME_BOOTC_INSTALL_PACKAGES="rsync beakerlib selinux-policy-devel swtpm swtpm-tools nmap keylime expect" | ||
| RUN dnf -y install ${KEYLIME_BOOTC_INSTALL_PACKAGES} | ||
| RUN sed -i '/tss/ d' /usr/lib/group; ls -ld /etc/keylime /var/lib/keylime; ls -l /etc/keylime /var/lib/keylime; /bin/true | ||
| RUN restorecon -Rv /usr /etc; /bin/true |
There was a problem hiding this comment.
suggestion: Running restorecon recursively on /usr and /etc may have unintended side effects.
Limit restorecon to only the files or directories changed during the build to avoid unnecessary processing and unintended changes.
Collaborator
Author
|
/packit test |
kkaarreell
force-pushed
the
ks_agent_resilience
branch
2 times, most recently
from
4fdc219 to
edd3558
Compare
Collaborator
Author
|
/packit test |
kkaarreell
force-pushed
the
ks_agent_resilience
branch
2 times, most recently
from
2f26d48 to
c44983b
Compare
Collaborator
Author
|
/packit test |
kkaarreell
force-pushed
the
ks_agent_resilience
branch
from
c44983b to
57e0dcd
Compare
kkaarreell
force-pushed
the
ks_agent_resilience
branch
from
89b3ff7 to
1f9c1c2
Compare
kkaarreell
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
Support the new push-model attestation agent across the test framework, parameterize tests and services for push vs standard agents, and add dedicated tests for agent resilience, reattestation, and push-attestation scenarios.
New Features:
Enhancements:
Tests: