Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtp (Exim 4.84_2) (envelope-from ) id 1bwqA1-00060F-4h for pgadmin-hackers@arkaria.postgresql.org; Wed, 19 Oct 2016 12:39:57 +0000 Received: from localhost ([127.0.0.1] helo=postgresql.org) by malur.postgresql.org with smtp (Exim 4.84_2) (envelope-from ) id 1bwqA0-0005C3-Iv for pgadmin-hackers@arkaria.postgresql.org; Wed, 19 Oct 2016 12:39:56 +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 1bwq9l-0004fF-IS for pgadmin-hackers@postgresql.org; Wed, 19 Oct 2016 12:39:41 +0000 Received: from mail-qk0-x22f.google.com ([2607:f8b0:400d:c09::22f]) by magus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.84_2) (envelope-from ) id 1bwq9e-0002Jo-Lc for pgadmin-hackers@postgresql.org; Wed, 19 Oct 2016 12:39:40 +0000 Received: by mail-qk0-x22f.google.com with SMTP id f128so31912102qkb.1 for ; Wed, 19 Oct 2016 05:39:34 -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=/Vl0jgAFt2bofeVKIrqacHZm1C8eUOlVWueU+GNkbm0=; b=I5iRJfgPH3/MI5gNGVHA7fjonXc/9assx5DrgALRgN5NecTfX8/lcU22ZC57Pin+U6 akFFMxI6Sz4hD/cOz/58xFH66iUSOVwPb2QUKv4b+d1OEP7C8V6HJWi7nXjnQzJSvEwk vx5oqSWhNqywQcgu/DKMjJXvcEhpEoTMQ5xUw6Lkhtcbqh2ZZ6L/v9vXJn4sTl/hmjBp HB4NJ5jY7C5buYaqaVnAryIq1b76cFOSy+WU2DA/CJJS7+FIMHLJ+cvOPtQgRTBK9FlT GRKJoyEgASPgbPqv107GNVXl7pVjGnZDIGXxm/+S4hHqMyn87JKkv+TF+z54qm7/RY/Q C3CQ== 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=/Vl0jgAFt2bofeVKIrqacHZm1C8eUOlVWueU+GNkbm0=; b=AGtc+4E5YA3rJ/giqu8K6hsAd+1+KX64VlM1jbDmiBurzQTctImF+D6aRSv2RJDW9W gnhx6rCPQP7nmdFzAeYsv69o/J6t86IaDd5JdvPBTfdLbPbYni8+WHaQdZzwkLciKX7i MYS5raZxLxX9yyg1IuSuGZcbvNTSRPn8Dj6AeMlE3guRFMZESTzFdS1Tg/XImeMJE81K XA4nool70aLr01zdifoyIicBNoXfHsuBcEwf/Q2vIa+Jgylk53UbwLv2HIUHuMGDdzyW 0R0JsoxdfJfR7beH2iiTYvuUtQn6XUo3baya2ZGfu431zIp1LXbE1b8OmiabYmAh/8Nt SHAQ== X-Gm-Message-State: AA6/9Rmzbmm5s3wCTHwu33vhRqgxyubyI5mcQygjztouXEK3l7CejV1AiBsuwc31btWGdK4Bhoejl/g1YjG2Pnzp X-Received: by 10.55.139.134 with SMTP id n128mr5184468qkd.263.1476880772206; Wed, 19 Oct 2016 05:39:32 -0700 (PDT) MIME-Version: 1.0 Received: by 10.140.88.227 with HTTP; Wed, 19 Oct 2016 05:39:31 -0700 (PDT) In-Reply-To: References: From: Sandeep Thakkar Date: Wed, 19 Oct 2016 18:09:31 +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/mixed; boundary=001a114f8b7ecdcfd6053f3719aa 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 --001a114f8b7ecdcfd6053f3719aa Content-Type: multipart/alternative; boundary=001a114f8b7ecdcfd0053f3719a8 --001a114f8b7ecdcfd0053f3719a8 Content-Type: text/plain; charset=UTF-8 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 --001a114f8b7ecdcfd0053f3719a8 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable
Here is the patch where we remove the config_local.py bein= g created during packaging. The mac build script missed creating config_dis= tro.py earlier and it has been take care of now. Please review the attached= patch.

I'll also make the changes in the EDB packag= ing scripts where we bundle pgAdmin in PG server and EPAS Meta.
=

On Wed, Oct 19, 2= 016 at 4:35 PM, Sandeep Thakkar <sandeep.thakkar@enterprise= db.com> wrote:
Hi Dave,

= On Wed, Oct 19, 2016 at 1:57 PM, Dave Page <dpage@pgadmin= .org> wrote:
=
=C2=A0=C2=A0
Thanks.
On Wed, Oct 19, 2016 at 6:46 AM, Ashesh Vashi = <ashesh.vashi@enterprisedb.com> wrote:
<= blockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-l= eft:1px solid rgb(204,204,204);padding-left:1ex">
Hi Dave,<= div>
On Sat, O= ct 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, Octo= ber 13, 2016, Ashesh Vashi <ashesh.vashi@enterprisedb.com>= ; wrote:
Hi Dave,

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

Can you please review the at= tached 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 wit= h 'web' mode.
- Apply the patch
- Start the pgA= dmin4 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.
=C2=A0

I had moved the Secu= rity object initialization after fetching these configurations from the dat= abase.
I have attached a addon patch for the same.

OK, thanks.
=C2=A0

Now - I = run into another issue.
Because - the existing password was hashe= d using the old SECURITY_PASSWORD_SALT, I am no more able to login to pgAdm= in 4.

I think - we need to think about different s= trategy for upgrading the configuration file in the 'web' mode.
I was thinking - we can store the existing security configurations i= n the database during upgrade process in 'web' mode.

My concern with that is that we&#= 39;ll likely be storing the default config values in many cases, thus for t= hose users, perpetuating the problem.

I guess what= we need to do is re-encrypt the password during the upgrade - however, tha= t 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 thou= ght.=C2=A0

OK, so I'v= e 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 dir= ection, 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 t= he users password, which is then passed to passlib which salts and hashes i= t. 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 th= at the resulting hashes were different in both databases - thus there is so= mething 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= 9;s clearly some other per user and per installation/database salting going= on anyway. New installations can have the random value for SECURITY_PASSWO= RD_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 commi= t the new patch (if you're ok with the change).


--

Thanks & Regards,

Ashesh Vashi
EnterpriseDB INDIA:=C2=A0Enterprise Post= greSQL Company



I= don't believe SECURITY_KEY and=C2=A0CSRF_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 a= s I'd appreciate any thoughts he may have.

Pat= ch attached - please review (Ashesh, but others too would be appreciated)!<= /div>

Thanks.


--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake
<= br>EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
=




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

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


--
Sandeep Thakkar


--
Sandeep Thakkar



--001a114f8b7ecdcfd0053f3719a8-- --001a114f8b7ecdcfd6053f3719aa Content-Type: application/octet-stream; name="donot-create-local.patch" Content-Disposition: attachment; filename="donot-create-local.patch" Content-Transfer-Encoding: base64 X-Attachment-Id: f_iugwm7ib0 ZGlmZiAtLWdpdCBhL01ha2UuYmF0IGIvTWFrZS5iYXQKaW5kZXggOWFiNzc3 My4uYmYxNzA3YyAxMDA2NDQKLS0tIGEvTWFrZS5iYXQKKysrIGIvTWFrZS5i YXQKQEAgLTI0Nyw5ICsyNDcsNiBAQCBHT1RPOkVPRgogICAgIEVDSE8gSEVM UF9QQVRIID0gJy4uLy4uLy4uL2RvY3MvZW5fVVMvaHRtbC8nID4+ICIlUEdC VUlMRFBBVEglXHdlYlxjb25maWdfZGlzdHJvLnB5IgogICAgIEVDSE8gTUlO SUZZX0hUTUwgPSBGYWxzZSA+PiAiJVBHQlVJTERQQVRIJVx3ZWJcY29uZmln X2Rpc3Ryby5weSIKIAotICAgIEVDSE8gQ3JlYXRpbmcgY29uZmlnX2xvY2Fs LnB5Ci0gICAgRUNITyAjIEFkZCBhbnkgY29uZmlndXJhdGlvbiBjaGFuZ2Vz IHRvIHRoaXMgZmlsZS4gPiAiJVBHQlVJTERQQVRIJVx3ZWJcY29uZmlnX2xv Y2FsLnB5IgotCiAgICAgRUNITyBCdWlsZGluZyBkb2NzLi4uCiAgICAgTUtE SVIgIiVQR0JVSUxEUEFUSCVcZG9jc1xlbl9VU1xodG1sIgogICAgIElGICVF UlJPUkxFVkVMJSBORVEgMCBFWElUIC9CICVFUlJPUkxFVkVMJQpkaWZmIC0t Z2l0IGEvcGtnL21hYy9idWlsZC5zaCBiL3BrZy9tYWMvYnVpbGQuc2gKaW5k ZXggMWZhZWUwOC4uM2ZkYjM0ZiAxMDA3NTUKLS0tIGEvcGtnL21hYy9idWls ZC5zaAorKysgYi9wa2cvbWFjL2J1aWxkLnNoCkBAIC0xNTYsOSArMTU2LDkg QEAgX2NvbXBsZXRlX2J1bmRsZSgpIHsKICAgICBjcCAtciAkU09VUkNFRElS L3dlYiAiJEJVSUxEUk9PVC8kQVBQX0JVTkRMRV9OQU1FL0NvbnRlbnRzL1Jl c291cmNlcy8iIHx8IGV4aXQgMQogICAgIGNkICIkQlVJTERST09ULyRBUFBf QlVORExFX05BTUUvQ29udGVudHMvUmVzb3VyY2VzL3dlYiIKICAgICBybSAt ZiBwZ2FkbWluNC5kYiBjb25maWdfbG9jYWwuKgotICAgIGVjaG8gIlNFUlZF Ul9NT0RFID0gRmFsc2UiID4gY29uZmlnX2xvY2FsLnB5Ci0gICAgZWNobyAi TUlOSUZZX0hUTUwgPSBGYWxzZSIgPj4gY29uZmlnX2xvY2FsLnB5Ci0gICAg ZWNobyAiSEVMUF9QQVRIID0gJy4uLy4uLy4uL2RvY3MvZW5fVVMvaHRtbC8n IiA+PiBjb25maWdfbG9jYWwucHkKKyAgICBlY2hvICJTRVJWRVJfTU9ERSA9 IEZhbHNlIiA+IGNvbmZpZ19kaXN0cm8ucHkKKyAgICBlY2hvICJNSU5JRllf SFRNTCA9IEZhbHNlIiA+PiBjb25maWdfZGlzdHJvLnB5CisgICAgZWNobyAi SEVMUF9QQVRIID0gJy4uLy4uLy4uL2RvY3MvZW5fVVMvaHRtbC8nIiA+PiBj b25maWdfZGlzdHJvLnB5CiAKICAgICAjIFJlbW92ZSB0aGUgLnB5YyBmaWxl cyBpZiBhbnkKICAgICBjZCAiJEJVSUxEUk9PVC8kQVBQX0JVTkRMRV9OQU1F IgpAQCAtMjExLDQgKzIxMSw0IEBAIF9idWlsZF9kb2MKIF9jb21wbGV0ZV9i dW5kbGUKIF9jb2Rlc2lnbl9idW5kbGUKIF9jcmVhdGVfZG1nCi1fY29kZXNp Z25fZG1nClwgTm8gbmV3bGluZSBhdCBlbmQgb2YgZmlsZQorX2NvZGVzaWdu X2RtZwo= --001a114f8b7ecdcfd6053f3719aa Content-Type: text/plain Content-Disposition: inline Content-Transfer-Encoding: 8bit MIME-Version: 1.0 -- Sent via pgadmin-hackers mailing list (pgadmin-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgadmin-hackers --001a114f8b7ecdcfd6053f3719aa--