installation: fix database password escaping #2846

eamonnmag · 2015-03-04T13:37:06Z

Wraps password in quotes. (closes installation: database password escaping #2844)

Signed-off-by: @eamonnmag
Reviewed-by: Jiri Kuncar [email protected]

Updated database.py to wrap password for init script in quotes

jirikuncar · 2015-03-04T14:26:27Z

@eamonnmag could you please amend the commit message according to Invenio standards.

Example:

installation: fix database password escaping

* FIX Wraps password in quotes.  (closes #2844)

Signed-off-by: you
Reviewed-by: Jiri Kuncar <[email protected]>

kaplun · 2015-03-04T14:53:21Z

invenio/base/scripts/database.py

@@ -85,7 +85,7 @@ def init(user='root', password='', yes_i_know=False):
            # Create user and grant access to database.
            (cmd_prefix + '-e "GRANT ALL PRIVILEGES ON '
             '{CFG_DATABASE_NAME}.* TO {CFG_DATABASE_USER}@localhost '
-             'IDENTIFIED BY {CFG_DATABASE_PASS}"'),
+             'IDENTIFIED BY \'{CFG_DATABASE_PASS}\'"'),


@eamonnmag actually this highlights the issue of proper escaping of the password. What if the password has a quotation sign? You should probably protect the CFG_DATABASE_PASS variable within args, so that any single quote is properly escaped.

kaplun · 2015-03-04T14:55:06Z

@eamonnmag could you please amend the commit message according to Invenio standards.

Yep. For reference these are detailed in https://github.com/inveniosoftware/invenio/blob/pu/CONTRIBUTING.rst (point 6)

jirikuncar · 2015-03-04T16:06:12Z

@kaplun can you test the PR? It looks useful for 2.0.0.

I checked how say the password 'pass\'\'word' was rendering and if it could be escaped. Python is automatically escaping this and actually creating this, pass'"'"''"'"'word. Given that developers will see that this password is clearly not what they intended from the log message, this is probably ok, they can just change it. Passwords such as 'pass''word' are rendered as 'password' due to python and it's concatenation. So, for me, I think this function does what it is supposed to do. The small fix at least removes any ambiguity for developers.

Pu

kaplun · 2015-03-05T19:45:39Z

Hi @eamonnmag, in Invenio we try to keep history as clean as possible (since it is then used to compile the change logs). Could you squash this commits into one clean commit (since it's implementing one functionality and it has not yet been merged)? Additionally you can happily just keep this PR. No need to close it or re-open it. Just push-force an updated branch. As soon as your branch will be integrated it will be automatically closed.

eamonnmag · 2015-03-05T19:58:57Z

Yeah, I sort of messed it up. Was the first time I tried it across forks :) I will look at the behaviour of the quotes function above my edits and modify it as discussed today.

jirikuncar · 2015-03-11T08:14:35Z

@eamonnmag please remove all extra newlines you have added outside init function. If you see that file doesn't follow PEP8/257 standard please create separate commit first. Thank you!

jirikuncar · 2015-03-11T08:18:59Z

Squash and rebase your branch:

$ git fetch upstream
$ git rebase -i upstream/maint-2.0
# in your favorite editor edit commits:
rework a1b2c3e foo: description
f b2c4d7d ...
# change first commit message squash or fixup commits
# see https://github.com/inveniosoftware/invenio/pull/2846#issuecomment-77166257
$ git push origin eamonnmag-dbinit-patch -f

PXke · 2015-03-17T14:17:01Z

invenio/base/scripts/database.py

@@ -77,17 +77,25 @@ def init(user='root', password='', yes_i_know=False):
        cmd_admin_prefix = prefix.format(cmd='mysqladmin', user=user,
                                         password=password,
                                         **args)
+
+        # we can't wrap all keys in quotes since the table name for instance is not quotable'
+        keys_to_wrap =['CFG_DATABASE_PASS', 'CFG_DATABASE_USER']


Perhaps should it be in config @jirikuncar ?

It's fine as it is, however @hachreak is working on some improvements https://github.com/hachreak/invenio/commits/refactor_database_init.

hachreak · 2015-03-18T09:23:15Z

the PR #2897 for my branch.

* BETTER Uses SQLAlchemy to init the database instead of execute mysql in a python subshell. (closes inveniosoftware#2846) (closes inveniosoftware#2844) * NEW Adds support for PostgreSQL database initialization. Signed-off-by: Leonardo Rossi <[email protected]> Reviewed-by: Jiri Kuncar <[email protected]>

* BETTER Uses SQLAlchemy and SQLAlchemy-Utils to init the database instead of execute mysql in a python subshell. (closes inveniosoftware#2846) (closes inveniosoftware#2844) * NEW Adds support for PostgreSQL database initialization. Signed-off-by: Leonardo Rossi <[email protected]> Reviewed-by: Jiri Kuncar <[email protected]>

* BETTER Uses SQLAlchemy and SQLAlchemy-Utils to init the database instead of executing mysql in a python subshell. (closes inveniosoftware#2846) (closes inveniosoftware#2844) * NEW Adds support for PostgreSQL database initialization. Signed-off-by: Leonardo Rossi <[email protected]> Reviewed-by: Jiri Kuncar <[email protected]> Signed-off-by: Leonardo Rossi <[email protected]>

* BETTER Uses SQLAlchemy and SQLAlchemy-Utils to init the database instead of executing mysql in a python subshell. (closes inveniosoftware#2846) (closes inveniosoftware#2844) * NEW Adds support for PostgreSQL database initialization. Signed-off-by: Leonardo Rossi <[email protected]> Reviewed-by: Jiri Kuncar <[email protected]>

Updated database.py to wrap password for init script in quotes

3fcf5df

Fixes inveniosoftware#2844

invenio-developers added the Status: in review label Mar 4, 2015

Merge pull request #1 from eamonnmag/eamonnmag-dbinit-patch

953e10a

Updated database.py to wrap password for init script in quotes

eamonnmag changed the title ~~BaseScripts: updated database.py to wrap password for init script in quotes~~ BaseScripts: updated database.py to wrap password in init Mar 4, 2015

jirikuncar added Type: bug Component: installation labels Mar 4, 2015

jirikuncar added this to the v2.0.0 milestone Mar 4, 2015

jirikuncar added the Branch: master label Mar 4, 2015

jirikuncar changed the title ~~BaseScripts: updated database.py to wrap password in init~~ installation: fix database password escaping Mar 4, 2015

kaplun reviewed Mar 4, 2015
View reviewed changes

jirikuncar modified the milestones: v2.0.x, v2.0.0 Mar 4, 2015

eamonnmag closed this Mar 4, 2015

eamonnmag reopened this Mar 4, 2015

Merge pull request #2 from eamonnmag/pu

720bed7

Pu

PXke reviewed Mar 17, 2015
View reviewed changes

jirikuncar modified the milestones: v2.0.1, 2.0.x Mar 20, 2015

jirikuncar modified the milestones: v2.1, v2.0.2 Mar 25, 2015

jirikuncar closed this in 09cc403 Apr 2, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

installation: fix database password escaping #2846

installation: fix database password escaping #2846

eamonnmag commented Mar 4, 2015

jirikuncar commented Mar 4, 2015

kaplun Mar 4, 2015

eamonnmag Mar 4, 2015

kaplun commented Mar 4, 2015

jirikuncar commented Mar 4, 2015

kaplun commented Mar 5, 2015

eamonnmag commented Mar 5, 2015

jirikuncar commented Mar 11, 2015

jirikuncar commented Mar 11, 2015

PXke Mar 17, 2015

jirikuncar Mar 17, 2015

hachreak commented Mar 18, 2015

installation: fix database password escaping #2846

installation: fix database password escaping #2846

Conversation

eamonnmag commented Mar 4, 2015

jirikuncar commented Mar 4, 2015

kaplun Mar 4, 2015

Choose a reason for hiding this comment

eamonnmag Mar 4, 2015

Choose a reason for hiding this comment

kaplun commented Mar 4, 2015

jirikuncar commented Mar 4, 2015

kaplun commented Mar 5, 2015

eamonnmag commented Mar 5, 2015

jirikuncar commented Mar 11, 2015

jirikuncar commented Mar 11, 2015

PXke Mar 17, 2015

Choose a reason for hiding this comment

jirikuncar Mar 17, 2015

Choose a reason for hiding this comment

hachreak commented Mar 18, 2015