[Gen-art] Gen-Art requested early review: draft-ietf-netconf-restconf-09

[Robert Sparks <[e-mail]>]

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.  This is a requested early review.
Please treat these comments just like any other comments.

For more information, please see the FAQ at

<http://wiki.tools.ietf.org/area/gen/trac/wiki/GenArtfaq>.

Document: draft-ietf-netconf-restconf-09
Reviewer: Robert Sparks
Review Date: 20Jan2015
IETF LC End Date: (not yet in IETF last call)
IESG Telechat date: (not yet scheduled)

Summary: This document has a few issues that should be addressed before 
requesting publication.

Please make sure the semantics you're giving to POST vs PUT sit well 
with HTTP experts. Similarly, please make sure you have someone steeped 
in character sets and encoding look closely at section 5.3. I'll leave 
commenting on those things to people more qualified than me. Other 
reviewers have asked for more information about the interaction of this 
use of HTTP with things like pipelining.

Major issues:

The document should explicitly discuss how the restconf protocol will be 
extended. It mentions that other documents might do that in several 
places (specifically with "Additional functionality can be defined in 
external documents, outside the scope of this document").

Section 3.4 claims a datastore resource can only be written directly 
with the optional to implement PATCH method. That does not seem 
consistent with the rest of the document.

Section 2.2 says "RESTCONF requires that the transport-layer protocol 
provides both data integrity and confidentiality, such as are provided 
by the TLS protocol [RFC5246]." and "RESTCONF implementations MUST 
support the "https" URI scheme", but I don't see any text that prohibits 
the use of RESTCONF over HTTP without TLS. If the intent was to prohibit 
that, please make it more explicit. If the intent is to allow the use of 
RESTCONF without requiring the use of TLS, the security considerations 
section (at least) needs more discussion about when doing so is a bad idea.

Section 2.3 (Certificate Validation) seems to be restating things that 
it could simply point to instead. Why not replace this section with 
something like "the RESTCONF peer will follow the procedures and 
recommendations in RFC6125."? If it's because you want this section to 
talk only about validating the client certificate, that should be 
clarified (and there is still probably something that already exists 
that you can point to rather than restate things here). The current text 
claims RFC5246 requires the peer to terminate the connection if the 
certificate path validation fails  - where in RFC5246 are you seeing that?

Should section 3.1 talk about what a client should do if more than one 
restconf link relation is returned when querying host-meta?

Section 4.3 speaks to returning different content for the same resource 
depending on what authentication is provided. Specifically "the 
unauthorized content is omitted from the response message-body, and the 
authorized content is returned to the client.". Some discussion of how 
this interacts with caching (or even the ETag mechanisms) is warranted 
here. What happens if the authorization policies for a given user 
changes - will they be able to fetch the new things they have been given 
permission to see, or will the cache, etag, or if-changed-since 
mechanics interfere?

Minor issues:

There are some requirements stated with 2119 keywords buried in the 
Introduction sections. Consider moving them to the protocol definition 
sections.

Section 2.5 cuts off the possibility of using session-based 
authentication. If that's something you want to be able to add later, 
make sure you're leaving the right extension points. (I know the typical 
restconf client won't be a browser, but for those use cases where using 
a browser makes sense, remember that browsers make it very hard to 
change identities when you are using HTTP Authentication).

In sections 4.2 and 4.3 consider explicitly saying what to return if 
something issues a HEAD or GET request against an operation resource.

The first paragraph in section 5 (before 5.1.1) seems confused in its 
use of message vs method. Please review the use of the terms and make 
sure they align with the words used in the introduction to rfc7231, for 
example.

I suggest deleting section 5.2. You already say RESTCONF uses HTTP, and 
that this section is not comprehensive. As it is, it restates things 
said in the HTTP documentation without adding anything - at best, it's 
an opportunity to introduce errors while restating things. Just point to 
HTTP instead. Similarly, the first table in section 7 could be replaced 
with a pointer to the relevant place in the HTTP specs.

Nits/editorial comments:

The introduction says a user should not need any prior knowledge of 
NETCONF to use RESTCONF. It would be good to also call out here that the 
implementer _will_ need prior knowledge of NETCONF.

The first sentence of section 2.4 ("The RESTCONF client MUST carefully 
examine the certificate presented by the RESTCONF server to determine if 
it meets the client's expectations.") adds nothing. Please delete the 
sentence or replace it with specific things the client needs to do..

The introduction says "Edits are usually applied". Why is this qualified 
with "usually"?

In the examples at the end of 3.5.1 (before 3.5.1.1), consider saying 
list1=key1,key2,key3/ instead of list1=key1val,key2val,key3val to be 
consistent with the way you refer to the elements in other places.

It would be useful to explicitly discuss what to return if a resource 
already exists in Section 4.4.1. Right now, that's specified primarily 
by an example in section 7.1.

The "MUST use this exact path" after the last bullet in 5.1 is not a 
good use of 2119. The sentence is observing a truth about the defined 
system, not placing an interoperability requirement on the client. The 
client uses the returned identifier to access the resource. If it uses 
anything else, it is not accessing the resource. I suggest replacing the 
prase with "uses this exact path".

In section 5.6, I suggest replacing "Instead of using HTTP caching" with 
"Instead of relying on HTTP caching".

Consider adding an example that shows a 500 response corresponding to a 
"partial-operation" error in NETCONF, demonstrating what information the 
client gets to distinguish this 500 from an "operation-failed" error.

There is a phrase missing in the first sentence of section 7.1. Perhaps 
"will be returned" just before the left-parenthesis?