Add exponential backoff for managed Kerberos account addition

Add retry logic with exponential backoff to KerberosCredentialsManager
for addition of managed accounts. This will cover the cases when the
first attempts fail because network is not ready yet, for example.
Network instabilities are common on first user login because
OpenNetworkConfigurations often changes network config on ChromeOS.

Bug: 1049331
Change-Id: I74b91cfd6fda3fc923061050a97fcb791761815e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2039453
Commit-Queue: Felipe Andrade <fsandrade@chromium.org>
Reviewed-by: Maksim Ivanov <emaxx@chromium.org>
Cr-Commit-Position: refs/heads/master@{#746481}

chrome/browser/chromeos/kerberos/kerberos_credentials_manager.cc

@@ -56,6 +56,17 @@ constexpr char kDefaultKerberosConfig[] = R"([libdefaults]
   permitted_enctypes = aes256-cts-hmac-sha1-96 aes128-cts-hmac-sha1-96
   forwardable = true)";
 
+// Backoff policy used to control managed accounts addition retries.
+const net::BackoffEntry::Policy kBackoffPolicyForManagedAccounts = {
+    0,               // Number of initial errors to ignore without backoff.
+    1 * 1000,        // Initial delay for backoff in ms: 1 second.
+    2,               // Factor to multiply for exponential backoff.
+    0,               // Fuzzing percentage.
+    10 * 60 * 1000,  // Maximum time to delay requests in ms: 10 minutes.
+    -1,              // Don't discard entry even if unused.
+    false            // Don't use initial delay unless the last was an error.
+};
+
 // If |principal_name| is "UsEr@realm.com", sets |principal_name| to
 // "user@REALM.COM". Returns false if the given name has no @ or one of the
 // parts is empty.
@@ -93,6 +104,13 @@ bool Succeeded(kerberos::ErrorType error) {
   return error == kerberos::ERROR_NONE;
 }
 
+bool ShouldRetry(kerberos::ErrorType error) {
+  // The error types that should trigger a managed accounts addition retry.
+  return error == kerberos::ERROR_NETWORK_PROBLEM ||
+         error == kerberos::ERROR_CONTACTING_KDC_FAILED ||
+         error == kerberos::ERROR_IN_PROGRESS;
+}
+
 }  // namespace
 
 // Encapsulates the steps to add a Kerberos account. Overview of the flow:
@@ -278,7 +296,8 @@ KerberosCredentialsManager::KerberosCredentialsManager(PrefService* local_state,
       primary_profile_(primary_profile),
       kerberos_files_handler_(std::make_unique<KerberosFilesHandler>(
           base::BindRepeating(&KerberosCredentialsManager::GetKerberosFiles,
-                              base::Unretained(this)))) {
+                              base::Unretained(this)))),
+      backoff_entry_for_managed_accounts_(&kBackoffPolicyForManagedAccounts) {
   DCHECK(primary_profile_);
   const user_manager::User* primary_user =
       chromeos::ProfileHelper::Get()->GetUserByProfile(primary_profile);
@@ -326,7 +345,7 @@ KerberosCredentialsManager::KerberosCredentialsManager(PrefService* local_state,
   pref_change_registrar_->Add(
       prefs::kKerberosAccounts,
       base::BindRepeating(&KerberosCredentialsManager::UpdateAccountsFromPref,
-                          weak_factory_.GetWeakPtr()));
+                          weak_factory_.GetWeakPtr(), false /* is_retry */));
 
   // Update accounts if policy is already available or start observing.
   policy_service_ =
@@ -335,7 +354,7 @@ KerberosCredentialsManager::KerberosCredentialsManager(PrefService* local_state,
       policy_service_->IsInitializationComplete(policy::POLICY_DOMAIN_CHROME);
   VLOG(1) << "Policy service initialized at startup: " << policy_initialized;
   if (policy_initialized)
-    UpdateAccountsFromPref();
+    UpdateAccountsFromPref(false /* is_retry */);
   else
     policy_service_->AddObserver(policy::POLICY_DOMAIN_CHROME, this);
 
@@ -398,7 +417,7 @@ void KerberosCredentialsManager::OnPolicyServiceInitialized(
   if (policy_service_->IsInitializationComplete(policy::POLICY_DOMAIN_CHROME)) {
     VLOG(1) << "Policy service initialized";
     policy_service_->RemoveObserver(policy::POLICY_DOMAIN_CHROME, this);
-    UpdateAccountsFromPref();
+    UpdateAccountsFromPref(false /* is_retry */);
   }
 }
 
@@ -472,6 +491,18 @@ void KerberosCredentialsManager::OnAddAccountRunnerDone(
 
 void KerberosCredentialsManager::OnAddManagedAccountRunnerDone(
     kerberos::ErrorType error) {
+  if (!managed_accounts_retry_timer_.IsRunning() && ShouldRetry(error)) {
+    backoff_entry_for_managed_accounts_.InformOfRequest(false);
+
+    if (backoff_entry_for_managed_accounts_.failure_count() <
+        kMaxFailureCountForManagedAccounts) {
+      managed_accounts_retry_timer_.Start(
+          FROM_HERE, backoff_entry_for_managed_accounts_.GetTimeUntilRelease(),
+          base::BindOnce(&KerberosCredentialsManager::UpdateAccountsFromPref,
+                         weak_factory_.GetWeakPtr(), true /* is_retry */));
+    }
+  }
+
   if (add_managed_account_callback_for_testing_) {
     add_managed_account_callback_for_testing_.Run(error);
   }
@@ -746,7 +777,7 @@ void KerberosCredentialsManager::UpdateEnabledFromPref() {
   if (IsKerberosEnabled()) {
     // Kerberos got enabled, re-populate managed accounts.
     VLOG(1) << "Kerberos got enabled, populating managed accounts";
-    UpdateAccountsFromPref();
+    UpdateAccountsFromPref(false /* is_retry */);
     return;
   }
 
@@ -781,7 +812,14 @@ void KerberosCredentialsManager::UpdateAddAccountsAllowedFromPref() {
                               EmptyResultCallback()));
 }
 
-void KerberosCredentialsManager::UpdateAccountsFromPref() {
+void KerberosCredentialsManager::UpdateAccountsFromPref(bool is_retry) {
+  if (is_retry) {
+    VLOG(1) << "Retrying to update KerberosAccounts from Prefs";
+  } else {
+    // Refreshing backoff entry, since this call was triggered by prefs change.
+    backoff_entry_for_managed_accounts_.Reset();
+  }
+
   if (!IsKerberosEnabled()) {
     VLOG(1) << "Kerberos disabled";
     NotifyRequiresLoginPassword(false);

chrome/browser/chromeos/kerberos/kerberos_credentials_manager.h

@@ -14,11 +14,13 @@
 #include "base/observer_list.h"
 #include "base/observer_list_types.h"
 #include "base/optional.h"
+#include "base/timer/timer.h"
 #include "chrome/browser/chromeos/authpolicy/kerberos_files_handler.h"
 #include "chromeos/dbus/kerberos/kerberos_service.pb.h"
 #include "components/keyed_service/core/keyed_service.h"
 #include "components/policy/core/common/policy_namespace.h"
 #include "components/policy/core/common/policy_service.h"
+#include "net/base/backoff_entry.h"
 
 class PrefRegistrySimple;
 class PrefService;
@@ -56,6 +58,9 @@ class KerberosCredentialsManager : public KeyedService,
     DISALLOW_COPY_AND_ASSIGN(Observer);
   };
 
+  // Maximum number of managed accounts addition retries per prefs change.
+  static constexpr int kMaxFailureCountForManagedAccounts = 10;
+
   KerberosCredentialsManager(PrefService* local_state,
                              Profile* primary_profile);
   ~KerberosCredentialsManager() override;
@@ -236,7 +241,7 @@ class KerberosCredentialsManager : public KeyedService,
   void UpdateEnabledFromPref();
   void UpdateRememberPasswordEnabledFromPref();
   void UpdateAddAccountsAllowedFromPref();
-  void UpdateAccountsFromPref();
+  void UpdateAccountsFromPref(bool is_retry);
 
   // Does the main work for UpdateAccountsFromPref(). To clean up stale managed
   // accounts, an up-to-date accounts list is needed. UpdateAccountsFromPref()
@@ -277,6 +282,12 @@ class KerberosCredentialsManager : public KeyedService,
   // List of objects that observe this instance.
   base::ObserverList<Observer, true /* check_empty */> observers_;
 
+  // Backoff entry used to control managed accounts addition retries.
+  net::BackoffEntry backoff_entry_for_managed_accounts_;
+
+  // Timer for keeping track of managed accounts addition retries.
+  base::OneShotTimer managed_accounts_retry_timer_;
+
   // Callback optionally used for testing.
   base::RepeatingCallback<void(kerberos::ErrorType)>
       add_managed_account_callback_for_testing_;

chromeos/dbus/kerberos/fake_kerberos_client.cc

@@ -121,14 +121,14 @@ void FakeKerberosClient::AddAccount(const kerberos::AddAccountRequest& request,
   if (it != accounts_.end()) {
     it->is_managed |= request.is_managed();
     PostResponse(std::move(callback), kerberos::ERROR_DUPLICATE_PRINCIPAL_NAME,
-                 mTaskDelay);
+                 task_delay_);
     return;
   }
 
   AccountData data(request.principal_name());
   data.is_managed = request.is_managed();
   accounts_.push_back(data);
-  PostResponse(std::move(callback), kerberos::ERROR_NONE, mTaskDelay);
+  PostResponse(std::move(callback), kerberos::ERROR_NONE, task_delay_);
 }
 
 void FakeKerberosClient::RemoveAccount(
@@ -146,7 +146,7 @@ void FakeKerberosClient::RemoveAccount(
   }
 
   MapAccountData(response.mutable_accounts());
-  PostProtoResponse(std::move(callback), response, mTaskDelay);
+  PostProtoResponse(std::move(callback), response, task_delay_);
 }
 
 void FakeKerberosClient::ClearAccounts(
@@ -183,7 +183,7 @@ void FakeKerberosClient::ClearAccounts(
   kerberos::ClearAccountsResponse response;
   MapAccountData(response.mutable_accounts());
   response.set_error(kerberos::ERROR_NONE);
-  PostProtoResponse(std::move(callback), response, mTaskDelay);
+  PostProtoResponse(std::move(callback), response, task_delay_);
 }
 
 void FakeKerberosClient::ListAccounts(
@@ -193,7 +193,7 @@ void FakeKerberosClient::ListAccounts(
   kerberos::ListAccountsResponse response;
   MapAccountData(response.mutable_accounts());
   response.set_error(kerberos::ERROR_NONE);
-  PostProtoResponse(std::move(callback), response, mTaskDelay);
+  PostProtoResponse(std::move(callback), response, task_delay_);
 }
 
 void FakeKerberosClient::SetConfig(const kerberos::SetConfigRequest& request,
@@ -202,19 +202,19 @@ void FakeKerberosClient::SetConfig(const kerberos::SetConfigRequest& request,
   AccountData* data = GetAccountData(request.principal_name());
   if (!data) {
     PostResponse(std::move(callback), kerberos::ERROR_UNKNOWN_PRINCIPAL_NAME,
-                 mTaskDelay);
+                 task_delay_);
     return;
   }
 
   kerberos::ConfigErrorInfo error_info =
       ValidateConfigLines(request.krb5conf());
   if (error_info.code() != kerberos::CONFIG_ERROR_NONE) {
-    PostResponse(std::move(callback), kerberos::ERROR_BAD_CONFIG, mTaskDelay);
+    PostResponse(std::move(callback), kerberos::ERROR_BAD_CONFIG, task_delay_);
     return;
   }
 
   data->krb5conf = request.krb5conf();
-  PostResponse(std::move(callback), kerberos::ERROR_NONE, mTaskDelay);
+  PostResponse(std::move(callback), kerberos::ERROR_NONE, task_delay_);
 }
 
 void FakeKerberosClient::ValidateConfig(
@@ -229,7 +229,7 @@ void FakeKerberosClient::ValidateConfig(
                          ? kerberos::ERROR_BAD_CONFIG
                          : kerberos::ERROR_NONE);
   *response.mutable_error_info() = std::move(error_info);
-  PostProtoResponse(std::move(callback), response, mTaskDelay);
+  PostProtoResponse(std::move(callback), response, task_delay_);
 }
 
 void FakeKerberosClient::AcquireKerberosTgt(
@@ -240,7 +240,7 @@ void FakeKerberosClient::AcquireKerberosTgt(
   AccountData* data = GetAccountData(request.principal_name());
   if (!data) {
     PostResponse(std::move(callback), kerberos::ERROR_UNKNOWN_PRINCIPAL_NAME,
-                 mTaskDelay);
+                 task_delay_);
     return;
   }
 
@@ -271,13 +271,21 @@ void FakeKerberosClient::AcquireKerberosTgt(
 
   // Reject empty passwords.
   if (password.empty()) {
-    PostResponse(std::move(callback), kerberos::ERROR_BAD_PASSWORD, mTaskDelay);
+    PostResponse(std::move(callback), kerberos::ERROR_BAD_PASSWORD,
+                 task_delay_);
+    return;
+  }
+
+  if (simulated_number_of_network_failures_ > 0) {
+    simulated_number_of_network_failures_--;
+    PostResponse(std::move(callback), kerberos::ERROR_NETWORK_PROBLEM,
+                 task_delay_);
     return;
   }
 
   // It worked! Magic!
   data->has_tgt = true;
-  PostResponse(std::move(callback), kerberos::ERROR_NONE, mTaskDelay);
+  PostResponse(std::move(callback), kerberos::ERROR_NONE, task_delay_);
 }
 
 void FakeKerberosClient::GetKerberosFiles(
@@ -287,7 +295,7 @@ void FakeKerberosClient::GetKerberosFiles(
   AccountData* data = GetAccountData(request.principal_name());
   if (!data) {
     PostResponse(std::move(callback), kerberos::ERROR_UNKNOWN_PRINCIPAL_NAME,
-                 mTaskDelay);
+                 task_delay_);
     return;
   }
 
@@ -297,7 +305,7 @@ void FakeKerberosClient::GetKerberosFiles(
     response.mutable_files()->set_krb5conf("Fake Kerberos configuration");
   }
   response.set_error(kerberos::ERROR_NONE);
-  PostProtoResponse(std::move(callback), response, mTaskDelay);
+  PostProtoResponse(std::move(callback), response, task_delay_);
 }
 
 void FakeKerberosClient::ConnectToKerberosFileChangedSignal(
@@ -315,7 +323,7 @@ void FakeKerberosClient::ConnectToKerberosTicketExpiringSignal(
 }
 
 void FakeKerberosClient::SetTaskDelay(base::TimeDelta delay) {
-  mTaskDelay = delay;
+  task_delay_ = delay;
 }
 
 void FakeKerberosClient::StartRecordingFunctionCalls() {
@@ -335,6 +343,11 @@ std::size_t FakeKerberosClient::GetNumberOfAccounts() const {
   return accounts_.size();
 }
 
+void FakeKerberosClient::SetSimulatedNumberOfNetworkFailures(
+    int number_of_failures) {
+  simulated_number_of_network_failures_ = number_of_failures;
+}
+
 void FakeKerberosClient::MaybeRecordFunctionCallForTesting(
     const char* function_name) {
   if (!recorded_function_calls_)

chromeos/dbus/kerberos/fake_kerberos_client.h

@@ -53,6 +53,7 @@ class COMPONENT_EXPORT(CHROMEOS_DBUS) FakeKerberosClient
   void StartRecordingFunctionCalls() override;
   std::string StopRecordingAndGetRecordedFunctionCalls() override;
   std::size_t GetNumberOfAccounts() const override;
+  void SetSimulatedNumberOfNetworkFailures(int number_of_failures) override;
 
  private:
   using RepeatedAccountField =
@@ -108,7 +109,11 @@ class COMPONENT_EXPORT(CHROMEOS_DBUS) FakeKerberosClient
   base::Optional<std::string> recorded_function_calls_;
 
   // Fake delay for any asynchronous operation.
-  base::TimeDelta mTaskDelay = base::TimeDelta::FromMilliseconds(100);
+  base::TimeDelta task_delay_ = base::TimeDelta::FromMilliseconds(100);
+
+  // The simulated number of network failures on |AcquireKerberosTgt()| (for
+  // testing).
+  int simulated_number_of_network_failures_ = 0;
 
   KerberosFilesChangedCallback kerberos_files_changed_callback_;
   KerberosTicketExpiringCallback kerberos_ticket_expiring_callback_;

chromeos/dbus/kerberos/kerberos_client.h

@@ -61,6 +61,12 @@ class COMPONENT_EXPORT(KERBEROS) KerberosClient {
     // Returns the number of accounts currently saved.
     virtual std::size_t GetNumberOfAccounts() const = 0;
 
+    // Sets the simulated number of network failures for |AcquireKerberosTgt()|.
+    // The default value is zero. This value should be set when testing the
+    // exponential backoff retry for adding managed accounts.
+    virtual void SetSimulatedNumberOfNetworkFailures(
+        int number_of_failures) = 0;
+
    protected:
     virtual ~TestInterface() {}
   };