-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
RFD 182: Multi-port TCP app access #46169
Conversation
```yaml | ||
apps: | ||
- name: "waldo" | ||
uri: "tcp://localhost" | ||
ports: [4080-4090, 5095] | ||
labels: | ||
env: prod | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more aspect I think we should address in the RFD is whether we should support port mapping, as opposed to the current design where the port VNet will "listen" on must match the "target" port we dial the app with
you could imagine that an app server has an app running locally listening on localhost:8080 and localhost:8443 that they want to expose via vnet on ports 80 and 443. It could be configured something like
- name: "waldo"
uri: "tcp://localhost"
ports:
- listen: 80
target: 8080
- listen: 443
target: 8443
- listen: 5000-5010 # maybe support ranges too
target: 6000-6010
- listen: 8081 # if either listen or target is omitted, you could default to matching the one that's set
There's slightly more to it but I could see it being a usecase people will want. It would be necessary if they want to expose multiple localhost
apps on the same ports, which we coincidentally support for single-port apps today by mapping all "listen" ports to the single target port
The slight quirk though is the listen
address would only be relevant to vnet
, not tsh proxy app
- or maybe we could make that the default listen port for tsh proxy app
? likely to conflict between multiple apps though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a good idea, but I think I'll leave it for later. As Sasha said in a response to my status email on August 8th, the design partner asks for a pretty narrow scope and there's enough work here already without introducing port mapping.
|
||
In order to pass the port number from the client to the app service, the underlying local proxy is | ||
going to include the port number as a part of a special subdomain in the SNI, e.g. | ||
`app-teleport-proxy-target-port-1337.teleport.cluster.local`. The ALPN proxy is then able to extract |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you explored how kubectl port-forward
addresses this issue?
kubectl port-forward
operates as an HTTP request, with the ports specified in the HTTP parameters. This request is then upgraded to either SPDY or WebSocket. The client and server negotiate the ports and multiplex all connections over the same link. With SPDY, this is done by creating an HTTP stream for each port plus one additional stream for errors. In the case of WebSockets, the data is prefixed with an integer representing the port, as Go doesn't support HTTP/2 WebSockets.
We can implement a similar approach, where everything is multiplexed through a single connection and maintained within the same session. However, if you're relying on SNI, each proxy or agent may receive requests for different ports, which could complicate audit logging and session recording.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is definitely nicer from a performance point of view - when I put my "Machine ID" hat on - I can easily see cases where someone may want to connect to a large number of ports. If we can handle all of that within a single TLS connection, it's much more performant. I think only concern would be head of line blocking with TCP which makes me think about QUIC here - but perhaps it's a little early to start going down that road.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
QUIC is preferable, but implementing it will require adding support to the Teleport API, especially if we choose the HTTP request-stream approach. I believe using this method would also allow us to extend these APIs with additional parameters in the future without relying on SNI tricks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does seem like a much better approach and I can see how it's much more future-proof than the SNI shenanigans. My main worry is that if I wanted to go with this, I'd have to start from scratch and, as I told Tiago over DMs, I have little idea where to begin.
With the SNI implementation, I estimated that I have a high chance of not finishing this work within Q3. But at least I have a proof of concept already (#45483). Figuring out how to implement this through HTTP/2 is likely going to add another couple of weeks.
After the discussion on Slack, I've updated the RFD to make multi-port TCP access work by putting the port number in This of course has its problems when accessing many different ports (especially with per-session MFA enabled), but it satisfies the needs of the design partner who would like to give this a try. The two most interesting sections to read after this update are:
|
@zmb3 @nklaassen @espadolini @smallinsky Ping, I'm moving on with the implementation unless you stop me. I planned it out so that I start with configuration and the app spec (#46758) which should be the same no matter how we choose to pass the port to the app service. |
+ // End describes the end of the range, inclusive. It must be between 2-65535 and be greater than | ||
+ // Start when describing a port range. When describing a single port, it must be set to 0. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would remove the special case of 0 and require the [start, end]
inclusive interval to be nonempty and contained in [1, 65535]
(which means 1 <= start <= end <= 65535
, with single ports resulting in start == end
but requiring no extra code).
Then again, kubernetes networkpolicies use something like the following, so there is prior art to having a zero/empty/missing end value to define a single port, so I am not super attached to the suggestion of always requiring a range.
ports:
- protocol: TCP
port: 1234
- protocol: TCP
port: 32000
endPort: 32768
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to disallow or implicitly convert apps that have one single-port range into a standard single-port app? Do we want to push users to define apps as multi-port even when they're only using a single port?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i kinda like port/endPort because "start" is pretty awkward on its own
ports:
- start: 1234
- start: 2345
feels worse than
ports:
- port: 1234
- port: 2345
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, as discussed on Slack, I'll have to redo how the ports are defined because it seems like my initial idea here (ports: [4080-4090, 4095]
) requires too much effort to be feasible at the moment.
i kinda like port/endPort because "start" is pretty awkward on its own
Agree, that's why I'm partial to following what k8s does too. I know Edoardo mentioned on Slack how people who use automation to set up apps are not going to care about shorthands, but I feel like port/endPort strikes a good balance between first-time experience of trying out stuff by hand, setting up automation around it and writing code for (un)marshalling and validation.
Do we want to disallow or implicitly convert apps that have one single-port range into a standard single-port app? Do we want to push users to define apps as multi-port even when they're only using a single port?
That's an interesting question. I'll think about it and come back to it tomorrow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to push users to define apps as multi-port even when they're only using a single port?
I think we don't as this appears to be strictly worse from the UX perspective. Having a single uri
field feels much better than being forced to separate it into uri
and ports
.
If we did this all over again, I suppose it'd be best to have something like target_host
and target_hostname
+ ports
for single-port and multi-port apps respectively. The current approach of cutting the port number from uri
is a bit weird, but we do this for backwards compatibility.
Do we want to disallow or implicitly convert apps that have one single-port range into a standard single-port app?
I'm partial to not doing this in order to keep the rules of the system simple. Either you use only uri
and the app is single-port, or you use uri
+ ports
and the app is multi-port. Going from one type to the other has consequences and I think I'd want it to be more involved than just modifying the number of ports in the list (which would probably be easy to trigger with some automation).
I can imagine a cluster admin who looks at the app spec reference somewhere, notices the ports
field and decides to define all apps like this, no matter if those apps need multi-port access or not. Assuming we won't be providing any special treatment here, the biggest effect it'd have is that connections over VNet to those apps would need to use the correct port. This in turns mean that the UIs would need to list ports.
I haven't thought about it before, but generating a cert for one type of app and then changing the app type on the backend has consequences too.
- Getting a cert for a single-port app which then becomes multi-port means the cert allows access only to the first port from the app spec.
- This is also what happens when an old client requests a cert for a multi-port app.
- Getting a cert for a multi-port app which then becomes single-port means that the cert allows access to the port from
uri
in the app spec, no matter whatRouteToApp.TargetPort
in the cert says.- We could technically disallow access in that case too. But given that we don't plan to do RBAC on ports for now, I think this behavior is okay.
+ // End describes the end of the range, inclusive. It must be between 2-65535 and be greater than | ||
+ // Start when describing a port range. When describing a single port, it must be set to 0. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i kinda like port/endPort because "start" is pretty awkward on its own
ports:
- start: 1234
- start: 2345
feels worse than
ports:
- port: 1234
- port: 2345
VNet [creates a proxy for an app](https://github.com/gravitational/teleport/blob/0b3163e7bd70afa5a2ddb5bd42f3efc034377d70/lib/vnet/app_resolver.go#L355) | ||
at the point of [resolving an A query for the app](https://github.com/gravitational/teleport/blob/0b3163e7bd70afa5a2ddb5bd42f3efc034377d70/lib/vnet/vnet.go#L567). | ||
With multi-port, VNet needs to create a local proxy at a later step, [in `vnet.NetworkStack.handleTCP`](https://github.com/gravitational/teleport/blob/b5df678e22d26776aa87f9dd493b3c443e013fa4/lib/vnet/vnet.go#L377), | ||
where it can read the port used by the connection (`req.ID().LocalPort`). It needs to maintain a map | ||
of port numbers to local proxies to avoid creating a new proxy and a cert on each connection. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might be worth a mention of the intended strategy for VNet to select an app:port and how we expect that to behave and perform
VNet would still have to query for the app spec by the public addr while resolving the DNS A (or AAAA) request. At this point we would not know the port that the client will later want to dial to. Should we support multiple apps with the same public_addr and different port ranges? I think it's possible edit: not possible
We'll probably hold on to this app spec in the tcpAppHandler, and when receiving an incoming TCP connection, check if the port matches the app spec. A question is will we ever update this app spec if ports get added to it or removed? Could we try to update it if we get a request on a port we don't know about? this could be an improvement after the initial implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
Minor question - how will we behave if the port is specified in the URI and the user also adds the ports:
section to the app spec? Will this be considered an invalid app resource, will we include all such ports, or will we ignore the port in the URI?
Those will be considered invalid:
|
Rendered