Re: [Pearg] Tsvart early review of draft-irtf-pearg-numeric-ids-generation-02

[Fernando Gont <[e-mail]>]

Hi, Michael,

Thanks a lot for your feedback! In-line...

On 7/8/20 09:22, Michael Tüxen via Datatracker wrote:
> Reviewer: Michael Tüxen
> Review result: Ready with Issues
> 
> The document is well written, provides algorithms which could be used to
> address identified problems. One  could add some text covering TCP timestamps.

You mean e.g. to spell out which of the proposed algorithms one might 
use for 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?

SCTP (RFC4960) is similar to TCP, in this respect. OTOH, I have only 
skimmed through the DTLS (RFC6347), and it seems that it initially sets 
sequence numbers to 0. -- while these are meant to be protected, I'm 
curious if they could have done with monotonically increasing sequence 
numbers ala 6528, or with a random origin.





> 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?

Are you referring to the case where an implementation need not check the 
generated ID against other existing I-Ds?


(FWIW, in order for check_suitable_id()to always return "true", it means 
that there are essentially no requirements on the ID -- just pick any 
random number... at which point, there's not really much of an algorithm 
( Section 7.1.1 and Section 7.1.2 cover "recovery" strategies for the 
cases where the algorithm finds collisions.



> 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.

Siphash *has* been employed for generating IDs.. although it seems that 
the 64-bits versions are considered too weak. see e.g. 
https://lore.kernel.org/patchwork/patch/1083206/



> 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.

You raise a very good point. I'm curious whether it might make sense to 
include the most generic version (as you suggest), and then also show 
the simplified version when id_inc=1  ? Thoughts?


Also, we may want to note that, while using increments of "1" reduces 
the reuse frequency, in principle id_inc does not even need to be 
constant... -- in this case, since the failure severity is "hard 
errors", one probably ones to keep id_inc small, to reduce the ID resuse 
frequency.



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

At least with the current increment values, if the algorithm iterates 
over count = max_id - min_id + 1; values (i.e., between the range min_id 
and max_id, the value to be stored would be the same one as originally 
found, so it's actually a no-op. I will remove it.


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

Are you referring to this line:
            linear(CONTEXT_2)= linear(CONTEXT_2) + increment();
?

If so, I'll try to find a replacements that's C-compatible and that's 
still clear...

Thanks!

Regards,
-- 
Fernando Gont
SI6 Networks
e-mail: [e-mail]
PGP Fingerprint: 6666 31C6 D484 63B2 8FB1 E3C4 AE25 0D55 1D4E 7492