Add TargetPort to RouteToApp & use it to route connections to multi-port TCP apps
#49047
Conversation
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
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. :~)
| 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. |
| return targetPort == port | ||
| } | ||
|
|
||
| return targetPort >= port && targetPort <= endPort |
There was a problem hiding this comment.
| return targetPort >= port && targetPort <= endPort | |
| return port <= targetPort && targetPort <= endPort |
| // In theory, this behavior could be removed in the future if we guarantee that all clients always | ||
| // send a target port when connecting to multi-port apps, but no such effort was undertaken so far. | ||
| if targetPort == 0 { | ||
| firstPort := int(app.GetTCPPorts()[0].Port) |
There was a problem hiding this comment.
This is sort of spooky, we're only checking for app.GetTCPPorts() being nonempty in the caller so if we change it in the wrong way we might end up with a panic here. The logic is also a bit too spread out, perhaps we can rework it as something like this instead?
var dialTarget string
switch {
case len(app.GetTCPPorts()) < 1:
// dialTarget = addr.String
case targetPort == 0:
// dialTarget = joinhostport(host, firstport)
default:
// check for port in range then dialTarget = joinhostport(host, port)
}
dialer.Dial(dialTarget)There was a problem hiding this comment.
Also, should we error out if there's a target port but the app is not multi-port?
#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.