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

use uuid.NewSHA1 package to generate uuids #997

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

tariq1890
Copy link
Contributor

@tariq1890 tariq1890 commented Oct 17, 2024

Here is a post that goes into detail on the security limitations of the math/rand package.

Since the IMEX changes haven't publicised yet, I thought this would be a safe starting point to implement this change

@tariq1890
Copy link
Contributor Author

It looks like CodeQL runs on an earlier version of Go

Copy link
Collaborator

@ArangoGutierrez ArangoGutierrez left a comment

Choose a reason for hiding this comment

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

lgtm
@elezar ?

@klueska
Copy link
Contributor

klueska commented Oct 17, 2024

Have you tested to ensure that the generated UUID is the same for a constant seed value?

@tariq1890
Copy link
Contributor Author

I haven't had the chance to test this on a running k8s cluster. I'll keep you posted on that.

However, I tried this sample go programme and I was able to confirm that UUID was generated deterministically for a given seed

func main() {

	ips := []string{
		"10.130.3.24",
		"10.130.3.53",
		"10.130.3.23",
		"10.130.3.31",
		"10.130.3.27",
		"10.130.3.25",
	}
	sort.Strings(ips)

	seed := strings.Join(ips, "\n")
	r := rand.NewChaCha8(sha256.Sum256([]byte(seed)))
	charset := make([]byte, 16)
	_, err := r.Read(charset)
	if err != nil {
		fmt.Printf("Error generating random number: %v", err)
	}
	u, _ := uuid.FromBytes(charset)

	fmt.Printf("Output: %s\n", u.String())
}

@elezar
Copy link
Member

elezar commented Oct 18, 2024

We are not using these random numbers to generate a cryptographically secure random number -- we just want a unique idenfiier for a set of input IPs. I'm happy to make this change, but let's add tests that capture the behaviour since we can't have IMEX domain IDs changing across device plugin updates.

@elezar
Copy link
Member

elezar commented Oct 18, 2024

Given the following post: https://stackoverflow.com/questions/77925673/generate-uuid-based-on-a-namespace-string-in-go

Would switching to using:

uuid.NewSHA1(namespace, content)

https://pkg.go.dev/github.com/google/uuid#NewSHA1

or https://pkg.go.dev/github.com/google/uuid#NewMD5

where namespace would be a cluster-wide constant and content is the content of the sorted IMEX config file.

Copy link
Member

@elezar elezar left a comment

Choose a reason for hiding this comment

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

If we're going to make a change such as this we should use the uuid.NewSHA1 or uuid.NewMD5 functions instead of rolling our own.

This was called out in the review of the original code in #965

internal/lm/fabric.go Outdated Show resolved Hide resolved
@tariq1890 tariq1890 force-pushed the math-rand-v2 branch 2 times, most recently from 88712a2 to 69e322a Compare October 18, 2024 16:25
@tariq1890 tariq1890 changed the title use math/rand/v2 package to generate uuids use uuid.NewSHA1 package to generate uuids Oct 18, 2024
@@ -32,7 +32,7 @@ func TestGerenerateDomainUUID(t *testing.T) {
{
description: "single IP",
ips: []string{"10.130.3.24"},
expected: "4dbd3d31-fbb3-8a40-33bb-bcc0dd7b68b8",
expected: "60ad7226-0130-54d0-b762-2a5385a3a26f",
Copy link
Member

Choose a reason for hiding this comment

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

@klueska @ArangoGutierrez are we OK with this "breaking" change given that we haven't released this yet.

I think the simplifications in this PR are worth it.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've rebased the PR. Requesting another round of review

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.

4 participants