[IMP] core: add session deletion mechanism

[thle-odoo]

Objective:

A user must be able to see which of his sessions/devices are active. Make it easy to block devices (individually or by group) and analyse current sessions. If a user notices unusual operations concerning him on another device, he must be able to stop these operations by blocking the session used by this device.

General:

• A device is identified by a static part, the device's fingerprint (the user agent).
• A device is tracked dynamically via its IP address and the time representing its last activity.
• A device is always linked to one and only one session.
• A session is linked to at least one device.

If the fingerprint or IP address of a session is modified, we assume that a new device is detected, so it will be created.

The same device is updated every X seconds in order to track its use in terms of duration of activity.

The _update_device method is placed at the point where we want to collect information and check whether we need to create a new device.

The location is important because it has to be able to intercept requests from scripts, for example, so as not to establish 'false' security.

Blocking sessions:

Blocking a session by selecting a device must block the session directly. From an administrator's point of view, if we want to block a session, we expect the session file to be deleted directly on the filesystem. If the session file is no longer present on the filesystem, we can be sure that there is no longer any risk of session usurpation.

Note:
There are several ways of blocking the session, but this is the safest. Blocking a session based on DB values does not cover scenarios in which we perform a backup, for example.

Find the session file:

To find a session file on the filesystem, we need to know the sid of the session (as this is its filename). We don't want to store the sid in the database.
However, we can store a part of it with:

• a large enough part to be certain of the uniqueness of the session;
• a small enough part that we cannot brute force the end of the sid.

Browsing the filesystem has a certain performance cost (and can therefore have defects if abused).

The proposed solution is to change the granularity of the way we store the session on the filesystem. This means finding a compromise between sub-folders and files per sub-folder to browse for a session file.

The sid will be Base64 encoded in order to increase the number of sub-folders (64^2 instead of 16^2).

Delete devices:

The record representing a device is deleted via a garbage collector.

Records for which the last activity is over a week old will be deleted. This allows us to keep recent traces of the device as well as its duration of activity (because we keep the first activity time).

Task:3627898

[robodoo]

Pull request status dashboard.

[beledouxdenis]

That's UX detail, but I wonder if I wouldn't put a Kanban view by default instead of a tree view to make it beautifuler.

To have a similar view than discord:

• A title with the OS / Browser
• A subtitle with the location and last active time
• A X button to delete the device
• A kind of tag to show the current device
  

[beledouxdenis]

I have a doubt about the UX here

• At the very minimum, I think the connected devices should be before the Log out from all devices
• But then, I am not sure it should be a button but more a link. I think, in the UX guidelines, a button is associated to an "action" to perform, while the "Connected devices" is more a link / menu /navigation to the list of connected devices.
• But at the same time it should be uniform with the rest of the form, and there is no such thing up to now as a link to another view in this profile view. For instance, API keys are listed directly into that form rather than redirecting to another view/menu.

If I take the lead of Discord, they split the "Account Security" page from the "Devices" Page:

This page contains the change password, enable TOTPs, and register a security key buttons. In the end, it's pretty similar to us. Especially with Bram's passkey feature with add the "Add a passkey" button:

And then the "Devices" page contains exclusively the connected devices.
Maybe we should simply add a new tab "Devices" on the user profile, with an embedded kanban view as stated in my previous comment, with the list of devices directly available from there.

But, performances must be considered, according if always retrieving the devices list is costly or not, because they will be retrieved each time the profile will be opened, whether or not the user opens the tab "Devices". Clearly, in Discord, the devices list is loaded only when you click the "Devices" menu, not before. But this is not how our framework works currently.

To be discussed with @odony

[beledouxdenis]

I am following the comment Antony said during the last meeting, that this should placed where we get the session from the filestore.

We get the session from the session store there:

odoo/odoo/http.py

Line 1413 in c308051

session = root.session_store.get(sid)

But, at that point, we do not have yet an env nor a cr nor even a db.
So, this might be too early.

Then, If I am following the callstack, my goal being to find the first place where we have an env or cr after that point:

odoo/odoo/http.py

Lines 1393 to 1394 in c308051

	def _post_init(self):
	self.session, self.db = self._get_session_and_dbname()

odoo/odoo/http.py

Line 2256 in c308051

request._post_init()

odoo/odoo/http.py

Line 2264 in c308051

response = request._serve_db()

odoo/odoo/http.py

Line 1808 in c308051

self.env = odoo.api.Environment(cr, self.session.uid, self.session.context)

The first cursor and env created after retrieving the session are in _serve_db. This is by the way the first place we can access self.registry['ir.http'].
So, a first good place could be there, just after the creation of the env and its assignation in self.env = ....
However, if I am following the stack a bit, _serve_db calls _serve_ir_http:

odoo/odoo/http.py

Line 1838 in c308051

return self._serve_ir_http(rule, args)

And then we land in the place where you currently put self.env['res.users.device']._update_device().
So, it's already a quite good place, enough early, in my opinion.

But, in the very beginning of _serve_ir_http, the first call is self.registry['ir.http']._authenticate(rule.endpoint)

odoo/odoo/http.py

Line 1843 in c308051

self.registry['ir.http']._authenticate(rule.endpoint)

Which is in charge to check the session:

odoo/odoo/addons/base/models/ir_http.py

Lines 154 to 167 in c308051

	def _authenticate(cls, endpoint):
	auth = 'none' if http.is_cors_preflight(request, endpoint) else endpoint.routing['auth']
	try:
	if request.session.uid is not None:
	if not security.check_session(request.session, request.env):
	request.session.logout(keep_db=True)
	request.env = api.Environment(request.env.cr, None, request.session.context)
	getattr(cls, f'_auth_method_{auth}')()
	except (AccessDenied, http.SessionExpiredException, werkzeug.exceptions.HTTPException):
	raise
	except Exception:
	_logger.info("Exception during request Authentication.", exc_info=True)
	raise AccessDenied()

In addition, there is that condition if request.session.uid is not None:,
which has most-likely something to do with the fact we don't want to create session in the filestore if they hold only the default values. Something we need to pay attention as well, to not suddenly screw the work done and saving more session than before with this feature. So placing this update_device after that if seems a good bet.

So, ir.http._authenticate looks as well to be a very good candidate to welcome _update_device, as it's even earlier then where you put it currently, plus it's already in charge to check the session, which is related to this _update_device.

But then, I see also see this security.check_session, which I was thinking could also be a good candidate.

Then, from the very beginning of my above investigation, I was wondering how things were working with the websocket ? Like, I my session is stolen and is being used exclusively used through the websocket server and not through the HTTP server, I would like to know it as well. So that _update_device should be either placed in a common place for the HTTP server and the Websocket server, or duplicated in both place.
And up to now, I didn't see a common method between the HTTP server and Websocket server.
Like _serve_db, _serve_ir_http , ir.http._authenticate, neither are called in the Websocket server implementation.
So, I was looking how the websocket server was working: Where does it get the session, choose the db, create the env, authenticate the user, ...:

odoo/addons/bus/websocket.py

Lines 788 to 789 in c308051

	def _get_session(self):
	session = root.session_store.get(self.ws._session.sid)

odoo/addons/bus/websocket.py

Lines 775 to 782 in c308051

	def _serve_ir_websocket(self, event_name, data):
	"""
	Delegate most of the processing to the ir.websocket model
	which is extensible by applications. Directly call the
	appropriate ir.websocket method since only two events are
	tolerated: `subscribe` and `update_presence`.
	"""
	self.env['ir.websocket']._authenticate()

And then like the messiah, I see this check_session appearing:

odoo/addons/bus/models/ir_websocket.py

Lines 57 to 59 in c308051

	def _authenticate(cls):
	if wsrequest.session.uid is not None:
	if not security.check_session(wsrequest.session, wsrequest.env):

So, the earliest common place between the HTTP server and the Websocket server with an env/cursor already created is that method security.check_session:

odoo/odoo/service/security.py

Lines 15 to 20 in c308051

	def check_session(session, env):
	self = env['res.users'].browse(session.uid)
	expected = self._compute_session_token(session.sid)
	if expected and odoo.tools.misc.consteq(expected, session.session_token):
	return True
	return False

So, it looks like a pretty good candidate to put that update_device as well.

Now, comes in the read-only routes... I have no clue how to solve this one or what we decided.
Because if an attacker steal my session and uses it exclusively on read-only routes, I would like to know it as well...
That can be for later, I guess.

Maybe as a first step, try to place this _update_device in either check_session, either in both ir.http._authenticate and ir.websocket._authenticate, and see if everything still work.

[beledouxdenis]

If possible I would avoid making these changes here but rather make them in our overridden version here:

odoo/odoo/http.py

Line 903 in c308051

class FilesystemSessionStore(sessions.FilesystemSessionStore):

For the change you do on generate_key which is not on the class but a standalone method, it looks like the class SessionStore has its own generate_key method, which you can then override in our own class FilesystemSessionStore.

[beledouxdenis]

Suggested change

	key = sha1(b"".join([salt, str(time()).encode("ascii"), os.urandom(30)])).hexdigest()
	return base64.urlsafe_b64encode(sha512(str(time()).encode() + os.urandom(64)).digest())[:-2]

• salt is useless. When checking the calls to generate_key we never pass salt. And when salt is None, it does salt = repr(salt).encode("ascii"), which always returns b'None'. Useless. No entropy added at all, this code regarding salt makes no sense.
• sha512 for higher entropy, returns 64 bytes always
• os.urandom(64) to have an entropy input of 64 bytes to match the entropy of sha512
• [:2] to remove the padding == of base64 . To have no padding you would need a multiple of 3, while sha512 returns 64 bytes. So either we remove 1 bytes from what is returned by sha512, to have 63 bytes, which is a multiple of 3, and then no padding to base64. Or we keep the 64 bytes returned by sha512 (to keep the entropy) and remove the padding ==.

[beledouxdenis]

Suggested change

	domain=[('session_identifier', '=', request.session.sid[:10])],
	domain=[('session_identifier', '=', request.session.sid[:42])],

10 seems a bit light regarding the risk of collision. The risk to delete the session of another user having the same 10 first bytes while deleting yours.
With 10 bytes of a base64, if the input is perfect (which is not the case currently with a hexdigest of sha1, which provide 40 characters of 16 possibilities), you have 1 chance on (64 ** 10), so 1 chance on 1152921504606846976 to have someone with the same 10 first character. Ok it's already huge maybe we exaggerate a bit.
An entropy of math.log(64 ** 10) = 41.58883083359672 .

But, in the current case, it's a base64 of a sha1 hexdigest. Hexadecimal it's only 16 possibilities.
A practical example Olivier did to showcase the issue:

In [127]: len(set(base64.urlsafe_b64encode(hashlib.sha1(os.urandom(30)).digest())[:10] for _ in range(40_000_000)))
Out[127]: 38457094

Out of 40.000.000 of generated session ID, 1542906 collided.
4% of collision

vs using digest instead of hexdigest

In [129]: len(set(base64.urlsafe_b64encode(hashlib.sha1(os.urandom(30)).digest())[:10] for _ in range(40_000_000)))
Out[129]: 40000000

With the above change in generate_key, using a base64 of a sha512 with os.urandom(64), it returns 86 bytes. If we take half for the identifier, it would be [:43]. But then Olivier and I feel that 42 is more appealing :D. Hence the 42, the answer to life the universe and everything.

And then the entropy will be way much higher:

In [27]: math.log(64 ** 42)
Out[27]: 174.6730895011062

[beledouxdenis]

Why an array for the sync ? Can't be a simple boolean ?

[beledouxdenis]

Suggested change

	<field name="last_activity_display"/>
	<t t-out="luxon.DateTime.fromISO(record.last_activity.raw_value).toRelative()"/>

You might need to ask a JS framework expert if this is allowed, because I did not see a single occurence in the code to this one exactly, but that works and it avoids the compute field last_activity_display