<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Jan 21, 2016 at 2:31 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 thought IPC_SHARED_KEY method was secure because the packet was<br>
sent using UNIX domain socket and nobody could read this directly.<br>
Hoever, I found the JSON data including IPC_SHARED_KEY was printed<br>
in the debug log messages, so anyone who watched debug messages is<br>
able to know the key and issue watchdog internal commands. Although<br>
the debug mode isn't used usually in product systems, it isn't secure<br>
so much.<br>
<br>
The log is printed at process_pgpool_replicate_command().<br>
I don't know whether there are ohter parts printing the json data.<br>
<br>
4719 ereport(DEBUG1,<br>
4720 (errmsg("processing pgpool replicate variable command"),<br>
4721 errdetail("%s",pkt->data)));<br>
<br>
I think it takes times to rewrite the authentication method, so<br>
this should be left for the future, which might be minnorrelease.<br>
It would be good to resolve by removing the codes at this time.<br>
<div><div class="h5"><br></div></div></blockquote><div><br></div><div>Yes, good point. The shared key should not be printed in the log message. But as far as an authentication method is concerned. I think it is good enough. Although it is not bullet-proof, but since IPC packets are not traveling on the wire and doesn't leave the system running the pgpool-II server, so simple shared key matching can serve the purpose. As if somehow the system itself is compromised so that an attacker is able to sniff the shared key from IPC communication, He/she could also be able access the pgpool-II config file that contains the wd_authkey, and could use that to launch the attack</div><div><br></div><div>Thanks</div><div>Best regards</div><div>Muhammad Usama</div><div><br></div><div> </div><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"><div><div class="h5">
On Thu, 24 Dec 2015 19:01:35 +0900<br>
Yugo Nagata <<a href="mailto:nagata@sraoss.co.jp">nagata@sraoss.co.jp</a>> wrote:<br>
<br>
> 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>
><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>
> > Basically the patch utilizes the *wd_authkey* configuration parameter to<br>
> > 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>
> > So what are your thoughts on the mechanism and using the *wd_authkey* for<br>
> > the purpose.<br>
> ><br>
> > Thanks<br>
> > Best regards<br>
> > Muhammad Usama<br>
><br>
><br>
> --<br>
> Yugo Nagata <<a href="mailto:nagata@sraoss.co.jp">nagata@sraoss.co.jp</a>><br>
</div></div>> _______________________________________________<br>
> pgpool-hackers mailing list<br>
> <a href="mailto:pgpool-hackers@pgpool.net">pgpool-hackers@pgpool.net</a><br>
> <a href="http://www.pgpool.net/mailman/listinfo/pgpool-hackers" rel="noreferrer" target="_blank">http://www.pgpool.net/mailman/listinfo/pgpool-hackers</a><br>
<span class=""><font color="#888888"><br>
<br>
--<br>
Yugo Nagata <<a href="mailto:nagata@sraoss.co.jp">nagata@sraoss.co.jp</a>><br>
</font></span></blockquote></div><br></div></div>