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

[Fernando Gont <[e-mail]>]

Hi, Michael,

FWIW, we have posted a rev that hopefully addresses your feedback.

The latest rev is at: 
https://tools.ietf.org/html/draft-irtf-pearg-numeric-ids-generation-03

And the diff from the previous version here: 
https://tools.ietf.org/rfcdiff?url2=draft-irtf-pearg-numeric-ids-generation-03.txt
https://tools.ietf.org/rfcdiff?url2=draft-irtf-pearg-numeric-ids-generation-03.txt

Please do let us know if you think there are ny further changes that 
should be applied.

Thanks a lot!

Regards,
Fernando




On 27/8/20 09:10, Fernando Gont wrote:
> 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