remoting Linux multi-process: Fix pairing [chromium/src : main]
Yuwei Huang (Gerrit)
Yuwei Huang added 1 comment![]( "Open in Gerrit")
Related details
- Lambros Lambrou
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lambros Lambrou (Gerrit)
Lambros Lambrou added 2 comments![]( "Open in Gerrit")
iterate the directory, which is fine as long as all the sensitive
files are not readable by other users.Please update the CL description to clarify that the network account owns this directory and all the files inside. The description already says that other users cannot read the files, but we should explain that the network process has full access - it can see tokens for all users.
return config_dir.Append(kRegistryDirectory);I was going to suggest `/var/lib/` but I think it's ok to put the private tokens under `/etc/`. There is a precedent for this in `/etc/shadow` which is still used in modern Linux.
Would it make sense for the Debian package to keep this config on uninstall, but delete this config on purge?
I think it's good for the package to not directly manage this directory. Otherwise it would be a "debconf" dir, and users would get pointless questions like "do you want to keep the system config, or apply the package maintainer's version?"
So we should keep this unmanaged, and then we'll need to update the maintainer-scripts to get the proper remove/purge behavior.
This applies to the top-level `/etc/chrome-remote-desktop` directory, so maybe it's better to do this in another CL?
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. |
Lambros Lambrou (Gerrit)
Lambros Lambrou voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
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 | +2 |
3 comments
iterate the directory, which is fine as long as all the sensitive
files are not readable by other users.Please update the CL description to clarify that the network account owns this directory and all the files inside. The description already says that other users cannot read the files, but we should explain that the network process has full access - it can see tokens for all users.
I just did `s/other/irrelevant/`.
#1 is about `/etc/chrome-remote-desktop`, and #2 is about `/etc/chrome-remote-desktop/paired-clients`, and #2 says to change the ownership to the network user, so I don't think there is much confusion.
I was going to suggest `/var/lib/` but I think it's ok to put the private tokens under `/etc/`. There is a precedent for this in `/etc/shadow` which is still used in modern Linux.
Would it make sense for the Debian package to keep this config on uninstall, but delete this config on purge?
I think it's good for the package to not directly manage this directory. Otherwise it would be a "debconf" dir, and users would get pointless questions like "do you want to keep the system config, or apply the package maintainer's version?"
So we should keep this unmanaged, and then we'll need to update the maintainer-scripts to get the proper remove/purge behavior.
This applies to the top-level `/etc/chrome-remote-desktop` directory, so maybe it's better to do this in another CL?
Yeah, I thought about using `/var/lib/chrome-remote-desktop`, but that would introduce another location for config management, which is very specific to Linux, and the code would look awkward on other platforms.
This applies to the top-level `/etc/chrome-remote-desktop` directory, so maybe it's better to do this in another CL?
Sounds like a good idea. I'll do that in a separate CL.
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")
Unreviewed changes
6 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.
Change information
remoting Linux multi-process: Fix pairing
For the multi-process host, the pairing directory is in
/etc/chrome-remote-desktop, which is owned by root so the network
process won't be able to create pairing entries under it.
This CL fixes this by:
1. Change the permissions of /etc/chrome-remote-desktop from 700 to 755.
This is required for the network process to access the
paired-client/ directory in it. This will no allow modifications of
files in /etc/chrome-remote-desktop, but will allow any users to
iterate the directory, which is fine as long as all the sensitive
files are not readable by irrelevant users.
2. Make the daemon process create the paired-client/ directory, change
its ownership to the network user, and set permissions to 700.
3. Add a check to the daemon process to ensure the host config file is
not readable by other users.
- M remoting/host/daemon_process_linux.cc
- M remoting/host/linux/daemon_process_main.cc
- M remoting/host/pairing_registry_delegate_linux.cc
- M remoting/host/pairing_registry_delegate_linux.h
- M remoting/host/setup/start_host_as_root.cc
Code-Review: +1 by Lambros Lambrou
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |