Telechat Review of draft-ietf-sshm-ssh-agent-12
review-ietf-sshm-ssh-agent-12-intdir-telechat-wessels-2025-11-20-00

I am the assigned int-dir reviewer for this draft. These comments
were written with the intent of improving the Internet area aspects
of the IETF drafts. Please wait for direction from your document
shepherd or AD before posting a new version of the draft. For
background on int-dir, please see the
[FAQ](https://wiki.ietf.org/en/group/intdir).

Document: draft-ietf-sshm-ssh-agent-12
Reviewer: Duane Wessels
Review Date: 2025-11-20

Overall I found the draft easy to follow and understand, except for
a small number of places that might benefit from clarification.


Minor issues:

In Section 3.1, under SSH_AGENT_FAILURE it says "or a request-specific
success message".  Should this be "request-specific failure message"?

In Section 3.4 there seems to be some inconsistent requirements for
removing keys.  For SSH_AGENTC_REMOVE_ALL_IDENTITIES it says "an
agent shall delete all keys" (lowercase?) and for
SSH_AGENTC_REMOVE_SMARTCARD_KEY it says "SHOULD ... remove all
[matching] keys".

Section 7.3 says "Future message number allocations" but it probably
should be "Future signature flag allocations".



Nitpicks / clarifications:

Section 1.1 says "The purpose of this document is to describe the
protocol as it has been implemented."  I think that is not quite
accurate since the document status is Proposed Standard and asks
for new registries to be created with expert review, etc.  If the
statement in 1.1 is removed before publishing as RFC as requested
then this probably doesn't matter.

Section 2 says "an agent may allow only local clients of an agent
to add or remove keys".  Maybe this would be better as "an agent
may allow only clients local to itself to add or remove keys"?

In section 3, both the first description of messages and the one
in section 3.2 describe a field named "contents". i.e., lines 235
and 269.  I could live with it but admit I found it a little confusing
on first read.

The start of section 3 says "the symbolic names of the messages are
shown".  Should this be "symbolic names of the types" instead?

Section 3.1 describes generic response messages and the fact that
they can also contain additional fields.  Later, starting with
section 3.5, we see response types other than SSH_AGENT_SUCCESS and
SSH_AGENT_FAILURE.  It might be good to clarify in 3.1 that a
response can have non-generic types as well as non-generic fields?

Section 3.2 says ""comment" is an optional human-readable key name
or comment as a UTF-8 string".  I believe it is not optional in an
SSH_AGENTC_ADD_IDENTITY message though, correct?  It would be present
as a zero-length string?

Section 3.2 says "with an empty constraints section".  Maybe "empty
constraints field" here instead?

Section 3.2 says "then it MUST fail gracefully and respond with a
SSH_AGENT_FAILURE reply."  To me, "fail gracefully" sounds strange
here.  Maybe just "MUST respond with ..."?

Section 3.2.5 says "Vendor-specific key types MUST use the domain-
qualified naming convention defined in Section 4.2 of [RFC4251]".
I think there was an earlier review that suggested Section 6 of
that RFC would be better and you made those changes. Just wanted
to check if this one should also be changed.

Section 5.3 says "An SSH client SHOULD be prepared to handle multiple
concurrent active agent connections."  Does this mean "active agent
forwarding connections" versus connections between the client and
one or more agents?

Section 5.3 says "... when agent forwarding is not desired by the
SSH client or requested but an SSH server ..."  I found this a
little difficult to parse.  Maybe better as something like "...when
agent forwarding is neither requested nor desired by the SSH
client..."?

Section 6 uses the phrase "message numbers" or just "numbers" to
describe what was "type" at the start of section 3.  Maybe add
something to section 3 forward referencing the use of message numbers
in the later sections?