<div dir="ltr">Hi Yugo<div><br></div><div>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.</div><div><br></div><div>I have committed the update patch</div><div><a href="http://git.postgresql.org/gitweb?p=pgpool2.git;a=commitdiff;h=88fdc5f2f2df98b1c6b5e8785924c6a3eabd1aed" rel="noreferrer" target="_blank" style="font-size:12.8px">http://git.postgresql.org/gitweb?p=pgpool2.git;a=commitdiff;h=88fdc5f2f2df98b1c6b5e8785924c6a3eabd1aed</a><br><div class="gmail_extra"><br></div><div class="gmail_extra"><br></div><div class="gmail_extra">Regards</div><div class="gmail_extra">Muhammad Usama</div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Dec 24, 2015 at 3:01 PM, Yugo Nagata <span dir="ltr"><<a href="mailto:nagata@sraoss.co.jp" target="_blank">nagata@sraoss.co.jp</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Hi Usama,<br>
<br>
I reviewed the patch and basically I think this is good to be commited.<br>
Here are some comments:<br>
<br>
(1)<br>
WD_IPC_AUTH_KEY and WD_IPC_SHARED_KEY are defined in watchdog.h. However,<br>
other json keys (e.g. "FailoverCMDType", "SyncRequestType", and so on) are<br>
hardcoded but I think it is better to define them similarly.<br>
<br>
(2)<br>
In check_IPC_client_authetication(), json value of WD_IPC_SHARED_KEY<br>
is checked and if it doesn't exist, watchdog says "invalid json data packet",<br>
but I think this is valid even if it is the packet from external and has only<br>
WD_IPC_AUTH_KEY. And WD_IPC_SHARED_KEY check is needed only when<br>
internal_client_only is ture, so this check can be in<br>
"if (internal_client_only) {...}" block.<br>
<br>
(3)<br>
a very small typo:<br>
<br>
+ * if internal_client_only is true than the JSON data must contain the<br>
+ * shared key present in the pgpool-II shared memory. This can be used<br>
<br>
than -> then<br>
<span class=""><br>
On Wed, 23 Dec 2015 20:17:38 +0500<br>
Muhammad Usama <<a href="mailto:m.usama@gmail.com">m.usama@gmail.com</a>> wrote:<br>
<br>
> Hi Yugo<br>
><br>
> As you mentioned in your watchdog code review comments that there should be<br>
> a mechanism to authenticate the watchdog IPC commands, Although this was<br>
> also one of the TODO items in my list but somehow slipped out of radar.<br>
><br>
> So please find the attached patch for the purpose.<br>
><br>
</span>> Basically the patch utilizes the *wd_authkey* configuration parameter to<br>
<span class="">> authenticate the watchdog IPC command clients. And since the IPC is meant<br>
> for communication between internal pgpool-II processes and with external<br>
> processes on the same machine, so instead of calculating the hash and<br>
> inventing a more complicated process of IPC authentication, the simple<br>
> wd_authkey matching is used to validate the clients. And when the<br>
> wd_authkey is not provided by the user in pgpool.conf than the<br>
> authentication is disabled.<br>
><br>
> The patch also takes care of other authentication related problem in the<br>
> watchdog IPC system. Since there are some watchdog functions visible by the<br>
> IPC commands, and we want to restrict their access to the outer world. Like<br>
> for example the interlocking command functions and failover related<br>
> functions. So for those functions, the patch creates a shared key in the<br>
> shared memory, and execution of these functions is only allowed if that<br>
> shard key in that function data matches with the current shred key in<br>
> shared memory. And when some external client tries to call these internal<br>
> only functions, the authentication error will be returned to that since the<br>
> key is secret to the pgpool-II only.<br>
><br>
</span>> So what are your thoughts on the mechanism and using the *wd_authkey* for<br>
<div class=""><div class="h5">> the purpose.<br>
><br>
> Thanks<br>
> Best regards<br>
> Muhammad Usama<br>
<br>
<br>
</div></div><span class=""><font color="#888888">--<br>
Yugo Nagata <<a href="mailto:nagata@sraoss.co.jp">nagata@sraoss.co.jp</a>><br>
</font></span></blockquote></div><br></div></div></div>