Re: [I2nsf] [secdir] (updated) review of draft-ietf-i2nsf-capability-data-model-21

[tom petch <[e-mail]>]

I see that this is scheduled for a IESG Telechat 3feb22 which may 
influence the appropriate action.

The various ...art reviews have introduced a number of changes to this
and other I2NSF I-D with the effect of introducing inconsistencies.

Thus 'nsf-facing' now has expanded description clauses (which I have yet
to read) for many YANG identity.  These may help the ...art reviwer but
'i2nsf-capability-data-model' has the identical YANG identity 
definitions without the description clauses; and as an AD said recently 
of another I-D, it is not always helpful to reproduce technical details 
as opposed to referencing them - the former introduces the risk of 
inconsistencies!

More technically, 'capability' adds a new action 'reject' something
which the authors of RFC8329 did not consider but as RFC8329 says 'such
as' for its list of actions that probably caters for it.   However,
where the possible actions are listed in YANG description clauses of
'capability', 'reject' does not appear (and should IMHO).

'capability' now has actions of pass, drop, reject,  mirror, rate-limit;
RFC8329 has pass, drop, rate  limiting, and mirroring in some contexts, 
but 'deny' instead of 'drop in others.

The treatment of length is much changed and I struggle to get my mind 
around it.  RFC 8329 specifies 'length' for
IPV4 header and IPv6 header; strictly the terminology is wrong; RFC791
has total length, RFC8200 has payload length. 'capability' has the one
field, 'total length', although the description explains how to 
interpret this for IPv6.  'nsf-facing' now imports its treatment of 
network and transport protocols from RFC8519. RFC8329 specifes that NSF 
'should include the matching criteria specified by [ACL-YANG]' ie
RFC8519 (which would be great if if had done so a year or two ago rather
than after Last Call).  This means that 'nsf-facing' uses 'length' and
'ihl' for IPv4 and just 'length' for IPv6.  Mapping these to fields in
'capability' is an exercise for the reader!

There is similar dissonance wtih TCP and UDP.  Here, RFC793 has 'data
offset', which is the header length counted in words, while RFC768 has
'length', counted in octet.  RFC8329 does not mention length as a
capability match for either UDP or TCP AFAICT.  'capability'  has 
'length' for both TCP and UDP with the description explaining the 
details and the differing units.  'nsf-monitoring' imports from RFC8519 
again which means  'data-offset' for TCP and 'length'  for UDP.

In nsf-facing, 'geography-location' has become 'geographic-location';
'capability still uses 'geography.

In passing, the TLP in the YANG module is out of date and the references 
to 'nsf-monitoring' do not use the precise title thereof,
missing out an 'interface'

Tom Petch






On 22/01/2022 08:43, Mr. Jaehoon Paul Jeong wrote:
> Hi Paul, Roman, and Tom,
> Here is the revision of NSF Capability Draft:
> https://datatracker.ietf.org/doc/html/draft-ietf-i2nsf-capability-data-model-22
>
> The revision letter is attached to explain how we have addressed your
> comments on the revision.
>
> Patrick and I have addressed all of your comments.
>
> If you are satisfied with the revision, let's move forward.
> Otherwise, please let me know further improvements.
>
> Thanks.
>
> Best Regards,
> Paul
>
>
> On Fri, Jan 7, 2022 at 7:54 AM Roman Danyliw <[e-mail]> wrote:
>
>> Hi Paul!
>> (adding I2NSF and document alias like an official response to a
>> directorate review)
>>
>> Thanks for this review.  A response below and the authors/WG can correct
>> me.
>>
>>> -----Original Message-----
>>> From: secdir <[e-mail]> On Behalf Of Paul Wouters
>>> Sent: Monday, November 29, 2021 4:06 PM
>>> To: secdir <[e-mail]>
>>> Subject: [secdir] (updated) review of
>> draft-ietf-i2nsf-capability-data-model-21
>>>
>>>
>>> Note to tools team: I was assigned
>> draft-ietf-i2nsf-capability-data-model,
>>> although I had already reviewed the -16 version. I did a review now of
>> the -21
>>> version but did not see a way within datatracker to update the review.
>> So I
>>> opted to use the secdir mailing list for now.
>>>
>>> Paul
>>>
>>>
>>>
>>> I have reviewed this document as part of the security directorate's
>> ongoing
>>> effort to review all IETF documents being processed by the IESG.  These
>>> comments were written primarily for the benefit of the security area
>> directors.
>>> Document editors and WG chairs should treat these comments just like any
>>> other last call comments.
>>>
>>> The summary of the review is Has Issues
>>>
>>> I have reviewed the document. I don't have any particular security
>> concerns,
>>> and the Security Considerations section is fine. I do have some
>> questions/issues
>>> from reading the document.
>>>
>>>
>>> I am a bit confused about this part:
>>>
>>>           |  |  +--rw ipv4-capability*       identityref
>>>           |  |  +--rw ipv6-capability*       identityref
>>>           |  |  +--rw icmpv4-capability*     identityref
>>>           |  |  +--rw icmpv6-capability*     identityref
>>>           |  |  +--rw tcp-capability*        identityref
>>>           |  |  +--rw udp-capability*        identityref
>>>
>>> There is an item for v4 and v6 support. Why is there a split of icmpv4
>> and
>>> icmpv6?
>>> Why isn't that done similarly to tcp and udp that don't have v4/v6
>> versions?
>>
>> This modeling choice was made because ICMPv4 and ICMPv6 are not the same
>> protocol.  TCP and UDP are the same protocol regardless of whether they are
>> using IPv4 or v6.
>>
>>> This term seems to be rather generic:
>>>
>>>           |  |  +--rw url-capability*                    identityref
>>>
>>> Perhaps what is meant is url-filtering-capability or
>> url-protection-capability ?
>>
>> I'll leave it up to the WG to decide if they want to scope it as such.
>>
>>> It also seems rw advanced-nsf-capabilities is really either "rw
>> protection-nsf-
>>> capabilities" or "rw filtering-nsf-capabilities" ? It seems "advanced"
>> is a very
>>> generic term? It could be useful to allow for further
>> non-filter/non-protective
>>> options, but it does seem right now this "advanced" category really means
>>> some kind of "client protection" category.
>>
>> There is a history in the naming convention of advanced vs.
>> generic-nsf-capabilities.  Advanced capabilities were initially extension
>> points discussed in other documents.  After refinement, the work didn't
>> evolve this way.  The WG has discussed and modified this convention, and
>> arrived at roughly the explanation documented in Section 5.1:
>>
>> ==[ snip ]==
>>     In this
>>     document, two kinds of condition capabilities are used to classify
>>     different capabilities of NSFs such as generic-nsf-capabilities and
>>     advanced-nsf-capabilities.  First, the generic-nsf-capabilities
>>     define NSFs that operate on packet header for layer 2 (i.e., Ethernet
>>     capability), layer 3 (i.e., IPv4 capability, IPv6 capability, ICMPv4
>>     capability, and ICMPv6 capability.), and layer 4 (i.e., TCP
>>     capability, UDP capability, SCTP capability, and DCCP capability).
>>     Second, the advanced-nsf-capabilities define NSFs that operate on
>>     features different from the generic-nsf-capabilities, e.g., the
>>     payload, cross flow state, application layer, traffic statistics,
>>     network behavior, etc.  This document defines the advanced-nsf into
>>     two categories such as content-security-control and attack-
>>     mitigation-control.
>> ==[ snip ]==
>>
>>
>>> Similarly, "rw target-capabilities" might be better descriped as "rw
>> destination-
>>> capabilities"
>>> to avoid confusing about this being a "targetting system" or the client
>> being
>>> "targetted".
>>
>> I can see your point.  "target" is used in place of "destination" in a few
>> places.  This seems editorial and I'd leave it up to the WG to decide.
>>
>>> I also find "rw action-capabilities" confusing. Isn't "anti-virus" and
>> "anti-ddos"
>>> also an action capability ? Or should I read this as a condition of
>> "anti-virus"
>>> kind activate an action capability (filter in, filter out, log).
>>
>> It's the latter.  Consider Example 5 of Section A.5 which depicts the
>> interrelationship between <anti-ddos-capability> and the
>> <action-capabilities>.
>>
>>> It also seems the
>>> selector (eg "anti-virus") is coupled to an action (eg "block") so I'm a
>> bit
>>> confused on why there is no capability link between these. Eg having
>> "filtering"
>>> as a capability seems related to some conditionals, but perhaps not all.
>> So I am
>>> not sure if the current model could describe that. Eg lets say there is
>> a packet
>>> filter, not but no filter based on anti-virus but it can detect
>> anti-virus. How
>>> would one know from these capabilities that anti-virus has "filter" and
>> not only
>>> "log" ?
>>
>> For your antivirus configuration there might be something like the
>> following:
>>
>> <condition-capabilities>
>>     <advanced-nsf-capabilities>
>>     <anti-virus-capability>detect</anti-virus-capability>
>> ...
>> <action-capabilities>
>> ...
>>       <egress-action-capability>drop</ingress-action-capability>
>>
>>> And "rw generic-nsf-capabilities" seems to be more like "rw
>> transport-nsf-
>>> capabilities"
>>
>> See explanation above on generic vs. advanced-capabilities
>>
>>> There are many email contacts listed in Section 6. These will not stand
>> the
>>> passing of time.
>>> Why are they needed? Should there be an IANA registry/contact instead ?
>>
>> Not question this contact information will age.  However, it seems to be
>> common convention to include all of the document authors in the YANG
>> contact information.
>>
>>> the identity targets include base target-device which only has a
>> description
>>> field. So all these identity targets _only_ have a description. Is the
>> presense of
>>> an empty identity entry enough to indicate this support, or is some kind
>> of
>>> boolean field needed?
>>
>> Thoughts from WG?
>>
>>> identity flags is only about TCP. Should it be called tcp-flags (like
>> tcp-options) ?
>>> Similar issue with identity total-length, verification-tag, identity
>> chunk-type and
>>> service-code.
>>
>> Seems like there should be consistency or an approach either way as to
>> whether the protocol name is a prefix to a field name.  I'll leave it to
>> the authors to decide on the approach.
>>
>>> I see identity for pop3 and imap. Does that include pop3s and imaps
>> (version of
>>> those protocols over TLS). If so, should it be clarified in the
>> description text? If
>>> not, do we need seperate identity types for these?
>>
>> I think the intent is for two identities: POP3+POP3S and IMAP+IMAPS, but
>> I'll let the WG make the right change.
>>
>>> I see identities for pass, drop, mirror and rate-limit, but not for
>> reject (eg send
>>> an ICMP back)
>>
>> Paul: Makes sense to me to add it in with your explanation.
>> WG: What do you think?  I believe "reject" was in -05, but removed in -06
>> after the first AD review (
>> https://mailarchive.ietf.org/arch/msg/i2nsf/Qkup2tKpVyAcelxy3QooLf7P1KI/)
>> pointed out that all of these identities were undocumented.
>>
>>> Security Considerations Section:
>>>
>>>        The lowest layer of RESTCONF protocol layers
>>>        MUST use HTTP over Transport Layer Security (TLS), that is, HTTPS
>>>        [RFC7230][RFC8446] as a secure transport layer.
>>>
>>> This excludes QUIC? Perhaps it is better to say use an encrypted and
>>> authenticated transport layer, such as TLS or QUIC using HTTPS.
>>
>> This text is a derivative of the standard YANG boiler plate text included
>> in most YANG document recently in recent years.  The working source is kept
>> at https://trac.ietf.org/trac/ops/wiki/yang-security-guidelines.
>>
>>> I am a little confused at Example 3. It shows:
>>>
>>> It's only capability is "user-defined". It's only actions are
>> "ingress/egree options
>>> that do pass/drop/mirror. Where does it state this is a web filter
>> capability ?
>>
>> It's a web-filter capability because of "<url-capability>".
>>
>> "user-defined" is a specific type of URL-filter whose list is generated by
>> the operator:
>>
>>       identity user-defined {
>>         base url-filtering;
>>         description
>>           "Identity for user-defined URL Database condition capability.
>>            that allows a users manual addition of URLs for URL
>>            filtering.";
>>       }
>>
>>
>>> And does it really mean the web URI and content can be
>>> passed/dropped/mirrored? It feels like these pass/drop/mirror targets
>> are for
>>> packets, not web-uri streams ?
>>
>> The semantics are definitely reused from packet focused behavior.  For
>> security mitigation devices that operate on streams
>> pass/drop/mirror/log/etc are common actions though.
>>
>>> And should these actions not be inside the capability <url-capability> ?
>>
>> The YANG module design is modeled on the premise that each part of the
>> E-C-A rule is a separate top-level container per Section 3.1.  That
>> certainly does remove flexibility but was a design choice.
>>
>>> What if
>>> you define an NSF that has url-capability and a packet filter
>> capability, how
>>> would one know the pass/mirror/drop targets are for the url-capability
>> ot the
>>> packet filter capability ?
>>>
>>> Maybe, one of the examples can include an NSF with multiple conditions
>> and
>>> actions that don't fully overlap?
>>
>> WG thoughts?
>>
>>> NITS
>>
>> [...]
>>
>> Thanks.  Leaving to the authors.
>>
>> Regards,
>> Roman
>>
>> _______________________________________________
>> I2nsf mailing list
>> [e-mail]
>> https://www.ietf.org/mailman/listinfo/i2nsf
>>
>
>
>
> _______________________________________________
> I2nsf mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/i2nsf
>