Re: Patch : PGPASSFILE fix

On Wed, Mar 4, 2015 at 4:40 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:

>
>
> On Wed, Mar 4, 2015 at 11:06 AM, Ashesh Vashi <
> ashesh(dot)vashi(at)enterprisedb(dot)com> wrote:
>
>> On Wed, Mar 4, 2015 at 4:09 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:
>>
>>> I think we should try to create the full path if necessary, and simply
>>> throw an error if we can't.
>>
>> And, I think - we should switch back to default pgpass configuration
>> file.
>>
>
> No, because that's a security risk (writing the password to a file that
> wasn't what the user intended).
>
Agree.

--

Thanks & Regards,

Ashesh Vashi
EnterpriseDB INDIA: Enterprise PostgreSQL Company
<http://www.enterprisedb.com>

*http://www.linkedin.com/in/asheshvashi*
<http://www.linkedin.com/in/asheshvashi>

>
>
>>
>> --
>>
>> Thanks & Regards,
>>
>> Ashesh Vashi
>> EnterpriseDB INDIA: Enterprise PostgreSQL Company
>> <http://www.enterprisedb.com>
>>
>>
>> *http://www.linkedin.com/in/asheshvashi*
>> <http://www.linkedin.com/in/asheshvashi>
>>
>>>
>>> On Wed, Mar 4, 2015 at 10:01 AM, Prasad <prasad(dot)s(at)mail(dot)com> wrote:
>>> > Alright , I'll revert to PGPASS check.
>>> > Existing function only creates folder containing file. With this case,
>>> whats expected ? Reading value in PGPASSFILE and try to create folder
>>> containing pgpass file (Assuming it's valid path)? Remember, it's
>>> environment variable. User can specify anything in there. Some garbage
>>> value as well. If we don't do any validation there, user will automatically
>>> see error with complain about file ?
>>> >
>>> > thanks and regards,
>>> > Prasad
>>> >
>>> >
>>> > Sent: Wednesday, March 04, 2015 at 7:48 AM
>>> > From: "Ashesh Vashi" <ashesh(dot)vashi(at)enterprisedb(dot)com>
>>> > To: Prasad <prasad(dot)s(at)mail(dot)com>
>>> > Cc: pgadmin-hackers <pgadmin-hackers(at)postgresql(dot)org>
>>> > Subject: Re: [pgadmin-hackers] Patch : PGPASSFILE fix
>>> >
>>> > On Wed, Mar 4, 2015 at 8:44 AM, Prasad <prasad(dot)s(at)mail(dot)com> wrote:
>>> >
>>> > Ashesh,
>>> >
>>> > Thanks for reviewing patch,
>>> > Code I have removed in I think, was switch statement inside if
>>> condition, which doesn't make sense.
>>> > ie.
>>> > if (var == 2)
>>> > {
>>> > switch (var)
>>> > case 2:
>>> > .....
>>> > break;
>>> > }
>>> >
>>> > that's why I removed it, because it's redundant.
>>> > Agree about redundancy, but you've also removed the code for checking
>>> the PGPASS check at the start of the function.
>>> > i.e.
>>> > @@ -762,35 +762,33 @@ void sysSettings::SetCanonicalLanguage(const
>>> wxLanguage &lang)
>>> >
>>> //////////////////////////////////////////////////////////////////////////
>>> > wxString sysSettings::GetConfigFile(configFileName cfgname)
>>> > {
>>> > - if (cfgname == PGPASS)
>>> > - {
>>> >
>>> > I am not agree with that.
>>> > About creation of directory, I'm not sure if this validation is
>>> required. Existing code creates directory postgresql (only on windows)
>>> according to
>>> http://www.postgresql.org/docs/9.3/static/libpq-pgpass.html[http://www.postgresql.org/docs/9.3/static/libpq-pgpass.html]
>>> , and it doesn't create file. I'm not sure whether this kind of validation
>>> is expected in this function.
>>> > I think - it is.
>>> > Because - it could be used to save the updated password in the PGPASS
>>> file.
>>> >
>>> > -- Ashesh
>>> > regards,
>>> > Prasad
>>> >
>>> > Sent: Wednesday, March 04, 2015 at 7:15 AM
>>> > From: "Ashesh Vashi" <ashesh(dot)vashi(at)enterprisedb(dot)com[
>>> ashesh(dot)vashi(at)enterprisedb(dot)com]>
>>> > To: Prasad <prasad(dot)s(at)mail(dot)com[prasad(dot)s@mail.com]>
>>> > Cc: pgadmin-hackers <pgadmin-hackers(at)postgresql(dot)org[
>>> pgadmin-hackers(at)postgresql(dot)org]>
>>> > Subject: Re: [pgadmin-hackers] Patch : PGPASSFILE fix
>>> >
>>> > Hi Prasad,
>>> > I see couple of issues with your patch.* Please generate the patch
>>> using 'git diff'.
>>> > I could not apply your patch straight forwardly.
>>> > I had to use the patch utility.
>>> > * Please follow the coding style of pgAdmin.
>>> > You can find it at
>>> https://wiki.postgresql.org/wiki/PgAdmin_Internals#Coding_Style.*[https://wiki.postgresql.org/wiki/PgAdmin_Internals#Coding_Style.*]
>>> Do not remove any of the existing code.
>>> > It has been kept there keeping in mind about future development
>>> extending support of the existing functionality.
>>> > You've removed couple of lines in the
>>> sysSettings::GetConfigFile(...) function, which is not good.
>>> >
>>> > In your code:* Checked only for PGPASSFILE environment variable.
>>> > * Need to check the existence of the file.
>>> > * Take required actions (if that file/parent directory does not
>>> exists).
>>> > i.e. Create parent directory
>>> >
>>> >
>>> >
>>> > --
>>> > Thanks & Regards,
>>> >
>>> > Ashesh Vashi
>>> > EnterpriseDB INDIA: Enterprise PostgreSQL Company[
>>> http://www.enterprisedb.com[http://www.enterprisedb.com]]
>>> >
>>> >
>>> http://www.linkedin.com/in/asheshvashi[http://www.linkedin.com/in/asheshvashi][http://www.linkedin.com/in/asheshvashi[http://www.linkedin.com/in/asheshvashi]]
>>> >
>>> > On Sun, Mar 1, 2015 at 11:08 PM, Prasad <prasad(dot)s(at)mail(dot)com[
>>> prasad(dot)s(at)mail(dot)com][prasad(dot)s@mail(dot)com[prasad(dot)s(at)mail(dot)com]]> wrote:
>>> > Hi,
>>> >
>>> > Find attached fix for reading PGPASSFILE environment variable for pg
>>> password file.
>>> >
>>> > regards,
>>> > Prasad
>>> >
>>> > --
>>> > Sent via pgadmin-hackers mailing list (pgadmin-hackers(at)postgresql(dot)org[
>>> pgadmin-hackers(at)postgresql(dot)org][pgadmin-hackers@postgresql.org[
>>> pgadmin-hackers(at)postgresql(dot)org]])
>>> > To make changes to your subscription:
>>> >
>>> http://www.postgresql.org/mailpref/pgadmin-hackers[http://www.postgresql.org/mailpref/pgadmin-hackers][http://www.postgresql.org/mailpref/pgadmin-hackers[http://www.postgresql.org/mailpref/pgadmin-hackers]]
>>> >
>>> >
>>> >
>>> > --
>>> > Sent via pgadmin-hackers mailing list (pgadmin-hackers(at)postgresql(dot)org)
>>> > To make changes to your subscription:
>>> > http://www.postgresql.org/mailpref/pgadmin-hackers
>>>
>>>
>>>
>>> --
>>> Dave Page
>>> Blog: http://pgsnake.blogspot.com
>>> Twitter: @pgsnake
>>>
>>> EnterpriseDB UK: http://www.enterprisedb.com
>>> The Enterprise PostgreSQL Company
>>>
>>
>>
>
>
> --
> Dave Page
> Blog: http://pgsnake.blogspot.com
> Twitter: @pgsnake
>
> EnterpriseDB UK: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>