Skip to content

Commit

Permalink
Address Web Discovery feedback
Browse files Browse the repository at this point in the history
  • Loading branch information
@DJAndries
DJAndries committed Oct 30, 2024
1 parent 29baf27 commit 4844e82
Show file tree
Hide file tree
Showing 5 changed files with 50 additions and 55 deletions.
36 changes: 18 additions & 18 deletions components/web_discovery/browser/double_fetcher.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@
#include "brave/components/web_discovery/browser/request_queue.h"
#include "brave/components/web_discovery/browser/util.h"
#include "components/prefs/pref_service.h"
#include "net/http/http_status_code.h"
#include "services/network/public/cpp/header_util.h"
#include "services/network/public/cpp/shared_url_loader_factory.h"
#include "services/network/public/cpp/simple_url_loader.h"
#include "services/network/public/mojom/url_response_head.mojom.h"
Expand Down Expand Up @@ -99,7 +101,22 @@ void DoubleFetcher::OnFetchTimer(const base::Value& request_data) {
void DoubleFetcher::OnRequestComplete(
GURL url,
std::optional<std::string> response_body) {
auto result = ProcessCompletedRequest(&response_body);
bool result = false;
auto* response_info = url_loader_->ResponseInfo();
if (response_info) {
auto response_code = response_info->headers->response_code();
if (!network::IsSuccessfulStatus(response_code)) {
if (response_code >= net::HttpStatusCode::HTTP_BAD_REQUEST &&
response_code < net::HttpStatusCode::HTTP_INTERNAL_SERVER_ERROR) {
// Only retry failures due to server error
// Mark as 'successful' if not a 5xx error, so we don't retry
result = true;
}
response_body = std::nullopt;
} else {
result = true;
}
}

auto request_data = request_queue_.NotifyRequestComplete(result);

Expand All @@ -112,21 +129,4 @@ void DoubleFetcher::OnRequestComplete(
}
}

bool DoubleFetcher::ProcessCompletedRequest(
std::optional<std::string>* response_body) {
auto* response_info = url_loader_->ResponseInfo();
if (!response_body || !response_info) {
return false;
}
auto response_code = response_info->headers->response_code();
if (response_code < 200 || response_code >= 300) {
if (response_code >= 500) {
// Only retry failures due to server error
return false;
}
*response_body = std::nullopt;
}
return true;
}

} // namespace web_discovery
1 change: 0 additions & 1 deletion components/web_discovery/browser/double_fetcher.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,6 @@ class DoubleFetcher {
private:
void OnFetchTimer(const base::Value& request_data);
void OnRequestComplete(GURL url, std::optional<std::string> response_body);
bool ProcessCompletedRequest(std::optional<std::string>* response_body);

raw_ptr<PrefService> profile_prefs_;
raw_ptr<network::SharedURLLoaderFactory> shared_url_loader_factory_;
Expand Down
42 changes: 19 additions & 23 deletions components/web_discovery/browser/ecdh_aes.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,12 @@
* License, v. 2.0. If a copy of the MPL was not distributed with this file,
* You can obtain one at https://mozilla.org/MPL/2.0/. */

#ifdef UNSAFE_BUFFERS_BUILD
#pragma allow_unsafe_buffers
#endif

#include "brave/components/web_discovery/browser/ecdh_aes.h"

#include <array>

#include "base/base64.h"
#include "base/containers/span_writer.h"
#include "base/logging.h"
#include "base/ranges/algorithm.h"
#include "base/strings/string_number_conversions.h"
Expand All @@ -37,11 +34,6 @@ constexpr size_t kComponentOctSize = 32 * 2 + 1;
constexpr size_t kEncodedPubKeyAndIv = 1 + kComponentOctSize + kIvSize;
constexpr uint8_t kP256TypeByte = 0xea;

bssl::UniquePtr<EC_KEY> CreateECKey() {
return bssl::UniquePtr<EC_KEY>(
EC_KEY_new_by_curve_name(NID_X9_62_prime256v1));
}

} // namespace

AESEncryptResult::AESEncryptResult(std::vector<uint8_t> data,
Expand All @@ -53,9 +45,10 @@ AESEncryptResult::~AESEncryptResult() = default;
AESEncryptResult::AESEncryptResult(const AESEncryptResult&) = default;

std::optional<AESEncryptResult> DeriveAESKeyAndEncrypt(
const std::vector<uint8_t>& server_pub_key,
const std::vector<uint8_t>& data) {
auto client_private_key = CreateECKey();
const base::span<uint8_t> server_pub_key,
const base::span<uint8_t> data) {
bssl::UniquePtr<EC_KEY> client_private_key(
EC_KEY_new_by_curve_name(NID_X9_62_prime256v1));

if (!client_private_key) {
VLOG(1) << "Failed to init P-256 curve";
Expand All @@ -80,8 +73,8 @@ std::optional<AESEncryptResult> DeriveAESKeyAndEncrypt(
return std::nullopt;
}

uint8_t shared_key_material[kKeyMaterialSize];
if (!ECDH_compute_key(shared_key_material, kKeyMaterialSize,
std::array<uint8_t, kKeyMaterialSize> shared_key_material;
if (!ECDH_compute_key(shared_key_material.data(), kKeyMaterialSize,
server_public_point.get(), client_private_key.get(),
nullptr)) {
VLOG(1) << "Failed to set derive key via ECDH";
Expand All @@ -90,8 +83,7 @@ std::optional<AESEncryptResult> DeriveAESKeyAndEncrypt(

auto key_material_hash = crypto::SHA256Hash(shared_key_material);

auto aes_key = std::vector<uint8_t>(key_material_hash.begin(),
key_material_hash.begin() + kAesKeySize);
auto aes_key = base::span<uint8_t>(key_material_hash).first<kAesKeySize>();
auto* algo = EVP_aead_aes_128_gcm();

bssl::ScopedEVP_AEAD_CTX ctx;
Expand All @@ -117,18 +109,22 @@ std::optional<AESEncryptResult> DeriveAESKeyAndEncrypt(
output.resize(len);

std::array<uint8_t, kEncodedPubKeyAndIv> public_component_and_iv;
public_component_and_iv[0] = kP256TypeByte;
auto public_component_and_iv_writer =
base::SpanWriter(base::span(public_component_and_iv));
public_component_and_iv_writer.Write(kP256TypeByte);

auto component = public_component_and_iv_writer.Skip(kComponentOctSize);

if (!EC_POINT_point2oct(
EC_group_p256(), EC_KEY_get0_public_key(client_private_key.get()),
POINT_CONVERSION_UNCOMPRESSED, public_component_and_iv.data() + 1,
kComponentOctSize, nullptr)) {
if (!EC_POINT_point2oct(EC_group_p256(),
EC_KEY_get0_public_key(client_private_key.get()),
POINT_CONVERSION_UNCOMPRESSED, component->data(),
kComponentOctSize, nullptr)) {
VLOG(1) << "Failed to export EC public point/key";
return std::nullopt;
}

base::ranges::copy(iv.begin(), iv.end(),
public_component_and_iv.begin() + kComponentOctSize + 1);
public_component_and_iv_writer.Write(iv);
CHECK_EQ(public_component_and_iv_writer.remaining(), 0u);

return std::make_optional<AESEncryptResult>(
output, base::Base64Encode(public_component_and_iv));
Expand Down
6 changes: 4 additions & 2 deletions components/web_discovery/browser/ecdh_aes.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@
#include <string>
#include <vector>

#include "base/containers/span.h"

namespace web_discovery {

struct AESEncryptResult {
Expand All @@ -24,8 +26,8 @@ struct AESEncryptResult {
};

std::optional<AESEncryptResult> DeriveAESKeyAndEncrypt(
const std::vector<uint8_t>& server_pub_key,
const std::vector<uint8_t>& data);
const base::span<uint8_t> server_pub_key,
const base::span<uint8_t> data);

} // namespace web_discovery

Expand Down
20 changes: 9 additions & 11 deletions components/web_discovery/browser/reporter.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,6 @@
* License, v. 2.0. If a copy of the MPL was not distributed with this file,
* You can obtain one at https://mozilla.org/MPL/2.0/. */

#ifdef UNSAFE_BUFFERS_BUILD
#pragma allow_unsafe_buffers
#endif

#include "brave/components/web_discovery/browser/reporter.h"

#include <utility>
Expand All @@ -21,6 +17,8 @@
#include "brave/components/web_discovery/browser/signature_basename.h"
#include "brave/components/web_discovery/browser/util.h"
#include "crypto/sha2.h"
#include "net/http/http_status_code.h"
#include "services/network/public/cpp/header_util.h"
#include "services/network/public/cpp/resource_request_body.h"
#include "services/network/public/cpp/shared_url_loader_factory.h"
#include "services/network/public/cpp/simple_url_loader.h"
Expand Down Expand Up @@ -100,7 +98,8 @@ std::optional<AESEncryptResult> CompressAndEncrypt(
uLongf compressed_data_size = compressBound(full_signed_message.size());
std::vector<uint8_t> compressed_data(compressed_data_size + 2);
if (zlib_internal::CompressHelper(
zlib_internal::ZLIB, compressed_data.data() + 2,
zlib_internal::ZLIB,
base::span(compressed_data).last(compressed_data_size).data(),
&compressed_data_size, full_signed_message.data(),
full_signed_message.size(), Z_DEFAULT_COMPRESSION, nullptr,
nullptr) != Z_OK) {
Expand All @@ -114,7 +113,7 @@ std::optional<AESEncryptResult> CompressAndEncrypt(
return std::nullopt;
}
base::ranges::copy(base::U16ToBigEndian(compressed_data_size),
compressed_data.begin());
compressed_data.data());
compressed_data[0] |= kCompressedMessageId;
return DeriveAESKeyAndEncrypt(server_pub_key, compressed_data);
}
Expand Down Expand Up @@ -208,9 +207,8 @@ void Reporter::OnRequestSigned(std::string final_payload_json,
final_payload_json.size());
base::SpanWriter<uint8_t> message_writer(full_signed_message);
if (!message_writer.WriteU8BigEndian(kSignedMessageId) ||
!message_writer.Write(base::span<uint8_t>(
reinterpret_cast<uint8_t*>(final_payload_json.data()),
final_payload_json.size())) ||
!message_writer.Write(std::vector<uint8_t>(final_payload_json.begin(),
final_payload_json.end())) ||
!message_writer.Write(base::DoubleToBigEndian(basename_count)) ||
!message_writer.Write(*signature)) {
VLOG(1) << "Failed to pack signed message";
Expand Down Expand Up @@ -273,8 +271,8 @@ bool Reporter::ValidateResponse(
return false;
}
auto response_code = headers->response_code();
if (response_code < 200 || response_code >= 300) {
if (response_code >= 500) {
if (!network::IsSuccessfulStatus(response_code)) {
if (response_code >= net::HttpStatusCode::HTTP_INTERNAL_SERVER_ERROR) {
// Only retry failures due to server error
return false;
}
Expand Down

0 comments on commit 4844e82

Please sign in to comment.