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

[Qin Wu <[e-mail]>]

Kent:
Thank for addressing all my comments, thank for clarification for the python code.
I just feel your python code in the appendix make IANA manager's life easier,:-)

-Qin
-----邮件原件-----
发件人: Kent Watsen [mailto:[e-mail]] 
发送时间: 2024年2月6日 7:15
收件人: Qin Wu <[e-mail]>
抄送: [e-mail]; [e-mail]; [e-mail]; [e-mail]
主题: Re: [netconf] Opsdir last call review of draft-ietf-netconf-ssh-client-server-36

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