From e007a15833548a6843e5e225fdafb66a4662d580 Mon Sep 17 00:00:00 2001 From: ryanml Date: Fri, 31 Jul 2020 15:18:36 -0700 Subject: [PATCH 1/3] Creating crypto_exchange module --- browser/BUILD.gn | 6 ++ components/crypto_exchange/browser/BUILD.gn | 14 +++++ .../browser/crypto_exchange_oauth_util.cc | 57 +++++++++++++++++++ .../browser/crypto_exchange_oauth_util.h | 20 +++++++ 4 files changed, 97 insertions(+) create mode 100644 components/crypto_exchange/browser/BUILD.gn create mode 100644 components/crypto_exchange/browser/crypto_exchange_oauth_util.cc create mode 100644 components/crypto_exchange/browser/crypto_exchange_oauth_util.h diff --git a/browser/BUILD.gn b/browser/BUILD.gn index 62b1c760960d..a5416d4225f0 100644 --- a/browser/BUILD.gn +++ b/browser/BUILD.gn @@ -312,6 +312,12 @@ source_set("browser_process") { ] } + if (gemini_enabled || binance_enabled) { + deps += [ + "//brave/components/crypto_exchange/browser", + ] + } + if (brave_together_enabled) { sources += [ "brave_together/brave_together_util.cc", diff --git a/components/crypto_exchange/browser/BUILD.gn b/components/crypto_exchange/browser/BUILD.gn new file mode 100644 index 000000000000..a8e63a9cd456 --- /dev/null +++ b/components/crypto_exchange/browser/BUILD.gn @@ -0,0 +1,14 @@ +source_set("browser") { + # Remove when https://github.com/brave/brave-browser/issues/10640 is resolved + check_includes = false + + sources = [ + "crypto_exchange_oauth_util.cc", + "crypto_exchange_oauth_util.h", + ] + + deps = [ + "//base", + "//crypto", + ] +} diff --git a/components/crypto_exchange/browser/crypto_exchange_oauth_util.cc b/components/crypto_exchange/browser/crypto_exchange_oauth_util.cc new file mode 100644 index 000000000000..ce07ff922cb7 --- /dev/null +++ b/components/crypto_exchange/browser/crypto_exchange_oauth_util.cc @@ -0,0 +1,57 @@ +/* Copyright (c) 2020 The Brave Authors. All rights reserved. + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this file, + * You can obtain one at http://mozilla.org/MPL/2.0/. */ + +#include + +#include "brave/components/crypto_exchange/browser/crypto_exchange_oauth_util.h" + +#include "base/base64.h" +#include "base/strings/string_number_conversions.h" +#include "crypto/random.h" +#include "crypto/sha2.h" + +namespace crypto_exchange { + +std::string GetCryptoRandomString(bool hex_encode) { + const size_t kSeedByteLength = 32; + uint8_t random_seed_bytes[kSeedByteLength]; + crypto::RandBytes(random_seed_bytes, kSeedByteLength); + + if (!hex_encode) { + std::string encoded_string; + base::Base64Encode( + reinterpret_cast(random_seed_bytes), &encoded_string); + return encoded_string; + } + + return base::HexEncode( + reinterpret_cast(random_seed_bytes), kSeedByteLength); +} + +std::string GetCodeChallenge( + const std::string& code_verifier, bool strip_chars) { + std::string code_challenge; + char raw[crypto::kSHA256Length] = {0}; + crypto::SHA256HashString(code_verifier, + raw, + crypto::kSHA256Length); + base::Base64Encode(base::StringPiece(raw, + crypto::kSHA256Length), + &code_challenge); + + if (strip_chars) { + std::replace(code_challenge.begin(), code_challenge.end(), '+', '-'); + std::replace(code_challenge.begin(), code_challenge.end(), '/', '_'); + + code_challenge.erase(std::find_if(code_challenge.rbegin(), + code_challenge.rend(), [](int ch) { + return ch != '='; + }).base(), code_challenge.end()); + } + + return code_challenge; +} + +} // namespace crypto_exchange diff --git a/components/crypto_exchange/browser/crypto_exchange_oauth_util.h b/components/crypto_exchange/browser/crypto_exchange_oauth_util.h new file mode 100644 index 000000000000..1db6d04fc744 --- /dev/null +++ b/components/crypto_exchange/browser/crypto_exchange_oauth_util.h @@ -0,0 +1,20 @@ +/* Copyright (c) 2020 The Brave Authors. All rights reserved. + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this file, + * You can obtain one at http://mozilla.org/MPL/2.0/. */ + +#ifndef BRAVE_COMPONENTS_CRYPTO_EXCHANGE_BROWSER_CRYPTO_EXCHANGE_OAUTH_UTIL_H_ +#define BRAVE_COMPONENTS_CRYPTO_EXCHANGE_BROWSER_CRYPTO_EXCHANGE_OAUTH_UTIL_H_ + +#include + +namespace crypto_exchange { + +std::string GetCryptoRandomString(bool hex_encode); + +std::string GetCodeChallenge( + const std::string& code_verifier, bool strip_chars); + +} + +#endif // BRAVE_COMPONENTS_CRYPTO_EXCHANGE_BROWSER_CRYPTO_EXCHANGE_OAUTH_UTIL_H_ From 38db33209d9141df3dde2894b0a7f163d6e475b8 Mon Sep 17 00:00:00 2001 From: ryanml Date: Fri, 31 Jul 2020 15:18:43 -0700 Subject: [PATCH 2/3] Fixes brave/brave-browser#11043 - Uses PKCE flow for Gemini auth Using new shared utility --- components/binance/browser/binance_service.cc | 39 ++----------------- components/binance/browser/binance_service.h | 1 - components/gemini/browser/gemini_service.cc | 22 +++++------ components/gemini/browser/gemini_service.h | 2 + 4 files changed, 15 insertions(+), 49 deletions(-) diff --git a/components/binance/browser/binance_service.cc b/components/binance/browser/binance_service.cc index 8fc3d4bc78b0..9e996021b8b0 100644 --- a/components/binance/browser/binance_service.cc +++ b/components/binance/browser/binance_service.cc @@ -14,7 +14,6 @@ #include "base/containers/flat_set.h" #include "base/files/file_enumerator.h" #include "base/files/file_util.h" -#include "base/strings/string_number_conversions.h" #include "base/strings/utf_string_conversions.h" #include "base/task/post_task.h" #include "base/task_runner_util.h" @@ -22,14 +21,13 @@ #include "base/token.h" #include "brave/common/pref_names.h" #include "brave/components/binance/browser/binance_json_parser.h" +#include "brave/components/crypto_exchange/browser/crypto_exchange_oauth_util.h" #include "components/country_codes/country_codes.h" #include "components/os_crypt/os_crypt.h" #include "components/prefs/pref_service.h" #include "components/user_prefs/user_prefs.h" #include "content/public/browser/browser_context.h" #include "content/public/browser/storage_partition.h" -#include "crypto/random.h" -#include "crypto/sha2.h" #include "net/base/load_flags.h" #include "net/base/url_util.h" #include "services/network/public/cpp/shared_url_loader_factory.h" @@ -72,15 +70,6 @@ GURL GetURLWithPath(const std::string& host, const std::string& path) { return GURL(std::string(url::kHttpsScheme) + "://" + host).Resolve(path); } -std::string GetHexEncodedCryptoRandomSeed() { - const size_t kSeedByteLength = 32; - // crypto::RandBytes is fail safe. - uint8_t random_seed_bytes[kSeedByteLength]; - crypto::RandBytes(random_seed_bytes, kSeedByteLength); - return base::HexEncode( - reinterpret_cast(random_seed_bytes), kSeedByteLength); -} - } // namespace BinanceService::BinanceService(content::BrowserContext* context) @@ -98,36 +87,14 @@ BinanceService::BinanceService(content::BrowserContext* context) BinanceService::~BinanceService() { } -// static -std::string BinanceService::GetCodeChallenge(const std::string& code_verifier) { - std::string code_challenge; - char raw[crypto::kSHA256Length] = {0}; - crypto::SHA256HashString(code_verifier, - raw, - crypto::kSHA256Length); - base::Base64Encode(base::StringPiece(raw, - crypto::kSHA256Length), - &code_challenge); - - // Binance expects the following conversions for the base64 encoded value: - std::replace(code_challenge.begin(), code_challenge.end(), '+', '-'); - std::replace(code_challenge.begin(), code_challenge.end(), '/', '_'); - code_challenge.erase(std::find_if(code_challenge.rbegin(), - code_challenge.rend(), [](int ch) { - return ch != '='; - }).base(), code_challenge.end()); - - return code_challenge; -} - std::string BinanceService::GetOAuthClientUrl() { // The code_challenge_ value is derived from the code_verifier value. // Step 1 of the oauth process uses the code_challenge_ value. // Step 4 of the oauth process uess the code_verifer_. // We never need to persist these values, they are just used to get an // access token. - code_verifier_ = GetHexEncodedCryptoRandomSeed(); - code_challenge_ = GetCodeChallenge(code_verifier_); + code_verifier_ = ::crypto_exchange::GetCryptoRandomString(true); + code_challenge_ = crypto_exchange::GetCodeChallenge(code_verifier_, true); GURL url(oauth_url); url = net::AppendQueryParameter(url, "response_type", "code"); diff --git a/components/binance/browser/binance_service.h b/components/binance/browser/binance_service.h index bdd19b3c73b5..26bf7bba8ea5 100644 --- a/components/binance/browser/binance_service.h +++ b/components/binance/browser/binance_service.h @@ -91,7 +91,6 @@ class BinanceService : public KeyedService { std::string GetBinanceTLD(); std::string GetOAuthClientUrl(); - static std::string GetCodeChallenge(const std::string& code_verifier); void SetAuthToken(const std::string& auth_token); private: diff --git a/components/gemini/browser/gemini_service.cc b/components/gemini/browser/gemini_service.cc index 744c0f6084ae..85f813d20e90 100644 --- a/components/gemini/browser/gemini_service.cc +++ b/components/gemini/browser/gemini_service.cc @@ -14,12 +14,12 @@ #include "base/files/file_enumerator.h" #include "base/files/file_util.h" #include "base/json/json_writer.h" -#include "base/strings/string_number_conversions.h" #include "base/strings/utf_string_conversions.h" #include "base/task/post_task.h" #include "base/task_runner_util.h" #include "base/time/time.h" #include "base/token.h" +#include "brave/components/crypto_exchange/browser/crypto_exchange_oauth_util.h" #include "brave/components/gemini/browser/gemini_json_parser.h" #include "brave/components/gemini/browser/pref_names.h" #include "components/os_crypt/os_crypt.h" @@ -27,7 +27,6 @@ #include "components/user_prefs/user_prefs.h" #include "content/public/browser/browser_context.h" #include "content/public/browser/storage_partition.h" -#include "crypto/random.h" #include "net/base/load_flags.h" #include "net/base/url_util.h" #include "services/network/public/cpp/shared_url_loader_factory.h" @@ -106,15 +105,6 @@ namespace { return encoded_payload; } - std::string GetEncodedCryptoRandomCSRF() { - uint8_t random_seed_bytes[24]; - crypto::RandBytes(random_seed_bytes, 24); - std::string encoded_csrf; - base::Base64Encode( - reinterpret_cast(random_seed_bytes), &encoded_csrf); - return encoded_csrf; - } - } // namespace GeminiService::GeminiService(content::BrowserContext* context) @@ -135,11 +125,16 @@ GeminiService::~GeminiService() { std::string GeminiService::GetOAuthClientUrl() { GURL url(oauth_url); + code_verifier_ = crypto_exchange::GetCryptoRandomString(false); + code_challenge_ = crypto_exchange::GetCodeChallenge(code_verifier_, false); url = net::AppendQueryParameter(url, "response_type", "code"); url = net::AppendQueryParameter(url, "client_id", client_id_); url = net::AppendQueryParameter(url, "redirect_uri", oauth_callback); url = net::AppendQueryParameter(url, "scope", oauth_scope); - url = net::AppendQueryParameter(url, "state", GetEncodedCryptoRandomCSRF()); + url = net::AppendQueryParameter(url, "code_challenge", code_challenge_); + url = net::AppendQueryParameter(url, "code_challenge_method", "S256"); + url = net::AppendQueryParameter( + url, "state", ::crypto_exchange::GetCryptoRandomString(false)); return url.spec(); } @@ -157,6 +152,7 @@ bool GeminiService::GetAccessToken(AccessTokenCallback callback) { dict.SetStringKey("client_secret", client_secret_); dict.SetStringKey("code", auth_token_); dict.SetStringKey("redirect_uri", oauth_callback); + dict.SetStringKey("code_verifier", code_verifier_); dict.SetStringKey("grant_type", "authorization_code"); std::string request_body = CreateJSONRequestBody(dict); @@ -278,6 +274,8 @@ void GeminiService::OnRevokeAccessToken( const std::map& headers) { bool success = status >= 200 && status <= 299; if (success) { + code_challenge_ = ""; + code_verifier_ = ""; ResetAccessTokens(); } std::move(callback).Run(success); diff --git a/components/gemini/browser/gemini_service.h b/components/gemini/browser/gemini_service.h index a16578394d97..4f71f3237d9d 100644 --- a/components/gemini/browser/gemini_service.h +++ b/components/gemini/browser/gemini_service.h @@ -143,6 +143,8 @@ class GeminiService : public KeyedService { std::string auth_token_; std::string access_token_; std::string refresh_token_; + std::string code_challenge_; + std::string code_verifier_; std::string client_id_; std::string client_secret_; std::string oauth_host_; From 0ad27827e9b35b8343b1107da27a12a764d6198d Mon Sep 17 00:00:00 2001 From: ryanml Date: Fri, 31 Jul 2020 15:45:40 -0700 Subject: [PATCH 3/3] Adding utility test --- browser/BUILD.gn | 3 +- components/binance/browser/BUILD.gn | 1 + components/binance/browser/binance_service.cc | 2 +- .../browser/binance_service_browsertest.cc | 9 ------ components/crypto_exchange/browser/BUILD.gn | 4 +++ .../browser/buildflags/BUILD.gn | 9 ++++++ .../browser/buildflags/buildflags.gni | 7 ++++ .../crypto_exchange_oauth_util_unittest.cc | 32 +++++++++++++++++++ components/gemini/browser/BUILD.gn | 1 + .../browser/gemini_service_browsertest.cc | 6 ++++ test/BUILD.gn | 10 ++++++ 11 files changed, 73 insertions(+), 11 deletions(-) create mode 100644 components/crypto_exchange/browser/buildflags/BUILD.gn create mode 100644 components/crypto_exchange/browser/buildflags/buildflags.gni create mode 100644 components/crypto_exchange/browser/crypto_exchange_oauth_util_unittest.cc diff --git a/browser/BUILD.gn b/browser/BUILD.gn index a5416d4225f0..e99d8d79c286 100644 --- a/browser/BUILD.gn +++ b/browser/BUILD.gn @@ -1,6 +1,7 @@ import("//brave/build/config.gni") import("//brave/components/binance/browser/buildflags/buildflags.gni") import("//brave/components/brave_together/buildflags/buildflags.gni") +import("//brave/components/crypto_exchange/browser/buildflags/buildflags.gni") import("//brave/components/gemini/browser/buildflags/buildflags.gni") import("//brave/browser/tor/buildflags/buildflags.gni") import("//brave/components/brave_ads/browser/buildflags/buildflags.gni") @@ -312,7 +313,7 @@ source_set("browser_process") { ] } - if (gemini_enabled || binance_enabled) { + if (crypto_exchanges_enabled) { deps += [ "//brave/components/crypto_exchange/browser", ] diff --git a/components/binance/browser/BUILD.gn b/components/binance/browser/BUILD.gn index b1bc8434234a..8597afca1f61 100644 --- a/components/binance/browser/BUILD.gn +++ b/components/binance/browser/BUILD.gn @@ -21,6 +21,7 @@ source_set("browser") { deps = [ "//base", "//brave/common:pref_names", + "//brave/components/crypto_exchange/browser", "//components/country_codes:country_codes", "//components/keyed_service/content", "//components/keyed_service/core", diff --git a/components/binance/browser/binance_service.cc b/components/binance/browser/binance_service.cc index 9e996021b8b0..5426b57b8f5f 100644 --- a/components/binance/browser/binance_service.cc +++ b/components/binance/browser/binance_service.cc @@ -93,7 +93,7 @@ std::string BinanceService::GetOAuthClientUrl() { // Step 4 of the oauth process uess the code_verifer_. // We never need to persist these values, they are just used to get an // access token. - code_verifier_ = ::crypto_exchange::GetCryptoRandomString(true); + code_verifier_ = crypto_exchange::GetCryptoRandomString(true); code_challenge_ = crypto_exchange::GetCodeChallenge(code_verifier_, true); GURL url(oauth_url); diff --git a/components/binance/browser/binance_service_browsertest.cc b/components/binance/browser/binance_service_browsertest.cc index b56274cc2140..0d9a203f5dec 100644 --- a/components/binance/browser/binance_service_browsertest.cc +++ b/components/binance/browser/binance_service_browsertest.cc @@ -457,15 +457,6 @@ class BinanceAPIBrowserTest : public InProcessBrowserTest { std::unique_ptr https_server_; }; - -IN_PROC_BROWSER_TEST_F(BinanceAPIBrowserTest, GetCodeChallenge) { - std::string verifier = - "FA87A1758E149A8BCD3A6D43DEAFAA013BCE2F132639ADA66C5BF101"; - ASSERT_EQ( - "1vw-WOmdXSW7OHQPgnuMsZjhaQKxi3LO5L7uX0YEtHs", - BinanceService::GetCodeChallenge(verifier)); -} - IN_PROC_BROWSER_TEST_F(BinanceAPIBrowserTest, GetOAuthClientURL) { EXPECT_TRUE(NavigateToNewTabUntilLoadStop()); auto* service = GetBinanceService(); diff --git a/components/crypto_exchange/browser/BUILD.gn b/components/crypto_exchange/browser/BUILD.gn index a8e63a9cd456..b878499ae5d7 100644 --- a/components/crypto_exchange/browser/BUILD.gn +++ b/components/crypto_exchange/browser/BUILD.gn @@ -1,3 +1,7 @@ +import("//brave/components/crypto_exchange/browser/buildflags/buildflags.gni") + +assert(crypto_exchanges_enabled) + source_set("browser") { # Remove when https://github.com/brave/brave-browser/issues/10640 is resolved check_includes = false diff --git a/components/crypto_exchange/browser/buildflags/BUILD.gn b/components/crypto_exchange/browser/buildflags/BUILD.gn new file mode 100644 index 000000000000..24288139562c --- /dev/null +++ b/components/crypto_exchange/browser/buildflags/BUILD.gn @@ -0,0 +1,9 @@ +import("//build/buildflag_header.gni") +import("//brave/components/crypto_exchange/browser/buildflags/buildflags.gni") + +buildflag_header("buildflags") { + header = "buildflags.h" + flags = [ + "CRYPTO_EXCHANGES_ENABLED=$crypto_exchanges_enabled", + ] +} diff --git a/components/crypto_exchange/browser/buildflags/buildflags.gni b/components/crypto_exchange/browser/buildflags/buildflags.gni new file mode 100644 index 000000000000..5ed2c4a4a2de --- /dev/null +++ b/components/crypto_exchange/browser/buildflags/buildflags.gni @@ -0,0 +1,7 @@ +import("//brave/build/config.gni") +import("//brave/components/binance/browser/buildflags/buildflags.gni") +import("//brave/components/gemini/browser/buildflags/buildflags.gni") + +declare_args() { + crypto_exchanges_enabled = binance_enabled || gemini_enabled +} diff --git a/components/crypto_exchange/browser/crypto_exchange_oauth_util_unittest.cc b/components/crypto_exchange/browser/crypto_exchange_oauth_util_unittest.cc new file mode 100644 index 000000000000..af3b8a6afd19 --- /dev/null +++ b/components/crypto_exchange/browser/crypto_exchange_oauth_util_unittest.cc @@ -0,0 +1,32 @@ +/* Copyright (c) 2020 The Brave Authors. All rights reserved. + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this file, + * You can obtain one at http://mozilla.org/MPL/2.0/. */ + +#include "brave/components/crypto_exchange/browser/crypto_exchange_oauth_util.h" + +#include "chrome/test/base/chrome_render_view_host_test_harness.h" + +// npm run test -- brave_unit_tests --filter=CryptoExchangeOauthUtilTest.* + +namespace { + +typedef testing::Test CryptoExchangeOauthUtilTest; + +TEST_F(CryptoExchangeOauthUtilTest, GetCodeChallengeStripChars) { + std::string verifier = + "FA87A1758E149A8BCD3A6D43DEAFAA013BCE2F132639ADA66C5BF101"; + ASSERT_EQ( + "1vw-WOmdXSW7OHQPgnuMsZjhaQKxi3LO5L7uX0YEtHs", + crypto_exchange::GetCodeChallenge(verifier, true)); +} + +TEST_F(CryptoExchangeOauthUtilTest, GetCodeChallengeNoStripChars) { + std::string verifier = + "aGVsbG9fd29ybGRfdGhpc19pc19hX3Rlc3Q="; + ASSERT_EQ( + "mTWSN0meBbs9rauVM4rSmWDYVKTWFhkFeECqn6W2ZC0=", + crypto_exchange::GetCodeChallenge(verifier, false)); +} + +} // namespace diff --git a/components/gemini/browser/BUILD.gn b/components/gemini/browser/BUILD.gn index 192e1cf6c570..e7b41a2c6c96 100644 --- a/components/gemini/browser/BUILD.gn +++ b/components/gemini/browser/BUILD.gn @@ -23,6 +23,7 @@ source_set("browser") { deps = [ "//base", + "//brave/components/crypto_exchange/browser", "//components/prefs:prefs", "//components/keyed_service/content", "//components/keyed_service/core", diff --git a/components/gemini/browser/gemini_service_browsertest.cc b/components/gemini/browser/gemini_service_browsertest.cc index 1d48b61b1f65..4e69f3756c63 100644 --- a/components/gemini/browser/gemini_service_browsertest.cc +++ b/components/gemini/browser/gemini_service_browsertest.cc @@ -314,11 +314,17 @@ IN_PROC_BROWSER_TEST_F(GeminiAPIBrowserTest, GetOAuthClientURL) { "client_id=fake-client-id&" "redirect_uri=com.brave.gemini%3A%2F%2Fauthorization&" "scope=addresses%3Aread%2Cbalances%3Aread%2Corders%3Acreate&" + "code_challenge=da0KASk6XZX4ksgvIGAa87iwNSVvmWdys2GYh3kjBZw&" + "code_challenge_method=S256&" "state=placeholder"); client_url = net::AppendOrReplaceQueryParameter(client_url, "state", "fake-state"); + client_url = net::AppendOrReplaceQueryParameter(client_url, "code_challenge", + "fake-challenge"); expected_url = net::AppendOrReplaceQueryParameter(expected_url, "state", "fake-state"); + expected_url = net::AppendOrReplaceQueryParameter(expected_url, + "code_challenge", "fake-challenge"); ASSERT_EQ(expected_url, client_url); } diff --git a/test/BUILD.gn b/test/BUILD.gn index 0326f5fdf5da..4dea2335a9f1 100644 --- a/test/BUILD.gn +++ b/test/BUILD.gn @@ -4,6 +4,7 @@ import("//brave/browser/tor/buildflags/buildflags.gni") import("//brave/browser/translate/buildflags/buildflags.gni") import("//brave/components/brave_ads/browser/buildflags/buildflags.gni") import("//brave/components/binance/browser/buildflags/buildflags.gni") +import("//brave/components/crypto_exchange/browser/buildflags/buildflags.gni") import("//brave/components/gemini/browser/buildflags/buildflags.gni") import("//brave/components/brave_referrals/buildflags/buildflags.gni") import("//brave/components/brave_rewards/browser/buildflags/buildflags.gni") @@ -274,6 +275,15 @@ test("brave_unit_tests") { ] } + if (crypto_exchanges_enabled) { + sources += [ + "//brave/components/crypto_exchange/browser/crypto_exchange_oauth_util_unittest.cc" + ] + deps += [ + "//brave/components/crypto_exchange/browser" + ] + } + if (is_linux) { configs += [ "//brave/build/linux:linux_channel_names",