Skip to main content

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 revision No specific revision (document currently at 12)
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
I-D 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
Request Early review on draft-irtf-pearg-numeric-ids-generation by Transport Area Review Team Assigned
Posted at https://mailarchive.ietf.org/arch/msg/tsv-art/LbTGcBstmlFxzSAQllQAzyQfpjo
Reviewed revision 02 (document currently at 12)
Result Ready w/issues
Completed 2020-08-07
review-irtf-pearg-numeric-ids-generation-02-tsvart-early-tuexen-2020-08-07-00
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))