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

["Per Andersson (perander)" <[e-mail]>]

Hi!

The following issues were identified during the Document
Shepherd Review of draft-ietf-netconf-ssh-client-server-13.


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


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.

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

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.

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


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


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.

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?


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


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.


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 *

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

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


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


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.


--
Per