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. Tom Petch ----- Original Message ----- From: "Robert Sparks" <rjsparks@xxxxxxxxxxx> To: "Andy Bierman" <andy@xxxxxxxxxxxxx> Cc: "General Area Review Team" <gen-art@xxxxxxxx>; <draft-ietf-netconf-restconf.all@xxxxxxxx>; <ietf@xxxxxxxx>; "Netconf" <netconf@xxxxxxxx> 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 <rjsparks@xxxxxxxxxxx > > <mailto:rjsparks@xxxxxxxxxxx>> 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 > Netconf@xxxxxxxx > https://www.ietf.org/mailman/listinfo/netconf >