ietf
[Top] [All Lists]

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

2016-07-30 06:50:15
----- Original Message -----
From: "t.petch" <ietfc(_at_)btconnect(_dot_)com>
To: "Andy Bierman" <andy(_at_)yumaworks(_dot_)com>; "Robert Sparks"
<rjsparks(_at_)nostrum(_dot_)com>
Cc: "General Area Review Team" <gen-art(_at_)ietf(_dot_)org>;
<draft-ietf-netconf-restconf(_dot_)all(_at_)ietf(_dot_)org>; 
<ietf(_at_)ietf(_dot_)org>; "Netconf"
<netconf(_at_)ietf(_dot_)org>
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(_at_)nostrum(_dot_)com>
To: "Andy Bierman" <andy(_at_)yumaworks(_dot_)com>
Cc: "General Area Review Team" <gen-art(_at_)ietf(_dot_)org>;
<draft-ietf-netconf-restconf(_dot_)all(_at_)ietf(_dot_)org>; 
<ietf(_at_)ietf(_dot_)org>; "Netconf"
<netconf(_at_)ietf(_dot_)org>
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(_at_)nostrum(_dot_)com
<mailto:rjsparks(_at_)nostrum(_dot_)com>> 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(_at_)ietf(_dot_)org
https://www.ietf.org/mailman/listinfo/netconf



<Prev in Thread] Current Thread [Next in Thread>
  • Fw: [Netconf] Gen-art LC review: draft-ietf-netconf-restconf-15, tom p. <=