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

Kill switch #1408

Merged
merged 16 commits into from
Aug 4, 2023
Merged

Kill switch #1408

merged 16 commits into from
Aug 4, 2023

Conversation

mallexxx
Copy link
Collaborator

@mallexxx mallexxx commented Jul 28, 2023

Task/Issue URL: https://app.asana.com/0/72649045549333/1204311881295998/f
Tech Design URL:
BSK PR: duckduckgo/BrowserServicesKit#438

Description:

  • Kill Switch (enforceRoutes) toggle
  • Excluded routes basic (hardcoded ddg) implementation
  • Connect on Launch feature
  • cleanup, some refactoring

Steps to test this PR:

  1. Validate VPN is working with default settings, Local Routes are excluded
  2. Disconnect, activate enforceRoutes, connect, validate VPN is working, routes bypassing vpn - not
  3. Activate DDG exclusion, validate "my ip" SERP request returns real ip, deactivate exclusion - ip should be VPN‘s
  4. Disconnect. Activate connect on log in; restart Status Bar Agent - connection shouldn‘t be initiated. Reboot: VPN should reconnect automatically
  5. Validate connection/disconnection from System Settings works and exclusions/enforceRoutes settings applied

Internal references:

Pull Request Review Checklist
Software Engineering Expectations
Technical Design Template
Pull Request Documentation

@github-actions
Copy link

github-actions bot commented Jul 28, 2023

Warnings
⚠️ PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS against c938e92

@mallexxx mallexxx changed the title kill switch [WIP] kill switch Jul 31, 2023
@mallexxx mallexxx assigned mallexxx and unassigned diegoreymendez Jul 31, 2023
@mallexxx mallexxx assigned diegoreymendez and unassigned mallexxx Jul 31, 2023
@@ -40,15 +40,16 @@ extension NSApplication {
}
}
}
@objc dynamic var runType: RunType { .normal }
@objc dynamic class var runType: RunType { .normal }
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

UserDefaultsWrapper uses runType to distinguish test runs - this was crashing in the Agent

case missingProviderConfiguration
case missingPixelHeaders
}
struct MissingPixelHeaders: Error { }
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

MissingProviderConfiguration() moved to the base class

@mallexxx mallexxx changed the title [WIP] kill switch Kill switch Jul 31, 2023
Copy link
Contributor

@diegoreymendez diegoreymendez left a comment

Choose a reason for hiding this comment

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

Adding some comments.

@diegoreymendez
Copy link
Contributor

Added some initial feedback, this is still ongoing though.

@diegoreymendez diegoreymendez self-requested a review July 31, 2023 14:39
@diegoreymendez diegoreymendez self-requested a review July 31, 2023 17:01
Copy link
Contributor

@diegoreymendez diegoreymendez left a comment

Choose a reason for hiding this comment

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

Added another set of comments, the review is sill ongoing.

One thing I didn't add inline is - the simulate tunnel option isn't working well for me. I pointed to a line were it seems we're mixing tunnel and controller errors, but I'm not sure if that's the cause of it.

It's simply not doing anything most of the time.

@diegoreymendez
Copy link
Contributor

@mallexxx - Could I ask you to share the configuration / rules that you're using in WireShark to test that the traffic is properly routed through the tunnel? I'd like to make sure I'm doing it right.

@mallexxx
Copy link
Collaborator Author

mallexxx commented Aug 2, 2023

Reorganized the menu and disabling 10.0.0.0 from exclusions when enforceRoutes is active because otherwise DNS isn‘t working (and tunnel not working when 10.11.12.1 added to inclusions)
fixed tunnel failure action mistake

I‘m not using any custom config for WireShark, just opening bridge100 inspector when the browser is run in VM (see https://app.asana.com/0/0/1204180319229419/f)

@diegoreymendez diegoreymendez self-requested a review August 2, 2023 16:03
@diegoreymendez diegoreymendez self-requested a review August 2, 2023 16:08
Copy link
Contributor

@diegoreymendez diegoreymendez left a comment

Choose a reason for hiding this comment

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

I rolled back the approval, as I just noticed there's no iOS PR. Would you add the integration PR for iOS?

@mallexxx mallexxx mentioned this pull request Aug 2, 2023
13 tasks
@mallexxx
Copy link
Collaborator Author

mallexxx commented Aug 2, 2023

duckduckgo/iOS#1891

@diegoreymendez diegoreymendez self-requested a review August 3, 2023 15:31
Copy link
Contributor

@diegoreymendez diegoreymendez left a comment

Choose a reason for hiding this comment

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

I'm approving this optimistically so we can move forward. If there's anything at this point we'll need to tackle it as a follow up.

mallexxx added a commit to duckduckgo/BrowserServicesKit that referenced this pull request Aug 4, 2023
@mallexxx mallexxx merged commit f8df91f into develop Aug 4, 2023
10 checks passed
@mallexxx mallexxx deleted the alex/kill-switch branch August 4, 2023 06:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants