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

[tom p. <[e-mail]>]

----- Original Message -----
From: "t.petch" <[e-mail]>
To: "Andy Bierman" <[e-mail]>; "Robert Sparks"
<[e-mail]>
Cc: "General Area Review Team" <[e-mail]>;
<[e-mail]>; <[e-mail]>; "Netconf"
<[e-mail]>
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" <[e-mail]>
> To: "Andy Bierman" <[e-mail]>
> Cc: "General Area Review Team" <[e-mail]>;
> <[e-mail]>; <[e-mail]>; "Netconf"
> <[e-mail]>
> 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
<[e-mail]
> > > <mailto:[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.
> > > 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
> > [e-mail]
> > https://www.ietf.org/mailman/listinfo/netconf
> >
>