Skip to content
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

Adds support for SharedDirectoryTruncateRequest and SharedDirectoryTruncateResponse messages #40068

Merged
merged 2 commits into from
Apr 5, 2024

Conversation

ibeckermayer
Copy link
Contributor

@ibeckermayer ibeckermayer commented Mar 30, 2024

Fixes #35269

changelog: Fixes a bug in desktop directory sharing which was preventing files from being properly truncated.

Due to the rust overhaul in v15, it would be a separate project to "backport" this to v13 and v14; for that reason I am going to let the bug stand in those versions barring specific complaints by customers.

Comment on lines 261 to 262
default:
this.logger.warn(`received unsupported message type ${messageType}`);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this is backwards compatible by default because an old client will hit this default case and log the message but otherwise continue to function. Is that right?

Copy link
Contributor Author

@ibeckermayer ibeckermayer Apr 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the truncate would simply be ignored were it to reach here. However, that would mean we have a vN w_d_s operating with a v(N-1) proxy, which iirc isn't supported in general.

Presuming that, we don't have to worry about backwards compatibility when adding new server-initiated message chains (such as this one).

| message type (33) | completion_id uint32 | directory_id uint32 | path_length uint32 | path []byte | end_of_file uint32 |
```

This message is sent by the server to the client to request a file be truncated to `end_of_file` bytes.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we be more specific w.r.t what is the server and what is the client.

In standard RDP it's pretty clear, but our architecture introduces some ambiguity - the w_d_s is an RDP client, but can also be considered the server to our web-based client.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a note on the terminology in 197a123

@@ -622,6 +622,15 @@ func (c *Client) startInputStreaming(stopCh chan struct{}) error {
return trace.Errorf("SharedDirectoryMoveResponse failed: %v", errCode)
}
}
case tdp.SharedDirectoryTruncateResponse:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need some backwards compat logic here, or is the thought that we will never send a response to an old w_d_s that doesn't understand it because said w_d_s would have never sent the initial request?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we will never send a response to an old w_d_s that doesn't understand it because said w_d_s would have never sent the initial request?

Yes, only a w_d_s that sends a truncate request will ever receive a truncate response, and all w_d_s's that send truncate requests can handle truncate responses.

@ibeckermayer ibeckermayer requested a review from zmb3 April 1, 2024 17:23
@zmb3 zmb3 enabled auto-merge April 5, 2024 19:33
@zmb3 zmb3 added this pull request to the merge queue Apr 5, 2024
Merged via the queue into master with commit 8695d51 Apr 5, 2024
42 checks passed
@zmb3 zmb3 deleted the isaiah/shared-directory-truncate branch April 5, 2024 19:54
@public-teleport-github-review-bot

@ibeckermayer See the table below for backport results.

Branch Result
branch/v15 Create PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Desktop Access Directory Sharing: TDP Truncate
3 participants