On Mon, Sep 21, 2026 at 9:20 AM Bharath Rupireddy <[email protected]> wrote: > > Hi, > > On Sun, Sep 20, 2026 at 11:24 PM Amit Kapila <[email protected]> wrote: > > > > 3. > > Restrict who can emit messages > > + on the publisher. Revoke the default privilege and grant it to a > > role created > > > > That above security recipe sounds reasonable, but it asks the > > subscriber's security to be enforced by an action in a cluster the > > subscriber admin frequently does not control. The Security section > > should say something on the lines of: if you don't administer both the > > subscriber and publisher, don't install a message handler that does > > anything you wouldn't let every publisher role do. Also, I think the > > above REVOKE has to be performed in all databases separately. > > > > 4. Based on above two points, I think we can fill the hole with something > > like: > > > > CREATE SUBSCRIPTION ... WITH (messages = true, message_handler_role = > > 'msg_handler') > > > > and then run the hook under SwitchToUntrustedUser(that_role). That > > gives the subscriber admin a local, explicit privilege mechanism that > > does not depend on the publisher's ACLs, doesn't touch WAL or the > > protocol, and reuses machinery already in usercontext.c. If the option > > is unset, either don't call the hook or keep today's behavior with the > > documented warning. > > I like this idea more than all the other approaches. The user > installing the hook would have to make a conscious choice, and even if > the role applying the message is more privileged than the emitter on > the publisher, that is the user's choice. Also, I don't think > defaulting the message handler role to the subscription owner (which > can lead to the same security concern) or not calling the hook > silently (which can lead to silent data loss) are good defaults. So, I > think requiring the handler role to be specified by the user is a good > choice, in my opinion. > > That said, not having a default role would make the ALTER SUBSCRIPTION > code for this option a bit more complex I guess (I haven't checked it > in depth though).
My concern is that since the handler is a C function, this doesn't help much as a security boundary. The handler can get back to the subscription owner by calling SetUserIdAndSecContext() with MySubscription->owner or even switch to the superuser. That function does no permission check, and since the security context is its second argument, the same call clears SECURITY_RESTRICTED_OPERATION as well. As I mentioned in the reply to Amit, the option would help in cases where the handler doesn't switch to the target role, or forgets to, and keeps working as the subscription owner unexpectedly. But I'm not sure we need this option just for that case. > > On Sun, Sep 20, 2026 at 11:54 PM Amit Kapila <[email protected]> wrote: > > > > One more comment: > > +static void > > +apply_handle_message(StringInfo s) > > +{ > > + LogicalRepMessageData msg; > > + > > + /* Tablesync worker should never receive MESSAGE */ > > + if (am_tablesync_worker()) > > + ereport(ERROR, > > + errcode(ERRCODE_PROTOCOL_VIOLATION), > > + errmsg_internal("tablesync worker received a MESSAGE message")); > > > > I think we should have a similar ERROR for parallel apply worker. I > > don't think this message can reach there but we should protect it like > > above. > > Also, error out for the sequence sync worker? I agree to add a check for parallel apply workers, but I don't think we need it for the sequence sync workers as they neither enter the apply loop nor receive logical replication changes in the first place. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com
