ietf-wg-uuidrev / rfc4122bis

revision to RFC4122
Other
57 stars 11 forks source link

INDIR Review 2 #140

Closed kyzer-davis closed 1 year ago

kyzer-davis commented 1 year ago
I have the following DISCUSS level issues:

Section 2.1, page 5: I think a MAC address identifies a network interface, not a "machine". Also, for MAC addresses, this document should Reference RFC 7042 or, even better, draft-ietf-intarea-rfc7042bis (which is through WGLC and, in my opinion, ready for publication request). Those documents point out the existence of 64-bit MAC addresses and where they have been and are currently used so this document should say something about how those are handled (perhaps by discarding the top 16 bits).
OLD
       address in the node field of UUID version 1.  Exposed MAC
       addresses can be used as an attack surface to locate machines and
       reveal various other information about such machines (minimally
NEW
       address in the node field of UUID version 1.  Exposed MAC
       addresses can be used as an attack surface to locate network interfaces
       and reveal various other information about such interfaces (minimally

Section 6.1, Page 27, 1st paragraph: The last sentence of this paragraph is "The count will range between zero and the number of 100-nanosecond intervals per system time interval." This is confusing. Why couldn't the count exceed that number?

Section 7: I think it is improper in the IANA Considerations to talk about what "the authors and the working group have concluded". The RFC this document will be is issued as the consensus of the IETF, not as the opinion of its authors or even the working group. I suggest replacing this section in its entirety with the following:
"This document requires no IANA actions. In particular, no update is required to the IANA URN namespace registration [URNNamespaces] for UUID filed in [RFC4122] and IANA is not required to track UUIDs used for identifying items such as versions, variants, namespaces, or hashspaces."

The following are other issues I found with this document that SHOULD be corrected before publication:
Section 5.5, page 20: "This SHA-1 value is then used to populate all 128 bits of the UUID layout.  Excess bits beyond 128 are discarded." Seems like you can and should be more precise. Maybe "The high order 128 bits of the SHA-1 value is then used to populate all 128 bits of the UUID layout and the remaining 32 low order bits of SHA-1 output are discarded"

Section 6.2, page 29: "Using floating point math," appears to mandate the use of floating point which might not be available on some small platforms. Suggest "Using floating point or scaled integer arithmetic,"

Section 6.2, Page 30, paragraph headed "Fixed-Length Dedicated Counter Seeding":
+    "However, when the timestamp has not incremented, the counter is frozen and
      incremented via the desired increment logic." To my mind, frozen means it is not changed and is inconsistent with incrementing it. Perhaps "When the timestamp has not increased, the counter is instead incremented by the desired increment logic."
+    "For example, a 24 bit counter could have
      the 23 bits in least-significant, right-most, position randomly
      initialized.  The remaining most significant, left-most counter
      bits are initialized as zero for the sole purpose of guarding
      against counter rollovers." This is inconsistent. Either it should say "the 23 or fewer bits" in the first sentence or it should say "bit is initialized to zero" in the second sentence.

Section 6.3, page 32, 3rd paragraph: Frequent generation of random numbers also puts more stress on any entropy source and or entropy pool being used as the basis for such random numbers. This should be mentioned.

Section 6.9: The discussion of the local/global bit here would be a good place to reference RFC 7042 or, even better, draft-ietf-intarea-ref4042bis.

Section 6.10: This section uses "quite large" without any indication of how large. A factor of ten? A factor of ten to the tenth power? Can some hint of how large be given? Similarly Section 2.1, item 1.

Section 8, Page 39: I think "transposed" is the wrong word. That just means to swap around various parts of something. While that is one type of modification, I think you mean any type of slight modification. "slightly transposed" -> "slightly modified"

The following are minor issues (typos, misspelling, minor text improvements) with the document:

Section 1, paragraph 3, page 3:
OLD
to implement services using UUIDs, UUIDs in combination with URNs [RFC8141], or otherwise.
NEW
to implement services using UUIDs either in combination with URNs [RFC8141] or otherwise.

Section 3.2: The expansions of UUIDv1 through UUIDv8 are quite repetitive and uninformative. Since you have already expanded UUID in the previous line, I don't think it needs to be expanded here. I suggest:
   UUIDv1    Gregorian time-based UUID
   UUIDv2    DCE Security version UUID
   UUIDv3    Name-based UUID with MD5
   UUIDv4    Pseudo-random UUID
   UUIDv5    Name-based UUID with SHA-1
   UUIDv6    Re-ordered Gregorian time-based UUID
   UUIDv7    Unix Epoch time-based UUID
   UUIDv8    Custom UUID

I think Section 6.2 is too long and should have subsections (6.2.1, 6.2.2, ...).

Section 6.2, page 29: "which is sorts" -> "which sorts"

Section 6.2, page 30: "randomly initialize a portion counter" -> "randomly initialize a portion of the counter"

Section 6.2., page 31: "the random being incremented." -> "the random value being incremented."

Section 6.5, last paragraph, page 35: "needs not be the only" -> "need not be the only"
kyzer-davis commented 1 year ago

Will end up in Draft 12, no time to get this done by draft 11 deadline.

tgross35 commented 1 year ago

Do you know what the rough outline or timeline is for the proposal at this point? I assume there will be at least one more draft to address all review concerns, but I am having trouble figuring out what happens after that point.

Also confusing is the recent milestone change, since that milestone is set for March 2023 https://mailarchive.ietf.org/arch/msg/uuidrev/Wt03UVzzrxqmHJT5DeM58UT0Hww/

jimfenton commented 1 year ago

@tgross35 This sort of question is really better suited for the mailing list, not the GitHub repository.

The change to the milestones was simply that the document the milestone referred to was left out. I added the reference to the document.

@mcr has sent out a poll to try to have an interim meeting of the working group. The timeframe will depend greatly on WG rough consensus on how much change will be done at this very late stage. Please try to attend if you have an opinion on any of this.

-Jim, co-chair

kyzer-davis commented 1 year ago

Changes completed in https://github.com/ietf-wg-uuidrev/rfc4122bis/commit/cfcb3b077b96fc6bb460579db368c48d50bea790 All are done except:

tgross35 commented 1 year ago

Thanks @jimfenton, I honestly did not know that the mailing list was open to nontechnical discussion. I appreciate the clarification.

bradleypeabody commented 1 year ago

@kyzer-davis I think it's safe to replace "quite large" with "one order of magnitude or more". I.e.:

The real-world differences in this approach of index locality vs random data inserts can be one order of magnitude or more.