Re: RM1849: Auto-generating security keys

Thanks, applied.

On Wed, Oct 19, 2016 at 1:39 PM, Sandeep Thakkar <
sandeep(dot)thakkar(at)enterprisedb(dot)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(dot)thakkar(at)enterprisedb(dot)com> wrote:
>
>> Hi Dave,
>>
>> On Wed, Oct 19, 2016 at 1:57 PM, Dave Page <dpage(at)pgadmin(dot)org> 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(dot)vashi(at)enterprisedb(dot)com> wrote:
>>>
>>>> Hi Dave,
>>>>
>>>> On Sat, Oct 15, 2016 at 8:02 AM, Dave Page <dpage(at)pgadmin(dot)org> wrote:
>>>>
>>>>> Hi
>>>>>
>>>>>
>>>>> On Friday, October 14, 2016, Dave Page <dpage(at)pgadmin(dot)org> wrote:
>>>>>
>>>>>> Hi
>>>>>>
>>>>>> On Thursday, October 13, 2016, Ashesh Vashi <
>>>>>> ashesh(dot)vashi(at)enterprisedb(dot)com> wrote:
>>>>>>
>>>>>>> Hi Dave,
>>>>>>>
>>>>>>> On Tue, Oct 11, 2016 at 9:10 PM, Dave Page <dpage(at)pgadmin(dot)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
>>>>>>> '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.enterprisedb.com/>
>>>>
>>>>
>>>> *http://www.linkedin.com/in/asheshvashi*
>>>> <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