[cifmw_cephadm] Allow legacy CephX while daemons use aes256k - #4130
[cifmw_cephadm] Allow legacy CephX while daemons use aes256k#4130rebtoor wants to merge 1 commit into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
PR Summary by QodoFix Ceph v20+ client.openstack cephx auth for HCI (aes256k + correct caps)
AI Description
Diagram
High-Level Assessment
Files changed (5)
|
Code Review by Qodo
1. Underscored vars violate naming
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
|
|
||
| - name: Select cephx cipher default for Ceph release | ||
| ansible.builtin.set_fact: | ||
| _cifmw_ceph_key_cipher_default: >- |
There was a problem hiding this comment.
1. Underscored vars violate naming 📘 Rule violation ⚙ Maintainability
New variables _cifmw_ceph_key_cipher_default and _cifmw_cephadm_* do not match the required ^cifmw_[a-z_][a-z0-9_]*$ pattern because they start with _. This can break variable naming compliance/ansible-lint expectations and makes variables inconsistent across the collection.
Agent Prompt
## Issue description
New variables were introduced with a leading underscore (e.g., `_cifmw_ceph_key_cipher_default`, `_cifmw_cephadm_rbd_pools`), which violates the required variable naming regex `^cifmw_[a-z_][a-z0-9_]*$`.
## Issue Context
The repository enforces a collection-scoped `cifmw_...` naming convention for Ansible variables.
## Fix Focus Areas
- hooks/playbooks/ceph.yml[375-429]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| - name: Generate a cephx key | ||
| cephx_key: | ||
| cipher: "{{ cifmw_ceph_key_cipher | default('aes') }}" | ||
| cipher: "{{ cifmw_ceph_key_cipher | default(_cifmw_ceph_key_cipher_default) }}" |
There was a problem hiding this comment.
2. cephx_key missing fqcn 📘 Rule violation ⚙ Maintainability
The modified task still calls the module as cephx_key: instead of a fully-qualified collection name (e.g., cifmw.general.cephx_key:). This can fail ansible-lint FQCN rules and reduces clarity about the module source.
Agent Prompt
## Issue description
A task uses the short module name `cephx_key:` rather than an FQCN, violating the FQCN requirement.
## Issue Context
`cephx_key` is provided by this collection (see `plugins/modules/cephx_key.py`), so the call should be fully qualified (e.g., `cifmw.general.cephx_key:`).
## Fix Focus Areas
- hooks/playbooks/ceph.yml[385-389]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| ansible.builtin.command: | | ||
| {{ cifmw_cephadm_ceph_cli }} mon set auth_preferred_cipher \ | ||
| {{ cifmw_cephadm_auth_preferred_cipher }} |
There was a problem hiding this comment.
3. Unquoted cipher command arg 🐞 Bug ☼ Reliability
cephadm_config_set.yml passes cifmw_cephadm_auth_preferred_cipher into a free-form ansible.builtin.command without quoting, so any whitespace or unexpected tokens in the value will be split into multiple argv entries and can break ceph mon set auth_preferred_cipher or alter the effective arguments.
Agent Prompt
### Issue description
`ansible.builtin.command` (free-form) is tokenized into argv; interpolating `{{ cifmw_cephadm_auth_preferred_cipher }}` without quoting means values containing whitespace (or accidental extra tokens) are split into multiple arguments, which can make the ceph CLI invocation fail or behave unexpectedly.
### Issue Context
`cifmw_cephadm_ceph_cli` is itself a long, space-delimited CLI string, so these tasks already rely on Ansible's tokenization. The safest minimal fix is to ensure the *variable* is passed as a single token by applying Jinja `| quote`.
### Fix Focus Areas
- roles/cifmw_cephadm/tasks/cephadm_config_set.yml[65-73]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Build failed (check pipeline). Post ✔️ openstack-k8s-operators-content-provider SUCCESS in 2h 29m 54s |
fmount
left a comment
fmount
left a comment
There was a problem hiding this comment.
We might need to iterate over this patch due to the recent ceph release, but I have a few questions:
- should we default to v20:aes256k?
- Instead of guessing the release via regex, do we want to keep a mapping of the current status (e.g. {v20:aes256k, v19:aes, etc ...} ) to make easy follow up based on the Ceph backports?
- is auth_preferred_cipher something we really need to enforce if we create the client.openstack key with the right cipher that we map to a struct or we guess via regex?
- should we update jobs like:
https://github.com/openstack-k8s-operators/ci-framework/blob/main/zuul.d/edpm_multinode.yaml#L348to finally switch to v20:aes256k as main is our 19 base (and 19 will be based on v20+).
I'd like also @fultonj to review the above and we can follow up in case CI is blocked and we need to land this patch asap.
| | map(attribute='name') | ||
| | list | ||
| }} | ||
| _cifmw_cephadm_cephfs_pools: >- |
There was a problem hiding this comment.
ok this worked in the past and probably changed with the new release. To give more context, this applied profile rbd pool=X to every pool, including the CephFS pools (cephfs.cephfs.meta and cephfs.cephfs.data) and while working, it was wrong.
I assume that on older Ceph releases this happened to work because cap validation was more flexible, but Ceph Tentacle (v20) enforces stricter cephx cap validation, causing RADOS permission denied errors when Manila try to access CephFS-backed pools.
Is that the case? @fultonj to double check.
| {{ | ||
| 'aes256k' if ( | ||
| cifmw_cephadm_version | default('') == 'tentacle' | ||
| or (cifmw_cephadm_container_tag | default('v18') is match('^v(20|[2-9][0-9])')) |
There was a problem hiding this comment.
should we swiftch the default here to v20 at this point for the main branch? 19 will be based on v20+ and I'm not sure we currently have upstream jobs based on previous releases.
We might need to patch github.com/openstack-k8s-operators/ci-framework/blob/1256a1cd4c0d52707f14c975f510cca324dabbe2/zuul.d/edpm_multinode.yaml#L347 and make sure it is reconciled we new defaults, but it might allow to simplify the logic for the main branch and start testing from v20 onward.
| - cifmw_cephadm_auth_preferred_cipher is defined | ||
| - cifmw_cephadm_auth_preferred_cipher | length > 0 | ||
| ansible.builtin.command: | | ||
| {{ cifmw_cephadm_ceph_cli }} mon set auth_preferred_cipher \ |
| - name: Generate a cephx key | ||
| cephx_key: | ||
| cipher: "{{ cifmw_ceph_key_cipher | default('aes') }}" | ||
| cipher: "{{ cifmw_ceph_key_cipher | default(_cifmw_ceph_key_cipher_default) }}" |
There was a problem hiding this comment.
Ok I recall we added support to customize ciphers via [1] and I'm wondering if we should simply update our defaults based on v20 instead of guessing the ceph tag (that can't be misleading unless you run podman run --rm <container> ceph versions and grep for the release).
So the real question is: should we update the defaults to tentacle (v20) by default and update the cipher to aes256k ? v20:aes256.
I assume that in the next future (weeks?) the assumption provided by this default will no longer be valid and we need to modify this logic again to meet new backports to the ceph clients.
[1] 03fdc8f
Ceph 20.2.4 (CVE-2025-30156) can reject or fail handshake for OpenStack Glance/Cinder/Manila clients whose librados only understands type `aes`. Keep `auth_allowed_ciphers=aes,aes256k`, mint `client.openstack` as `aes`, and do not set `auth_preferred_cipher`. Split RBD vs CephFS caps with `mon: allow r`. Assisted-By: Cursor Signed-off-by: Roberto Alfieri <ralfieri@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
f93cfaa to
c07d8d4
Compare
rebtoor
changed the title
|
Build failed (check pipeline). Post ✔️ openstack-k8s-operators-content-provider SUCCESS in 2h 26m 21s |
|
This patch changes the generated cephx key like this: cifmw_cephadm_keys:
- name: client.openstack
caps:
mgr: allow *
- mon: profile rbd
+ mon: allow r, profile rbd
- osd: profile rbd pool=vms, profile rbd pool=volumes, profile rbd pool=backups, profile rbd pool=images, profile rbd pool=manila_data, profile rbd pool=manila_metadata
+ osd: profile rbd pool=vms, profile rbd pool=volumes, profile rbd pool=backups, profile rbd pool=images, allow rw pool=manila_data, allow rw pool=manila_metadataSo:
This is not necessary since
Before we make this change, can I know which CI Job failed and how? I only want to CI the permissions we document and recommend for users. If we're going to change it, then that's something people are going to have to change on their systems so we need to be deliberate and know exactly why. I can see the argument that X is not necessary, but this patch is about making an update to deal with aes256k and I don't yet see the connection to the new cipher and the caps change. Let's split out the aes256k adjustments for our jobs with the caps change. I'll propose the change to fix the jobs in a separate PR. |
3 participants
Ceph 20.2.4 (CVE-2025-30156) can fail the CephX handshake for OpenStack Glance, Cinder, and Manila. Those Antelope service images still ship librados that only understands key type
aes, while Tentacle daemons may useaes256k.This is a fresh cephadm cluster every CI run, so the CVE upgrade path is:
auth_allowed_ciphers=aes,aes256kso daemons can use the new type and legacyaesclients can still authenticate.auth_preferred_cipher. New keys would otherwise default toaes256kbefore OpenStack images understand that type.client.openstackas typeaesuntil Glance/Cinder/Manila images ship librados that understandsaes256k.profile rbdon RBD pools,allow rwon CephFS pools, plusmon: allow r). That is a separate Manila/RBD fix, not the errno 5 connect path.HCI scenario
hci_ceph_backends.ymlsets the allow-list andcifmw_ceph_key_cipher: aes. Thecifmw_cephadmrole does not setauth_preferred_cipher.