Add ability to collect stats via WebSocket

[iliajie]

Howdy y'all,

I have added the ability to collect stats using WebSocket. It's twice as fast and avoids server bombardment every second. The process can be fully controlled: collection can be enabled or disabled, the collection interval can be changed, all without disconnecting.

1. Open WebSocket by running:

https://localhost:10000/stats-socket.cgi

2. Connect using browser:

let socket = new WebSocket("wss://localhost:10000/authentic-theme/ws-555");

socket.onopen = function() {
    console.log("Connected to server");
    // Send initial parameters to the server
    socket.send(JSON.stringify({ save: 0, interval: 2, running: 1 }));
};

socket.onmessage = function(event) {
    let message = JSON.parse(event.data);
    console.log("Received stats:", message);
};

socket.onclose = function() {
    console.log("Disconnected from server");
};

3. Try controlling connection:

socket.send(JSON.stringify({ save: 0, interval: 0, running: 1 })); // higher the interval
socket.send(JSON.stringify({ save: 0, interval: 0, running: 0 })); // pause collection
socket.close(); // disconnect

[iliajie]

Thanks for taking such a quick look!

No comments at all? 😅

Should I keep the old way of collecting stats (using server queries) for cases where the WebSockets proxy won't work, like the Webmin Servers Index module? Or should I just disable it completely if WebSockets aren't supported, making it simpler?

[jcameron]

I think just disable it, and for cases where a proxy is being used don't auto-refresh the stats

[iliajie]

I think just disable it, and for cases where a proxy is being used don't auto-refresh the stats

I will do that!

But look at what I've been working on today!

It's now possible to run a single stats server and let multiple clients connect/disconnect to it! No more overhead!

What are your thoughts?

https://github.com/webmin/authentic-theme/commit/438ee10bb740cd0701bff4237193891269f39bfc

[jcameron]

That seems like an over-optimization ... what if the stats are being accessed by different users?

[iliajie]

That seems like an over-optimization.

I don't think it is.

What if the stats are being accessed by different users?

The point is exactly that! Stats are only meant to be accessed by root-capable users.

If stats are accessed by 100 users, we will only collect stats using a single process and broadcast it to all connected users. Before this change, each of those 100 users would have to make a separate call to the server, and a script would be called to collect the stats, leading to server overload and huge overhead.

Now, the stats will be collected in the background and cached. When a user connects, they will receive the most recent data even without needing to be connected all the time to have it collected (as before).

[jcameron]

Does this mean that there will be just one websockets server process that all the browsers connect to ?

[iliajie]

Does this mean that there will be just one websockets server process that all the browsers connect to ?

Yes, absolutely! Check this final patch:

https://github.com/webmin/authentic-theme/commit/53d605cfdece7f4b473db07f0585693a6c3a2f5f

[jcameron]

Looks good! But please do a really careful check to see if there's any security risks from this case, especially if two users could potentially be sharing the same socket. Currently we lock down the xterm websockets connections by session ID to prevent mis-used ... how will that work in this case?

[iliajie]

Especially if two users could potentially be sharing the same socket.

What security risks can be for two or more root-capable users fetching stats?

Currently we lock down the xterm websockets connections by session ID to prevent mis-used ... how will that work in this case?

For now the process will just exit if a user is not a master admin. Isn't that enough?

[jcameron]

Check out this code here in the xterm websockets handler : https://github.com/webmin/webmin/blob/master/xterm/shellserver.pl#L121

Since the websockets connection could come from any user, we need to make sure it's from the same session as the one that launched the terminal.

[iliajie]

Oh, I see what you mean! Even though it's not possible to start a socket server for a regular user, any user can connect to the active socket and retrieve data.

I need to fix it ..

Since the websockets connection could come from any user, we need to make sure it's from the same session as the one that launched the terminal.

Actually, we need to check not just the same session, but whether BASE_REMOTE_USER and/or REMOTE_USER is root.

[jcameron]

A session can only be associated with one user, so I don't think we need to check that as well?

[iliajie]

The fix is to test the session hash and if a user associated with the session is root-capable user. It was tricky to figure out though.

We don't even need the handshake check, as we can only do the test at the moment a client sends session hash. It would be ideal to make this check upon the handshake but that doesn't seem to be possible.

Check this patch:

https://github.com/webmin/authentic-theme/commit/b3c820cbbe8946b53919bf8433c08291b366a99c#diff-e76c9654552e14aea525eb9fd752bec71a8a78dddad40e9421305d471ee26f55

[iliajie]

It would be ideal to make this check upon the handshake but that doesn't seem to be possible.

To be clear, I don't think it's possible to send session hash upon connection time to run an actual test in the handshake?

[jcameron]

Oh no, I don't like that patch .. it make a ton of assumptions about who is or isn't a root-capable user! I'm worried that this patch, while efficient, is going to open up a hard to debug security hole if one user connects to another user's websockets session.

[iliajie]

I'm worried that this patch, while efficient, is going to open up a hard to debug security hole if one user connects to another user's websockets session.

Yep, right, it was still possible! Check this patch out https://github.com/webmin/authentic-theme/commit/470f678f46b55d539f2a750020d4adf290f80353.

It will make sure that the connected user will have to take an action after making a connection and that connection must be verified to perform a broadcast.

[iliajie]

it make a ton of assumptions about who is or isn't a root-capable user!

This is exactly what we have already been doing in webmin_user_can_rpc and webmin_user_login_mode.

If you have other, better suggestions, I'm always opened to the ideas!

[jcameron]

My suggestion is don't make this so complex in order to handle the rare case of multiple concurrent root logins. Just do one server process per session, and ensure that the websockets connection comes from the same session that launched it.

Efficiency is nice, but it shouldn't be put ahead of security or result in code that's too complex to reason about.

[iliajie]

Well, I understand, though multiple root logins will create multiple calls to stats() function, and this is pretty undesirable!

Even though it may be potentially rare, it doesn't seem to be a right thing to do, as that was the main reason why I finally decided to have it reworked.

Besides, the same root user can have multiple sessions from multiple computers.

I suggest we try to make the code work. Currently it isn't possible to highjack the broadcast. Even if alarm is raised from 1 second to 10, it would still be impossible, because connection needs to have a verified flag to be broadcasted.

Before the last patch, a user could connect to the server and wait doing nothing while waiting for another privileged user making connection to the socket and hijacking the broadcast. That wasn't too good.

But even if we imagine for a second, that it would be possible, even though it's not (as far as I can tell) what is the harm? Reading servers' stats? Doesn't seem like a huge security hole either way.

I suggest we look even deeper in to the code and try to hack it right here and right now, to avoid any potential security issues in the future, as I'm not very happy to put so much time into the code and not finishing it in the right way. Having multiple calls to the stats() sucks, as the same admin with the same username can have multiple sessions opened, and each of it will create a background process to stats(), even if browser isn't connected!

The whole point is to have stats always to be collected and stored for the selected period of time. So an admin could open a browser in the morning, despite of the session and get the stats and see what happened through the night ..

Also many admins are paranoid and logout every night .. All this would make stats to be unusable in a modern/nice way ..

[iliajie]

Jamie, any further suggestions?

[iliajie]

Do you see any other potential issues?

Also, perhaps we should move this code to Webmin and activate stats collection upon Webmin start if the theme supports it and server admin have it enabled?