Re: [netconf] Document Shepherd Review of draft-ietf-netconf-ssh-client-server

[Kent Watsen <[e-mail]>]

Hi Per,

> On Oct 4, 2022, at 10:41 AM, Per Andersson (perander) <perander=[e-mail]> wrote:
> 
> Hi!
> 
> The following issues were identified during the Document
> Shepherd Review of draft-ietf-netconf-ssh-client-server-13.

Thank you for your valuable and very thorough comments!

Also, please see the just-posted updated draft.  

K.


> The description for the server-authentication/ca-certs container
> in the ssh-client-grouping contains a double space, harmless
> but gives a sloppy impression.
> 
> is authenticated if its certificate  has a valid chain
> 
>    -> is authenticated if its certificate has a valid chain

Extra space removed.


> Regarding iana-ssh-key-exchange-algs.yang:
> The identities for key exchange methods
> diffie-hellman-group-exchange-sha1 and diffie-hellman-group1-sha1 should
> be marked with "status deprecated;", as other identities for deprecated
> algorithms (such as gss-gex-sha1-*) are marked as such per from e.g.
> RFC 8732.

Added "status deprecated" for the two identities, and others defined by RFC 8268.


> If the modules listing algorithms are going to be updated in the future,
> and if the data format from the IANA Protocol Registries is stable, it
> would be helpful to also include a method or script for generating
> updated modules. This work probably exists somewhere already, and there
> has been discussions on list including it in the document, but no method
> is presented in the document. This is especially important if an
> algorithm moves to the "MUST NOT" state in the "OK to Implement".

*THIS* is exactly the issue I raised on the list awhile back, that there's no easy way (i.e., a script) for IANA to use to update the modules going forward.  Some hacky scripts were written before, which Jeff Hartley kindly updated slightly to generate updated modules, but agrees that they're not suitable for IANA use.  In the end the chairs decided to punt the issue to the IANA's response to being asked to maintain these modules.


> In the mail thread discussing creating IANA defined modules, a script is
> attached for iana-tls-cipher-suite-algs.yang. Direct link:
> https://mailarchive.ietf.org/arch/msg/netconf/TTMwp3veeBixD5k3NwHL2gyoCGs/
> 
> Similar work for the SSH IANA modules probably exist.
> 
> Further discussion about using XSLT template to create and update the
> IANA modules:
> https://mailarchive.ietf.org/arch/msg/netconf/cmudFQxPMQD67HIEjyT6BhapKFI/
> 
> The Internet Draft draft-boucadair-netmod-iana-registries [A] suggests
> that pre-RFC, a document should include in an appendix XSLT templates or
> scripts; which are to be removed by the RFC editor upon publication.
> 
>    -> Suggest to publish the methods that generated the initial IANA
>       modules in a new revision.

Yes, see comment above.


> Furthermore, [A] suggests adding to IANA maintained modules a motivation
> for why identities or enumerations have been chosen for the module
> contents. This is missing from the document.
> 
>    -> Suggest introducing a section called "YANG Considerations" and
>       presenting why identities are chosen over enumerations. Possibly
>       add it as a sub section to the introduction, since it will
>       probably be rather short.
> 
> See Section 3 Guidelines for IANA-Maintained Modules in [A].
> 
> [A] https://datatracker.ietf.org/doc/html/draft-boucadair-netmod-iana-registries-04

Good comment.  I added the following to the IANA Considerations section:

    Identities are chosen over enumerations for extensibility reasons.


> The examples below did
> not validate correctly with yanglint, even though they seem to be well
> formed and fulfill the must requirements. Perhaps it is an operator
> error during review?
> 
> Note that the generated ietf-ssh-*@*.yang modules from the refs are
> used, wherein they create a container which uses the grouping, used in
> respective example.
> 
> 
> refs/ex-ssh-client-local.xml:
> 
> $ yanglint -m ../ietf-crypto-types\@*.yang ../ietf-truststore\@*.yang \
>    ../ietf-keystore\@*.yang ../ietf-ssh-common\@*.yang ./ietf-origin.yang \
>    [e-mail] \
>    ex-ssh-client-local.xml \
>    ../../trust-anchors/refs/ex-truststore.xml \
>    ../../keystore/refs/ex-keystore.xml
> Segmentation fault
> 
> 
> refs/ex-ssh-client-keystore.xml:
> 
> $ yanglint -m ../ietf-crypto-types\@*.yang ../ietf-truststore\@*.yang \
>    ../ietf-keystore\@*.yang ../ietf-ssh-common\@*.yang ./ietf-origin.yang \
>    [e-mail] \
>    ex-ssh-client-keystore.xml \
>    ../../trust-anchors/refs/ex-truststore.xml \
>    ../../keystore/refs/ex-keystore.xml
> Segmentation fault
> 
> 
> refs/ex-ssh-server-local.xml:
> 
> $ yanglint -m ../ietf-crypto-types\@*.yang ../ietf-truststore\@*.yang \
>    ../ietf-keystore\@*.yang ../ietf-ssh-common\@*.yang ./ietf-origin.yang \
>    [e-mail] \
>    ex-ssh-server-local.xml \
>    ../../trust-anchors/refs/ex-truststore.xml \
>    ../../keystore/refs/ex-keystore.xml
> Segmentation fault
> 
> 
> refs/ex-ssh-server-keystore.xml:
> 
> $ yanglint -m ../ietf-crypto-types\@*.yang ../ietf-truststore\@*.yang \
>    ../ietf-keystore\@*.yang ../ietf-ssh-common\@*.yang ./ietf-origin.yang \
>    [e-mail] \
>    ../../trust-anchors/refs/ex-truststore.xml \
>    ../../keystore/refs/ex-keystore.xml \
>    ex-ssh-server-keystore.xml
> Segmentation fault

I recognize these as coming from the ref/validate-all.sh script, which is run each time I build the draft...

My guess is that your pyang may be out of date.  FWIW, I have yanglint 2.0.211.


> A minor editorial change is that the iana-*.yang set of modules
> could squash the current two revisions to only one revision, i.e.
> "Inital version", since they will be officially released when the
> document is published.

You're right - fixed (in the tis-client-server draft too)


> Should the iana-*.yang modules be regenerated before the document is
> published and then replace "June 16th, 2022" with the new date; and the
> same for the YANG module revision date? Or should it be kept as is with
> 2022-06-16 as the date?

These modules were recently updated.  Right now we're leaving it to IANA to determine if updates are needed immediately or not.


> The YANG modules for the IANA algorithm listings should contain a
> section with information corresponding to the first paragraph of
> Section 2.3, listing the normative references in the following YANG
> module. All such references should also be listed in
> Section 7.1 Normative References.
> 
> For iana-ssh-encryption-algs.yang include normative references:
> * FIPS-46-3
> * RFC 4253
> * RFC 4344
> * RFC 5647
> * RFC 8758
> 
> For iana-ssh-key-exchange-algs.yang include normative references:
> * RFC 4419
> * RFC 4432
> * RFC 8268
> * RFC 8308
> * RFC 8731
> * RFC 8732, since RFC 5656 is listed
> * RFC 9142, since RFC 5656 is listed
> 
> For iana-ssh-mac-algs.yang include normative references:
> * RFC 4253
> * RFC 5647
> * RFC 6668
> 
> For iana-ssh-public-key-algs.yang include normative references:
> * RFC 4253
> * RFC 4462
> * RFC 5656
> * RFC 6187
> * RFC 8332
> * RFC 8709
> 
> For ietf-ssh-common.yang include normative references:
> * RFC 8732, since RFC 5656 is listed
> * RFC 9142, since RFC 5656 is listed
> 
> For ietf-ssh-client.yang include normative references:
> * RFC 4252
> * RFC 4254
> * RFC 8341
> * I-D.ietf-crypto-types
> 
> For ietf-ssh-server.yang include normative references:
> * RFC 4252
> * RFC 4254
> * RFC 8341
> * I-D.ietf-crypto-types

All fixed - huge thanks!  :)





> Regarding the ietf-ssh-common grouping transport-params-grouping
> referencing Section 5 Security Considerations later in the document, for
> host-key algorithm compatibility with the private key; there is no such
> presentation of compatible combinations in the current revision, -30, of
> the document. This compatibility matrix was added in revision -08 and
> subsequently removed in revision -19. This compatibility matrix might
> however be out of scope for this document, but another reference to
> relevant RFCs is suitable. Whatever path is taken, the reference needs
> to be updated.

Removed the offending/obsolete text.



> The ietf-ssh-client container keepalives should reference unresponsive
> SSH servers in the description, not TLS servers.
> 
> the aliveness of the SSH server.  An unresponsive TLS
> server is dropped after approximatively max-wait *
> 
>    -> the aliveness of the SSH server.  An unresponsive SSH
>       server is dropped after approximatively max-wait *

Fixed.


> Likewise the ietf-ssh-client leaf max-wait in the keepalives container
> should reference SSH not TLS.
> 
>  no data has been received from the SSH server, a
>  TLS-level message will be sent to test the
>  aliveness of the SSH server.";
> 
>    -> no data has been received from the SSH server, an
>       SSH-level message will be sent to test the
>       aliveness of the SSH server.";

Fixed.


> The user "mary" in ex-ssh-server-local.xml has two public keys listed,
> they are listed as "User A" and "User B". Suggest naming them
> differently to indicate that they are used by the same user "mary" on
> different devices, e.g. "Key #1" and "Key #2".

Done.


> The ietf-ssh-server container keepalives should reference unresponsive
> SSH clients in the description, not SSL clients.
> 
>  the aliveness of the SSL client. an unresponsive SSL
>  client is dropped after approximately max-wait *
> 
>    -> the aliveness of the SSH client. an unresponsive SSH
>       client is dropped after approximately max-wait *
> 
> Likewise the ietf-ssh-server leafs seconds and max-attempts in the
> keepalives container should reference SSH not SSL.
> 
> keepalives/seconds description:
> 
>  if no data has been received from the SSL client,
>  a SSL-level message will be sent to test the
>  aliveness of the SSL client.";
> 
>    -> if no data has been received from the SSH client,
>       an SSH-level message will be sent to test the
>       aliveness of the SSH client.";
> 
> keepalives/max-attempts description:
> 
>  the SSL client before assuming the SSL client is
> 
>    -> the SSH client before assuming the SSH client is

All fixed.


> Section 6.6
> 
> Also mention the "status" statement "obsolete", since it is present in
> the YANG module identities listing.
> 
> Furthermore, the column to track algorithm status seems to be present
> alreday in the IANA Protocol registry for the SSH Protocol.


The must've added that column recently.  Removed the paragraph.


> --
> Per

Thanks again, Per!

Kent