Privileged access subsystem #393

whisperity · 2016-10-03T16:56:07Z

This merge requests contains the privileged access infrastructure — closes #339.

If a user is authenticated (via any means described below), full access to the server is given: they can view results and destroy them. If the user is not authenticated, the entire access is denied – there are no further fine-graining, as per discussed with @dkrupp and @gyorb.

The changes — summed up

A new Thrift endpoint at /Authentication was implemented, this handles authentication requests
Authentication can happen against a username-password storage, an LDAP server or the local PAM chain.
Existing interface was not changed, though an invalid session, on a privileged server, returns a HTTP error code which can make the CodeChecker cmd client fail on an exception — clients are expected to handle authentication before live request.
Two new configuration files, config/session_config.json and config/session_client.json. Both are created as working and descriptive templates.
- The first one is the server's configuration. It is copied to WORKSPACE/session_config.json for every server started and contains the authentication setup.
- The latter one gets copied for every user into their ~/.codechecker_passwords.json as it contains per-user credentials.
Web-browser client authenticates via basic pop-ups (HTTP WWW-Authenticate header)
CodeChecker cmd got a new subcommand: login. With this, users can authenticate on a server and kill their session.
- The ~/.codechecker_passwords.json file can contain preconfigured credentials — if they are valid, executing a live query without a session will automatically log the user in for screen security and convenience.
- Session information for a user is stored in a temporary file, identified on the system with the running user's name.

Full documentation for public use is available in docs/authentication.md.

Xazax-hun · 2016-10-03T18:36:51Z

Did you consider storing client side information in the designated codechecker workspace? Does it make sense to have a global one?

gyorb · 2016-10-04T08:27:51Z

If a user wants only to check the results and use only the command line no workspace is required. It is possible to store multiple server authentication credentials in one file.
By default a global one is created but the user should be able to use authentication files from different places if needed.

Xazax-hun · 2016-10-03T18:39:57Z

README.md

@@ -40,7 +40,7 @@ Tested on Ubuntu LTS 14.04.2

 # get ubuntu packages
 # clang-3.6 can be replaced by any later versions of clang
-sudo apt-get install clang-3.6 doxygen build-essential thrift-compiler python-virtualenv gcc-multilib git wget
+sudo apt-get install clang-3.6 doxygen build-essential thrift-compiler python-virtualenv gcc-multilib git wget libldap2-dev libsasl2-dev libssl-dev



Is ldap a strong dependency or only an optional one? I think in case this is optional (it should be IMO), lets have a separate list with optional dependencies.

Agreed. CQ @gyorb @dkrupp on what the best way to support this through the automatic build chain (maybe a --with-ldap option in build_package.py?) would be.

I think installing these extra requirements could go into the separate authentication documentation and start that if the user requires authentication he should install these extra packages and python modules.

Xazax-hun · 2016-10-03T18:43:18Z

build_package.py

+    ret = run_cmd(auth_cmd, thrift_files_dir, env, silent=silent)
+    if ret:
+        LOG.error('Failed to generate authentication interface files')
+        return ret


There seems to be some repetition. Does a local function make this code shorter overall?

So far we generated the source files with Thrift files differently for the report server (python only) and the report viewer (python and javascript). The authentication is python only which could be refactored into a separate function with the report server source file generation. I'm not sure it will be so much shorter, right now the generate_thrift_files function is not that long. If you think it is better you can refactor it into a separate function.

Xazax-hun · 2016-10-03T18:47:44Z

codechecker_lib/session_manager.py

+# ------------------------------
+# ----------- SERVER -----------
+class _Session():
+    '''A session for an authenticated, privileged client connection'''


End comments with periods.

Xazax-hun · 2016-10-03T18:50:01Z

codechecker_lib/session_manager.py

+        persistency hash is intended to be used for the "session recycle"
+        feature to prevent NAT endpoints from accidentally getting each
+        other's session.'''
+        return hashlib.sha256(auth_string + "@" + client_addr + ":" +


Is the port included in the addr? Can multiple sessions from the same client interfere?

Xazax-hun · 2016-10-03T18:51:08Z

codechecker_lib/session_manager.py

+            self.last_access = datetime.now()
+
+
+class sessionManager:


Does this start with a lowercase letter on purpose?

Xazax-hun · 2016-10-04T11:59:37Z

codechecker_lib/session_manager.py

+        ret = self.__save["credentials"].get(host + ":" + port)
+        if not ret:
+            ret = self.__save["credentials"].get(host)
+            if not ret:


I think you do not need all the indentation.

Xazax-hun · 2016-10-04T12:00:00Z

codechecker_lib/session_manager.py

+        return ret
+
+    def saveToken(self, host, port, token, destroy=False):
+        if not destroy:


Reverse the branches and remove the not.

Xazax-hun · 2016-10-04T12:25:02Z

thrift_api/authentication.thrift

+
+  // handles creating a session token for the user
+  string performLogin(1: string auth_method,
+                      2: string auth_string)


Is it good practice to have one auth string instead of separate user/password etc?

It was done this way to make sure that further extensions in auth methods (such as, e.g. SSH key based one) is most easily done without further breaking the Authentication API.

Xazax-hun · 2016-10-04T12:25:10Z

viewer_clients/cmdline_client/authentication_helper.py

+            # ------------------------------------------------------------
+
+    def ThriftClientCall(function):
+        # print type(function)


Commented code.

Xazax-hun · 2016-10-04T12:27:23Z

tests/test_utils/thrift_client_to_db.py

        # import only if necessary; some tests may not add this to PYTHONPATH
+        from codechecker_lib import session_manager
        from codeCheckerDBAccess import codeCheckerDBAccess
        from codeCheckerDBAccess.constants import MAX_QUERY_SIZE

        self.max_query_size = MAX_QUERY_SIZE
        transport = THttpClient.THttpClient(host, port, uri)


Is the login done through https?

No. (Docs will be updated.)

whisperity · 2016-10-04T12:57:59Z

To-Do:

Session tokens in separate files, proper locking
Server configuration into workspace
Warn that credentials travel without any encryption

gyorb · 2016-10-05T08:54:41Z

docs/authentication.md

@@ -0,0 +1,145 @@
+CodeChecker authentication subsytem
+===================================
+


A new additional package dependencies section could be created here, what packages should be installed by the user.

gyorb · 2016-10-05T08:56:40Z

tests/functional/package_test/__init__.py

+    #
+    # Create a dummy authentication-enabled configuration and an auth-enabled server
+    session_cfg_file = os.path.join(pkg_root, "config", "session_config.json")
+    with open(session_cfg_file, 'r') as scfg:


Could we open the file only once? (read up the config modify it and write out)

gyorb · 2016-10-05T09:10:07Z

codechecker_lib/session_manager.py

+import json
+import hashlib
+import time
+import ldap


importing ldap modules could be done optionally and if importing fails only the ldap authentication method will be disabled.

try: import ldap import ldap.sasl except ImportError: # no ldap authentication is available

gyorb · 2016-10-05T11:52:05Z

docs/authentication.md

+
+The `CodeChecker cmd` client needs to be authenticated for a server before any data communication could take place.
+
+The client's configuration file is expected to be at `~/.codechecker_passwords.json`, which is created at the first command executed


Please extend the documentation that the user should modify the access rights to the password file so only he can access it: chmod 0600 ~/.codechecker_password.json

😏 Better idea is to create the file initially as 0600. Though I added it to the documentation and a warning (in ssh style) for the user if the file is with the wrong permissions.

gyorb · 2016-10-14T15:52:34Z

Please update your branch, so all the tests can run on Linux and on OSX too. The CI is fixed now. Thanks!

whisperity · 2016-10-15T22:48:21Z

Should I cherry-pick the test fixing changes or should I do a full rebase? With this many commits in the history – and the reviews – I would vote for cherry picking.

Xazax-hun · 2016-10-16T07:43:05Z

I think rebasing is usually a cleaner solution for the history's sake.

gyorb

Are there any commits you could squash?

gyorb · 2016-10-19T08:30:47Z

codechecker_lib/session_manager.py

-        with open(session_cfg_file, 'w') as scfg:
-            json.dump(self.__save, scfg, indent=2, sort_keys=True)
+        with open(self.token_file, 'w') as scfg:
+            fcntl.lockf(scfg, fcntl.LOCK_EX)


Is this way of locking unix specific? Will it work on OSX and maybe later on Windows?

It is. It will work on OS X but will definitely not work on Windows, the filesystem concept of the two system families are entirely different.

Maybe we could use the package portalocker? It is essentially just a branching wrapper, but it looks like it could work good enough.

I think it would be nice to use a platform independent solution. portalocker seems to be a nice choice.

whisperity · 2016-10-19T11:08:33Z

As for squashing: not really without messing up order (I have done some reordering in this last rebase.)

I could squash the first two commits as they are one unit, I could also reorder the "update due to reviews" stuff and put it with the "misc." changes as one commit at the end, though. But putting any more commits together would create a big list of unrelated stuff in one commit...

gyorb · 2016-10-19T16:01:00Z

Let's squash that few commits you mentioned and keep the rest.

- Added a new Thrift service for handling authentication requests (not yet documented) - Added proper HTTP responses for the browser-based auth realm to work - Added wrapper for authentication requests on the command-line client - Implemented persistency for sessions (server) and valid tokens (client)

…n is found

…ehind a NAT

…y-based authentication

… logins to prevent memory leaks

…h grace

- User credential file is chmod 0600 at creation (if already exists and not this mode, the user is warned) - Active tokens are stored away from the credentials, in the /tmp folder - Server configuration is put into the workspace where the server is running instead of using an install-global file

whisperity force-pushed the authentications branch from 73c3d41 to 640d054 Compare October 3, 2016 17:23

Xazax-hun suggested changes Oct 4, 2016

View reviewed changes

whisperity added a commit to whisperity/CodeChecker that referenced this pull request Oct 4, 2016

Update comments and naming conventions as per review on Ericsson#393

a46be1b

Xazax-hun approved these changes Oct 4, 2016

View reviewed changes

gyorb reviewed Oct 5, 2016

View reviewed changes

whisperity changed the title ~~Privileged access system & LDAP authentication (#339)~~ Privileged access subsystem Oct 5, 2016

gyorb reviewed Oct 5, 2016

View reviewed changes

whisperity mentioned this pull request Oct 10, 2016

Add digest access authentication support #336

Closed

3 tasks

gyorb modified the milestone: release 5.7 Oct 11, 2016

whisperity force-pushed the authentications branch from 037f0c9 to 6e50896 Compare October 16, 2016 09:14

whisperity added a commit to whisperity/CodeChecker that referenced this pull request Oct 16, 2016

Update comments and naming conventions as per review on Ericsson#393

fb86530

whisperity force-pushed the authentications branch 2 times, most recently from 66d4102 to a731109 Compare October 18, 2016 14:30

whisperity added a commit to whisperity/CodeChecker that referenced this pull request Oct 18, 2016

Update comments and naming conventions as per review on Ericsson#393

adb9ebd

gyorb suggested changes Oct 19, 2016

View reviewed changes

whisperity added 2 commits October 20, 2016 15:22

Clientside preconfigured credentials and automatic login if no sessio…

7be0cf2

…n is found

whisperity added 11 commits October 20, 2016 15:23

Better authentication with less config reads and session storage

24f05d9

Increase security by preventing session bleedover with clients from b…

6b76a60

…ehind a NAT

Move configuration file to user's HOME folder and implement dictionar…

b86f406

…y-based authentication

Configurable session soft and hard timeout, and prune session after N…

b51c37e

… logins to prevent memory leaks

Made code pep8-compliant

cbef8cf

LDAP authentication and configuration

3d79813

Tests for privileged access

2781215

Make authentication dependencies optional and handle missing ones wit…

52b5a81

…h grace

PAM authentication module for privileged access

550d210

Moved authentication modules to a different .ci file and use portalocker

3d6f5b9

whisperity force-pushed the authentications branch from a731109 to 3d6f5b9 Compare October 20, 2016 13:47

gyorb approved these changes Oct 20, 2016

View reviewed changes

gyorb merged commit 3fc18a9 into Ericsson:master Oct 20, 2016

whisperity deleted the authentications branch October 20, 2016 15:49

whisperity restored the authentications branch December 23, 2016 13:34

whisperity deleted the authentications branch March 10, 2017 16:49

dkrupp mentioned this pull request Jul 13, 2017

Multiple products on the same server (multi-database system) [142h] #691

Closed

29 tasks

whisperity mentioned this pull request Apr 26, 2020

Implement requiring authentication to access a server Ericsson/CodeCompass#386

Merged

whisperity mentioned this pull request Jul 18, 2024

refactor(server/config): Implement checked access to server configuration, decoupled from SessionManager #4170

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Privileged access subsystem #393

Privileged access subsystem #393

whisperity commented Oct 3, 2016 •

edited

Loading

Xazax-hun commented Oct 3, 2016

gyorb commented Oct 4, 2016

Xazax-hun Oct 3, 2016

whisperity Oct 4, 2016

gyorb Oct 5, 2016

Xazax-hun Oct 3, 2016

gyorb Oct 5, 2016

Xazax-hun Oct 3, 2016

Xazax-hun Oct 3, 2016

Xazax-hun Oct 3, 2016

Xazax-hun Oct 4, 2016

Xazax-hun Oct 4, 2016

Xazax-hun Oct 4, 2016

whisperity Oct 4, 2016

Xazax-hun Oct 4, 2016

Xazax-hun Oct 4, 2016

whisperity Oct 4, 2016

whisperity commented Oct 4, 2016 •

edited

Loading

gyorb Oct 5, 2016

gyorb Oct 5, 2016

gyorb Oct 5, 2016

gyorb Oct 5, 2016

whisperity Oct 5, 2016

gyorb commented Oct 14, 2016

whisperity commented Oct 15, 2016

Xazax-hun commented Oct 16, 2016

gyorb left a comment

gyorb Oct 19, 2016

whisperity Oct 19, 2016

gyorb Oct 19, 2016

whisperity commented Oct 19, 2016

gyorb commented Oct 19, 2016

		@@ -0,0 +1,145 @@
		CodeChecker authentication subsytem
		===================================


		The `CodeChecker cmd` client needs to be authenticated for a server before any data communication could take place.

		The client's configuration file is expected to be at `~/.codechecker_passwords.json`, which is created at the first command executed

Privileged access subsystem #393

Privileged access subsystem #393

Conversation

whisperity commented Oct 3, 2016 • edited Loading

The changes — summed up

Xazax-hun commented Oct 3, 2016

gyorb commented Oct 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whisperity commented Oct 4, 2016 • edited Loading

To-Do:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gyorb commented Oct 14, 2016

whisperity commented Oct 15, 2016

Xazax-hun commented Oct 16, 2016

gyorb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whisperity commented Oct 19, 2016

gyorb commented Oct 19, 2016

whisperity commented Oct 3, 2016 •

edited

Loading

whisperity commented Oct 4, 2016 •

edited

Loading