Re: X forwarding

[nisse%lysator.liu.se@localhost (Niels Möller)]

der Mouse <mouse%Rodents.Montreal.QC.CA@localhost> writes:

> > Please write up a spec for this.
> 
> Sure.  See below.

Some comments. Unfortunately, it's several months since I hacked on
the X11-forwarding, so I don't remember all the relevant details.

> To request X forwarding, the client sends
> 
>      byte      SSH_MSG_CHANNEL_REQUEST
>      uint32    recipient channel
>      string    "fixed-x11-req%rodents.montreal.qc.ca@localhost"
>      boolean   want reply
>      uint32    token
>      uint32    x11 screen number

Looks ok to me. I'm not sure about the single-connection thing, at
times I wished for a similar feature on the general tcpforwarding. You
could rename "token" to "display id", as it's meant to identify an
X11 display.

> Upon getting a forwarded X connetion and its passing whatever security
> checks, the server generates
> 
>      byte      SSH_MSG_CHANNEL_OPEN
>      string    "fixed-x11%rodents.montreal.qc.ca@localhost"
>      uint32    sender channel
>      uint32    initial window size
>      uint32    maximum packet size
>      uint32    token from the corresponding CHANNEL_REQUEST
>      byte[6]   first six bytes of the client X setup packet (the byte
>                 order and version numbers)
>      string    connection method ("tcp" for TCP, others not yet
>                 specified - I'm imagining things like "decnet" and
>                 "local" for DECnet sockets and /tmp/.X11-unix/* - see
>                 below for more on this).
>      connection-method-specific data
>      for method "tcp":
>          string    originator address (e.g. "192.168.7.38")
>          uint32    originator port

I think this is a little too complex for my taste. I think I
understand why you want to include the part of the initial X message
that comes before the authentication data, but it does make the
request more dependent on X protocol details than I really like. I
think I'd prefer something like

     byte      SSH_MSG_CHANNEL_OPEN
     string    "fixed-x11%rodents.montreal.qc.ca@localhost"
     uint32    sender channel
     uint32    initial window size
     uint32    maximum packet size
     uint32    token from the corresponding CHANNEL_REQUEST
     string    prefix
     string    originator address
     uint32    originator port

  The prefix is the initial part of the X11 client's initial message,
  before the authentication data. It's usually 6 bytes, and the first
  byte is the byte-order mark, 'B' for a big-endian client, 'L' for a
  little endian client. (XXX Some X implementations seem to use 'b"
  and 'l' instead. I'm not sure if that is something we need to care
  about, or what a canonical X reference says about the case of the
  byte order characters).

As for the connection information, I don't quite like having the
structure of the request to depend on the connection type. If you
*really* want to support arbitrary connection information, I think I'd
prefer something like

  string    connection type  ("tcpip" or "foo%example.com@localhost")
  string    address blob     (internal structure depends on the type).

But we already have at least threee types of addresses mapped into a
string format (dns, ipv4, ipv6), so perhaps it's simpler to use just a
pair <string, number>, where the number part need not be used for
connection types where it isn't relevant.

I think it is unfortunate to introduce a new type of address
abstraction just for x11 forwarding; if the address abstraction in the
connection document isn't general enough, it's better to fix it there
(or in the architecture spec).

>      byte      SSH_MSG_CHANNEL_REQUEST
>      uint32    recipient channel
>      string    "cancel-x11-req%rodents.montreal.qc.ca@localhost"
>      boolean   want reply
>      uint32    token
>      uint32    x11 screen number
> 
> This cancels all previous X forwardings for that channel, token, and
> screen number.

You should also say what should happen when a channel is closed. It
would make sense to me to say that a channel close on the session, or
a cancel-x11-request, should cause the ssh server to stop accepting
new x11 connections, but let existing forwarded channels live.

Finally, one alternative to using a special token to identify displays
is to just add the id of the associated session to the "x11"
CHANNEL_OPEN message,

     byte      SSH_MSG_CHANNEL_OPEN
     string    "x11-mk2"
     uint32    sender channel
     uint32    initial window size
     uint32    maximum packet size
!    uint32    session channel (receiver's id for the associated session)
     string    originator address (e.g. "192.168.7.38")
     uint32    originator port

This is equivalent to the new display id token, under the assumptions that

  No new x11 channels can be opened after the session channel is
  closed.

  A session can only have a single forwarded x11 display.

Both of these make sense to me.

Regards,
/Niels