Plumb orphaned tokens through network context creation [chromium/src : main]
Jon Toohill (Gerrit)
Jon Toohill voted![]( "Open in Gerrit")
| Auto-Submit | +1 |
| Commit-Queue | +1 |
Related details
- Andrew Williams
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Williams (Gerrit)
Andrew Williams voted and added 4 comments![]( "Open in Gerrit")
Votes added by Andrew Williams
| Code-Review | +1 |
4 comments
LGTM! I had thought we would just send the prior tokens along with the first GetAuthTokens requests (per layer), but this makes the tokens available even sooner :) Leaving some questions about the MaybeRefill logic changes because that code is challenging to think through and every time it changes I'm afraid we'll introduce some cases that leads to tokens not fetching anymore.
? MakeTokenManagerMap(this, core_host_remote, initial_tokens)nit: can std::move(initial_tokens)
if (NeedsRefill(current_geo_id_) && !fetching_auth_tokens_) {do we still need to incorporate the `!fetching_auth_tokens_` check somewhere?
next_maybe_refill_cache_.Stop();should we add a test for this to ensure we start back again?
Related details
- Jon Toohill
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jon Toohill (Gerrit)
Jon Toohill voted and added 3 comments![]( "Open in Gerrit")
Votes added by Jon Toohill
| Auto-Submit | +1 |
| Commit-Queue | +1 |
3 comments
? MakeTokenManagerMap(this, core_host_remote, initial_tokens)Jon Toohillnit: can std::move(initial_tokens)
Done, and updated MakeTokenManagerMap to take a value rather than a reference.
if (NeedsRefill(current_geo_id_) && !fetching_auth_tokens_) {do we still need to incorporate the `!fetching_auth_tokens_` check somewhere?
No, `MaybeRefillCache()` does that already.
should we add a test for this to ensure we start back again?
- Andrew Williams
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Giovanni Ortuno Urquidi (Gerrit)
Giovanni Ortuno Urquidi added 2 comments![]( "Open in Gerrit")
array<ip_protection.mojom.BlindSignedAuthToken>>?I wonder if we should make it non-optional? Is the distinction between an empty map and a null map useful? We could avoid a couple of if statements otherwise.
Related details
- Jon Toohill
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jon Toohill (Gerrit)
Jon Toohill voted and added 1 comment![]( "Open in Gerrit")
Votes added by Jon Toohill
| Auto-Submit | +1 |
1 comment
array<ip_protection.mojom.BlindSignedAuthToken>>?I wonder if we should make it non-optional? Is the distinction between an empty map and a null map useful? We could avoid a couple of if statements otherwise.
- Andrew Williams
- Giovanni Ortuno Urquidi
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Giovanni Ortuno Urquidi (Gerrit)
Giovanni Ortuno Urquidi added 9 comments![]( "Open in Gerrit")
sorry for the drive-by review. I'm trying to learn more about this code and reviewing code is great for this.
if (profile_->IsIncognitoProfile()) {
network_context_params->initial_ip_protection_tokens =
ipp_core_host->TakeRecycledTokens();
}Just double checking here. I'm a bit confused. Above we are retrieving IPPCoreHost for the current profile which in this case is the incognito profile. But wouldn't the retrieved IPPCoreHost be brand new (and thus have no tokens) if the incognito window was closed before?
Would it be possible to add a browser test?
std::optional<IpProtectionCoreImpl::InitialTokensMap> initial_tokens) {no longer optional?
if (!initial_tokens.empty()) {nit: Move this to ProcessInitialTokens(). I think it would be good to encapsulate all the logic to process tokens (including skipping it if it's empty).
/*initial_tokens=*/std::vector<BlindSignedAuthToken>{},nit: Use a regular constructor unless it's not possible[[1]](https://chromium.googlesource.com/chromium/src/+/lkgr/styleguide/c++/c++-dos-and-donts.md#variable-initialization). Or we could do:
```suggestion
/*initial_tokens=*/{},
```
EXPECT_TRUE(token);nit: ASSERT_TRUE since it being false would cause a crash.
EXPECT_FALSE(token_manager->GetAuthToken(kMountainViewGeoId));super optional nit: I think we can use EXPECT_THAT so if it fails, it will print the token.
base::RunLoop run_loop;
task_environment_.GetMainThreadTaskRunner()->PostDelayedTask(
FROM_HERE, run_loop.QuitClosure(), base::Minutes(2));
run_loop.Run();Does `TaskEnvironment::FastForwardBy(TimeDelta delta)` not work here?
token_manager->SetOnTryGetAuthTokensCompletedForTesting(
task_environment_.QuitClosure());
token_manager->SetCurrentGeo(kMountainViewGeoId);
task_environment_.RunUntilQuit();Out of curiosity, why do we use a task_environment QuitClosure() here but a RunLoop QuitClosure() above?
Related details
- Andrew Williams
- Jon Toohill
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Williams (Gerrit)
Andrew Williams added 1 comment![]( "Open in Gerrit")
if (profile_->IsIncognitoProfile()) {
network_context_params->initial_ip_protection_tokens =
ipp_core_host->TakeRecycledTokens();
}Just double checking here. I'm a bit confused. Above we are retrieving IPPCoreHost for the current profile which in this case is the incognito profile. But wouldn't the retrieved IPPCoreHost be brand new (and thus have no tokens) if the incognito window was closed before?
Would it be possible to add a browser test?
The IPPCoreHost is shared between the regular profile window and the incognito profile because of this line: https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ip_protection/ip_protection_core_host_factory.cc;l=45;drc=4f61c109af56a9de48410e4c34ff3b4311431ec6
There's some more documentation on this here: https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/profiles/profile_selections.h;l=29;drc=bb80bda6b2e71bde7c38303c48484e86f6976e3c
We have to do it this way because the user's OAuth token is only available to the regular browsing mode profile. We do have some browser tests for this behavior:, for example: https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ip_protection/ip_protection_core_host_browsertest.cc;l=398;drc=88400a0631b078da8d308a44873084c053bd1c65
Related details
- Jon Toohill
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Williams (Gerrit)
Andrew Williams added 1 comment![]( "Open in Gerrit")
if (profile_->IsIncognitoProfile()) {
network_context_params->initial_ip_protection_tokens =
ipp_core_host->TakeRecycledTokens();
}Andrew WilliamsJust double checking here. I'm a bit confused. Above we are retrieving IPPCoreHost for the current profile which in this case is the incognito profile. But wouldn't the retrieved IPPCoreHost be brand new (and thus have no tokens) if the incognito window was closed before?
Would it be possible to add a browser test?
The IPPCoreHost is shared between the regular profile window and the incognito profile because of this line: https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ip_protection/ip_protection_core_host_factory.cc;l=45;drc=4f61c109af56a9de48410e4c34ff3b4311431ec6
There's some more documentation on this here: https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/profiles/profile_selections.h;l=29;drc=bb80bda6b2e71bde7c38303c48484e86f6976e3c
We have to do it this way because the user's OAuth token is only available to the regular browsing mode profile. We do have some browser tests for this behavior:, for example: https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ip_protection/ip_protection_core_host_browsertest.cc;l=398;drc=88400a0631b078da8d308a44873084c053bd1c65
to clarify instead of the "regular browsing mode profile", I should say the regular browsing mode IPPCoreHost KeyedService instance
Andrew Williams (Gerrit)
Andrew Williams voted and added 5 comments![]( "Open in Gerrit")
Votes added by Andrew Williams
| Code-Review | +1 |
5 comments
LGTM with a few more nits and % Gio's comments. Thanks!
using ::testing::Optional;nit: looks like this isn't used anymore, should we undo the change to this file?
? MakeTokenManagerMap(this, core_host_remote, initial_tokens)Jon Toohillnit: can std::move(initial_tokens)
Done, and updated MakeTokenManagerMap to take a value rather than a reference.
ah right, thanks!
cache_by_geo_[GetGeoIdFromGeoHint(token.geo_hint)].push_back(nit: can we move the `GetGeoIdFromGeoHint` call to it's own statement so we don't have to think about sequencing when trying to determine whether this code might use-after-move?
ASSERT_TRUE(token_fetcher->GotAllExpectedMockCalls());nit: we could change this to EXPECT_TRUE since it's safe to continue past this and is part of what we are testing vs. a precondition.
Jon Toohill (Gerrit)
Jon Toohill voted and added 11 comments![]( "Open in Gerrit")
Votes added by Jon Toohill
| Auto-Submit | +1 |
| Commit-Queue | +1 |
11 comments
nit: looks like this isn't used anymore, should we undo the change to this file?
Done
if (profile_->IsIncognitoProfile()) {
network_context_params->initial_ip_protection_tokens =
ipp_core_host->TakeRecycledTokens();
}Andrew WilliamsJust double checking here. I'm a bit confused. Above we are retrieving IPPCoreHost for the current profile which in this case is the incognito profile. But wouldn't the retrieved IPPCoreHost be brand new (and thus have no tokens) if the incognito window was closed before?
Would it be possible to add a browser test?
Andrew WilliamsThe IPPCoreHost is shared between the regular profile window and the incognito profile because of this line: https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ip_protection/ip_protection_core_host_factory.cc;l=45;drc=4f61c109af56a9de48410e4c34ff3b4311431ec6
There's some more documentation on this here: https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/profiles/profile_selections.h;l=29;drc=bb80bda6b2e71bde7c38303c48484e86f6976e3c
We have to do it this way because the user's OAuth token is only available to the regular browsing mode profile. We do have some browser tests for this behavior:, for example: https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ip_protection/ip_protection_core_host_browsertest.cc;l=398;drc=88400a0631b078da8d308a44873084c053bd1c65
to clarify instead of the "regular browsing mode profile", I should say the regular browsing mode IPPCoreHost KeyedService instance
Marked as resolved.
std::optional<IpProtectionCoreImpl::InitialTokensMap> initial_tokens) {Jon Toohillno longer optional?
Done
nit: Move this to ProcessInitialTokens(). I think it would be good to encapsulate all the logic to process tokens (including skipping it if it's empty).
Done
cache_by_geo_[GetGeoIdFromGeoHint(token.geo_hint)].push_back(nit: can we move the `GetGeoIdFromGeoHint` call to it's own statement so we don't have to think about sequencing when trying to determine whether this code might use-after-move?
Done
/*initial_tokens=*/std::vector<BlindSignedAuthToken>{},nit: Use a regular constructor unless it's not possible[[1]](https://chromium.googlesource.com/chromium/src/+/lkgr/styleguide/c++/c++-dos-and-donts.md#variable-initialization). Or we could do:
```suggestion
/*initial_tokens=*/{},
```
Done
nit: ASSERT_TRUE since it being false would cause a crash.
Done
EXPECT_FALSE(token_manager->GetAuthToken(kMountainViewGeoId));super optional nit: I think we can use EXPECT_THAT so if it fails, it will print the token.
Acknowledged
base::RunLoop run_loop;
task_environment_.GetMainThreadTaskRunner()->PostDelayedTask(
FROM_HERE, run_loop.QuitClosure(), base::Minutes(2));
run_loop.Run();Does `TaskEnvironment::FastForwardBy(TimeDelta delta)` not work here?
It does, changed to use that instead. I had originally tried `FastForwardUntilNoTasksRemain()` and ran into test failures, and I incorrectly assumed `FastForwardBy()` would hit the same issues.
token_manager->SetOnTryGetAuthTokensCompletedForTesting(
task_environment_.QuitClosure());
token_manager->SetCurrentGeo(kMountainViewGeoId);
task_environment_.RunUntilQuit();Out of curiosity, why do we use a task_environment QuitClosure() here but a RunLoop QuitClosure() above?
Resolved by eliminating the RunLoop above.
ASSERT_TRUE(token_fetcher->GotAllExpectedMockCalls());nit: we could change this to EXPECT_TRUE since it's safe to continue past this and is part of what we are testing vs. a precondition.
- Andrew Williams
- Giovanni Ortuno Urquidi
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Williams (Gerrit)
Andrew Williams voted and added 1 comment![]( "Open in Gerrit")
Votes added by Andrew Williams
| Code-Review | +1 |
1 comment
const std::string& geo_id = GetGeoIdFromGeoHint(token.geo_hint);Since GetGeoIdFromGeoHint returns a std::string, would this be a const reference to a temporary variable that's created (and the const reference extends the lifetime of the temporary)? If so I wonder if it'd be simpler to just make geo_id a std::string
Related details
- Giovanni Ortuno Urquidi
- Jon Toohill
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jon Toohill (Gerrit)
Jon Toohill voted and added 1 comment![]( "Open in Gerrit")
Votes added by Jon Toohill
| Auto-Submit | +1 |
1 comment
const std::string& geo_id = GetGeoIdFromGeoHint(token.geo_hint);Since GetGeoIdFromGeoHint returns a std::string, would this be a const reference to a temporary variable that's created (and the const reference extends the lifetime of the temporary)? If so I wonder if it'd be simpler to just make geo_id a std::string
- Andrew Williams
- Giovanni Ortuno Urquidi
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Williams (Gerrit)
Andrew Williams voted![]( "Open in Gerrit")
- Giovanni Ortuno Urquidi
- Jon Toohill
Code-Coverage
Code-Owners
Code-Review
| 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:
Shadow: ff...@chromium.org; IPC: tse...@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/).
Shadow IPC reviewer(s): ff...@chromium.org. Please conduct an IPC review and CR+1 when satisfied. Remember to add the main reviewers to the attention set if needed.
Main IPC reviewer(s): tse...@chromium.org. Please wait for the shadowed IPC reviewer to CR+1 before reviewing.
Shadowed: ff...@chromium.org
Reviewer source(s):
ff...@chromium.org, tse...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
- Andrew Williams
- Fred Shih
- Giovanni Ortuno Urquidi
- Tom Sepez
- Tsuyoshi Horo
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fred Shih (Gerrit)
Fred Shih voted and added 1 comment![]( "Open in Gerrit")
Votes added by Fred Shih
| Code-Review | +1 |
| Commit-Queue | +2 |
1 comment
Related details
- Andrew Williams
- Giovanni Ortuno Urquidi
- Tom Sepez
- Tsuyoshi Horo
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tom Sepez (Gerrit)
Tom Sepez added 1 comment![]( "Open in Gerrit")
mechanically, it seems OK, but I'm not that familiar with the design. Is there a security person who has been working with you on this? If so, they might be the appropriate reviewer.
Related details
- Andrew Williams
- Fred Shih
- Giovanni Ortuno Urquidi
- Jon Toohill
- Tsuyoshi Horo
Jon Toohill (Gerrit)
Jon Toohill voted and added 1 comment![]( "Open in Gerrit")
Votes added by Jon Toohill
| Auto-Submit | +1 |
mechanically, it seems OK, but I'm not that familiar with the design. Is there a security person who has been working with you on this? If so, they might be the appropriate reviewer.
No, we don't have a dedicated security contact for this project. I've expanded the commit description and field comments to provide more context around what data is being passed between processes, and the associated bug also links to a design doc with further details.
Let me know if you still have specific concerns that I haven't addressed, and I'll do my best to clarify!
Related details
- Andrew Williams
- Fred Shih
- Giovanni Ortuno Urquidi
- Tom Sepez
- Tsuyoshi Horo
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tom Sepez (Gerrit)
Tom Sepez added 1 comment![]( "Open in Gerrit")
Jon Toohillmechanically, it seems OK, but I'm not that familiar with the design. Is there a security person who has been working with you on this? If so, they might be the appropriate reviewer.
No, we don't have a dedicated security contact for this project. I've expanded the commit description and field comments to provide more context around what data is being passed between processes, and the associated bug also links to a design doc with further details.
Let me know if you still have specific concerns that I haven't addressed, and I'll do my best to clarify!
Thanks, that makes much more sense, and I think you're OK from a security perspective, but the privacy issues may be more subtle. I've left comments on the design doc. If we know that the privacy team is OK with this, then we should be OK.
Related details
- Andrew Williams
- Fred Shih
- Giovanni Ortuno Urquidi
- Jon Toohill
- Tsuyoshi Horo
Tom Sepez (Gerrit)
Tom Sepez voted and added 1 comment![]( "Open in Gerrit")
Votes added by Tom Sepez
| Code-Review | +1 |
| Commit-Queue | +2 |
1 comment
Privacy discussed offline and is minimal.
Tsuyoshi Horo (Gerrit)
Tsuyoshi Horo voted and added 1 comment![]( "Open in Gerrit")
Votes added by Tsuyoshi Horo
| Code-Review | +1 |
1 comment
- Andrew Williams
- Fred Shih
- Giovanni Ortuno Urquidi
- Jon Toohill
- Tom Sepez
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jon Toohill (Gerrit)
Jon Toohill voted Commit-Queue+2![]( "Open in Gerrit")
- Andrew Williams
- Fred Shih
- Giovanni Ortuno Urquidi
- Tom Sepez
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Plumb orphaned tokens through network context creation
This change implements the logic for pre-warming the IP Protection
token cache with orphaned tokens from previous Incognito sessions by
passing tokens from the browser process (high-privilege) to the new
Incognito network context within the network service process
(sandboxed). This parallels the existing token fetching flow, where the
browser process obtains signed tokens from a trusted server before
passing them to the network service process.
The tokens themselves are trusted data that have already been parsed
and validated when they were originally fetched, and are designed to
minimize the risk of linking multiple Incognito sessions from the same
user.
- M chrome/browser/ip_protection/ip_protection_core_host.h
- M chrome/browser/net/profile_network_context_service.cc
- M components/ip_protection/common/ip_protection_core_impl.h
- M components/ip_protection/common/ip_protection_core_impl_mojo.cc
- M components/ip_protection/common/ip_protection_core_impl_mojo.h
- M components/ip_protection/common/ip_protection_token_manager_impl.cc
- M components/ip_protection/common/ip_protection_token_manager_impl.h
- M components/ip_protection/common/ip_protection_token_manager_impl_unittest.cc
- M services/network/network_context.cc
- M services/network/public/mojom/network_context.mojom
Code-Review: +1 by Tsuyoshi Horo, +1 by Tom Sepez
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |