On Fri, Jul 29, 2016 at 1:47 PM, Robert Sparks <[email protected]> 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 <[email protected]> > 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 > > >
_______________________________________________ Gen-art mailing list [email protected] https://www.ietf.org/mailman/listinfo/gen-art
