theckman commented 5 years ago

This change removes our support for generating V2 UUIDs from the package, since it seems that the current implementation is not up to spec in relation to the DCE 1.1 Specification[1][2].

In our implementation the data written in to the V1, V3-V5 UUID uses Big Endian byte order as specified in RFC-4122[3]. However, when looking at the DCE specification they make it read like the data should be written in not Little Endian but a format where you completely reverse the bites of the value from most significant first to least significant first:

Set the time_low field equal to the least significant 32-bits (bits numbered 0 to 31 inclusive) of the time stamp in the same order of significance. If a DCE Security version UUID is being created, then replace the time_low field with the local user security attribute as defined by the DCE: Security Services specification.

There are more things later confirming the order:

Set the time_mid field equal to the bits numbered 32 to 47 inclusive of the time stamp in the same order of significance.

This timestamp handling instructions seem dangerous because the specification is telling us to overwrite the lest-significant bits of the UUID timestamp value with a deterministic value, meaning the remaining bits of the timestamp will now change much less frequently. Because the rest of the UUID consists of distinct / deterministic values, this will make the UUID much less unique.

This also means that for the DCE Security UUIDs we would need completely different generating and parsing code than V1 and V3-V5 UUIDs. Considering that it seems like there would be quite a bit of work to support them correctly.

To confirm that my understanding of the spec is correct, I tried to look at preexisting implementations to validate my understanding. Unfortunately, I've not had luck finding a UUID library that implements V2 UUIDs. That points to there being little to no value in continuing to try and support them, as there is not widespread availability of complementing implementations.

This change will require a major version bump.

[1] http://pubs.opengroup.org/onlinepubs/9629399/apdxa.htm#tagcjh_20_02_05 [2] http://pubs.opengroup.org/onlinepubs/9668899/chap5.htm#tagcjh_08_02_01_01 [3] https://tools.ietf.org/html/rfc4122#section-4.1.2

Signed-off-by: Tim Heckman t@heckman.io

codecov-io commented 5 years ago

Codecov Report

Merging #71 into master will decrease coverage by 0.04%. The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #71      +/-   ##
=======================================
- Coverage   99.05%   99%   -0.05%     
=======================================
  Files           4     4              
  Lines         318   303      -15     
=======================================
- Hits          315   300      -15     
  Misses          2     2              
  Partials        1     1

Impacted Files	Coverage Δ
uuid.go	`100% <ø> (ø)`	:arrow_up:
generator.go	`96.77% <ø> (-0.45%)`	:arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 2593f3d...6486973. Read the comment docs.

adamdecaf commented 5 years ago

There look to be some callers, but it sounds like they're broken anyway?

https://github.com/search?q=language%3Ago+%22uuid.NewV2%22&type=Code

gofrs / uuid

Remove support for DCE Security UUID (V2) generation #71

Codecov Report