[DBSC] Bind refresh token only when the server requests it
The server is adding a new "eligible_for_token_binding" parameter to the
DICE SIGNIN response header. When this parameter is not set, Chrome
shouldn't attempt to bind the refresh token.
When "eligible_for_token_binding" is set, its value will contain a list
of supported algorithms separated by space:
eligible_for_token_binding=ES256 RS256
Fixed: b:356938919
Change-Id: I0b0988219c17110deb11d7eb8c6005498d469f5e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5816854
Commit-Queue: Alex Ilin <alexilin@chromium.org>
Code-Coverage: findit-for-me@appspot.gserviceaccount.com <findit-for-me@appspot.gserviceaccount.com>
Reviewed-by: David Roger <droger@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1351695}
diff --git a/chrome/browser/signin/dice_browsertest.cc b/chrome/browser/signin/dice_browsertest.cc
index ee8c686..f9fc7c38 100644
--- a/chrome/browser/signin/dice_browsertest.cc
+++ b/chrome/browser/signin/dice_browsertest.cc
@@ -240,7 +240,8 @@
http_response->AddCustomHeader(
kDiceResponseHeader,
base::StringPrintf(
- "action=SIGNIN,authuser=1,id=%s,email=%s,authorization_code=%s",
+ "action=SIGNIN,authuser=1,id=%s,email=%s,authorization_code=%s,"
+ "eligible_for_token_binding=ES256 RS256",
signin::GetTestGaiaIdForEmail(main_email).c_str(),
main_email.c_str(), kAuthorizationCode));
}
@@ -817,8 +818,10 @@
SendRefreshTokenResponse();
EXPECT_TRUE(
GetIdentityManager()->HasAccountWithRefreshToken(GetMainAccountID()));
- // TODO(http://b/274463812): verify that the refresh token is bound once the
- // binding status is propagated to `IdentityManager`.
+ EXPECT_FALSE(
+ GetIdentityManager()
+ ->GetWrappedBindingKeyOfRefreshTokenForAccount(GetMainAccountID())
+ .empty());
}
#endif // BUILDFLAG(ENABLE_BOUND_SESSION_CREDENTIALS)
diff --git a/chrome/browser/signin/dice_response_handler.cc b/chrome/browser/signin/dice_response_handler.cc
index 21854f18..6c9adc6 100644
--- a/chrome/browser/signin/dice_response_handler.cc
+++ b/chrome/browser/signin/dice_response_handler.cc
@@ -368,7 +368,9 @@
dice_params.signin_info->account_info;
ProcessDiceSigninHeader(
info.gaia_id, info.email, dice_params.signin_info->authorization_code,
- dice_params.signin_info->no_authorization_code, std::move(delegate));
+ dice_params.signin_info->no_authorization_code,
+ dice_params.signin_info->supported_algorithms_for_token_binding,
+ std::move(delegate));
return;
}
case signin::DiceAction::ENABLE_SYNC: {
@@ -415,6 +417,7 @@
const std::string& email,
const std::string& authorization_code,
bool no_authorization_code,
+ const std::string& supported_algorithms_for_token_binding,
std::unique_ptr<ProcessDiceHeaderDelegate> delegate) {
if (no_authorization_code) {
lock_ = std::make_unique<AccountReconcilor::Lock>(account_reconcilor_);
@@ -464,9 +467,12 @@
#if BUILDFLAG(ENABLE_BOUND_SESSION_CREDENTIALS)
if (!registration_token_helper_factory_.is_null() &&
- !registration_token_helper_) {
+ !registration_token_helper_ &&
+ !supported_algorithms_for_token_binding.empty()) {
CHECK(switches::IsChromeRefreshTokenBindingEnabled(
signin_client_->GetPrefs()));
+ // TODO(crbug.com/362480455): pass the list of supported algorithms to the
+ // helper factory once supported.
registration_token_helper_ = registration_token_helper_factory_.Run(
GetWrappedBindingKeyToReuse(*identity_manager_));
}
@@ -476,7 +482,13 @@
gaia_id, email, authorization_code, signin_client_, account_reconcilor_,
std::move(delegate),
#if BUILDFLAG(ENABLE_BOUND_SESSION_CREDENTIALS)
- registration_token_helper_.get(),
+ // It's important to check `supported_algorithms_for_token_binding` here
+ // in addition to the factory call above because
+ // `registration_token_helper_` might be shared between several
+ // `DiceTokenFetcher`s.
+ supported_algorithms_for_token_binding.empty()
+ ? nullptr
+ : registration_token_helper_.get(),
#endif // BUILDFLAG(ENABLE_BOUND_SESSION_CREDENTIALS)
this));
}
diff --git a/chrome/browser/signin/dice_response_handler.h b/chrome/browser/signin/dice_response_handler.h
index 96f7b57..52cde41 100644
--- a/chrome/browser/signin/dice_response_handler.h
+++ b/chrome/browser/signin/dice_response_handler.h
@@ -204,6 +204,7 @@
const std::string& email,
const std::string& authorization_code,
bool no_authorization_code,
+ const std::string& supported_algorithms_for_token_binding,
std::unique_ptr<ProcessDiceHeaderDelegate> delegate);
// Process the Dice enable sync action.
diff --git a/chrome/browser/signin/dice_response_handler_unittest.cc b/chrome/browser/signin/dice_response_handler_unittest.cc
index 7d0800c..737e44be 100644
--- a/chrome/browser/signin/dice_response_handler_unittest.cc
+++ b/chrome/browser/signin/dice_response_handler_unittest.cc
@@ -61,6 +61,7 @@
namespace {
const char kAuthorizationCode[] = "authorization_code";
+const char kEligibleForTokenBinding[] = "ES256 RS256";
const char kEmail[] = "test@email.com";
const int kSessionIndex = 42;
@@ -212,6 +213,8 @@
std::make_unique<DiceResponseParams::SigninInfo>();
dice_params.signin_info->account_info = account_info;
dice_params.signin_info->authorization_code = kAuthorizationCode;
+ dice_params.signin_info->supported_algorithms_for_token_binding =
+ kEligibleForTokenBinding;
break;
case DiceAction::ENABLE_SYNC:
dice_params.enable_sync_info =
@@ -499,6 +502,36 @@
EXPECT_EQ(GoogleServiceAuthError::NONE, auth_error_.state());
}
+// Checks that no token binding attempt is made when an account is ineligible
+// for token binding.
+TEST_F(DiceResponseHandlerTest, SigninIneligibleForTokenBinding) {
+ EnableRegistrationTokenHelperFactory();
+ DiceResponseParams dice_params = MakeDiceParams(DiceAction::SIGNIN);
+ dice_params.signin_info->supported_algorithms_for_token_binding.clear();
+ const auto& account_info = dice_params.signin_info->account_info;
+ CoreAccountId account_id = identity_manager()->PickAccountIdForAccount(
+ account_info.gaia_id, account_info.email);
+ EXPECT_FALSE(identity_manager()->HasAccountWithRefreshToken(account_id));
+
+ dice_response_handler_->ProcessDiceHeader(
+ dice_params, std::make_unique<TestProcessDiceHeaderDelegate>(this));
+ // Check that a GaiaAuthFetcher has been created immediately.
+ GaiaAuthConsumer* consumer = signin_client_.GetAndClearConsumer();
+ ASSERT_THAT(consumer, testing::NotNull());
+ // Simulate GaiaAuthFetcher success.
+ consumer->OnClientOAuthSuccess(GaiaAuthConsumer::ClientOAuthResult(
+ "refresh_token", "access_token", 10, /*is_child_account=*/false,
+ /*is_under_advanced_protection=*/true, /*is_bound_to_key=*/false));
+ // Check that the token has been inserted in the token service and it is
+ // unbound.
+ EXPECT_TRUE(identity_manager()->HasAccountWithRefreshToken(account_id));
+ EXPECT_TRUE(identity_manager()
+ ->GetWrappedBindingKeyOfRefreshTokenForAccount(account_id)
+ .empty());
+ EXPECT_TRUE(auth_error_email_.empty());
+ EXPECT_EQ(GoogleServiceAuthError::NONE, auth_error_.state());
+}
+
TEST_F(DiceResponseHandlerTest, ReuseBindingKeyOtherTokenIsBound) {
EnableRegistrationTokenHelperFactory();
const std::vector<uint8_t> kWrappedKey = {1, 2, 3};
@@ -629,6 +662,47 @@
kWrappedKey);
}
+TEST_F(DiceResponseHandlerTest, TwoFetchersOneEligible) {
+ EnableRegistrationTokenHelperFactory();
+ auto authorization_code = [&](const DiceResponseParams& dice_params) {
+ return dice_params.signin_info->authorization_code;
+ };
+
+ DiceResponseParams eligible_dice_params_ = MakeDiceParams(DiceAction::SIGNIN);
+ DiceResponseParams ineligible_dice_params =
+ MakeDiceParams(DiceAction::SIGNIN);
+ ineligible_dice_params.signin_info->account_info =
+ GetDiceResponseParamsAccountInfo("other@email.com");
+ ineligible_dice_params.signin_info->authorization_code =
+ "other_authorization_code";
+ ineligible_dice_params.signin_info->supported_algorithms_for_token_binding
+ .clear();
+ ExpectRegistrationTokenHelperCreated(
+ {authorization_code(eligible_dice_params_)},
+ /*expected_wrapped_binding_key=*/{});
+
+ dice_response_handler_->ProcessDiceHeader(
+ eligible_dice_params_,
+ std::make_unique<TestProcessDiceHeaderDelegate>(this));
+ // Token fetch should be blocked on the binding registration token generation.
+ ASSERT_THAT(signin_client_.GetAndClearConsumer(), testing::IsNull());
+
+ dice_response_handler_->ProcessDiceHeader(
+ ineligible_dice_params,
+ std::make_unique<TestProcessDiceHeaderDelegate>(this));
+ // Token fetch should start immediately for ineligible account.
+ ASSERT_THAT(signin_client_.GetAndClearConsumer(), testing::NotNull());
+
+ // Simulate successful token generation and check that GaiaAuthFetcher has
+ // been created.
+ const std::vector<uint8_t> kWrappedKey = {1, 2, 3};
+ SimulateRegistrationTokenHelperResult(
+ authorization_code(eligible_dice_params_),
+ RegistrationTokenHelper::Result(unexportable_keys::UnexportableKeyId(),
+ kWrappedKey, "test_registration_token"));
+ ASSERT_THAT(signin_client_.GetAndClearConsumer(), testing::NotNull());
+}
+
TEST_F(DiceResponseHandlerTest,
NewRegistrationTokenHelperCreatedForConsecutiveFetchers) {
EnableRegistrationTokenHelperFactory();
diff --git a/components/signin/core/browser/dice_header_helper.cc b/components/signin/core/browser/dice_header_helper.cc
index 2031493..530f695 100644
--- a/components/signin/core/browser/dice_header_helper.cc
+++ b/components/signin/core/browser/dice_header_helper.cc
@@ -30,6 +30,8 @@
const char kSigninNoAuthorizationCodeAttrName[] = "no_authorization_code";
const char kSigninEmailAttrName[] = "email";
const char kSigninIdAttrName[] = "id";
+const char kSigninEligibleForTokenBindingAttrName[] =
+ "eligible_for_token_binding";
// Signout response parameters.
const char kSignoutEmailAttrName[] = "email";
@@ -110,6 +112,13 @@
DLOG(WARNING)
<< "No authorization code header expected only with SIGNIN action";
}
+ } else if (key_name == kSigninEligibleForTokenBindingAttrName) {
+ if (params.signin_info) {
+ params.signin_info->supported_algorithms_for_token_binding = value;
+ } else {
+ DLOG(WARNING) << "Eligible for token binding attribute expected only "
+ "with SIGNIN action";
+ }
} else {
DLOG(WARNING) << "Unexpected Gaia header attribute '" << key_name << "'.";
}
diff --git a/components/signin/core/browser/signin_header_helper.h b/components/signin/core/browser/signin_header_helper.h
index 5f4994d..fccd542 100644
--- a/components/signin/core/browser/signin_header_helper.h
+++ b/components/signin/core/browser/signin_header_helper.h
@@ -129,6 +129,9 @@
// Whether Dice response contains the 'no_authorization_code' header value.
// If true then LSO was unavailable for provision of auth code.
bool no_authorization_code = false;
+ // If the account is eligible for token binding, this string is non-empty
+ // and contains a list of supported binding algorithms separated by space.
+ std::string supported_algorithms_for_token_binding;
};
// Parameters for the SIGNOUT action.
diff --git a/components/signin/core/browser/signin_header_helper_unittest.cc b/components/signin/core/browser/signin_header_helper_unittest.cc
index 81bc44d9..ae0ffcd 100644
--- a/components/signin/core/browser/signin_header_helper_unittest.cc
+++ b/components/signin/core/browser/signin_header_helper_unittest.cc
@@ -548,6 +548,7 @@
const char kAuthorizationCode[] = "authorization_code";
const char kEmail[] = "foo@example.com";
const char kGaiaID[] = "gaia_id";
+ const char kSupportedTokenBindingAlgorithms[] = "ES256 RS256";
const int kSessionIndex = 42;
{
@@ -555,14 +556,18 @@
base::HistogramTester histogram_tester;
DiceResponseParams params =
BuildDiceSigninResponseParams(base::StringPrintf(
- "action=SIGNIN,id=%s,email=%s,authuser=%i,authorization_code=%s",
- kGaiaID, kEmail, kSessionIndex, kAuthorizationCode));
+ "action=SIGNIN,id=%s,email=%s,authuser=%i,authorization_code=%s,"
+ "eligible_for_token_binding=%s",
+ kGaiaID, kEmail, kSessionIndex, kAuthorizationCode,
+ kSupportedTokenBindingAlgorithms));
EXPECT_EQ(DiceAction::SIGNIN, params.user_intention);
ASSERT_TRUE(params.signin_info);
EXPECT_EQ(kGaiaID, params.signin_info->account_info.gaia_id);
EXPECT_EQ(kEmail, params.signin_info->account_info.email);
EXPECT_EQ(kSessionIndex, params.signin_info->account_info.session_index);
EXPECT_EQ(kAuthorizationCode, params.signin_info->authorization_code);
+ EXPECT_EQ(kSupportedTokenBindingAlgorithms,
+ params.signin_info->supported_algorithms_for_token_binding);
histogram_tester.ExpectUniqueSample("Signin.DiceAuthorizationCode", true,
1);
}
@@ -667,6 +672,47 @@
}
}
+TEST_F(SigninHeaderHelperTest,
+ BuildDiceSigninResponseParamsNotEligibleForTokenBinding) {
+ const char kAuthorizationCode[] = "authorization_code";
+ const char kEmail[] = "foo@example.com";
+ const char kGaiaID[] = "gaia_id";
+ const int kSessionIndex = 42;
+
+ // "eligible_for_token_binding" is missing.
+ DiceResponseParams params = BuildDiceSigninResponseParams(base::StringPrintf(
+ "action=SIGNIN,id=%s,email=%s,authuser=%i,authorization_code=%s", kGaiaID,
+ kEmail, kSessionIndex, kAuthorizationCode));
+ EXPECT_EQ(DiceAction::SIGNIN, params.user_intention);
+ ASSERT_TRUE(params.signin_info);
+ EXPECT_TRUE(
+ params.signin_info->supported_algorithms_for_token_binding.empty());
+}
+
+// Mainly tests that whitespace characters in the middle of a header don't break
+// parsing.
+TEST_F(SigninHeaderHelperTest, BuildDiceSigninResponseParamsMixedOrder) {
+ const char kAuthorizationCode[] = "authorization_code";
+ const char kEmail[] = "foo@example.com";
+ const char kGaiaID[] = "gaia_id";
+ const char kSupportedTokenBindingAlgorithms[] = "ES256 RS256";
+ const int kSessionIndex = 42;
+
+ DiceResponseParams params = BuildDiceSigninResponseParams(base::StringPrintf(
+ "id=%s,action=SIGNIN,authuser=%i,eligible_for_token_binding=%s,email=%s,"
+ "authorization_code=%s",
+ kGaiaID, kSessionIndex, kSupportedTokenBindingAlgorithms, kEmail,
+ kAuthorizationCode));
+ EXPECT_EQ(DiceAction::SIGNIN, params.user_intention);
+ ASSERT_TRUE(params.signin_info);
+ EXPECT_EQ(kGaiaID, params.signin_info->account_info.gaia_id);
+ EXPECT_EQ(kEmail, params.signin_info->account_info.email);
+ EXPECT_EQ(kSessionIndex, params.signin_info->account_info.session_index);
+ EXPECT_EQ(kAuthorizationCode, params.signin_info->authorization_code);
+ EXPECT_EQ(kSupportedTokenBindingAlgorithms,
+ params.signin_info->supported_algorithms_for_token_binding);
+}
+
#endif // BUILDFLAG(ENABLE_DICE_SUPPORT)
// Tests that the Mirror header request is returned normally when the redirect
