Subject: [P2PSIP] AD review: draft-ietf-p2psip-base-15

From: Gonzalo Camarillo <[email-masked]>
Date: Fri, 1 Jul 2011 14:38:56 +0300

Folks,

the P2PSIP WG chairs are in the process of requesting the publication of
the following draft:

https://datatracker.ietf.org/doc/draft-ietf-p2psip-base/

At this point, they are working with the secretary to fix the state of
the document in the tracker (it should be publication requested) and to
upload the document's PROTO write up.

While that gets fixed, I have done my AD review (see below) in order to
gain some time. As soon as these comments are addressed, I will start
the IETF LC on this draft.

Cheers,

Gonzalo

Review of draft-ietf-p2psip-base-15:

I have also included a number nits in this review because I spotted them
while reviewing the document. While some of them could have been caught
by the RFC editor at a later point, I chose to report them now anyway.

The last paragraph of page 8 says: This specification also defines how
RELOAD is used with the Chord DHT algorithm, which is mandatory to
implement.

However, the draft says later: This specification defines a DHT based on
Chord.

The draft should always talk about a Chord-based DHT or something along
those lines, then.

The draft references a few drafts that have expired such as the
following two:

http://tools.ietf.org/html/draft-ietf-p2psip-sip-05
http://tools.ietf.org/html/draft-ietf-p2psip-concepts-03

We need to have non-expired versions of those drafts in the repository
during the IETF LC and the IESG review. Chairs, please make sure that
happens.

On page 12, in the "Overlay Link Layer" bullet there is a normative MAY.
Given that Section 1 consists  on an introduction, the statement is not
about the protocol, and the RFC 2119 terms have not been introduced yet
(they are introduced in Section 2), I would s/MAY/may/.

Last two words of page 13: s/An usage/A usage/

Page 18 in the Kind bullet: s/story/store/

First paragraph of page 19: when talking about unstructured and
structured P2P networks, reference the IAB RFC that discusses them: RFC
5694.

Section 3 is an overview section. However, Section 3.2.1 contains a few
normative statements. They should probably be replaced by non-normative
statements.

Section 3.3 starts with:  This section will discuss the requirements
RELOAD's routing capabilities must meet...

Now that RELOAD is (virtually) finished, the sentences should talk about
the requirements as those that were used when designing RELOAD and that
RELOAD meets instead.

Page 24 says: Symmetric recursive routing requires that a message follow
a path through the overlay to the destination without returning to the
originating node:  each peer forwards the message closer to its
destination.  The return path of the response is then the same path
followed in reverse.

The fact that recursive routing is defined as a path that does not
"return" and then the following sentence talks about the return path can
be confusing. The authors may want to slightly rephrase that sentence to
avoid that.

The following two references:

o [I-D.maenpaa-p2psip-service-discovery]
o [I-D.maenpaa-p2psip-self-tuning]

should be replaced with the following ones:

o draft-ietf-p2psip-service-discovery
o draft-ietf-p2psip-self-tuning

The structure of Section 5.1 is confusing. Section 5.1 presents three
possible cases in three bullets. Those bullets seems to be expanded in
the three (sub)sections that follow (Sections 5.1.1, 5.1.2, and 5.1.3).
If that is the intent, Section 5.1 needs to be clearer. Right now, the
link between the bullets and those three sections is confusing. For
example, the first bullet uses the term peer while the first sentence in
Section 5.1.1 uses the term node. The third bullet refers to Section
5.3.2.2 but does not mention Section 5.1.3. Section 5.1.2 talks about
"the other three cases" but it is not clear what other three cases it
refers to.

Section 5.2.1 defines a set of timers by value. Why doesn't it define
the timers by name and then provides default values instead?

First paragraph of Section 5.3.1: "define TLS. [RFC5246]". The period
goes after the reference.

The last paragraph of Section 5.3.1 says: For instance, "uint16
array<0..2^8-2>;" represents up to 254 bytes but only up to 127 values
of two bytes (16 bits) each.

Doing s/but only up to/which corresponds to up to/ (or something
similar) would make the sentence clearer.

Section 5.3.2 says: (the string 'RELO' with the high bit of the first
byte set.).

The first period should be removed.

Section 5.5.1.3 says:

   o  In this case, the "stream" refers not to RTP or other types of
      media, but rather to a connection for RELOAD itself or for SIP
      signaling.

The connection could also be for other application-layer protocols other
than SIP.

Section 5.5.1.5 says: Therefore it is RECOMMENDED that full ICE be used
even for a node that has a public, unfiltered IP address, to take
advantage of STUN connectivity checks, etc.

This sentence seems to indicate that each node defines whether or not to
use full ICE. However, the previous paragraph talks about the use of ICE
being an overlay-wide setting that affects every node in a given overlay.

Last paragraph of page 66: include a reference to ICE TCP when talking
about new uses of ICE.

The three bullets at the beginning of page 67 prioritize different
transport protocols. The second bullet talks about "stream-oriented
protocols". However, that is not the main feature that makes TCP better
than the protocols in the third bullet. The main feature is that it
offers "well-understood congestion and flow control", as the protocols
in the first bullet do. The bullets should be clearer that bullet two is
better than bullet three because of its congestion avoidance properties
and worse than bullet 1 because of its lack of message orientation and
HOLB avoidance.

The same bullets and other sections in the draft talk about DCCP. Does
the WG think DCCP could be an appropriate transport for RELOAD?

Section 5.5.1.10 says: When neither side has provided an No-ICE
candidate, connectivity checks and nominations are used as in regular ICE.

The same question as before. Isn't the use of ICE an overlay-wide property?

First sentence of Section 5.5.1.13: s/(RELOAD)/(e.g., RELOAD)/

Section 5.6 explicitly talks about "three Overlay Link protocols" but
Section 5.6.1 says: The only currently defined overlay link protocols
are TLS and DTLS.

That sentence should include both DTLS with and without ICE for consistency.

Section 5.6.1.4: note that Baset's proposal for TCP over UDP
encapsulation was just one of the existing the proposals for TCP over
UDP encapsulation.

Section 5.6.3.1 says: "simple, inefficient, scheme". Remove the second
comma.

Section 6.4.1.2 talks about the Stat method for the same time. Add a
forward reference to Section 6.4.3, which describes it). For example,
adding "(see Section 6.4.3) would be enough.

Second paragraph of Section 14: for consistency with the other
paragraphs, instead of writing the initials, the given names are Jouni,
Gonzalo, and Jani respectively.

ID nits complains about the following references:

  ** Downref: Normative reference to an Informational RFC: RFC 2818
  ** Obsolete normative reference: RFC 2988 (Obsoleted by RFC 6298)
  ** Downref: Normative reference to an Informational RFC: RFC 3174
  ** Downref: Normative reference to an Informational RFC: RFC 3447
  ** Downref: Normative reference to an Informational RFC: RFC 6091
  ** Downref: Normative reference to an Informational RFC: RFC 6234

Can the authors confirm that all these downrefs need to be normative
references?

Also, should the reference to RFC 2988 be updated to refer to RFC 6298?

I am also working on getting an XML reviewer to have a look at Sections
10.1 and 10.1.1 of the draft. If you are interested, let me know.

---

From: Marc Petit-Huguenin <[email-masked]>
Date: Fri, 22 Jul 2011 08:55:18 -0700

> the IETF LC on this draft.

The IETF LC ends today but Michael Chen and I sent multiple questions to the
mailing-list that are still without answer.  I find it difficult to decide if
this specification is ready to be published until this questions are answered.

- -- 
Marc Petit-Huguenin
Personal email: [email-masked]
Professional email: [email-masked]
Blog: http://blog.marc.petit-huguenin.org

---

From: Bruce Lowekamp <[email-masked]>
Date: Fri, 22 Jul 2011 16:54:18 -0400

On Fri, Jul 22, 2011 at 11:55 AM, Marc Petit-Huguenin <[email-masked]> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>

Marc,

The other authors and I greatly appreciate the time you have put into
implementing and evaluating the protocol, and into providing comments
on the document.  However, it's difficult to keep track of them since
they are generally in separate emails, so I would appreciate it if you
could be specific listing any unanswered questions.  I've looked
through my email archive and responded to the two questions (one from
you and one from Michael Chen) that I found, both of which I think are
simple to resolve in the text.   If there are any others, please list
them so we can be sure to address them.

Thanks,
Bruce

> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.11 (GNU/Linux)
>
> iEYEARECAAYFAk4pnWMACgkQ9RoMZyVa61fJPgCfW2yfgvjVlpcOg0j272ma3yQR
> U8YAoJayxKmy+H9ARjW+G/nf2K+2KLsA
> =p5U9
> -----END PGP SIGNATURE-----
> _______________________________________________
> P2PSIP mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/p2psip
>

---

From: Marc Petit-Huguenin <[email-masked]>
Date: Fri, 22 Jul 2011 14:09:48 -0700

>> them so we can be sure to address them.

http://www.ietf.org/mail-archive/web/p2psip/current/msg05937.html

You may also want to look at my draft containing stuff I plan to publish as
extensions or in RELOAD 2.0:

http://www.ietf.org/internet-drafts/draft-petithuguenin-p2psip-reload-bis-00.txt

Version -01 will contains at least one additional section, here's the current
draft (which is related to the TURN-SERVICE question):

<section title="Certificate Processing">
  <t>
    An original store is able to verify the signature of the items to store
because the signer of the request is the same than the signer of the stored
values, but it is not true for replica stores, where the request signer and the
stored value signer are different.
    Even for the Access Control Policies where the certificate is stored at the
same peer than the stored value, there is no guarantee that during a replication
the certificate will be available for verification of the value.
  </t>

  <t>
    This document adds a rule to the list in section 6.4.1.1 that says that when
processing a StoreReq, none of the modifications is visible to the validation
process until the whole content of the StoreReq is validated.
    This means for example that sending a certificate with a CERTIFICATE_BY_NODE
kind in the same StoreReq than a a value signed by this certificate will not
permit to validate this value.
    The certificate has to be stored before in a different Store request or has
to be sent in the GenericCertificate field of the SecurityBlock of the request.
  </t>

  <t>
    This document also requires that each peer stores all the certificates
needed to verify all the stored values.
    When replicating the stored values, the peer MUST also send the matching
certificates in the GenericCertificate field of the SecurityBlock request.
  </t>
</section>

- -- 
Marc Petit-Huguenin

---

From: Bruce Lowekamp <[email-masked]>
Date: Sat, 23 Jul 2011 14:59:04 -0400

> http://www.ietf.org/mail-archive/web/p2psip/current/msg05937.html

Thanks for pointing that out.  I didn't go back beyond the beginning
of last call when I looked yesterday.

> extensions or in RELOAD 2.0:

If you have a comment to make about the current draft, please make it
clearly on-list.  I sure wouldn't expect a 2.0 document anytime soon,
and if you believe there are issues that somehow need to be resolved,
you should be making clear technical statements about them.  I've read
that draft, but I'm not going to go through guessing what you think
you want to do later as extensions and what you think should be
changed in the current draft.

> needed to verify all the stored values.

None of this seems to add anything beyond what's already in 5.3.4,
that the certificates necessary to validate a request SHOULD be in the
message.  Quoting again from that section:

  In systems which have some alternate
  certificate distribution mechanism, some certificates MAY be omitted.

that's the exception, clearly stated.  Otherwise, the certs are included.

> certificates in the GenericCertificate field of the SecurityBlock request.

The exception was requested by some people with specific scenarios
where they had (or could get) certificates quite easily.  The
exception also applies to the shared-secret security mechanism in the
document (12.4).  Since that specific case where certs don't need to
be included is already in the base document, I would be against
changing the current approach by making including certs a MUST.

Bruce

> iEYEARECAAYFAk4p5xQACgkQ9RoMZyVa61eHuQCgp4Cinv0NrOcCR7IWF/qcz67l
> KrgAniXutvo9Tp+k/TTtfbR89hP+3g/U
> =Aftz

---

From: Marc Petit-Huguenin <[email-masked]>
Date: Sat, 23 Jul 2011 21:12:28 +0200

>> changed in the current draft.

If there was something in it that I thought was necessary to the base draft, I
would have send it directly.

>> that's the exception, clearly stated.  Otherwise, the certs are included.

I can assure you that it is not what implementers understand (and I am not
talking about myself).  This is the problem when a SHOULD is used instead of a
conditional MUST.

>> changing the current approach by making including certs a MUST.

OK.

- -- 
Marc Petit-Huguenin