Re: [pve-devel] [PATCH v2 pve-manager 11/11] fix #4759: debian/postinst: configure ceph-crash.service and its key

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

On February 5, 2024 6:54 pm, Max Carrara wrote:
> This commit adds the `set_ceph_crash_conf` function, which dynamically
> adapts the host's Ceph configuration in order to allow the Ceph crash
> module's daemon to run without elevated privileges.
> 
> This adaptation is only performed if:
>  * Ceph is installed
>  * Ceph is configured ('/etc/pve/ceph.conf' exists)
>  * Connection to RADOS is successful
> 
> If the above conditions are met, the function will ensure that:
>  * Ceph possesses a key named 'client.crash'
>  * The key is saved to '/etc/pve/ceph/ceph.client.crash.keyring'
>  * A section for 'client.crash' exists in '/etc/pve/ceph.conf'
>  * The 'client.crash' section has a key named 'keyring' which
>    references '/etc/pve/ceph/ceph.client.crash.keyring'
> 
> Furthermore, if a key named 'client.crash' already exists within the
> cluster, it shall be reused and not regenerated. Also, the
> configuration is not altered if the conditions above are already met.
> 
> This way the keyring file is available as read-only in
> '/etc/pve/ceph/' for the `www-data` group (due to how pmxcfs works).
> Because the `ceph` user has been made part of said `www-data` group
> [0], it may access the file without requiring any additional
> privileges.
> 
> Thus, the configuration for the Ceph crash daemon is safely adapted as
> expected by PVE tooling and also shared via pmxcfs across one's
> cluster.

I still don't think this is a good idea, even a simple perl -e '..'
invocation or two (or a small helper script that doesn't live in $PATH)
for doing the two steps we want (initialize key if missing, lock+modify
config if key was missing) would be better (although compared to the
"hidden" or regular command approach, it has the downside that somebody
might miss the calls here when refactoring),

among other things the code below
- doesn't lock /etc/pve/ceph.conf but modifies it
- implements yet another broken parser for ceph.conf (e.g., it doesn't
  handle the stuff you fix in the perl variant in this series!)
- duplicates constants from the perl code that risk running out of sync,
  like paths or the key profile
- still has issues that you fixed in the perl code between v1 and v2
  (restarting services)

 I haven't reviewed the bash code in detail for that reason!

another issue - IMHO this should be version-guarded, since any new setup
would already gain it when setting up a monitor, and we avoid access to
pmxcfs in the upgrade hot path which can cause problems (cluster
non-quorate, ..).

> 
> [0]: https://git.proxmox.com/?p=ceph.git;a=commitdiff;h=f72c698a55905d93e9a0b7b95674616547deba8a
> 
> Signed-off-by: Max Carrara <m.carrara@proxmox.com>
> ---
> Changes v1 --> v2:
>   * fix 'keyring' key being appended to 'client.crash' section even
>     if it already exists and configured correctly
> 
>  debian/postinst | 113 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 113 insertions(+)
> 
> diff --git a/debian/postinst b/debian/postinst
> index 6138ef6d..267a62ae 100755
> --- a/debian/postinst
> +++ b/debian/postinst
> @@ -110,6 +110,118 @@ migrate_apt_auth_conf() {
>      fi
>  }
>  
> +set_ceph_crash_conf() {
> +    PVE_CEPH_CONFFILE='/etc/pve/ceph.conf'
> +    PVE_CEPH_CONFDIR='/etc/pve/ceph'
> +    PVE_CEPH_CRASH_KEY="${PVE_CEPH_CONFDIR}/ceph.client.crash.keyring"
> +    PVE_CEPH_CRASH_KEY_REF="${PVE_CEPH_CONFDIR}/\$cluster.\$name.keyring"
> +
> +    # ceph isn't installed -> nothing to do
> +    if ! which ceph > /dev/null 2>&1; then
> +        return 0
> +    fi
> +
> +    # ceph isn't configured -> nothing to do
> +    if test ! -f "${PVE_CEPH_CONFFILE}"; then
> +        return 0
> +    fi
> +
> +    CEPH_AUTH_RES="$(ceph auth get-or-create client.crash mon 'profile crash' mgr 'profile crash' 2>&1 || true)"
> +
> +    # ceph is installed and possibly configured, but no connection to RADOS
> +    # -> assume no monitor was created, nothing to do
> +    if echo "${CEPH_AUTH_RES}" | grep -i -q 'RADOS object not found'; then
> +        return 0
> +    fi
> +
> +    SECTION_RE='^\[\S+\]$'
> +    CRASH_SECTION_RE='^\[client\.crash\]$'
> +
> +    if echo "${CEPH_AUTH_RES}" | grep -q -E "${CRASH_SECTION_RE}"; then
> +        DO_RESTART_UNIT=0
> +        CRASH_KEY="$(echo "${CEPH_AUTH_RES}" | grep 'key' | sed -E 's/^\s+key\s+=\s+//')"
> +
> +        if test ! -d "${PVE_CEPH_CONFDIR}"; then
> +            mkdir -p "${PVE_CEPH_CONFDIR}"
> +        fi
> +
> +        # keyring file doesn't exist or contains wrong key
> +        if test ! -f "${PVE_CEPH_CRASH_KEY}" || ! grep -q "${CRASH_KEY}" "${PVE_CEPH_CRASH_KEY}"; then
> +            echo "Saving key for 'client.crash' as '${PVE_CEPH_CRASH_KEY}'"
> +            echo "${CEPH_AUTH_RES}" > "${PVE_CEPH_CRASH_KEY}"
> +            DO_RESTART_UNIT=1
> +        fi
> +
> +        # 'client.crash' section is in conf file
> +        if grep -q -E "${CRASH_SECTION_RE}" "${PVE_CEPH_CONFFILE}"; then
> +            IFS=''
> +            NEW_PVE_CEPH_CONFFILE=''
> +            IN_CRASH_SECTION=0
> +            HAS_KEYRING=0
> +            REPLACED_KEYRING=0
> +
> +            # look for 'keyring' key in 'client.crash' section
> +            # -> replace it if it points to the wrong location
> +            while read -r LINE; do
> +                if test "${IN_CRASH_SECTION}" = "1"; then
> +                    if echo "${LINE}" | grep -q -E "${SECTION_RE}"; then
> +                        IN_CRASH_SECTION=0
> +                    elif echo "${LINE}" | grep -q -E '\s+keyring'; then
> +                        HAS_KEYRING=1
> +
> +                        if ! echo "${LINE}" | grep -q "${PVE_CEPH_CRASH_KEY_REF}"; then
> +                            echo "Replacing keyring value in section 'client.crash' of '${PVE_CEPH_CONFFILE}'"
> +                            LINE="$(printf '\t keyring = %s' "${PVE_CEPH_CRASH_KEY_REF}")"
> +                            REPLACED_KEYRING=1
> +                        fi
> +                    fi
> +                elif echo "${LINE}" | grep -q -E "${CRASH_SECTION_RE}"; then
> +                    IN_CRASH_SECTION=1
> +                fi
> +
> +                NEW_PVE_CEPH_CONFFILE="${NEW_PVE_CEPH_CONFFILE}${LINE}\n"
> +            done < "${PVE_CEPH_CONFFILE}"
> +
> +            unset IFS
> +
> +            if test "${HAS_KEYRING}" = "1"; then
> +                # 'keyring' key was replaced -> write to file
> +                if test "${REPLACED_KEYRING}" = "1"; then
> +                    echo "${NEW_PVE_CEPH_CONFFILE}" > "${PVE_CEPH_CONFFILE}"
> +                    DO_RESTART_UNIT=1
> +                fi
> +
> +            # client.crash section exists, but contained no 'keyring' key
> +            # -> put 'keyring' key into 'client.crash' section
> +            else
> +                sed -i -E "s#(${CRASH_SECTION_RE})#\1\n\t keyring = ${PVE_CEPH_CRASH_KEY_REF}#" \
> +                    "${PVE_CEPH_CONFFILE}"
> +                DO_RESTART_UNIT=1
> +            fi
> +
> +        # 'client.crash' section doesn't exist -> add it
> +        else
> +            echo "Adding section for key in '${PVE_CEPH_CONFFILE}'"
> +            printf '[client.crash]\n\tkeyring = %s\n\n' "${PVE_CEPH_CRASH_KEY_REF}" \
> +                >> "${PVE_CEPH_CONFFILE}"
> +            DO_RESTART_UNIT=1
> +        fi
> +
> +        if test "${DO_RESTART_UNIT}" = "1"; then
> +            UNIT='ceph-crash.service'
> +
> +            if systemctl -q is-enabled "${UNIT}"; then
> +                echo "Restarting ceph-crash.service"
> +                deb-systemd-invoke restart "${UNIT}"
> +            fi
> +        fi
> +
> +    else
> +        echo "WARNING: Ceph: Unable to retrieve key for 'client.crash' - output:"
> +        printf '%s\n\n' "${CEPH_AUTH_RES}"
> +    fi
> +}
> +
>  case "$1" in
>    triggered)
>      # We don't print a status message here, as dpkg already said
> @@ -189,6 +301,7 @@ case "$1" in
>      fi
>  
>      set_lvm_conf
> +    set_ceph_crash_conf
>  
>      if test ! -e /proxmox_install_mode; then
>          # modeled after code generated by dh_start
> -- 
> 2.39.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
>