[aur-dev] [PATCH 0/4] FS#45410, TUs bypassing blacklist
This patch series are meant to firstly simplify somewhat, and then to implement FS#45410. First version. Johannes Löthberg (4): git: Use AUR_USER env var instead of ForceCommand argument git-auth: Set AUR_PRIVILEGED env var for TUs & devs git-serve: Drop direct AccountType checking, use AUR_PRIVILEGED git-update: Allow privileged users to bypass blacklist git-interface/git-auth.py | 29 +++++++++++++++++++++++++---- git-interface/git-serve.py | 6 +++--- git-interface/git-update.py | 3 ++- 3 files changed, 30 insertions(+), 8 deletions(-) -- 2.4.4
Also add an utility function for formatting the ForceCommand Signed-off-by: Johannes Löthberg <johannes@kyriasis.com> --- git-interface/git-auth.py | 18 ++++++++++++++++-- git-interface/git-serve.py | 2 +- 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/git-interface/git-auth.py b/git-interface/git-auth.py index c9e1f01..af1f8d4 100755 --- a/git-interface/git-auth.py +++ b/git-interface/git-auth.py @@ -6,6 +6,16 @@ import os import re import sys + +def format_command(env_vars, command, ssh_opts, key): + environment = '' + for key, var in env_vars.items(): + environment += '{}={} && '.format(key, var) + + msg = 'command="{}{}",{} {}'.format(environment, command, ssh_opts, key) + return msg + + config = configparser.RawConfigParser() config.read(os.path.dirname(os.path.realpath(__file__)) + "/../conf/config") @@ -40,5 +50,9 @@ user = cur.fetchone()[0] if not re.match(username_regex, user): exit(1) -print('command="%s %s",%s %s' % (git_serve_cmd, user, ssh_opts, - keytype + " " + keytext)) +env_vars = { + 'AUR_USER': user, +} +key = keytype + ' ' + keytext + +print(format_command(env_vars, git_serve_cmd, ssh_opts, key)) diff --git a/git-interface/git-serve.py b/git-interface/git-serve.py index 26aa02d..6f521cc 100755 --- a/git-interface/git-serve.py +++ b/git-interface/git-serve.py @@ -106,7 +106,7 @@ def die(msg): def die_with_help(msg): die(msg + "\nTry `%s help` for a list of commands." % (ssh_cmdline)) -user = sys.argv[1] +user = os.environ.get("AUR_USER") cmd = os.environ.get("SSH_ORIGINAL_COMMAND") if not cmd: die_with_help("Interactive shell is disabled.") -- 2.4.4
On Tue, 23 Jun 2015 at 00:00:18, Johannes Löthberg wrote:
Also add an utility function for formatting the ForceCommand
Missing punctuation :)
Signed-off-by: Johannes Löthberg <johannes@kyriasis.com> --- git-interface/git-auth.py | 18 ++++++++++++++++-- git-interface/git-serve.py | 2 +- 2 files changed, 17 insertions(+), 3 deletions(-)
diff --git a/git-interface/git-auth.py b/git-interface/git-auth.py index c9e1f01..af1f8d4 100755 --- a/git-interface/git-auth.py +++ b/git-interface/git-auth.py @@ -6,6 +6,16 @@ import os import re import sys
+ +def format_command(env_vars, command, ssh_opts, key): + environment = '' + for key, var in env_vars.items(): + environment += '{}={} && '.format(key, var)
I like this idea. I noticed three things, though: 1. We do not seem to use format() anywhere else. Maybe use the % operator for consistency? But then again, % is obsolete, so we should probably rather replace all the existing formatting operations by format() (in another patch)... 2. Is there any reason to &&-chain the environment variable assignments? Does this even work? When I run FOO=bar sh -c 'echo $FOO' it prints "bar" as expected, but when I insert "&&" between the first two tokens it doesn't. 3. I notice this is not strictly needed now but if we write a helper function, it might be a good idea to quote the arguments (e.g. using shlex.quote). It is not unlikely that we want to add arguments that may contains spaces some day. This is the most security critical part of the Git interface. If we do not escape things properly, users might be able to execute arbitrary code on the server. We should be extra cautious here...
+ + msg = 'command="{}{}",{} {}'.format(environment, command, ssh_opts, key) + return msg + + config = configparser.RawConfigParser() config.read(os.path.dirname(os.path.realpath(__file__)) + "/../conf/config")
@@ -40,5 +50,9 @@ user = cur.fetchone()[0] if not re.match(username_regex, user): exit(1)
-print('command="%s %s",%s %s' % (git_serve_cmd, user, ssh_opts, - keytype + " " + keytext)) +env_vars = { + 'AUR_USER': user, +} +key = keytype + ' ' + keytext + +print(format_command(env_vars, git_serve_cmd, ssh_opts, key)) diff --git a/git-interface/git-serve.py b/git-interface/git-serve.py index 26aa02d..6f521cc 100755 --- a/git-interface/git-serve.py +++ b/git-interface/git-serve.py @@ -106,7 +106,7 @@ def die(msg): def die_with_help(msg): die(msg + "\nTry `%s help` for a list of commands." % (ssh_cmdline))
-user = sys.argv[1] +user = os.environ.get("AUR_USER") cmd = os.environ.get("SSH_ORIGINAL_COMMAND") if not cmd: die_with_help("Interactive shell is disabled.") -- 2.4.4
On 23/06, Lukas Fleischer wrote:
On Tue, 23 Jun 2015 at 00:00:18, Johannes Löthberg wrote:
Also add an utility function for formatting the ForceCommand
Missing punctuation :)
Ah, silly me.
+def format_command(env_vars, command, ssh_opts, key): + environment = '' + for key, var in env_vars.items(): + environment += '{}={} && '.format(key, var)
I like this idea. I noticed three things, though:
1. We do not seem to use format() anywhere else. Maybe use the % operator for consistency? But then again, % is obsolete, so we should probably rather replace all the existing formatting operations by format() (in another patch)...
Yeah, I'll see about replacing the other uses.
2. Is there any reason to &&-chain the environment variable assignments? Does this even work? When I run
FOO=bar sh -c 'echo $FOO'
it prints "bar" as expected, but when I insert "&&" between the first two tokens it doesn't.
Thing is that the command will actually be sh -c 'FOO=bar echo $FOO' which won't work. Either sh -c 'FOO=bar && echo $FOO' or sh -c 'FOO=bar; echo $FOO' is needed.
3. I notice this is not strictly needed now but if we write a helper function, it might be a good idea to quote the arguments (e.g. using shlex.quote). It is not unlikely that we want to add arguments that may contains spaces some day. This is the most security critical part of the Git interface. If we do not escape things properly, users might be able to execute arbitrary code on the server. We should be extra cautious here...
Yes, that's fair, will add that. -- Sincerely, Johannes Löthberg PGP Key ID: 0x50FB9B273A9D0BB5 https://theos.kyriasis.com/~kyrias/
Signed-off-by: Johannes Löthberg <johannes@kyriasis.com> --- git-interface/git-auth.py | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/git-interface/git-auth.py b/git-interface/git-auth.py index af1f8d4..668f6c1 100755 --- a/git-interface/git-auth.py +++ b/git-interface/git-auth.py @@ -40,18 +40,25 @@ db = mysql.connector.connect(host=aur_db_host, user=aur_db_user, unix_socket=aur_db_socket, buffered=True) cur = db.cursor() -cur.execute("SELECT Username FROM Users WHERE SSHPubKey = %s " + +cur.execute("SELECT Username, AccountTypeID FROM Users WHERE SSHPubKey = %s " + "AND Suspended = 0", (keytype + " " + keytext,)) if cur.rowcount != 1: exit(1) -user = cur.fetchone()[0] +user, account_type = cur.fetchone()[0] if not re.match(username_regex, user): exit(1) +if account_type > 1: + privileged = True +else: + privileged = False + + env_vars = { 'AUR_USER': user, + 'AUR_PRIVILEGED': privileged, } key = keytype + ' ' + keytext -- 2.4.4
On Tue, 23 Jun 2015 at 00:00:19, Johannes Löthberg wrote:
Signed-off-by: Johannes Löthberg <johannes@kyriasis.com> --- git-interface/git-auth.py | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/git-interface/git-auth.py b/git-interface/git-auth.py index af1f8d4..668f6c1 100755 --- a/git-interface/git-auth.py +++ b/git-interface/git-auth.py @@ -40,18 +40,25 @@ db = mysql.connector.connect(host=aur_db_host, user=aur_db_user, unix_socket=aur_db_socket, buffered=True)
cur = db.cursor() -cur.execute("SELECT Username FROM Users WHERE SSHPubKey = %s " + +cur.execute("SELECT Username, AccountTypeID FROM Users WHERE SSHPubKey = %s " + "AND Suspended = 0", (keytype + " " + keytext,))
if cur.rowcount != 1: exit(1)
-user = cur.fetchone()[0] +user, account_type = cur.fetchone()[0] if not re.match(username_regex, user): exit(1)
+if account_type > 1: + privileged = True +else: + privileged = False + + env_vars = { 'AUR_USER': user, + 'AUR_PRIVILEGED': privileged,
Maybe drop the if-else-block above and directly use (account_type > 1) here? I am not sure about which version is more readable...
} key = keytype + ' ' + keytext
-- 2.4.4
On 23/06, Lukas Fleischer wrote:
On Tue, 23 Jun 2015 at 00:00:19, Johannes Löthberg wrote:
+if account_type > 1: + privileged = True +else: + privileged = False + + env_vars = { 'AUR_USER': user, + 'AUR_PRIVILEGED': privileged,
Maybe drop the if-else-block above and directly use (account_type > 1) here? I am not sure about which version is more readable...
I think I'd find the if-else more readable, but I can switch if you want. -- Sincerely, Johannes Löthberg PGP Key ID: 0x50FB9B273A9D0BB5 https://theos.kyriasis.com/~kyrias/
Signed-off-by: Johannes Löthberg <johannes@kyriasis.com> --- git-interface/git-serve.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/git-interface/git-serve.py b/git-interface/git-serve.py index 6f521cc..2083560 100755 --- a/git-interface/git-serve.py +++ b/git-interface/git-serve.py @@ -86,8 +86,8 @@ def check_permissions(pkgbase, user): unix_socket=aur_db_socket, buffered=True) cur = db.cursor() - cur.execute("SELECT AccountTypeID FROM Users WHERE UserName = %s ", [user]) - if cur.fetchone()[0] > 1: + privileged = os.environ.get('AUR_PRIVILEGED', False) + if privileged: return True cur.execute("SELECT COUNT(*) FROM PackageBases " + -- 2.4.4
On Tue, 23 Jun 2015 at 00:00:20, Johannes Löthberg wrote:
Signed-off-by: Johannes Löthberg <johannes@kyriasis.com> --- git-interface/git-serve.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/git-interface/git-serve.py b/git-interface/git-serve.py index 6f521cc..2083560 100755 --- a/git-interface/git-serve.py +++ b/git-interface/git-serve.py @@ -86,8 +86,8 @@ def check_permissions(pkgbase, user): unix_socket=aur_db_socket, buffered=True) cur = db.cursor()
- cur.execute("SELECT AccountTypeID FROM Users WHERE UserName = %s ", [user]) - if cur.fetchone()[0] > 1: + privileged = os.environ.get('AUR_PRIVILEGED', False) + if privileged:
I think this is readable enough as a one-liner (without the extra variable).
return True
cur.execute("SELECT COUNT(*) FROM PackageBases " + -- 2.4.4
On 23/06, Lukas Fleischer wrote:
On Tue, 23 Jun 2015 at 00:00:20, Johannes Löthberg wrote:
Signed-off-by: Johannes Löthberg <johannes@kyriasis.com> --- git-interface/git-serve.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/git-interface/git-serve.py b/git-interface/git-serve.py index 6f521cc..2083560 100755 --- a/git-interface/git-serve.py +++ b/git-interface/git-serve.py @@ -86,8 +86,8 @@ def check_permissions(pkgbase, user): unix_socket=aur_db_socket, buffered=True) cur = db.cursor()
- cur.execute("SELECT AccountTypeID FROM Users WHERE UserName = %s ", [user]) - if cur.fetchone()[0] > 1: + privileged = os.environ.get('AUR_PRIVILEGED', False) + if privileged:
I think this is readable enough as a one-liner (without the extra variable).
Indeed, didn't think about it. -- Sincerely, Johannes Löthberg PGP Key ID: 0x50FB9B273A9D0BB5 https://theos.kyriasis.com/~kyrias/
Fixes FS#45410. Signed-off-by: Johannes Löthberg <johannes@kyriasis.com> --- git-interface/git-update.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/git-interface/git-update.py b/git-interface/git-update.py index 41a3474..c7b3389 100755 --- a/git-interface/git-update.py +++ b/git-interface/git-update.py @@ -185,6 +185,7 @@ sha1_new = sys.argv[3] user = os.environ.get("AUR_USER") pkgbase = os.environ.get("AUR_PKGBASE") +privileged = os.environ.get("AUR_PRIVILEGED", False) if refname != "refs/heads/master": die("pushing to a branch other than master is restricted") @@ -295,7 +296,7 @@ for pkgname in srcinfo.GetPackageNames(): pkginfo = srcinfo.GetMergedPackage(pkgname) pkgname = pkginfo['pkgname'] - if pkgname in blacklist: + if pkgname in blacklist and not privileged: die('package is blacklisted: %s' % (pkginfo['pkgname'])) cur.execute("SELECT COUNT(*) FROM Packages WHERE Name = %s AND " + -- 2.4.4
participants (2)
-
Johannes Löthberg
-
Lukas Fleischer