VNet: Refuse conn if local port doesn't match app spec #49944
Conversation
This allows more reasons to be specified.
ravicious
added
no-changelog
github-actions
bot
added
application-access
size/md
tsh
| } | ||
| case 'invalidLocalPort': { | ||
| return { | ||
| title: `Invalid local port for ${publicAddrWithTargetPort(routeToApp)}`, |
There was a problem hiding this comment.
Gah, this shouldn't say "local port", I'll change it to just "port" tomorrow.
One more change that I'm considering is listing all valid ports in this notification. The problem is that the list is going to get unwieldy to display, so I suppose I could do so only when there's less than 10 port ranges in the app spec I guess?
The Electron app doesn't have access to the app spec, so the list of valid ports would have to be sent over RPC.
There was a problem hiding this comment.
One more change that I'm considering is listing all valid ports in this notification. The problem is that the list is going to get unwieldy to display, so I suppose I could do so only when there's less than 10 port ranges in the app spec I guess?
If it's not too much work, I think it's worth adding. Alternatively, we could say 'Verify available ports for the app in the resources tab' or something like that :)
There was a problem hiding this comment.
tsh and Connect now show valid ports if there's less than 10 port ranges.
web/packages/teleterm/src/ui/services/tshdNotifications/tshdNotificationService.ts
Show resolved
Hide resolved
lib/vnet/app_resolver.go
Outdated
| if clusterClient.ClusterName() != profileName { | ||
| leafClusterName = clusterClient.ClusterName() | ||
| clusterName := clusterClient.ClusterName() | ||
| if clusterName != "" && clusterName != profileName && clusterName != clusterClient.RootClusterName() { |
There was a problem hiding this comment.
Just curious: is it possible to clusterName == profileName but clusterName != clusterClient.RootClusterName()?
There was a problem hiding this comment.
That would be possible if you have a root cluster with the name foo available at foo.com and a leaf cluster with the name of foo.com.
Which is not very likely, but I suppose to be correct, we should perhaps only compare ClusterName to RootClusterName.
The problem with the previous implementation was that if the name of the root cluster was not equal to its proxy host (and thus the profile name), VNet would set leafClusterName to that of the root cluster. This worked fine in the client cache. However, in methods such as OnInvalidLocalPort, we'd generate invalid teleterm URIs, as they'd end up being like /clusters/<profile name>/leaves/<root cluster name>.
| } | ||
| case 'invalidLocalPort': { | ||
| return { | ||
| title: `Invalid local port for ${publicAddrWithTargetPort(routeToApp)}`, |
There was a problem hiding this comment.
One more change that I'm considering is listing all valid ports in this notification. The problem is that the list is going to get unwieldy to display, so I suppose I could do so only when there's less than 10 port ranges in the app spec I guess?
If it's not too much work, I think it's worth adding. Alternatively, we could say 'Verify available ports for the app in the resources tab' or something like that :)
web/packages/teleterm/src/ui/services/tshdNotifications/tshdNotificationService.ts
Show resolved
Hide resolved
|
@ravicious See the table below for backport results.
|
This PR implements the scenario described in the "Incorrect port" section of the RFD. The port is checked against the app spec that VNet already has. This version does not implement any kind of cache refresh that was discussed in #46169 (comment).
From the UX perspective, I made it so that error notifications can be automatically dismissed on a per-notification basis. My worry was that if a 3rd-party client would repeatedly try to open many new connections on ports that are not in the spec, the user would have dozens of error notifications in Connect that would need to be manually closed. Instead, I made it so that error notifications about invalid port are automatically dismissed after 5 seconds, similar to non-error notifications in the app.
The first half of the PR is focused on Go, the other on JS.