public inbox for [email protected]
help / color / mirror / Atom feedFrom: Ashesh Vashi <[email protected]>
To: Dave Page <[email protected]>
Cc: pgadmin-hackers <[email protected]>
Cc: Josh Berkus <[email protected]>
Cc: Devrim GÜNDÜZ <[email protected]>
Cc: Magnus Hagander <[email protected]>
Subject: Re: RM1849: Auto-generating security keys
Date: Wed, 19 Oct 2016 11:16:26 +0530
Message-ID: <CAG7mmoxdj89rZY8dKGVC6QFY6tvHCPyLt3Zqg3FBiqT5Ah21xA@mail.gmail.com> (raw)
In-Reply-To: <CA+OCxowJPuXo8QBnbhhA-kycJpzYSvhXnsrutuo4oce0Su9U+w@mail.gmail.com>
References: <CA+OCxownxfR2eDEaXNkgSdFqat6+AQgukrzcYOyoFX0V-zs_VA@mail.gmail.com>
<CAG7mmoyhJXXrLv+fgjmgd-Z5GFSaJfHTC89MWQ8LQX3Atw-04A@mail.gmail.com>
<CA+OCxozo4=FCjorfF8j4QH=p4iEa15Bp4P0bD5+Ch=aFY37ERg@mail.gmail.com>
<CA+OCxowJPuXo8QBnbhhA-kycJpzYSvhXnsrutuo4oce0Su9U+w@mail.gmail.com>
List-Unsubscribe: <mailto:[email protected]?body=unsub%20pgadmin-hackers>
Hi Dave,
On Sat, Oct 15, 2016 at 8:02 AM, Dave Page <[email protected]> wrote:
> Hi
>
>
> On Friday, October 14, 2016, Dave Page <[email protected]> wrote:
>
>> Hi
>>
>> On Thursday, October 13, 2016, Ashesh Vashi <
>> [email protected]> wrote:
>>
>>> Hi Dave,
>>>
>>> On Tue, Oct 11, 2016 at 9:10 PM, Dave Page <[email protected]> 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
>
>
--
Sent via pgadmin-hackers mailing list ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers
Attachments:
[application/octet-stream] auto_generate_security_keys_v3.patch (11.0K, 3-auto_generate_security_keys_v3.patch)
download | inline diff:
diff --git a/web/config.py b/web/config.py
index 20714f9..8411d79 100644
--- a/web/config.py
+++ b/web/config.py
@@ -140,21 +140,13 @@ DEFAULT_SERVER_PORT = 5050
# Enable CSRF protection?
CSRF_ENABLED = True
-# Secret key for signing CSRF data. Override this in config_local.py if
-# running on a web server
-CSRF_SESSION_KEY = 'SuperSecret1'
-
-# Secret key for signing cookies. Override this in config_local.py if
-# running on a web server
-SECRET_KEY = 'SuperSecret2'
-
-# Salt used when hashing passwords. Override this in config_local.py if
-# running on a web server
-SECURITY_PASSWORD_SALT = 'SuperSecret3'
-
# Hashing algorithm used for password storage
SECURITY_PASSWORD_HASH = 'pbkdf2_sha512'
+# NOTE: CSRF_SESSION_KEY, SECRET_KEY and SECURITY_PASSWORD_SALT are no
+# longer part of the main configuration, but are stored in the
+# configuration databases 'keys' table and are auto-generated.
+
# Should HTML be minified on the fly when not in debug mode?
# Note: This is disabled by default as it will error when processing the
# docs. If the serving of docs is handled by an Apache HTTPD
diff --git a/web/pgAdmin4.py b/web/pgAdmin4.py
index 1fb34f9..f894f8b 100644
--- a/web/pgAdmin4.py
+++ b/web/pgAdmin4.py
@@ -32,18 +32,6 @@ config.SETTINGS_SCHEMA_VERSION = SCHEMA_VERSION
# Sanity checks
##########################################################################
-# Check for local settings if running in server mode
-if config.SERVER_MODE is True:
- local_config = os.path.join(os.path.dirname(os.path.realpath(__file__)),
- 'config_local.py')
- if not os.path.isfile(local_config):
- print("The configuration file %s does not exist.\n" % local_config)
- print("Before running this application, ensure that config_local.py has been created")
- print("and sets values for SECRET_KEY, SECURITY_PASSWORD_SALT and CSRF_SESSION_KEY")
- print("at bare minimum. See config.py for more information and a complete list of")
- print("settings. Exiting...")
- sys.exit(1)
-
# Check if the database exists. If it does not, create it.
if not os.path.isfile(config.SQLITE_PATH):
setupfile = os.path.join(os.path.dirname(os.path.realpath(__file__)),
diff --git a/web/pgadmin/__init__.py b/web/pgadmin/__init__.py
index d988172..79fa1c6 100644
--- a/web/pgadmin/__init__.py
+++ b/web/pgadmin/__init__.py
@@ -26,7 +26,7 @@ from pgadmin.utils.session import create_session_interface
from werkzeug.local import LocalProxy
from werkzeug.utils import find_modules
-from pgadmin.model import db, Role, Server, ServerGroup, User, Version
+from pgadmin.model import db, Role, Server, ServerGroup, User, Version, Keys
# Configuration settings
import config
@@ -127,11 +127,6 @@ def create_app(app_name=config.APP_NAME):
app.config.update(dict(PROPAGATE_EXCEPTIONS=True))
##########################################################################
- # Setup session management
- ##########################################################################
- app.session_interface = create_session_interface(app)
-
- ##########################################################################
# Setup logging and log the application startup
##########################################################################
@@ -206,7 +201,7 @@ def create_app(app_name=config.APP_NAME):
# Setup Flask-Security
user_datastore = SQLAlchemyUserDatastore(db, User, Role)
- security = Security(app, user_datastore)
+ security = Security(None, user_datastore)
# Upgrade the schema (if required)
with app.app_context():
@@ -220,9 +215,29 @@ def create_app(app_name=config.APP_NAME):
)
)
from setup import do_upgrade
- do_upgrade(app, user_datastore, security, version)
+ do_upgrade(app, user_datastore, version)
+
+ ##########################################################################
+ # Setup security
+ ##########################################################################
+ with app.app_context():
+ config.CSRF_SESSION_KEY = Keys.query.filter_by(name = 'CSRF_SESSION_KEY').first().value
+ config.SECRET_KEY = Keys.query.filter_by(name = 'SECRET_KEY').first().value
+ config.SECURITY_PASSWORD_SALT = Keys.query.filter_by(name = 'SECURITY_PASSWORD_SALT').first().value
+
+ # Update the app.config with proper security keyes for signing CSRF data,
+ # signing cookies, and the SALT for hashing the passwords.
+ app.config.update(dict(CSRF_SESSION_KEY=config.CSRF_SESSION_KEY))
+ app.config.update(dict(SECRET_KEY=config.SECRET_KEY))
+ app.config.update(dict(SECURITY_PASSWORD_SALT=config.SECURITY_PASSWORD_SALT))
+ security.init_app(app)
+
+ app.session_interface = create_session_interface(app)
+
+ ##########################################################################
# Load all available server drivers
+ ##########################################################################
driver.init_app(app)
##########################################################################
diff --git a/web/pgadmin/model/__init__.py b/web/pgadmin/model/__init__.py
index 019e9b1..9727d2b 100644
--- a/web/pgadmin/model/__init__.py
+++ b/web/pgadmin/model/__init__.py
@@ -29,7 +29,7 @@ from flask_sqlalchemy import SQLAlchemy
#
##########################################################################
-SCHEMA_VERSION = 13
+SCHEMA_VERSION = 14
##########################################################################
#
@@ -207,3 +207,10 @@ class Process(db.Model):
end_time = db.Column(db.String(), nullable=True)
exit_code = db.Column(db.Integer(), nullable=True)
acknowledge = db.Column(db.String(), nullable=True)
+
+
+class Keys(db.Model):
+ """Define the keys table."""
+ __tablename__ = 'keys'
+ name = db.Column(db.String(), nullable=False, primary_key=True)
+ value = db.Column(db.String(), nullable=False)
\ No newline at end of file
diff --git a/web/setup.py b/web/setup.py
index 64273fb..12465f3 100755
--- a/web/setup.py
+++ b/web/setup.py
@@ -10,6 +10,7 @@
"""Perform the initial setup of the application, by creating the auth
and settings database."""
+import base64
import getpass
import os
import random
@@ -22,7 +23,7 @@ from flask_security import Security, SQLAlchemyUserDatastore
from flask_security.utils import encrypt_password
from pgadmin.model import db, Role, User, Server, \
- ServerGroup, Version
+ ServerGroup, Version, Keys
# Configuration settings
import config
@@ -40,6 +41,7 @@ if hasattr(__builtins__, 'raw_input'):
def do_setup(app):
"""Create a new settings database from scratch"""
+
if config.SERVER_MODE is False:
print("NOTE: Configuring authentication for DESKTOP mode.")
email = config.DESKTOP_USER
@@ -116,6 +118,17 @@ def do_setup(app):
name='ConfigDB', value=config.SETTINGS_SCHEMA_VERSION
)
db.session.merge(version)
+ db.session.commit()
+
+ # Create the keys
+ key = Keys(name='CSRF_SESSION_KEY', value=config.CSRF_SESSION_KEY)
+ db.session.merge(key)
+
+ key = Keys(name='SECRET_KEY', value=config.SECRET_KEY)
+ db.session.merge(key)
+
+ key = Keys(name='SECURITY_PASSWORD_SALT', value=config.SECURITY_PASSWORD_SALT)
+ db.session.merge(key)
db.session.commit()
@@ -128,7 +141,7 @@ def do_setup(app):
)
-def do_upgrade(app, datastore, security, version):
+def do_upgrade(app, datastore, version):
"""Upgrade an existing settings database"""
#######################################################################
# Run whatever is required to update the database schema to the current
@@ -329,6 +342,29 @@ ALTER TABLE SERVER
ADD COLUMN discovery_id TEXT
""")
+ if int(version.value) < 14:
+ db.engine.execute("""
+CREATE TABLE keys (
+ name TEST NOT NULL,
+ value TEXT NOT NULL,
+ PRIMARY KEY (name))
+ """)
+
+ sql = "INSERT INTO keys (name, value) VALUES ('CSRF_SESSION_KEY', '%s')" % base64.urlsafe_b64encode(os.urandom(32))
+ db.engine.execute(sql)
+
+ sql = "INSERT INTO keys (name, value) VALUES ('SECRET_KEY', '%s')" % base64.urlsafe_b64encode(os.urandom(32))
+ db.engine.execute(sql)
+
+ # If SECURITY_PASSWORD_SALT is not in the config, but we're upgrading, then it must (unless the
+ # user edited the main config - which they shouldn't have done) have been at it's default
+ # value, so we'll use that. Otherwise, use whatever we can find in the config.
+ if hasattr(config, 'SECURITY_PASSWORD_SALT'):
+ sql = "INSERT INTO keys (name, value) VALUES ('SECURITY_PASSWORD_SALT', '%s')" % config.SECURITY_PASSWORD_SALT
+ else:
+ sql = "INSERT INTO keys (name, value) VALUES ('SECURITY_PASSWORD_SALT', 'SuperSecret3')"
+ db.engine.execute(sql)
+
# Finally, update the schema version
version.value = config.SETTINGS_SCHEMA_VERSION
db.session.merge(version)
@@ -347,6 +383,7 @@ ALTER TABLE SERVER
###############################################################################
if __name__ == '__main__':
app = Flask(__name__)
+
app.config.from_object(config)
if config.TESTING_MODE:
@@ -364,15 +401,6 @@ if __name__ == '__main__':
'config_local.py'
)
- if not os.path.isfile(local_config):
- print("""
- The configuration file - {0} does not exist.
- Before running this application, ensure that config_local.py has been created
- and sets values for SECRET_KEY, SECURITY_PASSWORD_SALT and CSRF_SESSION_KEY
- at bare minimum. See config.py for more information and a complete list of
- settings. Exiting...""".format(local_config))
- sys.exit(1)
-
# Check if the database exists. If it does, tell the user and exit.
if os.path.isfile(config.SQLITE_PATH):
print("""
@@ -381,7 +409,6 @@ Entering upgrade mode...""" % config.SQLITE_PATH)
# Setup Flask-Security
user_datastore = SQLAlchemyUserDatastore(db, User, Role)
- security = Security(app, user_datastore)
# Always use "< REQUIRED_VERSION" as the test for readability
with app.app_context():
@@ -403,8 +430,13 @@ Exiting...""" % (version.value))
print("NOTE: Upgrading database schema from version %d to %d." % (
version.value, config.SETTINGS_SCHEMA_VERSION
))
- do_upgrade(app, user_datastore, security, version)
+ do_upgrade(app, user_datastore, version)
else:
+ # Get some defaults for the various keys
+ config.CSRF_SESSION_KEY = base64.urlsafe_b64encode(os.urandom(32))
+ config.SECRET_KEY = base64.urlsafe_b64encode(os.urandom(32))
+ config.SECURITY_PASSWORD_SALT = base64.urlsafe_b64encode(os.urandom(32))
+
directory = os.path.dirname(config.SQLITE_PATH)
if not os.path.exists(directory):
os.makedirs(directory, int('700', 8))
view thread (33+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: RM1849: Auto-generating security keys
In-Reply-To: <CAG7mmoxdj89rZY8dKGVC6QFY6tvHCPyLt3Zqg3FBiqT5Ah21xA@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox