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

[Muhammad Usama]

Hi Yugo

Many thanks for your valuable input on the patch. I have taken care of the
comments except of #1, which I will take care of, as a separate commit.

I have committed the update patch
http://git.postgresql.org/gitweb?p=pgpool2.git;a=commitdiff;h=88fdc5f2f2df98b1c6b5e8785924c6a3eabd1aed


Regards
Muhammad Usama

On Thu, Dec 24, 2015 at 3:01 PM, 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>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.sraoss.jp/pipermail/pgpool-hackers/attachments/20151229/6a8a40f6/attachment-0001.html>