Re: [HACKERS] [PATCH] pgpassfile connection option

Fabien COELHO writes: > I've switch in the CF to "ready for committer", and we'll see what the > next level thinks about it:-) Pushed with a few adjustments: * It seemed to me that the appropriate parameter name is "passfile" not "pgpassfile". In all but a couple of

Re: [HACKERS] [PATCH] pgpassfile connection option

On Tue, Nov 29, 2016 at 2:53 AM, Fabien COELHO wrote: > > Hello Julian, > > I've adressed those spacing errors. >> > > Ok. > > You are right, if pgpassfile_used is true, it SHOULD be defined, I just >> like to be careful whenever I'm working with strings. But I guess in this

Re: [HACKERS] [PATCH] pgpassfile connection option

Hello Julian, I've adressed those spacing errors. Ok. You are right, if pgpassfile_used is true, it SHOULD be defined, I just like to be careful whenever I'm working with strings. But I guess in this scenario I can trust the caller and omit those checks. Good. Patch looks ok, applies,

Re: [HACKERS] [PATCH] pgpassfile connection option

Fabien Coelho wrote: A few detailed comments: Beware of spacing: . "if(" -> "if (" (2 instances) . " =='\0')" -> " == '\0')" (at least one instance) Elsewhere: + if (conn->pgpassfile_used && conn->password_needed && conn->result && + conn->pgpassfile && conn->pgpassfile[0]!='\0' &&

Re: [HACKERS] [PATCH] pgpassfile connection option

> Independently of your patch, while testing I concluded that the multi-host feature and documentation should be improved: > - If a connection fails, it does not say for which host/port. Thanks I will re-test same. > In fact they are tried in turn if the network connection fails, but not > if

Re: [HACKERS] [PATCH] pgpassfile connection option

Hello Julian, I've updated my patch to work with the changes introduced to libpq by allowing multiple hosts. Ok. Patch applies cleanly, compiles & checks (although yet again the feature is not tested). Feature tested and works for me, although I'm not sure how the multi-host warning

Re: [HACKERS] [PATCH] pgpassfile connection option

All, * Robert Haas (robertmh...@gmail.com) wrote: > You could do something like that, I guess, but I think it might be a > good idea to wait and see if anyone else has opinions on (1) the > desirability of the basic feature, (2) the severity of the security > hazard it creates, and (3) your

Re: [HACKERS] [PATCH] pgpassfile connection option

I've updated my patch to work with the changes introduced to libpq by allowing multiple hosts. On Fabien's recommendations, I've kept the variable dot_pgpassfile_used, however I renamed that to pgpassfile_used, to keep a consistent naming scheme. I'm still not sure about the amount of error

Re: [HACKERS] [PATCH] pgpassfile connection option

Am 01.11.2016 um 15:14 schrieb Fabien COELHO: Thus, when returning with an error, if conn->pgpassfile was set and a password was necessary, we must have tried that pgpassfile, so i got rid of the field "dot_pgpass_used" No, you should not have done that, because it changes a feature which

Re: [HACKERS] [PATCH] pgpassfile connection option

Hello Julian, Alright, here goes another one: Patch v3 applies, make check ok, feature tested on Linux, one small issue found, see below. 1. Cleaned up the clutter with getPgPassFilename - the function is now named fillDefaultPGPassFile() and only does exactly that. Ok. 2. Since a

Re: [HACKERS] [PATCH] pgpassfile connection option

Welp, I was in head over heels, sorry for my messy email... *2. No more "dot_pgpass_used" - we fill the conn->pgpassfile field with any options that have been provided (connection parameter, environment variable, "default" ~/.pgpass) and in case there has been an error with the

Re: [HACKERS] [PATCH] pgpassfile connection option

Alright, here goes another one: 1. Cleaned up the clutter with getPgPassFilename - the function is now named fillDefaultPGPassFile() and only does exactly that. 2. Since a connection option "pgpassfile" or environment variable "PGPASSFILE" are picked up in conninfo_add_defaults() or in case a

Re: [HACKERS] [PATCH] pgpassfile connection option

I would suggest that the struct gets the value (from option, environment or default) and is always used elsewhere. The getPgPassFilename function should disappear and PasswordFromFile should be simplified significantly. I agree, however that's easier said than done because the "default" is

Re: [HACKERS] [PATCH] pgpassfile connection option

Thanks for your quick response, Fabien I think that PGPASSFILE is somehow checked twice yup, that was unintended redundancy... I would suggest that the struct gets the value (from option, environment or default) and is always used elsewhere. The getPgPassFilename function should disappear and

Re: [HACKERS] [PATCH] pgpassfile connection option

Hello Julian, The documentation needs to be updated. I've written a couple lines now. I aligned the definition of the connection option and the environment variable with that of other (conn.opt) pairs and added mention of the different options to the doc of the "Password File". Ok. [...]

Re: [HACKERS] [PATCH] pgpassfile connection option

On 10/16/2016 12:09 PM, Fabien COELHO wrote: Patch applies cleanly, make check ok... however AFAICS it only means that it compiles but it is not tested in anyway... This is is annoying. Well I'm not sure whether other options are tested either, but they should. Thanks for taking the time to

Re: [HACKERS] [PATCH] pgpassfile connection option

On Tue, Oct 11, 2016 at 5:06 PM, Oskari Saarenmaa wrote: > $ PASSWORD=xyz psql 'password=$PASSWORD dbname=foo' > > This does have the hazard of making it very easy to accidentally use double > quotes instead of single quotes and have the shell expand the variable > making it

Re: [HACKERS] [PATCH] pgpassfile connection option

My 0.02€: Patch applies cleanly, make check ok... however AFAICS it only means that it compiles but it is not tested in anyway... This is is annoying. Well I'm not sure whether other options are tested either, but they should. ISTM that this feature is already available through the

Re: [HACKERS] [PATCH] pgpassfile connection option

05.10.2016, 20:06, Robert Haas kirjoitti: You could do something like that, I guess, but I think it might be a good idea to wait and see if anyone else has opinions on (1) the desirability of the basic feature, (2) the severity of the security hazard it creates, and (3) your proposed remediation

Re: [HACKERS] [PATCH] pgpassfile connection option

On Tue, Oct 4, 2016 at 7:42 AM, Julian Markwort wrote: > On 09/26/2016 07:51 PM, Robert Haas wrote: >> However, they don't have >> to accept the possibility that arbitrary local files readable by the >> user ID will be used for authentication and/or disclosed;

Re: [HACKERS] [PATCH] pgpassfile connection option

On 09/26/2016 07:51 PM, Robert Haas wrote: However, they don't have to accept the possibility that arbitrary local files readable by the user ID will be used for authentication and/or disclosed; this patch would force them to accept that risk. I do agree with you, however we might have to take a

Re: [HACKERS] [PATCH] pgpassfile connection option

On Thu, Sep 22, 2016 at 11:34 AM, Julian Markwort wrote: > I haven't really thought about this as I had been asked to make this work as > an additional option to the connection parameters... > Now that I've looked at it - there is really only the benefit of saving

Re: [HACKERS] [PATCH] pgpassfile connection option

I haven't really thought about this as I had been asked to make this work as an additional option to the connection parameters... Now that I've looked at it - there is really only the benefit of saving the step of setting the PGPASSFILE environment variable. However, there might be cases in

Re: [HACKERS] [PATCH] pgpassfile connection option

On 09/22/2016 10:44 AM, Julian Markwort wrote: Hello psql-hackers! We thought it would be advantageous to be able to specify a 'custom' pgpassfile within the connection string along the lines of the existing parameters sslkey and sslcert. Which is exactly what this very compact patch

[HACKERS] [PATCH] pgpassfile connection option

Hello psql-hackers! We thought it would be advantageous to be able to specify a 'custom' pgpassfile within the connection string along the lines of the existing parameters sslkey and sslcert. Which is exactly what this very compact patch does. The patch is minimally invasive - when no