Always use the same address format in session trackers #52920
Conversation
d764f65 to
c13a75c
Compare
c13a75c to
59610fe
Compare
It is assumed that the `target_address` of a SessionTracker is the host UUID for SSH sessions. However, this was only the case when in node recording mode. The value is derived from the `ServerID` in the return value from `lib/srv.Server.TargetMetadata`. In node recording mode the value is populated from the host uuid of the process running the SSH service. In proxy recording mode, the value is populated from the server id in the dial request. The discrepency arises from the fact that the router was altering the server id to include the cluster name suffix to populate the prinicpals appropriately. To remedy this some slight tweaks were made to the semantics of the srv.Server.HostUUID and the reversetunnelclient.DialParams. All dial requests for SSH targets now provide the resolved types.Server. This server is used as the source of truth for the hostname and host uuid of the target by the forward server. `(forward.Server) ID()` now returns the proxy host uuid instead of a uuid generated when creating the server. `(forward.Server) HostUUID()` now returns the host uuid of the target server. Fixes #42346.
59610fe to
efdb95e
Compare
| // ID is the unique ID of the server. | ||
| // ID is the unique ID of the server. For the forwarding server | ||
| // this is the UUID of the forwarding proxy, NOT the UUID of | ||
| // the target host. | ||
| ID() string | ||
|
|
||
| // HostUUID is the UUID of the underlying host. For the forwarding | ||
| // server this is the proxy the forwarding server is running in. | ||
| // server this is the UUID of the target host, NOT the UUID of | ||
| // the forwarding proxy. | ||
| HostUUID() string |
There was a problem hiding this comment.
Isn't this change basically just flipping the claimed purpose of these methods? The original meaning of ID() was "id of the target" and the original meaning of HostUUID() was ID of wherever this logic is running. We were already bad about that distinction, and now it seems they are being fully flipped. That seems confusing. I have in progress PRs right now that would have their behavior silently changed by this change, and it seems plausible others might as well.
Admittedly, we're very inconsistent about using these and seem to use them wrong often, so I get the idea of trying to flip them to mean what they are currently being used as, but it might be better to just dump these two names and pick two new and highly unambiguous names. E.g. TargetResourceID and LocalAgentID or something along those lines. That way we make sure that anything affected by this change is actually forced to pick the one it really wants.
There was a problem hiding this comment.
The original meaning of ID() was "id of the target" and the original meaning of HostUUID() was ID of wherever this logic is running
I don't think this is accurate for proxy recording mode. Each forward server was assigned new UUID and that is what was returned by ID(). I do agree that silently changing the meaning of these fields is probably not what we want to do and choosing new, less ambiguous names likely makes more sense.
Renaming and naming choices aside, do you foresee any other issues with the changes in this PR? Now that open dial has been removed I can't see why requiring the dial target to always be a non nil types.Server would cause issue.
There was a problem hiding this comment.
Yeah, I'm 100% on board with requiring that the target is always non-nil.
It is assumed that the
target_addressof a SessionTracker is the host UUID for SSH sessions. However, this was only the case when in node recording mode. The value is derived from theServerIDin the return value fromlib/srv 8000 .Server.TargetMetadata. In node recording mode the value is populated from the host uuid of the process running the SSH service. In proxy recording mode, the value is populated from the server id in the dial request. The discrepancy arises from the fact that the router was altering the server id to include the cluster name suffix to populate the prinicpals appropriately.To remedy this some slight tweaks were made to the semantics of the
srv.Server.HostUUIDand thereversetunnelclient.DialParams. All dial requests for SSH targets now provide the resolved types.Server. This server is used as the source of truth for the hostname and host uuid of the target by the forward server.(forward.Server) ID()now returns the proxy host uuid instead of a uuid generated when creating the server.(forward.Server) HostUUID()now returns the host uuid of the target server.Fixes #42346.