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

[Michael Tuexen <[e-mail]>]

> On 27. Aug 2020, at 14:10, Fernando Gont <[e-mail]> wrote:
> 
> Hi, Michael,
> 
> Thanks a lot for your feedback! In-line...
Hi Fernando,

sorry for the late response. See comments in-line.

Best regards
Michael
> 
> 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?
For example.
> 
> 
>> 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.
My point is that SCTP/DTLS or QUIC are transport layers which use encryption.
Doesn't this affect the statement:
the security and privacy
properties of the associated identifiers tend to be overlooked

At least for the work on QUIC this doesn't seem to apply. Or am I missing something?
> 
> 
> 
> 
> 
>> 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?
Yes.
> 
> 
> (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.
Understood. But the document says that

in many (if
   not most) cases, the algorithm will not need to check the suitability
   of a selected identifier (i.e., check_suitable_id() would always be
   "true").

So why not show the simplified version, if it is used in 'most cases'.
> 
> 
> 
>> 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/
Is the commit supporting '*has* been employed' or 'are considered too weak'?
> 
> 
> 
> 
>> 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?
Sounds good to me.

Best regards
Michael
> 
> 
> 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
> 
> 
> 
>