Skip to content

Install/uninstall brave vpn wireguard service #18565

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jun 8, 2023
Merged

Install/uninstall brave vpn wireguard service #18565

merged 1 commit into from
Jun 8, 2023

Conversation

spylogsster
Copy link
Contributor

@spylogsster spylogsster commented May 22, 2023 •
edited
Loading

Resolves brave/brave-browser#30486

Security review: https://github.com/brave/security/issues/1280

Devops issue: https://github.com/brave/devops/pull/9787

  • The service installed to %(VersionDir)s\BraveVpnWireguardService
image image

Submitter Checklist:

  • I confirm that no security/privacy review is needed, or that I have requested one
  • 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:

@spylogsster spylogsster self-assigned this May 22, 2023
@github-actions github-actions bot added the CI/run-audit-deps Check for known npm/cargo vulnerabilities (audit_deps) label May 22, 2023
@spylogsster spylogsster changed the title WIP WIP: Install/uninstall brave vpn wireguard service May 22, 2023
@spylogsster spylogsster force-pushed the brave-30486 branch 2 times, most recently from e9f2093 to 987476f Compare May 23, 2023 10:01
@spylogsster spylogsster force-pushed the brave-30368 branch 2 times, most recently from 770fe73 to b49001b Compare May 23, 2023 10:47
@spylogsster spylogsster force-pushed the brave-30486 branch 3 times, most recently from 3ccb497 to 75e80a5 Compare May 24, 2023 14:04
@fmarier
Copy link
Member

fmarier commented May 25, 2023

In order to make the licenses of tunnel.dll and wireguard.dll appear in brave://credits, you can create two a directory for each of these projects in brave/components/third_party/ and then put copy a LICENSE and a README.chromium like we did here for the tor daemon: https://github.com/brave/brave-core/tree/master/components/third_party/tor

@fmarier
Copy link
Member

fmarier commented May 25, 2023 •
edited
Loading

Here's what I would put in the README.chromium files:

Name: WireGuard for the NT Kernel
URL: https://download.wireguard.com/wireguard-nt/
License: LicenseRef-WireguardNTSDK-PrebuiltBinaries-License

and:

Name: WireGuard for Windows
URL: https://git.zx2c4.com/wireguard-windows
License: MIT

The LICENSE files would be:

@fmarier
Copy link
Member

fmarier commented May 25, 2023

Or this if we decide to build wireguard.dll from source instead:

Name: WireGuard for the NT Kernel
URL: https://git.zx2c4.com/wireguard-nt/
License: GPL-2.0
License File: /brave/common/licenses/GPL-2.0

@spylogsster spylogsster force-pushed the brave-30368 branch 2 times, most recently from 4b5fee0 to ed089ee Compare May 25, 2023 16:45
@spylogsster spylogsster force-pushed the brave-30368 branch 2 times, most recently from 89049ca to 7d61cf2 Compare May 29, 2023 07:15
@spylogsster spylogsster force-pushed the brave-30486 branch 3 times, most recently from 605b47f to 183fd02 Compare May 29, 2023 09:47
@spylogsster spylogsster force-pushed the brave-30486 branch 6 times, most recently from de7a23b to 5717637 Compare May 31, 2023 05:20
Base automatically changed from brave-30368 to master May 31, 2023 08:28
@spylogsster spylogsster force-pushed the brave-30486 branch 3 times, most recently from 6cdde75 to a6b3963 Compare May 31, 2023 11:47
@spylogsster spylogsster requested a review from fmarier May 31, 2023 12:43
@spylogsster spylogsster marked this pull request as ready for review May 31, 2023 15:08
@spylogsster spylogsster requested review from a team as code owners May 31, 2023 15:08
@spylogsster spylogsster changed the title WIP: Install/uninstall brave vpn wireguard service Install/uninstall brave vpn wireguard service May 31, 2023
iefremov
iefremov approved these changes May 31, 2023
View reviewed changes
Copy link
Contributor

@iefremov iefremov left a comment

Choose a reason for hiding this comment

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

chromium_src/patches lgtm

simonhong
simonhong approved these changes Jun 1, 2023
View reviewed changes
Copy link
Member

@simonhong simonhong left a comment

Choose a reason for hiding this comment

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

++

chromium_src/chrome/installer/setup/install_worker.cc
return target_path.AppendASCII(version.GetString())
.Append(brave_vpn::kBraveVPNHelperExecutable);
}

bool ConfigureBraveVPNServiceAutoRestart(const base::FilePath& exe_path,
const CallbackWorkItem&) {
bool InstallBraveVPNService(const base::FilePath& exe_path,
Copy link
Member

Choose a reason for hiding this comment

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

nit: kBraveVpnHelperInstall constants name could be renamed to kBraveVpnServiceInstall

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@bsclifton
Copy link
Member

Licensing information added in #18815

bsclifton
bsclifton approved these changes Jun 8, 2023
View reviewed changes
Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

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

Changes look great! 😄👍

@spylogsster spylogsster merged commit 1db6b1f into master Jun 8, 2023
@spylogsster spylogsster deleted the brave-30486 branch June 8, 2023 07:28
@github-actions github-actions bot added this to the 1.54.x - Nightly milestone Jun 8, 2023
@bsclifton bsclifton mentioned this pull request Oct 18, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simonhong simonhong simonhong approved these changes

@iefremov iefremov iefremov approved these changes

@bsclifton bsclifton bsclifton approved these changes

@fmarier fmarier Awaiting requested review from fmarier

Assignees

@spylogsster spylogsster

Labels
CI/run-audit-deps Check for known npm/cargo vulnerabilities (audit_deps)
Projects
None yet
Milestone
1.57.x - Release
Development

Successfully merging this pull request may close these issues.

Install/uninstall brave vpn wireguard service
5 participants
@spylogsster @fmarier @bsclifton @iefremov @simonhong