Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtp (Exim 4.84_2) (envelope-from ) id 1bwqds-0001B8-GT for pgadmin-hackers@arkaria.postgresql.org; Wed, 19 Oct 2016 13:10:48 +0000 Received: from localhost ([127.0.0.1] helo=postgresql.org) by malur.postgresql.org with smtp (Exim 4.84_2) (envelope-from ) id 1bwqds-0006Ud-3D for pgadmin-hackers@arkaria.postgresql.org; Wed, 19 Oct 2016 13:10:48 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.84_2) (envelope-from ) id 1bwqdq-0006Sf-QE for pgadmin-hackers@postgresql.org; Wed, 19 Oct 2016 13:10:46 +0000 Received: from mail-it0-x229.google.com ([2607:f8b0:4001:c0b::229]) by magus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.84_2) (envelope-from ) id 1bwqdm-0002y8-5m for pgadmin-hackers@postgresql.org; Wed, 19 Oct 2016 13:10:46 +0000 Received: by mail-it0-x229.google.com with SMTP id 4so111589967itv.0 for ; Wed, 19 Oct 2016 06:10:41 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pgadmin-org.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=u1PDgxccn1refc6021kUDANAXHqRI1Jz79jQ2KwMzA8=; b=e4SlrzXUrMAZr6bI9G+xbkWUsdsIVkBHUrK+fc7dsqJY6J8BTsSWg5185tW/LZrBjh 5oQH/pLrF9gXuZIc2UM0zUWU0TPWQiEZUwlYVkaLUFDCvIHEMmTpv9UWHD90FATBEI9r sH/6b7aYIt0EcI1GyoGJz6X1GFsV6U7J8mmPWQr5u9hsqmVEhICA5O0FNNm1YIGMjEmP m0lLej4LG/fLwucFEelIOLwDm2yMhyq0JWtRGCq3BJEom5I8/PIxXTm3oLKGJafzz2lO crp5JP1CyVUkHixPqNWA6J2PVxeSvZY2LBz0coLnff/upj73V5QrBQQ9pWtqcpQ4DZJ4 Is/Q== 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=u1PDgxccn1refc6021kUDANAXHqRI1Jz79jQ2KwMzA8=; b=bDbCJ05U1zJolR//MHdVAN5qA9+mrzdpdbJwcGigyOGHjNWehIC/wmwaR37914PwWD L/vSHgqpcWxkLKwEqw9mAqUkGWp5nd1V9pUf7nTNObev89g0LqM1zmwewotP4fzi4JHc wB+Xn4FUA8IpUq+KQw0yAQXCD7jsalDSlUQgHb1dT/XVU8fGeuVRSs8ona1KrM+HH4hL itmjrMJabX6tF2acj+4KaeSd0g1Np6SMmMULEfxSKC/KEf9bVROZLwKupNZWvZay8+Th r+gwRzYwgdVn8ORnNRkdOrWbaq0kiL79a9ZWLOLa8VRIgqj+71aYV1v4qlUv2J+mbsi+ jVzA== X-Gm-Message-State: AA6/9RlqzIGvRnW1PhiiZk8ANk/xArA2M9LoSZha+xD8mu8LLYrTDToSonrul8Dv8inqYwsZ77tW8Mm0/FD/fA== X-Received: by 10.36.249.131 with SMTP id l125mr2902062ith.113.1476882640118; Wed, 19 Oct 2016 06:10:40 -0700 (PDT) MIME-Version: 1.0 Received: by 10.64.82.130 with HTTP; Wed, 19 Oct 2016 06:10:38 -0700 (PDT) In-Reply-To: References: From: Dave Page Date: Wed, 19 Oct 2016 14:10:38 +0100 Message-ID: Subject: Re: RM1849: Auto-generating security keys To: Sandeep Thakkar Cc: Ashesh Vashi , pgadmin-hackers , Josh Berkus , =?UTF-8?B?RGV2cmltIEfDnE5Ew5xa?= , Magnus Hagander , Syed Fahar Abbas , Hamid Quddus Akhtar Content-Type: multipart/alternative; boundary=94eb2c04791223b069053f37894d 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 --94eb2c04791223b069053f37894d Content-Type: text/plain; charset=UTF-8 Thanks, applied. On Wed, Oct 19, 2016 at 1:39 PM, Sandeep Thakkar < sandeep.thakkar@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.thakkar@enterprisedb.com> wrote: > >> Hi Dave, >> >> On Wed, Oct 19, 2016 at 1:57 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) >>> >>> 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.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 >>> >> >> >> >> -- >> Sandeep Thakkar >> >> >> >> > > > -- > Sandeep Thakkar > > > > -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company --94eb2c04791223b069053f37894d Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable
Thanks, applied.

On Wed, Oct 19, 2016 at 1:39 PM, Sandeep Thakkar <sandeep.thakkar@enterprisedb.com> wrote:
Here is the patch where we remo= ve 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 th= e 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 T= hakkar <sandeep.thakkar@enterprisedb.com> wrote:
Hi Dave,

On Wed, Oct 19= , 2016 at 1:57 PM, Dave Page <dpage@pgadmin.org> wrote:
<= blockquote class=3D"gmail_quote" style=3D"margin:0 0 0 .8ex;border-left:1px= #ccc solid;padding-left:1ex">
Patch applied.

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


Pac= kagers: This change means that packages are no longer forced to create a co= nfig_local.py file, and there is no longer any need to explicitly set=C2=A0= SECURITY_PASSWORD_SALT,=C2=A0SECURITY_KEY and=C2=A0CSRF_SESSION_KEY in t= he config (in fact, they should be removed for new installations, if you ha= ve included them in 1.0)

OK. Will remove config_local.py from the packaging. We do not set t= he mentioned directives in the config.
=C2=A0=C2=A0
Thanks.

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

<= /div>
On Sat, Oct 15, 2016 at 8:02 AM, Dave Page <dpage@pgadmin.org><= /span> wrote:
Hi
<= div class=3D"m_2566282580742540410m_-3735845490175527589m_-2560848569431394= 348gmail-m_1577765211293727506gmail-h5">

On Friday, October 14, 2016= , Dave Page <dpag= e@pgadmin.org> 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, Da= ve Page <dpage@pgadmin.org> wrote:
Hi Ashesh,

Can you please review the attached patch, and apply if you're hap= py 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 t= he patch
- Start the pgAdmin4 application (stand alone applicatio= n).
- Open pgAdmin home page.
- Log out (if already log= in).

And, you will see an exception.
I have figure out the issue with the patch.
We were s= etting the SECURITY_PASSWORD_SALT, after initializing the Security object.<= /div>
Hence - it could not set the SECURITY_KEY, and SECURITY_PASSWORD_= SALT properly.

Hmm.=
=C2=A0
<= br>
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 another issue.
Becaus= e - 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 conf= ig values in many cases, thus for those users, perpetuating the problem.

I guess what we need to do is re-encrypt the passwor= d 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.=C2=A0

OK, so I've been thinking about this and experimen= ting for a couple of hours, as well as annoying the crap out of Magnus by t= hinking 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 (a= side from really being a key not a salt) not the only salting that's do= ne.=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 tw= o 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 u= sers, then dropped the database and created the same user accounts with the= same passwords again, and found that the resulting hashes were different i= n both databases - thus there is something else ensuring the hashes are uni= que 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 t= he random value for SECURITY_PASSWORD_SALT.
<= div>We do not need to generate the random SECURITY_PASSWORD_SALT during upg= rade 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 t= he change).


--

Thanks & Regards,<= /span>

Ashesh Vashi
EnterpriseDB INDIA:=C2=A0Enterprise Po= stgreSQL Company


=

I don't believe SECURITY_KEY and=C2=A0CSRF_SESSION_= KEY are issues either, as they're used for purposes that are essentiall= y 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 wo= uld be appreciated)!

Thanks.

<= span class=3D"m_2566282580742540410m_-3735845490175527589HOEnZb">
--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgs= nake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Co= mpany




=
--
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.bl= ogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com<= br>The Enterprise PostgreSQL Company
--94eb2c04791223b069053f37894d--