Re: [Gen-art] [Netconf] Gen-art LC review: draft-ietf-netconf-restconf-15

[Robert Sparks <[e-mail]>]

On 7/30/16 6:45 AM, t.petch wrote:
> Robert
>
> Picking up on the point about terminating the connection when a
> certificate validation fails,  this is a straight lift from 'Netconf
> over TLS', RFC7589, where the reference is also in Section 4 which makes
> it clear (to me:-) that the reference is to how the connection is
> terminated, as per RFC5246 s.7.2.1, and nothing to do with the
> certificate validation, which is as per RFC5280.
Perhaps having the s.7.2.1 reference in the text in this document (that 
part wasn't
included in the lift from 7589)  would have let me to interpret the 
sentence that way,
after following the reference. I suggest a light touch to the sentence 
to make it clear
that you are talking about "how to terminate the connection" 
not"terminating the
connection because the cert check failed" with the reference.

(And this should be moved to a nit since it's an editorial clarification).

>
> Tom Petch
>
> ----- Original Message -----
> From: "Robert Sparks" <[e-mail]>
> To: "Andy Bierman" <[e-mail]>
> Cc: "General Area Review Team" <[e-mail]>;
> <[e-mail]>; <[e-mail]>; "Netconf"
> <[e-mail]>
> Sent: Friday, July 29, 2016 9:47 PM
> Subject: Re: [Netconf] Gen-art LC review: draft-ietf-netconf-restconf-15
>
>
>>
>> On 7/29/16 3:36 PM, Andy Bierman wrote:
>>> Hi,
>>>
>>> I will add this review to the list.
>>> A new version in in progress.
>>> Some comments inline
>>>
>>>
>>> On Fri, Jul 29, 2016 at 1:11 PM, Robert Sparks <[e-mail]
>>> <mailto:[e-mail]>> wrote:
>>>
>>>      I am the assigned Gen-ART reviewer for this draft. The General
> Area
>>>      Review Team (Gen-ART) reviews all IETF documents being processed
>>>      by the IESG for the IETF Chair.  Please treat these comments
> just
>>>      like any other last call comments.
>>>
>>>      For more information, please see the FAQ at
>>>
>>>      <http://wiki.tools.ietf.org/area/gen/trac/wiki/GenArtfaq>.
>>>
>>>      Document: draft-ietf-netconf-restconf-15
>>>      Reviewer: Robert Sparks
>>>      Review Date: 28Jul2016
>>>      IETF LC End Date: 3Aug2016
>>>      IESG Telechat date: not yet scheduled
>>>
>>>      Summary:
>>>
>>>      Major issues:
>>>
>>>      * I am not finding any discussion in the Security Considerations
>>>      or in the text around what a server's options are if a client is
>>>      asking it to keep more state than it is willing or capable of
>>>      holding. The possible values of the "depth" query parameter
>>>      (particularly "unbounded") points out that a misconfigured or
>>>      compromised client might start creating arbitrarily deep trees.
>>>      Should a server have the ability to say no?
>>>
>>>
>>>
>>> I guess we need more text somewhere explaining the "depth" parameter
> is
>>> a retrieval filter.
>> I got that. It's existence, however, caused me to think about the fact
>> that what is stored at the server can be arbitrarily deep. Clients
> using
>> POST can build trees that are arbitrarily deep, with bits at the node
>> that are arbitrarily large (subject to the constraints the YANG models
>> put on the node). There should be some discussion acknowledging that
>> this can happen, and discussion of what the server can do if some
> client
>> starts asking it to store more than it is willing to store.
>>> It is not used to create anything in the server.
>>> The server does not maintain any state except during the processing
> of
>>> the retrieval request
>>>
>>>
>>>      * The third paragraph of 3.7 paraphrases to "SHOULD NOT delete
>>>      more than one instance unless a proprietary query parameter says
>>>      it's ok". This isn't really helpful in a specification.
>>>      Proprietary things are proprietary. The SHOULD NOT already
> allows
>>>      proprietary things to do something different without
> trainwrecking
>>>      the protocol. Please just delete the 2nd and 3rd sentence from
> the
>>>      paragraph.
>>>
>>>
>>> OK
>>>
>>>
>>>      * Section 2.3 says "If X.509 certificate path validation fails
> and
>>>      the presented X.509 certificate does not match a locally
>>>      configured certificate fingerprint, the connection MUST be
>>>      terminated as defined in [RFC5246]." RFC5246 doesn't really talk
>>>      about certificate validation, and it certainly doesn't say "the
>>>      connection MUST be terminated" when certificates fail to
> validate.
>>>      What are you trying to point to in RFC5246 here? Should you be
>>>      pointing somewhere else? (It's perfectly reasonable for the
>>>      document to reference RFC5246, and it does so elsewhere without
>>>      problem).
>>>
>>>
>>>
>>> Please suggest replacement text if we are citing the wrong RFC.
>>> I will ask Kent to look into this issue
>>>
>>>
>>>      Minor issues:
>>>
>>>      * "A server MUST support XML or JSON encoding." is ambiguous.
> (2nd
>>>      paragraph of 5.2). Did you mean the server MUST support at least
>>>      one of XML or JSON but not necessarily both? I think you really
>>>      intended that the server support BOTH types of encoding.
>>>
>>>
>>> No -- it will be clarified that the server must support at least 1
> of
>>> the 2
>>>
>>>
>>>      * I _think_ I can infer that PUT can't be used with datastore
>>>      resources. Section 3.4 only speaks of POST and PATCH. Section
> 4.5
>>>      speaks about "target data resource" and is silent about
> datastore
>>>      resources. If I've understood the intent, please be explicit
> about
>>>      datastore resources in 4.5. If I've misunderstood then more
>>>      clarity is needed in both 3.4 and 4.5.
>>>
>>>
>>> The  next draft will be clarified to allow PUT on a datastore
> resource
>> Hrmm - that makes me less comfortable that you are actually aligned
> with
>> 7231. It may just be that you need to be more precise with your
>> description, but per 7231, PUT never creates resources - it can create
>> or replace the state of a resource.
>>>      * In 3.5.3.1 you restrict identifiers with "MUST NOT start with
>>>      'xml' (or any case variant of that string). Please call out why
>>>      (or point to an existing document that explains why).
>>>
>>>
>>> OK
>>>
>>>
>>>      * The text in 5.3 about access control interacting with caching
>>>      (added based on my early review I think) doesn't mesh well with
>>>      paragraph 3 of section 5.5. There you tell the client to use
> Etag
>>>      and Last-Modified, but in 5.3 you say it won't work reliably
> when
>>>      access permissions change. At the very least 5.5 should point
> back
>>>      into the paragraph in 5.3.
>>>
>>>      Nits/editorial comments:
>>>
>>>      * Introduction, 4th paragraph - please change "MAY provide" to
>>>      "provides". Section 3.6 explains the cases where there is choice
>>>      in what to provide.
>>>
>>>
>>>      * Section 2.3 paragraphs 1 and 2. There is edit-itis here left
> (I
>>>      suspect) from working in matching fingerprints. Consider
> combining
>>>      and simplifying these two paragraphs after improving the
> reference
>>>      issue called out above.
>>>
>>>      * Section 4 says "Access control mechanisms MUST be used to
>>>      limit..." This is not a good use of a 2119 MUST. I suggest
>>>      replacing "MUST be" with "are". The subsequent text already
>>>      captures the actual normative requirements on the server.
>>>
>>>      * Section 12 says "this protocol SHOULD be implemented
> carefully".
>>>      That is not a good use of a 2119 SHOULD. It is not a protocol
>>>      requirement. I suggest reformulating this into something like
>>>      "There are many patterns of attack that have been observed
> through
>>>      operational practice with existing management interfaces. It
> would
>>>      be wise for implementers to research them, and take them into
>>>      account when implementing this protocol." It would be far better
>>>      to provide a pointer to where the implementer should start this
>>>      research.
>>>
>>>      * (micronit) Lots of examples are internally inconsistent wrt
>>>      dates. For instance, look at the 200 OK in section 3.3.3 - it
> says
>>>      that back in 2012, a server returned something talking about a
>>>      library versioned in 2016.
>>>
>>>
>>>
>>> Andy
>>>
>>
>
> ------------------------------------------------------------------------
> --------
>
>
>> _______________________________________________
>> Netconf mailing list
>> [e-mail]
>> https://www.ietf.org/mailman/listinfo/netconf
>>