----- Original Message ----- From: "t.petch" <ietfc@xxxxxxxxxxxxx> To: "Andy Bierman" <andy@xxxxxxxxxxxxx>; "Robert Sparks" <rjsparks@xxxxxxxxxxx> Cc: "General Area Review Team" <gen-art@xxxxxxxx>; <draft-ietf-netconf-restconf.all@xxxxxxxx>; <ietf@xxxxxxxx>; "Netconf" <netconf@xxxxxxxx> Sent: Saturday, July 30, 2016 12:45 PM 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 > > >