From 917a568c81b64d04c8794b8897669e8b3ee6e8d7 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Sat, 27 Dec 2025 18:44:54 -0800 Subject: [PATCH 01/16] Refactor: libcrmcommon: Drop side effect from pcmk__marked_as_deleted() And make it static. Signed-off-by: Reid Wahl --- lib/common/crmcommon_private.h | 3 --- lib/common/xml.c | 22 +++++++++++++++++++++- lib/common/xml_attr.c | 13 ------------- 3 files changed, 21 insertions(+), 17 deletions(-) diff --git a/lib/common/crmcommon_private.h b/lib/common/crmcommon_private.h index a53ba60746e..1ee78f2215f 100644 --- a/lib/common/crmcommon_private.h +++ b/lib/common/crmcommon_private.h @@ -172,9 +172,6 @@ G_GNUC_PRINTF(2, 3); G_GNUC_INTERNAL void pcmk__mark_xml_node_dirty(xmlNode *xml); -G_GNUC_INTERNAL -bool pcmk__marked_as_deleted(xmlAttrPtr a, void *user_data); - G_GNUC_INTERNAL bool pcmk__dump_xml_attr(const xmlAttr *attr, void *user_data); diff --git a/lib/common/xml.c b/lib/common/xml.c index 1ac10c2bceb..5ccb645d910 100644 --- a/lib/common/xml.c +++ b/lib/common/xml.c @@ -532,6 +532,26 @@ pcmk__xml_position(const xmlNode *xml, enum pcmk__xml_flags ignore_if_set) return position; } +/*! + * \internal + * \brief Check whether an attribute is marked as deleted + * + * \param[in] attr XML attribute + * \param[in] user_data Ignored + * + * \return \c true if \c pcmk__xf_deleted is set for \p attr, or \c false + * otherwise + * + * \note This is compatible with \c pcmk__xe_remove_matching_attrs(). + */ +static bool +marked_as_deleted(xmlAttr *attr, void *user_data) +{ + const xml_node_private_t *nodepriv = attr->_private; + + return pcmk__is_set(nodepriv->flags, pcmk__xf_deleted); +} + /*! * \internal * \brief Remove all attributes marked as deleted from an XML node @@ -547,7 +567,7 @@ static bool commit_attr_deletions(xmlNode *xml, void *user_data) { pcmk__xml_reset_node_flags(xml, NULL); - pcmk__xe_remove_matching_attrs(xml, true, pcmk__marked_as_deleted, NULL); + pcmk__xe_remove_matching_attrs(xml, true, marked_as_deleted, NULL); return true; } diff --git a/lib/common/xml_attr.c b/lib/common/xml_attr.c index 92b3ecd0c0a..e7916592215 100644 --- a/lib/common/xml_attr.c +++ b/lib/common/xml_attr.c @@ -113,19 +113,6 @@ pcmk__mark_xml_attr_dirty(xmlAttr *a) pcmk__mark_xml_node_dirty(parent); } -// This also clears attribute's flags if not marked as deleted -bool -pcmk__marked_as_deleted(xmlAttrPtr a, void *user_data) -{ - xml_node_private_t *nodepriv = a->_private; - - if (pcmk__is_set(nodepriv->flags, pcmk__xf_deleted)) { - return true; - } - nodepriv->flags = pcmk__xf_none; - return false; -} - /*! * \internal * \brief Append an XML attribute to a buffer From 4e0a1a5e5a98332e8c67f89f15d50462cb7a2f2e Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Sat, 27 Dec 2025 18:49:22 -0800 Subject: [PATCH 02/16] Refactor: libcrmcommon: pcmk__xe_remove_matching_attrs() match const arg Signed-off-by: Reid Wahl --- daemons/attrd/attrd_messages.c | 5 +++-- include/crm/common/xml_element_internal.h | 2 +- lib/common/acl.c | 2 +- lib/common/digest.c | 2 +- lib/common/xml.c | 2 +- lib/common/xml_element.c | 4 ++-- lib/pengine/pe_digest.c | 6 +++--- 7 files changed, 12 insertions(+), 11 deletions(-) diff --git a/daemons/attrd/attrd_messages.c b/daemons/attrd/attrd_messages.c index 1a680bc6e69..ed90865fb18 100644 --- a/daemons/attrd/attrd_messages.c +++ b/daemons/attrd/attrd_messages.c @@ -24,9 +24,10 @@ int minimum_protocol_version = -1; static GHashTable *attrd_handlers = NULL; static bool -is_sync_point_attr(xmlAttrPtr attr, void *data) +is_sync_point_attr(const xmlAttr *attr, void *data) { - return pcmk__str_eq((const char *) attr->name, PCMK__XA_ATTR_SYNC_POINT, pcmk__str_none); + return pcmk__str_eq((const char *) attr->name, PCMK__XA_ATTR_SYNC_POINT, + pcmk__str_none); } static int diff --git a/include/crm/common/xml_element_internal.h b/include/crm/common/xml_element_internal.h index 3bc7197b907..3492e324506 100644 --- a/include/crm/common/xml_element_internal.h +++ b/include/crm/common/xml_element_internal.h @@ -48,7 +48,7 @@ xmlNode *pcmk__xe_first_child(const xmlNode *parent, const char *node_name, void pcmk__xe_remove_attr(xmlNode *element, const char *name); bool pcmk__xe_remove_attr_cb(xmlNode *xml, void *user_data); void pcmk__xe_remove_matching_attrs(xmlNode *element, bool force, - bool (*match)(xmlAttr *, void *), + bool (*match)(const xmlAttr *, void *), void *user_data); int pcmk__xe_delete_match(xmlNode *xml, xmlNode *search); int pcmk__xe_replace_match(xmlNode *xml, xmlNode *replace); diff --git a/lib/common/acl.c b/lib/common/acl.c index 693eb4a6efa..22525ab8aeb 100644 --- a/lib/common/acl.c +++ b/lib/common/acl.c @@ -819,7 +819,7 @@ attr_is_id(const xmlAttr *attr, void *user_data) * \note This is compatible with \c pcmk__xe_remove_matching_attrs(). */ static bool -attr_is_not_id(xmlAttr *attr, void *user_data) +attr_is_not_id(const xmlAttr *attr, void *user_data) { return !attr_is_id(attr, user_data); } diff --git a/lib/common/digest.c b/lib/common/digest.c index e17aa1ff393..2a2175a2a88 100644 --- a/lib/common/digest.c +++ b/lib/common/digest.c @@ -298,7 +298,7 @@ pcmk__xa_filterable(const char *name) // Return true if a is an attribute that should be filtered static bool -should_filter_for_digest(xmlAttrPtr a, void *user_data) +should_filter_for_digest(const xmlAttr *a, void *user_data) { if (g_str_has_prefix((const char *) a->name, CRM_META "_")) { return true; diff --git a/lib/common/xml.c b/lib/common/xml.c index 5ccb645d910..a7ea897198f 100644 --- a/lib/common/xml.c +++ b/lib/common/xml.c @@ -545,7 +545,7 @@ pcmk__xml_position(const xmlNode *xml, enum pcmk__xml_flags ignore_if_set) * \note This is compatible with \c pcmk__xe_remove_matching_attrs(). */ static bool -marked_as_deleted(xmlAttr *attr, void *user_data) +marked_as_deleted(const xmlAttr *attr, void *user_data) { const xml_node_private_t *nodepriv = attr->_private; diff --git a/lib/common/xml_element.c b/lib/common/xml_element.c index 8782d79dbfc..5e86806bb2c 100644 --- a/lib/common/xml_element.c +++ b/lib/common/xml_element.c @@ -510,7 +510,7 @@ struct remove_xa_if_matching_data { bool force; //! Match function to call for each attribute - bool (*match)(xmlAttr *, void *); + bool (*match)(const xmlAttr *, void *); //! User data argument for match function void *match_data; @@ -558,7 +558,7 @@ remove_xa_if_matching(xmlAttr *attr, void *user_data) */ void pcmk__xe_remove_matching_attrs(xmlNode *element, bool force, - bool (*match)(xmlAttr *, void *), + bool (*match)(const xmlAttr *, void *), void *user_data) { struct remove_xa_if_matching_data data = { diff --git a/lib/pengine/pe_digest.c b/lib/pengine/pe_digest.c index 563965666b0..e72a0cbf2d9 100644 --- a/lib/pengine/pe_digest.c +++ b/lib/pengine/pe_digest.c @@ -60,7 +60,7 @@ pe__free_digests(void *ptr) // Return true if XML attribute name is an element of a given gchar ** array static bool -attr_in_strv(xmlAttrPtr a, void *user_data) +attr_in_strv(const xmlAttr *a, void *user_data) { const char *name = (const char *) a->name; gchar **strv = user_data; @@ -70,7 +70,7 @@ attr_in_strv(xmlAttrPtr a, void *user_data) // Return true if XML attribute name is not an element of a given gchar ** array static bool -attr_not_in_strv(xmlAttrPtr a, void *user_data) +attr_not_in_strv(const xmlAttr *a, void *user_data) { return !attr_in_strv(a, user_data); } @@ -162,7 +162,7 @@ calculate_main_digest(pcmk__op_digest_t *data, pcmk_resource_t *rsc, // Return true if XML attribute name is a Pacemaker-defined fencing parameter static bool -is_fence_param(xmlAttrPtr attr, void *user_data) +is_fence_param(const xmlAttr *attr, void *user_data) { return pcmk_stonith_param((const char *) attr->name); } From 6433f254297a884bda6b1e81927df4809b5202f3 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Fri, 26 Dec 2025 02:42:56 -0800 Subject: [PATCH 03/16] Refactor: libcrmcommon: New pcmk__xml_tree_foreach_remove() This is similar to pcmk__xml_tree_foreach(), except for the following: * It removes a node and its subtree if the function returns true. pcmk__xml_tree_foreach() would have a use-after-free if its function freed its xmlNode * argument. (This has not been observed with any existing calls.) * pcmk__xml_tree_foreach_remove() currently offers no way to stop iterating. The function's return value instead tells us whether to remove a node. Obviously we don't recurse down a freed node's subtree, however. * The function does not currently accept user data. A user data argument may be added later if needed. * pcmk__xml_tree_foreach_remove() returns void. We can later return a boolean or integer value if we need to know whether any nodes were removed. Nothing uses this yet. Also correct the Doxygen of pcmk__xml_tree_foreach() (fn is an [in] parameter) and add a note that fn may not remove its argument. Signed-off-by: Reid Wahl --- lib/common/crmcommon_private.h | 3 ++ lib/common/xml.c | 51 ++++++++++++++++++++++++++++++++-- 2 files changed, 52 insertions(+), 2 deletions(-) diff --git a/lib/common/crmcommon_private.h b/lib/common/crmcommon_private.h index 1ee78f2215f..71aeff67f6d 100644 --- a/lib/common/crmcommon_private.h +++ b/lib/common/crmcommon_private.h @@ -113,6 +113,9 @@ typedef struct { G_GNUC_INTERNAL const char *pcmk__xml_element_type_text(xmlElementType type); +G_GNUC_INTERNAL +void pcmk__xml_tree_foreach_remove(xmlNode *xml, bool (*fn)(xmlNode *)); + G_GNUC_INTERNAL bool pcmk__xml_reset_node_flags(xmlNode *xml, void *user_data); diff --git a/lib/common/xml.c b/lib/common/xml.c index a7ea897198f..271d7468857 100644 --- a/lib/common/xml.c +++ b/lib/common/xml.c @@ -72,16 +72,19 @@ pcmk__xml_element_type_text(xmlElementType type) /*! * \internal - * \brief Apply a function to each XML node in a tree (pre-order, depth-first) + * \brief Call a function for each XML node in a tree (pre-order, depth-first) * * \param[in,out] xml XML tree to traverse - * \param[in,out] fn Function to call for each node (returns \c true to + * \param[in] fn Function to call for each node (returns \c true to * continue traversing the tree or \c false to stop) * \param[in,out] user_data Argument to \p fn * * \return \c false if any \p fn call returned \c false, or \c true otherwise * * \note This function is recursive. + * \note \c fn may not free or unlink its XML argument or any of that node's + * ancestors. \c fn may unlink the descendants of that node, and it may + * free them as long as it also unlinks them. */ bool pcmk__xml_tree_foreach(xmlNode *xml, bool (*fn)(xmlNode *, void *), @@ -107,6 +110,50 @@ pcmk__xml_tree_foreach(xmlNode *xml, bool (*fn)(xmlNode *, void *), return true; } +/*! + * \internal + * \brief Remove XML nodes for which a given function returns \c true + * + * Call a function for each XML node in a tree (pre-order, depth-first). If the + * function returns true, remove the node. This means to free the entire + * document if the node is the document root, or to unlink and free the node and + * its subtree otherwise. ACLs and change tracking are ignored. + * + * \param[in,out] xml XML tree to traverse + * \param[in] fn Function to call for each node (returns \c true to remove + * its argument or \c false to recurse down its argument's + * subtree) + * + * \note This function is recursive. + */ +void +pcmk__xml_tree_foreach_remove(xmlNode *xml, bool (*fn)(xmlNode *)) +{ + if (xml == NULL) { + return; + } + + if (fn(xml)) { + if (xml == xmlDocGetRootElement(xml->doc)) { + pcmk__xml_free_doc(xml->doc); + + } else { + pcmk__xml_free_node(xml); + } + + return; + } + + xml = pcmk__xml_first_child(xml); + + while (xml != NULL) { + xmlNode *next = pcmk__xml_next(xml); + + pcmk__xml_tree_foreach_remove(xml, fn); + xml = next; + } +} + void pcmk__xml_set_parent_flags(xmlNode *xml, uint64_t flags) { From d1e17a836211c08255960c83b14052e35302dcd9 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Fri, 26 Dec 2025 02:56:35 -0800 Subject: [PATCH 04/16] Refactor: libcrmcommon: Unindent pcmk__apply_creation_acl() Signed-off-by: Reid Wahl --- lib/common/acl.c | 68 ++++++++++++++++++++++++------------------------ 1 file changed, 34 insertions(+), 34 deletions(-) diff --git a/lib/common/acl.c b/lib/common/acl.c index 22525ab8aeb..fb9989d61ca 100644 --- a/lib/common/acl.c +++ b/lib/common/acl.c @@ -1009,51 +1009,51 @@ implicitly_allowed(const xmlNode *xml) void pcmk__apply_creation_acl(xmlNode *xml, bool check_top) { + bool is_root = (xml == xmlDocGetRootElement(xml->doc)); xml_node_private_t *nodepriv = xml->_private; - if (pcmk__is_set(nodepriv->flags, pcmk__xf_created)) { - if (implicitly_allowed(xml)) { - pcmk__trace("Creation of <%s> scaffolding with " - PCMK_XA_ID "=\"%s\" is implicitly allowed", - xml->name, display_id(xml)); - - } else if (pcmk__check_acl(xml, NULL, pcmk__xf_acl_write)) { - pcmk__trace("ACLs allow creation of <%s> with " - PCMK_XA_ID "=\"%s\"", - xml->name, display_id(xml)); + if (!pcmk__is_set(nodepriv->flags, pcmk__xf_created)) { + goto recurse; + } - } else if (check_top) { - /* is_root=true should be impossible with check_top=true, but check - * for sanity - */ - bool is_root = (xmlDocGetRootElement(xml->doc) == xml); - xml_doc_private_t *docpriv = xml->doc->_private; + if (implicitly_allowed(xml)) { + pcmk__trace("Creation of <%s> scaffolding with " PCMK_XA_ID "=\"%s\" " + "is implicitly allowed", xml->name, display_id(xml)); + goto recurse; + } - pcmk__trace("ACLs disallow creation of %s<%s> with " - PCMK_XA_ID "=\"%s\"", - (is_root? "root element " : ""), xml->name, - display_id(xml)); + if (pcmk__check_acl(xml, NULL, pcmk__xf_acl_write)) { + pcmk__trace("ACLs allow creation of <%s> with " PCMK_XA_ID "=\"%s\"", + xml->name, display_id(xml)); + goto recurse; + } - // pcmk__xml_free() checks ACLs if enabled, which would fail - pcmk__clear_xml_flags(docpriv, pcmk__xf_acl_enabled); - pcmk__xml_free(xml); + if (check_top) { + xml_doc_private_t *docpriv = xml->doc->_private; - if (!is_root) { - // If root, the document was freed. Otherwise re-enable ACLs. - pcmk__set_xml_flags(docpriv, pcmk__xf_acl_enabled); - } - return; + pcmk__trace("ACLs disallow creation of %s<%s> with " + PCMK_XA_ID "=\"%s\"", (is_root? "root element " : ""), + xml->name, display_id(xml)); - } else { - const bool is_root = (xml == xmlDocGetRootElement(xml->doc)); + // pcmk__xml_free() checks ACLs if enabled, which would fail + pcmk__clear_xml_flags(docpriv, pcmk__xf_acl_enabled); + pcmk__xml_free(xml); - pcmk__notice("ACLs would disallow creation of %s<%s> with " - PCMK_XA_ID "=\"%s\"", - (is_root? "root element " : ""), xml->name, - display_id(xml)); + /* is_root=true should be impossible with check_top=true, but check for + * sanity + */ + if (!is_root) { + // If root, the document was freed. Otherwise re-enable ACLs. + pcmk__set_xml_flags(docpriv, pcmk__xf_acl_enabled); } + return; } + pcmk__notice("ACLs would disallow creation of %s<%s> with " + PCMK_XA_ID "=\"%s\"", (is_root? "root element " : ""), + xml->name, display_id(xml)); + +recurse: for (xmlNode *cIter = pcmk__xml_first_child(xml); cIter != NULL; ) { xmlNode *child = cIter; cIter = pcmk__xml_next(cIter); /* In case it is free'd */ From 17585d7ba6f314c833224221ea9fee0061bba1b7 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Fri, 26 Dec 2025 02:56:35 -0800 Subject: [PATCH 05/16] Refactor: libcrmcommon: Make pcmk__apply_creation_acl() non-recursive And change its name to pcmk__check_creation_acls(), since it's checking rather than applying ACLs. Use pcmk__xml_tree_foreach_remove() to "drive" the recursion, instead of building it into pcmk__check_creation_acls(). Functionize the main logic into the helper check_creation_disallowed(). Signed-off-by: Reid Wahl --- cts/cli/regression.acls.exp | 18 +++---- cts/cts-cli.in | 3 +- lib/common/acl.c | 99 +++++++++++++++++----------------- lib/common/crmcommon_private.h | 2 +- lib/common/xml.c | 2 +- 5 files changed, 62 insertions(+), 62 deletions(-) diff --git a/cts/cli/regression.acls.exp b/cts/cli/regression.acls.exp index 353edeb53c3..521e2d52204 100644 --- a/cts/cli/regression.acls.exp +++ b/cts/cli/regression.acls.exp @@ -537,7 +537,7 @@ crm_attribute: Error performing operation: Permission denied * Passed: crm_attribute - unknownguy: Set fencing-enabled =#=#=#= Begin test: unknownguy: Create a resource =#=#=#= pcmk__check_acl trace: Lack of ACL denies user 'unknownguy' read/write access to /cib/configuration/resources/primitive[@id='dummy'] -pcmk__apply_creation_acl trace: ACLs disallow creation of with id="dummy" +check_creation_disallowed trace: ACLs disallow creation of with id="dummy" cibadmin: CIB API call failed: Permission denied =#=#=#= End test: unknownguy: Create a resource - Insufficient privileges (4) =#=#=#= * Passed: cibadmin - unknownguy: Create a resource @@ -555,7 +555,7 @@ crm_attribute: Error performing operation: Permission denied * Passed: crm_attribute - l33t-haxor: Set fencing-enabled =#=#=#= Begin test: l33t-haxor: Create a resource =#=#=#= pcmk__check_acl trace: Parent ACL denies user 'l33t-haxor' read/write access to /cib/configuration/resources/primitive[@id='dummy'] -pcmk__apply_creation_acl trace: ACLs disallow creation of with id="dummy" +check_creation_disallowed trace: ACLs disallow creation of with id="dummy" cibadmin: CIB API call failed: Permission denied =#=#=#= End test: l33t-haxor: Create a resource - Insufficient privileges (4) =#=#=#= * Passed: cibadmin - l33t-haxor: Create a resource @@ -639,7 +639,7 @@ crm_attribute: Error performing operation: Permission denied =#=#=#= End test: niceguy: Set enable-acl - Insufficient privileges (4) =#=#=#= * Passed: crm_attribute - niceguy: Set enable-acl =#=#=#= Begin test: niceguy: Set fencing-enabled =#=#=#= -pcmk__apply_creation_acl trace: ACLs allow creation of with id="cib-bootstrap-options-fencing-enabled" +check_creation_disallowed trace: ACLs allow creation of with id="cib-bootstrap-options-fencing-enabled" =#=#=#= Current cib after: niceguy: Set fencing-enabled =#=#=#= @@ -716,7 +716,7 @@ pcmk__apply_creation_acl trace: ACLs allow creation of with id="cib-bo * Passed: crm_attribute - niceguy: Set fencing-enabled =#=#=#= Begin test: niceguy: Create a resource =#=#=#= pcmk__check_acl trace: Default ACL denies user 'niceguy' read/write access to /cib/configuration/resources/primitive[@id='dummy'] -pcmk__apply_creation_acl trace: ACLs disallow creation of with id="dummy" +check_creation_disallowed trace: ACLs disallow creation of with id="dummy" cibadmin: CIB API call failed: Permission denied =#=#=#= End test: niceguy: Create a resource - Insufficient privileges (4) =#=#=#= * Passed: cibadmin - niceguy: Create a resource @@ -1041,8 +1041,8 @@ crm_resource: Error performing operation: Insufficient privileges * Passed: crm_resource - l33t-haxor: Remove a resource meta attribute =#=#=#= Begin test: niceguy: Create a resource meta attribute =#=#=#= unpack_resources error: Resource start-up disabled since no fencing resources have been defined. Either configure some or disable fencing with the fencing-enabled option. NOTE: Clusters with shared data need fencing to ensure data integrity. -pcmk__apply_creation_acl trace: Creation of scaffolding with id="dummy-meta_attributes" is implicitly allowed -pcmk__apply_creation_acl trace: ACLs allow creation of with id="dummy-meta_attributes-target-role" +check_creation_disallowed trace: Creation of scaffolding with id="dummy-meta_attributes" is implicitly allowed +check_creation_disallowed trace: ACLs allow creation of with id="dummy-meta_attributes-target-role" Set 'dummy' option: id=dummy-meta_attributes-target-role set=dummy-meta_attributes name=target-role value=Stopped =#=#=#= Current cib after: niceguy: Create a resource meta attribute =#=#=#= @@ -1293,7 +1293,7 @@ Deleted 'dummy' option: id=dummy-meta_attributes-target-role name=target-role * Passed: crm_resource - niceguy: Remove a resource meta attribute =#=#=#= Begin test: niceguy: Create a resource meta attribute =#=#=#= unpack_resources error: Resource start-up disabled since no fencing resources have been defined. Either configure some or disable fencing with the fencing-enabled option. NOTE: Clusters with shared data need fencing to ensure data integrity. -pcmk__apply_creation_acl trace: ACLs allow creation of with id="dummy-meta_attributes-target-role" +check_creation_disallowed trace: ACLs allow creation of with id="dummy-meta_attributes-target-role" Set 'dummy' option: id=dummy-meta_attributes-target-role set=dummy-meta_attributes name=target-role value=Started =#=#=#= Current cib after: niceguy: Create a resource meta attribute =#=#=#= @@ -1514,7 +1514,7 @@ cibadmin: CIB API call failed: Permission denied =#=#=#= Begin test: niceguy: Replace - create resource =#=#=#= pcmk__check_acl trace: Default ACL denies user 'niceguy' read/write access to /cib[@epoch] pcmk__check_acl trace: Default ACL denies user 'niceguy' read/write access to /cib/configuration/resources/primitive[@id='dummy2'] -pcmk__apply_creation_acl trace: ACLs disallow creation of with id="dummy2" +check_creation_disallowed trace: ACLs disallow creation of with id="dummy2" cibadmin: CIB API call failed: Permission denied =#=#=#= End test: niceguy: Replace - create resource - Insufficient privileges (4) =#=#=#= * Passed: cibadmin - niceguy: Replace - create resource @@ -2546,7 +2546,7 @@ cibadmin: CIB API call failed: Permission denied =#=#=#= Begin test: mike: Create another resource =#=#=#= -pcmk__apply_creation_acl trace: ACLs allow creation of with id="dummy2" +check_creation_disallowed trace: ACLs allow creation of with id="dummy2" =#=#=#= Current cib after: mike: Create another resource =#=#=#= diff --git a/cts/cts-cli.in b/cts/cts-cli.in index 8f557ce4b45..84e3b8ae90f 100644 --- a/cts/cts-cli.in +++ b/cts/cts-cli.in @@ -249,6 +249,7 @@ def sanitize_output(s): (r'(") - /*! * \internal - * \brief Drop XML nodes created in violation of ACLs + * \brief Check whether ACLs allow creation of an XML node * - * Given an XML element, free all of its descendant nodes created in violation - * of ACLs, with the exception of allowing "scaffolding" elements (i.e. those - * that aren't in the ACL section and don't have any attributes other than - * \c PCMK_XA_ID). + * "Scaffolding" elements (those that aren't in the ACLs section and don't have + * any attributes other than \c PCMK_XA_ID) are always allowed. * - * \param[in,out] xml XML to check - * \param[in] check_top Whether to apply checks to argument itself - * (if true, xml might get freed) + * \param[in,out] xml XML node * - * \note This function is recursive + * \return \c true if \p xml is newly created and ACLs disallow its creation, or + * \c false otherwise + * + * \note This is compatible with \c pcmk__xml_tree_foreach_remove(). */ -void -pcmk__apply_creation_acl(xmlNode *xml, bool check_top) +static bool +check_creation_disallowed(xmlNode *xml) { - bool is_root = (xml == xmlDocGetRootElement(xml->doc)); + const char *type = (const char *) xml->name; + const char *id = pcmk__s(pcmk__xe_id(xml), ""); xml_node_private_t *nodepriv = xml->_private; if (!pcmk__is_set(nodepriv->flags, pcmk__xf_created)) { - goto recurse; + return false; } if (implicitly_allowed(xml)) { pcmk__trace("Creation of <%s> scaffolding with " PCMK_XA_ID "=\"%s\" " - "is implicitly allowed", xml->name, display_id(xml)); - goto recurse; + "is implicitly allowed", type, id); + return false; } if (pcmk__check_acl(xml, NULL, pcmk__xf_acl_write)) { pcmk__trace("ACLs allow creation of <%s> with " PCMK_XA_ID "=\"%s\"", - xml->name, display_id(xml)); - goto recurse; - } - - if (check_top) { - xml_doc_private_t *docpriv = xml->doc->_private; - - pcmk__trace("ACLs disallow creation of %s<%s> with " - PCMK_XA_ID "=\"%s\"", (is_root? "root element " : ""), - xml->name, display_id(xml)); - - // pcmk__xml_free() checks ACLs if enabled, which would fail - pcmk__clear_xml_flags(docpriv, pcmk__xf_acl_enabled); - pcmk__xml_free(xml); - - /* is_root=true should be impossible with check_top=true, but check for - * sanity - */ - if (!is_root) { - // If root, the document was freed. Otherwise re-enable ACLs. - pcmk__set_xml_flags(docpriv, pcmk__xf_acl_enabled); - } - return; + type, id); + return false; } - pcmk__notice("ACLs would disallow creation of %s<%s> with " - PCMK_XA_ID "=\"%s\"", (is_root? "root element " : ""), - xml->name, display_id(xml)); + pcmk__trace("ACLs disallow creation of <%s> with " PCMK_XA_ID "=\"%s\"", + type, id); + return true; +} -recurse: - for (xmlNode *cIter = pcmk__xml_first_child(xml); cIter != NULL; ) { - xmlNode *child = cIter; - cIter = pcmk__xml_next(cIter); /* In case it is free'd */ - pcmk__apply_creation_acl(child, true); - } +/*! + * \internal + * \brief Remove XML nodes created in violation of ACLs + * + * Given an XML tree, free all nodes created in violation of ACLs, with the + * exception of allowing "scaffolding" elements (those that aren't in the ACLs + * section and don't have any attributes other than \c PCMK_XA_ID). + * + * \param[in,out] xml XML tree + */ +void +pcmk__check_creation_acls(xmlNode *xml) +{ + pcmk__xml_tree_foreach_remove(xml, check_creation_disallowed); } /*! @@ -1089,7 +1075,20 @@ xml_acl_disable(xmlNode *xml) /* Catch anything that was created but shouldn't have been */ pcmk__apply_acls(xml->doc); - pcmk__apply_creation_acl(xml, false); + + /* Be sure not to free xml itself. + * + * @TODO Maybe we should free xml if it's newly created and the creation + * is disallowed, but we would need a way to inform the caller. This is + * public API. + */ + xml = pcmk__xml_first_child(xml); + while (xml != NULL) { + xmlNode *next = pcmk__xml_next(xml); + + pcmk__check_creation_acls(xml); + xml = next; + } pcmk__clear_xml_flags(docpriv, pcmk__xf_acl_enabled); } } diff --git a/lib/common/crmcommon_private.h b/lib/common/crmcommon_private.h index 71aeff67f6d..01070585529 100644 --- a/lib/common/crmcommon_private.h +++ b/lib/common/crmcommon_private.h @@ -157,7 +157,7 @@ G_GNUC_INTERNAL void pcmk__apply_acls(xmlDoc *doc); G_GNUC_INTERNAL -void pcmk__apply_creation_acl(xmlNode *xml, bool check_top); +void pcmk__check_creation_acls(xmlNode *xml); G_GNUC_INTERNAL int pcmk__xa_remove(xmlAttr *attr, bool force); diff --git a/lib/common/xml.c b/lib/common/xml.c index 271d7468857..2cbd9d95a84 100644 --- a/lib/common/xml.c +++ b/lib/common/xml.c @@ -1886,7 +1886,7 @@ pcmk__xml_mark_changes(xmlNode *old_xml, xmlNode *new_xml) mark_xml_tree_dirty_created(new_child); // Check whether creation was allowed (may free new_child) - pcmk__apply_creation_acl(new_child, true); + pcmk__check_creation_acls(new_child); } } From 89e9bc5442263e3110d439d13b1f1e6917c90e5b Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Sat, 27 Dec 2025 21:49:36 -0800 Subject: [PATCH 06/16] Refactor: libcrmcommon: Unindent is_config_change() If xml->doc or xml->doc->_private were NULL, then we would not have called this function. See pcmk__xml_doc_all_flags_set(). Signed-off-by: Reid Wahl --- lib/common/patchset.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/lib/common/patchset.c b/lib/common/patchset.c index 778be7ff9bd..6ea38b1efbc 100644 --- a/lib/common/patchset.c +++ b/lib/common/patchset.c @@ -328,17 +328,16 @@ is_config_change(xmlNode *xml) return TRUE; } - if ((xml->doc != NULL) && (xml->doc->_private != NULL)) { - docpriv = xml->doc->_private; - for (gIter = docpriv->deleted_objs; gIter; gIter = gIter->next) { - pcmk__deleted_xml_t *deleted_obj = gIter->data; - - if (strstr(deleted_obj->path, - "/" PCMK_XE_CIB "/" PCMK_XE_CONFIGURATION) != NULL) { - return TRUE; - } + docpriv = xml->doc->_private; + for (gIter = docpriv->deleted_objs; gIter; gIter = gIter->next) { + pcmk__deleted_xml_t *deleted_obj = gIter->data; + + if (strstr(deleted_obj->path, + "/" PCMK_XE_CIB "/" PCMK_XE_CONFIGURATION) != NULL) { + return TRUE; } } + return FALSE; } From 76d3c24abde2df734164cb35ca070c4c3a1f3f3d Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Sat, 27 Dec 2025 21:54:05 -0800 Subject: [PATCH 07/16] Refactor: libcrmcommon: Minor best practices in is_config_change() Signed-off-by: Reid Wahl --- lib/common/patchset.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/lib/common/patchset.c b/lib/common/patchset.c index 6ea38b1efbc..930ab74e288 100644 --- a/lib/common/patchset.c +++ b/lib/common/patchset.c @@ -313,32 +313,33 @@ add_changes_to_patchset(xmlNode *xml, xmlNode *patchset) } static bool -is_config_change(xmlNode *xml) +is_config_change(const xmlNode *xml) { GList *gIter = NULL; xml_node_private_t *nodepriv = NULL; - xml_doc_private_t *docpriv; + xml_doc_private_t *docpriv = xml->doc->_private; xmlNode *config = pcmk__xe_first_child(xml, PCMK_XE_CONFIGURATION, NULL, NULL); - if (config) { + if (config != NULL) { nodepriv = config->_private; } + + // Arbitrary xml may come from the public API, so NULL-check nodepriv if ((nodepriv != NULL) && pcmk__is_set(nodepriv->flags, pcmk__xf_dirty)) { - return TRUE; + return true; } - docpriv = xml->doc->_private; for (gIter = docpriv->deleted_objs; gIter; gIter = gIter->next) { pcmk__deleted_xml_t *deleted_obj = gIter->data; if (strstr(deleted_obj->path, "/" PCMK_XE_CIB "/" PCMK_XE_CONFIGURATION) != NULL) { - return TRUE; + return true; } } - return FALSE; + return false; } // Guaranteed to return non-NULL From a29fcc87052709b13d3f9abb52f8cb7d845a036a Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Sat, 27 Dec 2025 22:06:34 -0800 Subject: [PATCH 08/16] Refactor: libcrmcommon: Functionize search for deleted config element Signed-off-by: Reid Wahl --- lib/common/patchset.c | 49 +++++++++++++++++++++++++++++++++---------- 1 file changed, 38 insertions(+), 11 deletions(-) diff --git a/lib/common/patchset.c b/lib/common/patchset.c index 930ab74e288..c43aca60b41 100644 --- a/lib/common/patchset.c +++ b/lib/common/patchset.c @@ -312,10 +312,45 @@ add_changes_to_patchset(xmlNode *xml, xmlNode *patchset) } } +/*! + * \internal + * \brief Check whether a deleted object path contains the configuration element + * + * \param[in] data Deleted object (const pcmk__deleted_xml_t *) + * \param[in] ignored Ignored + * + * \retval 0 if \p data->path contains + * "/" PCMK_XE_CIB "/" PCMK_XE_CONFIGURATION + * \retval 1 otherwise + * + * \note This is a \c GCompareFunc. + */ +static int +config_in_deleted_obj_path(const void *data, const void *ignored) +{ + const pcmk__deleted_xml_t *deleted_obj = data; + + if (strstr(deleted_obj->path, + "/" PCMK_XE_CIB "/" PCMK_XE_CONFIGURATION) != NULL) { + + return 0; + } + + return 1; +} + +/*! + * \internal + * \brief Check whether a CIB XML tree contains a configuration change + * + * \param[in] xml XML tree + * + * \return \c true if \p xml contains a dirty or deleted configuration element, + * or \c false otherwise + */ static bool is_config_change(const xmlNode *xml) { - GList *gIter = NULL; xml_node_private_t *nodepriv = NULL; xml_doc_private_t *docpriv = xml->doc->_private; xmlNode *config = pcmk__xe_first_child(xml, PCMK_XE_CONFIGURATION, NULL, @@ -330,16 +365,8 @@ is_config_change(const xmlNode *xml) return true; } - for (gIter = docpriv->deleted_objs; gIter; gIter = gIter->next) { - pcmk__deleted_xml_t *deleted_obj = gIter->data; - - if (strstr(deleted_obj->path, - "/" PCMK_XE_CIB "/" PCMK_XE_CONFIGURATION) != NULL) { - return true; - } - } - - return false; + return (g_list_find_custom(docpriv->deleted_objs, NULL, + config_in_deleted_obj_path) != NULL); } // Guaranteed to return non-NULL From 0edaa2a26f0f57448bf1a055e9f5d5602539d95c Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Sat, 27 Dec 2025 22:25:52 -0800 Subject: [PATCH 09/16] Refactor: libcrmcommon: Functionize filtering by one ACL Note to reviewer: this code is expected to be removed a few commits from now. This commit was part of my process of trying to understand the filtering logic. It may make review easier, or it may make it more tedious. Signed-off-by: Reid Wahl --- lib/common/acl.c | 106 ++++++++++++++++++++++++++++++----------------- 1 file changed, 68 insertions(+), 38 deletions(-) diff --git a/lib/common/acl.c b/lib/common/acl.c index c3df91eb16b..ea30911cec3 100644 --- a/lib/common/acl.c +++ b/lib/common/acl.c @@ -868,6 +868,70 @@ purge_xml_attributes(xmlNode *xml) return readable_children; } +/*! + * \internal + * \brief User data for \c acl_filter_doc() + */ +struct acl_filter_doc_data { + xmlDoc *doc; //!< XML document being filtered + bool all_filtered; //!< Whether access has been denied to entire document +}; + +/*! + * \internal + * \brief Filter an XML document using one ACL + * + * \param[in] acl ACL object + * \param[in,out] user_data User data (struct acl_filter_doc_data *) + */ +static void +acl_filter_doc(gpointer data, gpointer user_data) +{ + const xml_acl_t *acl = data; + struct acl_filter_doc_data *filter_data = user_data; + + xmlNode *root = xmlDocGetRootElement(filter_data->doc); + xml_doc_private_t *docpriv = filter_data->doc->_private; + xmlXPathObject *xpath_obj = NULL; + int num_results = 0; + + if (filter_data->all_filtered) { + return; + } + + if ((acl->mode != pcmk__xf_acl_deny) || (acl->xpath == NULL)) { + return; + } + + xpath_obj = pcmk__xpath_search(filter_data->doc, acl->xpath); + num_results = pcmk__xpath_num_results(xpath_obj); + + for (int i = 0; i < num_results; i++) { + xmlNode *match = pcmk__xpath_result(xpath_obj, i); + + if (match == NULL) { + continue; + } + + // @COMPAT See COMPAT comment in pcmk__apply_acls() + match = pcmk__xpath_match_element(match); + if (match == NULL) { + continue; + } + + if (!purge_xml_attributes(match) && (match == root)) { + xmlXPathFreeObject(xpath_obj); + filter_data->all_filtered = true; + return; + } + } + + pcmk__trace("ACLs deny user '%s' access to %s (%d match%s)", + docpriv->acl_user, acl->xpath, num_results, + pcmk__plural_alt(num_results, "", "es")); + xmlXPathFreeObject(xpath_obj); +} + /*! * \brief Copy ACL-allowed portions of specified XML * @@ -887,6 +951,7 @@ xml_acl_filtered_copy(const char *user, xmlNode *acl_source, xmlNode *xml, { xmlNode *target = NULL; xml_doc_private_t *docpriv = NULL; + struct acl_filter_doc_data data = { 0, }; *result = NULL; if ((acl_source == NULL) || (acl_source->doc == NULL) || (xml == NULL) @@ -902,45 +967,10 @@ xml_acl_filtered_copy(const char *user, xmlNode *acl_source, xmlNode *xml, pcmk__trace("Filtering XML copy using user '%s' ACLs", user); - for (const GList *iter = docpriv->acls; iter != NULL; iter = iter->next) { - const xml_acl_t *acl = iter->data; - xmlXPathObject *xpath_obj = NULL; - int num_results = 0; - - if ((acl->mode != pcmk__xf_acl_deny) || (acl->xpath == NULL)) { - continue; - } - - xpath_obj = pcmk__xpath_search(target->doc, acl->xpath); - num_results = pcmk__xpath_num_results(xpath_obj); - - for (int i = 0; i < num_results; i++) { - xmlNode *match = pcmk__xpath_result(xpath_obj, i); - - if (match == NULL) { - continue; - } - - // @COMPAT See COMPAT comment in pcmk__apply_acls() - match = pcmk__xpath_match_element(match); - if (match == NULL) { - continue; - } - - if (!purge_xml_attributes(match) && (match == target)) { - pcmk__trace("ACLs deny user '%s' access to entire XML document", - user); - xmlXPathFreeObject(xpath_obj); - return true; - } - } - pcmk__trace("ACLs deny user '%s' access to %s (%d match%s)", user, - acl->xpath, num_results, - pcmk__plural_alt(num_results, "", "es")); - xmlXPathFreeObject(xpath_obj); - } + data.doc = target->doc; + g_list_foreach(docpriv->acls, acl_filter_doc, &data); - if (!purge_xml_attributes(target)) { + if (data.all_filtered || !purge_xml_attributes(target)) { pcmk__trace("ACLs deny user '%s' access to entire XML document", user); return true; } From 38580426419a77288f0033259fb7334816865227 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Sat, 27 Dec 2025 23:40:07 -0800 Subject: [PATCH 10/16] Refactor: libcrmcommon: Functionize ACL-filtering one match Note to reviewer: this code is expected to be removed a few commits from now. This commit was part of my process of trying to understand the filtering logic. It may make review easier, or it may make it more tedious. Use pcmk__xpath_foreach_result(). Also drop a trace message, for two reasons: * We don't have easy access to num_results anymore. (We'd have to get it via a counter variable or something.) * The message tells us how many nodes were filtered by a given XPath, but not which nodes. So it doesn't seem especially useful. If we want tracing for ACL filtering (which seems like a reasonable thing to want), then we should make it more granular so that it's informative. Signed-off-by: Reid Wahl --- lib/common/acl.c | 75 +++++++++++++++++++++++++----------------------- 1 file changed, 39 insertions(+), 36 deletions(-) diff --git a/lib/common/acl.c b/lib/common/acl.c index ea30911cec3..82c5495f61a 100644 --- a/lib/common/acl.c +++ b/lib/common/acl.c @@ -870,13 +870,46 @@ purge_xml_attributes(xmlNode *xml) /*! * \internal - * \brief User data for \c acl_filter_doc() + * \brief User data for \c acl_filter_match() and \c acl_filter_doc() */ -struct acl_filter_doc_data { +struct acl_filter_data { xmlDoc *doc; //!< XML document being filtered bool all_filtered; //!< Whether access has been denied to entire document }; +/*! + * \internal + * \brief Filter a node that matches an ACL's XPath expression + * + * Given a node that matches an ACL's XPath expression, get the corresponding + * XML element. Then filter it. See \c purge_xml_atributes() for details. + * + * \param[in,out] match Node matched by the ACL's XPath expression + * \param[in,out] user_data User data (struct acl_filter_doc_data *) + * + * \note This is compatible with \c pcmk__xpath_foreach_result(). + */ +static void +acl_filter_match(xmlNode *match, void *user_data) +{ + struct acl_filter_data *data = user_data; + xmlNode *root = xmlDocGetRootElement(data->doc); + + if (data->all_filtered) { + return; + } + + // @COMPAT See COMPAT comment in pcmk__apply_acls() + match = pcmk__xpath_match_element(match); + if (match == NULL) { + return; + } + + if (!purge_xml_attributes(match) && (match == root)) { + data->all_filtered = true; + } +} + /*! * \internal * \brief Filter an XML document using one ACL @@ -888,12 +921,7 @@ static void acl_filter_doc(gpointer data, gpointer user_data) { const xml_acl_t *acl = data; - struct acl_filter_doc_data *filter_data = user_data; - - xmlNode *root = xmlDocGetRootElement(filter_data->doc); - xml_doc_private_t *docpriv = filter_data->doc->_private; - xmlXPathObject *xpath_obj = NULL; - int num_results = 0; + struct acl_filter_data *filter_data = user_data; if (filter_data->all_filtered) { return; @@ -903,33 +931,8 @@ acl_filter_doc(gpointer data, gpointer user_data) return; } - xpath_obj = pcmk__xpath_search(filter_data->doc, acl->xpath); - num_results = pcmk__xpath_num_results(xpath_obj); - - for (int i = 0; i < num_results; i++) { - xmlNode *match = pcmk__xpath_result(xpath_obj, i); - - if (match == NULL) { - continue; - } - - // @COMPAT See COMPAT comment in pcmk__apply_acls() - match = pcmk__xpath_match_element(match); - if (match == NULL) { - continue; - } - - if (!purge_xml_attributes(match) && (match == root)) { - xmlXPathFreeObject(xpath_obj); - filter_data->all_filtered = true; - return; - } - } - - pcmk__trace("ACLs deny user '%s' access to %s (%d match%s)", - docpriv->acl_user, acl->xpath, num_results, - pcmk__plural_alt(num_results, "", "es")); - xmlXPathFreeObject(xpath_obj); + pcmk__xpath_foreach_result(filter_data->doc, acl->xpath, acl_filter_match, + user_data); } /*! @@ -951,7 +954,7 @@ xml_acl_filtered_copy(const char *user, xmlNode *acl_source, xmlNode *xml, { xmlNode *target = NULL; xml_doc_private_t *docpriv = NULL; - struct acl_filter_doc_data data = { 0, }; + struct acl_filter_data data = { 0, }; *result = NULL; if ((acl_source == NULL) || (acl_source->doc == NULL) || (xml == NULL) From 454e554a0acd96be1873deaea29b8dc81322b82a Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Sun, 28 Dec 2025 00:56:51 -0800 Subject: [PATCH 11/16] Refactor: libcrmcommon: Check "no ACLs" sooner in xml_acl_filtered_copy There's no reason to filter all the children if we can already tell that we're going to deny the user access to the entire document. Signed-off-by: Reid Wahl --- lib/common/acl.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/lib/common/acl.c b/lib/common/acl.c index 82c5495f61a..1a45e5ab99f 100644 --- a/lib/common/acl.c +++ b/lib/common/acl.c @@ -970,6 +970,13 @@ xml_acl_filtered_copy(const char *user, xmlNode *acl_source, xmlNode *xml, pcmk__trace("Filtering XML copy using user '%s' ACLs", user); + if (docpriv->acls == NULL) { + pcmk__trace("User '%s' without ACLs denied access to entire XML " + "document", user); + pcmk__xml_free(target); + return true; + } + data.doc = target->doc; g_list_foreach(docpriv->acls, acl_filter_doc, &data); @@ -978,13 +985,6 @@ xml_acl_filtered_copy(const char *user, xmlNode *acl_source, xmlNode *xml, return true; } - if (docpriv->acls == NULL) { - pcmk__trace("User '%s' without ACLs denied access to entire XML " - "document", user); - pcmk__xml_free(target); - return true; - } - g_clear_pointer(&docpriv->acls, pcmk__free_acls); *result = target; return true; From 8d1b249ff55b5372b03521a0e2c4d6b5363e6634 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Sun, 28 Dec 2025 01:03:16 -0800 Subject: [PATCH 12/16] Refactor: libcrmcommon: Don't delete attrs if we're going to delete node This is less for efficiency and more for understandability, before we make a larger change to this code. Signed-off-by: Reid Wahl --- lib/common/acl.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/lib/common/acl.c b/lib/common/acl.c index 1a45e5ab99f..0c8e581c15e 100644 --- a/lib/common/acl.c +++ b/lib/common/acl.c @@ -848,8 +848,6 @@ purge_xml_attributes(xmlNode *xml) return true; } - pcmk__xe_remove_matching_attrs(xml, true, attr_is_not_id, NULL); - child = pcmk__xe_first_child(xml, NULL, NULL, NULL); while (child != NULL) { xmlNode *tmp = child; @@ -861,10 +859,14 @@ purge_xml_attributes(xmlNode *xml) } } - if (!readable_children) { + if (readable_children) { + pcmk__xe_remove_matching_attrs(xml, true, attr_is_not_id, NULL); + + } else { // Nothing readable under here, so purge completely pcmk__xml_free(xml); } + return readable_children; } From cec36ca39ae5d47d10a7b888e1419bc12a4466b5 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Sun, 28 Dec 2025 03:09:24 -0800 Subject: [PATCH 13/16] Refactor: libcrmcommon: Clarify xml_acl_filtered_copy() This is a substantial refactor. Most of the details are in the comments. It took me days to understand exactly what xml_acl_filtered_copy() was doing, particularly in the calls to purge_xml_attributes(). For a while I was pretty sure that we could be exposing "denied" children if an ancestor had the pcmk__xf_acl_read or pcmk__xf_acl_write flag set. I'm no longer confident that was the case, although it may have been. The whole process was convoluted. We unpacked all of a user's ACLs and applied them to matching nodes throughout the document. Then we went through the unpacked list of a user's ACLs (docpriv->acls) and found the ones with "deny" permissions and non-NULL xpath. (By the way, I believe all ACLs should have non-NULL xpath by that point.) Then for each of those "deny" ACLs, we filtered the document based on it. We ran an XPath query to find all the nodes in the document that matched the "deny" ACL, and we called purge_xml_attributes() on each such node. A return value of true indicated that the current node either was readable or had at least one readable descendant. * If the current node had the pcmk__xf_acl_read or pcmk__xf_acl_write flag set (is_mode_allowed()), then it was readable and we returned true immediately. Some read or write ACL had matched that node during the apply stage. Of course this could never be true for the top-level call, because the top-level call was always for some node that a "deny" ACL had matched. * Otherwise, the current node was unreadable. We called purge_xml_attributes() on all children. - If any children returned true, then we had at least one readable descendant. We removed all attributes except for ID and then returned true. - Otherwise, we had no readable descendants. Since the current node was also unreadable, we removed the current node's subtree and returned false. As mentioned, this was convoluted. The key observation is that before we did any of that, we had already applied all the ACLs throughout the document. So all we have to do is recurse from the top down, determine which nodes are readable for the current ACL user, and filter out the rest. The comments explain how this works. Signed-off-by: Reid Wahl --- lib/common/acl.c | 199 +++++++++++++++++++++++------------------------ 1 file changed, 98 insertions(+), 101 deletions(-) diff --git a/lib/common/acl.c b/lib/common/acl.c index 0c8e581c15e..c7bc85b62ce 100644 --- a/lib/common/acl.c +++ b/lib/common/acl.c @@ -826,169 +826,166 @@ attr_is_not_id(const xmlAttr *attr, void *user_data) /*! * \internal - * \brief Rid XML tree of all unreadable nodes and node properties + * \brief Filter out nodes that are unreadable by the current ACL user * - * \param[in,out] xml Root XML node to be purged of attributes + * Access or denial via ACLs is inherited, with more specific ACLs (those that + * match the node directly or match a more recent ancestor) taking precedence + * over less specific ones (those that match a less recent ancestor). Access is + * denied by default, if no ACL matches a node or any of its ancestors. * - * \return true if this node or any of its children are readable - * if false is returned, xml will be freed + * For each node in the tree, check whether any ACL matched the node. If so, + * then use that ACL for the current node. If not, then the current node + * inherits read access or denial from its parent. * - * \note This function is recursive + * Filter each child. Each child inherits read access or denial from the current + * node, unless an ACL matched the child directly. + * + * If the current node is readable, don't modify it. If it's unreadable but has + * at least one readable descendant, then remove all of its attributes other + * than \c PCMK_XA_ID. If it's unreadable and has no readable descendants, + * remove it. + * + * \param[in,out] xml XML tree (will be set to \c NULL if + * freed) + * \param[in] in_readable_context If \c true, the parent of \p xml is + * readable + * + * \note This function assumes that \c pcmk__apply_acls() has already been + * called for \p *xml->doc. + * \note This function is recursive. */ -static bool -purge_xml_attributes(xmlNode *xml) +static void +filter_unreadable_nodes(xmlNode **xml, bool in_readable_context) { + const xml_node_private_t *nodepriv = (*xml)->_private; + bool direct = false; + const char *how = NULL; + const char *name = (const char *) (*xml)->name; + const char *id = pcmk__s(pcmk__xe_id(*xml), "(unset)"); xmlNode *child = NULL; - bool readable_children = false; - xml_node_private_t *nodepriv = xml->_private; + /* If an ACL matched xml directly, update the context for xml and its + * descendants. Otherwise, keep the access that we inherited. + */ if (is_mode_allowed(nodepriv->flags, pcmk__xf_acl_read)) { - pcmk__trace("%s[@" PCMK_XA_ID "=%s] is readable", xml->name, - pcmk__xe_id(xml)); - return true; - } - - child = pcmk__xe_first_child(xml, NULL, NULL, NULL); - while (child != NULL) { - xmlNode *tmp = child; + in_readable_context = true; + direct = true; - child = pcmk__xe_next(child, NULL); - - if (purge_xml_attributes(tmp)) { - readable_children = true; - } + } else if (pcmk__is_set(nodepriv->flags, pcmk__xf_acl_deny)) { + in_readable_context = false; + direct = true; } - if (readable_children) { - pcmk__xe_remove_matching_attrs(xml, true, attr_is_not_id, NULL); + if (direct) { + how = "directly"; + + } else if (*xml == xmlDocGetRootElement((*xml)->doc)) { + how = "by default"; } else { - // Nothing readable under here, so purge completely - pcmk__xml_free(xml); + how = "by inheritance"; } - return readable_children; -} - -/*! - * \internal - * \brief User data for \c acl_filter_match() and \c acl_filter_doc() - */ -struct acl_filter_data { - xmlDoc *doc; //!< XML document being filtered - bool all_filtered; //!< Whether access has been denied to entire document -}; - -/*! - * \internal - * \brief Filter a node that matches an ACL's XPath expression - * - * Given a node that matches an ACL's XPath expression, get the corresponding - * XML element. Then filter it. See \c purge_xml_atributes() for details. - * - * \param[in,out] match Node matched by the ACL's XPath expression - * \param[in,out] user_data User data (struct acl_filter_doc_data *) - * - * \note This is compatible with \c pcmk__xpath_foreach_result(). - */ -static void -acl_filter_match(xmlNode *match, void *user_data) -{ - struct acl_filter_data *data = user_data; - xmlNode *root = xmlDocGetRootElement(data->doc); - - if (data->all_filtered) { - return; - } + pcmk__trace("ACLs %s read access to %s[@" PCMK_XA_ID "='%s'] %s", + (in_readable_context? "allow" : "deny"), name, id, how); - // @COMPAT See COMPAT comment in pcmk__apply_acls() - match = pcmk__xpath_match_element(match); - if (match == NULL) { - return; - } + // Filter children recursively + child = pcmk__xml_first_child(*xml); + while (child != NULL) { + xmlNode *next = pcmk__xml_next(child); - if (!purge_xml_attributes(match) && (match == root)) { - data->all_filtered = true; + filter_unreadable_nodes(&child, in_readable_context); + child = next; } -} -/*! - * \internal - * \brief Filter an XML document using one ACL - * - * \param[in] acl ACL object - * \param[in,out] user_data User data (struct acl_filter_doc_data *) - */ -static void -acl_filter_doc(gpointer data, gpointer user_data) -{ - const xml_acl_t *acl = data; - struct acl_filter_data *filter_data = user_data; - - if (filter_data->all_filtered) { + if (in_readable_context) { + // This node is readable, either directly or by inheritance return; } - if ((acl->mode != pcmk__xf_acl_deny) || (acl->xpath == NULL)) { + if ((*xml)->children != NULL) { + /* At least one descendant is readable, but this node is not. + * + * Remove all attributes except PCMK_XA_ID. Keep this node as a bare + * "scaffolding" element for structure, so that the path from the root + * to any readable descendant remains intact. Removing this node would + * also remove its readable descendants. + */ + pcmk__xe_remove_matching_attrs(*xml, true, attr_is_not_id, NULL); return; } - pcmk__xpath_foreach_result(filter_data->doc, acl->xpath, acl_filter_match, - user_data); + // This node and all its descendants are unreadable, so remove it + g_clear_pointer(xml, pcmk__xml_free); } /*! - * \brief Copy ACL-allowed portions of specified XML + * \brief Copy XML, filtering out portions that are unreadable based on ACLs * - * \param[in] user Username whose ACLs should be used + * Access or denial via ACLs is inherited, with more specific ACLs (those that + * match the node directly or match a more recent ancestor) taking precedence + * over less specific ones (those that match a less recent ancestor). Access is + * denied by default, if no ACL matches a node or any of its ancestors. + * + * If the user's ACLs grant read access to a node (either directly or through + * the most recent ancestor node matched by an ACL), then that node is kept + * intact. + * + * If the user's ACLs deny read access to a node (either directly, through the + * most recent ancestor node matched by an ACL, or by default), then: + * - If the current node has at least one readable descendant, then all of the + * current node's attributes are removed other than \c PCMK_XA_ID. + * - Otherwise, the current node is removed. + * + * \param[in] user User whose ACLs to use * \param[in] acl_source XML containing ACLs * \param[in] xml XML to be copied - * \param[out] result Copy of XML portions readable via ACLs + * \param[out] result Where to store ACL-filtered copy of \p xml (will be + * set to \c NULL if entire document is filtered out) * * \return \c true if \p acl_source and \p xml are non-NULL and ACLs * are required for \p user, or \c false otherwise * - * \note If this returns true, caller should use \p result rather than \p xml + * \note If this returns true, caller should use \p *result rather than \p xml. + * \note The caller is responsible for freeing \p *result using + * \c xmlFreeDoc((*result)->doc). The document and its nodes also contain + * Pacemaker-generated private data, which cannot be freed by external + * programs at this time. */ bool xml_acl_filtered_copy(const char *user, xmlNode *acl_source, xmlNode *xml, xmlNode **result) { - xmlNode *target = NULL; xml_doc_private_t *docpriv = NULL; - struct acl_filter_data data = { 0, }; - *result = NULL; if ((acl_source == NULL) || (acl_source->doc == NULL) || (xml == NULL) || !pcmk_acl_required(user)) { return false; } - target = pcmk__xml_copy(NULL, xml); - docpriv = target->doc->_private; + *result = pcmk__xml_copy(NULL, xml); - pcmk__enable_acls(acl_source->doc, target->doc, user); - - pcmk__trace("Filtering XML copy using user '%s' ACLs", user); + pcmk__enable_acls(acl_source->doc, (*result)->doc, user); + docpriv = (*result)->doc->_private; if (docpriv->acls == NULL) { pcmk__trace("User '%s' without ACLs denied access to entire XML " "document", user); - pcmk__xml_free(target); + g_clear_pointer(result, pcmk__xml_free); return true; } - data.doc = target->doc; - g_list_foreach(docpriv->acls, acl_filter_doc, &data); + pcmk__trace("Filtering XML copy using user '%s' ACLs", user); + filter_unreadable_nodes(result, false); - if (data.all_filtered || !purge_xml_attributes(target)) { + if (*result == NULL) { + // Entire document was freed, so don't free docpriv->acls here pcmk__trace("ACLs deny user '%s' access to entire XML document", user); return true; } g_clear_pointer(&docpriv->acls, pcmk__free_acls); - *result = target; return true; } From a2a2068f58a23008783262e40e6240d9090c39e8 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Sun, 28 Dec 2025 19:40:09 -0800 Subject: [PATCH 14/16] Refactor: libcrmcommon: New pcmk__acl_filtered_copy() To replace xml_acl_filtered_copy(). Signed-off-by: Reid Wahl --- include/crm/common/acl_internal.h | 3 + lib/cib/cib_utils.c | 14 ++--- lib/common/acl.c | 97 ++++++++++++++++++++++--------- 3 files changed, 81 insertions(+), 33 deletions(-) diff --git a/include/crm/common/acl_internal.h b/include/crm/common/acl_internal.h index cb4930bfaa1..5c7403ab915 100644 --- a/include/crm/common/acl_internal.h +++ b/include/crm/common/acl_internal.h @@ -38,6 +38,9 @@ pcmk__is_privileged(const char *user) void pcmk__enable_acls(xmlDoc *source, xmlDoc *target, const char *user); +xmlNode *pcmk__acl_filtered_copy(const char *user, xmlDoc *acl_source, + xmlNode *xml); + bool pcmk__check_acl(xmlNode *xml, const char *attr_name, enum pcmk__xml_flags mode); diff --git a/lib/cib/cib_utils.c b/lib/cib/cib_utils.c index 5a9c50c9e6b..275f9f113c8 100644 --- a/lib/cib/cib_utils.c +++ b/lib/cib/cib_utils.c @@ -233,8 +233,8 @@ cib__perform_op_ro(cib__op_fn_t fn, xmlNode *req, xmlNode **current_cib, cib = *current_cib; - if (cib_acl_enabled(cib, user) - && xml_acl_filtered_copy(user, cib, cib, &cib_filtered)) { + if (cib_acl_enabled(cib, user)) { + cib_filtered = pcmk__acl_filtered_copy(user, cib->doc, cib); if (cib_filtered == NULL) { pcmk__debug("Pre-filtered the entire cib"); @@ -662,12 +662,12 @@ cib__perform_op_rw(enum cib_variant variant, cib__op_fn_t fn, xmlNode *req, if ((rc != pcmk_rc_ok) && cib_acl_enabled(old_versions, user)) { xmlNode *saved_cib = *cib; - if (xml_acl_filtered_copy(user, old_versions, *cib, cib)) { - if (*cib == NULL) { - pcmk__debug("Pre-filtered the entire cib result"); - } - pcmk__xml_free(saved_cib); + *cib = pcmk__acl_filtered_copy(user, old_versions->doc, *cib); + if (*cib == NULL) { + pcmk__debug("Pre-filtered the entire cib result"); } + + pcmk__xml_free(saved_cib); } pcmk__xml_free(top); diff --git a/lib/common/acl.c b/lib/common/acl.c index c7bc85b62ce..da5828d09a6 100644 --- a/lib/common/acl.c +++ b/lib/common/acl.c @@ -919,6 +919,73 @@ filter_unreadable_nodes(xmlNode **xml, bool in_readable_context) g_clear_pointer(xml, pcmk__xml_free); } +/*! + * \internal + * \brief Copy XML, filtering out portions that are unreadable based on ACLs + * + * Access or denial via ACLs is inherited, with more specific ACLs (those that + * match the node directly or match a more recent ancestor) taking precedence + * over less specific ones (those that match a less recent ancestor). Access is + * denied by default, if no ACL matches a node or any of its ancestors. + * + * If the user's ACLs grant read access to a node (either directly or through + * the most recent ancestor node matched by an ACL), then that node is kept + * intact. + * + * If the user's ACLs deny read access to a node (either directly, through the + * most recent ancestor node matched by an ACL, or by default), then: + * - If the current node has at least one readable descendant, then all of the + * current node's attributes are removed other than \c PCMK_XA_ID. + * - Otherwise, the current node is removed. + * + * \param[in] user User whose ACLs to use + * \param[in] acl_source XML document whose ACL definitions to use + * \param[in] xml XML to copy + * + * \return Newly allocated ACL-filtered copy of \p xml, or \c NULL if the entire + * document is filtered out + * + * \note The caller is responsible for freeing the return value using + * \c pcmk__xml_free(). + */ +xmlNode * +pcmk__acl_filtered_copy(const char *user, xmlDoc *acl_source, xmlNode *xml) +{ + xmlNode *result = NULL; + xml_doc_private_t *docpriv = NULL; + + pcmk__assert((acl_source != NULL) && (xml != NULL)); + + result = pcmk__xml_copy(NULL, xml); + + if (!pcmk_acl_required(user)) { + // Return an unfiltered copy + return result; + } + + pcmk__enable_acls(acl_source->doc, result->doc, user); + + docpriv = result->doc->_private; + if (docpriv->acls == NULL) { + pcmk__trace("User '%s' without ACLs denied access to entire XML " + "document", user); + pcmk__xml_free(result); + return NULL; + } + + pcmk__trace("Filtering XML copy using user '%s' ACLs", user); + filter_unreadable_nodes(&result, false); + + if (result == NULL) { + // Entire document was freed, so don't free docpriv->acls here + pcmk__trace("ACLs deny user '%s' access to entire XML document", user); + return NULL; + } + + g_clear_pointer(&docpriv->acls, pcmk__free_acls); + return result; +} + /*! * \brief Copy XML, filtering out portions that are unreadable based on ACLs * @@ -943,8 +1010,9 @@ filter_unreadable_nodes(xmlNode **xml, bool in_readable_context) * \param[out] result Where to store ACL-filtered copy of \p xml (will be * set to \c NULL if entire document is filtered out) * - * \return \c true if \p acl_source and \p xml are non-NULL and ACLs - * are required for \p user, or \c false otherwise + * \return \c true if \p acl_source, \p acl_source->doc, and \p xml are + * non-NULL and ACLs are required for \p user, or \c false + * otherwise * * \note If this returns true, caller should use \p *result rather than \p xml. * \note The caller is responsible for freeing \p *result using @@ -956,36 +1024,13 @@ bool xml_acl_filtered_copy(const char *user, xmlNode *acl_source, xmlNode *xml, xmlNode **result) { - xml_doc_private_t *docpriv = NULL; - if ((acl_source == NULL) || (acl_source->doc == NULL) || (xml == NULL) || !pcmk_acl_required(user)) { return false; } - *result = pcmk__xml_copy(NULL, xml); - - pcmk__enable_acls(acl_source->doc, (*result)->doc, user); - - docpriv = (*result)->doc->_private; - if (docpriv->acls == NULL) { - pcmk__trace("User '%s' without ACLs denied access to entire XML " - "document", user); - g_clear_pointer(result, pcmk__xml_free); - return true; - } - - pcmk__trace("Filtering XML copy using user '%s' ACLs", user); - filter_unreadable_nodes(result, false); - - if (*result == NULL) { - // Entire document was freed, so don't free docpriv->acls here - pcmk__trace("ACLs deny user '%s' access to entire XML document", user); - return true; - } - - g_clear_pointer(&docpriv->acls, pcmk__free_acls); + *result = pcmk__acl_filtered_copy(user, acl_source->doc, xml); return true; } From eaa3b84d63358a23f6a662b608eabb9d4aee8650 Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Sun, 28 Dec 2025 20:08:24 -0800 Subject: [PATCH 15/16] API: libcrmcommon: Deprecate xml_acl_filtered_copy() Perform a CIB query as a given user if you want to filter CIB XML as that user. This function shouldn't have been used for arbitrary (non-CIB) XML anyway. Signed-off-by: Reid Wahl --- include/crm/common/acl.h | 2 -- include/crm/common/acl_compat.h | 4 +++ lib/common/acl.c | 62 ++++++++------------------------- 3 files changed, 18 insertions(+), 50 deletions(-) diff --git a/include/crm/common/acl.h b/include/crm/common/acl.h index bff52f63417..3b273e2dddb 100644 --- a/include/crm/common/acl.h +++ b/include/crm/common/acl.h @@ -25,8 +25,6 @@ extern "C" { void xml_acl_disable(xmlNode *xml); bool xml_acl_denied(const xmlNode *xml); -bool xml_acl_filtered_copy(const char *user, xmlNode* acl_source, xmlNode *xml, - xmlNode **result); bool pcmk_acl_required(const char *user); diff --git a/include/crm/common/acl_compat.h b/include/crm/common/acl_compat.h index a87db5ca26f..37e73a93d48 100644 --- a/include/crm/common/acl_compat.h +++ b/include/crm/common/acl_compat.h @@ -30,6 +30,10 @@ extern "C" { //! \deprecated Do not use bool xml_acl_enabled(const xmlNode *xml); +//! \deprecated Do not use +bool xml_acl_filtered_copy(const char *user, xmlNode *acl_source, xmlNode *xml, + xmlNode **result); + #ifdef __cplusplus } #endif diff --git a/lib/common/acl.c b/lib/common/acl.c index da5828d09a6..aa6a95d76e5 100644 --- a/lib/common/acl.c +++ b/lib/common/acl.c @@ -986,54 +986,6 @@ pcmk__acl_filtered_copy(const char *user, xmlDoc *acl_source, xmlNode *xml) return result; } -/*! - * \brief Copy XML, filtering out portions that are unreadable based on ACLs - * - * Access or denial via ACLs is inherited, with more specific ACLs (those that - * match the node directly or match a more recent ancestor) taking precedence - * over less specific ones (those that match a less recent ancestor). Access is - * denied by default, if no ACL matches a node or any of its ancestors. - * - * If the user's ACLs grant read access to a node (either directly or through - * the most recent ancestor node matched by an ACL), then that node is kept - * intact. - * - * If the user's ACLs deny read access to a node (either directly, through the - * most recent ancestor node matched by an ACL, or by default), then: - * - If the current node has at least one readable descendant, then all of the - * current node's attributes are removed other than \c PCMK_XA_ID. - * - Otherwise, the current node is removed. - * - * \param[in] user User whose ACLs to use - * \param[in] acl_source XML containing ACLs - * \param[in] xml XML to be copied - * \param[out] result Where to store ACL-filtered copy of \p xml (will be - * set to \c NULL if entire document is filtered out) - * - * \return \c true if \p acl_source, \p acl_source->doc, and \p xml are - * non-NULL and ACLs are required for \p user, or \c false - * otherwise - * - * \note If this returns true, caller should use \p *result rather than \p xml. - * \note The caller is responsible for freeing \p *result using - * \c xmlFreeDoc((*result)->doc). The document and its nodes also contain - * Pacemaker-generated private data, which cannot be freed by external - * programs at this time. - */ -bool -xml_acl_filtered_copy(const char *user, xmlNode *acl_source, xmlNode *xml, - xmlNode **result) -{ - if ((acl_source == NULL) || (acl_source->doc == NULL) || (xml == NULL) - || !pcmk_acl_required(user)) { - - return false; - } - - *result = pcmk__acl_filtered_copy(user, acl_source->doc, xml); - return true; -} - /*! * \internal * \brief Check whether creation of an XML element is implicitly allowed @@ -1407,5 +1359,19 @@ xml_acl_enabled(const xmlNode *xml) return false; } +bool +xml_acl_filtered_copy(const char *user, xmlNode *acl_source, xmlNode *xml, + xmlNode **result) +{ + if ((acl_source == NULL) || (acl_source->doc == NULL) || (xml == NULL) + || !pcmk_acl_required(user)) { + + return false; + } + + *result = pcmk__acl_filtered_copy(user, acl_source->doc, xml); + return true; +} + // LCOV_EXCL_STOP // End deprecated API From ca7affa3753ef7cbb0e47e76df8fac39ad75093b Mon Sep 17 00:00:00 2001 From: Reid Wahl Date: Tue, 16 Jun 2026 18:58:10 -0700 Subject: [PATCH 16/16] Low: libcrmcommon: Fix Coverity check_after_deref false positive Signed-off-by: Reid Wahl --- lib/common/acl.c | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/common/acl.c b/lib/common/acl.c index aa6a95d76e5..5ebf4daf716 100644 --- a/lib/common/acl.c +++ b/lib/common/acl.c @@ -976,6 +976,7 @@ pcmk__acl_filtered_copy(const char *user, xmlDoc *acl_source, xmlNode *xml) pcmk__trace("Filtering XML copy using user '%s' ACLs", user); filter_unreadable_nodes(&result, false); + // coverity[check_after_deref : FALSE] Doesn't understand g_clear_pointer if (result == NULL) { // Entire document was freed, so don't free docpriv->acls here pcmk__trace("ACLs deny user '%s' access to entire XML document", user);