Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtp (Exim 4.84_2) (envelope-from ) id 1bwmO4-0004kl-85 for pgadmin-hackers@arkaria.postgresql.org; Wed, 19 Oct 2016 08:38:12 +0000 Received: from localhost ([127.0.0.1] helo=postgresql.org) by malur.postgresql.org with smtp (Exim 4.84_2) (envelope-from ) id 1bwmO3-0001SP-RS for pgadmin-hackers@arkaria.postgresql.org; Wed, 19 Oct 2016 08:38:11 +0000 Received: from makus.postgresql.org ([2001:4800:1501:1::229]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.84_2) (envelope-from ) id 1bwmNo-0001DR-Un for pgadmin-hackers@postgresql.org; Wed, 19 Oct 2016 08:37:57 +0000 Received: from mail-lf0-x232.google.com ([2a00:1450:4010:c07::232]) by makus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.84_2) (envelope-from ) id 1bwmNl-0003Ap-DY for pgadmin-hackers@postgresql.org; Wed, 19 Oct 2016 08:37:55 +0000 Received: by mail-lf0-x232.google.com with SMTP id b81so14379914lfe.1 for ; Wed, 19 Oct 2016 01:37:53 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=enterprisedb-com.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=4wa5prp0LLCFDinDHIb5VZiUQrtDrYoA59bfvKJ709I=; b=lvKvEpGzrUYE+kwuNlKXh6L0N+X59QEugcQg0lWLFOsWZmoAli5wa505XA1tFXatxu kzANKiU0AsVRLRlnw/BZFQFzyAWTqSryLDXlYGzmaX/zrXrIWonENr2feY5f/HzkNKmU FFFBFJUGax+vZ4CRT9o8VBETd4eyn+0feRpaLztcWOMhDIb1LooJ018z9ImhUp0ZeUNq JTuoip5SKIQZJSs4HSQd62W+8ZwS33zViY+65TPN9KqRisWbP0FIfe8AhgLS7MfsB5iJ ZclbPRw5us3713TVF0yvSdc5ntjgi/8KpZGc27/jlnFGOGzrser/iuoQvQwU4GfLx42T XAag== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=4wa5prp0LLCFDinDHIb5VZiUQrtDrYoA59bfvKJ709I=; b=Q9rBgfc01vqhyqId6f7xVB2ouFK0+bRcQYWpCmiaBc3XP+9RKq0po6S18jZ8Yw+5B/ NsixJMX6pvDJ1uDbBcfhTNzSjO4TCHAFv0jalKXmFx9FzXFokvgUAeF206s41q4gf0Gk EkYP/t+7cEBiaj9FzKxiBZmpcQ5NY4RHL8gJAm7k4+FfrJPqWlhJcAE40peoB30izUDv R+PVarf7+VLQm7k/I9iCgXAxdpw7mR4UUJ3hPCp4pIBb6HbCwjoI3p55F7rJN2O3QOds qaK+ZGRgZmbvpA5f/f88U/GP5MzyX0zHZNIbjDIXTMX6mBefRyxOvxm4S4QH62uE6Dex tO7w== X-Gm-Message-State: AA6/9RnK3gm4I0ebuBqvXYUNUyhYcBXyurp1Os/WNCaZbWtmIiEjdGKlTMuRcivJSUpFju/FHsDV8UCL/2xscNUc X-Received: by 10.25.16.208 with SMTP id 77mr3268724lfq.167.1476866270323; Wed, 19 Oct 2016 01:37:50 -0700 (PDT) MIME-Version: 1.0 Received: by 10.25.92.219 with HTTP; Wed, 19 Oct 2016 01:37:49 -0700 (PDT) In-Reply-To: References: From: Fahar Abbas Date: Wed, 19 Oct 2016 13:37:49 +0500 Message-ID: Subject: Re: RM1849: Auto-generating security keys To: Dave Page Cc: Ashesh Vashi , pgadmin-hackers , Josh Berkus , =?UTF-8?B?RGV2cmltIEfDnE5Ew5xa?= , Magnus Hagander , Sandeep Thakkar , Hamid Quddus Akhtar Content-Type: multipart/alternative; boundary=001a114032946c7723053f33b99f X-Pg-Spam-Score: -2.6 (--) List-Archive: List-Help: List-ID: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: X-Mailing-List: pgadmin-hackers Precedence: bulk Sender: pgadmin-hackers-owner@postgresql.org --001a114032946c7723053f33b99f Content-Type: text/plain; charset=UTF-8 Sure, Will test this thoroughly after complete investigation. Kind Regards, On Wed, Oct 19, 2016 at 1:27 PM, Dave Page 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) > > Thanks. > > > On Wed, Oct 19, 2016 at 6:46 AM, Ashesh Vashi < > ashesh.vashi@enterprisedb.com> wrote: > >> Hi Dave, >> >> On Sat, Oct 15, 2016 at 8:02 AM, Dave Page wrote: >> >>> Hi >>> >>> >>> On Friday, October 14, 2016, Dave Page wrote: >>> >>>> Hi >>>> >>>> On Thursday, October 13, 2016, Ashesh Vashi < >>>> ashesh.vashi@enterprisedb.com> wrote: >>>> >>>>> Hi Dave, >>>>> >>>>> On Tue, Oct 11, 2016 at 9:10 PM, Dave Page 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.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 > -- Syed Fahar Abbas Quality Management Group EnterpriseDB Corporation Phone Office: +92-51-835-8874 Phone Direct: +92-51-8466803 Mobile: +92-333-5409707 Skype ID: syed.fahar.abbas Website: www.enterprisedb.com --001a114032946c7723053f33b99f Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable
Sure,

Will test this thoroughly aft= er complete investigation.

Kind Regards,

On Wed, Oct 19, 2016 at 1:2= 7 PM, Dave Page <dpage@pgadmin.org> wrote:
Patch applied.

Fahar, = can you please test this thoroughly in desktop and server modes, with both = fresh and upgraded installations?


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=C2=A0SECURITY_PASSWORD_SALT,=C2=A0SECURITY_KEY and=C2=A0CSRF_SESSION_KEY in the config (= in fact, they should be removed for new installations, if you have included= them in 1.0)

Thanks.


On Wed, Oct = 19, 2016 at 6:46 AM, Ashesh Vashi <ashesh.vashi@enterprisedb.c= om> wrote:
Hi Dave,

=
On Sat, Oct 15, 2016 at 8:02 AM, Dave Page <dpage@pgadmin.org> wrote:
Hi

<= br>On Friday, October 14, 2016, Dave Page <dpage@pgadmin.org> wrote:
Hi

On Thursday, October 13, 201= 6, Ashesh Vashi <ashesh.vashi@enterprisedb.com> wrote:
Hi Dave= ,

On Tu= e, Oct 11, 2016 at 9:10 PM, Dave Page <dpage@pgadmi= n.org> wrote:
Hi Ashesh,

Can you please review the attached patc= h, and apply if you're happy with it?
Overall th= e patch looked good to me.
But - I encounter an issue in 'web= ' mode, which wont happen with 'runtime'.

<= div>Steps for reproduction on existing pgAdmin 4 environment with 'web&= #39; mode.
- Apply the patch
- Start the pgAdmin4 appli= cation (stand alone application).
- Open pgAdmin home page.
=
- Log out (if already login).

And, you will s= ee an exception.

I have figure out the issue with = the patch.
We were setting the SECURITY_PASSWORD_SALT, after init= ializing the Security object.
Hence - it could not set the SECURI= TY_KEY, and SECURITY_PASSWORD_SALT properly.

Hmm.
=C2=A0

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.
=C2=A0

Now - I run into an= other 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 wa= s thinking - we can store the existing security configurations in the datab= ase during upgrade process in 'web' mode.

My concern with that is that we'll likel= y 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 da= tabase which is clearly not a good idea. Sigh... Needs more thought.=C2=A0<= /div>

OK, so I've been thin= king about this and experimenting for a couple of hours, as well as annoyin= g 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, =C2= =A0SECURITY_PASSWORD_SALT is (aside from really being a key not a salt) not= the only salting that's done.=C2=A0

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 d= ifferent 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 re= sulting hashes were different in both databases - thus there is something e= lse ensuring the hashes are unique across different installations/databases= .

So, I believe we can do as you suggest and migra= te existing values for SECURITY_PASSWORD_SALT, given that there's clear= ly some other per user and per installation/database salting going on anywa= y. New installations can have the random value for SECURITY_PASSWORD_SALT.<= /div>
We do not need to generate the random SE= CURITY_PASSWORD_SALT during upgrade mode, which was wrong added in my addon= patch.

Please find the updated patch.
<= br>
Otherwise - looks good to me.
Please commit the new= patch (if you're ok with the change).


--

Than= ks & Regards,

Ashesh Vashi
EnterpriseD= B INDIA:=C2=A0Enterprise PostgreSQL Company


=

I do= n't believe SECURITY_KEY and=C2=A0CSRF_SESSION_KEY are issues either, a= s they're used for purposes that are essentially ephemeral, and thus ca= n 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 Enterpr= ise PostgreSQL Company



--
Syed Fahar= Abbas
Quality Management Group

EnterpriseDB Cor= poration
Phone Office: +92-51-835-8874
Phone Direct: +92-51-8466803Mobile: +92-333-5409707
Skype ID: syed.fahar.abbas
Website: www.enterprisedb.com=
--001a114032946c7723053f33b99f--