Re: RM1849: Auto-generating security keys

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