Autoinstallers should read sql configuration from common files

[andersk]

The autoinstallers should be modified to read the SQL username and password from .my.cnf, and the SQL database name from a configuration file with a common format in the autoinstall directory (so that we can make scripts-remove work, and transfers between accounts easier).

In particular, the status quo of copying the password into the autoinstall means a (now only nearly) silent breach of the SQL database's security in the case where the locker has a couple of ACLs of different sizes -- anyone who can read the autoinstalled software's code can get the password, which the user may not expect.

---

• Mediawiki: see comment:9
• Django: fixed as per comment:2:ticket:343
• Trac: see comment:11
• Turbogears: see comment:13

[price]

Eric writes:

I was shocked today to find out that the scripts autoinstalls don't read the SQL password from ~/.sql/my.cnf, but instead copy the password from there into their own directories without telling the user.

The current state is misleading -- the autoinstaller doesn't ask for the password, and I assumed the autoinstall would do the same thing as the autoinstaller -- and leads to a couple bad consequences:

• It changes the security of my SQL password without telling me. Suppose I want my website's code to be readable by foo-discuss, and writable by foo-request. I would set web_scripts/ to be readable by foo-discuss, and .sql/ only readable by foo-request. When I autoinstall something, all my SQL databases suddenly become writable by foo-discuss and _I wouldn't realize it_.

The autoinstallers hard-code dropping privilege in the special case of "foo-discuss" being system:{any,auth}user, but that's cold comfort. When I decide to make all my web_scripts anyuser readable, it might not occur to me that some program I ran once that never mentioned SQL at all would expose my SQL password, jeopardizing databases that I care about.

• When I change my SQL password, my autoinstalls stop working. I won't check that they work for some time, and users in the meantime likely won't report the outage.

How hard can it be to change configuration lines from

$password='passwd'

to

$password=sed -n s/^password=//p ~/.sql/my.cnf | tr -d '\n' ?

And if fixing it is difficult, could you at least warn the user at install time that you're spewing his SQL password around?

Thanks, Eric

[ezyang]

Current autoinstallers tell the user that the SQL password is being copied into the directory. This bug still stands, however.

[adehnert]

How does this snippet look for doing this in Python? I don't just do ~/.sql/my.cnf since I'd like for stuff like manage.py to work from not-scripts.mit.edu. Presumably for auto-installers we'd want to hardcode the locker name and use the for_locker option.

The logic for auto-detecting the locker from the file could be punted without causing problems for this ticket, though it seems like it could have potential for other uses. This probably also fails without an automounter or for lockers that don't mount at /mit/lockername (which I thought I'd heard could exist back in the day, but probably break with Debathena?).

commit 26d71241be48ed968637c3d3523afd421f4cdc4f
Author: Alex Dehnert <adehnert@mit.edu>
Date:   Wed May 26 06:26:13 2010 -0400

    Locker and SQL detection for scripts-hosted sites

diff --git a/remit/util/scripts.py b/remit/util/scripts.py
new file mode 100644
index 0000000..f0ea993
--- /dev/null
+++ b/remit/util/scripts.py
@@ -0,0 +1,33 @@
+#!/usr/bin/python
+
+import ConfigParser
+import os.path
+
+def get_locker_root(for_file=None, for_locker=None):
+    if for_locker is not None:
+        return os.path.join('/mit', for_locker)
+    if for_file is None:
+        for_file = os.path.abspath(__file__)
+    installdirs = ['Scripts/django', 'web_scripts', ]
+    for installdir in installdirs:
+        start = for_file.find(installdir)
+        if start >= 0:
+            return for_file[:start]
+    return os.path.expanduser('~')
+
+
+def get_db_creds(for_file=None, for_locker=None):
+    cp = ConfigParser.ConfigParser()
+    cp.read([os.path.join(get_locker_root(for_file=for_file, for_locker=for_locker), '.my.cnf')])
+    ret = {}
+    for key in ['host', 'user', 'password', ]:
+        for section in ['client', 'mysql', ]:
+            if cp.has_section(section):
+                ret[key] = cp.get(section, key)
+    return ret
+
+if __name__ == '__main__':
+    print "Locker root:", get_locker_root()
+    print "Credentials:", get_db_creds()
+    print "Credentials (Remit for_file):", get_db_creds(for_file='/mit/remit/Scripts/django/')
+    print "Credentials (Remit for_locker):", get_db_creds(for_locker='remit')
adehnert@novgorod remit [06:36] $ python remit/util/scripts.py
Locker root: /afs/sipb.mit.edu/project/remit/
Credentials: {'host': 'sql.mit.edu', 'password': '[redacted]', 'user': 'remit'}
Credentials (Remit for_file): {'host': 'sql.mit.edu', 'password': '[redacted]', 'user': 'remit'}
Credentials (Remit for_locker): {'host': 'sql.mit.edu', 'password': '[redacted]', 'user': 'remit'}

[adehnert]

(As you might have guessed from some of those paths, if this seems reasonable I'll probably use it for Remit, whether I leave the code in Remit or it ends up somewhere in the scripts locker or on the servers or something.)

[xavid]

I'm not sure exactly how this is being used, but are you sure there isn't already built-in functionality for reading .my.cnf? I know TurboGears2 supports URIs like:

mysql://sql.mit.edu/xavid+scripts-pony?read_default_file=/mit/xavid/.my.cnf

and I'd expect other python apps to as well.

[andersk]

Yeah. For Django apps like Remit, you just set

DATABASE_ENGINE = 'mysql'
DATABASE_OPTIONS = {'read_default_file': '/path/to/my.cnf'}

[adehnert]

Not sure if there's a better mechanism, but for PHP/Mediawiki, putting something like parse_ini_file("/mit/asa/.my.cnf")['password'] seems to work. (Well, for Mediawiki itself. wizard doesn't know how to handle it.) Oh, and that requires my.cnf to be properly quoted, which may not be the case by default. At least it looks like mysql(1) and Python agree with PHP that double-quotes are okay. (See also ​http://www.php.net/manual/en/function.parse-ini-file.php.)

[adehnert]

Trac needs to wait until 0.13, I think (​http://trac.edgewall.org/ticket/5120), at which point read_default_file will be viable.

[cereslee]

Turbogears appears to have been fixed in some previous commit. I tested the autoinstaller today, and everything seems to we working fine.