Bug 49929 - UMC creates unnecessary amounts of sessions
UMC creates unnecessary amounts of sessions
Status: CLOSED FIXED
Product: UCS
Classification: Unclassified
Component: UMC (Generic)
UCS 4.4
Other Linux
: P5 normal (vote)
: UCS 4.4-1-errata
Assigned To: Florian Best
Jürn Brodersen
:
Depends on: 47106
Blocks: 48002
  Show dependency treegraph
 
Reported: 2019-07-29 11:18 CEST by Jürn Brodersen
Modified: 2019-07-31 13:58 CEST (History)
1 user (show)

See Also:
What kind of report is it?: Bug Report
What type of bug is this?: 4: Minor Usability: Impairs usability in secondary scenarios
Who will be affected by this bug?: 5: Will affect all installed domains
How will those affected feel about the bug?: 2: A Pain – users won’t like this once they notice it
User Pain: 0.229
Enterprise Customer affected?:
School Customer affected?:
ISV affected?:
Waiting Support:
Flags outvoted (downgraded) after PO Review:
Ticket number:
Bug group (optional):
Max CVSS v3 score:


Attachments
possible patch (2.89 KB, patch)
2019-07-29 11:18 CEST, Jürn Brodersen
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jürn Brodersen univentionstaff 2019-07-29 11:18:10 CEST
Created attachment 10135 [details]
possible patch

UMC creates unnecessary amounts of sessions

What is the Problem:
A new session means a new module process is created which takes time to start, needs memory and the umc server only connects to the latest process of a module, which can result in a state loss (especially for progress calls).

This might also be the reason for some of the random "Forbidden" and cross site scripting warnings.

How to reproduce:
"ucr set umc/module/timeout=30" for convenience
Open the diagnostic module
"tail -f /var/log/univention/management-console-server.log"
wait for "MAIN        ( PROCESS ) : Processor: dying" in the log
In the diagnostics module click the button to run the diagnostic again.
You should now see multiple umc module processes for the diagnostic module


The attached patch locks the auth request because I think I had problems with bug 47106. It also uses get_sessionid instead of create_sessionid to avoid multiple sessions. The basic problem seems to be that a new session_id is created by the create_sessionid if no session is found for the sessionid in the cookie, but the cookie is not updated immediately which can result in a new session again.
I'm not sure about possible side effects because I don't really know why create_sessionid was used in the first place.
Comment 1 Florian Best univentionstaff 2019-07-29 11:52:36 CEST
(In reply to Jürn Brodersen from comment #0)
> Created attachment 10135 [details]
> possible patch
> 
> UMC creates unnecessary amounts of sessions
> 
> What is the Problem:
> A new session means a new module process is created which takes time to
> start, needs memory and the umc server only connects to the latest process
> of a module, which can result in a state loss (especially for progress
> calls).
More precisely: A new session (in the UMC-Web-Server) means one new connection to the UMC-Server. That connection may have multiple singleton-module-processes.
Therefore the loss of a session means state loss and breaks e.g. progress calls.
So we have to ensure that every request in the same UMC session/tab/browser window uses the same connection to the UMC-Server.

> This might also be the reason for some of the random "Forbidden" and cross
> site scripting warnings.
Would be nice, if we find the reason for this :-)

> How to reproduce:
> "ucr set umc/module/timeout=30" for convenience
> Open the diagnostic module
> "tail -f /var/log/univention/management-console-server.log"
> wait for "MAIN        ( PROCESS ) : Processor: dying" in the log
> In the diagnostics module click the button to run the diagnostic again.
> You should now see multiple umc module processes for the diagnostic module
"Processor: dying" means that the module process has ended after $module_timeout seconds of inactivity (with no open request for that module process).

> The attached patch locks the auth request because I think I had problems
> with bug 47106. It also uses get_sessionid instead of create_sessionid to
> avoid multiple sessions. The basic problem seems to be that a new session_id
> is created by the create_sessionid if no session is found for the sessionid
> in the cookie, but the cookie is not updated immediately which can result in
> a new session again.
> I'm not sure about possible side effects because I don't really know why
> create_sessionid was used in the first place.

It is a security risk to use a cookie value which is generated by the client, not by the server.
Therefore we must use create_session(). If there are multiple requests in the queue they have to wait for the answer of the authentication request and use that session then.

I was currently searching for the cause in the UMC-Server, but your idea is an interesting approach, thanks for that analysis so far.
Comment 2 Florian Best univentionstaff 2019-07-29 12:54:19 CEST
I think this patch is enough:

diff --git a/management/univention-management-console/univention-management-console-web-server b/management/univention-management-console/univention-management-console-web-server
index f3b25c92f3..1806675569 100755
--- a/management/univention-management-console/univention-management-console-web-server
+++ b/management/univention-management-console/univention-management-console-web-server
@@ -616,12 +616,16 @@ class Ressource(object):
                        # we must not change the session ID cookie as this might cause
                        # race conditions in the frontend during login, especially when logged in via SAML
                        return self.get_session_id()
+               user = self.get_user()
+               if user:  # TODO:? and user.time_remaining > 1:
+                       return user.sessionid
                if random:
                        return str(uuid.uuid4())
                return sessionidhash()

        def get_session_id(self):
                """get the current session ID from cookie."""
+               # caution: use this wisely! do not create a new session with this ID (value coming from the Client)!
                return self.get_cookie('UMCSessionId') or sessionidhash()

        def get_session(self):
Comment 3 Florian Best univentionstaff 2019-07-29 14:36:18 CEST
Fixed :-)
If there was already a session, we use the same sesssionid again :-) This is done already in a similar manner if there is another module process running. This is a second fallack for the case where all module processes (i.e. the connection to the UMC-Server) are ended.

univention-management-console.yaml
c2956332660f | Bug #49929: do not create new session ids after module process dies

univention-management-console (11.0.4-31)
c2956332660f | Bug #49929: do not create new session ids after module process dies
Comment 4 Jürn Brodersen univentionstaff 2019-07-29 18:00:38 CEST
I really like the fix :)

But now bug 47106 is triggered... :(
Meaning some request are never answered (in my case the first five)


I see two possible solutions:
fix bug 47106
add a lock around the auth request

(I prefer fixing 47106...)
Comment 5 Jürn Brodersen univentionstaff 2019-07-31 12:10:27 CEST
Set to resolved again because bug 47106 is fixed
Comment 6 Jürn Brodersen univentionstaff 2019-07-31 12:15:08 CEST
What I tested:
Login with saml -> OK
Login without saml -> OK
@require_password decorator with saml -> OK
Logout with saml -> OK
Logout without saml -> OK
Password change -> OK
Can't reproduce comment 1 any more -> OK
As fa as I can tell the UMC Session ID only changes on a logout -> OK

yaml -> OK
Comment 7 Arvid Requate univentionstaff 2019-07-31 13:58:47 CEST
<http://errata.software-univention.de/ucs/4.4/203.html>