Early Review of draft-irtf-pearg-numeric-ids-generation-02
review-irtf-pearg-numeric-ids-generation-02-tsvart-early-tuexen-2020-08-07-00

Request Review of draft-irtf-pearg-numeric-ids-generation
Requested rev. no specific revision (document currently at 03)
Type Early Review
Team Transport Area Review Team (tsvart)
Deadline 2020-08-07
Requested 2020-06-29
Requested by Magnus Westerlund
Authors Fernando Gont, Ivan Arce
Draft last updated 2020-08-07
Completed reviews Tsvart Early review of -02 by Michael Tüxen (diff)
Comments
Request from PEARG chairs for a transport review. Note the document discusses Ports and TCP initial sequence number so I think this makes sense.
Assignment Reviewer Michael Tüxen
State Completed
Review review-irtf-pearg-numeric-ids-generation-02-tsvart-early-tuexen-2020-08-07
Posted at https://mailarchive.ietf.org/arch/msg/tsv-art/LbTGcBstmlFxzSAQllQAzyQfpjo
Reviewed rev. 02 (document currently at 03)
Review result Ready with Issues
Review completed: 2020-08-07

Review
review-irtf-pearg-numeric-ids-generation-02-tsvart-early-tuexen-2020-08-07

The document is well written, provides algorithms which could be used to address identified problems. One  could add some text covering TCP timestamps.

Section 1 states:
"Recent history indicates that when new protocols are standardized or
new protocol implementations are produced, the security and privacy
properties of the associated identifiers tend to be overlooked,..."
How does this related to recent/current activities like SCTP/DTLS or QUIC?

The Algorithms in 7.1.1 and 7.1.2 can be substantially simplified when check_suitable_id() always returns true. Why are not the simplified algorithms shown?

The algorithm in 7.3 (and later) uses a function F and it is stated that F must be a cryptographically-secure hash function. Couldn't you also use something like SipHash.

When reading the algorithm in 7.4.1, I had the impression that it should also work with id_inc > 1 (by just changing that parameter). If that is true, I guess you would need to change "if (next_id == max_id)" to "if (max_id - next_id < id_inc) {" and also "next_id = min_id;" to "next_id = min_id + (id_inc - (max_id - next_id + 1));".  If you don't want to be that generic, you might want to remove id_inc and just use 1.

The algorithm in 7.4.1 also calls "store_counter(CONTEXT, next_id)" when returning ERROR. Why?

The algorithms given before 9.4 are given in C code. The Algorithm in 9.4 breaks with this rule. Is this intended?

Nits:
Page 10: if(check_suitable_id(next_id)) -> if (check_suitable_id(next_id))
Page 11: if(check_suitable_id(next_id)) -> if (check_suitable_id(next_id))
Page 12: if(check_suitable_id(next_id)) -> if (check_suitable_id(next_id))
Page 14: if(lookup_counter(CONTEXT) == ERROR){ -> if (lookup_counter(CONTEXT) == ERROR) {
Page 14: if (check_suitable_id(next_id)){ -> if (check_suitable_id(next_id)) {
Page 14: }\n else { -> } else {
Page 16: if(check_suitable_id(next_id)) -> if (check_suitable_id(next_id))
Page 18: if(check_suitable_id(next_id)) -> if (check_suitable_id(next_id))
Page 24: linear(CONTEXT_2)= linear(CONTEXT_2) + increment(); -> linear(CONTEXT_2) = linear(CONTEXT_2) + increment();
Page 24: if(check_suitable_id(next_id)) -> if (check_suitable_id(next_id))