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

[Andy Bierman <[e-mail]>]

On Fri, Jul 29, 2016 at 1:47 PM, Robert Sparks <[e-mail]> wrote:

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


Clients can build trees according to the YANG modules supported by the
server.
The YANG module has conformance requirements.
The protocols have 'resource-denied' errors.
Not sure what kind of warning they need.
An implementation may have no problem with 4 deep table entries,
but cannot store 100,000 flat table entries.



Andy



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