security: Add version number to base64 form of join tokens

[nikola-n6c]

Add version number to base64 form of join tokens, to future proof the encoding and prevent command line issues when base64 encoded token starts with -.
Add unit test for version (un)marshaling.
Add error types to distinguish between version related error and other (i.e. encoding) errors in tests.

Release note: None.

[cockroach-teamcity]

All committers have signed the CLA.

[blathers-crl]

Thank you for contributing to CockroachDB. Please ensure you have followed the guidelines for creating a PR.

Before a member of our team reviews your PR, I have some potential action items for you:

• Please ensure your git commit message contains a release note.
• When CI has completed, please ensure no errors have appeared.

I was unable to automatically find a reviewer. You can try CCing one of the following members:

• A person you worked with closely on this PR.
• The person who created the ticket, or a CRDB organization member involved with the ticket (author, commenter, etc.).
• Join our community slack channel and ask on #contributors.
• Try find someone else from here.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

[cockroach-teamcity]

This change is 

[nikola-n6c]

Issue: #64885

[nikola-n6c]

Adding @itsbilal (author of the issue)

[nikola-n6c]

Adding @knz (owner of #60632)

[blathers-crl]

Thank you for updating your pull request.

Before a member of our team reviews your PR, I have some potential action items for you:

• We notice you have more than one commit in your PR. We try break logical changes into separate commits, but commits such as "fix typo" or "address review commits" should be squashed into one commit and pushed with --force
• Please ensure your git commit message contains a release note.
• When CI has completed, please ensure no errors have appeared.

I was unable to automatically find a reviewer. You can try CCing one of the following members:

• A person you worked with closely on this PR.
• The person who created the ticket, or a CRDB organization member involved with the ticket (author, commenter, etc.).
• Join our community slack channel and ask on #contributors.
• Try find someone else from here.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

[knz]

oh nice, thank you for this!
we're going to queue this up for review.

[blathers-crl]

Thank you for updating your pull request.

Before a member of our team reviews your PR, I have some potential action items for you:

• Please ensure your git commit message contains a release note.
• When CI has completed, please ensure no errors have appeared.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

[itsbilal]

Hey, thank you so much @nikola-n6c for working away on this! This mostly looks good, I've left some comments that you can get to in the meantime.

Also it'd be great if you could update the PR description and commit message to have a Release note: None. line at the end.

Reviewed 3 of 3 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @nikola-n6c)

---

pkg/security/join_token.go, line 33 at r1 (raw file):

// JoinTokenVersion is used to version the encoding of join tokens, and is the
// first byte in the marshaled token.
type JoinTokenVersion byte

The type doesn't need to be exported, so it could just start with a lowercase letter.

---

pkg/security/join_token.go, line 39 at r1 (raw file):

	// The format of the text is:
	// <version:1>base64(<TokenID:uuid.Size><SharedSecret:joinTokenSecretLen><fingerprint:variable><crc:4>)
	V0 JoinTokenVersion = '0'

Also lowercase as it's unexported, plus the name could be joinTokenV0 to be more descriptive

---

pkg/security/join_token.go, line 44 at r1 (raw file):

// joinTokenVersionError is used when unsupported version is found while
// (un)marshaling join tokens.
type joinTokenVersionError struct {

This error may not be necessary, however instead of this you could add a:

var invalidJoinTokenError = errors.New("invalid join token")

and use invalidJoinTokenError for all instances of errors.New("invalid join token") below, as well as for the version error.

---

pkg/security/join_token.go, line 67 at r1 (raw file):

	SharedSecret []byte
	fingerprint  []byte
	version      JoinTokenVersion

I don't think storing the version in the token itself is necessary - the marshaller could (currently) always marshal it as V0, and the unmarshaller could just have a switch based on it.

---

pkg/security/join_token.go, line 79 at r1 (raw file):

//
// The format of the text (after base64-decoding) is:
// <TokenID:uuid.Size><SharedSecret:joinTokenSecretLen><fingerprint:variable><crc:4>

I think it's a good idea to still keep the format here.

---

pkg/security/join_token.go, line 168 at r1 (raw file):

	// Prefix the encoded token with version number.
	version := []byte{byte(j.version)}

It's slightly more efficient to do:

version := make([]byte, 0, len(b.Bytes()) + 1)
version = append(version, j.version)
version = append(version, b.Bytes()...)
return version, nil

As it avoids a reallocation.

---

pkg/security/join_token_test.go, line 75 at r1 (raw file):

	require.True(t, token.VerifySignature(cm.CACert().FileContents))

	t.Run("Supported", func(t *testing.T) {

Test looks great! You could change the test name to lowercase as per convention (supported, unsupported_marshal etc)

[blathers-crl]

Thank you for updating your pull request.

My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

[nikola-n6c]

Thanks @itsbilal, all suggestions made sense.

Also it'd be great if you could update the PR description and commit message to have a Release note: None. line at the end.

Done!

[nikola-n6c]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @itsbilal)

---

pkg/security/join_token.go, line 33 at r1 (raw file):

Previously, itsbilal (Bilal Akhtar) wrote…

The type doesn't need to be exported, so it could just start with a lowercase letter.

Done.

---

pkg/security/join_token.go, line 39 at r1 (raw file):

Previously, itsbilal (Bilal Akhtar) wrote…

Also lowercase as it's unexported, plus the name could be joinTokenV0 to be more descriptive

Done.

---

pkg/security/join_token.go, line 44 at r1 (raw file):

Previously, itsbilal (Bilal Akhtar) wrote…

This error may not be necessary, however instead of this you could add a:

var invalidJoinTokenError = errors.New("invalid join token")

and use invalidJoinTokenError for all instances of errors.New("invalid join token") below, as well as for the version error.

Done.

---

pkg/security/join_token.go, line 67 at r1 (raw file):

Previously, itsbilal (Bilal Akhtar) wrote…

I don't think storing the version in the token itself is necessary - the marshaller could (currently) always marshal it as V0, and the unmarshaller could just have a switch based on it.

Done.

---

pkg/security/join_token.go, line 79 at r1 (raw file):

Previously, itsbilal (Bilal Akhtar) wrote…

I think it's a good idea to still keep the format here.

Done.

---

pkg/security/join_token.go, line 168 at r1 (raw file):

Previously, itsbilal (Bilal Akhtar) wrote…

It's slightly more efficient to do:

version := make([]byte, 0, len(b.Bytes()) + 1)
version = append(version, j.version)
version = append(version, b.Bytes()...)
return version, nil

As it avoids a reallocation.

Done.

---

pkg/security/join_token_test.go, line 75 at r1 (raw file):

Previously, itsbilal (Bilal Akhtar) wrote…

Test looks great! You could change the test name to lowercase as per convention (supported, unsupported_marshal etc)

Done.

[itsbilal]

this looks great, thanks! I have one last suggestion about the test, but it's a relatively minor one.

Also, while you finish that up, can you sign the CLA as the comment at the top mentions? That's necessary before we can merge this. Thanks again!

Reviewed 2 of 2 files at r2.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @nikola-n6c)

---

pkg/security/join_token_test.go, line 86 at r2 (raw file):

		// Set first byte to unsupported version. Other parts of the token are
		// omitted for simplicity.
		tokenBytes := []byte{'x'}

It would be a better test to take the token.MarshalText() output from the valid token above, then change b[0] = 'x' and then try Unmarshaling that. That'll ensure you're testing the version mismatch case.

Also nit: we prefer to do var j2 JoinToken (or if you want the pointer type, j2 := &JoinToken{}) instead of new(JoinToken) by convention.

[nikola-n6c]

this looks great, thanks! I have one last suggestion about the test, but it's a relatively minor one.

Also, while you finish that up, can you sign the CLA as the comment at the top mentions? That's necessary before we can merge this. Thanks again!

Reviewed 2 of 2 files at r2.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @nikola-n6c)

pkg/security/join_token_test.go, line 86 at r2 (raw file):

		// Set first byte to unsupported version. Other parts of the token are
		// omitted for simplicity.
		tokenBytes := []byte{'x'}

It would be a better test to take the token.MarshalText() output from the valid token above, then change b[0] = 'x' and then try Unmarshaling that. That'll ensure you're testing the version mismatch case.

Also nit: we prefer to do var j2 JoinToken (or if you want the pointer type, j2 := &JoinToken{}) instead of new(JoinToken) by convention.

Thanks, agreed on the test improvement, and applying the convention nit.

[itsbilal]

Looks great. thanks for addressing this!

Can you sign the CLA so I can merge this? You can do it here: https://cla.crdb.dev/cockroachdb/cockroach?pullRequest=65329

Reviewed 1 of 1 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @nikola-n6c)

[nikola-n6c]

Looks great. thanks for addressing this!

Can you sign the CLA so I can merge this? You can do it here: https://cla.crdb.dev/cockroachdb/cockroach?pullRequest=65329

Reviewed 1 of 1 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @nikola-n6c)

Thanks, it took forever to make sure this is ok with policies at work. Signed the CLA.

[knz]

thank you!

bors r=itsbilal

[craig]

Build succeeded:

• GitHub CI (Cockroach)