Subject: Re: [P2PSIP] draft-ietf-p2psip-base-12 (3)

From: Eric Rescorla <[email-masked]>
Date: Tue, 18 Jan 2011 05:21:16 -0800

On Dec 6, 2010, at 2:12 PM, Marc Petit-Huguenin wrote:
> bytes, or 2032 bits.
> 
> A.14. Section 5.3.2  "uint32 length;"
> 
> I can assume that this length covers the Header, MessageContents and Signature,
> but I would guess that this was useful when Message was sent without
> FramingHeader over TCP (same than for the relo_token used to disambiguate STUN
> packets).  Now that this is useless, can't this length been only Header and
> MessageContents, so it is possible to find the boundary between MessageContents
> and Signature without having to parse MessageContents at all?
> 

I think it's a mistake to assume we'll never want messages to be self-contained/unframed.
Is there a real advantage to this  breaking change?

> A.15. Section 5.3.4 "HashAlgorithm hash_alg;"
> 
> What are the algorithms that should be supported?
> Is the hash only used to identify the certificate in SecurityBlock.certificates?
> Also the text says that "[t]he certificate_hash contains the hash of the
> certificate object."  Are we talking about the hash of the DER encoded
> certificate, as stored in GenericCertificate.certificate?

Yes. Clarified.

> A.16. Section 5.5.1.1. "IceCandidate candidates<0..2^16-1>;"
> 
> Because there must be at least one candidate, should this be:
> 
> IceCandidate candidates<1..2^16-1>;

It's actually quite a but longer than that, because IceCandidate is fairly long.
However, I think it's harmless to leave it as-is and actually putting 
12..2^16-1 or whatever is just confusing.

> A.17. "config_data (type==config)"
> 
> The definition of this field is "[t]he contents of the configuration document",
> but 10.1 says that a file "...can contain multiple 'configuration' elements...".
> So should not this field be in fact defined as "One XML configuration
> production.  This MUST be encoded with UTF-8 and assume a default namespace of
> ...", like for the kinds value?  And in this case what about the signature
> element in the XML fragment?
> 

Hmm.... I'm going to defer to Cullen on this one.. He did all the XML :)

> A.18. Section 5.6.2 "...not all aspects of the header apply to both types of
> transports."
> 
> It would be great if this could be explained a little bit more.  For example I
> assume that on a reliable link, the receiver still need to send the Ack for each
> frame received (to calculate the RTO), but what about the content of the
> ack_sequence and received fields?
> 

In retrospect, I think this sentence is stupid. Removed.

> A.19. Section 5.6.3.1.1, last paragraph
> 
> This paragraph described a lockstep algorithm, but if it is the case then there
> is no need for the received field, as we would always know that the previous
> messages have been received.
> 

Sorry, I'm not sure I understand the question. The received field is there to 
permit the sender to unilaterally use more sophisticated algorithms.

> A.20. Section 5.7, "If the message is not fragmented, then both the first and
> last fragment are set to 1..."
> 
> If the first fragment bit is set to 1 when the message is fragmented and is also
> set to one when the message is not fragmented, then it is always set to 1, so
> what is the point of having it in the first place?

Good point. OTOH, does this do any damage? If not, I'm tempted to write it
down as a misfeature and leave as-is.

> A.21. Section 10.1. "<kind name='sip-registration'><data-model>..."
> 
> Why do we need to repeat the data-model and access-control that were already in
> the IANA registry?  Or is it a way to override the IANA definitions as the
> example would suggest? (as the registration for SIP-REGISTRATION is using
> DICTIONARY and USER-NODE-MATCH).
> 

No, it's just to preserve uniform syntax for IANA-defined and user-defined kinds.

> A.22. Section 10.1. "sequence: a monotonically increasing sequence number
> between 1 and 2^32"
> 
> According to section 5.3.2 and 5.3.2.1, it should be between 0 and 2^16-2 (65534).
> 

Fixed.

> A.23. Section 10.1. "root-cert  This element..."
> 
> I guess that the encoding to use is base64, but it is said nowhere.
> 

Yes, the text here says "PEM" but that's stupid. Changed to base64--unless someone objects.

> A.24. Section 10.1.1. "| attribute id { xsd:int }),"
> 
> Does not match 13.6, that is saying that this is an unsigned integer.  Should
> probably use xsd:unsignedInt instead.
> 
> Note that there is other places in the RELAX NG document where the type can be
> improved (initial-ttl, etc...)

I'll leave this to Cullen.

> A.25. Section 10.1.1. 'access-control-type |= "user-match-with-anon-create"'
> 
> This control type is never defined in the document.
> 

Removed.

> A.26. Section 10.1.1. 'kind-names |= "sip-registration"'
> 
> the sip-registration kind is not defined in this document.  OTOH,
> certificate-by-node and certificate-by-name are defined, but not listed here.

A cullen problem.

> A.27. Section 10.1.1. 'self-signed-digest-type |= "sha1"'
> 
> Section 10.1. says that '[v]alid values for this parameter are "SHA-1" and
> "SHA-256"'.

Adjusted the text.

> A.28. Section 10.1.1 last paragraph "such an XML configuration file sent over
> email."
> 
> Because the signatures on the XML document are done on exact byte string and
> because emails servers are known to mess with end of lines, we will see
> configuration documents that cannot be verified after been sent by email (what
> was wrong with using XML-sig anyway?).
> 

It's ridiculously overcomplicated for this application (and arguably for any application).

> A.29. Section 13.15 "destination: a hex-encoded Destination List object"
> 
> A Destination List object is never formally defined in the whole document, so is
> it just Destination objects packed together, or is it in fact a "Destination
> destination_list<0..2^16-1>" ?  In other words, does it start with a length field?

Added something.

-Ekr

---

From: Marc Petit-Huguenin <[email-masked]>
Date: Tue, 18 Jan 2011 10:22:21 -0800

Inline.

> Is there a real advantage to this  breaking change?

A little easier to parse the message, but you are right that a message can be
unframed.

[...]

> 12..2^16-1 or whatever is just confusing.

OK

> permit the sender to unilaterally use more sophisticated algorithms.

Here's what the last paragraph says:

"Once an ACK has been received for a message, the next message can be
 sent, but the peer SHOULD ensure that there is at least 10 ms between
 sending any two messages.  The only time a value less than 10 ms can
 be used is when it is known that all nodes are on a network that can
 support retransmissions faster than 10 ms with no congestion issues."

What I understand from this text is that a message cannot be sent until the
previous one has been acknowledged.  If other algorithms can be used (e.g. one
that use the received field), then the text should say that the algorithm above
is the basic algorithm that must at least been implemented.

> down as a misfeature and leave as-is.

OK.  Perhaps a note somewhere about this would be useful for future developers.

> No, it's just to preserve uniform syntax for IANA-defined and

OK, but what happen if the values in the XML are different from what is in the
RFC describing the Kind (like it is currently in the example).  My suggestion
would be to simply say that the values in the XML files are ignored for
IANA-defined kinds.

[...]

> It's ridiculously overcomplicated for this application (and arguably for

OK.

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

---

From: Eric Rescorla <[email-masked]>
Date: Wed, 19 Jan 2011 08:57:19 +0200

> is the basic algorithm that must at least been implemented.

Sounds good. Willdo.

>> down as a misfeature and leave as-is.

Done.

> would be to simply say that the values in the XML files are ignored for

I've made this change, but I'd like to have other people weigh in if they don't like it.

-Ekr