[prev in list] [next in list] [prev in thread] [next in thread]
List: ietf
Subject: Re: Gen-art LC review: draft-ietf-netconf-restconf-15
From: Andy Bierman <andy () yumaworks ! com>
Date: 2016-07-29 21:11:08
Message-ID: CABCOCHQVVQ6B4R_0AMtKvcd2C4BM3uuWbXHAc1=iOeFGig2P1g () mail ! gmail ! com
[Download RAW message or body]
On Fri, Jul 29, 2016 at 1:47 PM, Robert Sparks <rjsparks@nostrum.com> wrote:
>
>
> 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@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.
>
> 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.
>
Clients can build trees according to the YANG modules supported by the
server.
The YANG module has conformance requirements.
The protocols have 'resource-denied' errors.
Not sure what kind of warning they need.
An implementation may have no problem with 4 deep table entries,
but cannot store 100,000 flat table entries.
Andy
> 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
>
>
>
[Attachment #3 (text/html)]
<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Jul \
29, 2016 at 1:47 PM, Robert Sparks <span dir="ltr"><<a \
href="mailto:rjsparks@nostrum.com" \
target="_blank">rjsparks@nostrum.com</a>></span> wrote:<br><blockquote \
class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc \
solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000">
<p><br>
</p>
<br>
<div>On 7/29/16 3:36 PM, Andy Bierman wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr">Hi,
<div><br>
</div>
<div>I will add this review to the list.</div>
<div>A new version in in progress.</div>
<div>Some comments inline</div>
<div><br>
<div class="gmail_extra"><br>
<div class="gmail_quote">On Fri, Jul 29, 2016 at 1:11 PM,
Robert Sparks <span dir="ltr"><<a href="mailto:rjsparks@nostrum.com" \
target="_blank">rjsparks@nostrum.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 \
.8ex;border-left:1px #ccc solid;padding-left:1ex">I am the assigned Gen-ART reviewer \
for this draft. The General Area<br>
Review Team (Gen-ART) reviews all IETF documents being
processed<br>
by the IESG for the IETF Chair. Please treat these
comments just<br>
like any other last call comments.<br>
<br>
For more information, please see the FAQ at<br>
<br>
<<a href="http://wiki.tools.ietf.org/area/gen/trac/wiki/GenArtfaq" \
rel="noreferrer" target="_blank">http://wiki.tools.ietf.org/area/gen/trac/wiki/GenArtfaq</a>>.<br>
<br>
Document: draft-ietf-netconf-restconf-15<br>
Reviewer: Robert Sparks<br>
Review Date: 28Jul2016<br>
IETF LC End Date: 3Aug2016<br>
IESG Telechat date: not yet scheduled<br>
<br>
Summary:<br>
<br>
Major issues:<br>
<br>
* 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?<br>
</blockquote>
<div><br>
</div>
<div><br>
</div>
<div>I guess we need more text somewhere explaining the
"depth" parameter is</div>
<div>a retrieval filter. </div>
</div>
</div>
</div>
</div>
</blockquote>
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.<br></div></blockquote><div><br></div><div><br></div><div>Clients can build \
trees according to the YANG modules supported by the server.</div><div>The YANG \
module has conformance requirements.</div><div>The protocols have \
'resource-denied' errors.</div><div>Not sure what kind of warning they \
need.</div><div>An implementation may have no problem with 4 deep table \
entries,</div><div>but cannot store 100,000 flat table \
entries.</div><div><br></div><div><br></div><div><br></div><div>Andy</div><div><br></div><div> \
</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc \
solid;padding-left:1ex"><div bgcolor="#FFFFFF" text="#000000"> <blockquote \
type="cite"> <div dir="ltr">
<div>
<div class="gmail_extra">
<div class="gmail_quote">
<div>It is not used to create anything in the server.</div>
<div>The server does not maintain any state except during
the processing of</div>
<div>the retrieval request</div>
<div><br>
</div>
<div> </div>
<blockquote class="gmail_quote" style="margin:0 0 0 \
.8ex;border-left:1px #ccc solid;padding-left:1ex"> <br>
* 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.<br>
</blockquote>
<div><br>
</div>
<div>OK</div>
<div> </div>
<blockquote class="gmail_quote" style="margin:0 0 0 \
.8ex;border-left:1px #ccc solid;padding-left:1ex"> <br>
* 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).<br>
</blockquote>
<div><br>
</div>
<div><br>
</div>
<div>Please suggest replacement text if we are citing the
wrong RFC.</div>
<div>I will ask Kent to look into this issue</div>
<div><br>
</div>
<div> </div>
<blockquote class="gmail_quote" style="margin:0 0 0 \
.8ex;border-left:1px #ccc solid;padding-left:1ex"> <br>
Minor issues:<br>
<br>
* "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.<br>
</blockquote>
<div><br>
</div>
<div>No -- it will be clarified that the server must
support at least 1 of the 2</div>
<div><br>
</div>
<div> </div>
<blockquote class="gmail_quote" style="margin:0 0 0 \
.8ex;border-left:1px #ccc solid;padding-left:1ex"> <br>
* 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.<br>
<br>
</blockquote>
<div><br>
</div>
<div>The next draft will be clarified to allow PUT on a
datastore resource</div>
</div>
</div>
</div>
</div>
</blockquote>
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.<br>
<blockquote type="cite">
<div dir="ltr">
<div>
<div class="gmail_extra">
<div class="gmail_quote">
<div> </div>
<blockquote class="gmail_quote" style="margin:0 0 0 \
.8ex;border-left:1px #ccc solid;padding-left:1ex">
* 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).<br>
</blockquote>
<div><br>
</div>
<div>OK</div>
<div> </div>
<blockquote class="gmail_quote" style="margin:0 0 0 \
.8ex;border-left:1px #ccc solid;padding-left:1ex"> <br>
* 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.<br>
<br>
Nits/editorial comments:<br>
<br>
* Introduction, 4th paragraph - please change "MAY
provide" to "provides". Section 3.6 explains the cases
where there is choice in what to provide.<br>
<br>
<br>
* 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.<br>
<br>
* 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.<br>
<br>
* 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.<br>
<br>
* (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.<br>
<br>
</blockquote>
</div>
<br>
</div>
</div>
<div class="gmail_extra"><br>
</div>
<div class="gmail_extra">Andy</div>
<div class="gmail_extra"><br>
</div>
</div>
</blockquote>
<br>
</div>
</blockquote></div><br></div></div>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic