From b46e3dc9bc770cd36222e5c36ed44a34ff6a05b0 Mon Sep 17 00:00:00 2001 From: Antoine Martin Date: Fri, 8 Apr 2016 06:20:44 +0000 Subject: [PATCH] #1159 / #1133: * restore authentication using XPRA_PASSWORD env var, directly for client side * add env_auth module * man page updates * ensure file auth modules use hmac and not xor! * var reference fixes (move auth code to sys auth so we can re-use it - but don't reference files there) * more unit test coverage git-svn-id: https://xpra.org/svn/Xpra/trunk@12334 3bb7dfac-3a0b-4e04-842a-767bc560f471 --- src/man/xpra.1 | 45 +++++++++++++++-------- src/tests/unit/server/auth_test.py | 49 ++++++++++++++++++++++---- src/xpra/client/client_base.py | 16 +++++---- src/xpra/server/auth/env_auth.py | 26 ++++++++++++++ src/xpra/server/auth/file_auth_base.py | 3 +- src/xpra/server/auth/multifile_auth.py | 5 ++- src/xpra/server/auth/password_auth.py | 26 ++++++++++++++ src/xpra/server/auth/sys_auth_base.py | 44 ++++++++++++++++++++--- src/xpra/server/server_core.py | 13 +++++-- 9 files changed, 190 insertions(+), 37 deletions(-) create mode 100644 src/xpra/server/auth/env_auth.py create mode 100644 src/xpra/server/auth/password_auth.py diff --git a/src/man/xpra.1 b/src/man/xpra.1 index 218be6095a..43405c79ea 100644 --- a/src/man/xpra.1 +++ b/src/man/xpra.1 @@ -54,7 +54,7 @@ xpra \- viewer for remote, persistent X applications [\fB\-\-tcp\-encryption\-keyfile\fP=\fIKEYFILE\fP] [\fB\-\-auth\fP=\fIMODULE\fP] [\fB\-\-tcp\-auth\fP=\fIMODULE\fP] -[\fB\-\-password\-file\fP=\fIFILENAME\fP] +[\fB\-\-vsock\-auth\fP=\fIMODULE\fP] [\fB\-\-idle\-timeout\fP=\fIIDLETIMEOUT\fP] [\fB\-\-server\-idle\-timeout\fP=\fIIDLETIMEOUT\fP] [\fB\-\-clipboard\-filter\-file\fP=\fIFILENAME\fP] @@ -136,7 +136,7 @@ xpra \- viewer for remote, persistent X applications [\fB\-\-bind\-vsock\fP=\fICID:PORT\fP] [\fB\-\-auth\fP=\fIMODULE\fP] [\fB\-\-tcp\-auth\fP=\fIMODULE\fP] -[\fB\-\-password\-file\fP=\fIFILENAME\fP] +[\fB\-\-vsock\-auth\fP=\fIMODULE\fP] [\fB\-\-idle\-timeout\fP=\fIIDLETIMEOUT\fP] [\fB\-\-server\-idle\-timeout\fP=\fIIDLETIMEOUT\fP] [\fB\-\-socket\-dir\fP=\fIDIR\fP] @@ -603,7 +603,7 @@ of your actions. \fB\-\-bind\-vsock\fP=\fICID:PORT\fP Create a VSOCK socket for each \fB\-\-bind\-vsock\fP option specified. .TP -\fB\-\-auth\fP=\fIMODULE\fP +\fB\-\-auth\fP=\fIMODULE[:OPTION=VALUE]\fP Specifies the authentication module to use for unix domain sockets created using the \fBbind\fP switch. Authentication modules can validate a username and password against a variety of backend modules: @@ -613,9 +613,19 @@ always allows authentication - this is dangerous and should only be used for testing .IP \fBfail\fP always fails authentication, useful for testing +.IP \fBenv\fP +matches against the environment variable specified by the \fIname\fP option +(which defaults to \fBXPRA_PASSWORD\fP). +ie: \fB--auth=env:name=SOME_OTHER_ENV_VAR_NAME\fP. +.IP \fBpassword\fP +matches against the password specified using the \fIvalue\fP option. +ie: \fB--auth=password:value=YOURPASSWORD\fP. +Note: this command line option may be exposed to other processes +on the same system. .IP \fBfile\fP checks the password against the password data found in the file -specified using the \fBpassword\-file\fP switch. +specified using the \fBfilename\fP option. +ie: \fB--auth=file:filename=./password.txt\fP. The contents of this file will be treated as binary data, there are no restrictions on character encodings or file size, @@ -626,7 +636,7 @@ and the pipe character \fI|\fP. .IP \fBmultifile\fP checks the username and password against the file specified using -the \fBpassword\-file\fP switch. +the filename option. The file must contain each user credentials on one line of the form: \fIusername|password|uid|gid|displays|env_opts|session_opts\fP @@ -649,6 +659,10 @@ automatically (either \fBpam\fP or \fBwin32\fP) Just like the \fBauth\fP switch, except this one only applies to TCP sockets (sockets defined using the \fBbind\-tcp\fP switch). .TP +\fB\-\-vsock\-auth\fP=\fIMODULE\fP +Just like the \fBauth\fP switch, except this one only applies +to VSOCK sockets (sockets defined using the \fBbind\-vsock\fP switch). +.TP \fB\-\-mdns\fP=\fIyes\fP|\fIno\fP Enable or disable the publication of new sessions via mDNS. .TP @@ -765,14 +779,6 @@ Defaults to 600. This is ignored when \fImmap\-group\fP is enabled. .SS Options for start, upgrade and attach .TP -\fB\-\-password\-file\fP=\fIFILENAME\fP -This allows sessions to be secured with a password stored in a text -file. You should use this if you use the \fB\-\-bind\-tcp\fP option. -If this is used on the server, it will reject any client connections -that do not provide the same password value. -The format of this file depends on the authentication option used, -see \fIauth\fP and \fItcp\-auth\fP for details. -.TP \fB\-\-encryption\fP=\fICIPHER\fP Specifies the cipher to use for securing the connection from prying eyes. @@ -881,6 +887,12 @@ it to the server and keep it locally. .SS Options for attach .TP +\fB\-\-password\-file\fP=\fIFILENAME\fP +Supply the password to be used for connecting to a server that +uses authentication. See \fIauth\fP, \fItcp\-auth\fP and \fIvsock\-auth\fP +for details. +Alternatively, you may use the \fP=\fIXPRA_PASSWORD\fP environment variable. +.TP \fB\-\-opengl\fP=\fIyes\fP|\fIno\fP|\fIauto\fP Use OpenGL accelerated rendering on the client. The default is to detect if the graphics card and drivers are @@ -1199,11 +1211,14 @@ environment of the child to point to the xpra display. \fBxpra attach\fP, on the other hand, uses this variable to determine which display the remote applications should be shown on. +fIXPRA_PASSWORD\fP may be used with \fBxpra attach\fP instead of the +\fBpassword\-file\fP option. + .\" -------------------------------------------------------------------- .SH FILES \fIxpra.conf\fP stores default values for most options. -There is a global config file in \fI/etc\fP or \fI/usr/local/etc\fP, -and each user may override it using \fI.xpra/xpra.conf\fP. +There is a global configuration file in \fI/etc\fP or \fI/usr/local/etc\fP, +and each user may override those defaults by creating the file \fI.xpra/xpra.conf\fP. Xpra uses the directory \fI~/.xpra\fP to store a number of files. (The examples below are given for the display \fI:7\fP.) .TP diff --git a/src/tests/unit/server/auth_test.py b/src/tests/unit/server/auth_test.py index 7e290405d2..3d8aa800aa 100755 --- a/src/tests/unit/server/auth_test.py +++ b/src/tests/unit/server/auth_test.py @@ -4,6 +4,7 @@ # Xpra is released under the terms of the GNU GPL v2, or, at your option, any # later version. See the file COPYING for details. +import os import sys import unittest import tempfile @@ -14,7 +15,7 @@ from xpra.os_util import strtobytes try: - from xpra.server.auth import fail_auth, reject_auth, allow_auth, none_auth, file_auth, multifile_auth + from xpra.server.auth import fail_auth, reject_auth, allow_auth, none_auth, file_auth, multifile_auth, env_auth, password_auth except: pass @@ -26,7 +27,7 @@ def __getattr__(self, name): class TestAuth(unittest.TestCase): - def _init_auth(self, module, options={}, username="foo"): + def _init_auth(self, module, options={}, username="foo", **kwargs): opts = FakeOpts(options) module.init(opts) try: @@ -34,7 +35,7 @@ def _init_auth(self, module, options={}, username="foo"): except Exception as e: raise Exception("module %s does not contain an Authenticator class!") try: - return c(username) + return c(username, **kwargs) except Exception as e: raise Exception("failed to instantiate %s: %s" % (c, e)) @@ -47,11 +48,15 @@ def _test_module(self, module): assert challenge def test_all(self): - test_modules = [reject_auth, + test_modules = [ + reject_auth, allow_auth, none_auth, file_auth, - multifile_auth] + multifile_auth, + env_auth, + password_auth, + ] try: from xpra.server.auth import pam_auth test_modules.append(pam_auth) @@ -99,6 +104,37 @@ def test_allow(self): assert a.authenticate(x, "") assert a.authenticate("", x) + def _test_hmac_auth(self, auth_class, password, **kwargs): + for x in (password, "somethingelse"): + a = self._init_auth(auth_class, **kwargs) + assert a.requires_challenge() + assert a.get_password() + salt, mac = a.get_challenge() + assert salt + assert mac=="hmac", "invalid mac: %s" % mac + client_salt = strtobytes(uuid.uuid4().hex+uuid.uuid4().hex) + auth_salt = strtobytes(xor(salt, client_salt)) + verify = hmac.HMAC(x, auth_salt, digestmod=hashlib.md5).hexdigest() + passed = a.authenticate(verify, client_salt) + assert passed == (x==password), "expected authentication to %s with %s vs %s" % (["fail", "succeed"][x==password], x, password) + assert not a.authenticate(verify, client_salt) + + def test_env(self): + for var_name in ("XPRA_PASSWORD", "SOME_OTHER_VAR_NAME"): + password = strtobytes(uuid.uuid4().hex) + os.environ[var_name] = password + try: + kwargs = {} + if var_name!="XPRA_PASSWORD": + kwargs["name"] = var_name + self._test_hmac_auth(env_auth, password, name=var_name) + finally: + del os.environ[var_name] + + def test_password(self): + password = strtobytes(uuid.uuid4().hex) + self._test_hmac_auth(password_auth, password, value=password) + def _test_file_auth(self, name, module, genauthdata): #no file, no go: @@ -116,6 +152,7 @@ def _test_file_auth(self, name, module, genauthdata): with f: a = self._init_auth(module, {"password_file" : filename}) password, filedata = genauthdata(a) + print("saving password file data='%s' to '%s'", filedata, filename) f.write(strtobytes(filedata)) f.flush() assert a.requires_challenge() @@ -145,7 +182,7 @@ def genfiledata(a): password = uuid.uuid4().hex return password, "%s|%s|||" % (a.username, password) self._test_file_auth("multifile", multifile_auth, genfiledata) - + def main(): try: diff --git a/src/xpra/client/client_base.py b/src/xpra/client/client_base.py index 477e6b3053..3d1bc75546 100644 --- a/src/xpra/client/client_base.py +++ b/src/xpra/client/client_base.py @@ -291,11 +291,13 @@ def init_aliases(self): self._reverse_aliases[key] = i i += 1 + def has_password(self): + return self.password_file or os.environ.get('XPRA_PASSWORD') def send_hello(self, challenge_response=None, client_salt=None): try: hello = self.make_hello_base() - if self.password_file and not challenge_response: + if self.has_password() and not challenge_response: #avoid sending the full hello: tell the server we want #a packet challenge first hello["challenge"] = True @@ -311,7 +313,7 @@ def send_hello(self, challenge_response=None, client_salt=None): self.quit(EXIT_INTERNAL_ERROR) return if challenge_response: - assert self.password_file + assert self.has_password(), "got a password challenge response but we don't have a password! (malicious or broken server?)" hello["challenge_response"] = challenge_response if client_salt: hello["challenge_client_salt"] = client_salt @@ -514,7 +516,7 @@ def warn_server_and_exit(code, message, server_message="authentication failed"): authlog.error("Error: authentication failed:") authlog.error(" %s", message) self.disconnect_and_quit(code, server_message) - if not self.password_file: + if not self.has_password(): warn_server_and_exit(EXIT_PASSWORD_REQUIRED, "this server requires authentication, please provide a password", "no password available") return password = self.load_password() @@ -588,20 +590,22 @@ def set_server_encryption(self, caps, key): def get_encryption_key(self): key = load_binary_file(self.encryption_keyfile) - if key is None: + if not key: + key = os.environ.get('XPRA_ENCRYPTION_KEY') + if not key: raise InitExit(1, "no encryption key") return key.strip("\n\r") def load_password(self): if not self.password_file: - return None + return os.environ.get('XPRA_PASSWORD') filename = os.path.expanduser(self.password_file) password = load_binary_file(filename) netlog("password read from file %s is %s", self.password_file, "".join(["*" for _ in password])) return password def _process_hello(self, packet): - if not self.password_sent and self.password_file: + if not self.password_sent and self.has_password(): self.warn_and_quit(EXIT_NO_AUTHENTICATION, "the server did not request our password") return try: diff --git a/src/xpra/server/auth/env_auth.py b/src/xpra/server/auth/env_auth.py new file mode 100644 index 0000000000..0a63503122 --- /dev/null +++ b/src/xpra/server/auth/env_auth.py @@ -0,0 +1,26 @@ +# This file is part of Xpra. +# Copyright (C) 2016 Antoine Martin +# Xpra is released under the terms of the GNU GPL v2, or, at your option, any +# later version. See the file COPYING for details. + +import os +from xpra.server.auth.sys_auth_base import SysAuthenticator, init + +assert init + + +class Authenticator(SysAuthenticator): + + def __init__(self, username, **kwargs): + SysAuthenticator.__init__(self, username) + self.var_name = kwargs.get("name", "XPRA_PASSWORD") + self.authenticate = self.authenticate_hmac + + def __repr__(self): + return "env" + + def get_challenge(self): + return SysAuthenticator.get_challenge(self, mac="hmac") + + def get_password(self): + return os.environ.get(self.var_name) diff --git a/src/xpra/server/auth/file_auth_base.py b/src/xpra/server/auth/file_auth_base.py index 95d96a0ab3..0adcc2d16d 100644 --- a/src/xpra/server/auth/file_auth_base.py +++ b/src/xpra/server/auth/file_auth_base.py @@ -27,6 +27,7 @@ def __init__(self, username, **kwargs): self.password_filename = kwargs.get("filename", password_file) self.password_filedata = None self.password_filetime = None + self.authenticate = self.authenticate_hmac def requires_challenge(self): return True @@ -68,7 +69,7 @@ def load_password_file(self): self.password_filedata = self.parse_filedata(data) self.password_filetime = ptime except Exception as e: - log.error("Error password from '%s':", password_file) + log.error("Error reading password data from '%s':", self.password_filename, exc_info=True) log.error(" %s", e) self.password_filedata = None return self.password_filedata diff --git a/src/xpra/server/auth/multifile_auth.py b/src/xpra/server/auth/multifile_auth.py index 631ba337e7..d149e395aa 100644 --- a/src/xpra/server/auth/multifile_auth.py +++ b/src/xpra/server/auth/multifile_auth.py @@ -83,10 +83,13 @@ def parse_filedata(self, data): log.error(" '%s'", line) log.error(" %s", e) continue - log("parsed auth data from file %s: %s", password_file, auth_data) + log("parsed auth data from file %s: %s", self.password_filename, auth_data) return auth_data def get_auth_info(self): + self.load_password_file() + if not self.password_filedata: + return None return self.password_filedata.get(strtobytes(self.username)) def get_password(self): diff --git a/src/xpra/server/auth/password_auth.py b/src/xpra/server/auth/password_auth.py new file mode 100644 index 0000000000..2580f7fe5f --- /dev/null +++ b/src/xpra/server/auth/password_auth.py @@ -0,0 +1,26 @@ +# This file is part of Xpra. +# Copyright (C) 2016 Antoine Martin +# Xpra is released under the terms of the GNU GPL v2, or, at your option, any +# later version. See the file COPYING for details. + +from xpra.server.auth.sys_auth_base import SysAuthenticator, init + +assert init + + +class Authenticator(SysAuthenticator): + + def __init__(self, username, **kwargs): + print("kwargs=%s" % (kwargs, )) + SysAuthenticator.__init__(self, username) + self.value = kwargs.get("value") + self.authenticate = self.authenticate_hmac + + def __repr__(self): + return "password" + + def get_challenge(self): + return SysAuthenticator.get_challenge(self, mac="hmac") + + def get_password(self): + return self.value diff --git a/src/xpra/server/auth/sys_auth_base.py b/src/xpra/server/auth/sys_auth_base.py index 2ecef00749..db650a682b 100644 --- a/src/xpra/server/auth/sys_auth_base.py +++ b/src/xpra/server/auth/sys_auth_base.py @@ -3,9 +3,11 @@ # Xpra is released under the terms of the GNU GPL v2, or, at your option, any # later version. See the file COPYING for details. +import hmac, hashlib, binascii + from xpra.platform.dotxpra import DotXpra from xpra.util import xor -from xpra.os_util import get_hex_uuid +from xpra.os_util import get_hex_uuid, strtobytes from xpra.log import Logger log = Logger("auth") @@ -44,13 +46,13 @@ def get_gid(self): def requires_challenge(self): return True - def get_challenge(self): + def get_challenge(self, mac="xor"): if self.salt is not None: log.error("challenge already sent!") return None self.salt = get_hex_uuid()+get_hex_uuid() #we need the raw password, so tell the client to use "xor": - return self.salt, "xor" + return self.salt, mac def get_password(self): return None @@ -59,8 +61,12 @@ def check(self, password): raise NotImplementedError() def authenticate(self, challenge_response, client_salt): + #this will call check(password) + self.authenticate_check(challenge_response, client_salt) + + def authenticate_check(self, challenge_response, client_salt): if self.salt is None: - log.error("got a challenge response with no salt!") + log.error("Error: illegal challenge response received - salt cleared or unset") return False if client_salt is None: salt = self.salt @@ -75,7 +81,35 @@ def authenticate(self, challenge_response, client_salt): if not self.check(password): return False except Exception as e: - log.error("authentication error: %s", e) + log.error("Error in %s authentication checks:", self) + log.error(" %s", e) + return False + return True + + def authenticate_hmac(self, challenge_response, client_salt): + if not self.salt: + log.error("Error: illegal challenge response received - salt cleared or unset") + return None + #ensure this salt does not get re-used: + if client_salt is None: + salt = self.salt + else: + salt = xor(self.salt, client_salt) + self.salt = None + password = self.get_password() + if not password: + log.error("Error: %s authentication failed", self) + log.error(" no password defined for '%s'", self.username) + return False + verify = hmac.HMAC(strtobytes(password), strtobytes(salt), digestmod=hashlib.md5).hexdigest() + log("authenticate(%s) password=%s, hex(salt)=%s, hash=%s", challenge_response, password, binascii.hexlify(strtobytes(salt)), verify) + if hasattr(hmac, "compare_digest"): + eq = hmac.compare_digest(verify, challenge_response) + else: + eq = verify==challenge_response + if not eq: + log("expected '%s' but got '%s'", verify, challenge_response) + log.error("Error: hmac password challenge for '%s' does not match", self.username) return False return True diff --git a/src/xpra/server/server_core.py b/src/xpra/server/server_core.py index 8a597e7e36..182c52587b 100644 --- a/src/xpra/server/server_core.py +++ b/src/xpra/server/server_core.py @@ -225,12 +225,14 @@ def get_auth_module(self, socket_type, auth_str, opts): else: auth = "pam" authlog("will try to use sys auth module '%s' for %s", auth, sys.platform) - from xpra.server.auth import fail_auth, reject_auth, allow_auth, none_auth, file_auth, multifile_auth + from xpra.server.auth import fail_auth, reject_auth, allow_auth, none_auth, file_auth, multifile_auth, password_auth, env_auth AUTH_MODULES = { "fail" : fail_auth, "reject" : reject_auth, "allow" : allow_auth, "none" : none_auth, + "env" : env_auth, + "password" : password_auth, "multifile" : multifile_auth, "file" : file_auth, } @@ -805,10 +807,15 @@ def filedata_nocrlf(self, filename): def get_encryption_key(self, authenticator=None, keyfile=None): #if we have a keyfile specified, use that: authlog("get_encryption_key(%s, %s)", authenticator, keyfile) + v = None if keyfile: authlog("loading encryption key from keyfile: %s", keyfile) - return self.filedata_nocrlf(keyfile) - return None + v = self.filedata_nocrlf(keyfile) + if not v: + v = os.environ.get('XPRA_ENCRYPTION_KEY') + if v: + authlog("using encryption key from %s environment variable", 'XPRA_ENCRYPTION_KEY') + return v def hello_oked(self, proto, packet, c, auth_caps): pass