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

Client store generalization #19420

Merged
merged 25 commits into from
Jan 6, 2023
Merged

Conversation

Joerger
Copy link
Contributor

@Joerger Joerger commented Dec 16, 2022

This PR generalizes client store logic to enable more operability between client backends (~/.tsh, identity file, in-memory, agent forwarded keys). This lays the groundwork for #19421.

  • Replace the old KeyStore with ClientStore, which is made up of a key store, trusted certs store, and profile store.
    • Add new KeyStore interface with an FS and in-memory implementation, based on original key store.
    • Add new TrustedCertsStore interface with an FS and in-memory implementation, based on original key store.
    • Add new ProfileStore interface with an FS and in-memory implementation.
    • Each individual store can have a different backend (FS or in-memory). This is useful, for example, with tsh --add-keys-to-agent=only, which keeps keys in memory while writing trusted certs and profiles to disk.
    • Identity files are now used to populate a full in-memory ClientStore on tsh start.
    • Remote agent forwarding sessions will also use a full in-memory ClientStore populated from forwarded keys.
  • Fix inconsistencies in Trusted Host certificate handling across different types of keys and key stores.
    • auth.TrustedCerts.ClusterName should never be empty so that it can actually be useful for logic.
    • auth.TrustedCerts.AuthorizedKeys, previously inaptly named HostCertificates, should always be in authorized_keys format rather than known_hosts format to maintain consistency across backends. We only convert to known_hosts when writing to ~/.tsh or identity file.

@Joerger Joerger force-pushed the joerger/client-store-fixes-improvements branch 4 times, most recently from 80e9d77 to fdb3d53 Compare December 21, 2022 03:39
api/utils/sshutils/ssh.go Outdated Show resolved Hide resolved
api/profile/profile.go Outdated Show resolved Hide resolved
api/utils/slices.go Outdated Show resolved Hide resolved
api/utils/sshutils/ssh_test.go Outdated Show resolved Hide resolved
api/utils/sshutils/ssh_test.go Outdated Show resolved Hide resolved
lib/client/trusted_certs_store.go Outdated Show resolved Hide resolved
lib/client/trusted_certs_store.go Outdated Show resolved Hide resolved
lib/client/trusted_certs_store.go Outdated Show resolved Hide resolved
lib/client/trusted_certs_store.go Outdated Show resolved Hide resolved
lib/client/trusted_certs_store_test.go Show resolved Hide resolved
- Generalize KeyStore logic to support non-disk stores in a streamlined
  way
  - Replace custom identity file logic with
    NewMemLocalKeyStoreFromIdentityFile
  - Add NonSessionKeyStore interface and an in-memory implementation
- Fix inconsistencies in Host CA handling across different types of keys
  and key stores. e.g. Missing Host certs, missing cluster name.
…ecker with key.CheckHostKey; cleanup and fix tests.
@Joerger Joerger force-pushed the joerger/client-store-fixes-improvements branch from c31fe63 to 152c499 Compare December 23, 2022 02:00
@Joerger Joerger marked this pull request as ready for review December 23, 2022 03:09
@github-actions github-actions bot added kubernetes-access machine-id size/lg tctl tctl - Teleport admin tool tsh tsh - Teleport's command line tool for logging into nodes running Teleport. labels Dec 23, 2022
Copy link
Contributor

@marcoandredinis marcoandredinis left a comment

Choose a reason for hiding this comment

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

Partial pass
Looking good 👍

Copy link
Contributor

@marcoandredinis marcoandredinis left a comment

Choose a reason for hiding this comment

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

Most changes look good 👍
There are a lot of changes that are just moving around methods/structs to different files

I wonder if grouping those changes together would make this PR easier to review

api/profile/profile.go Show resolved Hide resolved
api/utils/keypaths/keypaths.go Show resolved Hide resolved
lib/client/identityfile/identity_test.go Outdated Show resolved Hide resolved
lib/sshutils/marshal_test.go Show resolved Hide resolved
lib/sshutils/marshal.go Outdated Show resolved Hide resolved
lib/client/client_store.go Outdated Show resolved Hide resolved
lib/client/profile.go Outdated Show resolved Hide resolved
// Deduplicate deduplicates list of strings
func Deduplicate(in []string) []string {
// Deduplicate deduplicates list of comparable values.
func Deduplicate[T comparable](in []T) []T {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I'm not sure about the performance differences, but you could use DeduplicateAny() in the implementation here:

func Deduplicate[T comparable](in []T) []T {
	return DeduplicateAny(in, func(t T, t2 T) bool {
		return t == t2
	})
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Deduplicate[T comparable] has better performance since it can use a map[T]bool for O(1) existence checks, so I think it's better not to generalize all uses to the current DeduplicateAny with O(n) existence checks. Let me know if you disagree since the performance is pretty negligible to be honest.

I was considering changing DeduplicateAny to func[T any](in []T, hashFunc func(T) string) []T so Deduplicate could simply call that, but I don't think it's worth the extra complexity right now.

@@ -37,3 +38,19 @@ func TestDeduplicate(t *testing.T) {
})
}
}

func TestDeduplicateAny(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I'd be nice to also unit test the Deduplicate()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These tests already exist above here, no change was necessary for the generification.

api/utils/sshutils/ssh_test.go Outdated Show resolved Hide resolved
lib/client/interfaces.go Outdated Show resolved Hide resolved
@Joerger Joerger enabled auto-merge (squash) January 5, 2023 19:56
@Joerger Joerger merged commit 488af75 into master Jan 6, 2023
@Joerger Joerger deleted the joerger/client-store-fixes-improvements branch January 6, 2023 01:40
@ravicious
Copy link
Member

It looks like this change broke adding in a new cluster in Connect. clusters.Storage.addCluster calls client.Config.SaveProfile directly on client.Config instead of client.TeleportClient, without setting ClientStore first.

I fixed that in #20263 and added an integration test which would've caught this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kubernetes-access machine-id size/lg tctl tctl - Teleport admin tool tsh tsh - Teleport's command line tool for logging into nodes running Teleport.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants