Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtp (Exim 4.84_2) (envelope-from ) id 1bwogz-0003yY-SF for pgadmin-hackers@arkaria.postgresql.org; Wed, 19 Oct 2016 11:05:54 +0000 Received: from localhost ([127.0.0.1] helo=postgresql.org) by malur.postgresql.org with smtp (Exim 4.84_2) (envelope-from ) id 1bwogz-0004zi-Eo for pgadmin-hackers@arkaria.postgresql.org; Wed, 19 Oct 2016 11:05:53 +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 1bwogy-0004x3-2o for pgadmin-hackers@postgresql.org; Wed, 19 Oct 2016 11:05:52 +0000 Received: from mail-qk0-x231.google.com ([2607:f8b0:400d:c09::231]) by makus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.84_2) (envelope-from ) id 1bwogu-0005rA-KO for pgadmin-hackers@postgresql.org; Wed, 19 Oct 2016 11:05:50 +0000 Received: by mail-qk0-x231.google.com with SMTP id f128so27191581qkb.1 for ; Wed, 19 Oct 2016 04:05:48 -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=YTHSOmJZQmzHAeRw2qYL+otZBNEZQwhl/ot3dZv9LY0=; b=S2pLVcRpLO8KSGpnqb8I9AhBZCgJmxhjIG0R8P8p2qkytW+sgKwB13ZU/w0BIdshbx JqHzbHvQmmHNOR1hpG7wL8Qt46s4txTfePe0mNj8W14w2OokSP+4hBGIHBgB+fd3juYo dhaGKbOniQUMLk1ywNoonKFcA+hwnI/5eePtcNfUfqzAYIsjC7sduAmVknHr6uPSvNCl KUOd1PrYhO+uuEO7XUO+5lYo++U4yl8esWhMWhYPE4/JU8p+Oz4gLAAFPmRFmE8v7SWa Tm2nlC3HrSZGsNhPgXgDksCEXxjbgYrex2LdoqGJ+sVcZWHzyiZKtRG089KbryYDNxgG n+5Q== 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=YTHSOmJZQmzHAeRw2qYL+otZBNEZQwhl/ot3dZv9LY0=; b=Gr35Q5+ihvvMO6UMVn9cGXd9ZQs4TmRkdavkzCsYZsb39FksmNLyd4wwdNA5HwogSY fjrgsHMckpA5nNjWe2bOdODXIf3I5jsLQLUU5NZo4d4Sm9Kn0D2xUUyyueqTDmSPQp6l M2+ALwBUpyw9vlOLdp8YPqxLjvzJGOoWhS7gMXE8pTL3CFDrczQcjBfnTk1KKTugXvYH 3lQqNEUBcYQ1HGedc7bOG5r2OVKQrqOmsNzTaOfoMlBq5wWhJSvIxOCclJwdfiJjC4YR AV4VTF1b2V6avk/68Mj5Sz6OaGIBoqhT31TkaogSp8lrVIbb+wAI8MlDtti5MxNghW30 NOTQ== X-Gm-Message-State: AA6/9RnVh4vYLAGWQ16hv08HXFcbrCTBOfdRNGx+nHOt4b7VOr/wadQMZxky46ib+ZeSN5W5cmxfH5lQnfaaHcjs X-Received: by 10.55.3.7 with SMTP id 7mr5194956qkd.291.1476875147620; Wed, 19 Oct 2016 04:05:47 -0700 (PDT) MIME-Version: 1.0 Received: by 10.140.88.227 with HTTP; Wed, 19 Oct 2016 04:05:46 -0700 (PDT) In-Reply-To: References: From: Sandeep Thakkar Date: Wed, 19 Oct 2016 16:35:46 +0530 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 , Syed Fahar Abbas , Hamid Quddus Akhtar Content-Type: multipart/alternative; boundary=001a114c9b6e8d8276053f35ca41 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 --001a114c9b6e8d8276053f35ca41 Content-Type: text/plain; charset=UTF-8 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 --001a114c9b6e8d8276053f35ca41 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable
Hi Dave,

On Wed, Oct 19, 2016 at 1:57 PM, Dave Page &l= t;dpage@pgadmin.org<= /a>> wrote:
Pa= tch 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 lo= nger forced to create a config_local.py file, and there is no longer any ne= ed to explicitly set=C2=A0SECURITY_PASSWOR= D_SALT,=C2=A0SECURITY_KEY and= =C2=A0CSRF_SESSION_KEY in the config (in fact, they should be removed for n= ew installations, if you have included them in 1.0)

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

On We= d, Oct 19, 2016 at 6:46 AM, Ashesh Vashi <ashesh.vashi@enterpr= isedb.com> wrote:
Hi Dave,

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


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

On Thursday, October= 13, 2016, Ashesh Vashi <ashesh.vashi@enterprisedb.com> w= rote:
Hi Dave,

On Tue, Oct 11, 2016 at 9:10 PM, Dave Page <dpag= e@pgadmin.org> wrote:
Hi Ashesh,

Can you please review the attac= hed patch, and apply if you're happy with it?
Ov= erall 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 &= #39;web' mode.
- Apply the patch
- Start the pgAdmi= n4 application (stand alone application).
- Open pgAdmin home pag= e.
- Log out (if already login).

And, yo= u will see an exception.

I have figure out the iss= ue with the patch.
We were setting the SECURITY_PASSWORD_SALT, af= ter initializing the Security object.
Hence - it could not set th= e SECURITY_KEY, and SECURITY_PASSWORD_SALT properly.

Hmm.
=C2=A0

I had moved the Securit= y object initialization after fetching these configurations from the databa= se.
I have attached a addon patch for the same.
=

OK, thanks.
=C2=A0

Now - I run= into another issue.
Because - the existing password was hashed u= sing the old SECURITY_PASSWORD_SALT, I am no more able to login to pgAdmin = 4.

I think - we need to think about different stra= tegy for upgrading the configuration file in the 'web' mode.
<= div>I was thinking - we can store the existing security configurations in t= he 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 thos= e users, perpetuating the problem.

I guess what we= need to do is re-encrypt the password during the upgrade - however, that m= akes 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 b= een 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 direct= ion, and it looks like this isn't a major problem as from what I can se= e, =C2=A0SECURITY_PASSWORD_SALT is (aside from really being a key not a sal= t) 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 different hashes in the database, so clearly there is also per-user sa= lting happening. I also created two users, then dropped the database and cr= eated the same user accounts with the same passwords again, and found that = the resulting hashes were different in both databases - thus there is somet= hing else ensuring the hashes are unique across different installations/dat= abases.

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 ran= dom 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 t= he new patch (if you're ok with the change).


--

= Thanks & Regards,

Ashesh Vashi
E= nterpriseDB INDIA:=C2=A0Enterprise PostgreSQL Company


<= /span>


=
I don't believe SECURITY_KEY and=C2=A0CSRF_SESSION_KEY are issues = either, as they're used for purposes that are essentially ephemeral, an= d thus can be changed during an upgrade.

Adding Ma= gnus as I'd appreciate any thoughts he may have.

Patch attached - please review (Ashesh, but others too would be apprecia= ted)!

Thanks.


--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter= : @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise Postgre= SQL Company

=



--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @p= gsnake

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


--
Sandeep Thakkar



--001a114c9b6e8d8276053f35ca41--