Why are "self-addressed" privileged IQs forwarded back to the sending component? #4346
-
When mod_privilege is enabled and a component sends a privileged IQ where the recipient is the same as the impersonated user: <iq id="priv_iq_1" from="matridge.aptnet.home.arpa" to="[email protected]" type="get">
<privileged_iq xmlns='urn:xmpp:privilege:2'>
<iq xmlns='jabber:client' id="b212a556567e4a53aa42b5ca9658e9d5"
type="get"
to="[email protected]">
<pubsub xmlns="http://jabber.org/protocol/pubsub"><items node="urn:xmpp:avatar:metadata" /></pubsub>
</iq>
</privileged_iq>
</iq> Then this transform rewrites the packet to wrap it in <iq to='matridge.aptnet.home.arpa' from='[email protected]' type='get' id='priv_iq_1'>
<privilege xmlns='urn:xmpp:privilege:2'>
<forwarded xmlns='urn:xmpp:forward:0'>
<iq to='[email protected]' from='[email protected]' type='get' id='b212a556567e4a53aa42b5ca9658e9d5' xmlns='jabber:client'>
<pubsub xmlns='http://jabber.org/protocol/pubsub'><items node='urn:xmpp:avatar:metadata'/></pubsub>
</iq>
</forwarded>
</privilege>
</iq> Why does ejabberd do this rewrite in this specific case? There's no comment about it in the code, and it was added as part of the larger blob implementing XEP-0356 support, so there's no detail in the commit message about what this is for. This looks like the mechanism XEP-0356 specifies for wrapping and returning the results of privileged IQs (section 6.3, particularly Example 11), but as far as I can tell there's no mention of forwarding requests in this manner. In general, I don't think it's safe to assume that the sending component is in a position to satisfy this request, forwarded or not, so I'm not sure this should be routed back to the original component; besides that, XEP-0356 is set up so that privileged IQ requests are transparent to the recipient entity:
That's not the case here, because the original IQ has been wrapped up in a forwarding and privilege element, so the receiving entity (which is the original sending component, in this case) would need to be set up to check for those in order to satisfy the request, unlike a regular non-privileged IQ request with the same payload. In real-world practical terms, this shows up in slidge-based transports, which are using these "self-addressed" privileged IQs to create and manage MDS pubsub nodes -- I haven't looked into it enough to know if that's the correct/reasonable way to do that, but at least on the surface it seems like a plausible use-case. Finally, ejabberd seems to be doing all the right things as long as the recipient and impersonated user aren't the same, and the packet rewrite doesn't trigger -- the server receives the IQ response, and correctly wraps the response up and forwards it back to the sending component. I'm inclined to just remove that packet-rewrite, but I'd like to know why it's there first. |
Beta Was this translation helpful? Give feedback.
Replies: 2 comments 1 reply
-
Ah, this seems important: removing the packet rewrite seems to interfere with the correct wrapping and routing of privileged-IQ response packets (when the recipient and impersonated users are not the same, the packet ends up being dropped as indicated in the logs below). I guess I need to check the details, but what seems to be happening here is that rewrite was meant to handle the forwarding of response packets only, and the bug is that it treats self-addressed request packets as response packets.
|
Beta Was this translation helpful? Give feedback.
-
Hi! I see you were able to investigate this, solve it, and published #4348. There's also #4341 fixing other problem in mod_privilege. Are you finished with those PR and consider them ready for merging, or is there any last minute thing that should be addressed? |
Beta Was this translation helpful? Give feedback.
Both PRs are ready, as far as I'm concerned. The only issue I can think of is that they're a bit behind tip, but they should be easy rebases. I can do that here (but then somebody's going to have to re-trigger the test jobs and whatnot, I think?) or you guys could probably do it on merge.