Re: [netconf] Opsdir last call review of draft-ietf-netconf-ssh-client-server-36

[Kent Watsen <[e-mail]>]

Hi Qin,

Thank you for the OpsDir review.
Please find responses below.

FWIW, many of your comments (and my responses) apply to the tls-client-server
draft as well.

Kent


> On Feb 5, 2024, at 7:51 AM, Qin Wu via Datatracker <[e-mail]> wrote:
> 
> Reviewer: Qin Wu
> Review result: Ready
> 
> I have reviewed this document as part of the Operational directorate's ongoing
> effort to review all IETF documents being processed by the IESG.  These
> comments were written with the intent of improving the operational aspects of
> the IETF drafts. Comments that are not addressed in last call may be included
> in AD reviews during the IESG review.  Document editors and WG chairs should
> treat these comments just like any other last call comments.
> 
> This document defines 7 YANG modules for Communication between SSH Client and
> SSH Server. In these 7 YANG modules, there are three "IETF" modules and four
> "IANA" modules. This document is well written and I believe it is ready for
> publication. However I have a few comments on the latest version v-36: Major
> issues: None
> 
> Minor issues
> 1. The abstract is not consistent with Introduction section, in the abstract,
> only 3 YANG modules are described but in section 1 or introduction section, 7
> YANG modules are explored. Suggest to change abstract to align with section 1.

Quite right.  Below is the updated abstract.  I made a similar update to the TLS draft.

   This document defines seven YANG 1.1 modules.  Three "IETF" modules,
   and four supporting "IANA" modules.

   The three IETF modules are: ietf-ssh-common, ietf-ssh-client, and
   ietf-ssh-server.  The "ietf-ssh-client" and "ietf-ssh-server" modules
   are the primary productions of this work, enabling the configuration
   and monitoring of SSH clients and servers.

   The four IANA modules are: iana-ssh-encryption-algs, iana-ssh-key-
   exchange-algs, iana-ssh-mac-algs, and iana-ssh-public-key-algs.
   These modules each define YANG identities for an IANA-maintained
   algorithm registry.




> 2. The title of each subsection of section 5.1 doesn’t reflect what is
> documented within it, I think each subsection focuses on security
> consideration, suggest the following change to each subsection of section 5.1
> s/ Template for the ……/ Consideration for the ……

I was just thinking exactly the same.  The “Template for” prefix was
suggested in one of the IESG-reviews but, in context, “Considerations
for" makes more sense.

Fixed in all nine drafts.



> 3. Appendix A.1 said: “ This
> section provides an overview of the "iana-ssh-encryption-algs" module in terms
> of its identities and protocol-accessible nodes.
> 
> ”
> It is clear that iana-ssh-encryption-algs" module doesn’t define
> Any protocol-accessible nodes such as container node, list node, leaf node,
> etc. It only defines identities and typedefs.Suggest the following change as
> follows: NEW TEXT: “This section provides an overview of the
> "iana-ssh-encryption-algs" module in terms of its identities and typedefs. ”

Fixed in both the SSH and TLS drafts.


> 4.
> Appendix A.2 said: “   This section provides an overview of the
> "iana-ssh-mac-algs" module
>   in terms of its identities and protocol-accessible nodes.
> “
> It is clear that  "iana-ssh-mac-algs " module doesn’t define
> Any protocol-accessible nodes such as container node, list node, leaf node,
> etc. It only defines identities and typedefs. Suggest the following change as
> follows: NEW TEXT: “This section provides an overview of the "
> iana-ssh-mac-algs" module in terms of its identities and typedefs. ”

Fixed (per above comment)


> 5.
> Appendix A.3 said: “
>   This section provides an overview of the "iana-ssh-public-key-algs"
>   module in terms of its identities and protocol-accessible nodes.
> “
> It is clear that  " iana-ssh-public-key-algs " module doesn’t define
> Any protocol-accessible nodes such as container node, list node, leaf node,
> etc. It only defines identities and typedefs. Suggest the following change as
> follows: NEW TEXT: “This section provides an overview of the "
> iana-ssh-public-key-algs " module in terms of its identities and typedefs.

Fixed (per above comment)


> 6.  Appendix A.4 said:
> “
>   This section provides an overview of the "iana-ssh-key-exchange-algs"
>   module in terms of its identities and protocol-accessible nodes.
> ”
> It is clear that  " iana-ssh-key-exchange-algs " module doesn’t define
> Any protocol-accessible nodes such as container node, list node, leaf node,
> etc. It only defines identities and typedefs. Suggest the following change as
> follows: NEW TEXT: “This section provides an overview of the "
> iana-ssh-key-exchange-algs " module in terms of its identities and typedefs.

Fixed (per above comment)


> 7. Section 6 IANA Section
> Section 6.3,Section 6.4, Section 6.5, Section 6.6 describe IANA maintained
> module, but doesn’t follows Guidance for Writing the IANA Considerations for
> RFCs Defining IANA-Maintained Modules defined in RFC8047bis. So the question is
> for IANA maintained Module, if such module shadows algorithm name sub-registry
> of SSH protocol parameters registry, should IANA section follow guidance for
> Defining IANA Maintained Modules defined in RFC8047bis, My understanding is
> yes. Similar example can be found in the section 3 of RFC7224.

Yes, they should follow rfc8047bis template.

I just updated the four instances in this document and the one instance in the TLS document.



> 8. Appendix A
> It looks the gen-identities python code is specific to SSH related encryption
> algorithm, I am wondering whether there is more generic code for get identity
> or get enum to automatically help IANA generate IANA maintained modules every
> time the new value is added? If the answer is, I think such code can be
> integrated into RFC8407bis as well.

The problem with that is that each IANA registry is different:
	- column names
	- column ordering
	- how deprecate/obsolete are expressed, if at all
	- how reserved/unassigned entries are expressed
	- etc.

The `gen-identities.py` script included in the SSH and TLS drafts are 85% overlap, but vary in important ways.


> 9. There are 31 errors spot by the current YANG validation tool. It looks line
> wrapping per RFC 8792 is root cause for such errors. I am wondering whether
> YANG validation tool in the datatracker should be upgraded to ignore line
> folding per RFC8792 or provide YANG validation check after removing line
> wrapping from YANG module code.

Fixed.  It was easy once I found the “textwrap” Python library.


PS: I could publish these update now, but I think that I’ll wait until getting (hopefully) closure on the first three drafts going thru IESG review...

Kent