On Sun, Dec 10, 2017 at 7:04 AM, Or Cohen <[email protected]> wrote: > On Sun, Dec 10, 2017 at 2:09 AM, Mike Jumper <[email protected]> > wrote: > > > On Thu, Dec 7, 2017 at 6:44 AM, Or Cohen <[email protected]> wrote: > > > > I was thinking of adding filesystem support to non-readonly non-owner > users > > > and I see that I need to change this in a few places (join handler, > file > > > request handlers, maybe more?). Is it easy as just making sure new > users > > > get a filesystem object and matching file download/upload requests, or > > am I > > > missing something? > > > > > > > > That sounds about right. I would suggest tying filesystem access to > > existing enable flags, like the existing "enable-drive" and "enable-sftp" > > parameters, rather than combining everything into "read-only". I doubt > that > > assuming all non-readonly users should have access to available > filesystems > > is a safe assumption. > > > > Yeah, you're right. I mentioned read-only because it can be expected > that users with write (non-readonly) access also have filesystem access, > i.e. it won't seem weird in that context. > > So I'm looking at the ssh protocol client and I checked if libssh2_sftp_* > operations are thread-safe, and seems like they aren't. > Regardless, I also see that file uploads actually have a listener for > non-owners. > > I'm trying to think of a way to workaround the thread-safety issue, if it's > indeed > an issue. Since it's implementation dependent, maybe limit file operations > on > the protocol level? i.e. respond with "SFTP: Too many streams" or > something. >
If libssh2's handling of SFTP is not threadsafe, then the best route would depend on whether SSH supports multiple simultaneous transfers via SFTP on the same session. Assuming it does (I believe this is the case), then adding the necessary mutexes to the SFTP abstraction used for file transfer by the various protocols (and to RDP's drive abstraction) would be sufficient, and would result in the blocks of multiple simultaneous transfers being interleaved. From the user's perspective, things would just work. If simultaneous transfer of multiple streams is not supported by SSH/SFTP at all, then yes, responding with an "ack" containing an appropriate error code would be correct. GUAC_PROTOCOL_STATUS_SERVER_BUSY would probably be the best choice there. In this case, the user would see the following message when they attempt to transfer a file while another transfer is in progress: "Too many files are currently being transferred. Please wait for existing transfers to complete, and then try again." Before resorting to simply allowing one transfer at a time, I'd double check that multiple transfers currently work. I'm 99% positive that multiple transfers for a single user are supported for SFTP, and extending that to cover multiple users would be a matter of adding any necessary mutexes. - Mike
