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