Re: RM1849: Auto-generating security keys

Hi Dave,

On Sat, Oct 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, October 13, 2016, Ashesh Vashi <ashesh.vashi@enterprisedb.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 '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