[signin] Plumb and gate mTLS token binding in the identity stack [chromium/src : main]
Ali Hijazi (Gerrit)
Ali Hijazi added 1 comment![]( "Open in Gerrit")
Related details
- Alex Ilin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Ilin (Gerrit)
Alex Ilin added 2 comments![]( "Open in Gerrit")
I haven't done an in-depth review yet, just wanted to share an immediate concern with the feature setup.
2. The `IsRefreshTokenBoundToMtls()` method will return false if the
feature is disabled, causing the system to treat all tokens as
unbound to mTLS, even if they were previously marked as bound in the
local database.I'm not sure we want to implement this second bit.
Based on my prior experience with DBSC, this will make potential Finch group reassignments much more painful because a feature rollback will mean signing-out for already enrolled users.
We had the same strategy for DBSC and I wished we had done it differently, so let's avoid making the same mistakes.
I think it'd be reasonable to guard the database reads with a separate kill-switch flag that we'd deploy only in emergency scenarios. This kill-switch should also reset the value in the database so that we don't have to keep it enabled forever. (I'd implement it in a follow-up CL)
Related details
- Ali Hijazi
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. |
Alex Ilin (Gerrit)
Alex Ilin added 13 comments![]( "Open in Gerrit")
2. The `IsRefreshTokenBoundToMtls()` method will return false if the
feature is disabled, causing the system to treat all tokens as
unbound to mTLS, even if they were previously marked as bound in the
local database.I'm not sure we want to implement this second bit.
Based on my prior experience with DBSC, this will make potential Finch group reassignments much more painful because a feature rollback will mean signing-out for already enrolled users.
We had the same strategy for DBSC and I wished we had done it differently, so let's avoid making the same mistakes.
I think it'd be reasonable to guard the database reads with a separate kill-switch flag that we'd deploy only in emergency scenarios. This kill-switch should also reset the value in the database so that we don't have to keep it enabled forever. (I'd implement it in a follow-up CL)
Discussed offline that it still makes sense to keep this check % avoiding the feature activation on every startup (already done).
please add mTLS test to this file
We need to check that mtls_token_binding is plumbed correctly to `GaiaAuthFetcher` and to `AddOrUpdateAccount()`
mtls_token_binding = true;Unrelated to this CL, but I'm worried that this sets `true` directly instead of checking the attribute value. The code would work unexpectedly if the server sends `mtls_token_binding=false`
base::test::ScopedFeatureList scoped_feature_list;Could you please also test that "mtls_token_binding=true" is ignored if the feature is disabled?
// In memory store mapping account_id to mtls_token_binding info.
std::map<CoreAccountId, bool> mtls_token_bindings_;I don't necessarily like the idea of having a separate mapping for this.
I see two better alternatives:
1. store these values in `token_binding_helper_`; the main disadvantage is that mTLS binding won't work without enabling the TPM key binding first, as otherwise `token_binding_helper_` will be null. We could refactor this logic to make `token_binding_helper_` non-null
2. extend the `refresh_token` map to also store a `bool` next to `crypto::ProcessBoundString`. This way we'll ensure that every refresh token will store an `mtls_token_binding` value
Option 2 seems to be more straightforward to me and it avoid coupling mTLS and TPM binding together.
bool IsRefreshTokenBoundToMtls(I feel like this creates a conflict between `IsRefreshTokenBound()` and `IsRefreshTokenBoundToMtls()`.
It sounds like `IsRefreshTokenBound` should be a superset of both but it is not, i.e. `IsRefreshTokenBound() == false && IsRefreshTokenBoundToMtls() == true` is a valid state.
Possible solutions:
1. Rename `IsRefreshTokenBound()` to `IsRefreshTokenBoundToKey()`
2. Say `ShouldUseMtlsForAccessTokenFetches()` instead of `IsRefreshTokenBoundToMtls()`
3. Combine both functions into one returning an EnumSet (syntax is not accurate): `EnumSet<mtls_bound, key_bound> GetRefreshTokenBindingState()`
PersistenceLoadMtlsBoundToken) {Could we also test what happens if the feature gets disabled after a restart?
bool IsRefreshTokenBoundToMtls(const CoreAccountId& account_id) const;I don't see a need for publicly exposing this method from `ProfileOAuth2TokenService`. Can this be kept as an implementation detail within `MutableProfileOAuth2TokenServiceDelegate`?
BASE_FEATURE(kEnableMtlsTokenBinding, base::FEATURE_DISABLED_BY_DEFAULT);nit: Could you please add a short feature description in a comment?
// Whether the refresh token is bound to mTLS endpoints. If true,
// access token requests and other Gaia interactions using this token
// should use mTLS-specific endpoints.nit: minor corrections
"other Gaia interactions" is too vague and may be misinterpreted. For now, we are only redirecting access token requests to mTLS endpoints. The auth_code -> LST exchange doesn't count because it happens before the `TokenBindingInfo` struct can be even created.
```suggestion
// Whether the refresh token is bound to an mTLS certificate. If true,
// access token requests using this token should use mTLS-specific
// endpoints.
```
sql::Statement s(db()->GetUniqueStatement(
"SELECT service, encrypted_token, binding_key, mtls_token_binding "
"FROM token_service"));nit: This is a pretty random formatting change Can it be rolled back?
bool use_mtls_endpoints_for_fetching_tokens_;nit: could be made `const`
also, please give it a default value (and to the `is_refresh_token_bound_` param below)
```suggestion
const bool use_mtls_endpoints_for_fetching_tokens_ = false;
```
/*use_mtls_endpoints_for_fetching_tokens=*/false,nit: it'd be good to have a test when this parameter is true
Related details
- Ali Hijazi
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. |
Ali Hijazi (Gerrit)
Ali Hijazi added 12 comments![]( "Open in Gerrit")
please add mTLS test to this file
We need to check that mtls_token_binding is plumbed correctly to `GaiaAuthFetcher` and to `AddOrUpdateAccount()`
Done
Unrelated to this CL, but I'm worried that this sets `true` directly instead of checking the attribute value. The code would work unexpectedly if the server sends `mtls_token_binding=false`
The server responds with "mtls_token_binding=true" or doesn't add mtls_token_binding attribute at all. It should be okay to check against "true"
Could you please also test that "mtls_token_binding=true" is ignored if the feature is disabled?
Done
// In memory store mapping account_id to mtls_token_binding info.
std::map<CoreAccountId, bool> mtls_token_bindings_;I don't necessarily like the idea of having a separate mapping for this.
I see two better alternatives:
1. store these values in `token_binding_helper_`; the main disadvantage is that mTLS binding won't work without enabling the TPM key binding first, as otherwise `token_binding_helper_` will be null. We could refactor this logic to make `token_binding_helper_` non-null
2. extend the `refresh_token` map to also store a `bool` next to `crypto::ProcessBoundString`. This way we'll ensure that every refresh token will store an `mtls_token_binding` valueOption 2 seems to be more straightforward to me and it avoid coupling mTLS and TPM binding together.
Done
I feel like this creates a conflict between `IsRefreshTokenBound()` and `IsRefreshTokenBoundToMtls()`.
It sounds like `IsRefreshTokenBound` should be a superset of both but it is not, i.e. `IsRefreshTokenBound() == false && IsRefreshTokenBoundToMtls() == true` is a valid state.
Possible solutions:
1. Rename `IsRefreshTokenBound()` to `IsRefreshTokenBoundToKey()`
2. Say `ShouldUseMtlsForAccessTokenFetches()` instead of `IsRefreshTokenBoundToMtls()`
3. Combine both functions into one returning an EnumSet (syntax is not accurate): `EnumSet<mtls_bound, key_bound> GetRefreshTokenBindingState()`
Done
Could we also test what happens if the feature gets disabled after a restart?
Done
bool IsRefreshTokenBoundToMtls(const CoreAccountId& account_id) const;I don't see a need for publicly exposing this method from `ProfileOAuth2TokenService`. Can this be kept as an implementation detail within `MutableProfileOAuth2TokenServiceDelegate`?
Done
BASE_FEATURE(kEnableMtlsTokenBinding, base::FEATURE_DISABLED_BY_DEFAULT);nit: Could you please add a short feature description in a comment?
Done
// Whether the refresh token is bound to mTLS endpoints. If true,
// access token requests and other Gaia interactions using this token
// should use mTLS-specific endpoints.nit: minor corrections
"other Gaia interactions" is too vague and may be misinterpreted. For now, we are only redirecting access token requests to mTLS endpoints. The auth_code -> LST exchange doesn't count because it happens before the `TokenBindingInfo` struct can be even created.
```suggestion
// Whether the refresh token is bound to an mTLS certificate. If true,
// access token requests using this token should use mTLS-specific
// endpoints.
```
Done
sql::Statement s(db()->GetUniqueStatement(
"SELECT service, encrypted_token, binding_key, mtls_token_binding "
"FROM token_service"));nit: This is a pretty random formatting change Can it be rolled back?
Done
nit: could be made `const`
also, please give it a default value (and to the `is_refresh_token_bound_` param below)
```suggestion
const bool use_mtls_endpoints_for_fetching_tokens_ = false;
```
Done
nit: it'd be good to have a test when this parameter is true
- Alex Ilin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Ilin (Gerrit)
Alex Ilin voted and added 8 comments![]( "Open in Gerrit")
Votes added by Alex Ilin
| Code-Review | +1 |
8 comments
Nice, LGTM!
Let's add a browser test to `chrome/browser/signin/dice_browsertest.cc` next to ensure that the whole flow works end-to-end.
const auto* account = dice_params.signin_info()->GetInitiator();
ASSERT_TRUE(account);
const auto& account_info = account->account_info;
CoreAccountId account_id = identity_manager()->PickAccountIdForAccount(
account_info.gaia_id, account_info.email);nit: you already have a local `account_information` object that should be identical to `account_info` you're extracting
```suggestion
CoreAccountId account_id = identity_manager()->PickAccountIdForAccount(
account_information.gaia_id, account_information.email);
```
EXPECT_TRUE(auth_error_email_.empty());
EXPECT_EQ(GoogleServiceAuthError::NONE, auth_error_.state());
// Check HandleTokenExchangeSuccess parameters.
EXPECT_EQ(token_exchange_account_id_, account_id);
EXPECT_TRUE(token_exchange_is_new_account_);
// Check that the reconcilor was blocked and unblocked exactly once.
EXPECT_EQ(1, reconcilor_blocked_count_);
EXPECT_EQ(1, reconcilor_unblocked_count_);
// Check that the AccountInfo::is_under_advanced_protection is set.
AccountInfo extended_account_info =
identity_manager()->FindExtendedAccountInfoByAccountId(account_id);
EXPECT_TRUE(extended_account_info.is_under_advanced_protection);
// Check that the AccessPoint was propagated from the delegate.
EXPECT_EQ(extended_account_info.access_point,
signin_metrics::AccessPoint::kSettings);
EXPECT_EQ(
identity_test_env_.GetNumCallsToPrepareForFetchingAccountCapabilities(),
1);
histogram_tester_.ExpectUniqueSample(
kTokenBindingOutcomeHistogram,
DiceResponseHandler::TokenBindingOutcome::kNotBoundNotSupported,
/*expected_bucket_count=*/1);nit: This stuff is not very relevant for this test, so I'd just remove it.
One think that's missing is verification that the account will be using an mtls endpoint for access token fetches. Could you please add this to your test?
if (value == "true" &&nit: let's make this comparison case-insensitive
```suggestion
if (base::EqualsCaseInsensitiveASCII(value, "true") &&
```
base::test::ScopedFeatureList scoped_feature_list;
scoped_feature_list.InitAndEnableFeature(switches::kEnableMtlsTokenBinding);optional: this can be done in a single statement
ditto L495, L575
```suggestion
base::test::ScopedFeatureList
scoped_feature_list(switches::kEnableMtlsTokenBinding);
```
EXPECT_EQ(kGaiaID, account->account_info.gaia_id);
EXPECT_EQ(kEmail, account->account_info.email);
EXPECT_EQ(kSessionIndex, account->account_info.session_index);
EXPECT_EQ(kAuthorizationCode, account->authorization_code);
EXPECT_EQ(kSupportedTokenBindingAlgorithms,
account->supported_algorithms_for_token_binding);
EXPECT_FALSE(account->mtls_token_binding);
histogram_tester.ExpectUniqueSample("Signin.DiceAuthorizationCode", true, 1);nit: Most of these comparisons are unnecessary for this test. The test will be much more readable if you focus it on a single purpose of verifying the `mtls_token_binding` parameter.
```suggestion
EXPECT_FALSE(account->mtls_token_binding);
```
// Maps account ids to mtls_token_binding.
std::map<CoreAccountId, bool> mtls_token_bindings_;nit: `mtls_token_bindings_` isn't used for anything, please remove
MutableProfileOAuth2TokenServiceDelegate::TokenData::TokenData()
: refresh_token(std::string()) {}nit: do we need a default constructor here at all? `crypto::ProcessBoundString` doesn't have one, so I'd say that `TokenData` doesn't need one either.
Related details
- Ali Hijazi
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. |
Ali Hijazi (Gerrit)
Ali Hijazi added 7 comments![]( "Open in Gerrit")
const auto* account = dice_params.signin_info()->GetInitiator();
ASSERT_TRUE(account);
const auto& account_info = account->account_info;
CoreAccountId account_id = identity_manager()->PickAccountIdForAccount(
account_info.gaia_id, account_info.email);nit: you already have a local `account_information` object that should be identical to `account_info` you're extracting
```suggestion
CoreAccountId account_id = identity_manager()->PickAccountIdForAccount(
account_information.gaia_id, account_information.email);
```
Done
EXPECT_TRUE(auth_error_email_.empty());
EXPECT_EQ(GoogleServiceAuthError::NONE, auth_error_.state());
// Check HandleTokenExchangeSuccess parameters.
EXPECT_EQ(token_exchange_account_id_, account_id);
EXPECT_TRUE(token_exchange_is_new_account_);
// Check that the reconcilor was blocked and unblocked exactly once.
EXPECT_EQ(1, reconcilor_blocked_count_);
EXPECT_EQ(1, reconcilor_unblocked_count_);
// Check that the AccountInfo::is_under_advanced_protection is set.
AccountInfo extended_account_info =
identity_manager()->FindExtendedAccountInfoByAccountId(account_id);
EXPECT_TRUE(extended_account_info.is_under_advanced_protection);
// Check that the AccessPoint was propagated from the delegate.
EXPECT_EQ(extended_account_info.access_point,
signin_metrics::AccessPoint::kSettings);
EXPECT_EQ(
identity_test_env_.GetNumCallsToPrepareForFetchingAccountCapabilities(),
1);
histogram_tester_.ExpectUniqueSample(
kTokenBindingOutcomeHistogram,
DiceResponseHandler::TokenBindingOutcome::kNotBoundNotSupported,
/*expected_bucket_count=*/1);nit: This stuff is not very relevant for this test, so I'd just remove it.
One think that's missing is verification that the account will be using an mtls endpoint for access token fetches. Could you please add this to your test?
I attempted to modify the test by adding:
```c++
base::RunLoop run_loop;
identity_test_env_.SetCallbackForNextAccessTokenRequest(
run_loop.QuitClosure());
signin::AccessTokenFetcher::TokenCallback callback =
base::BindOnce([](GoogleServiceAuthError error,
signin::AccessTokenInfo access_token_info) {});
std::unique_ptr<signin::AccessTokenFetcher> token_fetcher =
identity_manager()->CreateAccessTokenFetcherForAccount(
account_id, signin::OAuthConsumerId::kExtensionsIdentityAPI,
signin_client_.GetURLLoaderFactory(), std::move(callback),
signin::AccessTokenFetcher::Mode::kImmediate);
run_loop.Run();
network::TestURLLoaderFactory* factory =
signin_client_.GetTestURLLoaderFactory();
size_t num_pending = factory->NumPending();
ASSERT_GE(num_pending, 1u);
EXPECT_EQ(factory->GetPendingRequest(num_pending - 1)->request.url,
GaiaUrls::GetInstance()->mtls_oauth2_token_url());
```
Unfortunately this didn't work as expected as we end up calling `FakeOAuth2AccessTokenManager::FetchOAuth2Token` instead of `OAuth2AccessTokenManager::FetchOAuth2Token`.
If you have another approach in mind, please let me know. It seems we might need a browsertest for this.
nit: let's make this comparison case-insensitive
```suggestion
if (base::EqualsCaseInsensitiveASCII(value, "true") &&
```
Done
base::test::ScopedFeatureList scoped_feature_list;
scoped_feature_list.InitAndEnableFeature(switches::kEnableMtlsTokenBinding);optional: this can be done in a single statement
ditto L495, L575
```suggestion
base::test::ScopedFeatureList
scoped_feature_list(switches::kEnableMtlsTokenBinding);
```
Done
EXPECT_EQ(kGaiaID, account->account_info.gaia_id);
EXPECT_EQ(kEmail, account->account_info.email);
EXPECT_EQ(kSessionIndex, account->account_info.session_index);
EXPECT_EQ(kAuthorizationCode, account->authorization_code);
EXPECT_EQ(kSupportedTokenBindingAlgorithms,
account->supported_algorithms_for_token_binding);
EXPECT_FALSE(account->mtls_token_binding);
histogram_tester.ExpectUniqueSample("Signin.DiceAuthorizationCode", true, 1);nit: Most of these comparisons are unnecessary for this test. The test will be much more readable if you focus it on a single purpose of verifying the `mtls_token_binding` parameter.
```suggestion
EXPECT_FALSE(account->mtls_token_binding);
```
Done
// Maps account ids to mtls_token_binding.
std::map<CoreAccountId, bool> mtls_token_bindings_;nit: `mtls_token_bindings_` isn't used for anything, please remove
nice catch, thanks!
MutableProfileOAuth2TokenServiceDelegate::TokenData::TokenData()
: refresh_token(std::string()) {}nit: do we need a default constructor here at all? `crypto::ProcessBoundString` doesn't have one, so I'd say that `TokenData` doesn't need one either.
- Alex Ilin
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. |