Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from Dec 30, 2020
Merged

Conversation

theckman
Copy link
Member

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

@theckman theckman requested review from acln0 and niaow February 15, 2019 18:43
@codecov-io
Copy link

codecov-io commented Feb 15, 2019

Codecov Report

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

Impacted file tree graph

@@           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% <ø> (ø) ⬆️
generator.go 96.77% <ø> (-0.45%) ⬇️

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
Copy link
Member

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

Copy link
Member

@niaow niaow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably remove the info about v2 UUIDs at the top of uuid.go.

Other than these issues, it seems good.

uuid.go Show resolved Hide resolved
uuid.go Outdated Show resolved Hide resolved
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>
Copy link
Member

@niaow niaow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@cameracker cameracker merged commit 4b36aa0 into master Dec 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants