A YANG Model for Transmission Control Protocol (TCP) Configuration and State
draft-ietf-tcpm-yang-tcp-09

Thanks for this document. I have two minor comments.

1. In §1,

   *  TCP-AO and TCP MD5 configuration for Layer 3 VPNs is modeled in A
      Layer 3 VPN Network YANG Model [RFC9182].  This model assumes that
      TCP-AO specific parameters are preconfigured in addition to the
      keychain parameters.

When you say "this model" are you referring to RFC 9182, or the present document? You could clarify by saying "that model" if you're talking about 9182 (which I think is the case?), or "the present document" if you mean your own spec.

2. In §6, perhaps consider using a different term for an on-path attacker other than "MITM", which conventionally (to my knowledge) expands to "man in the middle". (Or I think you could just drop "e.g. MITM", which doesn't seem to add much.)

Just some editing suggestions:

In Section 1:

   ... This is conscious decision as these parameters
   hardly matter in a state-of-the-art TCP implementation.  It would
   also be possible also to translate a MIB into a YANG module, ...

Need "a" before "conscious", and drop one instance of "also".

Section 2.1 should probably indicate that there are lots of places where "I-D.ietf-tcpm-rfc793bis" in the YANG module will need to be replaced by its final RFC number.

Section 5.2:

OLD:

   This document registers a YANG module in the "YANG Module Names"
   registry YANG - A Data Modeling Language [RFC6020].  Following the
   format in YANG - A Data Modeling Language [RFC6020], the following
   registration is requested:

NEW:

   The following entry is requested to be added to the "YANG Module Names"
   registry created by [RFC6020]:

old DISCUSSes:

[ballot text incomplete as cloudflare triggers a weird refusal if I include a certain sentence, see Cloudflare Ray ID: 722aa6477e01a1db]

Thanks for the -07 changes in response to the various area reviews!

I still have two small discuss questions left which might be caused by a limited
understanding of the underlying yang model inclusions.


              1. For an application willing to accept both IPv4 and
                 IPv6 datagrams, the value of this object must be
                 ''h (a zero-length octet-string), with the value
                 of the corresponding 'type' object being
                 unknown (0).

              2. For an application willing to accept only IPv4 or
                 IPv6 datagrams, the value of this object must be
                 '0.0.0.0' or '::' respectively, with
                 'type' representing the appropriate address type.

#D1

To me, it seems very counter-intuitive that '' means (0.0.0.0 or ::). For
LISTEN, I find it concerning because it means that '' as a default means to
listen to any IP, rather than the more secure default of '' meaning 'none'.

Is it possible to have some kind of "None" default? (or is that achieved by
omission of this option?

#D2

Is it intended that this cannot be specified using names



#C1

It is unclear to me if '0.0.0.0' denotes the type inet:ip-address or
the type string with text value "0.0.0.0". I also worry that if this is
represented in C with a struct with union, that than it is unclear what
a zero'ed out struct is set to? The string or the v4 ANY or the v6 ANY ?
(I personally like enums to start from value 1 for that reason)

#C2

How is this option set when fully disabled (eg not listening on anything, only
willing to make an outgoing TCP connection). Is it by being omitted?

NITS:

listner -> listener

for TCP connection that -> for a TCP connection that

Hi,

Thanks for another YANG module!

I have some non-blocking comments, I suspect that many of these have already been raised/discussed so really just want to check that they had been proactively considered.

1) 
      TCP connection list: Access to status information for all TCP
      connections.  Note, the connection table is modeled as a list that
      is read-writeable, even though a connection cannot be created by
      adding entries to the table.  Similarly, deletion of connections
      from this list is implementation-specific.

I think that it would be helpful to further describe and perhaps constrain the behaviour here:
 - I would suggest clarifying that the list is read/writable to allow configuration to be associated with existing connections, or connections that may be subsequently made.
 - It would be helpful to understand what happens if the configuration for a connection changes for an existing connection.  Does it force the connection to be closed and reopened?  Or does it depend on the configuration change?
 - I don't think that deleting the configuration properties should cause a connection to be closed (although it might cause the connection to flap, as per my comment above.)  E.g., for BGP, isn't it the BGP neighbour configuration that should control whether a connection logically exists?

2) With the ambiguity regarding YANG IP address and zones, can I clarify that in all cases, the intention is that zoned ip-addresses are allowed where the inet:ip-address type is used?

3) leaf enable-ao {
I presume that there was a conscious decision not to use a presence container here (and hence avoid the need for the must statements)?

4) leaf enable-md5 {
I presume that implementations won't need to add extra MD5 specific parameters?  I.e., it doesn't make sense for this to be a presence container?

5) Connection is closed. Connections in this state
                    may not appear in this list.";

In the operational state tree then what does the tcp/connections list represent?  Does it contain all connections that either exist or have some configuration associated with them?

6) I think that leaf "state" (describing the connection state) should be marked as "config false".  A client should not be able to configure the current protocol "state".

7)      leaf address {
           type union {
             type string;
             type inet:ip-address;
           }

Given the described behaviour, then I think that you should probably:
 (i) List the inet:ip-address type first.
 (ii) Add a length "0" restriction to the string type.

8)              For an application willing to accept both IPv4 and
                 IPv6 datagrams, the value of this object must be
                 ''h (a zero-length octet-string), with the value
                 of the corresponding 'type' object being
                 unknown (0).

What does the h mean?  I've got a nagging feeling that this is a known convention ...  I presumed that the WG considered and dismissed the option of having two separate listeners listed, one for v4 and one for v6 rather than the union with the string type?

9)
list tcp-listeners {
         key "type address port";
This allows multiple listeners for the same port.  I just wanted to check that is allowed/expected?

10) container statistics

These counters are global level, so there are not many of them.  Some of these counters are 32 bits, I presume that there is no reasonable risk that they would overflow over a reasonable time frame?

Nits:

included in TCP MIB => included in the TCP MIB
TCP-AO TCP-AO [RFC5925] => Duplicate TCP-AO.

Thanks,
Rob

Thank you to Hilarie Orman for the SECDIR review.

Thank you for addressing my DISCUSS and COMMENTs point.

Thanks for addressing my discuss points. Thanks for the effort on this specification.

# Éric Vyncke, INT AD, comments for draft-ietf-tcpm-yang-tcp-07
CC @evyncke

Thank you for the work put into this document. 

Please find below some non-blocking COMMENT points (but replies would be appreciated even if only for my own education), and some nits.

Special thanks to Yoshifumi Nishidar for the shepherd's detailed write-up including the WG consensus even if it lacks the justification of the intended status and uses an unusual template. 

I hope that this helps to improve the document,

Regards,

-éric

## COMMENTS

### Section 1

```
   As such, TCP is implemented on network
   elements that can be configured via network management protocols such
   as NETCONF [RFC6241] or RESTCONF [RFC8040].
```
AFAIK, NETCONF & RESTCONF can also be used to monitor/collect statistics. So "configured" seems rather limited.

### Section 1 YANG usefulness

I wonder whether the paragraph containing: `Moreover, many existing TCP/IP stacks do not use YANG data models.` really brings any value to this document.

### Section 1, orthogonal or not ?

```
   This specification is orthogonal to the Management Information Base
   (MIB) for the Transmission Control Protocol (TCP) [RFC4022].  The
   basic statistics defined in this document follow the model of the TCP
   MIB. 
```
Is the model is based on the MIB, can it really be orthogonal to it ?


## NITS  


### listner ?  

Is there a typo in "listner" ?

### Section 3.1

Perhaps a rendering issue, but "TCP-AO TCP-AO" is probably wrong ;-)

## Notes

This review is in the ["IETF Comments" Markdown format][ICMF], You can use the
[`ietf-comments` tool][ICT] to automatically convert this review into
individual GitHub issues. 

[ICMF]: https://github.com/mnot/ietf-comments/blob/main/format.md
[ICT]: https://github.com/mnot/ietf-comments

I'm happy to see the changes from the -06 version.  Thanks to Jeff Haas for his review!

[One more comment from Jeff, which I'm transcribing here as it wasn't wart of an archived discussion.]

It occurs to me that as the keychain extensions are defined it might be desirable to protect them via if-feature.  Basically, what happens if tcp-ao or tcp-md5 aren't supported by the implementation?