feat: Add Trustee quadlet and secret registration server#2
Open
litian1992 wants to merge 5 commits intolinux-system-roles:mainfrom
Open
feat: Add Trustee quadlet and secret registration server#2litian1992 wants to merge 5 commits intolinux-system-roles:mainfrom
litian1992 wants to merge 5 commits intolinux-system-roles:mainfrom
Conversation
Reviewer's GuideImplements the initial trustee_attestation_server Ansible role that deploys Trustee attestation components via Podman Quadlets and introduces a companion HTTPS secret registration service, wiring them into tests, metadata, and documentation while renaming template-era variables and references. Sequence diagram for secret registration flow via secret registration serversequenceDiagram
actor ClientCVM
participant SecretRegistrationServer
participant TokenParser as get_evidence_from_attestation_token
participant KBSClient as kbs_client_container
participant TrusteeKBS as KBS
participant PolicyFile as policy_rego
ClientCVM->>SecretRegistrationServer: POST /register-encryption-key\n{ attestation_token, client_id }
SecretRegistrationServer->>TokenParser: get_evidence_from_attestation_token(attestation_token)
TokenParser-->>SecretRegistrationServer: evidence (includes pcr15)
SecretRegistrationServer->>SecretRegistrationServer: Generate 32-byte disk key
SecretRegistrationServer->>KBSClient: podman run kbs-client\nset-resource --path disk-encryption/client_id/luks-key-0
KBSClient->>TrusteeKBS: HTTPS set-resource\n(auth.key, server.crt)
TrusteeKBS-->>KBSClient: 200 OK
KBSClient-->>SecretRegistrationServer: success
SecretRegistrationServer->>PolicyFile: append_resource_policy(resource_path, evidence, client_id)
PolicyFile-->>SecretRegistrationServer: policy updated
SecretRegistrationServer-->>ClientCVM: 200 OK\n{ resource_path, message }
Class diagram for SecretRegistrationHandler and helper functionsclassDiagram
class SecretRegistrationHandler {
+do_POST()
-_send_json(status int, data dict)
}
class get_evidence_from_attestation_token {
+get_evidence_from_attestation_token(token str) dict
}
class store_key_in_kbs {
+store_key_in_kbs(resource_path str, key_data bytes) bool
}
class append_resource_policy {
+append_resource_policy(resource_path str, evidence dict, client_id str) bool
}
class HTTPServer {
+serve_forever()
+server_close()
}
class main_entrypoint {
+main()
}
SecretRegistrationHandler ..> get_evidence_from_attestation_token : uses
SecretRegistrationHandler ..> store_key_in_kbs : uses
SecretRegistrationHandler ..> append_resource_policy : uses
main_entrypoint ..> HTTPServer : creates
HTTPServer ..> SecretRegistrationHandler : handles_requests
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![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 7 issues, and left some high level feedback:
- In templates/secret_registration_server.py.j2 there are several unguarded assumptions about the token/evidence structure and temp file handling (e.g. direct indexing into payload['submods']['cpu0'] and using tmp_path in finally even if creation failed) which can raise unexpected exceptions; consider using dict.get/try/except around the structure parsing and initializing tmp_path to None before the try so cleanup is safe.
- The example playbook in examples/simple.yml references the role as linux-system-roles.trustee-server whereas the rest of the repo uses linux-system-roles.trustee_attestation_server, which will confuse consumers; align the role name in the example with the actual published role name.
- The secret_registration_server systemd unit is configured with ProtectHome=yes and ReadWritePaths=/tmp, but the service writes to /etc/trustee (e.g. POLICY_FILE) and shells out to podman; review the hardening options so the service retains the required write and execution access to /etc/trustee and the container runtime paths.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In templates/secret_registration_server.py.j2 there are several unguarded assumptions about the token/evidence structure and temp file handling (e.g. direct indexing into payload['submods']['cpu0'] and using tmp_path in finally even if creation failed) which can raise unexpected exceptions; consider using dict.get/try/except around the structure parsing and initializing tmp_path to None before the try so cleanup is safe.
- The example playbook in examples/simple.yml references the role as linux-system-roles.trustee-server whereas the rest of the repo uses linux-system-roles.trustee_attestation_server, which will confuse consumers; align the role name in the example with the actual published role name.
- The secret_registration_server systemd unit is configured with ProtectHome=yes and ReadWritePaths=/tmp, but the service writes to /etc/trustee (e.g. POLICY_FILE) and shells out to podman; review the hardening options so the service retains the required write and execution access to /etc/trustee and the container runtime paths.
## Individual Comments
### Comment 1
<location path="templates/secret_registration_server.py.j2" line_range="47-56" />
<code_context>
+ try:
</code_context>
<issue_to_address>
**issue:** Avoid referencing tmp_path in finally if an exception occurs before it is assigned.
If an exception occurs before `tmp_path` is assigned (e.g. `NamedTemporaryFile` fails), the `finally` block will raise `UnboundLocalError` when calling `os.path.exists(tmp_path)`. Initialize `tmp_path = None` before the `try` and wrap the cleanup with `if tmp_path and os.path.exists(tmp_path):` to avoid this.
</issue_to_address>
### Comment 2
<location path="templates/secret_registration_server.py.j2" line_range="36" />
<code_context>
+
+def get_evidence_from_attestation_token(token: str) -> dict:
+ """Get PCR15 from attestation token."""
+ payload = json.loads(base64.b64decode(token.split('.')[1] + "==").decode('utf-8'))
+ evidence = payload["submods"]["cpu0"]["ear.veraison.annotated-evidence"]
+ # Only support Azure TPM-based attestation for now
</code_context>
<issue_to_address>
**issue (bug_risk):** JWT/base64 parsing in get_evidence_from_attestation_token can raise unhandled exceptions for malformed tokens.
This assumes `token.split('.')[1]` exists and that padding can always be fixed by appending `"=="`, so malformed/non-JWT tokens can raise `IndexError` or `binascii.Error` and become 500s instead of a controlled 4xx. Please validate the token has the expected number of segments and catch base64/JSON decoding errors (e.g., return `None` so the caller can send a 400).
</issue_to_address>
### Comment 3
<location path="templates/secret_registration_server.py.j2" line_range="94" />
<code_context>
+ if f"# machine-id: {client_id}" in f.read():
+ LOG.exception("Resource policy already exists for machine-id: %s", client_id)
+ return False
+ tee_key = list(evidence.keys())[0]
+ pcr15_val = evidence[tee_key]["tpm"]["pcr15"]
+ {% raw %}policy = f'''# machine-id: {client_id}
</code_context>
<issue_to_address>
**🚨 issue (security):** Selecting the first evidence key is brittle and may not match the TPM-based attestation actually used.
`get_evidence_from_attestation_token` explicitly selects `azsnpvtpm` / `aztdxvtpm`, but `append_resource_policy` then uses `list(evidence.keys())[0]`. If other keys are present, the policy may be bound to the wrong TEE. Consider returning the chosen `tee_key` from `get_evidence_from_attestation_token`, or reapplying the same selection logic here instead of relying on dictionary key order.
</issue_to_address>
### Comment 4
<location path="tasks/trustee_quadlet.yml" line_range="70" />
<code_context>
+ force: true
+ when: __repo_configs_dir.stat.exists
+
+- name: Generate certificates for all components
+ ansible.builtin.shell: |
+ # Trustee Server SSL
</code_context>
<issue_to_address>
**issue (bug_risk):** Certificate generation assumes /etc/trustee/kbs exists, which may not be true.
The shell block writes to `/etc/trustee/kbs/server.key` and `/etc/trustee/kbs/auth.key` before ensuring `/etc/trustee/kbs` exists. If the configs copy step didn’t create that directory (e.g. no `configs/` in the repo), these `openssl` commands will fail. Please ensure the directories are created first, either with `mkdir -p /etc/trustee/kbs /etc/trustee/as` in the shell script or via a preceding `file` task.
</issue_to_address>
### Comment 5
<location path="README.md" line_range="49" />
<code_context>
-
-## Example Playbook
+1. Downloads the Podman Quadlets from designated repo
+2. Generates all required certficates of Trustee server components
+3. Add KBS port 8080 to firewalld
+3. Enables the services by default
</code_context>
<issue_to_address>
**issue (typo):** Fix the typo in "certficates" and consider adjusting the preposition.
`certficates` → `certificates`. Also consider `certificates for Trustee server components` for clearer wording.
```suggestion
2. Generates all required certificates for Trustee server components
```
</issue_to_address>
### Comment 6
<location path="contributing.md" line_range="14-20" />
<code_context>
**Bugs and needed implementations** are listed on
-[Github Issues](https://github.com/linux-system-roles/template/issues).
+[Github Issues](https://github.com/linux-system-roles/trustee_attestation_server/issues).
Issues labeled with
-[**help wanted**](https://github.com/linux-system-roles/template/issues?q=is%3Aissue+is%3Aopen+label%3A%22help+wanted%22)
</code_context>
<issue_to_address>
**issue (typo):** Use the correct capitalization "GitHub" instead of "Github".
Please update the link text to use the official `GitHub` capitalization, e.g. `[GitHub Issues](...)`.
```suggestion
**Bugs and needed implementations** are listed on
[GitHub Issues](https://github.com/linux-system-roles/trustee_attestation_server/issues).
Issues labeled with
[**help wanted**](https://github.com/linux-system-roles/trustee_attestation_server/issues?q=is%3Aissue+is%3Aopen+label%3A%22help+wanted%22)
are likely to be suitable for new contributors!
**Code** is managed on [GitHub](https://github.com/linux-system-roles/trustee_attestation_server), using
```
</issue_to_address>
### Comment 7
<location path="contributing.md" line_range="15" />
<code_context>
are likely to be suitable for new contributors!
-**Code** is managed on [Github](https://github.com/linux-system-roles/template), using
+**Code** is managed on [Github](https://github.com/linux-system-roles/trustee_attestation_server), using
[Pull Requests](https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/about-pull-requests).
</code_context>
<issue_to_address>
**issue (typo):** Correct "Github" to "GitHub" in the link text.
Use `GitHub` with a capital H here to match the official name.
```suggestion
[GitHub Issues](https://github.com/linux-system-roles/trustee_attestation_server/issues).
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| def get_evidence_from_attestation_token(token: str) -> dict: | ||
| """Get PCR15 from attestation token.""" | ||
| payload = json.loads(base64.b64decode(token.split('.')[1] + "==").decode('utf-8')) |
There was a problem hiding this comment.
issue (bug_risk): JWT/base64 parsing in get_evidence_from_attestation_token can raise unhandled exceptions for malformed tokens.
This assumes token.split('.')[1] exists and that padding can always be fixed by appending "==", so malformed/non-JWT tokens can raise IndexError or binascii.Error and become 500s instead of a controlled 4xx. Please validate the token has the expected number of segments and catch base64/JSON decoding errors (e.g., return None so the caller can send a 400).
| if f"# machine-id: {client_id}" in f.read(): | ||
| LOG.exception("Resource policy already exists for machine-id: %s", client_id) | ||
| return False | ||
| tee_key = list(evidence.keys())[0] |
There was a problem hiding this comment.
🚨 issue (security): Selecting the first evidence key is brittle and may not match the TPM-based attestation actually used.
get_evidence_from_attestation_token explicitly selects azsnpvtpm / aztdxvtpm, but append_resource_policy then uses list(evidence.keys())[0]. If other keys are present, the policy may be bound to the wrong TEE. Consider returning the chosen tee_key from get_evidence_from_attestation_token, or reapplying the same selection logic here instead of relying on dictionary key order.
spetrosi
reviewed
Mar 11, 2026
litian1992
force-pushed
the
main
branch
2 times, most recently
from
a3c6ed0 to
b3c8785
Compare
The deployment of Trustee server includes quadlet and associated configurations. Signed-off-by: Li Tian <litian@redhat.com>
The secret registration server service is an HTTPS server. It receives only request from the counterpart secret registration client service. It creates a disk encryption key along with resource policy tied to PCR15 of the client. Signed-off-by: Li Tian <litian@redhat.com>
richm
reviewed
Mar 16, 2026
richm
reviewed
Mar 16, 2026
litian1992
force-pushed
the
main
branch
2 times, most recently
from
c8ae2c2 to
bf6d2de
Compare
richm
reviewed
Mar 17, 2026
richm
reviewed
Mar 18, 2026
richm
reviewed
Mar 18, 2026
richm
reviewed
Mar 18, 2026
Contributor
|
there are a couple of files that do not end in a newline |
Integration tests for trustee_quadlet and secret_registration_server. Signed-off-by: Li Tian <litian@redhat.com>
Contributor
|
lgtm - @spetrosi ? |
Contributor
|
@litian1992 @spetrosi are we going to rename this to trustee_server as with client? |
richm
reviewed
Mar 26, 2026
…s to private We don't want users to easily move away from designated Trustee quadlets. So move the repo and install path to private to reduce variants. Signed-off-by: Li Tian <litian@redhat.com>
Collaborator
Author
Yes, intended. Should I just do it in this PR? |
Contributor
Yes, since this PR is already doing the rename of template -> trustee_server |
The present name 'trustee_attestation_server' is too long. The keyword attestation is not obviously reflected in the role. Signed-off-by: Li Tian <litian@redhat.com>
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.
Enhancement:
Trustee attestation server includes components AS, KBS and RVPS. Its deployment is via Podman Quadlet.
Secret registration server serves as a companion to Trustee server that pushes secrets and updates the policies.
This way Trustee server can serve each CVM as individuals.
Reason:
Result:
Issue Tracker Tickets (Jira or BZ if any):
Summary by Sourcery
Add a dedicated trustee_attestation_server Ansible role that deploys Trustee attestation components via Podman Quadlets and an optional secret registration server for disk encryption key management.
New Features:
Enhancements:
Documentation:
Tests: