Propagating a Proxy Config [JSON] String from browser down to go-tun2socks layer #1605

bi-kh · 2023-03-23T02:10:41Z

The config string is mostly opaque (except host retrieval in iOS) and passed along the layers unchanged.

Note: In iOS, We no longer update the config's host with the resolved IP address and pass along the original hostname, if not an IP already, to tun2socks layer so the config string effectively remains unchanged all the way.

…socks layer The config string is mostly opaque (except host retrieval in iOS) and passed along the layers unchanged. Note: In iOS, We no longer update the config's host with the resolved IP address and pass along the original hostname, if not an IP already, to tun2socks layer so the config string effectively remains unchanged all the way.

src/cordova/android/OutlineAndroidLib/outline/src/main/aidl/org/outline/TunnelConfig.aidl

fortuna · 2023-03-31T22:36:46Z

...ordova/android/OutlineAndroidLib/outline/src/main/java/org/outline/vpn/VpnTunnelService.java

   * @throws IllegalArgumentException if `tunnelId` or `config` are null.
   * @throws JSONException if parsing `config` fails.
   * @return populated TunnelConfig
   */
-  public static TunnelConfig makeTunnelConfig(final String tunnelId, final JSONObject config)
+  public static TunnelConfig makeTunnelConfig(final String tunnelId, final String configString)


Revert to the JSONObject. You'll extract the proxy string from a field in the JSON.

See CordovaTunnel in cordova_main.ts
start receives a ShadowsocksSessionConfig, right? And now I pass the opaque string version rather than its original JSON format.
Do we intend to introduce a new container config which wraps this proxy config [string] and pass that from Cordova layer?

I believe we can get rid of the IS_REACHABLE call too. It's not really useful. We use it to determine the status of the Tunnel, but we can do that in a different way. It makes sense to have a separate PR to just remove IsReachable first.

...ordova/android/OutlineAndroidLib/outline/src/main/java/org/outline/vpn/VpnTunnelService.java

fortuna · 2023-03-31T22:42:46Z

src/cordova/apple/OutlineAppleLib/Sources/OutlineTunnelSources/OutlineTunnel.swift

-  public static func decode(_ jsonData: Data) -> OutlineTunnel? {
-    return try? JSONDecoder().decode(OutlineTunnel.self, from: jsonData)
+  // Private helper to retrieve the host from the config string.
+  private func configToDictionary() -> [String: Any]? {


This means the config is not opaque to the platform-specific code, defeating the purpose of wiring the config.

What is the host used for besides the reachability test?

We may need to think about what we will show in the case of dynamic keys and what abstractions we need to support that. Perhaps we need to give different configs different ids. Or have the Client have a method to return the information to show.

Fair point. I had an earlier version passing 'host' explicitly, for the reachability purpose, along with proxyString, i.e., the Cordova command arguments like (tunnelId, proxyString, host). Does that make sense?

The action item here is to first understand what the host is being used for. We've talked about the Is Reachable: we should get rid of that. What else?

fortuna · 2023-04-14T15:40:32Z

FYI, we've finally merged the change to support config-based clients. See Jigsaw-Code/outline-go-tun2socks#117

That means some of the calls will need to change.

fortuna · 2023-04-14T15:44:36Z

...ordova/android/OutlineAndroidLib/outline/src/main/java/org/outline/vpn/VpnTunnelService.java

   * @throws IllegalArgumentException if `tunnelId` or `config` are null.
   * @throws JSONException if parsing `config` fails.
   * @return populated TunnelConfig
   */
-  public static TunnelConfig makeTunnelConfig(final String tunnelId, final JSONObject config)
+  public static TunnelConfig makeTunnelConfig(final String tunnelId, final String configString)


Let's qualify all mentions to "config", since it's ambiguous.

Suggested change

public static TunnelConfig makeTunnelConfig(final String tunnelId, final String configString)

public static TunnelConfig makeTunnelConfig(final String tunnelId, final String proxyConfigString)

fortuna · 2023-04-14T15:45:15Z

...ordova/android/OutlineAndroidLib/outline/src/main/java/org/outline/vpn/VpnTunnelService.java

@@ -251,7 +224,7 @@ private ErrorCode startTunnel(final TunnelConfig config) {
  private synchronized ErrorCode startTunnel(
      final TunnelConfig config, boolean isAutoStart) {
    LOG.info(String.format(Locale.ROOT, "Starting tunnel %s.", config.id));
-    if (config.id == null || config.proxy == null) {
+    if (config.id == null || config.proxyString == null) {


Suggested change

if (config.id == null || config.proxyString == null) {

if (tunnelConfig.id == null || tunnelConfig.proxyConfigString == null) {

fortuna · 2023-04-14T15:48:47Z

...ordova/android/OutlineAndroidLib/outline/src/main/java/org/outline/vpn/VpnTunnelService.java

-      String serverName = config.name;
-      if (serverName == null || serverName.equals("")) {
-        serverName = config.proxy.host;
+      JSONObject object = (JSONObject) new JSONTokener(config.proxyString).nextValue();


We should treat the proxy config as opaque. Move the name back to the TunnelConfig.

fortuna · 2023-04-14T15:49:04Z

...ordova/android/OutlineAndroidLib/outline/src/main/java/org/outline/vpn/VpnTunnelService.java

@@ -621,17 +573,23 @@ public int getResourceId(final String name, final String type) {
    return getResources().getIdentifier(name, type, getPackageName());
  }

-  /* Returns the server's name from |serverConfig|. If the name is not present, it falls back to the
+  /* Returns the server's name from |proxyString|. If the name is not present, it falls back to the
   * host name (IP address), or the application name if neither can be retrieved. */
  private String getServerName(final TunnelConfig config) {


Qualify the config.

Suggested change

private String getServerName(final TunnelConfig config) {

private String getServerName(final TunnelConfig tunnelConfig) {

fortuna · 2023-04-14T15:49:36Z

src/cordova/apple/OutlineAppleLib/Sources/OutlineTunnelSources/OutlineTunnel.swift

-    return ["host": host ?? "", "port": port ?? "", "password": password ?? "",
-            "method": method ?? "", "prefix": prefixStr]
-  }
+  public var configString: String?


Qualify the config:

Suggested change

public var configString: String?

public var proxyConfigString: String?

fortuna · 2023-04-14T15:52:19Z

src/cordova/apple/OutlineAppleLib/Sources/OutlineTunnelSources/OutlineTunnel.swift

@@ -41,26 +26,39 @@ public class OutlineTunnel: NSObject, Codable {
    case reconnecting = 2
  }

-  public convenience init(id: String, config: [String: Any]) {
+  public convenience init(id: String?, configString: String) {


Suggested change

public convenience init(id: String?, configString: String) {

public convenience init(id: String?, proxyConfigString: String) {

fortuna · 2023-04-14T15:54:34Z

src/cordova/plugin/android/java/org/outline/OutlinePlugin.java

-          final JSONObject config = args.getJSONObject(1);
-          int errorCode = startVpnTunnel(tunnelId, config);
+          final String configString = args.getString(1);
+          int errorCode = startVpnTunnel(tunnelId, configString);


Qualify the config

fortuna · 2023-04-14T15:54:41Z

src/cordova/plugin/android/java/org/outline/OutlinePlugin.java

@@ -259,11 +259,11 @@ public void onActivityResult(int request, int result, Intent data) {
    startVpnRequest = null;
  }

-  private int startVpnTunnel(final String tunnelId, final JSONObject config) throws Exception {
+  private int startVpnTunnel(final String tunnelId, final String configString) throws Exception {


Qualify the config.

fortuna · 2023-04-14T15:54:54Z

src/cordova/plugin/apple/src/OutlinePlugin.swift

      return sendError("Invalid configuration", callbackId: command.callbackId,
                       errorCode: OutlineVpn.ErrorCode.illegalServerConfiguration)
    }
-    let tunnel = OutlineTunnel(id: tunnelId, config: config)
+    let tunnel = OutlineTunnel(id: tunnelId, configString: configString)


Qualify the config.

…owser layer - revises Cordova 'start' command arguments# to 1 by wrapping (id, host, proxyConfigstring) in TunnelConfig. This will simplify future updates. - propagates 'host' explicitly to keep proxyConfigString opaque - simplifies some method arguments - revises OutlineTunnel.swift Note: not tested yet

fortuna

FYI, outline-go-tun2socks is not ready yet. We are working on a fix, then we'll release the library to use in outline-client.

fortuna · 2023-04-17T14:30:24Z

src/cordova/plugin/apple/src/OutlinePlugin.swift

   */
  func start(_ command: CDVInvokedUrlCommand) {
-    guard let tunnelId = command.argument(at: 0) as? String else {
-      return sendError("Missing tunnel ID", callbackId: command.callbackId,
+    guard let tunnelCnfig = command.argument(at: 0) as? [String: Any] else {


Cnfig -> Config

fortuna · 2023-04-17T14:41:54Z

...ordova/android/OutlineAndroidLib/outline/src/main/java/org/outline/vpn/VpnTunnelService.java

   * @throws IllegalArgumentException if `tunnelId` or `config` are null.
   * @throws JSONException if parsing `config` fails.
   * @return populated TunnelConfig
   */
-  public static TunnelConfig makeTunnelConfig(final String tunnelId, final JSONObject config)
+  public static TunnelConfig makeTunnelConfig(final String tunnelId, final String configString)


I believe we can get rid of the IS_REACHABLE call too. It's not really useful. We use it to determine the status of the Tunnel, but we can do that in a different way. It makes sense to have a separate PR to just remove IsReachable first.

fortuna · 2023-04-17T14:43:28Z

src/cordova/apple/OutlineAppleLib/Sources/OutlineTunnelSources/OutlineTunnel.swift

-  public static func decode(_ jsonData: Data) -> OutlineTunnel? {
-    return try? JSONDecoder().decode(OutlineTunnel.self, from: jsonData)
+  // Private helper to retrieve the host from the config string.
+  private func configToDictionary() -> [String: Any]? {


The action item here is to first understand what the host is being used for. We've talked about the Is Reachable: we should get rid of that. What else?

…ser layer (iOS) - Refactors app/VpnExtension layers 'start' args to a 'Data" encoded 'OutlineTunnel' format instead of individual arg for each property - Compiled/tested iOS TODO: - Compile/test Android - Update the app/tun2socks interface from the initial ProxyClient to the merged tun2socks

fortuna · 2023-04-18T19:10:31Z

src/cordova/android/OutlineAndroidLib/outline/src/main/java/org/outline/vpn/VpnTunnel.java

   *
-   * @param client provides access to the Shadowsocks proxy.
+   * @param client provides access to the  proxy.


Suggested change

* @param client provides access to the proxy.

* @param client provides access to the proxy.

fortuna · 2023-04-18T19:13:41Z

...ordova/android/OutlineAndroidLib/outline/src/main/java/org/outline/vpn/VpnTunnelService.java

  private synchronized ErrorCode startTunnel(
      final TunnelConfig config, boolean isAutoStart) {


I'd like to reduce the dependency on the TunnelConfig and be more explicit about what's needed here:

Suggested change

private synchronized ErrorCode startTunnel(

final TunnelConfig config, boolean isAutoStart) {

private synchronized ErrorCode startTunnel(

String id, String proxyConfigString, boolean isAutoStart) {

In fact, can we get rid of TunnelConfig and makeTunnelConfig for now?

fortuna · 2023-04-18T19:25:46Z

src/cordova/plugin/android/java/org/outline/OutlinePlugin.java

@@ -259,13 +258,14 @@ public void onActivityResult(int request, int result, Intent data) {
    startVpnRequest = null;
  }

-  private int startVpnTunnel(final String tunnelId, final JSONObject config) throws Exception {
-    LOG.info(String.format(Locale.ROOT, "Starting VPN tunnel %s", tunnelId));
+  private int startVpnTunnel(final JSONObject jsonTunnelConfig) throws Exception {


Let's revert to the more explicit interface we had before.
I'd rather not have the TunnelConfig for now, as we don't configure anything else other than the proxy.
Also, I'm not sure the id belongs in a tunnel config if it's not used to configure the VPN.

fortuna · 2023-04-18T19:34:05Z

...ordova/android/OutlineAndroidLib/outline/src/main/java/org/outline/vpn/VpnTunnelService.java

@@ -621,19 +575,9 @@ public int getResourceId(final String name, final String type) {
    return getResources().getIdentifier(name, type, getPackageName());
  }

-  /* Returns the server's name from |serverConfig|. If the name is not present, it falls back to the
-   * host name (IP address), or the application name if neither can be retrieved. */
+  /* Returns the server's host name (IP address). */
  private String getServerName(final TunnelConfig config) {


It looks like this is only used to build the foreground service notification.

We should be more explicit about that. If we need a name to identify the service we are using, we should pass that down instead of a host address. The name is nice because it's protocol-agnostic. Some strategies may need multiple IP addresses, which doesn't fit the host address model.

It seems we need:

id (why?)

service name (for system VPN UI)

proxy config (for setting up the proxy)

fortuna · 2023-04-18T19:35:25Z

src/cordova/android/OutlineAndroidLib/outline/src/main/aidl/org/outline/TunnelConfig.aidl

 parcelable TunnelConfig {
  String id;
-  String name;
-  ShadowsocksConfig proxy;
+  String host;


Revert. We need a service name, not a host address.
Perhaps we should rename the field to serviceName.

fortuna · 2023-04-18T19:36:24Z

src/cordova/android/OutlineAndroidLib/outline/src/main/aidl/org/outline/TunnelConfig.aidl

@@ -14,10 +14,8 @@

 package org.outline;

-import org.outline.shadowsocks.ShadowsocksConfig;
-
 parcelable TunnelConfig {


I'm, not convinced we need this type, versus passing the values that are needed explicitly in the different points. The extra type adds an intermediate step that sometimes hurts readability, especially since not all fields are needed everywhere this type is being used.

…ndroid) - Adds 'serverName' to the browser layer TunnelConfig and fills it with 'host' for now - Renames Android TunnelConfig's 'host' to 'serverName' - Tested TODO: - Update the app/tun2socks interface from the initial ProxyClient to the merged tun2socks (iOS & Android) - Wire the actual 'serverName' to the browser's TunnelConfig

…ndroid) - Wired the server name to the browser-side TunnelConfig propagated to the native-side 'start'. Verified that Android makeTunnelConfig retrieves it properly. TODO: - Update the app/tun2socks interface from the initial ProxyClient to the merged tun2socks (iOS & Android)

bi-kh requested review from a team as code owners March 23, 2023 02:10

fortuna reviewed Mar 31, 2023

View reviewed changes

fortuna requested a review from jyyi1 April 14, 2023 15:39

fortuna requested changes Apr 14, 2023

View reviewed changes

fortuna reviewed Apr 17, 2023

View reviewed changes

fortuna requested changes Apr 18, 2023

View reviewed changes

bi-kh added 2 commits April 18, 2023 22:10

fortuna mentioned this pull request Jun 21, 2023

Allow custom DNS resolver in client #568

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Propagating a Proxy Config [JSON] String from browser down to go-tun2socks layer #1605

Propagating a Proxy Config [JSON] String from browser down to go-tun2socks layer #1605

bi-kh commented Mar 23, 2023

fortuna Mar 31, 2023

bi-kh Apr 12, 2023

fortuna Apr 17, 2023

fortuna Mar 31, 2023

bi-kh Apr 12, 2023

fortuna Apr 17, 2023

fortuna commented Apr 14, 2023

fortuna Apr 14, 2023

fortuna Apr 14, 2023

fortuna Apr 14, 2023

fortuna Apr 14, 2023

fortuna Apr 14, 2023

fortuna Apr 14, 2023

fortuna Apr 14, 2023

fortuna Apr 14, 2023

fortuna Apr 14, 2023

fortuna left a comment

fortuna Apr 17, 2023

fortuna Apr 17, 2023

fortuna Apr 17, 2023

fortuna Apr 18, 2023

fortuna Apr 18, 2023

fortuna Apr 18, 2023

fortuna Apr 18, 2023

fortuna Apr 18, 2023

fortuna Apr 18, 2023

	public static TunnelConfig makeTunnelConfig(final String tunnelId, final String configString)
	public static TunnelConfig makeTunnelConfig(final String tunnelId, final String proxyConfigString)

	if (config.id == null \|\| config.proxyString == null) {
	if (tunnelConfig.id == null \|\| tunnelConfig.proxyConfigString == null) {

	private String getServerName(final TunnelConfig config) {
	private String getServerName(final TunnelConfig tunnelConfig) {

	public var configString: String?
	public var proxyConfigString: String?

	public convenience init(id: String?, configString: String) {
	public convenience init(id: String?, proxyConfigString: String) {

	* @param client provides access to the proxy.
	* @param client provides access to the proxy.

		private synchronized ErrorCode startTunnel(
		final TunnelConfig config, boolean isAutoStart) {

Propagating a Proxy Config [JSON] String from browser down to go-tun2socks layer #1605

Are you sure you want to change the base?

Propagating a Proxy Config [JSON] String from browser down to go-tun2socks layer #1605

Conversation

bi-kh commented Mar 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fortuna commented Apr 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fortuna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment