Add TargetPort to RouteToApp & use it to route connections to multi-port TCP apps
#49047
Conversation
ravicious
added
do-not-merge
no-changelog
| + // long as the port is defined in the app spec. When specified, it must be between 1 and 65535 and | ||
| + // the URI is expected to use this port as well. |
There was a problem hiding this comment.
It isn't true that the URI is expected to include TargetPort. This is a section that I forgot to update in the RFD PR when I was making this change.
Also wow, whatever GitHub uses for syntax highlighting breaks terribly when the things that being changed uses diff syntax. :~)
lib/auth/grpcserver_test.go
Outdated
| require.True(t, net.ParseIP(identity.LoginIP).IsLoopback()) | ||
| require.Equal(t, []string{teleport.UsageAppsOnly}, identity.Usage) | ||
| require.Equal(t, "app-a", identity.RouteToApp.Name) | ||
| require.Equal(t, uint16(1337), identity.RouteToApp.TargetPort) |
There was a problem hiding this comment.
I just copy-pasted the test case above and added a single line which checks TargetPort.
|
@marcoandredinis @espadolini I'm adding you both since you already have context on this. This is the last server-side change that's needed for multi-port TCP app access. |
lib/srv/app/tcpserver.go
Outdated
| } | ||
|
|
||
| if targetPort != addrPort { | ||
| return trace.BadParameter("target port is not equal to port number from URI (%d vs %d)", targetPort, addrPort) |
There was a problem hiding this comment.
This is one of those cases where I wish I could provide a more meaningful error message. In most cases, this is probably going to happen because of what's described in the comment for this function, but there's a chance it can happen due to a programmer error.
Maybe the error should say something like "attempt to connect to a TCP app with a multi-port cert where the target port from the cert does not match the port from the app URI (%d vs %q)"?
Saying just "target port is not equal to port number from URI" is certainly true, but it doesn't help you much unless you know how Teleport and multi-port TCP access work.
There was a problem hiding this comment.
Maybe the error should say something like "attempt to connect to a TCP app with a multi-port cert where the target port from the cert does not match the port from the app URI (%d vs %q)"?
I prefer that one, but I think we can go a step further and give the user some actionable. Maybe ask them to restart the App Service? Would re-login help here as well?
I'm not a fan of using x vs y, because it is not always clear what is x and y. And you need to check the source code to be sure which is which.
Can we use something like this instead?
| return trace.BadParameter("target port is not equal to port number from URI (%d vs %d)", targetPort, addrPort) | |
| return trace.BadParameter("target port is not equal to port number from URI target_port=%d uri_port=%d", targetPort, addrPort) |
There was a problem hiding this comment.
I prefer that one, but I think we can go a step further and give the user some actionable. Maybe ask them to restart the App Service? Would re-login help here as well?
The problem with giving some actionable here is that this is a client-side issue reported in server-side environment. We have some plans already to catch problems like this one on the client-side too, see #46169 (comment) and the "Incorrect port" section in the RFD.
lib/srv/app/tcpserver.go
Outdated
| } | ||
|
|
||
| if targetPort != addrPort { | ||
| return trace.BadParameter("target port is not equal to port number from URI (%d vs %d)", targetPort, addrPort) |
There was a problem hiding this comment.
Maybe the error should say something like "attempt to connect to a TCP app with a multi-port cert where the target port from the cert does not match the port from the app URI (%d vs %q)"?
I prefer that one, but I think we can go a step further and give the user some actionable. Maybe ask them to restart the App Service? Would re-login help here as well?
I'm not a fan of using x vs y, because it is not always clear what is x and y. And you need to check the source code to be sure which is which.
Can we use something like this instead?
| return trace.BadParameter("target port is not equal to port number from URI (%d vs %d)", targetPort, addrPort) | |
| return trace.BadParameter("target port is not equal to port number from URI target_port=%d uri_port=%d", targetPort, addrPort) |
This will make it easier to add targetPort to it.
This makes them easier to distinguish when routing doesn't work as expected.
If we kept the old code, we'd need to manually create a session for each target port, which would create a lot of duplication.
ravicious
force-pushed
the
r7s/mp-route-to-app
branch
from
3964b36 to
0889510
Compare
|
@ravicious See the table below for backport results.
|
#47706 made it so that it's possible to configure a multi-port TCP app. This PR makes it possible to generate a cert with
TargetPortand then makes the app agent use that field from the cert to route the connection to the target port. This is described in the "Passing the port number from the client to the app agent" section of the RFD and backwards compatibility is described in the "Server-client compatibility" section.Best reviewed commit-by-commit. The actual implementation is rather simple, but I had to change a bunch of stuff to make tests for this feature more terse.
I manually tested that this works by modifying the code in lib/teleterm that generates app certs to include a hardcoded
TargetPort. Then I configured a TCP app with an appropriate port range with Postgres running on that port and then I connected to it. All of the audit events include a separate field with the target port.