[crd host][linux] Recover desktop session when host restarts [chromium/src : main]
Yuwei Huang (Gerrit)
Yuwei Huang added 3 comments![]( "Open in Gerrit")
string client_email;I also thought about just having the daemon process "push" a recovered desktop session to [IpcDesktopEnvironment](https://source.chromium.org/chromium/chromium/src/+/main:remoting/host/ipc_desktop_environment.h;l=171;drc=373482a795ba8d47958dd09d49669cc23d076ec8), which seems to make sense in the first glance since it already has some session reuse logic, but it is going to have race conditions if a client tries to connect at the beginning of the session recovery process. The terminal ID clash is one issue, and another issue is that if the recovered desktop session is delivered after the connection, then the user will end up with two remote displays.
Related details
- Joe Downing
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yuwei Huang (Gerrit)
Yuwei Huang added 1 comment![]( "Open in Gerrit")
HandleSessionInfoQueriesBlockingStartup();This is to ensure that `DesktopSessionFactoryLinux::OnStartResult` can observe the environment variables of recovered sessions.
Related details
- Joe Downing
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joe Downing (Gerrit)
Joe Downing added 5 comments![]( "Open in Gerrit")
static constexpr std::string_view kCharset =
"0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz";
std::string result;
result.reserve(16);
for (int i = 0; i < 16; ++i) {
result += kCharset[base::RandInt(0, kCharset.size() - 1)];Why not just use a UUID? I think that's more common and would simplify your impl.
std::string_view client_email,nit: This isn't critical but it's very possible for the client user to only have a username and not an email address (though you could always synthesize one). I'd suggest calling this client_id which is a string to represent the remote user which could be an email, username, etc. I don't think it's too critical though so feel free to ack if you prefer using email here and want to synthesize it.
protocol::RemoteDisplays proto;WDYT about using base::Value::Dict and serializing to JSON instead of a protobuf? It seems like storing state between host instances (crash, update, etc) would be useful or other purposes and then you don't need to add a (to me) confusingly name 'remote_display.proto' file which is specific to Linux and actually represents GDM sessions (not remote displays in the video sense)?
Another way to resolve this would be to store the proto in the same directory as this file. IMO /remoting/proto is for service and protocol protos which is why I find it a little confusing to see a Linux-specific, local-only proto there. I would be fine with the name and such if the proto were stored and built in a /linux directory.
for (const auto& [display_name, weak_session] : desktop_sessions_) {Can you call this 'session'? It seems like the fact that it is a WeakPtr isn't important to call out.
}From Review Agent (and it seems reasonable):
These two lines were moved inside the `if (result.has_value())` block. This seems to be in error.
If `result` does not have a value (i.e., the `logind` query fails), `session_info_queries_blocking_startup_` will not be erased for this `display_path`, and `HandleSessionInfoQueriesBlockingStartup` will not be called for this path.
This will likely cause the startup to hang, waiting for a query that has already completed (with a failure).
These lines should be moved back outside the `if (result.has_value())` block to ensure startup can proceed even if session info cannot be fetched.
Related details
- Yuwei Huang
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yuwei Huang (Gerrit)
Yuwei Huang voted and added 5 comments![]( "Open in Gerrit")
Votes added by Yuwei Huang
| Commit-Queue | +1 |
5 comments
static constexpr std::string_view kCharset =
"0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz";
std::string result;
result.reserve(16);
for (int i = 0; i < 16; ++i) {
result += kCharset[base::RandInt(0, kCharset.size() - 1)];Why not just use a UUID? I think that's more common and would simplify your impl.
Done. GDM doesn't like `-` in the remote ID though, so I have to remove them.
std::string_view client_email,nit: This isn't critical but it's very possible for the client user to only have a username and not an email address (though you could always synthesize one). I'd suggest calling this client_id which is a string to represent the remote user which could be an email, username, etc. I don't think it's too critical though so feel free to ack if you prefer using email here and want to synthesize it.
Done
protocol::RemoteDisplays proto;WDYT about using base::Value::Dict and serializing to JSON instead of a protobuf? It seems like storing state between host instances (crash, update, etc) would be useful or other purposes and then you don't need to add a (to me) confusingly name 'remote_display.proto' file which is specific to Linux and actually represents GDM sessions (not remote displays in the video sense)?
Another way to resolve this would be to store the proto in the same directory as this file. IMO /remoting/proto is for service and protocol protos which is why I find it a little confusing to see a Linux-specific, local-only proto there. I would be fine with the name and such if the proto were stored and built in a /linux directory.
Done
for (const auto& [display_name, weak_session] : desktop_sessions_) {Can you call this 'session'? It seems like the fact that it is a WeakPtr isn't important to call out.
Done
From Review Agent (and it seems reasonable):
These two lines were moved inside the `if (result.has_value())` block. This seems to be in error.
If `result` does not have a value (i.e., the `logind` query fails), `session_info_queries_blocking_startup_` will not be erased for this `display_path`, and `HandleSessionInfoQueriesBlockingStartup` will not be called for this path.
This will likely cause the startup to hang, waiting for a query that has already completed (with a failure).
These lines should be moved back outside the `if (result.has_value())` block to ensure startup can proceed even if session info cannot be fetched.
The display path needs to be erased if `result` has no value, yes. AI's suggestion of moving it out the else block doesn't make sense though. The point is that, if we need to fetch the systemd environment, then we should wait for it to complete. The else block is for the session reporter code path, so we could wait for that as well, but we are going to delete that code path at some point, so I just didn't bother to fix it.
Related details
- Joe Downing
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joe Downing (Gerrit)
Joe Downing voted and added 1 comment![]( "Open in Gerrit")
Votes added by Joe Downing
| Code-Review | +1 |
1 comment
return config_dir.Append("remote_displays.json");Can you confirm some details about this file (probably also worth a comment).
This file will be read by a process running as root and the file will be ACL'd to that process correct?
If the file is tampered with, and the 'required_username' field is set (as is the case for Corp connections) then the remoting desktop process will not be injected in the 'remote display' since the user associated with the session will not match.
I think these are important details and worth documenting, meaning the mitigations in place to prevent the file from being tampered with to gain access to arbitrary sessions.
Related details
- Yuwei Huang
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yuwei Huang (Gerrit)
Yuwei Huang voted and added 2 comments![]( "Open in Gerrit")
Votes added by Yuwei Huang
| Commit-Queue | +1 |
2 comments
New approval is needed for the async file util change. PTAL thanks!
Also adding IPC reviewer.
Can you confirm some details about this file (probably also worth a comment).
This file will be read by a process running as root and the file will be ACL'd to that process correct?
If the file is tampered with, and the 'required_username' field is set (as is the case for Corp connections) then the remoting desktop process will not be injected in the 'remote display' since the user associated with the session will not match.
I think these are important details and worth documenting, meaning the mitigations in place to prevent the file from being tampered with to gain access to arbitrary sessions.
I was kind of relying on the default 022 umask. I think it could be exposing information if everyone can read that file, i.e. who has connected to CRD. I just changed the permission of the file to 600.
If the file is tampered with, and the 'required_username' field is set (as is the case for Corp connections) then the remoting desktop process will not be injected in the 'remote display' since the user associated with the session will not match.
We are not checking `required_username` on the `client_id` (which is the client email), since as you said, `client_id` could become something other than the client email.
The check is in `IsSessionUsernameAllowed`, which happens during client connection. If the file is tampered with so that it points to another user's graphical session, then the entry will still be in the map, but the desktop session will be immediately closed when the client connects.
I've added some more comments.
Related details
- Chromium IPC Reviews
- Joe Downing
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
From googleclient/chrome/chromium_gwsq/ipc/config.gwsq:
IPC: dch...@chromium.org
📎 It looks like you’re making a possibly security-sensitive change! 📎 IPC security review isn’t a rubberstamp, so your friendly security reviewer will need a fair amount of context to review your CL effectively. Please review your CL description and code comments to make sure they provide context for someone unfamiliar with your project/area. Pay special attention to where data comes from and which processes it flows between (and their privilege levels). Feel free to point your security reviewer at design docs, bugs, or other links if you can’t reasonably make a self-contained CL description. (Also see https://cbea.ms/git-commit/).
IPC reviewer(s): dch...@chromium.org
Reviewer source(s):
dch...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
- Daniel Cheng
- Joe Downing
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joe Downing (Gerrit)
Joe Downing added 1 comment![]( "Open in Gerrit")
return config_dir.Append("remote_displays.json");Yuwei HuangCan you confirm some details about this file (probably also worth a comment).
This file will be read by a process running as root and the file will be ACL'd to that process correct?
If the file is tampered with, and the 'required_username' field is set (as is the case for Corp connections) then the remoting desktop process will not be injected in the 'remote display' since the user associated with the session will not match.
I think these are important details and worth documenting, meaning the mitigations in place to prevent the file from being tampered with to gain access to arbitrary sessions.
I was kind of relying on the default 022 umask. I think it could be exposing information if everyone can read that file, i.e. who has connected to CRD. I just changed the permission of the file to 600.
If the file is tampered with, and the 'required_username' field is set (as is the case for Corp connections) then the remoting desktop process will not be injected in the 'remote display' since the user associated with the session will not match.
We are not checking `required_username` on the `client_id` (which is the client email), since as you said, `client_id` could become something other than the client email.
The check is in `IsSessionUsernameAllowed`, which happens during client connection. If the file is tampered with so that it points to another user's graphical session, then the entry will still be in the map, but the desktop session will be immediately closed when the client connects.
I've added some more comments.
We are not checking required_username on the client_id (which is the client email), since as you said, client_id could become something other than the client email.
I wasn't implying that you should, I just wanted to ensure that at some point it is being checked such that modifications to the file would not allow an arbitrary client to connect to another user's session.
Related details
- Daniel Cheng
- Yuwei Huang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yuwei Huang (Gerrit)
Yuwei Huang added 1 comment![]( "Open in Gerrit")
return config_dir.Append("remote_displays.json");Yuwei HuangCan you confirm some details about this file (probably also worth a comment).
This file will be read by a process running as root and the file will be ACL'd to that process correct?
If the file is tampered with, and the 'required_username' field is set (as is the case for Corp connections) then the remoting desktop process will not be injected in the 'remote display' since the user associated with the session will not match.
I think these are important details and worth documenting, meaning the mitigations in place to prevent the file from being tampered with to gain access to arbitrary sessions.
Joe DowningI was kind of relying on the default 022 umask. I think it could be exposing information if everyone can read that file, i.e. who has connected to CRD. I just changed the permission of the file to 600.
If the file is tampered with, and the 'required_username' field is set (as is the case for Corp connections) then the remoting desktop process will not be injected in the 'remote display' since the user associated with the session will not match.
We are not checking `required_username` on the `client_id` (which is the client email), since as you said, `client_id` could become something other than the client email.
The check is in `IsSessionUsernameAllowed`, which happens during client connection. If the file is tampered with so that it points to another user's graphical session, then the entry will still be in the map, but the desktop session will be immediately closed when the client connects.
I've added some more comments.
We are not checking required_username on the client_id (which is the client email), since as you said, client_id could become something other than the client email.
I wasn't implying that you should, I just wanted to ensure that at some point it is being checked such that modifications to the file would not allow an arbitrary client to connect to another user's session.
- Daniel Cheng
- Joe Downing
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joe Downing (Gerrit)
Joe Downing voted and added 1 comment![]( "Open in Gerrit")
Votes added by Joe Downing
| Code-Review | +1 |
1 comment
return config_dir.Append("remote_displays.json");Yuwei HuangCan you confirm some details about this file (probably also worth a comment).
This file will be read by a process running as root and the file will be ACL'd to that process correct?
If the file is tampered with, and the 'required_username' field is set (as is the case for Corp connections) then the remoting desktop process will not be injected in the 'remote display' since the user associated with the session will not match.
I think these are important details and worth documenting, meaning the mitigations in place to prevent the file from being tampered with to gain access to arbitrary sessions.
Joe DowningI was kind of relying on the default 022 umask. I think it could be exposing information if everyone can read that file, i.e. who has connected to CRD. I just changed the permission of the file to 600.
If the file is tampered with, and the 'required_username' field is set (as is the case for Corp connections) then the remoting desktop process will not be injected in the 'remote display' since the user associated with the session will not match.
We are not checking `required_username` on the `client_id` (which is the client email), since as you said, `client_id` could become something other than the client email.
The check is in `IsSessionUsernameAllowed`, which happens during client connection. If the file is tampered with so that it points to another user's graphical session, then the entry will still be in the map, but the desktop session will be immediately closed when the client connects.
I've added some more comments.
Yuwei HuangWe are not checking required_username on the client_id (which is the client email), since as you said, client_id could become something other than the client email.
I wasn't implying that you should, I just wanted to ensure that at some point it is being checked such that modifications to the file would not allow an arbitrary client to connect to another user's session.
Could you re-approve?
Ugh, I highly dislike Geritt and how it resets +1s to OWNERS :(
Related details
- Daniel Cheng
- Yuwei Huang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yuwei Huang (Gerrit)
Yuwei Huang voted and added 1 comment![]( "Open in Gerrit")
Votes added by Yuwei Huang
| Commit-Queue | +1 |
1 comment
- Chromium IPC Reviews
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
From googleclient/chrome/chromium_gwsq/ipc/config.gwsq:
IPC: aj...@chromium.org
📎 It looks like you’re making a possibly security-sensitive change! 📎 IPC security review isn’t a rubberstamp, so your friendly security reviewer will need a fair amount of context to review your CL effectively. Please review your CL description and code comments to make sure they provide context for someone unfamiliar with your project/area. Pay special attention to where data comes from and which processes it flows between (and their privilege levels). Feel free to point your security reviewer at design docs, bugs, or other links if you can’t reasonably make a self-contained CL description. (Also see https://cbea.ms/git-commit/).
IPC reviewer(s): aj...@chromium.org
Reviewer source(s):
aj...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
- Alex Gough
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Gough (Gerrit)
Alex Gough voted and added 1 comment![]( "Open in Gerrit")
Votes added by Alex Gough
| Code-Review | +1 |
1 comment
- Yuwei Huang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yuwei Huang (Gerrit)
Yuwei Huang voted and added 1 comment![]( "Open in Gerrit")
Votes added by Yuwei Huang
| Commit-Queue | +2 |
1 comment
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[crd host][linux] Recover desktop session when host restarts
This CL updates the multi-process Linux host so that it stores the
client_email=>remote_display_name mapping in a protobuf file and reuses
these remote displays when creating new desktop sessions. This allows
users to connect to graphical sessions created by the previous host
incarnation, in case of host crashes or updates.
The display name is also updated to a UUID to avoid race conditions and
management pains of the terminal ID, which is currently assigned by the
network process.
RemoteDisplaySessionManager is also updated to wait for environment
variables of user sessions to be loaded before they run the Init
callback, eliminating a potential race condition.
- M remoting/base/async_file_util.cc
- M remoting/base/async_file_util.h
- M remoting/host/ipc_desktop_environment.cc
- M remoting/host/ipc_desktop_environment.h
- M remoting/host/linux/desktop_session_factory_linux.cc
- M remoting/host/linux/desktop_session_factory_linux.h
- M remoting/host/linux/remote_display_session_manager.cc
- M remoting/host/mojom/desktop_session.mojom
Code-Review: +1 by Joe Downing, +1 by Alex Gough
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |