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

Andy Bierman <andy@yumaworks.com> Fri, 29 July 2016 20:37 UTC

Return-Path: <andy@yumaworks.com>
X-Original-To: ietf@ietfa.amsl.com
Delivered-To: ietf@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id 210C412D88F for <ietf@ietfa.amsl.com>; Fri, 29 Jul 2016 13:37:00 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -1.9
X-Spam-Level:
X-Spam-Status: No, score=-1.9 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, HTML_MESSAGE=0.001, SPF_PASS=-0.001] autolearn=unavailable autolearn_force=no
Authentication-Results: ietfa.amsl.com (amavisd-new); dkim=pass (2048-bit key) header.d=yumaworks-com.20150623.gappssmtp.com
Received: from mail.ietf.org ([4.31.198.44]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id om2N_JuG88Bp for <ietf@ietfa.amsl.com>; Fri, 29 Jul 2016 13:36:58 -0700 (PDT)
Received: from mail-ua0-x22f.google.com (mail-ua0-x22f.google.com [IPv6:2607:f8b0:400c:c08::22f]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id D333B12D878 for <ietf@ietf.org>; Fri, 29 Jul 2016 13:36:55 -0700 (PDT)
Received: by mail-ua0-x22f.google.com with SMTP id j59so69711228uaj.3 for <ietf@ietf.org>; Fri, 29 Jul 2016 13:36:55 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=yumaworks-com.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=Y2E/7l4KhSVXrx4majPbGM/Rq+bq866fk/6YHW4tR1A=; b=W6A/xIdMxoUI5jAz9Z/iK1P+yftHIcrUKQyEyRT+F6mpPF8HoFiIku7bcVugEI6S50 yGkjG7iMLORCJp5PdSYlCjUvmunn1rvAcSHr8n6wMpKkrLBCgWBampPN4jfthyA9duAq bDCVOYKOwVCJf+cmc+0OhGHRY7ygRoMbnCvktYBF3XqGs1MKJbORhguq+6mr+th3he4u KqBP3MerbmdMqvUbRNy71NKnuCJ7fi79hsQ8sPZsHakZI9rbCoMYvLKA142OqLcsiqLW AFWNn/m5k1cLD0Hj9ERkaUz+IW1Ixa0gZLH4XEEkTWBYkRow+f5H6BOwlmqSuUv4tm0v y/Zg==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=Y2E/7l4KhSVXrx4majPbGM/Rq+bq866fk/6YHW4tR1A=; b=LJ8JXDI5vi/8afnF6otC85/fj9R8w4mTvRVlB3b6WpDhPYXM//UhlzYfUjSbd9HnyA Y0Hhod7kBneo6IQbCaVIyaylK6bKECxaoGN7Me5rGfQ6lMJHx2Y+DQsztDMnjioWT1IL zZh0NrkRoJRFyKpyRO5BYtBYgN6gu3d75Q7DFEzz24qQ22GLi2B8ehYu8iAWNd5O4OOD OMmvDRt9RW1hbEioKTq7GMmGQzisW4HMnaQNAWZEKIjH2SPKSM9mpodEZ4Ngd14KhOW0 EpO/VH5H4y7LVYiaCyoxYdEWDUzI4jgXagbn5YhxSMPqiTcT6j13nBzGx2+QrX1PAfTx 6sJw==
X-Gm-Message-State: AEkooutRYNcZa0eZtnz1mhXpwBUkR/4r3vuZ3zDvKS2Y94Oq5BbAji7TDLR9OVIgnCuLEXZ3QFdPSaA0BPtUpQ==
X-Received: by 10.176.67.4 with SMTP id k4mr19777127uak.47.1469824614969; Fri, 29 Jul 2016 13:36:54 -0700 (PDT)
MIME-Version: 1.0
Received: by 10.103.4.198 with HTTP; Fri, 29 Jul 2016 13:36:54 -0700 (PDT)
In-Reply-To: <5c5a0c28-7cb3-da17-53dc-ec6401be9d1c@nostrum.com>
References: <5c5a0c28-7cb3-da17-53dc-ec6401be9d1c@nostrum.com>
From: Andy Bierman <andy@yumaworks.com>
Date: Fri, 29 Jul 2016 13:36:54 -0700
Message-ID: <CABCOCHSEG0VyWvP=oSFDD8kkWR=+-5ACV=1Es=2WfbjYhaV1UQ@mail.gmail.com>
Subject: Re: Gen-art LC review: draft-ietf-netconf-restconf-15
To: Robert Sparks <rjsparks@nostrum.com>
Content-Type: multipart/alternative; boundary="94eb2c09507a0ed21f0538cc363a"
Archived-At: <https://mailarchive.ietf.org/arch/msg/ietf/9LqO6FXclMNpCK3b5i8ZYusg14k>
Cc: General Area Review Team <gen-art@ietf.org>, draft-ietf-netconf-restconf.all@ietf.org, "ietf@ietf.org" <ietf@ietf.org>, Netconf <netconf@ietf.org>
X-BeenThere: ietf@ietf.org
X-Mailman-Version: 2.1.17
Precedence: list
List-Id: IETF-Discussion <ietf.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/ietf>, <mailto:ietf-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/ietf/>
List-Post: <mailto:ietf@ietf.org>
List-Help: <mailto:ietf-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/ietf>, <mailto:ietf-request@ietf.org?subject=subscribe>
X-List-Received-Date: Fri, 29 Jul 2016 20:37:00 -0000

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@nostrum.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.  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


> * 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