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

Add support for configurable proxy target port #278

Closed

Conversation

daalbert
Copy link

New Feature: Specify Proxy Target Port

Description

This PR adds a new feature that allows users to specify an alternative port for the Tailscale proxy setup. Useful for setting up a reverse proxy, e.g., with the Nginx add-on. Defaults to the Home Assistant core port if not specified.

Changes

  • Configuration: Added proxy_target_port to config.yaml.
  • Documentation: Updated DOCS.md to include the new option.
  • Run Script (s6-rc.d/proxy/run): Modified to read the proxy_target_port option.
  • Translation: Added an entry for the new option.

@@ -37,6 +37,7 @@ schema:
log_level: list(trace|debug|info|notice|warning|error|fatal)?
login_server: url?
proxy: bool?
proxy_target_port: int?
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest: proxy_target_port: port?

proxy_target_port=$(bashio::core.port)
fi

if ! /opt/tailscale serve https:443 / "http://127.0.0.1:${proxy_target_port}"; then
Copy link
Contributor

Choose a reason for hiding this comment

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

If you redirect the target of the tailscale serve command to another httpS port, it won't work (tailscale serve can accept self signed certs, but can't accept "invalid" certs, like DuckDNS cert behind 127.0.0.1:target_port). So tailscale serve will work only, when the NGINX reverse proxy is a plain HTTP proxy.

And the current version of the PR checks HA to be on a HTTP port, but then redirects tailscale serve to another "unchecked" port, it seems to me inconsistent.

Copy link
Author

Choose a reason for hiding this comment

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

Good points. One option would be to change the serve command to include +insecure which I believe would overcome "invalid" certs. All that said I think this needs a lot of consideration.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, +insecure won't work. Tested it 1 year ago, and now again, same problem, different error message.

Insecure seems to be for self signed certs only, https+insecure://127.0.0.1:xxx or https://127.0.0.1:xxx works the same in case of a real cert behind 127.0.0.1:xxx:

  • browser receives 502
  • tailscale logs "http: proxy error: remote error: tls: unrecognized name" a year ago the message was more useful: "proxy error: x509: cannot validate certificate for 127.0.0.1 because it doesn't contain any IP SANs"
  • because it will not accept a valid (eg. Let's Encrypt) cert for a server on 127.0.0.1, and you can't change 127.0.0.1 in tailscale serve, and you can't add localhost to certs (if I know correctly)

I don't know whether it is a tailscale bug, or I did something wrong. If you are successful to target with tailscale proxy a https server, please give some example.

Copy link
Author

Choose a reason for hiding this comment

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

I don't have easy access to a test environment for this setup at the moment, but perhaps the TCP forwarder would work? Ex: serve tcp:443 tcp://127.0.0.1:xxx

Copy link
Contributor

Choose a reason for hiding this comment

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

tcp:443 will result in ERR_SSL_UNRECOGNIZED_NAME_ALERT

Copy link
Member

@frenck frenck left a comment

Choose a reason for hiding this comment

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

Also: Merge conflicts.

@@ -69,6 +69,7 @@ advertise_routes:
log_level: info
login_server: "https://controlplane.tailscale.com"
proxy: true
proxy_target_port: 8123
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure about this option, I understand the reasoning, however, I'm not a fan of the fixed/single port to begin with.

We need to figure out / create better long-term plan/idea before adding stuff like this.

../Frenck

Copy link
Contributor

Choose a reason for hiding this comment

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

Just a note: Yes, this route once was dropped for good reason, because it makes the add-on config over-complicated, see: #137 (comment)

  • after the target port, we want to modify the target mount pont
  • then put multiple locations (port, mount) behind a proxy/funnel port
  • then we want multiple proxy/funnel ports

And Tailscale has plans to add a UI for the tailscale serve and funnel configs, after we should drop all these.

Copy link
Member

Choose a reason for hiding this comment

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

Tailscale would have released that months ago... I'm not a believer in any of that at this point and don't believe much of what they have to tell at this point.

IMHO, we can just ignore the idea of them building anything useful for cases like this, completely.

Copy link
Author

Choose a reason for hiding this comment

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

Here is a concise proposal for two possible approaches to extend the Tailscale add-on configuration to support multiple serve commands:

Approach 1: a structured way to configure multiple serve commands.

serve_commands:
  - protocol: "https"
    port: 443
    mount_point: "/"
    source: "http://127.0.0.1:8123"
    funnel: true
  - protocol: "tcp"
    port: 10000
    source: "tcp://127.0.0.1:22222"
    funnel: true

Approach 2: Full control over the tailscale serve command line.

advanced_serve_commands:
  - cmdline: "https:443 / https://127.0.0.1:8123"
    funnel: true
  - cmdline: "tcp:10000 tcp://127.0.0.1:22222"
    funnel: true

Schema Update for Either Approach

schema:
  serve_commands:
    - protocol: list(http|https|https+insecure|tcp|tls-terminated-tcp)
      port: port
      mount_point: str
      source: str
      funnel: bool?
  advanced_serve_commands:
    - cmdline: str
      funnel: bool?

Obviously, this would need a lot of testing, documentation, and logging. However, we can structure it in a way that if undefined, it will revert to the current logic/flow—thus mostly limiting the risk of adding this functionality to the more adventurous/advanced needs folk. Also, the funnel option would need to be limited to ports: 443, 8443 and 10000.

Copy link
Contributor

Choose a reason for hiding this comment

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

My opinion:

  • Tailscale serve is more hierarchical, eg. you can have multiple mount points behind a port. But only 2 level hierarchy is supported by the config schema.
  • You can't use insecure at that place, that is for sources.
  • Source can't be a general str, we should use at least a regex.
  • Advanced cmdline is dangerous: https:443 /xxx/ text:"What will this: ";echo xxx" do?"

Copy link
Contributor

@lmagyar lmagyar Oct 24, 2023

Choose a reason for hiding this comment

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

Though we can use tailscale serve set-raw.

NGINX proxy add-on can use an NGINX config fragment from the /share folder, but tailscale has no official way to import a serve config. Though a JSON file (like the output of the tailscale serve status --json can be imported from the stdin of tailscale serve set-raw. This is full control, tailscale verifies the JSON, but this is an undocumented feature and the structure of the JSON also has no specs (at least I can't find it). But for an advanced user it is not rocket science.

This JSON is quite hacky, but at least we should not bother care any changes on tailscale's side!

Problem: this configures serve and funnel in one step, so the current 2 internal service structure needs some rethink, or a third service using sg. advanced_proxy_funnel_config: str? (path to a file in /share).

Copy link
Author

Choose a reason for hiding this comment

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

Perhaps its a bit confusing as-is, but my thought for https+insecure was it would map to something like:
tailscale serve https+insecure / https://127.0.0.1:8123.

Regarding configuring in 1 step/architecture changes: The funnel config as presented - even though it is grouped with the serve commands, could still be parsed/applied in the current funnel service. Also, after further consideration, it may be more elegant to modify the existing funnel: bool? to a list.

I'm a fan of the advanced cmdline; however, your concerns about danger very real! I think it wouldn't be that hard to parse for command injection - though still very good point.

tailscale serve set-raw is also very intriguing. I think the closest we will get to specs/documentation is the code itself. I've dug through Tailscale's code and found the implementation:
type ServeConfig struct

Tailscale set-raw JSON Configuration Insights

For those interested in the tailscale serve set-raw option, I've dissected the relevant Go structs to understand the JSON structure it would accept. Here's a pseudo-JSON representation that maps to Tailscale's Go structs, complete with comments for better understanding:

{
  "TCP": {
    // Key: uint16 (port number), Value: Pointer to TCPPortHandler
    "443": {
      "HTTPS": true,  // bool, mutually exclusive with TCPForward
      "HTTP": false,  // bool, mutually exclusive with TCPForward
      "TCPForward": "",  // string, IP:Port to forward to
      "TerminateTLS": ""  // string, SNI name for TLS termination
    }
  },
  "Web": {
    // Key: HostPort (string), Value: Pointer to WebServerConfig
    "host.tailnet.ts.net:443": {
      "Handlers": {
        // Key: string (mount point), Value: Pointer to HTTPHandler
        "/": {
          "Path": "",  // string, absolute path to directory or file
          "Proxy": "http://127.0.0.1:8123",  // string, Proxy URL or host:port
          "Text": ""  // string, plain text to serve
        }
      }
    }
  },
  "AllowFunnel": {
    // Key: HostPort (string), Value: bool
    "host.tailnet.ts.net:443": true
  },
  "Foreground": {
    // Key: string (IPN Bus session ID), Value: Pointer to ServeConfig
    "some_session_id": {
      // Nested ServeConfig
    }
  },
  "ETag": "some_checksum"  // string, not included in JSON serialization
}

Copy link
Contributor

Choose a reason for hiding this comment

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

My thoughts:

  • Based on my experiments, set-raw never returns an error, and invalid JSON-s are not rejected, but partially processed, so after a set-raw we can test the execution with sg. like diff user-config.json <(/opt/tailscale serve status --json), if there are differences, we can emit a warning.
  • If there is some advanced config present, we can ignore the proxy: bool? and funnel: bool? settings, advanced config overwrites these, these are for less advanced users.
  • Foreground and ETag seems to me some technical/implementation details, not something users can/should modify.
  • On insecure: see https://tailscale.com/kb/1242/tailscale-serve/#ignoring-invalid-and-self-signed-certificate-checks, you can't use it on the suggested position.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just a note: Foreground was a "preview" for the new serve and funnel config capabilities in v1.52.0, ie. running a serve/funnel command in the foreground and not served by the built in proxy, see #287, it makes no sense for users to import it:

  • if existing running proxies are not imported back with set-raw, they will be shut down
  • new (invalid) entries are not accepted by set-raw (there is no process to serve them)

Copy link
Contributor

Choose a reason for hiding this comment

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

The new add-on config feature (/config folder, see: https://developers.home-assistant.io/docs/add-ons/configuration/#add-on-advanced-options) can be used to store the tailscale serve set-raw config JSON file.

This is quite elegant from the add-on's point of view, though still a bit hacky to use an undocumented tailscale feature.

@frenck frenck marked this pull request as draft October 22, 2023 17:12
@frenck frenck added the enhancement Enhancement of the code, not introducing new features. label Oct 23, 2023
@lmagyar
Copy link
Contributor

lmagyar commented Nov 11, 2023

I came up with a really simple solution (see PR #293): do nothing, let tailscale remember the settings, we just simply do not touch any proxy and funnel config if the new "advanced_config: bool?" is enabled.

Copy link

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. Thank you for your contributions.

@github-actions github-actions bot added the stale There has not been activity on this issue or PR for quite some time. label Dec 12, 2023
@github-actions github-actions bot closed this Dec 20, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Dec 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement Enhancement of the code, not introducing new features. stale There has not been activity on this issue or PR for quite some time.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants