Skip to content

Commit

Permalink
[DBSC] Use RegistrationTokenHelper in the registration fetcher
Browse files Browse the repository at this point in the history 
The registration endpoint requires a challenge to be signed and attached
to the DBSC session registration request. This is step 3 of adding
support for handling the server-issued challenges. In this CL we use
`RegistrationTokenHelper` & challenge to produce the binding key and the
registration token in the registration fetcher.

Bug: b/293996619
Change-Id: I38b5108e6423eb3927ed6d8244caa403c2a4b6b3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4767858
Reviewed-by: Alex Ilin <alexilin@chromium.org>
Commit-Queue: Elizaveta Sheremetyeva <sheremetyeva@google.com>
Cr-Commit-Position: refs/heads/main@{#1185176}
  • Loading branch information
Elizaveta Sheremetyeva authored and Chromium LUCI CQ committed Aug 18, 2023
1 parent 547d218 commit cec005a
Show file tree
Hide file tree
Showing 5 changed files with 61 additions and 85 deletions.
72 changes: 21 additions & 51 deletions chrome/browser/signin/bound_session_credentials/bound_session_registration_fetcher_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,18 +37,6 @@ bound_session_credentials::RegistrationParams CreateRegistrationParams(
params.set_wrapped_key(wrapped_key);
return params;
}

std::string GetAlgoName(crypto::SignatureVerifier::SignatureAlgorithm algo) {
if (algo == crypto::SignatureVerifier::SignatureAlgorithm::ECDSA_SHA256) {
return "ES256";
}

if (algo == crypto::SignatureVerifier::SignatureAlgorithm::RSA_PKCS1_SHA256) {
return "RS256";
}

return std::string();
}
} // namespace

BoundSessionRegistrationFetcherImpl::BoundSessionRegistrationFetcherImpl(
Expand All @@ -65,12 +53,20 @@ BoundSessionRegistrationFetcherImpl::~BoundSessionRegistrationFetcherImpl() =
void BoundSessionRegistrationFetcherImpl::Start(
RegistrationCompleteCallback callback) {
callback_ = std::move(callback);
// TODO: Make `key_service_` not optional. It was made optional to facilitate
// testing but we now have a feature flag for software emulation, so this
// should be no longer needed.
if (key_service_) {
key_service_->GenerateSigningKeySlowlyAsync(
registration_params_.SupportedAlgos(),
unexportable_keys::BackgroundTaskPriority::kBestEffort,
base::BindOnce(&BoundSessionRegistrationFetcherImpl::OnKeyCreated,
weak_ptr_factory_.GetWeakPtr()));
// base::Unretained() is safe since `this` owns
// `registration_token_helper_`.
registration_token_helper_ =
RegistrationTokenHelper::CreateForSessionBinding(
*key_service_, registration_params_.Challenge(),
registration_params_.RegistrationEndpoint(),
base::BindOnce(&BoundSessionRegistrationFetcherImpl::
OnRegistrationTokenCreated,
base::Unretained(this)));
registration_token_helper_->Start();
} else {
// Early fail of request, object is invalid after this
std::move(callback_).Run(absl::nullopt);
Expand Down Expand Up @@ -127,40 +123,21 @@ void BoundSessionRegistrationFetcherImpl::OnURLLoaderComplete(
std::move(callback_).Run(return_value);
}

void BoundSessionRegistrationFetcherImpl::OnKeyCreated(
unexportable_keys::ServiceErrorOr<unexportable_keys::UnexportableKeyId>
created_key) {
if (!created_key.has_value()) {
std::move(callback_).Run(absl::nullopt);
return;
}
unexportable_keys::UnexportableKeyId key_id = *created_key;

unexportable_keys::ServiceErrorOr<std::vector<uint8_t>> public_key =
key_service_->GetSubjectPublicKeyInfo(key_id);
std::string pkey(reinterpret_cast<const char*>(public_key->data()),
public_key->size());
std::string pkey_base64;
base::Base64Encode(pkey, &pkey_base64);

unexportable_keys::ServiceErrorOr<
crypto::SignatureVerifier::SignatureAlgorithm>
algo_used = key_service_->GetAlgorithm(key_id);
std::string algo_string = GetAlgoName(algo_used.value());
if (algo_string.empty()) {
void BoundSessionRegistrationFetcherImpl::OnRegistrationTokenCreated(
absl::optional<RegistrationTokenHelper::Result> result) {
if (!result.has_value()) {
std::move(callback_).Run(absl::nullopt);
return;
}

std::vector<uint8_t> wrapped_key = *key_service_->GetWrappedKey(key_id);
const std::vector<uint8_t>& wrapped_key = result->wrapped_binding_key;
wrapped_key_str_ = std::string(wrapped_key.begin(), wrapped_key.end());

StartFetchingRegistration(std::move(pkey_base64), std::move(algo_string));
StartFetchingRegistration(result->registration_token);
}

void BoundSessionRegistrationFetcherImpl::StartFetchingRegistration(
std::string public_key,
std::string algo_used) {
const std::string& registration_token) {
net::NetworkTrafficAnnotationTag traffic_annotation =
net::DefineNetworkTrafficAnnotation("device_bound_session_register",
R"(
Expand Down Expand Up @@ -210,18 +187,11 @@ void BoundSessionRegistrationFetcherImpl::StartFetchingRegistration(
net::IsolationInfo::CreateForInternalRequest(
url::Origin::Create(registration_params_.RegistrationEndpoint()));

std::string body = *base::WriteJson(
base::Value::Dict()
.Set("binding_alg", std::move(algo_used)) // Algorithm
.Set("key",
std::move(public_key)) // Public key
.Set("client_constraints",
base::Value::Dict().Set("signature_quota_per_minute", 1)));
std::string content_type = "application/json";
std::string content_type = "application/jwt";

url_loader_ =
network::SimpleURLLoader::Create(std::move(request), traffic_annotation);
url_loader_->AttachStringForUpload(body, content_type);
url_loader_->AttachStringForUpload(registration_token, content_type);
url_loader_->SetRetryOptions(
3, network::SimpleURLLoader::RETRY_ON_NETWORK_CHANGE);
url_loader_->DownloadToString(
Expand Down
12 changes: 5 additions & 7 deletions chrome/browser/signin/bound_session_credentials/bound_session_registration_fetcher_impl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
#include "base/memory/weak_ptr.h"
#include "chrome/browser/signin/bound_session_credentials/bound_session_cookie_refresh_service.h"
#include "chrome/browser/signin/bound_session_credentials/bound_session_registration_fetcher_param.h"
#include "chrome/browser/signin/bound_session_credentials/registration_token_helper.h"
#include "components/unexportable_keys/service_error.h"
#include "components/unexportable_keys/unexportable_key_id.h"
#include "services/network/public/cpp/simple_url_loader.h"
Expand Down Expand Up @@ -52,11 +53,10 @@ class BoundSessionRegistrationFetcherImpl

private:
void OnURLLoaderComplete(std::unique_ptr<std::string> response_body);
void OnKeyCreated(
unexportable_keys::ServiceErrorOr<unexportable_keys::UnexportableKeyId>
callback);
void OnRegistrationTokenCreated(
absl::optional<RegistrationTokenHelper::Result> result);

void StartFetchingRegistration(std::string public_key, std::string algo_used);
void StartFetchingRegistration(const std::string& registration_token);

BoundSessionRegistrationFetcherParam registration_params_;
const raw_ptr<unexportable_keys::UnexportableKeyService> key_service_;
Expand All @@ -65,10 +65,8 @@ class BoundSessionRegistrationFetcherImpl
// Non-null after a fetch has started.
std::unique_ptr<network::SimpleURLLoader> url_loader_;
const scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory_;
std::unique_ptr<RegistrationTokenHelper> registration_token_helper_;

RegistrationCompleteCallback callback_;

base::WeakPtrFactory<BoundSessionRegistrationFetcherImpl> weak_ptr_factory_{
this};
};
#endif // CHROME_BROWSER_SIGNIN_BOUND_SESSION_CREDENTIALS_BOUND_SESSION_REGISTRATION_FETCHER_IMPL_H_
21 changes: 19 additions & 2 deletions ...wser/signin/bound_session_credentials/bound_session_registration_fetcher_impl_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#include "base/test/test_future.h"
#include "chrome/browser/signin/bound_session_credentials/bound_session_cookie_refresh_service.h"
#include "chrome/browser/signin/bound_session_credentials/bound_session_registration_fetcher.h"
#include "components/signin/public/base/session_binding_test_utils.h"
#include "components/unexportable_keys/service_error.h"
#include "components/unexportable_keys/unexportable_key_service.h"
#include "components/unexportable_keys/unexportable_key_service_impl.h"
Expand Down Expand Up @@ -58,11 +59,20 @@ bound_session_credentials::RegistrationParams CreateTestRegistrationParams() {
void ConfigureURLLoaderFactoryForRegistrationResponse(
network::TestURLLoaderFactory* url_loader_factory,
const std::string response_body,
bool* out_made_download) {
bool* out_made_download,
std::string* request_body = nullptr) {
url_loader_factory->SetInterceptor(
base::BindLambdaForTesting([=](const network::ResourceRequest& request) {
*out_made_download = true;
ASSERT_TRUE(request.url.is_valid());
ASSERT_FALSE(request.request_body.get()->elements()->empty());
if (request_body) {
*request_body = std::string(request.request_body.get()
->elements()
->at(0)
.As<network::DataElementBytes>()
.AsStringPiece());
}
auto response_head = network::mojom::URLResponseHead::New();
response_head->headers =
base::MakeRefCounted<net::HttpResponseHeaders>("");
Expand Down Expand Up @@ -131,11 +141,12 @@ TEST_F(BoundSessionRegistrationFetcherImplTest, ValidInput) {
crypto::ScopedMockUnexportableKeyProvider scoped_mock_key_provider_;
network::TestURLLoaderFactory url_loader_factory;
bool made_download = false;
std::string request_body;

ConfigureURLLoaderFactoryForRegistrationResponse(
&url_loader_factory,
std::string(kXSSIPrefix) + std::string(kJSONRegistrationParams),
&made_download);
&made_download, &request_body);

BoundSessionRegistrationFetcherParam params =
BoundSessionRegistrationFetcherParam::CreateInstanceForTesting(
Expand Down Expand Up @@ -169,6 +180,12 @@ TEST_F(BoundSessionRegistrationFetcherImplTest, ValidInput) {
wrapped_key_to_key_id.GetCallback());
EXPECT_TRUE(wrapped_key_to_key_id.IsReady());
EXPECT_TRUE(wrapped_key_to_key_id.Get().has_value());

// Verify that the request body contains a valid registration token.
UnexportableKeyId key_id = wrapped_key_to_key_id.Get().value();
EXPECT_TRUE(signin::VerifyJwtSignature(
request_body, *unexportable_key_service().GetAlgorithm(key_id),
*unexportable_key_service().GetSubjectPublicKeyInfo(key_id)));
}

TEST_F(BoundSessionRegistrationFetcherImplTest, MissingXSSIPrefix) {
Expand Down
7 changes: 4 additions & 3 deletions chrome/browser/signin/bound_session_credentials/bound_session_registration_fetcher_param.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -94,11 +94,12 @@ BoundSessionRegistrationFetcherParam::MaybeCreateInstance(
}

if (base::EqualsCaseInsensitiveASCII(key, kChallengeItemKey)) {
if (!base::Base64UrlDecode(value,
base::Base64UrlDecodePolicy::DISALLOW_PADDING,
&challenge)) {
// Challenge will be eventually written into a `base::Value`, when
// generating the registration token, which is restricted to UTF8.
if (!base::IsStringUTF8AllowingNoncharacters(value)) {
return absl::nullopt;
}
challenge = value;
}
}

Expand Down
34 changes: 12 additions & 22 deletions ...ser/signin/bound_session_credentials/bound_session_registration_fetcher_param_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
// found in the LICENSE file.

#include "chrome/browser/signin/bound_session_credentials/bound_session_registration_fetcher_param.h"
#include "base/base64url.h"
#include "base/strings/strcat.h"
#include "base/test/bind.h"
#include "base/test/task_environment.h"
Expand All @@ -15,13 +14,6 @@

namespace {
constexpr char kChallenge[] = "test_challenge";

std::string Base64UrlEncode(base::StringPiece data) {
std::string output;
base::Base64UrlEncode(data, base::Base64UrlEncodePolicy::OMIT_PADDING,
&output);
return output;
}
} // namespace

class BoundSessionRegistrationFetcherParamTest : public testing::Test {
Expand All @@ -46,9 +38,9 @@ TEST_F(BoundSessionRegistrationFetcherParamTest, AllValid) {
auto response_headers = base::MakeRefCounted<net::HttpResponseHeaders>("");
response_headers->SetHeader(
"Sec-Session-Google-Registration",
base::StrCat({"registration=startsession; supported-alg=ES256,RS256; "
"challenge=",
Base64UrlEncode(kChallenge), ";"}));
base::StrCat(
{"registration=startsession; supported-alg=ES256,RS256; challenge=",
kChallenge, ";"}));
absl::optional<BoundSessionRegistrationFetcherParam> maybe_params =
BoundSessionRegistrationFetcherParam::MaybeCreateInstance(
registration_request, response_headers.get());
Expand All @@ -70,7 +62,7 @@ TEST_F(BoundSessionRegistrationFetcherParamTest, AllValidFullUrl) {
"Sec-Session-Google-Registration",
base::StrCat({"registration=https://accounts.google.com/"
"startsession; supported-alg=ES256,RS256; challenge=",
Base64UrlEncode(kChallenge), ";"}));
kChallenge, ";"}));
absl::optional<BoundSessionRegistrationFetcherParam> maybe_params =
BoundSessionRegistrationFetcherParam::MaybeCreateInstance(
registration_request, response_headers.get());
Expand All @@ -92,7 +84,7 @@ TEST_F(BoundSessionRegistrationFetcherParamTest, AllValidFullDifferentUrl) {
"Sec-Session-Google-Registration",
base::StrCat({"registration=https://accounts.different.url/"
"startsession; supported-alg=ES256,RS256; challenge=",
Base64UrlEncode(kChallenge), ";"}));
kChallenge, ";"}));
absl::optional<BoundSessionRegistrationFetcherParam> maybe_params =
BoundSessionRegistrationFetcherParam::MaybeCreateInstance(
registration_request, response_headers.get());
Expand All @@ -106,7 +98,7 @@ TEST_F(BoundSessionRegistrationFetcherParamTest, AllValidSwapAlgo) {
"Sec-Session-Google-Registration",
base::StrCat(
{"registration=startsession; supported-alg=RS256,ES256; challenge=",
Base64UrlEncode(kChallenge), ";"}));
kChallenge, ";"}));
absl::optional<BoundSessionRegistrationFetcherParam> maybe_params =
BoundSessionRegistrationFetcherParam::MaybeCreateInstance(
registration_request, response_headers.get());
Expand All @@ -128,7 +120,7 @@ TEST_F(BoundSessionRegistrationFetcherParamTest, AllValidOneAlgo) {
"Sec-Session-Google-Registration",
base::StrCat(
{"registration=startsession; supported-alg=RS256; challenge=",
Base64UrlEncode(kChallenge), ";"}));
kChallenge, ";"}));
absl::optional<BoundSessionRegistrationFetcherParam> maybe_params =
BoundSessionRegistrationFetcherParam::MaybeCreateInstance(
registration_request, response_headers.get());
Expand Down Expand Up @@ -158,7 +150,7 @@ TEST_F(BoundSessionRegistrationFetcherParamTest, MissingUrl) {
"Sec-Session-Google-Registration",
base::StrCat(
{"registration=startsession; supported-alg=ES256,RS256; challenge=",
Base64UrlEncode(kChallenge), ";"}));
kChallenge, ";"}));
absl::optional<BoundSessionRegistrationFetcherParam> maybe_params =
BoundSessionRegistrationFetcherParam::MaybeCreateInstance(
registration_request, response_headers.get());
Expand All @@ -171,7 +163,7 @@ TEST_F(BoundSessionRegistrationFetcherParamTest, MissingAlgo) {
response_headers->SetHeader(
"Sec-Session-Google-Registration",
base::StrCat({"registration=startsession; supported-alg=; challenge=",
Base64UrlEncode(kChallenge), ";"}));
kChallenge, ";"}));
absl::optional<BoundSessionRegistrationFetcherParam> maybe_params =
BoundSessionRegistrationFetcherParam::MaybeCreateInstance(
registration_request, response_headers.get());
Expand All @@ -183,8 +175,7 @@ TEST_F(BoundSessionRegistrationFetcherParamTest, MissingRegistration) {
auto response_headers = base::MakeRefCounted<net::HttpResponseHeaders>("");
response_headers->SetHeader(
"Sec-Session-Google-Registration",
base::StrCat({"supported-alg=ES256,RS256; challenge=",
Base64UrlEncode(kChallenge), ";"}));
base::StrCat({"supported-alg=ES256,RS256; challenge=", kChallenge, ";"}));
absl::optional<BoundSessionRegistrationFetcherParam> maybe_params =
BoundSessionRegistrationFetcherParam::MaybeCreateInstance(
registration_request, response_headers.get());
Expand Down Expand Up @@ -215,14 +206,13 @@ TEST_F(BoundSessionRegistrationFetcherParamTest, EmptyChallenge) {
ASSERT_FALSE(maybe_params.has_value());
}

TEST_F(BoundSessionRegistrationFetcherParamTest, ChallengeDecodingFailed) {
TEST_F(BoundSessionRegistrationFetcherParamTest, ChallengeInvalidUtf8) {
GURL registration_request = GURL("https://www.google.com/registration");
auto response_headers = base::MakeRefCounted<net::HttpResponseHeaders>("");
// Encoded challenge with characters not in the base64url alphabet ("/").
response_headers->SetHeader(
"Sec-Session-Google-Registration",
"registration=startsession; supported-alg=ES256,RS256; "
"challenge=ab/d;");
"challenge=ab\xC0\x80;");
absl::optional<BoundSessionRegistrationFetcherParam> maybe_params =
BoundSessionRegistrationFetcherParam::MaybeCreateInstance(
registration_request, response_headers.get());
Expand Down

0 comments on commit cec005a

Please sign in to comment.