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

Return Public Web Port in TLS mode for postgres when listen addr specified. #22058

Merged
merged 3 commits into from
Mar 10, 2023

Conversation

stevenGravy
Copy link
Contributor

Addresses #12863

This handles the case of a TLS mode and there is a listen address specified but no public postgres addr specified. This can happen in cases like a helm deployment with multiplex enabled and a postgres listen address is 3080. The right address to return is teleport.example.com:443, not 3080 which is not an available port to a user.

Copy link
Collaborator

@zmb3 zmb3 left a comment

Choose a reason for hiding this comment

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

We should also add a test for this.

@@ -3715,14 +3715,16 @@ func (tc *TeleportClient) applyProxySettings(proxySettings webclient.ProxySettin
proxySettings.DB.PostgresPublicAddr)
}
tc.PostgresProxyAddr = net.JoinHostPort(addr.Host(), strconv.Itoa(addr.Port(tc.WebProxyPort())))
case proxySettings.DB.PostgresListenAddr != "":
// When in TLS routing mode the web port should be used if no Postgres Public Addr is set
Copy link
Collaborator

Choose a reason for hiding this comment

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

Postgres public addr or listen addr? The comment says one thing and the code says another.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated comment

addr, err := utils.ParseAddr(proxySettings.DB.PostgresListenAddr)
if err != nil {
return trace.BadParameter("failed to parse Postgres listen address received from server: %q, contact your administrator for help",
proxySettings.DB.PostgresListenAddr)
}
tc.PostgresProxyAddr = net.JoinHostPort(tc.WebProxyHost(), strconv.Itoa(addr.Port(defaults.PostgresListenPort)))
default:
log.Debugf("Returning default")
Copy link
Collaborator

Choose a reason for hiding this comment

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

This isn't going to be very helpful. I'd remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed, thanks

@r0mant r0mant requested review from smallinsky and greedy52 and removed request for r0mant and jimbishopp March 8, 2023 21:48
@@ -3715,7 +3715,8 @@ func (tc *TeleportClient) applyProxySettings(proxySettings webclient.ProxySettin
proxySettings.DB.PostgresPublicAddr)
}
tc.PostgresProxyAddr = net.JoinHostPort(addr.Host(), strconv.Itoa(addr.Port(tc.WebProxyPort())))
case proxySettings.DB.PostgresListenAddr != "":
// Listen address port applies if set and not in TLS routing mode.
case proxySettings.DB.PostgresListenAddr != "" && !proxySettings.TLSRoutingEnabled:
Copy link
Collaborator

Choose a reason for hiding this comment

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

If Postgres listen address is specified then isn't the intent that Postgres listener is separate and client should connect to it even if TLS routing is enabled? @smallinsky @greedy52 Can you folks check as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think that this is intended behavior and by design in case of TLS Routing enabled the Web Port takes precedence over
tunnel_public_addr, postgres_public_addr, mysql_public_addrs or kube_public_addr

I don't think that we want to change this behavior because it will mean that if the postgres_public_addr was provided and TLS Routing is enable the local client will try to connect with TLS Routing connection to raw postgres listener that cannot handle TLS Routing Layer.

This logic when postgres_public_addrs is used instead of Web Port address make only sense in context of tsh db config command that is use in case of separate listener mode and in case of TLS Routing the tsh proxy db command should be used.

https://goteleport.com/docs/connect-your-client/gui-clients/#get-connection-information
Screenshot 2023-03-09 at 11 00 26

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe I misunderstood the issue. multiplex is enabled, and Postgres listener address is set but NOT the Postgres public address. In my mind, using the Postgres listener port (e.g. :3080) is the correct behaviour for tsh db config, assuming it's reachable. If teleport.example.com:3080 is not reachable, what the Postgres listener address is used for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem when this occurs most is in K8s helm deployments. The public address is Teleport.example.com:443 and using multiplex. Everything else is then 3080 as listen addresses. It's automatically adding the Postgres listen addr.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. In TLS Routing mode logic we are using the web port where tsh db config is intended to be used only is separate listener mode but we don't forbid this call from multiplex mode so a client can still connect to the proxy using raw postgres connection without local proxy. In multiple mode, in ping response, we are setting all listeners to the webport address:

settings.SSH.ListenAddr = multiplexAddr

So the response looks like:

  "proxy": {
    "kube": {
      "enabled": true,
      "listen_addr": "0.0.0.0:3080"
    },
    "ssh": {
      "listen_addr": "0.0.0.0:3080",
      "tunnel_listen_addr": "0.0.0.0:3080",
      "web_listen_addr": "0.0.0.0:3080",
      "public_addr": "my.ice-berg.dev:443"
    },
    "db": {
      "postgres_listen_addr": "0.0.0.0:3080",
      "mysql_listen_addr": "0.0.0.0:3080"
    },
    "tls_routing_enabled": true
  },

Where the postgres address is build from public_addr host:my.ice-berg.dev and postgres_listen_addr port 3080

but in the case of Multiplex mode the WebPortHost should take precedence.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for confirming!

Copy link
Contributor

@greedy52 greedy52 Mar 10, 2023

Choose a reason for hiding this comment

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

I see. In TLS Routing mode logic we are using the web port where tsh db config is intended to be used only is separate listener mode but we don't forbid this call from multiplex mode so a client can still connect to the proxy using raw postgres connection without local proxy. In multiple mode, in ping response, we are setting all listeners to the webport address:

settings.SSH.ListenAddr = multiplexAddr

So the response looks like:

  "proxy": {
    "kube": {
      "enabled": true,
      "listen_addr": "0.0.0.0:3080"
    },
    "ssh": {
      "listen_addr": "0.0.0.0:3080",
      "tunnel_listen_addr": "0.0.0.0:3080",
      "web_listen_addr": "0.0.0.0:3080",
      "public_addr": "my.ice-berg.dev:443"
    },
    "db": {
      "postgres_listen_addr": "0.0.0.0:3080",
      "mysql_listen_addr": "0.0.0.0:3080"
    },
    "tls_routing_enabled": true
  },

Where the postgres address is build from public_addr host:my.ice-berg.dev and postgres_listen_addr port 3080

but in the case of Multiplex mode the WebPortHost should take precedence.

@smallinsky that makes sense (for this fix). If mysql_listen_addr and postgres_listen_addr are inheriting multiplexAddr, shouldn't postgres_public_addr and mysql_public_addr also inherit web public address? And this webport wouldn't work with MySQL without local proxy (we blocked this possibility in tsh, but maybe we shouldn't advertise through ping)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I would be nice to align the ping response too and make it clear. For instance In case of TLS Routing mode we can just return:

  "proxy": {
    "kube": {
      "enabled": true,
    },
    "ssh": {
      "public_addr": "my.ice-berg.dev:443"
    },
    "tls_routing_enabled": true
  },

but I'm a bit afraid about backward compatibility and breaking something.

@stevenGravy stevenGravy added this pull request to the merge queue Mar 10, 2023
Merged via the queue into master with commit a2a6527 Mar 10, 2023
@public-teleport-github-review-bot

@stevenGravy See the table below for backport results.

Branch Result
branch/v10 Create PR
branch/v11 Create PR
branch/v12 Create PR

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

Successfully merging this pull request may close these issues.

tsh db config reporting Port 3080 instead of the public_addr 443 when using TLS routing
5 participants