guacd: Exposing filesystem to non-owner users

[Or Cohen <or...@strigo.io>]

Hi,

I have a use-case where I need to enable other users to access the
filesystem object. Looking at the (server) code, I saw that it's not shared
with users that are not the owners and that it's intentional.
I also saw that it's consistent, e.g. not responding to file download
requests if the user is not the owner.

I'm wondering if this choice is due to some limitation (shared data between
threads, etc.) I'm not aware of, or some behavior that is chosen because
this is what the users expect.

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?

Cheers,
Or.

Re: guacd: Exposing filesystem to non-owner users

[Mike Jumper <mi...@guac-dev.org>]

On Sun, Dec 10, 2017 at 7:04 AM, Or Cohen <or...@strigo.io> wrote:

> On Sun, Dec 10, 2017 at 2:09 AM, Mike Jumper <mi...@guac-dev.org>
> wrote:
>
> > On Thu, Dec 7, 2017 at 6:44 AM, Or Cohen <or...@strigo.io> 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

Re: guacd: Exposing filesystem to non-owner users

[Or Cohen <or...@strigo.io>]

On Sun, Dec 10, 2017 at 2:09 AM, Mike Jumper <mi...@guac-dev.org>
wrote:

> On Thu, Dec 7, 2017 at 6:44 AM, Or Cohen <or...@strigo.io> 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.

Re: guacd: Exposing filesystem to non-owner users

[Mike Jumper <mi...@guac-dev.org>]

On Thu, Dec 7, 2017 at 6:44 AM, Or Cohen <or...@strigo.io> wrote:

> Hi,
>
> I have a use-case where I need to enable other users to access the
> filesystem object. Looking at the (server) code, I saw that it's not shared
> with users that are not the owners and that it's intentional.
> I also saw that it's consistent, e.g. not responding to file download
> requests if the user is not the owner.
>
>
Correct.

I'm wondering if this choice is due to some limitation (shared data between
> threads, etc.) I'm not aware of, or some behavior that is chosen because
> this is what the users expect.
>
>
Only human limitations. ;)

Implementing file transfer for only the owner simplified the initial
release of screen sharing support. There is no technical reason why this
can't be done, with the original intent being to provide flags allowing
administrators to explicitly enable filesystem access for joining users via
the parameters submitted when joining.

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.

- Mike