Skip to content

Added BraveVPN Windows service for setting DNS filters #15915

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

Merged
merged 8 commits into from
Feb 3, 2023
Merged

Conversation

spylogsster
Copy link
Contributor

@spylogsster spylogsster commented Nov 11, 2022

Resolves brave/brave-browser#25489

Sec review and video in https://github.com/brave/security/issues/1029

  • Added a windows service to setup/remove DNS filters via WFP api when VPN connected/disconnected. The service is installed and registered with mini_installer when user has elevated permissions.
  • Crash reports from the service are expected to be inside %channel%\User Data\Crashpad\reports
  • Added runtime fallback to override DoH if the helper service is not installed or failed to start more than 3 times.
  • Disconnects brave vpn connection if unable to set filters
  • The service stops 10 seconds after disconnecting the vpn entrypoint, if during this time the vpn is reconnected, the service will not stop and will continue to work and set filters.
  • Installed filters can be viewable by netsh wfp show filters
  • Updated dialog text for DoH fallback:

image

Helper's crash reports:
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, npm run lint, 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:

  • in issue

@github-actions github-actions bot added the CI/run-audit-deps Check for known npm/cargo vulnerabilities (audit_deps) label Nov 11, 2022
@spylogsster spylogsster added CI/run-draft and removed CI/run-audit-deps Check for known npm/cargo vulnerabilities (audit_deps) labels Nov 11, 2022
@spylogsster spylogsster force-pushed the brave-25489 branch 18 times, most recently from fb99aec to df2a42e Compare November 17, 2022 11:44
@spylogsster spylogsster force-pushed the brave-25489 branch 8 times, most recently from 52c29c4 to be02a20 Compare November 27, 2022 15:48
@spylogsster spylogsster force-pushed the brave-25489 branch 4 times, most recently from 5212180 to 7ffe672 Compare January 30, 2023 13:20
@spylogsster
Copy link
Contributor Author

and please check brave_vpn_helper.exe is signed.

image

@spylogsster spylogsster marked this pull request as draft January 31, 2023 12:17
@spylogsster spylogsster force-pushed the brave-25489 branch 2 times, most recently from 38000f4 to dfb50c5 Compare February 1, 2023 11:21
@spylogsster spylogsster marked this pull request as ready for review February 1, 2023 11:24
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.

This is great! 😄👍

I can help get the final text updated before we ship

@spylogsster spylogsster enabled auto-merge (squash) February 2, 2023 21:19
@spylogsster spylogsster merged commit 0928911 into master Feb 3, 2023
@spylogsster spylogsster deleted the brave-25489 branch February 3, 2023 00:10
@github-actions github-actions bot added this to the 1.50.x - Nightly milestone Feb 3, 2023
process_type);
if (process_type == crash_reporter::switches::kCrashpadHandler) {
// Check if we should monitor the exit code of this process
std::unique_ptr<browser_watcher::ExitCodeWatcher> exit_code_watcher;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This line is default initialising exit_code_watcher to nullptr. However, a few lines down we check for the validity of exit_code_watcher, which will always return false, so it won't ever call exit_code_watcher->StopWatching();.

Do we need this browser_watcher::ExitCodeWatcher instance here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement OpenVPN style DNS resolving
7 participants