Subject: [P2PSIP] review of draft-ietf-p2psip-base-06.txt

From: John Buford <[email-masked]>
Date: Thu, 17 Dec 2009 22:43:42 -0500

Here is my review of draft-ietf-p2psip-base-06
that I agreed to provide at IETF76 WG meeting.

John

General
=======

1.4 Structure of the Document
   Adding a new usage - it would be useful to have a section on this
   Adding a new overlay algorithm - it would be useful to have a section on
this

3.2 Clients
   How does a peer transition to a client?

3.3 Routing
   "Client promotion" - is there client demotion?

4.1.1 Storage Permissions
  This limits the storage load which another peer can impose,
  but doesn't address loading due to frequency of
  write/fetches another peer can impose.

6.3 Access Control Policies
  How is access control managed for replicated values?

It might be helpful for implementers to have a table
in an appendix listing the various config parameters
and their default / max values, etc, such as:

holdown_time 30 sec
periodic Update requests 10 min
Ping interval  3000 sec
Partition detection
Max Message size
etc.

Detailed Comments
=================

1.1 "partly connected graph" -- connected graph subsumes partly connected
case

1.2.5 "a packet" => a message

3.3 "In all cases, the response to
  a request needs ..."  -- should this be written as a MUST?

4 Application Support Overview
  "To form direct connections ..." -- why "direct"?

5.1.1 Responsible ID (2nd bullet)
"If the message is a response ... the state
is reinserted ... first" -- <before forwarding or
before sending to the upper layer?>

5.3.2 Forward Header
relo_token = 0xc2454c4f, i.e., 'RELO' with msb set in first byte.
  Shouldn't this be 0xd2454c4f ?  (this was commented on the
  list already in July 2008)

transaction_id "unique" - globally unique in the overlay or unique
    respect to transaction ids generated by this peer?

5.3.2.2
"This may be one of "peer", ..."  -- should be node instead of peer

"A DestinationData can be one of three types ...  peer
   A Node-id"  -- change "peer" to "node" for consistency with
   the select statement

5.3.2.2 bottom p. 44 to top of p. 45
 "the encoding MUST include a generation counter designed ..."
  -- since this is opaque, I don't see how it is "MUST".
     there could be other ways for nodes to prevent misrouting
     due to dynamics of the connection table.

5.3.2.3
   should the flags be defined as enums in the notation?

5.3.3
   message_code  0x8000 .. 0xfffe seems to be omitted

5.3.3.1 Response Codes and Response Errors

  There seem to be a number of cases where "peer" is used in the
  text where it should be "node":

Error_Forbidden: The requesting _peer_ does not have permission to
  make this request.
Error_Not_Found: The resource or _peer_ cannot be found ...
Error_Request_Timeout: ...The requesting _peer_ MAY resend the
  request at a later time.

Also it is not clear whether clients can receive some requests,
such as update_req, stat_req, or ping.  If so, some of the other
Response Error text may need to be changed to refer to "node" instead
of "peer".

Finally, these definitions appear here but are not in section 13.7:
ERror_Response_Too_Large, Error_Config_Too_Old, Error_Config_Too_New.

5.4.1 Topology Plugin Requirements
"The length of the Resource-IDs and Node-IDs".  -- Doesn't RELOAD fix these
   to 128 bits?

5.4.2.5.1

"The two currently defined..."  -- strike "two".
Also should "peer" be "node" in this section, at least for uptime?
If so, this would effect usage of peer in sec. 5.4.2.5.2 as well.

5.5.3.1 Request Definition
should it be "requesting node" instead of "requesting peer"?
(also in 5.5.4.1)

5.5.5 Ping
  what are the operational semantics of Ping to broadcast Node-ID?

5.5.6.2 Config_UpdateReq
  This could potentially be used to attack the overlay by disabling nodes
   with erroneous configurations.

5.6.2.1
  if we replace N-M with N-M-1, then the bit numberings are 0..N-1
  instead of 1..N in the mask, which is the usual convention.

5.6.2.2
  "Another would be to implement the _AIMD_ scheme described in
   Appendix B".

6 Data Storage Protocol

storage_time
   what about a store request whose time equals the time of the
   current value?

lifetime
   can a usage define a max_lifetime for a kind?

6.3 Access Control Policies
   what about control of fetch access?

6.4.1.1  Request Definition

  resource: The _Resource-ID_ to store at.

6.4..4.2 Response Definition

  "... it SHOULD return a 404 error"  -- replace 404 with a RELOAD error
code

9.4 "joining party" -- "joining node" seems clearer and more consistent
terminology
  A statement that the enrollment server assigns the Node-Id could be useful
here.

"numBitsInNodeId" - this is already fixed to 128, so should we drop the
variable use here?

9.6.3 Receiving Updates
  "The UpdateReq defines peers that indicate a neighbor table further away
from
   N than some of its neighbor table" -- could be made clearer, e.g.,
   => The UpdateReq contains peers that are further away from N than some of
the
       peers in N's neighbor table.

11. Message Flow Example

PPP, PP, NP, NNP acronyms should be defined

Some other examples that could be useful:
client-peer interaction
bootstrap with enrollment server
interactions between the node internal architecture elements

12.6.5 Residual Attacks

enrollment server is a central point of failure
(as implied by 3.6.2)

13.6
Message codes remove_req and remove_ans seem to
be no longer used according to 6.4.1.3

14 Acknowledgements - "Michael Chen" is listed twice.

Spelling/Grammar
================

1.2
"To clarify the role of the various layer,..." => layers

4.1.3 "on the its" => "on its"

5.3 "introduce new potential" => introduces

5.3.1 "the ability to mechanically (compile)..."
      -- is a verb missing here, such as generate or translate?

5.5.2.4  "be done be doing"  => be done by doing

5.6.2.1 "that the sender to figure"  => "that the sender can figure"

5.6.2.2 "no more aggressive then" => "no more aggressive than"
    also add "(see Appendix C)" after [RFC5348] in this sentence.

9.1 "entreis" => entries

9.6.3 "These Update requests are what ends up" => end up

9.8 extentions => extensions (also several places in 10.1)

10.1 "boradcast" => broadcast

10.3 "[RFC5272])" => "[RFC5272]"

10.3  "HTTP stats codes" => HTTP status codes

12.3 "certificates are signed by _require_ a central enrollment...." -- ?

12.6.3  "Replay is typical prevented ..." => typically

Will be resolved by RFC Editor, or before WGLC?
====================================
1.2.1 "this draft"
2 "are marked with an asterisk"
5.3.1  "This presentation is to some extent a placeholder....
   We expect this to be a question for the WG to decide."

5.6.1 Future Support for HIP
  "We leave this work as a topic for another draft".
  Is this to be addressed before WGLC?