[crd host][linux] Match login usernames in daemon process [chromium/src : main]
Yuwei Huang (Gerrit)
Yuwei Huang voted and added 1 comment![]( "Open in Gerrit")
Votes added by Yuwei Huang
| Commit-Queue | +1 |
1 comment
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 2 comments![]( "Open in Gerrit")
std::vector<std::string>& allowed_login_usernames();
const std::vector<std::string>& allowed_login_usernames() const;I don't think this belongs here. This feels like a policy related field rather than a behavior.
const std::vector<std::string>& allowed_login_usernames,I think this should just be 'required_username' or 'username' which, if empty means no policy is set (or you can use std::optional if you want instead of empty).
The gist is that if the 'MatchUsername' policy is set, it is reasonable to assume that this is an enterprise configuration and it is reasonable to expect that the usernames in the host config (and from the initial heartbeat) will match but potentially have different domains due to the Gaia dependency.
IMO it would be cleaner to provide a single username which is used for injecting or reconnecting to a Desktop process and there isn't a use case where the username will vary (unless it's empty and the host has no policy set).
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 3 comments![]( "Open in Gerrit")
Votes added by Yuwei Huang
| Commit-Queue | +1 |
3 comments
I just rewrote this CL to hook into `OnUpdateHostOwner()`. PTAL!
std::vector<std::string>& allowed_login_usernames();
const std::vector<std::string>& allowed_login_usernames() const;I don't think this belongs here. This feels like a policy related field rather than a behavior.
Done
I think this should just be 'required_username' or 'username' which, if empty means no policy is set (or you can use std::optional if you want instead of empty).
The gist is that if the 'MatchUsername' policy is set, it is reasonable to assume that this is an enterprise configuration and it is reasonable to expect that the usernames in the host config (and from the initial heartbeat) will match but potentially have different domains due to the Gaia dependency.
IMO it would be cleaner to provide a single username which is used for injecting or reconnecting to a Desktop process and there isn't a use case where the username will vary (unless it's empty and the host has no policy set).
- 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 4 comments![]( "Open in Gerrit")
looks way better to me but I have one question about however username is provide.
required_username_ = username;You only need to set this value once per network process lifetime so I think it would be good to pass it in when a desktop session is created however if that isn't feasible, then maybe check it here to ensure it doesn't change.
// it is ready, so we just return true here.Did you want to return true here to match the comment? :)
SetRequiredUsername(string username);Could this be a param in CreateDesktopSession? I think the gist is that the username will not change during the lifetime of the network process so it should be known at start-up and the value could be provided when the desktop session is created.
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 added 4 comments![]( "Open in Gerrit")
You only need to set this value once per network process lifetime so I think it would be good to pass it in when a desktop session is created however if that isn't feasible, then maybe check it here to ensure it doesn't change.
Done
Did you want to return true here to match the comment? :)
Oops... Done!
Could this be a param in CreateDesktopSession? I think the gist is that the username will not change during the lifetime of the network process so it should be known at start-up and the value could be provided when the desktop session is created.
- 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 Code-Review+1![]( "Open in Gerrit")
- 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")
- 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 2 comments![]( "Open in Gerrit")
Votes added by Alex Gough
| Code-Review | +1 |
2 comments
lgtm mojom
// If non-empty, the login user of the desktop session must match `username`.you could use an optional here `string?` but up to you
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 | +2 |
2 comments
Thanks!
// If non-empty, the login user of the desktop session must match `username`.you could use an optional here `string?` but up to you
Yeah, I considered this, but we use an empty string for this value everywhere else, so it seems more consistent to just use that.
Related details
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] Match login usernames in daemon process
Similar to the PAM check, we cannot enforce the username match policy on
the network process since the username would always be `_crd_network`.
This CL fixes this by having the network process pass the required
username to the daemon process, so that the daemon process can enforce
the policy. This may also allow us to enforce the username match policy
on Windows at some point.
The login username will be checked whenever the host owner username is
known, changed, or a new desktop session is created. It will not be
checked for greeter sessions, since the greeter session will be run as a
system user (e.g. `Debian-gdm`) that is generally not the host owner.
- M remoting/host/daemon_process.cc
- M remoting/host/daemon_process.h
- M remoting/host/daemon_process_linux.cc
- M remoting/host/daemon_process_unittest.cc
- M remoting/host/daemon_process_win.cc
- M remoting/host/desktop_session_connector.h
- M remoting/host/desktop_session_win.cc
- M remoting/host/desktop_session_win.h
- M remoting/host/ipc_desktop_environment.cc
- M remoting/host/ipc_desktop_environment.h
- M remoting/host/ipc_desktop_environment_unittest.cc
- M remoting/host/linux/desktop_session_factory_linux.cc
- M remoting/host/linux/desktop_session_factory_linux.h
- M remoting/host/mojom/desktop_session.mojom
- M remoting/host/remoting_me2me_host.cc
Code-Review: +1 by Joe Downing, +1 by Alex Gough
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |