Thanks, applied. On Wed, Oct 19, 2016 at 1:39 PM, Sandeep Thakkar < sandeep.thak...@enterprisedb.com> wrote:
> Here is the patch where we remove the config_local.py being created during > packaging. The mac build script missed creating config_distro.py earlier > and it has been take care of now. Please review the attached patch. > > I'll also make the changes in the EDB packaging scripts where we bundle > pgAdmin in PG server and EPAS Meta. > > On Wed, Oct 19, 2016 at 4:35 PM, Sandeep Thakkar < > sandeep.thak...@enterprisedb.com> wrote: > >> Hi Dave, >> >> On Wed, Oct 19, 2016 at 1:57 PM, Dave Page <dp...@pgadmin.org> wrote: >> >>> Patch applied. >>> >>> Fahar, can you please test this thoroughly in desktop and server modes, >>> with both fresh and upgraded installations? >>> >>> https://redmine.postgresql.org/issues/1849 >>> >>> Packagers: This change means that packages are no longer forced to >>> create a config_local.py file, and there is no longer any need to >>> explicitly set SECURITY_PASSWORD_SALT, SECURITY_KEY >>> and CSRF_SESSION_KEY in the config (in fact, they should be removed for new >>> installations, if you have included them in 1.0) >>> >>> OK. Will remove config_local.py from the packaging. We do not set the >> mentioned directives in the config. >> >> >>> Thanks. >>> >>> On Wed, Oct 19, 2016 at 6:46 AM, Ashesh Vashi < >>> ashesh.va...@enterprisedb.com> wrote: >>> >>>> Hi Dave, >>>> >>>> On Sat, Oct 15, 2016 at 8:02 AM, Dave Page <dp...@pgadmin.org> wrote: >>>> >>>>> Hi >>>>> >>>>> >>>>> On Friday, October 14, 2016, Dave Page <dp...@pgadmin.org> wrote: >>>>> >>>>>> Hi >>>>>> >>>>>> On Thursday, October 13, 2016, Ashesh Vashi < >>>>>> ashesh.va...@enterprisedb.com> wrote: >>>>>> >>>>>>> Hi Dave, >>>>>>> >>>>>>> On Tue, Oct 11, 2016 at 9:10 PM, Dave Page <dp...@pgadmin.org> >>>>>>> wrote: >>>>>>> >>>>>>>> Hi Ashesh, >>>>>>>> >>>>>>>> Can you please review the attached patch, and apply if you're happy >>>>>>>> with it? >>>>>>>> >>>>>>> Overall the patch looked good to me. >>>>>>> But - I encounter an issue in 'web' mode, which wont happen with >>>>>>> 'runtime'. >>>>>>> >>>>>>> Steps for reproduction on existing pgAdmin 4 environment with 'web' >>>>>>> mode. >>>>>>> - Apply the patch >>>>>>> - Start the pgAdmin4 application (stand alone application). >>>>>>> - Open pgAdmin home page. >>>>>>> - Log out (if already login). >>>>>>> >>>>>>> And, you will see an exception. >>>>>>> >>>>>>> I have figure out the issue with the patch. >>>>>>> We were setting the SECURITY_PASSWORD_SALT, after initializing the >>>>>>> Security object. >>>>>>> Hence - it could not set the SECURITY_KEY, and >>>>>>> SECURITY_PASSWORD_SALT properly. >>>>>>> >>>>>> >>>>>> Hmm. >>>>>> >>>>>> >>>>>>> >>>>>>> I had moved the Security object initialization after fetching these >>>>>>> configurations from the database. >>>>>>> I have attached a addon patch for the same. >>>>>>> >>>>>> >>>>>> OK, thanks. >>>>>> >>>>>> >>>>>>> >>>>>>> Now - I run into another issue. >>>>>>> Because - the existing password was hashed using the old >>>>>>> SECURITY_PASSWORD_SALT, I am no more able to login to pgAdmin 4. >>>>>>> >>>>>>> I think - we need to think about different strategy for upgrading >>>>>>> the configuration file in the 'web' mode. >>>>>>> I was thinking - we can store the existing security configurations >>>>>>> in the database during upgrade process in 'web' mode. >>>>>>> >>>>>> >>>>>> My concern with that is that we'll likely be storing the default >>>>>> config values in many cases, thus for those users, perpetuating the >>>>>> problem. >>>>>> >>>>>> I guess what we need to do is re-encrypt the password during the >>>>>> upgrade - however, that makes me think; we then have both the key and the >>>>>> encrypted passwords in the same database which is clearly not a good >>>>>> idea. >>>>>> Sigh... Needs more thought. >>>>>> >>>>> >>>>> OK, so I've been thinking about this and experimenting for a couple of >>>>> hours, as well as annoying the crap out of Magnus by thinking out loud in >>>>> his general direction, and it looks like this isn't a major problem as >>>>> from >>>>> what I can see, SECURITY_PASSWORD_SALT is (aside from really being a key >>>>> not a salt) not the only salting that's done. >>>>> >>>>> It looks like it's used system-wide as the key to generate an HMAC of >>>>> the users password, which is then passed to passlib which salts and hashes >>>>> it. I did some testing, and found that two users with the same password >>>>> end >>>>> up with different hashes in the database, so clearly there is also >>>>> per-user >>>>> salting happening. I also created two users, then dropped the database and >>>>> created the same user accounts with the same passwords again, and found >>>>> that the resulting hashes were different in both databases - thus there is >>>>> something else ensuring the hashes are unique across different >>>>> installations/databases. >>>>> >>>>> So, I believe we can do as you suggest and migrate existing values for >>>>> SECURITY_PASSWORD_SALT, given that there's clearly some other per user and >>>>> per installation/database salting going on anyway. New installations can >>>>> have the random value for SECURITY_PASSWORD_SALT. >>>>> >>>> We do not need to generate the random SECURITY_PASSWORD_SALT during >>>> upgrade mode, which was wrong added in my addon patch. >>>> >>>> Please find the updated patch. >>>> >>>> Otherwise - looks good to me. >>>> Please commit the new patch (if you're ok with the change). >>>> >>>> >>>> -- >>>> >>>> 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> >>>> >>>>> >>>>> I don't believe SECURITY_KEY and CSRF_SESSION_KEY are issues either, >>>>> as they're used for purposes that are essentially ephemeral, and thus can >>>>> be changed during an upgrade. >>>>> >>>>> Adding Magnus as I'd appreciate any thoughts he may have. >>>>> >>>>> Patch attached - please review (Ashesh, but others too would be >>>>> appreciated)! >>>>> >>>>> Thanks. >>>>> >>>>> >>>>> -- >>>>> 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 >>> >> >> >> >> -- >> Sandeep Thakkar >> >> >> >> > > > -- > Sandeep Thakkar > > > > -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company
