feat: change name to trustee_client, add systemd-cryptenroll and allow kbs_cert by file path#18
Merged
Merged
Conversation
The present name 'trustee_attestation_client' is too long. The keyword attestation is not obviously reflected in the role. Signed-off-by: Li Tian <litian@redhat.com>
Reviewer's GuideRefactors the Ansible role from trustee_attestation_client to trustee_client across code, docs, CI, and tests, while adding TPM2-backed systemd-cryptenroll disk encryption support when the secret registration client is disabled and introducing support for providing the KBS certificate via either inline content or a file path, with corresponding tests and documentation updates. Sequence diagram for updated disk encryption flow with Secret Registration Client and systemd_cryptenrollsequenceDiagram
actor Admin
participant AnsibleController
participant trustee_client_role
participant SecretRegistrationClient
participant systemd_cryptenroll
participant TPM2
participant Cryptsetup
participant OS
Admin->>AnsibleController: Run playbook with trustee_client_encrypt_disk=true
AnsibleController->>trustee_client_role: Execute tasks/main.yml
trustee_client_role->>trustee_client_role: include_tasks encrypt_disk.yml
trustee_client_role->>trustee_client_role: Detect first unpartitioned disk
alt trustee_client_secret_registration_enabled=true
trustee_client_role->>SecretRegistrationClient: secret_registration_client.sh --fetch-key-to tempfile
SecretRegistrationClient-->>trustee_client_role: LUKS key written to tempfile
else trustee_client_secret_registration_enabled=false
trustee_client_role->>OS: type systemd-cryptenroll
OS-->>trustee_client_role: rc=0 if available
alt systemd-cryptenroll available
trustee_client_role->>OS: head -c 32 /dev/urandom | base64 > tempfile
OS-->>trustee_client_role: Random key stored in tempfile
end
end
trustee_client_role->>Cryptsetup: luksFormat --key-file tempfile disk_partition
Cryptsetup-->>trustee_client_role: LUKS container created
trustee_client_role->>Cryptsetup: open --key-file tempfile disk_partition encrypted_disk
Cryptsetup-->>trustee_client_role: /dev/mapper/encrypted_disk
trustee_client_role->>OS: mkfs.ext4 /dev/mapper/encrypted_disk
trustee_client_role->>OS: mkdir -p trustee_client_encrypt_disk_mount_point
trustee_client_role->>OS: mount /dev/mapper/encrypted_disk trustee_client_encrypt_disk_mount_point
alt trustee_client_secret_registration_enabled=false and systemd_cryptenroll available
trustee_client_role->>systemd_cryptenroll: Enroll TPM2 slot with unlock-key-file=tempfile
systemd_cryptenroll->>TPM2: Bind key to PCR7
TPM2-->>systemd_cryptenroll: TPM2 policy created
trustee_client_role->>systemd_cryptenroll: Wipe password slot
systemd_cryptenroll-->>trustee_client_role: LUKS updated
trustee_client_role->>OS: lsblk -dno UUID disk_partition
OS-->>trustee_client_role: UUID
trustee_client_role->>OS: Update /etc/crypttab with encrypted_disk UUID and tpm2-device=auto
trustee_client_role->>OS: Configure /etc/fstab and mount with x-systemd.requires=systemd-cryptsetup@encrypted_disk.service
end
trustee_client_role->>OS: Remove tempfile
OS-->>trustee_client_role: Key file deleted
trustee_client_role-->>AnsibleController: Disk encrypted and configured
AnsibleController-->>Admin: Playbook run complete
Flow diagram for updated task inclusion and disk encryption conditions in main roleflowchart TD
A_start["Start: tasks/main.yml"] --> B_quadlet["Include trustee_quadlet.yml\nwhen trustee_client_trustee_gc is true"]
B_quadlet --> C_secretReg["Include secret_registration_client.yml\nwhen trustee_client_secret_registration_enabled is true\nAND trustee_client_trustee_gc is true"]
C_secretReg --> D_encrypt["Include encrypt_disk.yml\nwhen trustee_client_encrypt_disk is true"]
B_quadlet --> D_encrypt
D_encrypt --> E_branch["Inside encrypt_disk.yml:\nIs there an unpartitioned disk?"]
E_branch -->|No| Z_end["End: no encryption performed"]
E_branch -->|Yes| F_checkSecretReg["Check trustee_client_secret_registration_enabled"]
F_checkSecretReg -->|true| G_useSecretReg["Use Secret Registration Client\nfetch key to tempfile"]
F_checkSecretReg -->|false| H_checkCryptenroll["Check for systemd-cryptenroll command"]
H_checkCryptenroll -->|Available| I_useTPM2["Generate random key to tempfile\nEncrypt disk and bind key via systemd-cryptenroll\nConfigure crypttab and fstab for auto-unlock"]
H_checkCryptenroll -->|Not available| J_encryptOnly["Encrypt and mount using key file\n(no TPM2 auto-unlock path)"]
G_useSecretReg --> K_encryptMount["Encrypt LUKS partition with fetched key\nand mount encrypted_disk"]
I_useTPM2 --> K_encryptMount
J_encryptOnly --> K_encryptMount
K_encryptMount --> L_cleanup["Remove key tempfile"]
L_cleanup --> Z_end
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 2 issues, and left some high level feedback:
- In
encrypt_disk.yml, whentrustee_client_secret_registration_enabledis false andsystemd-cryptenrollis not available, the role still partitions, encrypts, and mounts the disk without configuring any auto-unlock mechanism; consider skipping disk encryption entirely in this case (or failing early with a clear message) to avoid leaving an unusable encrypted volume. - The
systemd-cryptenrollpresence check usescommand: type systemd-cryptenroll, which relies on shell built-ins and may be less portable; usingstaton a known path orcommand: systemd-cryptenroll --version(and checkingrc) would be more robust and explicit.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `encrypt_disk.yml`, when `trustee_client_secret_registration_enabled` is false and `systemd-cryptenroll` is not available, the role still partitions, encrypts, and mounts the disk without configuring any auto-unlock mechanism; consider skipping disk encryption entirely in this case (or failing early with a clear message) to avoid leaving an unusable encrypted volume.
- The `systemd-cryptenroll` presence check uses `command: type systemd-cryptenroll`, which relies on shell built-ins and may be less portable; using `stat` on a known path or `command: systemd-cryptenroll --version` (and checking `rc`) would be more robust and explicit.
## Individual Comments
### Comment 1
<location path="tasks/encrypt_disk.yml" line_range="47-52" />
<code_context>
+ when: trustee_client_secret_registration_enabled | bool
+ no_log: true
+
+ - name: Check systemd-cryptenroll command exists
+ ansible.builtin.command: type systemd-cryptenroll
+ register: __trustee_client_cryptenroll_check
+ changed_when: false
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Using `type systemd-cryptenroll` may be shell-dependent; consider a more portable existence check.
`ansible.builtin.command` runs via `/bin/sh`, and `type` isn’t portable across all shells or may not be available as a builtin, which can cause false negatives or different rc/output handling. Prefer a more portable check such as `command -v systemd-cryptenroll` or a direct path check with `stat` on `/usr/bin/systemd-cryptenroll` (or the expected location).
```suggestion
no_log: true
- name: Check systemd-cryptenroll command exists
ansible.builtin.command: command -v systemd-cryptenroll
register: __trustee_client_cryptenroll_check
changed_when: false
```
</issue_to_address>
### Comment 2
<location path="README.md" line_range="72" />
<code_context>
1. Finds the first unpartitioned and unmounted disk
-2. Requests disk encryption key from Secret Registration Client
-3. Encrypts the disk using above encryption key and mounts it at the designated path
+3. Encrypts the disk using key either from:
+ a. secret key fetched using Secret Registration Client (when enabled), or
+ b. `systemd-cryptenroll` which binds to PCR 7
</code_context>
<issue_to_address>
**nitpick (typo):** Minor grammar issue in "Encrypts the disk using key either from".
Consider rephrasing to: "Encrypts the disk using a key from either:" or "Encrypts the disk using the key from either:".
```suggestion
3. Encrypts the disk using a key from either:
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
litian1992
force-pushed
the
main
branch
2 times, most recently
from
e337166 to
6f8787a
Compare
richm
reviewed
Mar 19, 2026
richm
reviewed
Mar 19, 2026
richm
reviewed
Mar 19, 2026
richm
reviewed
Mar 19, 2026
Collaborator
|
@spetrosi are we going to rename the repo also? |
litian1992
force-pushed
the
main
branch
3 times, most recently
from
c446cfd to
c3ff7b5
Compare
richm
reviewed
Mar 21, 2026
Collaborator
|
lgtm - @spetrosi ? |
Contributor
|
[citest] |
richm
reviewed
Mar 26, 2026
richm
reviewed
Mar 26, 2026
richm
reviewed
Mar 26, 2026
richm
reviewed
Mar 26, 2026
richm
reviewed
Mar 26, 2026
Collaborator
|
you will also need to rename tests/roles/linux-system-roles.trustee_attestation_client -> tests/roles/linux-system-roles.trustee_client |
Contributor
Author
Ah, thank you. I was wondering why the tests fail. |
Collaborator
|
lgtm - @spetrosi ? |
spetrosi
reviewed
Apr 8, 2026
| __trustee_client_kbs_cert_content: "{{ trustee_client_kbs_cert_content | ||
| if (trustee_client_kbs_cert_content | length > 0) | ||
| else lookup('file', trustee_client_kbs_cert_src) | ||
| if (trustee_client_kbs_cert_src | length > 0) else '' }}" |
Contributor
There was a problem hiding this comment.
It would be good to support having the trustee_client_kbs_cert_src file both on the controller and remotely. I.e. have a boolean var trustee_client_kbs_cert_remote_src that users can set to true to look for the file remotely.
Like in https://github.com/linux-system-roles/mssql?tab=readme-ov-file#mssql_tls_remote_src
Contributor
Author
There was a problem hiding this comment.
This file is actually the SSL cert from the Trustee server which the client should trust. It would beat the purpose for users to put on the client side (remotely) manually which simply won't scale and breaks the sync.
Collaborator
There was a problem hiding this comment.
The convention we use for TLS related parameter naming is specified here - https://linux-system-roles.github.io/documentation/tls_crypto_parameter_and_key_names.html
A suffix of _src indicates the file is on the controller. Without that suffix, the file is on the managed (i.e. remote) node. so trustee_client_kbs_cert_src is a file on the controller, and the contents of that file will be copied to the managed/remote node.
| group: "{{ __trustee_client_crypttab.stat.gr_name | d('root') }}" | ||
|
|
||
| - name: Configure /etc/fstab and mount volume | ||
| ansible.posix.mount: |
Contributor
There was a problem hiding this comment.
Suggested change
| ansible.posix.mount: | |
| mount: |
@richm in the downstream we will vendor the mount module and it will be callable with mount:. And in the upstream it will be called from ansible.posix. Right?
Collaborator
There was a problem hiding this comment.
@richm in the downstream we will vendor the mount module and it will be callable with
mount:. And in the upstream it will be called from ansible.posix. Right?
Correct. But it should be fine to use ansible.posix.mount here - we have logic in the rpm spec file to convert ansible.posix.something to redhat.rhel_system_roles.something
Besides passing in the kbs cert file content, also allow passing in the file by its path. Signed-off-by: Li Tian <litian@redhat.com>
When Secret Registration Client is not enabled, allow disk encryption with systemd-cryptenroll. This binds disk encryption with TPM PCR7. Use crypttab and fstab for auto mount. Signed-off-by: Li Tian <litian@redhat.com>
…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>
The encrypted disk name is trustee_client_encrypted_disk_0. Signed-off-by: Li Tian <litian@redhat.com>
Contributor
|
[citest] |
Collaborator
|
lgtm - @spetrosi ready to merge? |
spetrosi
approved these changes
Apr 9, 2026
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_client. Becausetrustee_attestation_clientis too long andattestationis not a key factor in this role.kbs_cert_content, also allow users to pass in the cert by path usingkbs_cert.systemd-cryptenrollcapability to encrypt_disk task when Secret Registration Client is not enabled.Reason:
Result:
Issue Tracker Tickets (Jira or BZ if any):
Summary by Sourcery
Rename the role and its variables from trustee_attestation_client to trustee_client, expand disk encryption support to use systemd-cryptenroll when the Secret Registration Client is disabled, and allow KBS certificates to be provided either as inline content or via a file path, updating tests and documentation accordingly.
New Features:
Enhancements:
Tests: