Commit 2b8136fc authored by Maksim Ivanov's avatar Maksim Ivanov Committed by Commit Bot

Pass AccountId to PIN dialogs during user login

Pipe the authenticating user's AccountId down to the UI-related code
that shows security token PIN dialogs, so that the UI layer has ability
to correctly tie the PIN UI with the needed user.

Before this CL, the UI code has to assume that the currently selected
user is the one that for which the PIN request should be shown. However,
this assumption is unreliable in general: for example, the user could
have already clicked a different user pod before the PIN request
arrived, and as a result the PIN dialog would be shown on a wrong user
pod.

This CL extracts the AccountId based on the AccountIdentifier
information supplied by the cryptohomed to the
org.chromium.CryptohomeKeyDelegateInterface D-Bus service exposed by
the browser.

Bug: 983103
Change-Id: I55999c64f3c9af29fa8c231210ad3282643da885
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1742172
Commit-Queue: Maksim Ivanov <emaxx@chromium.org>
Reviewed-by: default avatarIgor <igorcov@chromium.org>
Reviewed-by: default avatarRyo Hashimoto <hashimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#685548}
parent 7bcbafc1
......@@ -258,8 +258,9 @@ void CertificateProviderService::SSLPrivateKey::SignDigestOnServiceTaskRunner(
std::move(callback).Run(net::ERR_FAILED, no_signature);
return;
}
service->RequestSignatureFromExtension(extension_id, certificate, algorithm,
input, std::move(callback));
service->RequestSignatureFromExtension(
extension_id, certificate, algorithm, input,
/*authenticating_user_account_id=*/{}, std::move(callback));
}
void CertificateProviderService::SSLPrivateKey::Sign(
......@@ -428,6 +429,7 @@ void CertificateProviderService::RequestSignatureBySpki(
const std::string& subject_public_key_info,
uint16_t algorithm,
base::span<const uint8_t> digest,
const base::Optional<AccountId>& authenticating_user_account_id,
net::SSLPrivateKey::SignCallback callback) {
DCHECK(thread_checker_.CalledOnValidThread());
bool is_currently_provided = false;
......@@ -442,7 +444,8 @@ void CertificateProviderService::RequestSignatureBySpki(
}
RequestSignatureFromExtension(extension_id, info.certificate, algorithm,
digest, std::move(callback));
digest, authenticating_user_account_id,
std::move(callback));
}
bool CertificateProviderService::GetSupportedAlgorithmsBySpki(
......@@ -526,11 +529,14 @@ void CertificateProviderService::RequestSignatureFromExtension(
const scoped_refptr<net::X509Certificate>& certificate,
uint16_t algorithm,
base::span<const uint8_t> digest,
const base::Optional<AccountId>& authenticating_user_account_id,
net::SSLPrivateKey::SignCallback callback) {
DCHECK(thread_checker_.CalledOnValidThread());
const int sign_request_id =
sign_requests_.AddRequest(extension_id, certificate, std::move(callback));
pin_dialog_manager_.AddSignRequestId(extension_id, sign_request_id,
authenticating_user_account_id);
if (!delegate_->DispatchSignRequestToExtension(
extension_id, sign_request_id, algorithm, certificate, digest)) {
scoped_refptr<net::X509Certificate> local_certificate;
......
......@@ -18,12 +18,14 @@
#include "base/memory/ref_counted.h"
#include "base/memory/weak_ptr.h"
#include "base/observer_list.h"
#include "base/optional.h"
#include "base/threading/thread_checker.h"
#include "chrome/browser/chromeos/certificate_provider/certificate_info.h"
#include "chrome/browser/chromeos/certificate_provider/certificate_requests.h"
#include "chrome/browser/chromeos/certificate_provider/pin_dialog_manager.h"
#include "chrome/browser/chromeos/certificate_provider/sign_requests.h"
#include "chrome/browser/chromeos/certificate_provider/thread_safe_certificate_map.h"
#include "components/account_id/account_id.h"
#include "components/keyed_service/core/keyed_service.h"
#include "net/cert/x509_certificate.h"
#include "net/ssl/client_cert_identity.h"
......@@ -170,9 +172,11 @@ class CertificateProviderService : public KeyedService {
// key. |algorithm| is a TLS 1.3 SignatureScheme value. See net::SSLPrivateKey
// for details. |callback| will be run with the reply of the extension or an
// error.
void RequestSignatureBySpki(const std::string& subject_public_key_info,
void RequestSignatureBySpki(
const std::string& subject_public_key_info,
uint16_t algorithm,
base::span<const uint8_t> digest,
const base::Optional<AccountId>& authenticating_user_account_id,
net::SSLPrivateKey::SignCallback callback);
// Looks up the certificate identified by |subject_public_key_info|. If any
......@@ -222,6 +226,7 @@ class CertificateProviderService : public KeyedService {
const scoped_refptr<net::X509Certificate>& certificate,
uint16_t algorithm,
base::span<const uint8_t> digest,
const base::Optional<AccountId>& authenticating_user_account_id,
net::SSLPrivateKey::SignCallback callback);
std::unique_ptr<Delegate> delegate_;
......
......@@ -112,7 +112,6 @@ bool DefaultDelegate::DispatchSignRequestToExtension(
return false;
api_cp::SignRequest request;
service_->pin_dialog_manager()->AddSignRequestId(extension_id, request_id);
request.sign_request_id = request_id;
switch (algorithm) {
case SSL_SIGN_RSA_PKCS1_MD5_SHA1:
......
......@@ -572,6 +572,7 @@ TEST_F(CertificateProviderServiceTest, SignUsingSpkiAsIdentification) {
std::vector<uint8_t> received_signature;
service_->RequestSignatureBySpki(
client1_spki, SSL_SIGN_RSA_PKCS1_SHA256, input,
/*authenticating_user_account_id=*/{},
base::BindOnce(&ExpectOKAndStoreSignature, &received_signature));
task_runner_->RunUntilIdle();
......
......@@ -18,11 +18,14 @@ PinDialogManager::PinDialogManager() = default;
PinDialogManager::~PinDialogManager() = default;
void PinDialogManager::AddSignRequestId(const std::string& extension_id,
int sign_request_id) {
void PinDialogManager::AddSignRequestId(
const std::string& extension_id,
int sign_request_id,
const base::Optional<AccountId>& authenticating_user_account_id) {
ExtensionNameRequestIdPair key(extension_id, sign_request_id);
// Cache the ID with current timestamp.
sign_request_times_[key] = base::Time::Now();
sign_requests_.insert(
std::make_pair(key, SignRequestState(/*begin_time=*/base::Time::Now(),
authenticating_user_account_id)));
}
PinDialogManager::RequestPinResult PinDialogManager::RequestPin(
......@@ -36,6 +39,12 @@ PinDialogManager::RequestPinResult PinDialogManager::RequestPin(
DCHECK_GE(attempts_left, -1);
const bool accept_input = (attempts_left != 0);
// Check the validity of sign_request_id.
const SignRequestState* const sign_request_state =
FindSignRequestState(extension_id, sign_request_id);
if (!sign_request_state)
return RequestPinResult::kInvalidId;
// Start from sanity checks, as the extension might have issued this call
// incorrectly.
if (active_dialog_state_) {
......@@ -50,22 +59,20 @@ PinDialogManager::RequestPinResult PinDialogManager::RequestPin(
return RequestPinResult::kDialogDisplayedAlready;
}
} else {
// Check the validity of sign_request_id
const ExtensionNameRequestIdPair key(extension_id, sign_request_id);
if (sign_request_times_.find(key) == sign_request_times_.end())
return RequestPinResult::kInvalidId;
// Check that the sign request hasn't timed out yet.
const base::Time current_time = base::Time::Now();
if (current_time - sign_request_times_[key] > kSignRequestIdTimeout)
if (current_time - sign_request_state->begin_time > kSignRequestIdTimeout)
return RequestPinResult::kInvalidId;
// A new dialog will be opened, so initialize the related internal state.
active_dialog_state_.emplace(GetHostForNewDialog(), extension_id,
extension_name, code_type);
extension_name, sign_request_id, code_type);
}
active_dialog_state_->request_pin_callback = std::move(callback);
active_dialog_state_->host->ShowSecurityTokenPinDialog(
extension_name, code_type, accept_input, error_label, attempts_left,
sign_request_state->authenticating_user_account_id,
base::BindOnce(&PinDialogManager::OnPinEntered,
weak_factory_.GetWeakPtr()),
base::BindOnce(&PinDialogManager::OnPinDialogClosed,
......@@ -92,11 +99,16 @@ PinDialogManager::StopPinRequestWithError(
return StopPinRequestResult::kNoUserInput;
}
const SignRequestState* const sign_request_state =
FindSignRequestState(extension_id, active_dialog_state_->sign_request_id);
if (!sign_request_state)
return StopPinRequestResult::kNoActiveDialog;
active_dialog_state_->stop_pin_request_callback = std::move(callback);
active_dialog_state_->host->ShowSecurityTokenPinDialog(
active_dialog_state_->extension_name, active_dialog_state_->code_type,
/*enable_user_input=*/false, error_label,
/*attempts_left=*/-1,
/*attempts_left=*/-1, sign_request_state->authenticating_user_account_id,
base::BindOnce(&PinDialogManager::OnPinEntered,
weak_factory_.GetWeakPtr()),
base::BindOnce(&PinDialogManager::OnPinDialogClosed,
......@@ -133,10 +145,9 @@ void PinDialogManager::ExtensionUnloaded(const std::string& extension_id) {
last_response_closed_[extension_id] = false;
for (auto it = sign_request_times_.cbegin();
it != sign_request_times_.cend();) {
for (auto it = sign_requests_.cbegin(); it != sign_requests_.cend();) {
if (it->first.first == extension_id)
sign_request_times_.erase(it++);
sign_requests_.erase(it++);
else
++it;
}
......@@ -156,18 +167,43 @@ void PinDialogManager::RemovePinDialogHost(
base::Erase(added_dialog_hosts_, pin_dialog_host);
}
PinDialogManager::SignRequestState::SignRequestState(
base::Time begin_time,
const base::Optional<AccountId>& authenticating_user_account_id)
: begin_time(begin_time),
authenticating_user_account_id(authenticating_user_account_id) {}
PinDialogManager::SignRequestState::SignRequestState(const SignRequestState&) =
default;
PinDialogManager::SignRequestState& PinDialogManager::SignRequestState::
operator=(const SignRequestState&) = default;
PinDialogManager::SignRequestState::~SignRequestState() = default;
PinDialogManager::ActiveDialogState::ActiveDialogState(
SecurityTokenPinDialogHost* host,
const std::string& extension_id,
const std::string& extension_name,
int sign_request_id,
SecurityTokenPinCodeType code_type)
: host(host),
extension_id(extension_id),
extension_name(extension_name),
sign_request_id(sign_request_id),
code_type(code_type) {}
PinDialogManager::ActiveDialogState::~ActiveDialogState() = default;
PinDialogManager::SignRequestState* PinDialogManager::FindSignRequestState(
const std::string& extension_id,
int sign_request_id) {
const ExtensionNameRequestIdPair key(extension_id, sign_request_id);
const auto sign_request_iter = sign_requests_.find(key);
if (sign_request_iter == sign_requests_.end())
return nullptr;
return &sign_request_iter->second;
}
void PinDialogManager::OnPinEntered(const std::string& user_input) {
DCHECK(!active_dialog_state_->stop_pin_request_callback);
last_response_closed_[active_dialog_state_->extension_id] = false;
......
......@@ -18,6 +18,7 @@
#include "chrome/browser/chromeos/certificate_provider/security_token_pin_dialog_host.h"
#include "chrome/browser/chromeos/certificate_provider/security_token_pin_dialog_host_popup_impl.h"
#include "chromeos/constants/security_token_pin_types.h"
#include "components/account_id/account_id.h"
namespace chromeos {
......@@ -49,7 +50,10 @@ class PinDialogManager final {
~PinDialogManager();
// Stores internally the |signRequestId| along with current timestamp.
void AddSignRequestId(const std::string& extension_id, int sign_request_id);
void AddSignRequestId(
const std::string& extension_id,
int sign_request_id,
const base::Optional<AccountId>& authenticating_user_account_id);
// Creates and displays a new PIN dialog, or reuses the old dialog with just
// updating the parameters if active one exists.
......@@ -113,11 +117,24 @@ class PinDialogManager final {
}
private:
struct SignRequestState {
SignRequestState(
base::Time begin_time,
const base::Optional<AccountId>& authenticating_user_account_id);
SignRequestState(const SignRequestState&);
SignRequestState& operator=(const SignRequestState&);
~SignRequestState();
base::Time begin_time;
base::Optional<AccountId> authenticating_user_account_id;
};
// Holds information related to the currently opened PIN dialog.
struct ActiveDialogState {
ActiveDialogState(SecurityTokenPinDialogHost* host,
const std::string& extension_id,
const std::string& extension_name,
int sign_request_id,
SecurityTokenPinCodeType code_type);
~ActiveDialogState();
......@@ -128,6 +145,7 @@ class PinDialogManager final {
const std::string extension_id;
const std::string extension_name;
const int sign_request_id;
const SecurityTokenPinCodeType code_type;
RequestPinCallback request_pin_callback;
StopPinRequestCallback stop_pin_request_callback;
......@@ -135,6 +153,10 @@ class PinDialogManager final {
using ExtensionNameRequestIdPair = std::pair<std::string, int>;
// Returns the sign request state for the given key, or null if not found.
SignRequestState* FindSignRequestState(const std::string& extension_id,
int sign_request_id);
// The callback that gets invoked once the user sends some input into the PIN
// dialog.
void OnPinEntered(const std::string& user_input);
......@@ -155,9 +177,9 @@ class PinDialogManager final {
// been exceeded.
std::unordered_map<std::string, bool> last_response_closed_;
// The map with extension_id and sign request id issued by Chrome as key while
// the time when the id was generated is the value.
std::map<ExtensionNameRequestIdPair, base::Time> sign_request_times_;
// The map from extension_id and an active sign request id to the state of the
// request.
std::map<ExtensionNameRequestIdPair, SignRequestState> sign_requests_;
SecurityTokenPinDialogHostPopupImpl default_dialog_host_;
// The list of dynamically added dialog hosts, in the same order as they were
......
......@@ -8,7 +8,9 @@
#include <string>
#include "base/callback_forward.h"
#include "base/optional.h"
#include "chromeos/constants/security_token_pin_types.h"
#include "components/account_id/account_id.h"
namespace chromeos {
......@@ -43,6 +45,8 @@ class SecurityTokenPinDialogHost {
// (note that a non-empty error does NOT disable the user input per se).
// |attempts_left| - when non-negative, the UI should indicate this number to
// the user; otherwise must be equal to -1.
// |authenticating_user_account_id| - when set, is the ID of the user whose
// authentication triggered this PIN request.
// |pin_entered_callback| - called when the user submits the input.
// |pin_dialog_closed_callback| - called when the dialog is closed (either by
// the user or programmatically; it's optional whether to call it after
......@@ -53,6 +57,7 @@ class SecurityTokenPinDialogHost {
bool enable_user_input,
SecurityTokenPinErrorLabel error_label,
int attempts_left,
const base::Optional<AccountId>& authenticating_user_account_id,
SecurityTokenPinEnteredCallback pin_entered_callback,
SecurityTokenPinDialogClosedCallback pin_dialog_closed_callback) = 0;
......
......@@ -50,6 +50,7 @@ void SecurityTokenPinDialogHostPopupImpl::ShowSecurityTokenPinDialog(
bool enable_user_input,
SecurityTokenPinErrorLabel error_label,
int attempts_left,
const base::Optional<AccountId>& /*authenticating_user_account_id*/,
SecurityTokenPinEnteredCallback pin_entered_callback,
SecurityTokenPinDialogClosedCallback pin_dialog_closed_callback) {
DCHECK(!caller_extension_name.empty());
......
......@@ -38,6 +38,7 @@ class SecurityTokenPinDialogHostPopupImpl final
bool enable_user_input,
SecurityTokenPinErrorLabel error_label,
int attempts_left,
const base::Optional<AccountId>& authenticating_user_account_id,
SecurityTokenPinEnteredCallback pin_entered_callback,
SecurityTokenPinDialogClosedCallback pin_dialog_closed_callback) override;
void CloseSecurityTokenPinDialog() override;
......
......@@ -16,8 +16,10 @@
#include "chrome/browser/chromeos/certificate_provider/certificate_provider_service_factory.h"
#include "chrome/browser/chromeos/profiles/profile_helper.h"
#include "chrome/browser/profiles/profile.h"
#include "chromeos/cryptohome/cryptohome_parameters.h"
#include "chromeos/dbus/cryptohome/key.pb.h"
#include "chromeos/dbus/cryptohome/rpc.pb.h"
#include "components/account_id/account_id.h"
#include "dbus/message.h"
#include "net/base/net_errors.h"
#include "third_party/boringssl/src/include/openssl/digest.h"
......@@ -104,6 +106,7 @@ void CompleteSignatureKeyChallenge(
void HandleSignatureKeyChallenge(
dbus::MethodCall* method_call,
const cryptohome::SignatureKeyChallengeRequestData& challenge_request_data,
const AccountId& account_id,
dbus::ExportedObject::ResponseSender response_sender) {
if (challenge_request_data.data_to_sign().empty()) {
response_sender.Run(dbus::ErrorResponse::FromMethodCall(
......@@ -168,6 +171,7 @@ void HandleSignatureKeyChallenge(
certificate_provider_service->RequestSignatureBySpki(
challenge_request_data.public_key_spki_der(), ssl_algorithm, digest,
account_id,
base::BindOnce(&CompleteSignatureKeyChallenge,
base::Unretained(method_call), response_sender));
}
......@@ -207,7 +211,14 @@ void CryptohomeKeyDelegateServiceProvider::HandleChallengeKey(
"Unable to parse AccountIdentifier from request"));
return;
}
// For now |account_identifier| is not used.
const AccountId account_id =
cryptohome::GetAccountIdFromAccountIdentifier(account_identifier);
if (!account_id.is_valid() ||
account_id.GetAccountType() == AccountType::UNKNOWN) {
response_sender.Run(dbus::ErrorResponse::FromMethodCall(
method_call, DBUS_ERROR_FAILED, "Unknown account"));
return;
}
cryptohome::KeyChallengeRequest request;
if (!reader.PopArrayOfBytesAsProto(&request)) {
......@@ -232,7 +243,7 @@ void CryptohomeKeyDelegateServiceProvider::HandleChallengeKey(
return;
}
HandleSignatureKeyChallenge(method_call, request.signature_request_data(),
response_sender);
account_id, response_sender);
return;
}
......
......@@ -26,6 +26,7 @@
#include "chromeos/dbus/cryptohome/rpc.pb.h"
#include "chromeos/dbus/services/service_provider_test_helper.h"
#include "components/account_id/account_id.h"
#include "components/user_manager/user_names.h"
#include "components/version_info/channel.h"
#include "content/public/browser/browser_context.h"
#include "dbus/message.h"
......@@ -183,7 +184,8 @@ class CryptohomeKeyDelegateServiceProviderTest
cryptohome::ChallengeSignatureAlgorithm signature_algorithm,
std::vector<uint8_t>* signature) {
const cryptohome::AccountIdentifier account_identifier =
cryptohome::CreateAccountIdentifierFromAccountId(EmptyAccountId());
cryptohome::CreateAccountIdentifierFromAccountId(
user_manager::StubAccountId());
cryptohome::KeyChallengeRequest request;
request.set_challenge_type(
cryptohome::KeyChallengeRequest::CHALLENGE_TYPE_SIGNATURE);
......
......@@ -173,7 +173,7 @@ class CertificateProviderRequestPinTest : public CertificateProviderApiTest {
void AddFakeSignRequest() {
cert_provider_service_->pin_dialog_manager()->AddSignRequestId(
extension_->id(), kFakeSignRequestId);
extension_->id(), kFakeSignRequestId, {});
}
void NavigateTo(const std::string& test_page_file_name) {
......
......@@ -41,6 +41,29 @@ const std::string GetCryptohomeId(const AccountId& account_id) {
return account_id.GetUserEmail();
}
AccountId LookupUserByCryptohomeId(const std::string& cryptohome_id) {
const std::vector<AccountId> known_account_ids =
user_manager::known_user::GetKnownAccountIds();
// A LOT of tests start with --login_user <user>, and not registering this
// user before. So we might have "known_user" entry without gaia_id.
for (const AccountId& known_id : known_account_ids) {
if (known_id.HasAccountIdKey() &&
known_id.GetAccountIdKey() == cryptohome_id) {
return known_id;
}
}
for (const AccountId& known_id : known_account_ids) {
if (known_id.GetUserEmail() == cryptohome_id) {
return known_id;
}
}
return user_manager::known_user::GetAccountId(
cryptohome_id, std::string() /* id */, AccountType::UNKNOWN);
}
} // anonymous namespace
Identification::Identification() = default;
......@@ -63,25 +86,7 @@ bool Identification::operator<(const Identification& right) const {
}
AccountId Identification::GetAccountId() const {
const std::vector<AccountId> known_account_ids =
user_manager::known_user::GetKnownAccountIds();
// A LOT of tests start with --login_user <user>, and not registing this user
// before. So we might have "known_user" entry without gaia_id.
for (const AccountId& known_id : known_account_ids) {
if (known_id.HasAccountIdKey() && known_id.GetAccountIdKey() == id_) {
return known_id;
}
}
for (const AccountId& known_id : known_account_ids) {
if (known_id.GetUserEmail() == id_) {
return known_id;
}
}
return user_manager::known_user::GetAccountId(id_, std::string() /* id */,
AccountType::UNKNOWN);
return LookupUserByCryptohomeId(id_);
}
AccountIdentifier CreateAccountIdentifierFromAccountId(const AccountId& id) {
......@@ -97,6 +102,11 @@ AccountIdentifier CreateAccountIdentifierFromIdentification(
return out;
}
AccountId GetAccountIdFromAccountIdentifier(
const AccountIdentifier& account_identifier) {
return LookupUserByCryptohomeId(account_identifier.account_id());
}
KeyDefinition::AuthorizationData::Secret::Secret() : encrypt(false),
sign(false),
wrapped(false) {
......
......@@ -62,6 +62,11 @@ COMPONENT_EXPORT(CHROMEOS_CRYPTOHOME)
AccountIdentifier CreateAccountIdentifierFromIdentification(
const Identification& id);
// Look up known user for the given AccountIdentifier and return its AccountId.
COMPONENT_EXPORT(CHROMEOS_CRYPTOHOME)
AccountId GetAccountIdFromAccountIdentifier(
const AccountIdentifier& account_identifier);
// Definition of the key (e.g. password) for the cryptohome.
// It contains authorization data along with extra parameters like permissions
// associated with this key.
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment