remoting: Refactor security key pipeline to support different transports [chromium/src : main]
0 views
Skip to first unread message
Yuwei Huang (Gerrit)
Jun 25, 2026, 12:13:17 AM (4 days ago) Jun 25
to chromium...@chromium.org, chromotin...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org
Yuwei Huang voted and added 1 comment![]( "Open in Gerrit")
Votes added by Yuwei Huang
| Commit-Queue | +1 |
1 comment
File remoting/host/client_session.cc
Line 164, Patchset 4:
if (desktop_environment_options.enable_security_key()) {Yuwei Huang . resolved
Yuwei HuangBy moving `SecurityKeyExtension` creation here, it is added unconditionally (if the host option is enabled), bypassing the `security_key_auth_policy_enabled_` check that used to be in `RemotingMe2MeHost`.
Because `SecurityKeyExtensionSession` no longer has access to policies, it will process gnubby control/data messages even if the enterprise policy disables it, potentially causing a policy bypass.
To fix this in the interim before the data channel migration is complete, read the local policy from `local_session_policies_provider_`. This will ensure you are on par with the old behavior.
Done
Related details
Attention set is empty
Submit Requirements:
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)
Jun 25, 2026, 12:14:00 AM (4 days ago) Jun 25
to Lambros Lambrou, chromium...@chromium.org, chromotin...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org
Attention needed from Lambros Lambrou
Yuwei Huang voted and added 1 comment![]( "Open in Gerrit")
Votes added by Yuwei Huang
| Auto-Submit | +1 |
1 comment
Patchset-level comments
Related details
Attention is currently required from:
- Lambros Lambrou
Submit Requirements:
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)
Jun 25, 2026, 12:15:33 AM (4 days ago) Jun 25
to Chromium LUCI CQ, Lambros Lambrou, chromium...@chromium.org, chromotin...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org
Attention needed from Lambros Lambrou
Yuwei Huang added 1 comment![]( "Open in Gerrit")
File remoting/host/remoting_me2me_host.cc
Line 2053, Patchset 4 (Parent):
if (security_key_auth_policy_enabled_ &&Yuwei Huang . resolved
Yuwei HuangSince `SecurityKeyExtension` lifecycle has been moved to `ClientSession`, `security_key_auth_policy_enabled_` is no longer used to gate the extension.
As mentioned in the TODO above `OnGnubbyAuthPolicyUpdate()`, this callback and the associated host restart logic can likely be completely removed now.
Done
Related details
Attention is currently required from:
- Lambros Lambrou
Submit Requirements:
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)
Jun 25, 2026, 2:03:52 PM (3 days ago) Jun 25
to Yuwei Huang, Chromium LUCI CQ, chromium...@chromium.org, chromotin...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org
Attention needed from Yuwei Huang
Lambros Lambrou voted and added 1 comment![]( "Open in Gerrit")
Votes added by Lambros Lambrou
| Code-Review | +1 |
1 comment
File remoting/host/security_key/security_key_extension_session.cc
Line 81, Patchset 7 (Latest):
auth_handler_->SetSendMessageCallback(base::NullCallback());Lambros Lambrou . unresolved
Just a question: does every caller of GetSendMessageCallback() check the returned value for null before using it?
Related details
Attention is currently required from:
- Yuwei Huang
Submit Requirements:
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)
Jun 26, 2026, 3:33:32 PM (2 days ago) Jun 26
to Lambros Lambrou, Chromium LUCI CQ, chromium...@chromium.org, chromotin...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org
Yuwei Huang voted and added 2 comments![]( "Open in Gerrit")
Votes added by Yuwei Huang
| Commit-Queue | +2 |
2 comments
Patchset-level comments
Yuwei Huang . resolved
Thanks!
File remoting/host/security_key/security_key_extension_session.cc
Line 81, Patchset 7 (Latest):
auth_handler_->SetSendMessageCallback(base::NullCallback());Lambros Lambrou . resolved
Just a question: does every caller of GetSendMessageCallback() check the returned value for null before using it?
Yuwei Huang
It looks like it is only used in tests. Maybe we should add the `ForTesting` suffice, but that's for another day.
https://source.chromium.org/search?q=GetSendMessageCallback%20f:Remoting
Related details
Attention set is empty
Submit Requirements:
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)
Jun 26, 2026, 4:03:58 PM (2 days ago) Jun 26
to Yuwei Huang, Lambros Lambrou, chromium...@chromium.org, chromotin...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Commit message:
remoting: Refactor security key pipeline to support different transports
The goal is to allow ClientSession to create
SecurityKeyDataChannelHandler that uses the same underlying
SecurityKeyAuthHandler when the client supports the securityKeyV2
capability.
- Pull SecurityKeyAuthHandler out of SecurityKeyExtension and make
ClientSession own it. This allows SecurityKeyDataChannelHandler to
take WeakPtr<SecurityKeyAuthHandler>.
- Create SecurityKeyExtensionSession inside ClientSession instead of
HostProcess since HostProcess doesn't have access to the
SecurityKeyAuthHandler.
- Remove `send_message_callback` from SecurityKeyAuthHandler::Create
since SecurityKeyExtension and SecurityKeyDataChannelHandler call
SetSendMessageCallback() in their constructor instead.
- Remove `security_key_auth_policy_enabled_`,
HostProcess::OnGnubbyAuthPolicyUpdate and friends, since we now
check the policy for each session, and there is no need to restart
the host upon policy change.
In the next CL, ClientSession will create SecurityKeyDataChannelHandler
after authentication and hook it up to SecurityKeyAuthHandler if the
client supports the securityKeyV2 capability.
TAG=agy
CONV=a74bbced-b6c1-454e-adea-b8a5ad41a1ef
Bug: b:517007701
Change-Id: I1f89734ab57f369c1f6f0393c20d0c0f88c51de3
Reviewed-by: Lambros Lambrou <lambros...@chromium.org>
Commit-Queue: Yuwei Huang <yuw...@chromium.org>
Auto-Submit: Yuwei Huang <yuw...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1653391}
Files:
- M remoting/host/client_session.cc
- M remoting/host/client_session.h
- M remoting/host/host_mock_objects.cc
- M remoting/host/host_mock_objects.h
- M remoting/host/remoting_me2me_host.cc
- M remoting/host/security_key/security_key_auth_handler.cc
- M remoting/host/security_key/security_key_auth_handler.h
- M remoting/host/security_key/security_key_auth_handler_mojo.cc
- M remoting/host/security_key/security_key_auth_handler_mojo.h
- M remoting/host/security_key/security_key_auth_handler_posix.cc
- M remoting/host/security_key/security_key_auth_handler_posix.h
- M remoting/host/security_key/security_key_extension.cc
- M remoting/host/security_key/security_key_extension.h
- M remoting/host/security_key/security_key_extension_session.cc
- M remoting/host/security_key/security_key_extension_session.h
- M remoting/host/security_key/security_key_extension_session_unittest.cc
Change size: M
Delta: 16 files changed, 111 insertions(+), 111 deletions(-)
Branch: refs/heads/main
Submit Requirements:
Code-Review: +1 by Lambros Lambrou
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages