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 <noreply@xxxxxxxx> 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 -- last-call mailing list last-call@xxxxxxxx https://www.ietf.org/mailman/listinfo/last-call
