remoting: Introduce enable_security_key to DesktopEnvironmentOptions [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 voted and added 1 comment![]( "Open in Gerrit")
Votes added by Lambros Lambrou
| Code-Review | +1 |
1 comment
if (!extension_session || !allow_gnubby) {Could you split this into separate checks with different log messages?
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 1 comment![]( "Open in Gerrit")
Votes added by Yuwei Huang
| Auto-Submit | +1 |
| Commit-Queue | +1 |
1 comment
Related details
- Chromium IPC Reviews
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. |
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
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 1 comment![]( "Open in Gerrit")
Votes added by Yuwei Huang
| Auto-Submit | +1 |
| Commit-Queue | +1 |
1 comment
Could you split this into separate checks with different log messages?
Done
Related details
- Daniel Cheng
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Cheng (Gerrit)
Daniel Cheng voted and added 3 comments![]( "Open in Gerrit")
Votes added by Daniel Cheng
| Code-Review | +1 |
3 comments
LGTM though it seems to me like there's some room to simplify the SequenceBound plumbing (but that's out of scope here)
security_key_auth_handler_
.AsyncCall(&SecurityKeyAuthHandler::SetSendMessageCallback)
.WithArgs(base::BindPostTaskToCurrentDefault(
base::BindRepeating(&DesktopSessionAgent::OnSecurityKeyMessage,
weak_factory_.GetWeakPtr())));I know this is pre-existing but it seems like this is probably better off as an additional constructor param rather than requiring two PostTasks to set up the object.
&SecurityKeyAuthHandler::CreateSecurityKeyConnection);(And this could be combined as well)
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
Thanks!
security_key_auth_handler_
.AsyncCall(&SecurityKeyAuthHandler::SetSendMessageCallback)
.WithArgs(base::BindPostTaskToCurrentDefault(
base::BindRepeating(&DesktopSessionAgent::OnSecurityKeyMessage,
weak_factory_.GetWeakPtr())));I know this is pre-existing but it seems like this is probably better off as an additional constructor param rather than requiring two PostTasks to set up the object.
In a follow-up CL, I'd expect `SetSendMessageCallback` to be called after the security key handler is created, so I don't think we can do it here.
&SecurityKeyAuthHandler::CreateSecurityKeyConnection);(And this could be combined as well)
I'll clean that up later. For the legacy signaling code path in [SecurityKeyExtensionSession](https://source.chromium.org/chromium/chromium/src/+/main:remoting/host/security_key/security_key_extension_session.cc;l=146;drc=362dbaf3c1048a93929fff003e784d537f3ebfbc) `CreateSecurityKeyConnection` is called in response to a client message, but with the new WIP data channel approach, I think we should just create it immediately when the channel is open.
Related details
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 Commit-Queue+0![]( "Open in Gerrit")
| Commit-Queue | +0 |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
remoting: Introduce enable_security_key to DesktopEnvironmentOptions
The motivation is to pass `security_key_extension_supported_` via
DesktopEnvironmentOptions so that we don't need to plumb the flag
through ChromotingHost, but Gemini suggests that I should also enforce
it (i.e. not starting the UDS server) in the desktop process, so I just
do both in this CL.
Specifically:
1. Adds enable_security_key getter/setter to DesktopEnvironmentOptions,
defaulting to false.
2. Updates DesktopEnvironmentOptions Mojo struct and traits to serialize
the new option, and adds a round-trip serialization unit test.
3. Hardens DesktopSessionAgent to check options.enable_security_key()
and verify that the socket name is not empty. If it is empty (due to
misconfiguration like missing XDG_RUNTIME_DIR), it gracefully logs a
warning and disables forwarding rather than crashing.
4. Hardens ClientSession::OnSecurityKeyConnection to check both
enable_security_key() and the effective policy and reject binding
request if the check fails.
5. Consolidates the capability check in HostProcess to use the
DesktopEnvironmentOptions container as the single source of truth,
eliminating the redundant security_key_extension_supported_ member.
BUG=517007701
- M remoting/host/base/desktop_environment_options.cc
- M remoting/host/base/desktop_environment_options.h
- M remoting/host/client_session.cc
- M remoting/host/desktop_session_agent.cc
- M remoting/host/mojom/desktop_session.mojom
- M remoting/host/mojom/remoting_mojom_traits.cc
- M remoting/host/mojom/remoting_mojom_traits.h
- M remoting/host/mojom/remoting_mojom_traits_unittest.cc
- M remoting/host/remoting_me2me_host.cc
Code-Review: +1 by Lambros Lambrou, +1 by Daniel Cheng
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |