Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove VPN services; only install after customer purchases #20754

Merged
merged 28 commits into from
Jan 18, 2024

Conversation

bsclifton
Copy link
Member

@bsclifton bsclifton commented Oct 30, 2023


Removal of services happen when mini-installer executes for system level installs. The install worker is executed for new users but also runs on each upgrade.

Install of services will happen only once a person has purchased the Brave VPN product and has credentials.
Services will then be installed (DNS and WireGuard).

Fixes brave/brave-browser#33726

Submitter Checklist:

  • I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally:
    • npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
    • npm run lint, npm run presubmit wiki, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

Clean install test plan

Some steps are given using DEVELOPMENT (ex: CI) builds. Official builds will be similar but will look like Release channel builds (orange lion instead of gray lion; naming doesn't have channel name in it, etc).

Installing an official build (ex: using stub installer)

If using a final binary (ex: QA testing), install the release candidate as a user that has admin access. Make sure a UAC prompt is shown when launching the installer.

Installing a CI / Developer build (ex: a build from this pull request)

  1. Download the brave_installer.exe. Employees can join #browser-artifacts-bot in Slack and search for 20754 (the pull request number) and get the latest Win64 binary.

    NOTE: It must be a signed binary - otherwise the WireGuard functionality won't work. It will look like it's connecting and then immediately disconnect. If that happens to you, you can view logs at C:\Windows\Temp\BraveVpn\tunnel\log.bin. Rename as TXT and if you're not using a signed binary, you will see something like:
    [TUN] [4517804174477276943.brave] Installing driver
    [TUN] [4517804174477276943.brave] Could not install driver C:\WINDOWS\Temp\9b17ffc3774ce8bbe054f3ed731ae977f03770e1b474eaa9c8d6ac6626f0e4aa\wireguard.inf to store: A certificate chain processed, but terminated in a root certificate which is not trusted by the trust provider. (Code 0x800B0109)
    [TUN] [4517804174477276943.brave] Unable to create network adapter: Error creating adapter: A certificate chain processed, but terminated in a root certificate which is not trusted by the trust provider.
    [TUN] [4517804174477276943.brave] Shutting down
    
  2. Open a command prompt (cmd.exe). Does NOT need to be an admin console.

    NOTE: This is required because of the binary built by CI. The real install (using a stub installer) will follow a different set of steps. Test plan will be updated for QA when this is approved/merged.
  3. Navigate via CLI to the folder that has the executable
  4. Install the executable using brave_installer.exe --system-level (or whatever the filename is)
  5. If you are using a regular cmd.exe instance, the OS will present a UAC prompt (escalating to admin). If you are using an admin command prompt the install will take place
  6. Close the command prompt

Rest of steps for Clean install test plan

Verifying behavior of what is expected for a new install with this fix.

  1. Open Add/Remove programs (Windows + R, appwiz.cpl)
  2. Verify the build shows. CI builds should have a GRAY icon. It should have today's date for Installed On
  3. Open services.msc and search for the elevation service.
    • For CI / developer builds, this will be Brave Development.
    • Verify there is only one service (ex: Brave Development Elevation Service (BraveDevelopmentElevationService))
  4. Launch into Brave
  5. Login to account.brave.com with an account that has VPN
  6. Click Refresh Brave VPN or buy VPN - something to get VPN into "purchased" state
  7. Refresh services.msc and verify there are two new services
    • For CI / developer builds this will be:
      • Brave Development Vpn Service (BraveDevelopmentVpnService)
      • Brave Development Vpn Wireguard Service (BraveDevelopmentVpnWireguardService)
    • Release builds will be the channel specific naming
      • Brave Nightly Vpn Service (BraveNightlyVpnService)
      • Brave Beta Vpn Service (BraveBetaVpnService)
      • Brave Vpn Service (BraveVpnService)
      • etc
  8. Close services.msc
  9. Verify you can use VPN and it works. It should be using WireGuard by default.
  10. Switch protocols on the VPN. It should be set to WireGuard; visit brave://settings/system and switch to IKEv2 by disabling Use WireGuard protocol in Brave VPN. This will require a restart of Brave.
  11. Verify IKEv2 works as expected

Upgrade scenario - removal of VPN service

Testing scenario for an existing Brave user. It should remove the VPN services.
Because of the way the code is written, the services will be removed for EVERYBODY. Even if they had purchased Brave VPN.

  1. Install a version of Brave that does NOT have this fix yet. It's important to install on a channel which you have access to a binary WITH the fix (ex: if latest on Nightly has fix but no version of Beta has it, test on Nightly). On the current Nightly (as of 12 Feb 2024) this is going to be version 1.64.3 or earlier. Click here to get 1.64.3 binaries.
  2. Open services.msc and search for the VPN services.
    • Verify both VPN services are shown. Name will be like:
    • Brave Nightly Vpn Service (BraveNightlyVpnService)
    • Brave Nightly Vpn Wireguard Service (BraveNightlyVpnWireguardService)
  3. Brave Nightly Elevation Service (BraveNightlyElevationService) will be there too.
    • Right click the service and pick Start
    • You should get an error like Windows could not start the Brave Nightly Elevation Service (BraveNightlyElevationService) service on Local Computer.
  4. Install the new version of Brave which has this fix.
  5. Refresh services.msc using either F5 or right click => refresh
  6. Verify the services you found in step 2 are gone now
  7. Try step 3 again. It should NOT have an error this time because now there is a binary in place for elevation service.

Upgrade scenario - User had Brave VPN before upgrade

  1. Have a profile which already has a Brave VPN subscription
  2. Run steps from Upgrade scenario - removal of VPN service
  3. The VPN button will still be visible in the browser. Click it to bring up the server connection screen
  4. At this point and time, the services should be installed.
  5. Verify the VPN works
  6. Open services.msc and search for the VPN services.
    • Verify both VPN services are shown. Name will be like:
    • Brave Nightly Vpn Service (BraveNightlyVpnService)
    • Brave Nightly Vpn Wireguard Service (BraveNightlyVpnWireguardService)

Upgrade scenario - User buys Brave VPN after upgrade

  1. Have a profile which does NOT have Brave VPN
  2. Run steps from Upgrade scenario - removal of VPN service
  3. Login to account.brave.com with an account that has VPN
  4. Click Refresh Brave VPN or buy VPN - something to get VPN into "purchased" state
  5. At this point and time, the services should be installed.
  6. Verify the VPN works
  7. Open services.msc and search for the VPN services.
    • Verify both VPN services are shown. Name will be like:
    • Brave Nightly Vpn Service (BraveNightlyVpnService)
    • Brave Nightly Vpn Wireguard Service (BraveNightlyVpnWireguardService)

@bsclifton bsclifton added CI/skip-android Do not run CI builds for Android CI/skip-macos-x64 Do not run CI builds for macOS x64 CI/skip-ios Do not run CI builds for iOS unused-CI/skip-linux-x64 Do not run CI builds for Linux x64 labels Oct 30, 2023
@bsclifton bsclifton self-assigned this Oct 30, 2023
@bsclifton bsclifton requested a review from a team as a code owner October 30, 2023 22:05
@bsclifton bsclifton force-pushed the bsc-windows-vpn-remove-services branch from 871df5d to 9d96d6e Compare October 31, 2023 07:50
@bsclifton bsclifton requested a review from a team as a code owner October 31, 2023 07:50
@bsclifton bsclifton force-pushed the bsc-windows-vpn-remove-services branch from 9d96d6e to 807afff Compare October 31, 2023 08:40
@bsclifton
Copy link
Member Author

bsclifton commented Nov 1, 2023

Verified what's in the PR so far 👍

Basically, if you take a download from any other pull request (something only Brave employees can do) and open an admin command prompt, you can install it with brave_installer_119_1_62_12.exe --system-level. I grabbed a stock development binary (DOES NOT HAVE THE CHANGE FROM THIS PULL REQUEST) and verified it installs the BraveDevelopmentVpnService and BraveDevelopmentVpnWireguardService in services.msc. I also verified this installs the registry key for the tray icon at HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\Windows\CurrentVersion\Run.

Then I downloaded the packaged executable for this build and installed that on top of this with brave_installer_119_1_62_13.exe --system-level. The update ran and this ran the install_worker logic. BraveDevelopmentVpnService and BraveDevelopmentVpnWireguardService were removed from services.msc. The tray icon program was removed from HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\Windows\CurrentVersion\Run.

Now I will focus on:

  • detecting the user should install Brave VPN dependencies (ex: they have purchased and have credentials)
  • installing the service when that condition is met
  • making sure the upgrade logic doesn't remove the service if someone is using it

@bsclifton bsclifton requested a review from a team as a code owner November 1, 2023 07:48
@bsclifton bsclifton force-pushed the bsc-windows-vpn-remove-services branch 2 times, most recently from 18874f3 to 526f3a8 Compare November 1, 2023 17:02
@bsclifton bsclifton force-pushed the bsc-windows-vpn-remove-services branch from ee4dcec to 5ce01c1 Compare November 3, 2023 09:06
@bsclifton bsclifton force-pushed the bsc-windows-vpn-remove-services branch 9 times, most recently from fd226cf to ed975f1 Compare November 8, 2023 07:29
@bsclifton bsclifton force-pushed the bsc-windows-vpn-remove-services branch 4 times, most recently from c393148 to f8b22f5 Compare November 9, 2023 18:38
bsclifton and others added 9 commits January 16, 2024 15:08
Matches the `*Installed` check used by VPN helper service
Method is now passed in when connection object is created in browser
process.
Updates code to use that code too (removing from the service).

This also removes installation that could happen on brave://settings/system
This settings install use-case is obsolete now because the services will be
installed once VPN is actually in use.

This use-case would only happen if the person had VPN but it was set to
IKEv2 and they did not have the services installed. For example, a user
install (not a system install).
When system service installing is in-progress, wait till it's completed
and connect again.
@bsclifton bsclifton force-pushed the bsc-windows-vpn-remove-services branch from f69ee6c to ce8bb65 Compare January 16, 2024 22:08
@brave-builds
Copy link
Collaborator

A Storybook has been deployed to preview UI for the latest push

Copy link
Contributor

[puLL-Merge] - brave/brave-core@20754

Description

This pull request modifies the behavior of the Brave VPN feature within the Brave browser. The changes primarily revolve around the installation and uninstallation of system-level services necessary for the VPN to function, especially on Windows. Additionally, the changes include modifications to MIDl (Microsoft Interface Definition Language) files related to the elevation service, which is used to perform certain privileged operations on Windows.

Changes

Changes

  • .gitattributes

    • Added patterns to manage end-of-line normalization for VPN related files.
  • browser/brave_browser_process_impl.cc

    • Adjusted browser process implementation to incorporate VPN system services installation callback logic.
  • browser/brave_vpn/dns/brave_vpn_dns_observer_service_win.cc

    • Updated DNS observer service implementation with renamed headers and adjusted paths for VPN components.
  • browser/brave_vpn/sources.gni

    • Added new sources and dependencies to the build configuration for Brave VPN.
  • browser/brave_vpn/win/DEPS

    • Created new DEPS file declaring dependencies for the Win-specific Brave VPN components.
  • browser/brave_vpn/win/brave_vpn_wireguard_service/BUILD.gn

    • Updated build configuration with new sources and dependencies for the VPN WireGuard service.
  • browser/brave_vpn/win/brave_vpn_wireguard_service/install_utils.cc

    • Renamed and realigned implementation file for install utility functions pertaining to the VPN WireGuard service.
  • browser/brave_vpn/win/brave_vpn_wireguard_service/install_utils.h

    • Renamed and realigned header file for install utility function declarations.
  • browser/brave_vpn/win/brave_vpn_wireguard_service/main.cc

    • Removed unnecessary command handling logic for VPN service registration.
  • browser/brave_vpn/win/brave_vpn_wireguard_service/service/BUILD.gn

    • Adjusted dependencies in the build file for the WireGuard service.
  • browser/brave_vpn/win/brave_vpn_wireguard_service/status_tray/BUILD.gn

    • Removed unnecessary sources from the build file for the system tray component.
  • browser/brave_vpn/win/brave_vpn_wireguard_service/{status_tray/install_utils.cc, status_tray/install_utils.h}

    • Deleted redundant status tray installation utility files.
  • browser/brave_vpn/win/vpn_utils_win.cc and .h

    • New files created for handling VPN utility functions on Windows platform.
  • browser/resources/settings/brave_system_page/{brave_vpn_browser_proxy.ts, brave_vpn_page.ts}

    • Adjusted TypeScript logic for UI components related to VPN settings, reflecting the new service registration model.
  • browser/ui/webui/settings/brave_vpn/brave_vpn_handler.cc and .h

    • Updated WebUI handler for Brave VPN settings, removing obsolete service registration handling.
  • build/commands/lib/util.js

    • Modified JavaScript utilities for the build process to handle new file paths.
  • build/commands/scripts/updatePatches.js

    • Adjusted patching script for build process changes.
  • build/config/brave_build.gni

    • Included new source group for the elevation service.
  • chromium_src/check_chromium_src_config.json5

    • Configuration file for ensuring the encompassed directories are included in build process verification.
  • chromium_src/chrome/elevation_service/{DEPS, elevation_service_idl.idl, elevator.cc, elevator.h}

    • Chromium source modifications for the elevation service, which handles privileged operations needed by VPN components.
  • chromium_src/chrome/installer/{DEPS, setup/install_worker.cc, setup/install_worker.h}

    • Installer and setup changes for managing VPN components during the installation process.
  • chromium_src/chrome/installer/{setup/test/BUILD.gn, setup/test/install_worker_vpn_unittest.cc}

    • Build configuration for tests related to the installer's VPN changes, including Added new unit test file for VPN related installer logic.
  • components/brave_vpn/{browser, common}

    • Updated VPN component files with refactored service installation logic and file relocations.
  • elevation_service

    • New folder with README and .gni file oriented around the elevation service used by Brave VPN on Windows.
  • patches/chrome-{elevation_service-BUILD.gn.patch, installer-mini_installer-chrome.release.patch}

    • Newly introduced patches for the Brave VPN elevation service build configuration and mini installer.
  • test/BUILD.gn

    • Added VPN unit tests to the main test configuration.
  • win_build_output/midl/chrome/elevation_service/

    • Added new generated files for the elevation service interfaces, in various architectures (arm64, x64, x86).

Security Hotspots

  1. Modifications to elevation service and installation of system services (browser/brave_vpn/win and related files) - High risk: These changes interact with Windows system services, which, if misconfigured, could weaken security.
  2. MIDl files related to the elevation service - Moderate risk: Improper handling of object interfaces can potentially lead to security issues.
  3. Patch files: Changes to patches (patches/) - Low risk: Ensure patches don't unintentionally alter security-critical code.

@brave-builds
Copy link
Collaborator

A Storybook has been deployed to preview UI for the latest push

Comment on lines +6 to +13
#include "chrome/elevation_service/elevator.h"

#include <windows.h>
#include <winerror.h>

#include <intsafe.h>

#include "base/win/windows_types.h"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bridiver Is there are particular reason for these Windows headers to be listed as they are? I had to correct an issue in this file recently, and now clang-format for some insists in reordering these Windows inclusions to:

 #include "chrome/elevation_service/elevator.h"
 
 #include <windows.h>
-#include <winerror.h>
 
 #include <intsafe.h>
+#include <winerror.h>
 
 #include "base/path_service.h"
 #include "base/win/windows_types.h"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI/storybook-url Deploy storybook and provide a unique URL for each build puLL-Merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Windows should not install VPN services until VPN is purchased/enabled
9 participants