[art] Re: [Ssh] draft-ietf-sshm-ssh-agent-07 early Artart review

[Damien Miller <[e-mail]>]

On Sun, 28 Sep 2025, Martin Thomson via Datatracker wrote:

> Document: draft-ietf-sshm-ssh-agent
> Title: SSH Agent Protocol
> Reviewer: Martin Thomson
> Review result: Almost Ready
> 
> 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.

Thanks for the detailed review. I've uploaded
https://datatracker.ietf.org/doc/draft-ietf-sshm-ssh-agent-08 to address
most of these comments.

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

Is this a bad thing? I don't know...

> 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.)

I generally agree about S1.1 but I think S2.1 is mostly a historical
curiosity that would only be confusing in the final RFC.

> 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.

I'll defer to the WG chairs and others who are more experienced with
the process here on this.

> 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".

Thanks, I've added a terminology subsection under Protocol Overview and
have tried to clarify the various roles throughout the document.

> 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.

I've added a paragraph Protocol Overview to explain this.

> 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.

Thanks, I've added some text to clarify that these are indeed full responses.

> 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...).

IMO there is value to showing the fields that are common across all
key addition messages, especially when someone in the future consults
this document before adding a new key type.

> 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.

I added some text to mention this.

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

Yeah, that was an implementation oversight that was widely deployed before
this draft was written.

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

Yes, OpenSSH implements a system of restrictions for forwarded keys that
is signalled used constraint extensions. These degrade in a safe way (i.e.
an agent will not accept adding so-constrained keys if it doesn't support
these extensions) if they follow the rule in the Key Constraints section
against accepting unsupported constraints.

https://www.openssh.com/agent-restrict.html has a little more information
on this, but not the protocol mechanics.

> 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?

Good catch, changed to "token id" in all cases.

> 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.

Added

> 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.

Yes, this is consistent with the use of uint32 values throughout the SSH
RFCs. IMO no additional clarification is required here.

> 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).

Because agents are locked with passphrases. I guess one could argue for
idempotence IFF the passphase was identical to the previous request, but
that would create an oracle that indicates whether a passphrase was
correct or not.

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

My goal was to encourage implementors to consider the problem without being
too prescriptive, but yeah - it's not really actionable where it is.

I've moved this to the Security Considerations section.

> 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.

Yeah, and the next subsection goes into more detail. I've removed this.

> 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.

I've added a Guidance for Designated Experts section for this.

> 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.

I've reworded this slightly, but am not sure how to improve it further.

> # 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/

"direct" should have been "prevent" here.

-d