Added brave vpn connection manager interface and initial Windows impls #8920

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

simonhong merged 14 commits into master from brave_vpn_stub

Jun 10, 2021

Member

simonhong commented May 25, 2021 •

edited

Loading

Added vpn connection manager interface
Added Initial impls for Windows OS.

fix brave/brave-browser#15805

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

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
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:

Build command line tool vpntool in bc directory - ninja -C ../out/Components vpntool
Run ../out/Components/vpntool.exe with various options
Print connections ../out/Components/vpntool.exe --connections
Print entries ../out/Components/vpntool.exe --entries
Print devices ../out/Components/vpntool.exe --devices
Create new entry ../out/Components/vpntool.exe --create --host_name=xxx --vpn_name=xxx --user_name=xxx --password=xxx
Delete entry ../out/Components/vpntool.exe --remove --vpn_name=xxx
Connect entry ../out/Components/vpntool.exe --connect --vpn_name=xxx
Disconnect entry ../out/Components/vpntool.exe --disconnect --vpn_name=xxx

github-actions bot assigned simonhong

simonhong force-pushed the brave_vpn_stub branch 2 times, most recently from 1cf2771 to b89220b Compare

May 25, 2021 07:11

simonhong requested a review from bsclifton

May 25, 2021 07:44

simonhong force-pushed the brave_vpn_stub branch from b89220b to 1ba5dc7 Compare

May 25, 2021 23:42

simonhong changed the title ~~Added brave vpn connection manager interface and stub impl~~ Added brave vpn connection manager interface and initial Windows impls

simonhong added the CI/skip label

simonhong force-pushed the brave_vpn_stub branch from df01ce1 to cae4711 Compare

May 26, 2021 09:19

bsclifton reviewed

View reviewed changes

components/brave_vpn/winvpntool.cc Outdated Show resolved Hide resolved

bsclifton reviewed

View reviewed changes

components/brave_vpn/winvpntool.cc Show resolved Hide resolved

simonhong force-pushed the brave_vpn_stub branch 2 times, most recently from 91d7217 to caaed8a Compare

May 27, 2021 06:46

simonhong removed the CI/skip label

simonhong force-pushed the brave_vpn_stub branch 2 times, most recently from 8302e74 to 1d41aa8 Compare

May 27, 2021 12:55

Member Author

simonhong commented May 27, 2021

@bsclifton Ready to review! PTAL :)

bsclifton requested a review from deeppandya

May 27, 2021 16:56

Member Author

simonhong commented May 28, 2021

@deeppandya My PR will make some merge conflict to your PR (#7904).
Both PR added new brave_vpn target :)
I think all conflict will on happen on gn files.
The difference is usingbrowser sub directory and ENABLE_BRAVE_VPN.
As brave vpn feature will only be used on browser side, I think we don't need to use browser sub directory.
and build flag name convension is ENABLE_FOO.

bsclifton reviewed

View reviewed changes

BUILD.gn Show resolved Hide resolved

bsclifton reviewed

View reviewed changes

components/brave_vpn/brave_vpn_connection_info.h Outdated Show resolved Hide resolved

bsclifton reviewed

View reviewed changes

components/brave_vpn/brave_vpn_connection_manager_win.cc Show resolved Hide resolved

bsclifton reviewed

View reviewed changes

components/brave_vpn/winvpntool.cc Show resolved Hide resolved

bsclifton reviewed

View reviewed changes

components/brave_vpn/winvpntool.cc Outdated Show resolved Hide resolved

bsclifton reviewed

View reviewed changes

components/brave_vpn/winvpntool.cc Show resolved Hide resolved

bsclifton reviewed

View reviewed changes

Member

bsclifton left a comment

Comments left; great job here! 😄

Member Author

simonhong commented May 28, 2021

@bsclifton Addressed. Thanks for review!

bsclifton approved these changes

View reviewed changes

Member

bsclifton commented Jun 1, 2021

Approved, pending security review and also review by @deeppandya 😄

fmarier requested changes

View reviewed changes

Member

fmarier left a comment

I left a few suggestions around buffer sizes. Memory management is easy to get wrong in C so I tend to be extra-careful around these, but let me know if there's a reason why these were done with bare constants.

components/brave_vpn/utils_win.cc Outdated

+              // https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-formatmessage
+              void PrintSystemError(DWORD error) {
+                DWORD c_buf_size = 512;
+                TCHAR lpsz_error_string[512];

Member

fmarier Jun 4, 2021

To make sure there is never a mismatch between the buffer size and what we pass to FormatMessage(), I would do something like:

constexpr DWORD c_buf_size = 512; 
TCHAR lpsz_error_string[c_buf_size];

Member Author

simonhong Jun 7, 2021

fixed.

components/brave_vpn/utils_win.cc Outdated

+                credentials.dwMask = RASCM_UserName | RASCM_Password;
+                wcscpy_s(credentials.szUserName, 256, username);
+                wcscpy_s(credentials.szPassword, 256, password);

Member

fmarier Jun 4, 2021

Instead of 256, it might be better to use UNLEN+1 and PWLEN+1.

Member Author

simonhong Jun 7, 2021

fixed.

components/brave_vpn/utils_win.cc Outdated Show resolved Hide resolved

components/brave_vpn/utils_win.cc Outdated Show resolved Hide resolved

components/brave_vpn/utils_win.cc Outdated Show resolved Hide resolved

components/brave_vpn/utils_win.cc Outdated Show resolved Hide resolved

components/brave_vpn/utils_win.cc Outdated Show resolved Hide resolved

components/brave_vpn/utils_win.cc Outdated Show resolved Hide resolved

components/brave_vpn/utils_win.cc Outdated Show resolved Hide resolved

components/brave_vpn/utils_win.cc Show resolved Hide resolved

simonhong added the CI/skip label

simonhong force-pushed the brave_vpn_stub branch from 2591138 to b501fdc Compare

June 7, 2021 02:10

simonhong commented

View reviewed changes

Member Author

simonhong left a comment

Addressed all comments for buf length.
and looking at https://github.com/brave/brave-core/pull/8920/files#r645919998

components/brave_vpn/utils_win.cc Outdated

+              // https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-formatmessage
+              void PrintSystemError(DWORD error) {
+                DWORD c_buf_size = 512;
+                TCHAR lpsz_error_string[512];

Member Author

simonhong Jun 7, 2021

fixed.

components/brave_vpn/utils_win.cc Outdated

+                credentials.dwMask = RASCM_UserName | RASCM_Password;
+                wcscpy_s(credentials.szUserName, 256, username);
+                wcscpy_s(credentials.szPassword, 256, password);

Member Author

simonhong Jun 7, 2021

fixed.

components/brave_vpn/utils_win.cc Outdated Show resolved Hide resolved

components/brave_vpn/utils_win.cc Outdated Show resolved Hide resolved

components/brave_vpn/utils_win.cc Outdated Show resolved Hide resolved

components/brave_vpn/utils_win.cc Outdated

+                }
+                lp_ras_dial_params->dwSize = sizeof(RASDIALPARAMS);
+                wcscpy_s(lp_ras_dial_params->szEntryName, 256, entry_name);
+                wcscpy_s(lp_ras_dial_params->szDomain, 15, L"*");

Member Author

simonhong Jun 7, 2021

Changed to DNLEN + 1

components/brave_vpn/utils_win.cc Outdated Show resolved Hide resolved

components/brave_vpn/utils_win.cc Outdated Show resolved Hide resolved

components/brave_vpn/utils_win.cc Outdated Show resolved Hide resolved

components/brave_vpn/utils_win.cc Outdated Show resolved Hide resolved

Member Author

simonhong commented Jun 7, 2021 •

edited

Loading

Hmm, I got Policy match error again with this PR. need to check.
I can see the below result and I think it's what we expect now.
@bsclifton not sure but there is some changes from server-side and it's not matched?
Left comment about this in slack channel also.

fmarier reviewed

View reviewed changes

components/brave_vpn/utils_win.cc Outdated Show resolved Hide resolved

components/brave_vpn/utils_win.cc Outdated Show resolved Hide resolved

simonhong force-pushed the brave_vpn_stub branch from b501fdc to 2a8ab56 Compare

June 8, 2021 01:57

fmarier approved these changes

View reviewed changes

simonhong and others added 13 commits

June 10, 2021 09:47


          Added brave vpn connection manager interface and stub impl

e3f309a

Implementation will depends on OS platform.


          Added initial windows impls

24e7a72

and vpntool command line tool is introduced.


          Applied chromium coding convensions

b0dfbc7

and used chromium api as many as possible.


          Fix return value check for ExpandEnvironmentStrings

ec6009c


          Use %ls for wide string

9ec3d99


          vpntool's entries command will print policy value also

60ecc00


          Filled Connect() method

3dfbd8c


          Fill disconnect()

1bb8fca


          Separated vpn apis from manager

ca855cd


          Run win32 ras api asynchronously

f98f1fc

ras api could block main thread.
Client will get the result by observation.


          Deleted unused lib and headers including

f482403


          Address review comments

a4d9e02


          Used exact buffer length for buffer operations

5862e7f

and uses constness as many as possible.

simonhong force-pushed the brave_vpn_stub branch from 6d7f61e to 9ac70fe Compare

June 10, 2021 00:51

simonhong removed the CI/skip label


          Updated after testing policy and verifying it works (previous policy …

8ef351e

…had broken)

simonhong force-pushed the brave_vpn_stub branch from 9ac70fe to 8ef351e Compare

June 10, 2021 02:03

Member

bsclifton commented Jun 10, 2021

Thanks for the great review, @fmarier 😄👍

simonhong merged commit a39f967 into master

simonhong deleted the brave_vpn_stub branch

June 10, 2021 10:19

simonhong added this to the 1.27.x - Nightly milestone

simonhong mentioned this pull request

Fixed invalid string is passed to apis in utils_win.h #9861

Merged

24 tasks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet