Early Review of draft-ietf-sshm-ssh-agent-07
review-ietf-sshm-ssh-agent-07-artart-early-thomson-2025-09-28-00

Overall, this seems pretty clear.  My notes below are made with the intent of
making it clearer and more precise.  However, it's clear that this protocol has
solid implementation experience, because it clearly fits together well.  This
is an extremely sharp tool to leave lying around, but the security
considerations address the sorts of things that matter; or at least I was
unable to think of anything that would be necessary to address beyond the
clearly identified need for authentication and proper authorization.

# Comments

This is the first document I've seen to include an RFC editor note in the
abstract.

There are a number of notes that I would not remove from this document.  Those
in S1.1 and S2.1 both contain information that would be useful to a reader.  (A
specification exists not just to specify, but to contextualize and these seem
to be highly relevant context.)

In the background section (which probably shouldn't be removed as noted), this:

> This agent protocol is already widely used and a de-facto standard, having
been implemented by a number of popular SSH clients and servers for many years.
The purpose of this document is to describe the protocol as it has been
implemented.

This makes me question whether the intended status (proposed standard) and the
note are consistent.  Who holds change control of this protocol?  If this is
documenting a de facto standard, I'd expect informational status.

In S2, there are three roles mentioned: client, server, and agent.  On first
read, it is not clear that the agent in question (which is a very generic term)
is the server.  This confusing interchangeable use of server and agent
continues throughout.  This is compounded by the statement in the introduction:
"Clients (and possibly servers) can invoke the agent".

S2 fails to say what the protocol is *for*.  The description in S1 isn't
complete, but I would expect S2 to say what a client (or server?) can do with
the aid of a client.

S3.1 describes generic messages, but fails to explain whether these contain
more information than the code.  Or even that these byte values are in fact
message types.  More words, even if they might be redundant, would help here.

In S3.2, the use of byte[] is novel and not something that is defined in RFC
4251.  I understand this to mean "some unspecified number of bytes", where the
specific number is determined by the type of key being transferred.  This means
that the recipient will need to know the format of the key type in order to
make sense of this and (importantly) later fields.  It's almost not useful to
define the generic format of this message given this.  No generic parser will
be able to recover the values successfully; the parser needs to know the
definition of each key type to parse them (and the constraints...).

In S3.2.7, constraints are formatted using byte[], which means that any unknown
constraint makes the entire set of constraints unparseable. This is fine,
because the message has to be rejected if there are unknown constraints, but it
is worth noting this point.

In S3.2.3, the ENC(A) value is encoded twice.  That seems unnecessary.

For S3.2.7.3, are there any known uses of constraint extensions?  Or can we
assume that this extension point is potentially unusable?

Is the "reader id" in S3.4 the same as the "id" in S3.2.6?  The description is
the same, but why the change of name?

S3.6.1 defines two flags, but does not define which bit each corresponds to. 
... I see these much further down.  A forward reference would help.  As for the
values, does 4 mean 0x1000_0000 (the 4th bit counting from MSB), 0x0000_0008
(the fourth bit counting from LSB), or 0x0000_0004 (the value 4)?  I think that
it's the last.

In S3.7, locking and unlocking are idempotent.  Why would attempting to lock an
agent fail if the agent were already locked?  That seems like a success case to
me (you are not asking it whether it was unlocked).

... "An agent SHOULD take countermeasures against brute-force guessing attacks
against the pass-phrase." seems pretty weak in terms of a specification.

In S5, I found "the deployed integration with the SSH protocol uses
vendor-specific names" confusing.  I think that this means that the remainder
of this section defines the protocol extension, which happens to use a
vendor-specific name or two.  This statement could be dropped.

In S7, for new registries, please provide instructions to experts about what
criteria they should use in accepting (or rejecting) registrations.  Also,
please consider changing to specification required instead.

In S8, there is an implication that a client of the agent might cause the agent
to load arbitrary code for smartcards.  I don't believe that this is the case,
but it would be good to have that made much clearer.  That is, an agent might
have the ability to interact with a pre-arranged set of smartcards, some of
which would need code to be loaded.  Loading that code is a risk, but this is a
risk that the agent implementation takes on deliberately, hopefully not as
directed by an arbitrary client.  (Clients in this protocol have considerable
powers, so maybe the security posture could be looser than this, but the whole
discussion of potential side channels makes me think otherwise.)  Whatever the
story, this could be a little crisper about the threat model.

# Nits

In S8 "prevent its memory being read by other processes to direct theft of
loaded keys. This typically include disabling " are two grammatical errors, I
think.  "to direct theft" doesn't parse, and s/include/includes/