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

[t.petch <[e-mail]>]

Kent

Yes, good now,

Tom Petch

----- Original Message -----
From: "Kent Watsen" <[e-mail]>
Sent: Monday, August 01, 2016 10:17 PM


>
> To address the Gen-Art comment, below I replaced “as defined in
[RFC5246]” with “, as described in Section 7.2.1 of [RFC5246]”.   Good
now?
>
> Additionally, while looking at Section 2.3, I realized an opportunity
to fix a couple more things.  First, when looking at RFC 7589, Section
5, I see the language “certificate obtained by a trusted mechanism”,
which I think is better than saying it has to match a locally configured
fingerprint.  Second, I found a redundant sentence which seemed
misleading.  Finally, I collapsed the two paragraphs into one since they
’re more connected than not.  The net result follows:
>
> NEW
>
>    2.3.  Certificate Validation
>
>    The RESTCONF client MUST either use X.509 certificate path
validation
>    [RFC5280] to verify the integrity of the RESTCONF server's TLS
>    certificate, or match the server’s TLS certificate with a
certificate
>    obtained by a trusted mechanism (e.g. a pinned certificate).   If
X.509
>    certificate path validation fails, and the presented X.509
certificate
>    does not match a certificate obtained by a trusted mechanism, the
>    connection MUST be terminated, as described in Section 7.2.1 of
>    [RFC5246].
>
>
> OLD:
>
>    2.3.  Certificate Validation
>
>    The RESTCONF client MUST either use X.509 certificate path
validation
>    [RFC5280] to verify the integrity of the RESTCONF server's TLS
>    certificate, or match the presented X.509 certificate with locally
>    configured certificate fingerprints.
>
>    The presented X.509 certificate MUST also be considered valid if it
>    matches a locally configured certificate fingerprint.  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].
>
>
> This change will be made in a few days if no objection is raised by
then.
>
> Thanks,
> Kent
>
>
> On 8/1/16, 1:57 PM, "Netconf on behalf of Robert Sparks"
<[e-mail] on behalf of [e-mail]> wrote:
>
>
>
>     On 7/30/16 6:45 AM, t.petch wrote:
>     > 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.
>     Perhaps having the s.7.2.1 reference in the text in this document
(that
>     part wasn't
>     included in the lift from 7589)  would have let me to interpret
the
>     sentence that way,
>     after following the reference. I suggest a light touch to the
sentence
>     to make it clear
>     that you are talking about "how to terminate the connection"
>     not"terminating the
>     connection because the cert check failed" with the reference.
>
>     (And this should be moved to a nit since it's an editorial
clarification).
>
>     >
>     > 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
>     >>
>
>     _______________________________________________
>     Netconf mailing list
>     [e-mail]
>     https://www.ietf.org/mailman/listinfo/netconf
>
>
>