Update backend.Key to be its own distinct type #46701

rosstimothy · 2024-09-17T23:12:37Z

Converts the backend.Key from a slice of bytes to a concrete struct. The main motivation behind this change is to be able to better distinguish individual components of a key. The textual representation of a backend key is constructed by joining all components of the key with a /. By representing the entire key as a single textual object it prevented resources containing a / in their name from being properly identified. There was a lot of code that interpolated keys in various manners and expected only a specific number of subcomponents for a particular resource. This lead to most, if not all, of the RPCs used to create a resource to permit a name containing /, (i.e. a user named test/llama/1), but any RPCs used to retrieve the resources would fail to retrieve them from the backend.

Every backend.Key now carries a slice of all the individual components in addition to the textual representation. This permits more fine grained inspection per component, while also not having to construct the textual representation for a group of components more than once.

The backend.Sanitizer was updated to only validate keys for backend writes. Retrieving and deleting invalid of malformed keys is however permitted. This will allow any existing resources that were created with / in one of the components to become visible in the UI or tctl get and deletable. However, any new resources with / in there name are explicitly prevented.

Closes #6088
Closes #9107
Closes #10576
Closes #42823

lib/backend/key.go

rosstimothy · 2024-09-30T16:00:20Z

Friendly ping @fspmarshall @espadolini

lib/backend/key.go

espadolini · 2024-09-30T16:29:55Z

lib/backend/key.go

+		}
+	}
+
+	return Key{components: keyComponents, s: strings.Join(append([]string{internalPrefix}, keyComponents...), SeparatorString)}


Not necessarily something we have to deal with now, but KeyFromString has the nice property of having the components be subslices of the full string, we should consider having that be part of the behavior of Key to reduce fragmentation and avoid the double memory consumption.

lib/backend/key.go

espadolini · 2024-09-30T16:35:43Z

lib/backend/key.go

@@ -102,9 +159,16 @@ func (k Key) Compare(o Key) int {
 func (k *Key) Scan(scan any) error {


Is this just for litebk? I think there's one use of this in pgbk that we could replace with a KeyFromString instead.

lite, pgbk, and crdb use this

lib/services/local/trust_test.go

lib/backend/key.go

fspmarshall

LGTM once existing feedback is addressed.

lib/backend/key.go

Converts the backend.Key from a slice of bytes to a concrete struct. The main motivation behind this change is to be able to better distinguish individual components of a key. The textual representation of a backend key is constructed by joining all components of the key with a /. By representing the entire key as a single textual object it prevented resources containing a / in their name from being properly identified. There was a lot of code that interpolated keys in various manners and expected only a specific number of subcomponents for a particular resource. This lead to most, if not all, of the RPCs used to create a resource to permit a name containing /, (i.e. a user named test/llama/1), but any RPCs used to retrieve the resources would fail to retrieve them from the backend. Every backend.Key now carries a slice of all the individual components in addition to the textual representation. This permits more fine grained inspection per component, while also not having to construct the textual representation for a group of components more than once. The backend.Sanitizer was updated to only validate keys for backend writes. Retrieving and deleting invalid of malformed keys is now permitted. This will allow any existing resources that were created with / in one of the components to become retrievable and deletable. However, any new resources with / in their name are explicitly prevented. Closes #6088 Closes #9107 Closes #10576 Closes #42823

#46701 incorrectly changed how the prefixes for the etcd and dynamo backends were applied and removed. In addition, the backend lock key was not being constructed correctly and caused aqcuisition of locks to fail on a cluster that had been upgraded. Tests have been added to ensure that keys are now being constructed correctly in all cases. Additionally, backend/helpers.go was moved to backed/lock.go to better described what functionality the file contains. Backends with prefixes (etcd, dynamo) were incorrectly constructing the key for locks. In addition the prefixes were not being trimmed correctly which broke CAS operations.

#46701 incorrectly changed how the prefixes for the etcd and dynamo backends were applied and removed. In addition, the backend lock key was not being constructed correctly and caused acquisition of locks to fail on a cluster that had been upgraded. Tests have been added to ensure that keys are now being constructed correctly in all cases. Additionally, backend/helpers.go was moved to backed/lock.go to better described what functionality the file contains. Backends with prefixes (etcd, dynamo) were incorrectly constructing the key for locks. In addition the prefixes were not being trimmed correctly which broke CAS operations.

rosstimothy force-pushed the tross/backend_key_type branch 3 times, most recently from 2a6890d to 1709cc5 Compare September 19, 2024 17:02

rosstimothy commented Sep 19, 2024

View reviewed changes

lib/backend/key.go Show resolved Hide resolved

rosstimothy requested review from espadolini and fspmarshall September 19, 2024 17:35

rosstimothy marked this pull request as ready for review September 19, 2024 17:35

github-actions bot requested review from greedy52, smallinsky and tcsc September 19, 2024 17:36

github-actions bot added database-access Database access related issues and PRs size/md labels Sep 19, 2024

rosstimothy added the no-changelog Indicates that a PR does not require a changelog entry label Sep 19, 2024

gravitational deleted a comment from github-actions bot Sep 19, 2024

espadolini reviewed Sep 30, 2024

View reviewed changes

fspmarshall approved these changes Sep 30, 2024

View reviewed changes

lib/backend/key.go Show resolved Hide resolved

rosstimothy force-pushed the tross/backend_key_type branch from 1709cc5 to ab2a80b Compare October 1, 2024 20:54

espadolini approved these changes Oct 1, 2024

View reviewed changes

public-teleport-github-review-bot bot removed request for tcsc, greedy52 and smallinsky October 1, 2024 21:41

rosstimothy force-pushed the tross/backend_key_type branch from 44c9e8b to 155e4da Compare October 2, 2024 21:12

rosstimothy requested review from espadolini and fspmarshall October 2, 2024 21:22

espadolini approved these changes Oct 3, 2024

View reviewed changes

fspmarshall approved these changes Oct 3, 2024

View reviewed changes

rosstimothy force-pushed the tross/backend_key_type branch from b25008a to 4bcabfb Compare October 4, 2024 13:45

rosstimothy enabled auto-merge October 4, 2024 14:01

rosstimothy added this pull request to the merge queue Oct 4, 2024

Merged via the queue into master with commit 0486c09 Oct 4, 2024
39 checks passed

rosstimothy deleted the tross/backend_key_type branch October 4, 2024 14:19

rosstimothy mentioned this pull request Oct 7, 2024

Fix backends with prefixes #47238

Merged

zmb3 mentioned this pull request Oct 23, 2024

Teleport Auth Service allows names to have a slash / but tctl cannot delete them #47838

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update backend.Key to be its own distinct type #46701

Update backend.Key to be its own distinct type #46701

rosstimothy commented Sep 17, 2024 •

edited

Loading

rosstimothy commented Sep 30, 2024

espadolini Sep 30, 2024

espadolini Sep 30, 2024

rosstimothy Oct 1, 2024

fspmarshall left a comment

		@@ -102,9 +159,16 @@ func (k Key) Compare(o Key) int {
		func (k *Key) Scan(scan any) error {

Update backend.Key to be its own distinct type #46701

Update backend.Key to be its own distinct type #46701

Conversation

rosstimothy commented Sep 17, 2024 • edited Loading

rosstimothy commented Sep 30, 2024

espadolini Sep 30, 2024

Choose a reason for hiding this comment

espadolini Sep 30, 2024

Choose a reason for hiding this comment

rosstimothy Oct 1, 2024

Choose a reason for hiding this comment

fspmarshall left a comment

Choose a reason for hiding this comment

rosstimothy commented Sep 17, 2024 •

edited

Loading