Re: Publickey subsystem draft posted

[Joseph Galbraith <galb-list%vandyke.com@localhost>]

> OK, here are some comments. I'm in favour of a protocol of this type
> existing, but I think this draft is going to need some work before
> it's useful.
>
>  - You don't mention any kind of packet structure within the
>    subsystem channel, like SFTP has. Is this deliberate? It looks at
>    the moment as if the individual components of any given request
>    (e.g. string "add", string comment, string algorithm, string
>    pubkey blob) are sent straight to the subsystem without any
>    overall length field.
>
>    If that's correct and not simply a documentation error, it
>    _completely_ destroys extensibility: if a subsystem receives any
>    request with a name it doesn't recognise, then it cannot know how
>    much of the following data belongs to that request, so after
>    returning REQUEST_NOT_SUPPORTED it _can't_ resume parsing at the
>    next request boundary. Without this basic extensibility property,
>    what was the point of having request types described in an
>    extensible string namespace at all?

It is a documentation error-- thank heavens.

The generalized message format is:

UINT32 length
string type
... request specific data follows

Sections 2.2 and 2.3 should be updated to reflect this.

>  - Speaking of extensibility, I strongly support Nicolas Williams's
>    comment that there already exist SSH server implementations
>    supporting a greater range of public key options and restrictions
>    than your draft specifies. Surely at the very least there ought
>    to be some sort of extension mechanism within the "add" request
>    type?

Yes, this makes sense.  The "command" option was supposed to deal
with the various restrictions supported by openssh and other 
implementations, but...

  a. Since we don't support it, we were kind of shooting in the
     dark as to what people needed, and clearly, we undershot.
  b. As you point out below, it needs to be integrated with the
     add command to prevent 'vulnerability windows'

>  - The "command" request is seriously under-specified. It does not
>    mention whether the key in question is expected to have been
>    added using "add" previously, or is added by the "command"
>    request itself.
>
>     * If the former, this is a security hazard if naively
>       implemented: the _only_ way to set up a restricted key is by
>       issuing "add" and then "command", but between those two
>       requests, there is a window of opportunity in which someone
>       possessing the private key can gain unrestricted access to the
>       target account. Since restricted keys are typically keys you
>       don't fully trust not to fall into enemy hands, there should
>       never be such a window of opportunity. Therefore, an
>       implementation MUST delay actually making the changes until
>       the subsystem channel is closed or a subsequent
>       synchronisation message is sent; this should be clearly
>       specified.
>
>     * If the latter, this would make far more sense to me, except
>       that the comment field present in the "add" request is missing
>       in the "command" request.
>
>    Since I've already proposed an extensibility field in the "add"
>    request, I'm tempted to suggest that "command" is a complete
>    misfeature and ought to be implemented in terms of that instead.

What if the "add" command where changed as follows?

	string "add"
	string public-key algorithm
	string public-key blob
	uint32 attribute-count
		string attrib-name
		string attrib-value
		bool   mandatory
	repeated attribute-count times

  The server MUST attempt to store the public key for the user in the
  appropriate location so the public key can be used for subsequent
  public-key authentications.

  attrib-names without a '@' are reserved to the IETF.  Extensions
  MAY be used by composing the '@' with a DNS name, for example,
  'spiffy-extension%vandyke.com@localhost'

  If the server does not implement a mandatory attribute, it MUST fail
  the add.  For the purposes of a mandatory attribute, storage of the
  attribute is not sufficient, but requires that the server understand
  and implement the intent of the attribute.

  The following attributes are defined by this draft.

  "comment"
	The comment field contains user-specified text
	about the public key.  The server SHOULD make every
	effort to preserve this value and return it with the
	key during a list operation.

	The comment field is useful so the user can identify the
	key without resorting to comparing it's fingerprint.

  "command"
	Command bypasses the session channel "exec" and "shell"
        requests by always execution the specified command.

	This attribute SHOULD be mandatory.

  "restrict"
	The value of this attribute contains server functions that
	may not be preformed when this key is used.  It is a comma
	seperated list.  Elements containing an '@' are reserved for
	implementation / site specific extension; all other elements
	are reserved for use by the IETF.

	Currently defined restrictions are:

		"x11"
		"shell"
		"exec"
		"port-forward"
		"reverse-forward"

	This attribute SHOULD be mandatory.

  For example, we might have:

  "add"
  "ssh-rsa"
  public-key-blob
  4
  "comment"
  "2048 bit RSA key for my daughter"
  "special%openssh.org@localhost"
  <special data, could be binary>
  "restrict"
  "port-forward,reverse-forward,x11,exec"
  "command"
  "daughter-shell"

The 'list' command would need to be updated to have
a similar extension format.

What do you think?  What other extensions should we
define here?  What other types ofrestrictions should
be defined?

- Joseph