[pgpool-hackers: 1346] Re: new watchdog IPC authentication mechanism

[Yugo Nagata]

Hi Usama,

I thought IPC_SHARED_KEY method was secure because the packet was
sent using UNIX domain socket and nobody could read this directly.
Hoever, I found the JSON data including IPC_SHARED_KEY was printed
in the debug log messages, so anyone who watched debug messages is
able to know the key and issue watchdog internal commands. Although
the debug mode isn't used usually in product systems, it isn't secure
so much.
 
The log is printed at process_pgpool_replicate_command().
I don't know whether there are ohter parts printing the json data.

4719     ereport(DEBUG1,
4720         (errmsg("processing pgpool replicate variable command"),
4721              errdetail("%s",pkt->data)));

I think it takes times to rewrite the authentication method, so
this should be left for the future, which might be minnorrelease.
It would be good to resolve by removing the codes at this time.

On Thu, 24 Dec 2015 19:01:35 +0900
Yugo Nagata <nagata at sraoss.co.jp> wrote:

> Hi Usama,
> 
> I reviewed the patch and basically I think this is good to be commited.
> Here are some comments:
> 
> (1)
> WD_IPC_AUTH_KEY and WD_IPC_SHARED_KEY are defined in watchdog.h. However,
> other json keys (e.g. "FailoverCMDType", "SyncRequestType", and so on) are
> hardcoded but I think it is better to define them similarly.
> 
> (2)
> In check_IPC_client_authetication(), json value of WD_IPC_SHARED_KEY
> is checked and if it doesn't exist, watchdog says "invalid json data packet",
> but I think this is valid even if it is the packet from external and has only
> WD_IPC_AUTH_KEY. And WD_IPC_SHARED_KEY check is needed only when 
> internal_client_only is ture, so this check can be in 
> "if (internal_client_only) {...}" block.
> 
> (3)
> a very small typo:
> 
> + * if internal_client_only is true than the JSON data must contain the
> + * shared key present in the pgpool-II shared memory. This can be used
> 
> than -> then
> 
> On Wed, 23 Dec 2015 20:17:38 +0500
> Muhammad Usama <m.usama at gmail.com> wrote:
> 
> > Hi Yugo
> > 
> > As you mentioned in your watchdog code review comments that there should be
> > a mechanism to authenticate the watchdog IPC commands, Although this was
> > also one of the TODO items in my list but somehow slipped out of radar.
> > 
> > So please find the attached patch for the purpose.
> > 
> > Basically the patch utilizes the *wd_authkey* configuration parameter to
> > authenticate the watchdog IPC command clients. And since the IPC is meant
> > for communication between internal pgpool-II processes and with external
> > processes on the same machine, so instead of calculating the hash and
> > inventing a more complicated process of IPC authentication, the simple
> > wd_authkey matching is used to validate the clients. And when the
> > wd_authkey is not provided by the user in pgpool.conf than the
> > authentication is disabled.
> > 
> > The patch also takes care of other authentication related problem in the
> > watchdog IPC system. Since there are some watchdog functions visible by the
> > IPC commands, and we want to restrict their access to the outer world. Like
> > for example the interlocking command functions and failover related
> > functions. So for those functions, the patch creates a shared key in the
> > shared memory, and execution of these functions is only allowed if that
> > shard key in that function data matches with the current shred key in
> > shared memory. And when some external client tries to call these internal
> > only functions, the authentication error will be returned to that since the
> > key is secret to the pgpool-II only.
> > 
> > So what are your thoughts on the mechanism and using the *wd_authkey* for
> > the purpose.
> > 
> > Thanks
> > Best regards
> > Muhammad Usama
> 
> 
> -- 
> Yugo Nagata <nagata at sraoss.co.jp>
> _______________________________________________
> pgpool-hackers mailing list
> pgpool-hackers at pgpool.net
> http://www.pgpool.net/mailman/listinfo/pgpool-hackers


-- 
Yugo Nagata <nagata at sraoss.co.jp>