Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
TDP Shared Directory Announce and Acknowledge #12405
TDP Shared Directory Announce and Acknowledge #12405
Changes from all commits
24c1a90
41ac367
58cac52
fa3531f
ab4d66d
aaa8369
54b7e3d
071620b
52a3d5d
ceddf84
6baccf6
9036618
770f6e7
17a35e8
c946b52
787b67e
6cd9ac2
997b7d6
eb8ed1a
54b53d0
8f1ead3
9a88738
fb60ebb
51f83e5
b4192a6
6cf694a
fa0d555
4b9c330
66eb895
687b84c
058530b
1543585
549eb02
8c5e2b1
de13b64
8017ef2
1ce587f
66b5b00
de8e052
0feae71
20d9aeb
ec900b8
e43694b
95af26b
eec39f5
c43d688
a56236f
20efe3e
dac6c41
6251502
2cb663d
3615580
7fe5b95
9274c31
7975209
ce1c6c5
1732e90
9ec0cba
ee34ddd
dbedfc8
df26b8f
e0bbfcf
eccab5a
51c956c
8d532c8
750fe60
c4371da
750e7ff
b449124
332af85
416b8a0
aff394c
d773592
ded34fc
7df9b4f
901aa3d
eade41e
9077167
88847b6
ae2e515
9298408
97a5c3d
66dc1a0
3d716ca
0c7b1da
2dc6e32
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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 assume a return here breaks the connection and the session is terminated. Is that desirable? Is there a graceful way to handle the error instead?
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 in keeping with how we handle the other TDP --> RDP translation failures.
This could be improved by sending back a
SharedDirectoryAcknowledge
with a failure code in cases where handle_tdp_sd_announce fails, and only returning aCGOErrCode::ErrCodeFailure
when an irrecoverable failure occurs liketeleport/lib/srv/desktop/rdp/rdpclient/src/lib.rs
Lines 485 to 490 in 332af85
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.
Right. I think if we can't translate a bitmap or something then terminating the connection is probably reasonable.
Question is, how likely is it that we see an error here? If not likely, then we can stick with the existing pattern.
I think this is all out of scope for this PR to be honest, but is makes me wonder whether we should have support for a TDP error message that doesn't kill the connection. We could show a little toast item or something in the UI to indicate a non-critical error but keep the session running.
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.
Based on my experience working with this thus far, I think it's unlikely enough that we are fine to leave this as is for now.
I agree with all of that. I had the same general thought while developing this feature, noticing that we didn't have a consistent system for when we kill the connection (typically by returning an
Err(...)
for anRdpResult
, vs when we send back an RDP response withNTSTATUS::STATUS_UNSUCCESSFUL
, vs when we send a TDP error between those options.I created an issue to remind us to go back and do that: #13041
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.
Thus far, the Rust code didn't know anything about TDP - it just invoked callbacks into Go with some data, and Go handled all TDP.
Have we moved away from this, or is it just a naming thing?
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've decided that we're moving away from this.
It's possible to name this something else, and pass data as function parameters and/or differently named structs (and so on with other similar functions I add in the future), but ultimately that would just be a slight of hand. Functionally, this client would still be carrying out the same process of converting RDP into TDP and vice versa.
I can see an argument for treating this client as a "pure" RDP client, which blindly takes RDP messages it receives and passes them into Go, and which exposes an API that only allows users to call functions that directly send RDP messages to the Windows server. That would require some refactoring of our existing code, and a major overhaul of my stack of existing PR's.
Such a design would offer the benefit of pushing much (all?) of the Teleport specific functionality out of this client (besides the fact that we use Go). It would allow us to do things like, for example, swapping out TDP for another protocol (such as say, Guacamole, or just using RDP directly) without changing this client.
My thought is that we don't have a good reason to refactor this client in that way right now, because we don't have any plans for moving away from TDP. Granted I might be being biased due to my personal investment in building this system out without that design goal in mind, or I may be missing some other important line of reasoning.
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.
Hmm, the crate is literally called "rdpclient" so I'm of the mind it should only do RDP.
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.
Putting this here for documentation: we discussed on a call that our
rdpclient
primarily does RDP at the low level protocol. It will also have functionality to create data structures and call functions with those data structures that are named for the TDP messages they stand for, but it remains blind to the low level details of TDP. After this implementation is working, we should decide whether our naming conventions should change in order to make this separation more explicit.