feat: Support key and keyfile for authentication#357
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChangesChrony NTP authentication keys are configurable per server, rendered into a keyfile, referenced from 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.md`:
- Around line 62-72: Update the README configuration entry for key so the
description states that the key type is required and the value contains the key
type followed by key contents, excluding the numeric ID. Replace the
placeholder-only `key: !vault ...` example with a concrete redacted vaulted
value that demonstrates the expected format while preserving the existing
encryption guidance.
In `@tasks/main.yml`:
- Around line 179-181: Add a safe empty-list fallback to
__timesync_ntp_unique_keys before applying the length check in the
timesync_ntp_provider condition, matching the existing pattern elsewhere such as
line 197. Preserve the current requirement that the resulting list length must
be greater than zero.
- Around line 187-188: Update the chrony key file task’s group setting near the
existing owner declaration to use the role’s OS-specific internal variable for
the chrony group instead of the hardcoded `chrony` value, ensuring Debian/Ubuntu
resolve `_chrony` while other platforms retain their configured group.
In `@tests/tasks/cleanup.yml`:
- Line 16: Update the condition near the cleanup task’s package check to provide
a default empty mapping when ansible_facts.packages is undefined, while
preserving the existing other_provider membership test when package facts are
available.
In `@vars/Debian.yml`:
- Line 7: Rename the platform-specific variable to
__timesync_chrony_keyfile_path in vars/Debian.yml:7-7, vars/Fedora.yml:7-7,
vars/RedHat_10.yml:7-7, and vars/RedHat_6.yml:7-7, preserving each existing path
value; update every consumer of timesync_chrony_keyfile_path to use the new
internal variable name.
In `@vars/RedHat_9.yml`:
- Line 7: Rename timesync_chrony_keyfile_path to __timesync_chrony_keyfile_path
in vars/RedHat_9.yml and any other introduced vars/*.yml platform files. Update
all references consistently: templates/chrony.conf.j2 lines 64-66,
tests/tests_keys.yml lines 43-47 and 93-97, and the search strings in
tests/tests_keys.yml lines 52-63 and 64-76.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0cca7cb7-2f6c-4641-bd5a-2cc386bc12ba
📒 Files selected for processing (18)
README.mddefaults/main.ymltasks/main.ymltemplates/chrony.conf.j2templates/chrony.keys.j2tests/tasks/cleanup.ymltests/tests_keys.ymltests/vars/vault-variables.ymltests/vault_pwdvars/Debian.ymlvars/Fedora.ymlvars/RedHat_10.ymlvars/RedHat_6.ymlvars/RedHat_7.ymlvars/RedHat_8.ymlvars/RedHat_9.ymlvars/SL-Micro.ymlvars/default.yml
cce5201 to
25550c4
Compare
|
[citest] |
25550c4 to
e6eb494
Compare
|
[citest] |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/tests_keys.yml (1)
43-50: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winMask sensitive key material in test logs.
Even though these are test keys, applying
no_log: trueto tasks that read and decode sensitive files prevents the raw key material from leaking into CI console output and establishes a secure pattern for anyone reusing this code.🔒️ Proposed fix
- name: Fetch chrony key file - slurp: + ansible.builtin.slurp: src: "{{ timesync_chrony_keyfile_path }}" register: __keys_chrony_keys_encoded + no_log: true - name: Decode chrony key file - set_fact: + ansible.builtin.set_fact: __keys_chrony_keys: "{{ __keys_chrony_keys_encoded.content | b64decode }}" + no_log: true🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/tests_keys.yml` around lines 43 - 50, Apply no_log: true to both the “Fetch chrony key file” slurp task and the “Decode chrony key file” set_fact task so raw key material is masked from CI logs while preserving their existing registration and decoding behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/tests_keys.yml`:
- Around line 26-27: Update the task module invocations throughout
tests_keys.yml to use their fully qualified Ansible names, including
ansible.builtin.include_tasks for the shown Run the role task and the
corresponding ansible.builtin prefixes for slurp, assert, stat, set_fact,
command, and meta, while preserving existing task behavior.
- Around line 111-114: Update the command in the chrony journal task registering
__keys_chrony_journal to query the wildcard unit pattern 'chrony*' instead of
the hardcoded chronyd unit name, preserving the existing task behavior and
assertions.
---
Nitpick comments:
In `@tests/tests_keys.yml`:
- Around line 43-50: Apply no_log: true to both the “Fetch chrony key file”
slurp task and the “Decode chrony key file” set_fact task so raw key material is
masked from CI logs while preserving their existing registration and decoding
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9a188d6c-097c-406e-82ca-7c18d6b788ad
📒 Files selected for processing (19)
README.mddefaults/main.ymltasks/main.ymltemplates/chrony.conf.j2templates/chrony.keys.j2tests/tasks/cleanup.ymltests/tests_keys.ymltests/vars/vault-variables.ymltests/vault_pwdvars/Debian.ymlvars/Fedora.ymlvars/RedHat_10.ymlvars/RedHat_6.ymlvars/RedHat_7.ymlvars/RedHat_8.ymlvars/RedHat_9.ymlvars/SL-Micro.ymlvars/default.ymlvars/main.yml
🚧 Files skipped from review as they are similar to previous changes (16)
- vars/Debian.yml
- vars/SL-Micro.yml
- vars/default.yml
- vars/RedHat_10.yml
- vars/RedHat_6.yml
- tests/vars/vault-variables.yml
- vars/RedHat_7.yml
- tests/vault_pwd
- tests/tasks/cleanup.yml
- vars/RedHat_9.yml
- defaults/main.yml
- vars/RedHat_8.yml
- vars/Fedora.yml
- templates/chrony.keys.j2
- tasks/main.yml
- README.md
| - name: Run the role | ||
| include_tasks: tasks/run_role_with_clear_facts.yml |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use FQCN for module invocations.
As per path instructions, you must ALWAYS use the centrally managed wrapper with its Fully Qualified Collection Name (ansible.builtin.include_tasks). Please apply the ansible.builtin. prefix to this task and all other module invocations (slurp, assert, stat, set_fact, command, meta) throughout this test file.
♻️ Proposed fix
- - name: Run the role
- include_tasks: tasks/run_role_with_clear_facts.yml
+ - name: Run the role
+ ansible.builtin.include_tasks: tasks/run_role_with_clear_facts.yml📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Run the role | |
| include_tasks: tasks/run_role_with_clear_facts.yml | |
| - name: Run the role | |
| ansible.builtin.include_tasks: tasks/run_role_with_clear_facts.yml |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/tests_keys.yml` around lines 26 - 27, Update the task module
invocations throughout tests_keys.yml to use their fully qualified Ansible
names, including ansible.builtin.include_tasks for the shown Run the role task
and the corresponding ansible.builtin prefixes for slurp, assert, stat,
set_fact, command, and meta, while preserving existing task behavior.
Source: Path instructions
| - name: Grab the journal for chronyd | ||
| command: journalctl -u chronyd | ||
| register: __keys_chrony_journal | ||
| changed_when: false |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how the chrony service is defined across platforms
rg -i "service.*chrony" vars/ defaults/Repository: linux-system-roles/timesync
Length of output: 165
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)(tests/tests_keys\.yml|defaults/|vars/|tasks/|README\.md)$' || true
echo "== chrony references =="
rg -n -i 'chrony|chronyd|timesync_.*service' .
echo "== tests/tests_keys.yml excerpt =="
cat -n tests/tests_keys.yml | sed -n '90,140p'Repository: linux-system-roles/timesync
Length of output: 27076
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file map for repo root =="
find . -maxdepth 2 \( -path './.git' -o -path './.venv' \) -prune -o -type f | sed 's#^\./##' | sort | head -200
echo "== chrony variables in likely files =="
for f in $(git ls-files | rg '^(defaults|vars|tasks|tests)/'); do
rg -n -i 'chrony|chronyd|service' "$f" || true
doneRepository: linux-system-roles/timesync
Length of output: 40644
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('tests/tests_keys.yml')
for start,end in [(1,220)]:
print(p)
with p.open() as f:
for i,line in enumerate(f,1):
if start <= i <= end:
print(f"{i:4d}: {line.rstrip()}")
PYRepository: linux-system-roles/timesync
Length of output: 7992
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== tests/tests_keys.yml around the journal command =="
nl -ba tests/tests_keys.yml | sed -n '100,130p'
echo "== search for journalctl use in tests =="
rg -n 'journalctl -u|journalctl' tests
echo "== search for chrony service variable names anywhere =="
rg -n 'timesync_.*service|chrony.*service|service.*chrony|chronyd|chrony' .Repository: linux-system-roles/timesync
Length of output: 259
🌐 Web query:
Debian chrony service unit name chrony.service chronyd.service journalctl -u
💡 Result:
On Debian, the official systemd service unit name for the chrony package is chrony.service [1][2]. While the underlying daemon itself is named chronyd [1][3], the service management unit provided by the Debian package is named chrony.service [1][2]. When interacting with systemd, you should use the unit name chrony.service. For example, to check the status or view logs with journalctl, you can use the following commands: Check service status: systemctl status chrony.service View logs for the service: journalctl -u chrony.service If you prefer to view the most recent logs for the service, you can combine this with standard journalctl flags: journalctl -u chrony.service -e Some third-party distributions or custom configurations may sometimes use chronyd.service [3][4][5], but for standard Debian installations, chrony.service is the correct unit name [1][2].
Citations:
- 1: https://packages.debian.org/sid/amd64/chrony/filelist
- 2: https://forum.openmediavault.org/index.php?thread%2F46683-chrony-issue-after-upgrade-to-omv6%2F=
- 3: https://github.com/meta-debian/meta-debian/blob/warrior/recipes-debian/chrony/chrony_debian.bb
- 4: https://listengine.tuxfamily.org/chrony.tuxfamily.org/chrony-users/2017/11/msg00065.html
- 5: https://community.spiceworks.com/t/linux-chrony-service-not-starting-log-says-invalid-option/945382
🌐 Web query:
Debian chrony package service unit name chrony.service or chronyd.service journalctl -u supports glob patterns
💡 Result:
In Debian, the chrony package provides the primary systemd service unit named chrony.service [1][2]. While historical versions or custom configurations may sometimes refer to it as chronyd.service [3][4], the standard unit file provided in current Debian packages is chrony.service [1][2]. Regarding journalctl, the --unit (or -u) flag supports shell-style glob patterns [5]. When you provide a pattern to --unit, systemd generates a match for that pattern [6]. For example, running the command: journalctl -u 'chrony*' will return logs for any unit starting with "chrony", effectively capturing both chrony.service and other related units like chrony-wait.service [1][5]. Note that it is recommended to wrap the pattern in quotes (e.g., 'chrony*') to prevent the shell from expanding the glob before it reaches journalctl [5].
Citations:
- 1: https://packages.debian.org/sid/amd64/chrony/filelist
- 2: https://sources.debian.org/src/chrony/4.8-3/debian
- 3: https://github.com/meta-debian/meta-debian/blob/warrior/recipes-debian/chrony/chrony_debian.bb
- 4: https://listengine.tuxfamily.org/chrony.tuxfamily.org/chrony-users/2017/11/msg00065.html
- 5: https://askubuntu.com/questions/1403946/how-to-find-out-which-unit-is-logging-to-journalctl
- 6: https://www.freedesktop.org/software/systemd/man/252/systemd.journal-fields.html
🌐 Web query:
site:manpages.debian.org chrony service unit name Debian chrony.service
💡 Result:
On Debian systems, the systemd service unit name for the chrony daemon is chrony.service [1]. The chrony daemon itself is managed by this unit, which ensures that the chronyd process starts correctly [1][2]. When managing the service using systemctl, you would use the following command structure: systemctl status chrony.service systemctl restart chrony.service Additionally, in configurations involving systemd-timedated, the unit may be referenced as chronyd.service in certain contexts [1].
Citations:
- 1: https://manpages.debian.org/bookworm/systemd/systemd-timedated.service.8.en.html
- 2: https://manpages.debian.org/chronyd
Avoid hardcoding the journal unit name. Debian/Ubuntu use chrony.service, so journalctl -u chronyd can miss key-related errors and let the negative assertions pass. Use journalctl -u 'chrony*' here.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/tests_keys.yml` around lines 111 - 114, Update the command in the
chrony journal task registering __keys_chrony_journal to query the wildcard unit
pattern 'chrony*' instead of the hardcoded chronyd unit name, preserving the
existing task behavior and assertions.
e6eb494 to
348af4f
Compare
|
[citest] |
|
Fixes #321 |
mlichvar
left a comment
mlichvar
left a comment
There was a problem hiding this comment.
The key IDs need to be specified by the user, so they match the server configuration. This seems quite a lot of code for such a minor feature.
Ah, ok. I thought the key ID was just used as an index into the chrony.keys "table". That is, I thought the use case was this: As an administrator configuring the client side of secure timesync, I need to configure the client to use the same authentication key as used by the server, which is just the "[type ]keystring". So I figured "hey, the user doesn't care about the key ID since it is just an index into the chrony.keys table - they just want to associate a key with a server, so the role can auto-generate the key IDs". So the key ID is significant? That will definitely change the implementation, and possibly simplify somewhat.
It may be "minor" but it is security related, which invokes several considerations.
If you can suggest a better way to do this, I would appreciate it |
|
348af4f to
0ec98dd
Compare
|
@mlichvar the new implementation takes a |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tasks/main.yml`:
- Line 164: Update the regex string in the item.key search condition to use YAML
single-quote delimiters, preserving the backslash in \s so the regex continues
matching digits followed by whitespace.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a96a74fe-fc8f-4fd6-9b7c-31d4cd0fab22
📒 Files selected for processing (18)
README.mddefaults/main.ymltasks/main.ymltemplates/chrony.conf.j2templates/chrony.keys.j2tests/tests_keys.ymltests/vars/vault-variables.ymltests/vault_pwdvars/Debian.ymlvars/Fedora.ymlvars/RedHat_10.ymlvars/RedHat_6.ymlvars/RedHat_7.ymlvars/RedHat_8.ymlvars/RedHat_9.ymlvars/SL-Micro.ymlvars/default.ymlvars/main.yml
🚧 Files skipped from review as they are similar to previous changes (15)
- vars/default.yml
- vars/RedHat_9.yml
- vars/RedHat_6.yml
- templates/chrony.conf.j2
- tests/vars/vault-variables.yml
- vars/RedHat_10.yml
- vars/Fedora.yml
- vars/Debian.yml
- templates/chrony.keys.j2
- vars/SL-Micro.yml
- defaults/main.yml
- tests/vault_pwd
- vars/RedHat_8.yml
- vars/RedHat_7.yml
- README.md
0ec98dd to
3d7a021
Compare
|
[citest] |
|
[citest_bad] |
3d7a021 to
1d53232
Compare
|
[citest] |
|
[citest_bad] |
Looks good to me. Thanks. What will happen if there are two servers using the same key ID but different key? |
1d53232 to
7d4cf4d
Compare
The latest commit addresses this:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/tests_keys.yml`:
- Around line 3-21: Load tests/vars/vault-variables.yml in the Test NTP server
authentication keys play so the __timesync_test_key_simple,
__timesync_test_key_md5, and __timesync_test_key_sha1 variables used by
timesync_ntp_servers are defined before the role runs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: de10553b-7cda-413b-94a3-3e65add869ca
📒 Files selected for processing (18)
README.mddefaults/main.ymltasks/main.ymltemplates/chrony.conf.j2templates/chrony.keys.j2tests/tests_keys.ymltests/vars/vault-variables.ymltests/vault_pwdvars/Debian.ymlvars/Fedora.ymlvars/RedHat_10.ymlvars/RedHat_6.ymlvars/RedHat_7.ymlvars/RedHat_8.ymlvars/RedHat_9.ymlvars/SL-Micro.ymlvars/default.ymlvars/main.yml
🚧 Files skipped from review as they are similar to previous changes (16)
- defaults/main.yml
- vars/RedHat_8.yml
- vars/RedHat_10.yml
- vars/default.yml
- tests/vault_pwd
- vars/RedHat_9.yml
- vars/SL-Micro.yml
- vars/Debian.yml
- vars/Fedora.yml
- tests/vars/vault-variables.yml
- templates/chrony.keys.j2
- vars/main.yml
- vars/RedHat_6.yml
- templates/chrony.conf.j2
- vars/RedHat_7.yml
- tasks/main.yml
|
[citest] |
|
The new "Ensure the same key value is not used with different IDs" check doesn't seem right to me. It might not make much sense to use different IDs for the same key, but I think it should not cause the role to fail. |
How about a warning in that case? |
|
Yes, that would be fine I think. |
7d4cf4d to
929217b
Compare
|
[citest] |
ok - check the latest commit - ok to merge? |
|
"Each unique key value should have a unique ID." in the warning seems wrong. I think the warning should just hint it might be a misconfiguration, possibly also on the server side (two different servers using same key). Also the new text in documentation should be fixed to not say it's an error. |
Feature: Allow users to specify an authentication key for a server using the new `key` parameter for items in the `timesync_ntp_servers` list. The `key` value is as described at https://chrony-project.org/doc/latest/chrony.conf.html#keyfile and includes the key ID, the optional type, and the key string. Users are strongly encouraged to use Ansible Vault to encrypt the key. Reason: Users need to be able to specify authentication keys for secure time synchronization. Result: Users can configure secure time synchronization. Assisted-by: Cursor using models Cursor Grok 4.5, Composer 2.5, Sonnet 5. Signed-off-by: Rich Megginson <rmeggins@redhat.com>
929217b to
b9d9263
Compare
Done. |
|
[citest] |
3 participants
Feature: Allow users to specify an authentication key for a server using the
new
keyparameter for items in thetimesync_ntp_serverslist. Thekeyvalue is as described at https://chrony-project.org/doc/latest/chrony.conf.html#keyfile
and includes the key ID, the optional type, and the key string. Users are strongly
encouraged to use Ansible Vault to encrypt the key.
Reason: Users need to be able to specify authentication keys for secure
time synchronization.
Result: Users can configure secure time synchronization.
Assisted-by: Cursor using models Cursor Grok 4.5, Composer 2.5, Sonnet 5.
Signed-off-by: Rich Megginson rmeggins@redhat.com
Summary by CodeRabbit
keyvalues, automatically applied to generatedpool/serverentries.keyformat and new configuration options, including related error cases.