Re: RM1849: Auto-generating security keys

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
>
>