Implement Tor client updater as component extension #316
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,61 @@ | ||
| /* 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/browser/extensions/brave_component_extension.h" | ||
|
|
||
| #include <string> | ||
|
|
||
| #include "base/bind.h" | ||
| #include "base/bind_helpers.h" | ||
| #include "base/callback.h" | ||
| #include "brave/browser/component_updater/brave_component_installer.h" | ||
| #include "chrome/browser/browser_process.h" | ||
|
|
||
| void ComponentsUI::OnDemandUpdate( | ||
| component_updater::ComponentUpdateService* cus, | ||
| const std::string& component_id) { | ||
| cus->GetOnDemandUpdater().OnDemandUpdate( | ||
| component_id, component_updater::OnDemandUpdater::Priority::FOREGROUND, | ||
| component_updater::Callback()); | ||
| } | ||
|
|
||
| BraveComponentExtension::BraveComponentExtension() { | ||
| } | ||
|
|
||
| BraveComponentExtension::~BraveComponentExtension() { | ||
| } | ||
|
|
||
| void BraveComponentExtension::Register( | ||
| const std::string& component_name, | ||
| const std::string& component_id, | ||
| const std::string& component_base64_public_key) { | ||
| component_name_ = component_name; | ||
| component_id_ = component_id; | ||
| component_base64_public_key_ = component_base64_public_key; | ||
|
|
||
| base::Closure registered_callback = | ||
| base::Bind(&BraveComponentExtension::OnComponentRegistered, | ||
| base::Unretained(this), component_id_); | ||
| ReadyCallback ready_callback = | ||
| base::Bind(&BraveComponentExtension::OnComponentReady, | ||
| base::Unretained(this), component_id_); | ||
| brave::RegisterComponent(g_browser_process->component_updater(), | ||
| component_name_, component_base64_public_key_, | ||
| registered_callback, ready_callback); | ||
| } | ||
|
|
||
| // static | ||
| bool BraveComponentExtension::Unregister(const std::string& component_id) { | ||
| return g_browser_process->component_updater()->UnregisterComponent( | ||
| component_id); | ||
| } | ||
|
|
||
| void BraveComponentExtension::OnComponentRegistered(const std::string& component_id) { | ||
| OnDemandUpdate(g_browser_process->component_updater(), component_id); | ||
| } | ||
|
|
||
| void BraveComponentExtension::OnComponentReady( | ||
| const std::string& component_id, | ||
| const base::FilePath& install_dir) { | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| /* 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_BROWSER_EXTENSIONS_BRAVE_COMPONENT_EXTENSION_H_ | ||
| #define BRAVE_BROWSER_EXTENSIONS_BRAVE_COMPONENT_EXTENSION_H_ | ||
|
|
||
| #include "base/files/file_path.h" | ||
| #include "components/component_updater/component_updater_service.h" | ||
|
|
||
| // Just used to give access to OnDemandUpdater since it's private. | ||
| // Chromium has ComponentsUI which is a friend class, so we just | ||
| // do this hack here to gain access. | ||
| class ComponentsUI { | ||
| public: | ||
| void OnDemandUpdate(component_updater::ComponentUpdateService* cus, | ||
| const std::string& component_id); | ||
| }; | ||
|
|
||
| class BraveComponentExtension : public ComponentsUI { | ||
| public: | ||
| BraveComponentExtension(); | ||
| virtual ~BraveComponentExtension(); | ||
| void Register(const std::string& component_name, | ||
| const std::string& component_id, | ||
| const std::string& component_base64_public_key); | ||
| static bool Unregister(const std::string& component_id); | ||
|
|
||
| protected: | ||
| virtual void OnComponentRegistered(const std::string& component_id); | ||
| virtual void OnComponentReady(const std::string& component_id, | ||
| const base::FilePath& install_dir); | ||
|
|
||
| private: | ||
| std::string component_name_; | ||
| std::string component_id_; | ||
| std::string component_base64_public_key_; | ||
| }; | ||
|
|
||
| #endif // BRAVE_BROWSER_EXTENSIONS_BRAVE_COMPONENT_EXTENSION_H_ |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,84 @@ | ||
| /* 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/browser/extensions/brave_tor_client_updater.h" | ||
|
|
||
| #include "base/files/file_enumerator.h" | ||
| #include "base/files/file_path.h" | ||
| #include "base/task_scheduler/post_task.h" | ||
| #include "third_party/re2/src/re2/re2.h" | ||
|
|
||
| namespace extensions { | ||
|
|
||
| std::string BraveTorClientUpdater::g_tor_client_component_id_( | ||
| kTorClientComponentId); | ||
| std::string BraveTorClientUpdater::g_tor_client_component_base64_public_key_( | ||
| kTorClientComponentBase64PublicKey); | ||
|
|
||
| BraveTorClientUpdater::BraveTorClientUpdater() | ||
| : task_runner_( | ||
| base::CreateSequencedTaskRunnerWithTraits({base::MayBlock()})), | ||
| registered_(false) { | ||
| } | ||
|
|
||
| BraveTorClientUpdater::~BraveTorClientUpdater() { | ||
| } | ||
|
|
||
| void BraveTorClientUpdater::Register() { | ||
| if (registered_) | ||
| return; | ||
|
|
||
| BraveComponentExtension::Register(kTorClientComponentName, | ||
| g_tor_client_component_id_, | ||
| g_tor_client_component_base64_public_key_); | ||
| registered_ = true; | ||
| } | ||
|
|
||
| base::FilePath BraveTorClientUpdater::GetExecutablePath() const { | ||
| return executable_path_; | ||
| } | ||
|
|
||
| void BraveTorClientUpdater::InitExecutablePath( | ||
| const base::FilePath& install_dir) { | ||
| base::FileEnumerator traversal(install_dir, false, | ||
|
There was a problem hiding this comment. This should be flagged for security audit. We shouldn't be using a wildcard here to find files, we need to look for a specific file and match the contents against a checksum cc @diracdeltas There was a problem hiding this comment. Ah, good point. I implemented it like that because the Tor client embeds versioning and target platform information in its executable name (example: A checksum makes sense, but I think it would mean that we couldn't update the Tor client without also updating Brave. I think that's at least part of the problem the extension was meant to address. cc: @riastradh-brave and @bbondy for any other thoughts here. There was a problem hiding this comment. we do validate the crx so the checksum can be included along with the exe There was a problem hiding this comment. Thank you for flagging, I am assigning to @riastradh-brave for retroactive security review. There was a problem hiding this comment. I would be perfectly fine with hard-coding a hash -- that's what we do currently. As I understand it, the only real specific motivation we have for doing a separate extension is to avoid antivirus software that is unhappy if Brave is bundled with a tor executable -- but even then, I'm not sure have any reports of antivirus software complaining that it's there; all the reports I'm aware of are complaints that it got executed. There was a problem hiding this comment. Generally, we put out updates more frequently than tor does anyway. There was a problem hiding this comment. I see what's going on here. @emerick Can we just use a fixed pathname at a fixed location in the extension's install directory for the executable file? The only reason the file that is published via S3 has a different name is that we are publishing several different files, one for each platform. There was a problem hiding this comment. @riastradh-brave Yeah, I'll update the packager to use a more generic name for the executable and then we'll just look specifically for that. |
||
| base::FileEnumerator::FILES, | ||
| FILE_PATH_LITERAL("tor-*")); | ||
| for (base::FilePath current = traversal.Next(); !current.empty(); | ||
| current = traversal.Next()) { | ||
| base::FileEnumerator::FileInfo file_info = traversal.GetInfo(); | ||
| if (RE2::FullMatch(file_info.GetName().MaybeAsASCII(), | ||
| "tor-\\d+\\.\\d+\\.\\d+\\.\\d+-\\w+-brave-\\d+")) { | ||
| executable_path_ = current; | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| LOG(ERROR) << "Failed to locate Tor client executable in " | ||
| << install_dir.value().c_str(); | ||
| } | ||
|
|
||
| void BraveTorClientUpdater::OnComponentReady( | ||
| const std::string& component_id, | ||
| const base::FilePath& install_dir) { | ||
| GetTaskRunner()->PostTask( | ||
| FROM_HERE, base::Bind(&BraveTorClientUpdater::InitExecutablePath, | ||
| base::Unretained(this), install_dir)); | ||
| } | ||
|
|
||
| // static | ||
| void BraveTorClientUpdater::SetComponentIdAndBase64PublicKeyForTest( | ||
| const std::string& component_id, | ||
| const std::string& component_base64_public_key) { | ||
| g_tor_client_component_id_ = component_id; | ||
| g_tor_client_component_base64_public_key_ = component_base64_public_key; | ||
| } | ||
|
|
||
| /////////////////////////////////////////////////////////////////////////////// | ||
|
|
||
| // The Brave Tor client extension factory. | ||
| std::unique_ptr<BraveTorClientUpdater> BraveTorClientUpdaterFactory() { | ||
| return std::make_unique<BraveTorClientUpdater>(); | ||
| } | ||
|
|
||
| } // namespace extensions | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need this in BraveBrowserProcess? Seems like it could just live here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My thinking here was that
BraveBrowserProcesswould be the owner of long-lived services like the Tor client updater and then we'd just access it here via its getter so that we can register the extension on startup.Are you suggesting to move the
Registerfunction intoBraveExtensionManagement(or even the entire updater)? I haven't followed the code paths all the way through, but it seems like that could work. My only concerns would be that the registration happens at the correct time at startup (I definitely had a few issues with this before I settled on havingBraveBrowserProcessown them) and that the updaters are easily accessible to other parts of the code.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the tor client updater isn't a long-lived service, the component updater is the long-lived service and you are just registering things to be updated by it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think maybe there is some confusion here because of the naming. I would use the WidevineCDMComponentInstaller as an example for this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I see what you're saying about component updater. I did model this on some of the existing components (not specifically
WidevineCDMComponentInstaller, but something similar). In our code,BraveComponentInstalleris fulfilling the same role asWidevineCDMComponentInstaller(i.e., it implements the installer policy and provides a register function).It seems like making
BraveComponentInstallergeneric was a mistake; instead, it should be specific to this component and contain all of the logic that gets executed upon registration. Also, it seems like registration should occur inchrome_browser_mainor somewhere similar. Is that the approach you're thinking of?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably in BraveBrowserMainExtraParts::PreMainMessageLoopRun
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw we'll want this opt in and not always registered. Installed first time switch is turned on in a private tab.
@cezaraugusto can you track this work and do the fix for the private tab UI? You could land a dead switch control any time.