Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtp (Exim 4.84_2) (envelope-from ) id 1bwmDa-0003VN-HT for pgadmin-hackers@arkaria.postgresql.org; Wed, 19 Oct 2016 08:27:22 +0000 Received: from localhost ([127.0.0.1] helo=postgresql.org) by malur.postgresql.org with smtp (Exim 4.84_2) (envelope-from ) id 1bwmDZ-0006bc-Uw for pgadmin-hackers@arkaria.postgresql.org; Wed, 19 Oct 2016 08:27:21 +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 1bwmDY-0006bV-VH for pgadmin-hackers@postgresql.org; Wed, 19 Oct 2016 08:27:21 +0000 Received: from mail-io0-x234.google.com ([2607:f8b0:4001:c06::234]) by magus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.84_2) (envelope-from ) id 1bwmDV-0006Be-Mn for pgadmin-hackers@postgresql.org; Wed, 19 Oct 2016 08:27:20 +0000 Received: by mail-io0-x234.google.com with SMTP id j37so24081257ioo.3 for ; Wed, 19 Oct 2016 01:27:17 -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=Bk30eq2tvyWBSGgwnoBv9oetJuulhgOUwgE/HL9xAoA=; b=ZTMOhve+YcLkzrCKQIYWC97PUTQcczqHJmkEHySFeiC4Z/KeUXsJWCW1meXZKRFTQV t/TGAXpfqqcDvytXjX1LBRXG5cZEV/PojOv01CTQIlbGRc0OVeXmCP85w1Ay22HYmht5 r2XEwHEQh4/DdRHDT5ZNp3NwwAD6Z3tLCOBsuxyApY8YFYjWpcTsONRAdcJHgy0bDgj5 Erzj6vE6F5hDYsg/ks667+Ur+Kw9LawOcWYCMdQUJTSFqkiOE8dFmRmzfPTPzIeCNd7V QjKeNkOHvQd+w4k8lGbGB9De/nQjqjD9UTgQrOxwAaENy5w5nH0d0G9idRLBKNI3HhaM hLMw== 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=Bk30eq2tvyWBSGgwnoBv9oetJuulhgOUwgE/HL9xAoA=; b=Kx95SUUAbX/rnsWrJxg3oMIw+rB+/8z+mNYb7t6OfjFRsgKkysEHF7y7AEdXru8vl9 O5w0u/SUTHXzG2jBoP1Ek5VoVqWPITTjUif1mTvO9LrJSri7fwp81wEG/q0vximpJoaQ oBeVzCxNwyR4I7vNcLv6XTSbkz6quDk4gpg5NVcbWW3YLtM3p+yPKUlENKcJXxeG7bap LZ3GNZto04NE78OJn4SNsHT8OY0kGWsIvVbsRD28nR19zmsx80nAAhY88q6RUK3r3K5z 4IuLiKaAXeG4Efgt8lPzC7aFbCkb7nAan4JfK9NLuwnXFxMbJbu1ut5zAqWTRiO36GPi 2PgA== X-Gm-Message-State: AA6/9RmHnXQryFiCwOrtdo5O1oQfmligiW9tFmMhlSM9WegdpdJPwjR27iMOSDqZA7OA+vh33UUPv/4iA/Kg1w== X-Received: by 10.107.12.206 with SMTP id 75mr5159477iom.208.1476865635779; Wed, 19 Oct 2016 01:27:15 -0700 (PDT) MIME-Version: 1.0 Received: by 10.64.132.227 with HTTP; Wed, 19 Oct 2016 01:27:15 -0700 (PDT) In-Reply-To: References: From: Dave Page Date: Wed, 19 Oct 2016 09:27:15 +0100 Message-ID: Subject: Re: RM1849: Auto-generating security keys To: Ashesh Vashi Cc: pgadmin-hackers , Josh Berkus , =?UTF-8?B?RGV2cmltIEfDnE5Ew5xa?= , Magnus Hagander , Syed Fahar Abbas , Sandeep Thakkar , Hamid Quddus Akhtar Content-Type: multipart/alternative; boundary=001a113f97069a1050053f339348 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 --001a113f97069a1050053f339348 Content-Type: text/plain; charset=UTF-8 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 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 --001a113f97069a1050053f339348 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable
Patch applied.

Fahar, can you please te= st this thoroughly in desktop and server modes, with both fresh and upgrade= d installations?

<= div>
Packagers: This change means that packages are no longer= forced to create a config_local.py file, and there is no longer any need t= o explicitly set=C2=A0SECURITY_PASSWORD_SA= LT,=C2=A0SECURITY_KEY and=C2=A0CSRF= _SESSION_KEY in the config (in fact, they should be removed for new install= ations, if you have included them in 1.0)

T= hanks.

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

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


On Friday, October 14, 2016, Dave Page &= lt;dpage@pgadmin.org= > wrote:
Hi
=
On Thursday, October 13, 2016, Ashesh Vashi <ashesh.vashi@enterpr= isedb.com> wrote:
Hi Dave,

=
On Tue, Oct 11, 2016 at 9:10 PM, Dave Page <dpage@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 'run= time'.

Steps for reproduction on existing pgAd= min 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 SE= CURITY_PASSWORD_SALT, after initializing the Security object.
Hen= ce - it could not set the SECURITY_KEY, and SECURITY_PASSWORD_SALT properly= .

Hmm.
= =C2=A0

I had moved the Security object initialization after fetching these conf= igurations from the database.
I have attached a addon patch for t= he same.

OK, thanks= .
=C2=A0
<= div dir=3D"ltr">
=
Now - I run into another issue.
Because - the exis= ting 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 se= curity configurations in the database during upgrade process in 'web= 9; 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.=C2=A0

OK, so I've been thinking about this and experimenting for a c= ouple 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 pro= blem as from what I can see, =C2=A0SECURITY_PASSWORD_SALT is (aside from re= ally 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 t= o generate an HMAC of the users password, which is then passed to passlib w= hich 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 d= ropped the database and created the same user accounts with the same passwo= rds again, and found that the resulting hashes were different in both datab= ases - thus there is something else ensuring the hashes are unique across d= ifferent installations/databases.

So, I believe we= can do as you suggest and migrate existing values for SECURITY_PASSWORD_SA= LT, given that there's clearly some other per user and per installation= /database salting going on anyway. New installations can have the random va= lue for SECURITY_PASSWORD_SALT.
We do no= t need to generate the random SECURITY_PASSWORD_SALT during upgrade mode, w= hich was wrong added in my addon patch.

Please fin= d the updated patch.

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


--

Thanks & R= egards,

Ashesh Vashi
EnterpriseDB INDIA:= =C2=A0Ent= erprise PostgreSQL Company



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

Adding Magnus as I'd appreciate any though= ts he may have.

Patch attached - please review (As= hesh, but others too would be appreciated)!

Thanks= .


--
Dave Page
Blog: http://pgsnake.blogs= pot.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 Compan= y
--001a113f97069a1050053f339348--